Re: [BUG?] Patches created with 'diff.noprefix=true' don't 'git apply'.

From: Junio C Hamano <gitster@pobox.com>
Date: May 7, 2014, 18:42 UTC
Message-ID: <xmqqzjita0sn.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAO8RVveF6Oh0mqqyiVRHmnqQ+vXX7riDW_=zn0g=t88cQjCVrg@mail.gmail.com>

Nathan Collins <nathan.collins@gmail.com> writes:

Show 6 quoted lines

> More concretely, what I had in mind was that if 'diff.noprefix=true'
> is set in the user's config, and the patch is in '-p0' format, then
> Git could suggest to the user that the 'diff.noprefix' setting *might*
> be causing them to generate '-p0' patches. If the user had in fact not
> generated the patch themselves, then they could safely ignore the
> suggestion.

Issuing a suggestion that can be ignored too often sounds like crying wolf to train users to ignore all other more useful suggestions, so I would advise to be very cautious before doing so. If you cannot come up with a way to make the heuristics very sure and foolproof, I do not think it would help more than it hurt.

One possibility I considered briefly before discarding it was to see if the input is coming from a pipe *and* these configuration is set.

> But this may just be an overcomplicated solution to my and others'
> misuse of the 'diff.noprefix' option; see below.
>
>> So that is not workable.

Yes.

Show 11 quoted lines

>> However, Before issuing this error message
>>
>>>   git -c diff.noprefix=true diff HEAD~ | git apply --reverse
>>>   error: Data/Function/Decorator/Memoizer/Unsafe.hs: No such file or directory
>>
>> we _could_ check that there is Data/ directory in the target tree
>> the patch is being applied and suggest to:
>
> To clarify, the actual path was
>
>   src/Data/Function/Decorator/Memoizer/Unsafe.hs

Yeah, and I meant to suggest checking that path is plausible. It would not help when adding a new file, but we can issue the original error message and that is perfectly readable, so catching only paths that are modified or deleted and suggesting -p$n is still making things better.

> 1. 'git apply' doesn't give a very helpful error message when the
> patch does not apply due to not being in '-p1' format.

That "Data/Function/..." path in the error message is helpful enough, once you learn that -p$n strips the leading directory and that -p1 is the default. For new people, however, -p1 being the default might be unexpected, but the learning curve is not that steep, I would think. So this is a one-time thing.

Show 5 quoted lines

> 2. the 'diff.noprefix=true' option is used for two unrelated things in
> practice. One of them is related to diffing -- namely, making Git
> generate '-p0' patches -- and the other is unrelated to diffing --
> namely, users want file names that can be easily copied with
> double-click.

I do not share this observation, as I never double-click. I let my shell to complete instead ;-)

Show 11 quoted lines

> For (2), the solution may be to add a separate
> 'diff.add-clickable-paths' option (probably there is a better name?
> 'diff.add-copyable-paths'? ...),...
> ...
> Concretely, if 'diff.add-clickable-paths' is set, then instead of e.g.
>
>   diff --git a/src/Data/Function/Decorator/Memoizer/Unsafe.hs
> b/src/Data/Function/Decorator/Memoizer
>   index 3ef17da..a0586d3 100644
>   --- a/src/Data/Function/Decorator/Memoizer/Unsafe.hs
>   +++ b/src/Data/Function/Decorator/Memoizer/Unsafe.hs

If you do something along that line, perhaps

	Index: src/Data/Function/Decorator/Memoizer/Unsafe.hs
        diff --git a/src/Data/Function/Decorator/Memoizer/Unsafe.hs ...
        index 3ef17da..a0586d3 100644
        --- a/src/Data/Function/Decorator/Memoizer/Unsafe.hs
        +++ b/src/Data/Function/Decorator/Memoizer/Unsafe.hs

to imitate what "cvs diff" does may be more familar to people.

What would you propose to make clickable in a renaming diff, though?