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

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

[Nathan Collins]

Bug?
====

Patches created with 'diff.noprefix=true' don't 'git apply' without
specifying '-p0'.

I'm not sure this is a bug -- the 'man git-apply' just says "Reads the
supplied diff output (i.e. "a patch") and applies it to files" -- but
I would expect patches I create locally to apply cleanly locally. In
real life the 'diff.noprefix=true' is in my ~/.gitconfig, so this was
pretty confusing.

Here's an old bug that's kind of related:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=607044

I'm using Git 1.9.2.

Example
=======

Create a repo with a test commit:

  git init bug.git
  cd bug.git
  git add test
  git commit test -m Test

Revert the test commit in a contrived way (like
'git revert HEAD --no-commit; git reset'). This works:

  git -c diff.noprefix=false show | git -c diff.noprefix=false apply --reverse

And this works:

  git reset --hard
  git -c diff.noprefix=true show | git -c diff.noprefix=true apply -p0 --reverse

But this fails:

  git reset --hard
  git -c diff.noprefix=true show | git -c diff.noprefix=true apply --reverse

    fatal: git diff header lacks filename information when removing 1
leading pathname component (line 12)

Use Case
========

Partially reverting a commit:

http://git.661346.n2.nabble.com/Revert-a-single-commit-in-a-single-file-td6064050.html#a6064406

Cheers,

-nathan

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

[Jonathan Nieder]

Hi,

Nathan Collins wrote:

> Patches created with 'diff.noprefix=true' don't 'git apply' without
> specifying '-p0'.
>
> I'm not sure this is a bug -- the 'man git-apply' just says "Reads the
> supplied diff output (i.e. "a patch") and applies it to files" -- but
> I would expect patches I create locally to apply cleanly locally.

Sounds like a documentation bug, at least.  Any ideas for clearer
wording?

>                                                                   In
> real life the 'diff.noprefix=true' is in my ~/.gitconfig, so this was
> pretty confusing.

I personally think setting diff.noprefix is not very sane (it also
breaks "patch -p1"), and I suppose I should have been louder about
that when it was introduced.

Can you say more about the workflow you use that requires
diff.noprefix?  Maybe we can make other changes to improve it, too.

At first glance I don't suspect making diff.noprefix imply -p0 for
"git am" would be great, since that would generate the the opposite
problem when applying patches from the outside world.  But maybe we
need better autodetection and maybe noprefix is a good signal about
when to use it.

Another complication is that unlike 'git diff', 'git apply' is
plumbing that is meant to be useful and reliable for scripts.  And
unlike most plumbing, there is no higher-level command with similar
functionality for which we can experiment more freely with the UI.
Adding a new command to fix that might be a good direction toward
handling noprefix patches better.

[...]
> git show | git apply --reverse

The following which only uses plumbing commands should work:

	git diff-tree -p HEAD^! |
	git apply --reverse

Thanks for some food for thought,
Jonathan

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

[Nathan Collins]

On Wed, Apr 30, 2014 at 7:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Nathan Collins wrote:
>
>> Patches created with 'diff.noprefix=true' don't 'git apply' without
>> specifying '-p0'.
>>
>> I'm not sure this is a bug -- the 'man git-apply' just says "Reads the
>> supplied diff output (i.e. "a patch") and applies it to files" -- but
>> I would expect patches I create locally to apply cleanly locally.
>
> Sounds like a documentation bug, at least.  Any ideas for clearer
> wording?

Hmmm. Maybe a warning that the patch is expected to be in '-p1'
format, and that setting 'diff.noprefix=true' makes some commands
generate '-p0' patches? But I worry this would just confuse / distract
the people that don't have 'diff.noprefix=true' set, which I expect is
the majority of users.  Better I think would be for 'git apply' to be
smarter, as you suggest below.

>>                                                                   In
>> real life the 'diff.noprefix=true' is in my ~/.gitconfig, so this was
>> pretty confusing.
>
> I personally think setting diff.noprefix is not very sane (it also
> breaks "patch -p1"), and I suppose I should have been louder about
> that when it was introduced.
>
> Can you say more about the workflow you use that requires
> diff.noprefix?  Maybe we can make other changes to improve it, too.

I have 'diff.noprefix=true' set so I can copy and paste paths from the
'git diff' output easily.  I like to create small, logically
independent commits, usually comprising a subset of my current
changes. So, I do 'git diff' in one terminal, and then 'git add
<path>' or 'git add --patch <path>' in another terminal to build up a
commit (I suppose this is the work flow that 'git add --interactive'
is designed for ...), where I get '<path>' from the diff by copying
and pasting. With 'diff.noprefix=true', I can copy with double left
click and paste with middle click; with 'diff.noprefix=false', to copy
I instead have to carefully highlight the non-prefix part of the path
in the diff, which is less convenient.

> At first glance I don't suspect making diff.noprefix imply -p0 for
> "git am" would be great, since that would generate the the opposite
> problem when applying patches from the outside world.  But maybe we
> need better autodetection and maybe noprefix is a good signal about
> when to use it.

Autodetecting the lack or presence of the 'a/' and 'b/' prefixes seems
like a great solution to me: externally user friendly and easy to
implement internally.

> Another complication is that unlike 'git diff', 'git apply' is
> plumbing that is meant to be useful and reliable for scripts.  And
> unlike most plumbing, there is no higher-level command with similar
> functionality for which we can experiment more freely with the UI.
> Adding a new command to fix that might be a good direction toward
> handling noprefix patches better.

Related to 'git apply' being a scriptable plumbing command: naively I
would expect there to be a "scripting mode" for Git commands which
ignored the local configuration entirely (e.g. ~/.gitconfig). I've
wanted this a few times and was surprised I could find no very sane
way to achieve it. In fact, here's the corresponding question I posted
on Stack Overflow while I was composing my original email (I wanted to
be sure that 'diff.noprefix=true' was the only relevant part of my
~/.gitconfig, so I wanted disable my ~/.gitconfig entirely):

http://stackoverflow.com/questions/23400449/how-to-make-git-temporarily-ignore-gitconfig

> [...]
>> git show | git apply --reverse
>
> The following which only uses plumbing commands should work:
>
>         git diff-tree -p HEAD^! |
>         git apply --reverse

Nice! However, I don't now how to generalize this solution to other
(probably insane) use cases, e.g.

  git log -S<string> --patch | git apply --reverse

(Context: http://stackoverflow.com/a/23401018/470844).

> Thanks for some food for thought,
> Jonathan

Thanks for your reply. I didn't see it until today because a GMail
filter ate it :P

-nathan

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

[Jonathan Nieder]

Nathan Collins wrote:
> On Wed, Apr 30, 2014 at 7:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Nathan Collins wrote:

>>> git show | git apply --reverse
>>
>> The following which only uses plumbing commands should work:
>>
>>         git diff-tree -p HEAD^! |
>>         git apply --reverse
>
> Nice! However, I don't now how to generalize this solution to other
> (probably insane) use cases, e.g.
>
>   git log -S<string> --patch | git apply --reverse

This should do it:

	git rev-list HEAD |
	git diff-tree --no-commit-id -p -S<string> --stdin |
	git apply --reverse

More generally, when scripting plumbing commands tend to do the right
thing.

Will think more about the documentation and other parts (or if someone
else sends a patch before I can, I won't mind).

Thanks,
Jonathan

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

[Junio C Hamano]

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

> Hmmm. Maybe a warning that the patch is expected to be in '-p1'
> format, and that setting 'diff.noprefix=true' makes some commands
> generate '-p0' patches?

"some"?  Do you have exceptions in mind?

> But I worry this would just confuse / distract
> the people that don't have 'diff.noprefix=true' set,

Probably.  But that would suggest that the place to improve the doc
is for diff.noprefix configuration variable, no?

> Better I think would be for 'git apply' to be
> smarter, as you suggest below.

As it is a plumbing command behind "add -p", "am", and friends, I
would hate to see "git apply" pretend to be smarter than its users.
When the user tells it to use -p0, it shouldn't guess, and when the
user tells it to use -p1 by not giving any -p$n, it shouldn't guess.

As long as we make it clear "git apply" without any explicit -p$n
means the user is telling it to do -p1 in its documentation, I think
it would be fine.

>> I personally think setting diff.noprefix is not very sane (it also
>> breaks "patch -p1"), and I suppose I should have been louder about
>> that when it was introduced.

I share the same feeling ;-)  But the boat has sailed, so the best
we could do is to warn in its doc (i.e. where diff.noprefix is
described) about its pitfalls.

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

[Nathan Collins]

On Tue, May 6, 2014 at 11:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nathan Collins <nathan.collins@gmail.com> writes:
>
>> Hmmm. Maybe a warning that the patch is expected to be in '-p1'
>> format, and that setting 'diff.noprefix=true' makes some commands
>> generate '-p0' patches?
>
> "some"?  Do you have exceptions in mind?

As Jonathan pointed out in his first reply, 'git diff-tree' ignores
the 'diff.noprefix=true' setting.  Compare

  git -c diff.noprefix=true diff HEAD~

with

  git -c diff.noprefix=true diff-tree -p HEAD

(E.g.

   diff <(git -c diff.noprefix=true diff HEAD~) <(git -c
diff.noprefix=true diff-tree -p HEAD)

)

>> But I worry this would just confuse / distract
>> the people that don't have 'diff.noprefix=true' set,
>
> Probably.  But that would suggest that the place to improve the doc
> is for diff.noprefix configuration variable, no?

I don't think that would actually help much in practice. The problem
is that a person (like me) that set 'diff.noprefix=true' in their
~/.gitconfig months or years ago is unlikely to do 'man git-config'
when 'git apply' fails. Having the warning in 'man git-apply' is
better than (only) in 'man git-config', if making 'git apply' smarter
is not an option.

>> Better I think would be for 'git apply' to be
>> smarter, as you suggest below.
>
> As it is a plumbing command behind "add -p", "am", and friends, I
> would hate to see "git apply" pretend to be smarter than its users.
> When the user tells it to use -p0, it shouldn't guess, and when the
> user tells it to use -p1 by not giving any -p$n, it shouldn't guess.

Is there a non-plumbing command for applying patches not in mailboxes?
I don't see how to replace '| git apply --reverse' with '| git am ???'
here.

> As long as we make it clear "git apply" without any explicit -p$n
> means the user is telling it to do -p1 in its documentation, I think
> it would be fine.

OK, then how about a smarter error message? Right now I get

  git -c diff.noprefix=true diff HEAD~ | git -c diff.noprefix=true
apply --reverse
  error: Data/Function/Decorator/Memoizer/Unsafe.hs: No such file or directory

vs

  git -c diff.noprefix=true diff HEAD~ | patch --reverse
  can't find file to patch at input line 5
  Perhaps you should have used the -p or --strip option?
  [...]

But 'git apply' could be much more helpful than 'patch' even, since
the presence or absence of the 'a/' and 'b/' prefixes in the patch,
and the 'diff.noprefix' setting, give Git enough info to be very
helpful to the user.

Cheers,

-nathan

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

[Junio C Hamano]

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

> But 'git apply' could be much more helpful than 'patch' even, since
> the presence or absence of the 'a/' and 'b/' prefixes in the patch,
> and the 'diff.noprefix' setting, give Git enough info to be very
> helpful to the user.

The prefix would be unreliable as the generator may be using the
mnemonicprefix option to use i/ and w/ prefixes.  Worse yet, the
configuration variables are for people who generated the diff you
are feeding "git apply", and there is nothing that tells "apply"
that the patch is generated with _your_ setting.

So that is not workable.

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:

 * "use -p0", if noprefix, which I agree with Jonathan is an insane
   thing to use by default, is common enough; or

 * "use different setting for -p$n", if noprefix is not common.

in the error message.  Extra computation necessary to do so would
happen only in the error codepath, and we wouldn't mind spending
some cycles if they help the end user.

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

[Nathan Collins]

On Tue, May 6, 2014 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nathan Collins <nathan.collins@gmail.com> writes:
>
>> But 'git apply' could be much more helpful than 'patch' even, since
>> the presence or absence of the 'a/' and 'b/' prefixes in the patch,
>> and the 'diff.noprefix' setting, give Git enough info to be very
>> helpful to the user.
>
> The prefix would be unreliable as the generator may be using the
> mnemonicprefix option to use i/ and w/ prefixes.  Worse yet, the
> configuration variables are for people who generated the diff you
> are feeding "git apply", and there is nothing that tells "apply"
> that the patch is generated with _your_ setting.

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.

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.
>
> 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

The path in the error message,

  Data/Function/Decorator/Memoizer/Unsafe.hs

was the '-p1' version of that path. This is extra confusing if the
user is unfamiliar with the '-p' option for patch and unaware that
'git apply' is assuming '-p1'.

>  * "use -p0", if noprefix, which I agree with Jonathan is an insane
>    thing to use by default, is common enough; or
>
>  * "use different setting for -p$n", if noprefix is not common.
>
> in the error message.  Extra computation necessary to do so would
> happen only in the error codepath, and we wouldn't mind spending
> some cycles if they help the end user.

I'm starting to think there are really two separate issues here:

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

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.

For (1), I think the solution is check if the patch makes sense as
'-p0', in the error case, and tell the user about this in the error
message as you suggested above.  In fact, in case the '-p1' path
doesn't exist, Git could just try all possible '-p$n' values, and
report the first that yields valid paths, if any. Mentioning to the
user that they have 'diff.noprefix=true' set in case '-p0' is
discovered might be helpful, but a better solution to (2) might
eliminate this problem in practice.

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'? ...), which makes Git insert clickable
paths in the comments in the diff output. This handles the
clickable-paths use case which lead me and others to abuse
'diff.noprefix=true'. Examples where people suggest using
'diff.noprefix=true' to make it easier to double-click copy paths
include [1 - 5]. Examples where people suggest using 'noprefix' to
generate '-p0' patches include [6 - 10].

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

we get e.g.

  # src/Data/Function/Decorator/Memoizer/Unsafe.hs
  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

as the diff header.

In any case, warning the user in the 'diff.noprefix' docs that the
point of the option is to create '-p0' patches, and that setting this
permanently will cause bad interactions with other Git commands (like
'git apply') seems like a great idea -- you suggested this in your
first email, but I hadn't really understood why my use of
'diff.noprefix' was "insane" yet. This probably won't help people that
blindly use 'noprefix' in the "insane" way based on a suggestion they
found with Google, but it can't hurt. If there were a
'diff.add-clickable-paths' option, then the 'diff.noprefix' docs could
also mention this, and suggest the user use that instead if their use
case is the "easy copy" use case.

Thank you both for pointing out that my usage of 'diff.noprefix=true'
is insane. Sorry if my suggestion for the 'diff.add-clickable-paths'
option is also insane; making the 'git apply' error message better --
keeping in mind that users who use 'noprefix' to create clickable
paths may not even know what "'-p$n' patch" is -- is probably
sufficient in practice.

Cheers,

-nathan

References for copy and paste:

[1] http://stackoverflow.com/a/20370519/470844: answer to SO question
about why there are 'a/' and 'b/' prefixes. Last comment says that
'--no-prefix' makes copy and paste easier.

[2] https://coderwall.com/p/oaekvg: points out that 'noprefix' makes
copy and paste easier.

[3] http://git.661346.n2.nabble.com/PATCH-git-jump-ignore-custom-prefix-in-diff-mode-tp7567162p7567308.html:
post to Git mailing list with user mentioning they use 'noprefix' to
make copy and paste easier.

[4] http://hugogiraudel.com/2014/03/17/git-tips-and-tricks-part-2/:
blog post suggesting you can set 'noprefix' to make it easier to copy
and paste file names.

[5] http://www.databasesandlife.com/why-subversion-is-better-than-git/:
page arguing that SVN is better than Git (hahaha) because in SVN you
can copy file names from diff output with double click, but in Git
there is an annoying leading 'a/' or 'b/'. A Git defender replies
suggesting that 'noprefix' can be used to make copy/paste work.

References for creating '-p0' patches:

[6] http://tamsler.blogspot.com/2009/02/patching-with-git-diff.html

[7] http://stackoverflow.com/questions/4610744/can-i-get-a-patch-compatible-output-from-git-diff

[8] http://lists.drupal.org/pipermail/development/2011-April/038006.html

[9] http://www.lullabot.com/blog/article/git-best-practices-upgrading-patch-process

[10]
http://scribu.net/wordpress/contributing-to-wordpress-using-github.html:
SVN uses '-p0' patches, so they suggest using 'noprefix' to generate
SVN compatible patches from Git.

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

[Junio C Hamano]

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

> 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.

>> 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.

> 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 ;-)

> 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?

        

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

[Nathan Collins]

On Wed, May 7, 2014 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nathan Collins <nathan.collins@gmail.com> writes:

>> 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?

Your 'Index' header looks good, and I would expect a renaming diff to
have something like

  Index: foo -> bar

as in 'git status', but I just realized that a "clickable paths"
option already exists in some sense! There is a '--patch-with-raw'
option (which is "short" for '--patch' and '--raw', hahaha) which
inserts clickable file names in the patch, above each diff.  Moreover,
it respects the '--relative' option, so you can get relative or
absolute (relative repo root) clickable paths. It handles renaming by
inserting the old and new paths separated by space.

So then, having a way to make '--patch-with-raw' the default for all
non-plumbing patch-producing commands would solve the clickable paths
problem.

In a summary, a possible complete solution:

1. improve Git apply error message: mention '-p$n' and '-p1' default,
   and report if path in patch makes sense for some non-default '-p'
   value.

2. improve 'diff.noprefix' documentation: tell user that this option
   is for producing '-p0' patches, and that using it to produce
   clickable paths is insane and may cause problems with generated
   patches.  Suggest the user use '--patch-with-raw', and possibly
   '--relative', instead, or refer to (3).

3. add a Git config for making '--patch-with-raw' and optionally
   '--relative' the default for non-plumbing patch-producing commands.

Cheers,

-nathan

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

[Nathan Collins]

On Wed, May 7, 2014 at 4:39 PM, Nathan Collins <nathan.collins@gmail.com> wrote:
> On Wed, May 7, 2014 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nathan Collins <nathan.collins@gmail.com> writes:
>
>>> 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?
>
> Your 'Index' header looks good, and I would expect a renaming diff to
> have something like
>
>   Index: foo -> bar
>
> as in 'git status', but I just realized that a "clickable paths"
> option already exists in some sense! There is a '--patch-with-raw'
> option (which is "short" for '--patch' and '--raw', hahaha) which
> inserts clickable file names in the patch, above each diff.

Or not: I stupidly only tested this with a single file modified: it
turns out that all the clickable file names appear at the top of the
patch, not as one file name above each corresponding diff as I
claimed.

D'oh,

-nathan

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

[Nathan Collins]

On Wed, May 7, 2014 at 9:38 PM, Nathan Collins <nathan.collins@gmail.com> wrote:
> On Wed, May 7, 2014 at 4:39 PM, Nathan Collins <nathan.collins@gmail.com> wrote:
>> On Wed, May 7, 2014 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nathan Collins <nathan.collins@gmail.com> writes:
>>
>>>> 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?
>>
>> Your 'Index' header looks good, and I would expect a renaming diff to
>> have something like
>>
>>   Index: foo -> bar
>>
>> as in 'git status', but I just realized that a "clickable paths"
>> option already exists in some sense! There is a '--patch-with-raw'
>> option (which is "short" for '--patch' and '--raw', hahaha) which
>> inserts clickable file names in the patch, above each diff.
>
> Or not: I stupidly only tested this with a single file modified: it
> turns out that all the clickable file names appear at the top of the
> patch, not as one file name above each corresponding diff as I
> claimed.

The following may be a non-option, since presumably many tools depend
on the current Git patch format.

The paths in the "extended header lines" in Git patches are clickable
by default, and respect the '--relative' option. So, adding a path to
the extended header lines that don't already have one would solve the
"clickable paths" problem.

E.g.

  index <hash>..<hash> <mode>

becomes

  index <hash>..<hash> <mode> <path>

The 'man git-diff' description of extended header lines in the
"Generating Patches with -p" section:

  2. It is followed by one or more extended header lines:

         old mode <mode>
         new mode <mode>
         deleted file mode <mode>
         new file mode <mode>
         copy from <path>
         copy to <path>
         rename from <path>
         rename to <path>
         similarity index <number>
         dissimilarity index <number>
         index <hash>..<hash> <mode>

     File modes are printed as 6-digit octal numbers including the
file type and file
     permission bits.

     Path names in extended headers do not include the a/ and b/ prefixes.

Cheers,

-nathan

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

[Junio C Hamano]

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

>> What would you propose to make clickable in a renaming diff, though?
>
> Your 'Index' header looks good, and I would expect a renaming diff to
> have something like
>
>   Index: foo -> bar
>
> as in 'git status',

Heh, please don't call "Index:" *mine* --- It is a CVS abomination
;-).

For renames and copies, we do have separate "rename from" and
"rename to" in the extended header part, so there is no reason to
worry about them at all.  I would suggest showing the name _after_
the change (unless it is a deletion---instead of showing /dev/null
to signal that it was deleted, show the original filename) for
consistency so that users can do "show -p | grep '^Index: ' to see
what resulting paths there are without missing the renamed ones.

> but I just realized that a "clickable paths"
> option already exists in some sense! There is a '--patch-with-raw'
> option...

I do not think that would be useful (neither --stat which would be
more commonly used for other reasosn), because these come at the top
and by the time you see individual patch, they may be long scrolled
off the top of the screen.

Of course, the CVS "Index:" or "rename to" would be the same thing
if a file is heavily modified, so it may not be too big a deal, but
as I said, I never felt any need to double-click, so I wouldn't be
the best judge.