let's not complain about harmless patch-apply failures

On 16 January 2018 at 16:56, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Jan 16, 2018 at 4:04 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

At Mon, 15 Jan 2018 21:45:34 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <26718.1516070734@sss.pgh.pa.us>

Robert Haas <robertmhaas@gmail.com> writes:

Since the "Stripping trailing CRs from patch" message is totally
harmless, I'm not sure why you should need to devote any effort to
avoiding it. Anyone who gets it should just ignore it.

I know that and totally agree to Robert but still I wonder why
(and am annoyed by) I sometimes receive such complain or even an
accusation that I sent an out-of-the-convention patch and I was
afraid that it is not actually common.

I've seen that before as well.

I have also noticed people complaining

People complain... asking them not to is unlikely to get anywhere.

We must encourage people to speak up if they see an improvement or a
lack of quality. I have benefited from such comments and they are not
often intended negatively.

Every complaint is not a hard blocker and complainers can also be
wrong, so we just need perspective.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 16, 2018 at 12:10 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

People complain... asking them not to is unlikely to get anywhere.

It doesn't hurt to ask.

We must encourage people to speak up if they see an improvement or a
lack of quality. I have benefited from such comments and they are not
often intended negatively.

Every complaint is not a hard blocker and complainers can also be
wrong, so we just need perspective.

I'm not disagreeing with any of that. However, sending a patch with
CRLF line endings, or one that applies with minor offsets, is not a
lack of quality. Complaining about it serves no purpose.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 16, 2018 at 11:56:55AM -0500, Robert Haas wrote:

On Tue, Jan 16, 2018 at 4:04 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

At Mon, 15 Jan 2018 21:45:34 -0500, Tom Lane <tgl@sss.pgh.pa.us> wrote in <26718.1516070734@sss.pgh.pa.us>

Robert Haas <robertmhaas@gmail.com> writes:

Since the "Stripping trailing CRs from patch" message is
totally harmless, I'm not sure why you should need to devote
any effort to avoiding it. Anyone who gets it should just
ignore it.

I know that and totally agree to Robert but still I wonder why
(and am annoyed by) I sometimes receive such complain or even an
accusation that I sent an out-of-the-convention patch and I was
afraid that it is not actually common.

I've seen that before as well.

I have also noticed people complaining about patches that apply
"with offsets", which also seems like needless nitpicking. If the
offsets are large and the patch has been sitting around for a long
time, there's a small chance it could be applying to the wrong
place, but that is extremely rare. Most patches have small offsets,
just a few lines, and there is no problem. Complaining about the
offsets, on the other hand, is unhelpful: it not only forces the
patch author to update the patch for no good reason, but it clutters
the mailing list with useless traffic that everyone else has to
ignore.

I think we should have a firm policy that if patch -p1 can apply
your patch, your patch is sufficiently well-formatted. If someone
wants the result as a context diff, a unified diff, with one kind of
line endings vs. another, or whatever, they can apply the patch
locally and use whatever tools they like to get a diff in the format
they prefer.

When posting large patch stacks, 'git format-patch' is nice because
it lets you give a sequence number and a commit message to each
patch in a sensible way. I recommend it, but I don't think we
should insist on it.

I'm sure I'm not alone in finding it helpful when patch sets come with
a single-sentence summary of the patch set and a commit message for
each individual patch.

Is git format-patch really too heavy a lift to ask of people?

Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter wrote:

I'm sure I'm not alone in finding it helpful when patch sets come with
a single-sentence summary of the patch set and a commit message for
each individual patch.

Is git format-patch really too heavy a lift to ask of people?

I think it's okay as general guideline, but not as a hard requirement.
Just like we advise people to trim quoted text when they reply to
mailing list postings, but we don't boot those that fail to.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jan 16, 2018 at 8:56 AM, Robert Haas <robertmhaas@gmail.com> wrote:

I've seen that before as well.

I have also noticed people complaining about patches that apply "with
offsets", which also seems like needless nitpicking. If the offsets
are large and the patch has been sitting around for a long time,
there's a small chance it could be applying to the wrong place, but
that is extremely rare. Most patches have small offsets, just a few
lines, and there is no problem.

+1

The parallel CREATE INDEX patch is something that I've worked on
(fairly inconsistently) for 2 years now. I remember two occasions in
which somebody else changed a function signature for functions that my
code called, and without that causing even a compiler warning after
rebasing on top of these changes (e.g., changing an int argument to a
bool argument). On both occasions, this led to a real bug in a version
of the patch that was posted to the list.

Mechanical detection of problems is great, but there is no substitute
for vigilance. I think that people that complain about stuff like
patches applying with offsets have a false sense of security about
detecting problems mechanically. Rebasing a patch without conflicts
(including seeing a warning about offsets) does not mean that your
patch didn't become broken in some subtle, harmful way. Mechanical
detection is only useful to the extent that it guides and augments
human oversight.

--
Peter Geoghegan

Peter Geoghegan <pg@bowt.ie> writes:

The parallel CREATE INDEX patch is something that I've worked on
(fairly inconsistently) for 2 years now. I remember two occasions in
which somebody else changed a function signature for functions that my
code called, and without that causing even a compiler warning after
rebasing on top of these changes (e.g., changing an int argument to a
bool argument). On both occasions, this led to a real bug in a version
of the patch that was posted to the list.

FWIW, I think that that represents bad practice in those changes,
precisely because of the hazard it poses for uncommitted patches.
If you're changing a function signature, it's usually not that hard
to make sure that un-updated code will produce a failure or warning,
and you should generally do so IMO.

regards, tom lane

On Tue, Jan 16, 2018 at 3:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

FWIW, I think that that represents bad practice in those changes,
precisely because of the hazard it poses for uncommitted patches.
If you're changing a function signature, it's usually not that hard
to make sure that un-updated code will produce a failure or warning,
and you should generally do so IMO.

I strongly agree. That's an example of the programmer exploiting
mechanical detection of conflicts deliberately, which is great. All of
these things are tools, and like all tools they are generally not
helpful unless used thoughtfully.

--
Peter Geoghegan

On Tue, Jan 16, 2018 at 04:51:13PM -0300, Alvaro Herrera wrote:

David Fetter wrote:

I'm sure I'm not alone in finding it helpful when patch sets come with
a single-sentence summary of the patch set and a commit message for
each individual patch.

Is git format-patch really too heavy a lift to ask of people?

I think it's okay as general guideline, but not as a hard requirement.
Just like we advise people to trim quoted text when they reply to
mailing list postings, but we don't boot those that fail to.

I agree with this position. Sometimes even patches created with
format-patch fail to apply with git apply after rotting a bit (because
git apply/am also complains about offsets more easily? And cherry-pick
forgives more easily?). At the end I generally finish by applying things
with patch -p1 after testing with git apply/am. As a general guideline,
if a patch can be applied cleanly with patch -p1, then the thing should
not need a rebase. There could be issues with misplaced blocks because
of offsets, those usually finish with compilation failures, not usually
with regression test failures. If those happen requesting a rebase is
fine, but like Robert there is no point to complain about a patch that
applies and works with offsets. Mentioning that is good because that's a
sign that a patch is aging, but that's not an argument sufficient for a
rebase.

Some communities have hard guidelines for patch format with their patch
submission, which tend to make people refrain to contribute even small
patches (which get ignored by upstream committers at the end), as the
set of basic guidelines is harder to learn than producing a simple
patch. Personally as long as I can read a patch proposed for a bug fix
from a text file, on which I am able to understand the intention behind,
then that's acceptable to dive into as the goal is to fix an existing
problem. Patches for features able to apply are fine to look at (of
course this depends on the feature, the docs it has, what the
contributor is proposing, etc).

An idea could be to add more detailed guidelines in the wiki for the
patch review section:
https://wiki.postgresql.org/wiki/Submitting_a_Patch
Perhaps something among the lines: "When a feature requires deep
surgery, dividing a patch into several entries with git-format-patch
with a proper commit log is recommended and eases review, though this is
not mandatory. git apply/am are very picky commands, so as long as a
patch can apply with patch -p1 consider yourself covered."
--
Michael