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

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

[Mischa POSLAWSKY]

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;
-- 
1.7.12.165.g8cb9d9c

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

[Mischa POSLAWSKY]

> 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(+)

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

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

[Junio C Hamano]

Mischa POSLAWSKY <git@shiar.nl> writes:

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

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

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

[Junio C Hamano]

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?

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

[perryh]

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.

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

[Jeff King]

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() {
> >  }
> >  
> >  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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Mischa POSLAWSKY]

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?

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

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

[Junio C Hamano]

Mischa POSLAWSKY <git@shiar.nl> writes:

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

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

[Bert Wesarg]

On Tue, Sep 18, 2012 at 4:52 AM, Mischa POSLAWSKY <git@shiar.nl> wrote:
> 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

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