| Message ID | 897c200c-afb3-ceb4-bf44-9af651f5feb4@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Better suggestions when git-am(1) fails | expand |
On Wed, Mar 08, 2023 at 09:15:53PM +0100, Alejandro Colomar wrote: > I had the following error already a few times, when some contributors, > for some reason unknown to me, remove the leading path components from > the patch. The reason is probably that they have set diff.noprefix in their config, and git-format-patch respects that. Which is arguably a bug. There's a little discussion in this message, along with references to some previous discussions: https://lore.kernel.org/git/ZAWnDUkgO5clf6qu@coredump.intra.peff.net/ > Now I know that the fix is to use -p0, but the first times it wasn't > obvious. And still I forget about -p0 sometimes and it's hard to find > in the manual pages. I think it would be good to suggest using it > when such an error appears. I agree it may be reasonable to have "git am" be more helpful on the receiving side. Hopefully if format-patch is changed then you wouldn't see the situation as often, but it could still happen. -Peff
On Wed, Mar 08, 2023 at 10:17:11PM -0500, Jeff King wrote: > On Wed, Mar 08, 2023 at 09:15:53PM +0100, Alejandro Colomar wrote: > > > I had the following error already a few times, when some contributors, > > for some reason unknown to me, remove the leading path components from > > the patch. > > The reason is probably that they have set diff.noprefix in their config, > and git-format-patch respects that. Which is arguably a bug. There's a > little discussion in this message, along with references to some > previous discussions: > > https://lore.kernel.org/git/ZAWnDUkgO5clf6qu@coredump.intra.peff.net/ So here's a patch series which I think should help with the sending side. Most of it is just filling in gaps in the code and tests for current features. Patch 4 is the actual change. Patch 5 adds an equivalent option just for format-patch. I'm not convinced anybody really wants it (which is why I split it out), but it's probably worth doing just in case. [1/5]: diff: factor out src/dst prefix setup [2/5]: t4013: add tests for diff prefix options [3/5]: diff: add --default-prefix option [4/5]: format-patch: do not respect diff.noprefix [5/5]: format-patch: add format.noprefix option Documentation/config/format.txt | 7 ++++++ Documentation/diff-options.txt | 5 ++++ builtin/log.c | 17 +++++++++++++ diff.c | 33 ++++++++++++++++++++++---- diff.h | 2 ++ t/t4013-diff-various.sh | 42 +++++++++++++++++++++++++++++++++ t/t4014-format-patch.sh | 16 +++++++++++++ 7 files changed, 117 insertions(+), 5 deletions(-) -Peff
Hi Jeff, On 3/9/23 07:06, Jeff King wrote: > On Wed, Mar 08, 2023 at 10:17:11PM -0500, Jeff King wrote: > >> On Wed, Mar 08, 2023 at 09:15:53PM +0100, Alejandro Colomar wrote: >> >>> I had the following error already a few times, when some contributors, >>> for some reason unknown to me, remove the leading path components from >>> the patch. >> >> The reason is probably that they have set diff.noprefix in their config, >> and git-format-patch respects that. Which is arguably a bug. There's a >> little discussion in this message, along with references to some >> previous discussions: >> >> https://lore.kernel.org/git/ZAWnDUkgO5clf6qu@coredump.intra.peff.net/ > > So here's a patch series which I think should help with the sending > side. Most of it is just filling in gaps in the code and tests for > current features. Patch 4 is the actual change. Patch 5 adds an > equivalent option just for format-patch. I'm not convinced anybody > really wants it (which is why I split it out), but it's probably worth > doing just in case. Thanks for the rapid patch set :) > > [1/5]: diff: factor out src/dst prefix setup > [2/5]: t4013: add tests for diff prefix options > [3/5]: diff: add --default-prefix option > [4/5]: format-patch: do not respect diff.noprefix > [5/5]: format-patch: add format.noprefix option 1, 3, and 4 LGTM. I'm not used to your tests, so can't really check what 2 does without further reading, and I'm not sure 5 is useful. BTW, I'll probably report a few more things I don't like from git-am(1)'s error reports, whenever I find them again. ;) Cheers, Alex > > Documentation/config/format.txt | 7 ++++++ > Documentation/diff-options.txt | 5 ++++ > builtin/log.c | 17 +++++++++++++ > diff.c | 33 ++++++++++++++++++++++---- > diff.h | 2 ++ > t/t4013-diff-various.sh | 42 +++++++++++++++++++++++++++++++++ > t/t4014-format-patch.sh | 16 +++++++++++++ > 7 files changed, 117 insertions(+), 5 deletions(-) > > -Peff
Jeff King <peff@peff.net> writes: > On Wed, Mar 08, 2023 at 09:15:53PM +0100, Alejandro Colomar wrote: > >> I had the following error already a few times, when some contributors, >> for some reason unknown to me, remove the leading path components from >> the patch. > > The reason is probably that they have set diff.noprefix in their config, > and git-format-patch respects that. Which is arguably a bug. FWIW, I've always considered it a feature to help projects that prefer their patches in -p0 form. Of course, Git optimized itself for the usecase we consider the optimum, i.e. using a/ and b/ prefix on the diff generation side, while stripping them with -p1 on the applying side. I wonder apply.plevel or am.plevel would be a good way to help them further? I am not sure making format-patch _ignore_ diff.src/dst_prefix is a good approach. If we were wiser, we may not have introduced the diff.noprefix option, made sure diff.src/dstprefix to be always a single level, and kept -p<n> on the application side as an escape hatch only to deal with non-Git generated patches. The opportunity to simplify the world that way however we missed 15 years ago X-<.
Jeff King <peff@peff.net> writes: > So here's a patch series which I think should help with the sending > side. Most of it is just filling in gaps in the code and tests for > current features. Patch 4 is the actual change. Patch 5 adds an > equivalent option just for format-patch. I'm not convinced anybody > really wants it (which is why I split it out), but it's probably worth > doing just in case. > > [1/5]: diff: factor out src/dst prefix setup > [2/5]: t4013: add tests for diff prefix options > [3/5]: diff: add --default-prefix option > [4/5]: format-patch: do not respect diff.noprefix > [5/5]: format-patch: add format.noprefix option I've reviewed these five changes, and while I am not 100% sold to the idea that we should force our -p1 worldview to those who choose to use diff.noprefix for whatever reason, I think these patches describe what they want to do and implement it in a very readable way. Thanks. Queued.
On Thu, Mar 09, 2023 at 08:22:00AM -0800, Junio C Hamano wrote: > > The reason is probably that they have set diff.noprefix in their config, > > and git-format-patch respects that. Which is arguably a bug. > > FWIW, I've always considered it a feature to help projects that > prefer their patches in -p0 form. Of course, Git optimized itself > for the usecase we consider the optimum, i.e. using a/ and b/ prefix > on the diff generation side, while stripping them with -p1 on the > applying side. > > I wonder apply.plevel or am.plevel would be a good way to help them > further? I doubt they would help, because they imply a constant project workflow. We have seen several reports of "sometimes I get a patch without a prefix, and it doesn't apply. What's going on?". But I don't think anybody asked for "my project doesn't use prefixes, and I am tired of typing -p0". The more interesting case to me is that the receiver _isn't_ using Git. They are using "patch" or similar, and they expect senders to send them patches without prefixes. And there, diff.noprefix is doing what they want. But I have to wonder if these hypothetical maintainers exist: 1. I feel like "-p1" was pretty standard even before Git. You'd extract two copies of the tarball, one into "foo-1.2.3" and one into "foo-1.2.3.orig", and then "diff -Nru" between them to send a patch. 2. It feels weird that a maintainer who isn't using Git would expect a lot of contributions from folks who are. And even weirder, that they would insist that all of the folks sending patches set diff.noprefix. So I won't say it's not possible (especially in some closed community). But I'm skeptical. All that said, if "apply" and "am" could automatically figure out and handle "-p0" patches, that would be a useful way to help people. I'm just hesitant because it probably involves some heuristics. E.g., we get "foo/bar", realize that "bar" doesn't exist, but "foo/bar" does. Except that fails if a project does have "bar". And so on. > I am not sure making format-patch _ignore_ diff.src/dst_prefix is a > good approach. If we were wiser, we may not have introduced the > diff.noprefix option, made sure diff.src/dstprefix to be always a > single level, and kept -p<n> on the application side as an escape > hatch only to deal with non-Git generated patches. The opportunity > to simplify the world that way however we missed 15 years ago X-<. Yeah, I am as always a little concerned that one person's fix is another one's regression. But it really just seems to that on balance people set diff.noprefix with no thought at all to how it would affect format-patch (in fact, I'd guess 99% of Git users do not use format-patch at all). And then they are surprised (or worse, the receiver is surprised) when it doesn't work. -Peff
On Thu, Mar 09, 2023 at 01:53:55PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > So here's a patch series which I think should help with the sending > > side. Most of it is just filling in gaps in the code and tests for > > current features. Patch 4 is the actual change. Patch 5 adds an > > equivalent option just for format-patch. I'm not convinced anybody > > really wants it (which is why I split it out), but it's probably worth > > doing just in case. > > > > [1/5]: diff: factor out src/dst prefix setup > > [2/5]: t4013: add tests for diff prefix options > > [3/5]: diff: add --default-prefix option > > [4/5]: format-patch: do not respect diff.noprefix > > [5/5]: format-patch: add format.noprefix option > > I've reviewed these five changes, and while I am not 100% sold to > the idea that we should force our -p1 worldview to those who choose > to use diff.noprefix for whatever reason, I think these patches > describe what they want to do and implement it in a very readable > way. > > Thanks. Queued. Thanks for looking at them. Let's see if we get any other comments on the direction, and then I may re-roll. Even if we don't do 4 or 5, I think the extra tests are worth adding. Either way I'd probably drop 3 (in favor of --src-prefix) and squash its tests into 2. Patch 1 isn't worthwhile if we don't do 3-5, since we wouldn't be adding any new callers of the helpers. If we do proceed, I'd suggest trying to cook in 'next' for a long time to get comment. Though I think both you and I are pessimistic that we get a wide variety of user testing that way. -Peff
Jeff King <peff@peff.net> writes: > 1. I feel like "-p1" was pretty standard even before Git. You'd > extract two copies of the tarball, one into "foo-1.2.3" and one > into "foo-1.2.3.orig", and then "diff -Nru" between them to send a > patch. I would too, but then we wouldn't have accepted the request to add .noprefix configuration; I do not recall where it came from. > 2. It feels weird that a maintainer who isn't using Git would expect a > lot of contributions from folks who are. And even weirder, that > they would insist that all of the folks sending patches set > diff.noprefix. > > So I won't say it's not possible (especially in some closed community). > But I'm skeptical. The scenario I would find more likely is a project established long before we were popular wants to keep using -p0 even after switching to use Git. > All that said, if "apply" and "am" could automatically figure out > and handle "-p0" patches, that would be a useful way to help > people. I'm just hesitant because it probably involves some heuristics. I am not all that interested in that direction, for exactly the same reason as I are heditant. Such a tool that outsmarts users will eventually bite them. > Yeah, I am as always a little concerned that one person's fix is another > one's regression. But it really just seems to that on balance people set > diff.noprefix with no thought at all to how it would affect format-patch > (in fact, I'd guess 99% of Git users do not use format-patch at all). > And then they are surprised (or worse, the receiver is surprised) when > it doesn't work. For these 99% users, if format-patch paid attention to their diff.noprefix and used -p0, the world would become even more interesting place. I am not sure this particular cure is an overall win. And as you mentioned elsewhere, a change that is deliberately designed to be breaking like this does not become much safer by cooking in 'next', which is another sad thing.
On Fri, Mar 10, 2023 at 08:28:55AM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > 1. I feel like "-p1" was pretty standard even before Git. You'd > > extract two copies of the tarball, one into "foo-1.2.3" and one > > into "foo-1.2.3.orig", and then "diff -Nru" between them to send a > > patch. > > I would too, but then we wouldn't have accepted the request to add > .noprefix configuration; I do not recall where it came from. I always thought it was an aesthetic thing for humans viewing diffs (and likewise mnemonicprefix). The original thread doesn't give much motivation, though: https://lore.kernel.org/git/1272852221-14927-1-git-send-email-eli@cloudera.com/ That is not really important as what the option has grown to be used for, of course, but it's another data point (or lack thereof in this case). -Peff
Jeff King <peff@peff.net> writes: > On Fri, Mar 10, 2023 at 08:28:55AM -0800, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >> > 1. I feel like "-p1" was pretty standard even before Git. You'd >> > extract two copies of the tarball, one into "foo-1.2.3" and one >> > into "foo-1.2.3.orig", and then "diff -Nru" between them to send a >> > patch. >> >> I would too, but then we wouldn't have accepted the request to add >> .noprefix configuration; I do not recall where it came from. > > I always thought it was an aesthetic thing for humans viewing diffs (and > likewise mnemonicprefix). The original thread doesn't give much > motivation, though: > > https://lore.kernel.org/git/1272852221-14927-1-git-send-email-eli@cloudera.com/ Interesting. Comparison with mnemonicprefix is a bit unfair, as it does not break other tools, though ;-).
diff --git CONTRIBUTING CONTRIBUTING index 3b4408108..3bb671eca 100644 --- CONTRIBUTING +++ CONTRIBUTING @@ -8,7 +8,7 @@ Description Mailing list The main discussions regarding development of the project, patches, bugs, news, doubts, etc. happen on the mailing list. To send an email - to the project, send it to both maintainers and CC the mailing list: + to the project, send it to Alejandro and CC the mailing list: To: Alejandro Colomar <alx@kernel.org> Cc: <linux-man@vger.kernel.org>