Обсуждение: let's not complain about harmless patch-apply failures
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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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