| Message ID | 20200602204924.GA1853335@spk-laptop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | format-patch: generate valid patch with diff.noprefix config | expand |
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 ;-)
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
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.
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 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.
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.
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
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(+)