threads / patch / 31560

patchgit-jump: ignore (custom) prefix in diff mode

Subject: [PATCH] git-jump: ignore (custom) prefix in diff mode

## tl;dr

10 messages between Sep 17, 2012 and Sep 18, 2012. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Mischa POSLAWSKY· Sep 17, 2012, 01:21 UTC · lore

Matching the default file prefix b/ does not yield any results if config option diff.noprefix or diff.mnemonicprefix is enabled.

Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
---
Very useful script otherwise; thanks.
 contrib/git-jump/git-jump | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to diff +2 −2
diff --git contrib/git-jump/git-jump contrib/git-jump/git-jump
index a33674e..dc90cd6 100755
--- contrib/git-jump/git-jump
+++ contrib/git-jump/git-jump
@@ -21,9 +21,9 @@ open_editor() {
 }
 
 mode_diff() {
-	git diff --relative "$@" |
+	git diff --no-prefix --relative "$@" |
 	perl -ne '
-	if (m{^\+\+\+ b/(.*)}) { $file = $1; next }
+	if (m{^\+\+\+ (.*)}) { $file = $1; next }
 	defined($file) or next;
 	if (m/^@@ .*\+(\d+)/) { $line = $1; next }
 	defined($line) or next;

-- 
1.7.12.165.g8cb9d9c
Mischa POSLAWSKY· Sep 17, 2012, 03:01 UTC · re: Mischa POSLAWSKY · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

> diff --git contrib/git-jump/git-jump contrib/git-jump/git-jump
> index a33674e..dc90cd6 100755
> --- contrib/git-jump/git-jump
> +++ contrib/git-jump/git-jump

Apparently diff.noprefix also applies to git format-patch. Even though git am does explicitly support -p0, I would argue against diff options creating non-standard patches.

-- >8 --
Subject: [PATCH/RFC] format-patch: force default file prefixes in diff

Override user configuration (eg. diff.noprefix) in patches intended for external consumption to match the default prefixes expected by git-am.

Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
---
 builtin/log.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to builtin/log.c +2 −0
diff --git a/builtin/log.c b/builtin/log.c
index dff7921..91ded25 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1131,6 +1131,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.max_parents = 1;
 	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
+	rev.diffopt.a_prefix = "a/";
+	rev.diffopt.b_prefix = "b/";
 	rev.subject_prefix = fmt_patch_subject_prefix;
 	memset(&s_r_opt, 0, sizeof(s_r_opt));
 	s_r_opt.def = "HEAD";

-- 
1.7.12.166.ga54f379
Junio C Hamano· Sep 17, 2012, 05:22 UTC · re: Mischa POSLAWSKY · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

Mischa POSLAWSKY <git@shiar.nl> writes:
Show 17 quoted lines
>> diff --git contrib/git-jump/git-jump contrib/git-jump/git-jump
>> index a33674e..dc90cd6 100755
>> --- contrib/git-jump/git-jump
>> +++ contrib/git-jump/git-jump
>
> Apparently diff.noprefix also applies to git format-patch.  Even though
> git am does explicitly support -p0, I would argue against diff options
> creating non-standard patches.
>
> -- >8 --
> Subject: [PATCH/RFC] format-patch: force default file prefixes in diff
>
> Override user configuration (eg. diff.noprefix) in patches intended for
> external consumption to match the default prefixes expected by git-am.
>
> Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
> ---

Not all projects expect to see a/ & b/ prefix and these are configurable for a reason. Robbing the choice that has been supported for quite a long time from them is an unacceptable regression.

Why did you think this may be a good idea in the first place?

Perhaps you had configured your diff.noprefix in a wrong configuration file? This is primarily per-project choice, and your clone of git.git should not have diff.noprefix set, neither your $HOME/.gitconfig unless you always work on projects that want diff.noprefix.

Show 16 quoted lines
>  builtin/log.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index dff7921..91ded25 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1131,6 +1131,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	rev.diff = 1;
>  	rev.max_parents = 1;
>  	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
> +	rev.diffopt.a_prefix = "a/";
> +	rev.diffopt.b_prefix = "b/";
>  	rev.subject_prefix = fmt_patch_subject_prefix;
>  	memset(&s_r_opt, 0, sizeof(s_r_opt));
>  	s_r_opt.def = "HEAD";
Mischa POSLAWSKY· Sep 18, 2012, 02:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

Junio C Hamano skribis 2012-9-16 22:22 (-0700):
Show 14 quoted lines
> Mischa POSLAWSKY <git@shiar.nl> writes:
> 
> > Subject: [PATCH/RFC] format-patch: force default file prefixes in diff
> >
> > Override user configuration (eg. diff.noprefix) in patches intended for
> > external consumption to match the default prefixes expected by git-am.
> >
> > Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
> > ---
> 
> Not all projects expect to see a/ & b/ prefix and these are
> configurable for a reason.  Robbing the choice that has been
> supported for quite a long time from them is an unacceptable
> regression.
My bad, I was assuming format-patch would mostly interact with git am.
Show 7 quoted lines
> Why did you think this may be a good idea in the first place?
> 
> Perhaps you had configured your diff.noprefix in a wrong
> configuration file?  This is primarily per-project choice, and your
> clone of git.git should not have diff.noprefix set, neither your
> $HOME/.gitconfig unless you always work on projects that want
> diff.noprefix.

Then I'm not using it as intended. For me it's just a personal preference of how I'd like to review commits (diff/show) so I can easily copy-paste file names (less essential since my discovery of git jump, but still). It's not something I'd like to be communicated with any upstream project (format-patch).

So it seems I'm asking for a new feature: to be able to configure local and inter-project diff options differently. In this case I'd be helped by either format.noprefix=0 or a to be bikeshedded localdiff.noprefix=1. I don't know about other options though. Does anybody actually want mnemonicprefix to be sent out as well?

Another solution could be a single option defining behaviour exceptions: format.diff = normal | textconv | noconfig Expanding on the existing --(no-)textconv difference in format-patch.

-- 
Mischa
Junio C Hamano· Sep 18, 2012, 03:57 UTC · re: Mischa POSLAWSKY · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

Mischa POSLAWSKY <git@shiar.nl> writes:
Show 7 quoted lines
>> Perhaps you had configured your diff.noprefix in a wrong
>> configuration file?  This is primarily per-project choice, and your
>> clone of git.git should not have diff.noprefix set, neither your
>> $HOME/.gitconfig unless you always work on projects that want
>> diff.noprefix.
>
> Then I'm not using it as intended.
I would imagine that you could do
	(in ~/.gitconfig)
	[diff] noprefix
        (in ~/git.git/.git/config
        [diff] noprefix = false
or something.
Bert Wesarg· Sep 18, 2012, 09:00 UTC · re: Mischa POSLAWSKY · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

On Tue, Sep 18, 2012 at 4:52 AM, Mischa POSLAWSKY <git@shiar.nl> wrote:
Show 38 quoted lines
> Junio C Hamano skribis 2012-9-16 22:22 (-0700):
>
>> Mischa POSLAWSKY <git@shiar.nl> writes:
>>
>> > Subject: [PATCH/RFC] format-patch: force default file prefixes in diff
>> >
>> > Override user configuration (eg. diff.noprefix) in patches intended for
>> > external consumption to match the default prefixes expected by git-am.
>> >
>> > Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
>> > ---
>>
>> Not all projects expect to see a/ & b/ prefix and these are
>> configurable for a reason.  Robbing the choice that has been
>> supported for quite a long time from them is an unacceptable
>> regression.
>
> My bad, I was assuming format-patch would mostly interact with git am.
>
>> Why did you think this may be a good idea in the first place?
>>
>> Perhaps you had configured your diff.noprefix in a wrong
>> configuration file?  This is primarily per-project choice, and your
>> clone of git.git should not have diff.noprefix set, neither your
>> $HOME/.gitconfig unless you always work on projects that want
>> diff.noprefix.
>
> Then I'm not using it as intended.  For me it's just a personal
> preference of how I'd like to review commits (diff/show) so I can easily
> copy-paste file names (less essential since my discovery of git jump,
> but still).  It's not something I'd like to be communicated with any
> upstream project (format-patch).
>
> So it seems I'm asking for a new feature: to be able to configure local
> and inter-project diff options differently.  In this case I'd be helped
> by either format.noprefix=0 or a to be bikeshedded localdiff.noprefix=1.
> I don't know about other options though.  Does anybody actually want
> mnemonicprefix to be sent out as well?
I once had the same idea and posted a patch for it:
http://article.gmane.org/gmane.comp.version-control.git/146215

The implementation differs though, the pure path without any mnemonicprefix is in its own line 'path <path>'. But my current patch also differs to this old one, in the current one, the path is at the end 'index' line. But I do not need this feature anymore.

Bert
Show 7 quoted lines
>
> Another solution could be a single option defining behaviour exceptions:
> format.diff = normal | textconv | noconfig
> Expanding on the existing --(no-)textconv difference in format-patch.
>
> --
> Mischa
perryh@pluto.rain.com· Sep 17, 2012, 06:03 UTC · re: Mischa POSLAWSKY · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

Mischa POSLAWSKY <git@shiar.nl> wrote:
> ... I would argue against diff options creating non-standard patches.
Seems to me it might depend on what one means by "non-standard".

I can envision cases in which increasing the number of context lines would result in the patch being more robust WRT applying correctly to a recipient's version that might be a bit different than the one against which it was created.

OTOH one most likely does not want to create a patch with -b unless the apply tool also supports such and there is a way to communicate to the apply tool that -b was used in creating the patch.

Junio C Hamano· Sep 17, 2012, 05:24 UTC · re: Mischa POSLAWSKY · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

Mischa POSLAWSKY <git@shiar.nl> writes:
Show 26 quoted lines
> Matching the default file prefix b/ does not yield any results if config
> option diff.noprefix or diff.mnemonicprefix is enabled.
>
> Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
> ---
> Very useful script otherwise; thanks.
>
>  contrib/git-jump/git-jump | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git contrib/git-jump/git-jump contrib/git-jump/git-jump
> index a33674e..dc90cd6 100755
> --- contrib/git-jump/git-jump
> +++ contrib/git-jump/git-jump
> @@ -21,9 +21,9 @@ open_editor() {
>  }
>  
>  mode_diff() {
> -	git diff --relative "$@" |
> +	git diff --no-prefix --relative "$@" |
>  	perl -ne '
> -	if (m{^\+\+\+ b/(.*)}) { $file = $1; next }
> +	if (m{^\+\+\+ (.*)}) { $file = $1; next }
>  	defined($file) or next;
>  	if (m/^@@ .*\+(\d+)/) { $line = $1; next }
>  	defined($line) or next;
Makes sense to me.  Peff?
Jeff King· Sep 17, 2012, 17:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

On Sun, Sep 16, 2012 at 10:24:04PM -0700, Junio C Hamano wrote:
Show 30 quoted lines
> Mischa POSLAWSKY <git@shiar.nl> writes:
> 
> > Matching the default file prefix b/ does not yield any results if config
> > option diff.noprefix or diff.mnemonicprefix is enabled.
> >
> > Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
> > ---
> > Very useful script otherwise; thanks.
> >
> >  contrib/git-jump/git-jump | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git contrib/git-jump/git-jump contrib/git-jump/git-jump
> > index a33674e..dc90cd6 100755
> > --- contrib/git-jump/git-jump
> > +++ contrib/git-jump/git-jump
> > @@ -21,9 +21,9 @@ open_editor() {
> >  }
> >  
> >  mode_diff() {
> > -	git diff --relative "$@" |
> > +	git diff --no-prefix --relative "$@" |
> >  	perl -ne '
> > -	if (m{^\+\+\+ b/(.*)}) { $file = $1; next }
> > +	if (m{^\+\+\+ (.*)}) { $file = $1; next }
> >  	defined($file) or next;
> >  	if (m/^@@ .*\+(\d+)/) { $line = $1; next }
> >  	defined($line) or next;
> 
> Makes sense to me.  Peff?
Yes, looks obviously correct. Thanks.
Acked-by: Jeff King <peff@peff.net>
-Peff
Junio C Hamano· Sep 17, 2012, 19:48 UTC · re: Jeff King · lore

Re: [PATCH] git-jump: ignore (custom) prefix in diff mode

Jeff King <peff@peff.net> writes:
Show 26 quoted lines
> On Sun, Sep 16, 2012 at 10:24:04PM -0700, Junio C Hamano wrote:
>
>> Mischa POSLAWSKY <git@shiar.nl> writes:
>> 
>> > Matching the default file prefix b/ does not yield any results if config
>> > option diff.noprefix or diff.mnemonicprefix is enabled.
>> >
>> > Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
>> > ---
>> > Very useful script otherwise; thanks.
>> >
>> >  contrib/git-jump/git-jump | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git contrib/git-jump/git-jump contrib/git-jump/git-jump
>> > index a33674e..dc90cd6 100755
>> > --- contrib/git-jump/git-jump
>> > +++ contrib/git-jump/git-jump
>> > @@ -21,9 +21,9 @@ open_editor() {
>> >  ...
>> 
>> Makes sense to me.  Peff?
>
> Yes, looks obviously correct. Thanks.
>
> Acked-by: Jeff King <peff@peff.net>
Thanks.
It may not be obvious to many people, so here is tip of the day.

I knew Mischa knew that the patch was prepared with wrong src/dst prefix (notice the lack of a/ and b/), but I did not say anything special when I drove "git am". I just did the usual "git am -s3c" and the patch was applied just fine ;-)

What is happening is that:
 - I didn't give '-p0" to "git am", so it thought that patch was
   based on a tree that has "git-jump" directory at the top-level
   of the working tree, and the file that is being patched lived at
   "git-jump/git-jump" in Mischa's working tree;
 - However, the official git.git tree has the corresponding file at
   "contrib/git-jump/git-jump", and there is no such file at
   "git-jump/git-jump".
 - The three-way merge logic kicks in because of the "-3" option,
   using a (fake) tree that is shaped like Mischa's tree with the
   version before the patch as a common ancestor, to merge the
   version "git am" thought Mischa has (i.e. no contrib/ directory)
   and the version in my tree.  From the point of view of the
   three-way merge logic, I renamed the path to be in contrib/
   directory while Mischa kept the path intact and fixed the
   contents of the script.  This merges cleanly to produce the
   expected result.

← back to recent threads