[PATCH] format-patch: generate valid patch with diff.noprefix config

[PATCH] format-patch: generate valid patch with diff.noprefix config

[Laurent Arnoud]

With diff.noprefix enabled the patch generated with format-patch does not
include prefix a/ and b/ so not applicable with `git am`.
Solution is to force a_prefix and b_prefix on diffopt.

Signed-off-by: Laurent Arnoud <laurent@spkdev.net>
---
 builtin/log.c           | 2 ++
 t/t4014-format-patch.sh | 8 ++++++++
 2 files changed, 10 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index d104d5c688..ca63f8ceda 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1744,6 +1744,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.diff = 1;
 	rev.max_parents = 1;
 	rev.diffopt.flags.recursive = 1;
+	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";
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index db7e733af9..5d7930e106 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1602,6 +1602,14 @@ test_expect_success 'format patch ignores color.ui' '
 	test_cmp expect actual
 '
 
+test_expect_success 'format patch ignores diff.noprefix' '
+	test_unconfig diff.noprefix &&
+	git format-patch --stdout -1 >expect &&
+	test_config diff.noprefix true &&
+	git format-patch --stdout -1 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cover letter with invalid --cover-from-description and config' '
 	test_config branch.rebuild-1.description "config subject
 
-- 
2.27.0.rc2.129.g29f2dd231a

Re: [PATCH] format-patch: generate valid patch with diff.noprefix config

[Junio C Hamano]

Laurent Arnoud <laurent@spkdev.net> writes:

> With diff.noprefix enabled the patch generated with format-patch does not
> include prefix a/ and b/ so not applicable with `git am`.

Some projects (not this one) do not want to see a/ and b/ prefix in
their patches, and noprefix is an option for those who needs to give
patches to such projects.  As "am" can be told with -p<num> to accept
such a patch just fine, I do not think this change is appropriate.

> Solution is to force a_prefix and b_prefix on diffopt.

Sorry, I do not think there is any problem to be solved here ;-)

Re: [PATCH] format-patch: generate valid patch with diff.noprefix config

[Laurent Arnoud]

On Tue, Jun 02, 2020 at 02:12:37PM -0700, Junio C Hamano wrote:
> Some projects (not this one) do not want to see a/ and b/ prefix in
> their patches, and noprefix is an option for those who needs to give
> patches to such projects.  As "am" can be told with -p<num> to accept
> such a patch just fine, I do not think this change is appropriate.
> 
> > Solution is to force a_prefix and b_prefix on diffopt.
> 
> Sorry, I do not think there is any problem to be solved here ;-)

Haha I see thanks for the explanations

-- 
Laurent

Re: [PATCH] format-patch: generate valid patch with diff.noprefix config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Laurent Arnoud <laurent@spkdev.net> writes:
>
>> With diff.noprefix enabled the patch generated with format-patch does not
>> include prefix a/ and b/ so not applicable with `git am`.
>
> Some projects (not this one) do not want to see a/ and b/ prefix in
> their patches, and noprefix is an option for those who needs to give
> patches to such projects.  As "am" can be told with -p<num> to accept
> such a patch just fine, I do not think this change is appropriate.
>
>> Solution is to force a_prefix and b_prefix on diffopt.
>
> Sorry, I do not think there is any problem to be solved here ;-)

Having said all that, we seem to be letting more configurations for
the diff.* family to affect the format-patch command.  The latest
addition was "diff.relative"---together with the "diff.noprefix"
that triggered this thread, some users may feel that it is a bug to
allow these configuration variables to affect the output from the
"log -p", "show", and "format-patch" commands.  

It *might* make sense to introduce log.diff.* configuration
variables so that when they are set, they are used by
log/show/format-patch instead of diff.* counterparts, or something
along those lines.  I dunno.

Re: [PATCH] format-patch: generate valid patch with diff.noprefix config

[Laurent Arnoud]

On Tue, Jun 02, 2020 at 03:09:36PM -0700, Junio C Hamano wrote:
> Having said all that, we seem to be letting more configurations for
> the diff.* family to affect the format-patch command.  The latest
> addition was "diff.relative"---together with the "diff.noprefix"
> that triggered this thread, some users may feel that it is a bug to
> allow these configuration variables to affect the output from the
> "log -p", "show", and "format-patch" commands.  
> 
> It *might* make sense to introduce log.diff.* configuration
> variables so that when they are set, they are used by
> log/show/format-patch instead of diff.* counterparts, or something
> along those lines.  I dunno.

I don't know if its a bug but I founded strange that I needed to use an alias
"git -c diff.noprefix=false format-patch" to generate a patch that I can apply
directly with "git am". I didn't know the -p option but to send a patch to a
mailinglist it should have the prefix I guess ?
I can try to implement this option if you think it can get merged

-- 
Laurent

Re: [PATCH] format-patch: generate valid patch with diff.noprefix config

[Junio C Hamano]

Laurent Arnoud <laurent@spkdev.net> writes:

> I don't know if its a bug but I founded strange that I needed to use an alias
> "git -c diff.noprefix=false format-patch" to generate a patch that I can apply
> directly with "git am".

The same thing can be said about the diff.relative option.

As I do not use either of these variables myself, I am somewhat
indifferent, if those who set these variables find the consequences
of doing so unpleasant.  As people often say, if it hurts, then...
;-)

Because the recipient of format-patch output who consumes it is
typically different from the one who generates it, it probably does
not make much sense to attempt linking diff.noprefix=true with the
-p0 option (there isn't even a configuration for 'git am -p<num>',
if I am not mistaken).

Depending on the project a user works with, allowing
log/show/format-patch to honor certain diff.* configuration
variables, without a way to countermand them with more specific
configuration for log/show/format-patch, may smell like a bug.  

I am not sure where to draw the line, though.  Would we treat only
format-patch and no other commands in the log family specially?
Would we treat each commands in the log family specially and
separately, so that you could say "diff and show uses noprefix, but
'log -p' and 'whatchanged' uses the standard a/ and b/ prefix and
format-patch uses old/ and new/ prefix" independently?

> I didn't know the -p option but to send a patch to a
> mailinglist it should have the prefix I guess ?

The participants in this project would certainly find it unusual
when they see a prefix-less patch.

There probably are projects older than Git whose convention is to
use .noprefix; we didn't want to force them to switch and instead
accomodated their preference with .noprefix but in hindsight, it may
have been a better idea to force one true way to everybody, which
would have kept the world simpler.  I dunno.

Re: [PATCH] format-patch: generate valid patch with diff.noprefix config

[Đoàn Trần Công Danh]

On 2020-06-04 13:18:57-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Laurent Arnoud <laurent@spkdev.net> writes:
> 
> > I don't know if its a bug but I founded strange that I needed to use an alias
> > "git -c diff.noprefix=false format-patch" to generate a patch that I can apply
> > directly with "git am".
> 
> The same thing can be said about the diff.relative option.
> 
> As I do not use either of these variables myself, I am somewhat
> indifferent, if those who set these variables find the consequences
> of doing so unpleasant.  As people often say, if it hurts, then...
> ;-)

FWIW, people need to dig into git-config(1) to know diff.noprefix
We don't mention it in git-diff(1). Me neither until today.

> Because the recipient of format-patch output who consumes it is
> typically different from the one who generates it, it probably does
> not make much sense to attempt linking diff.noprefix=true with the
> -p0 option (there isn't even a configuration for 'git am -p<num>',
> if I am not mistaken).

Could we extend the patch in someway that can hint git-am(1) to
use "-p0" if patch was generated by --no-prefix?
I couldn't come up with any idea right now, since all parts of the
patch is very busy with a lot of information.
An RFC 822 header, perhap?

Of course old git-am(1) couldn't understand the extension.
And the new git-am may come up with false positive?

> Depending on the project a user works with, allowing
> log/show/format-patch to honor certain diff.* configuration
> variables, without a way to countermand them with more specific
> configuration for log/show/format-patch, may smell like a bug.  
> 
> I am not sure where to draw the line, though.  Would we treat only
> format-patch and no other commands in the log family specially?
> Would we treat each commands in the log family specially and
> separately, so that you could say "diff and show uses noprefix, but
> 'log -p' and 'whatchanged' uses the standard a/ and b/ prefix and
> format-patch uses old/ and new/ prefix" independently?
> 
> > I didn't know the -p option but to send a patch to a
> > mailinglist it should have the prefix I guess ?
> 
> The participants in this project would certainly find it unusual
> when they see a prefix-less patch.
> 
> There probably are projects older than Git whose convention is to
> use .noprefix; we didn't want to force them to switch and instead
> accomodated their preference with .noprefix but in hindsight, it may
> have been a better idea to force one true way to everybody, which
> would have kept the world simpler.  I dunno.

I do use format-patch --no-prefix.
It's unlikely that I will use diff.noprefix, though.
I think other people also have different preferences.

-- 
Danh