{"thread":{"id":"53602","subject":"[PATCH] format-patch: generate valid patch with diff.noprefix config","startedAt":"2020-06-02T20:49:30Z","lastAt":"2020-06-04T23:26:25Z","messageCount":7,"participants":["Laurent Arnoud","Junio C Hamano","Đoàn Trần Công Danh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"399051","messageId":"20200602204924.GA1853335@spk-laptop","threadId":"53602","inReplyTo":null,"subject":"[PATCH] format-patch: generate valid patch with diff.noprefix config","fromName":"Laurent Arnoud","fromEmail":"laurent@spkdev.net","sentAt":"2020-06-02T20:49:24Z","receivedAt":"2020-06-02T20:49:30Z","isPatch":true,"sender":{"key":"laurent@spkdev.net","avatar":"https://avatars.githubusercontent.com/u/98590?v=4"},"body":"With diff.noprefix enabled the patch generated with format-patch does not\ninclude prefix a/ and b/ so not applicable with `git am`.\nSolution is to force a_prefix and b_prefix on diffopt.\n\nSigned-off-by: Laurent Arnoud \n---\n builtin/log.c | 2 ++\n t/t4014-format-patch.sh | 8 ++++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex d104d5c688..ca63f8ceda 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1744,6 +1744,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trev.diff = 1;\n \trev.max_parents = 1;\n \trev.diffopt.flags.recursive = 1;\n+\trev.diffopt.a_prefix = \"a/\";\n+\trev.diffopt.b_prefix = \"b/\";\n \trev.subject_prefix = fmt_patch_subject_prefix;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.def = \"HEAD\";\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex db7e733af9..5d7930e106 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1602,6 +1602,14 @@ test_expect_success 'format patch ignores color.ui' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'format patch ignores diff.noprefix' '\n+\ttest_unconfig diff.noprefix &&\n+\tgit format-patch --stdout -1 >expect &&\n+\ttest_config diff.noprefix true &&\n+\tgit format-patch --stdout -1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cover letter with invalid --cover-from-description and config' '\n \ttest_config branch.rebuild-1.description \"config subject\n \n-- \n2.27.0.rc2.129.g29f2dd231a\n"},{"id":"399052","messageId":"xmqqpnahrx2y.fsf@gitster.c.googlers.com","threadId":"53602","inReplyTo":"20200602204924.GA1853335@spk-laptop","subject":"Re: [PATCH] format-patch: generate valid patch with diff.noprefix config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-02T21:12:37Z","receivedAt":"2020-06-02T21:12:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Laurent Arnoud writes:\n\n> With diff.noprefix enabled the patch generated with format-patch does not\n> include prefix a/ and b/ so not applicable with `git am`.\n\nSome projects (not this one) do not want to see a/ and b/ prefix in\ntheir patches, and noprefix is an option for those who needs to give\npatches to such projects. As \"am\" can be told with -p to accept\nsuch a patch just fine, I do not think this change is appropriate.\n\n> Solution is to force a_prefix and b_prefix on diffopt.\n\nSorry, I do not think there is any problem to be solved here ;-)\n\n\n"},{"id":"399054","messageId":"20200602213351.GB1853335@spk-laptop","threadId":"53602","inReplyTo":"xmqqpnahrx2y.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] format-patch: generate valid patch with diff.noprefix config","fromName":"Laurent Arnoud","fromEmail":"laurent@spkdev.net","sentAt":"2020-06-02T21:33:51Z","receivedAt":"2020-06-02T21:33:54Z","isPatch":true,"sender":{"key":"laurent@spkdev.net","avatar":"https://avatars.githubusercontent.com/u/98590?v=4"},"body":"On Tue, Jun 02, 2020 at 02:12:37PM -0700, Junio C Hamano wrote:\n> Some projects (not this one) do not want to see a/ and b/ prefix in\n> their patches, and noprefix is an option for those who needs to give\n> patches to such projects. As \"am\" can be told with -p to accept\n> such a patch just fine, I do not think this change is appropriate.\n> \n> > Solution is to force a_prefix and b_prefix on diffopt.\n> \n> Sorry, I do not think there is any problem to be solved here ;-)\n\nHaha I see thanks for the explanations\n\n-- \nLaurent\n"},{"id":"399055","messageId":"xmqqeeqxrufz.fsf@gitster.c.googlers.com","threadId":"53602","inReplyTo":"xmqqpnahrx2y.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] format-patch: generate valid patch with diff.noprefix config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-02T22:09:36Z","receivedAt":"2020-06-02T22:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Laurent Arnoud writes:\n>\n>> With diff.noprefix enabled the patch generated with format-patch does not\n>> include prefix a/ and b/ so not applicable with `git am`.\n>\n> Some projects (not this one) do not want to see a/ and b/ prefix in\n> their patches, and noprefix is an option for those who needs to give\n> patches to such projects. As \"am\" can be told with -p to accept\n> such a patch just fine, I do not think this change is appropriate.\n>\n>> Solution is to force a_prefix and b_prefix on diffopt.\n>\n> Sorry, I do not think there is any problem to be solved here ;-)\n\nHaving said all that, we seem to be letting more configurations for\nthe diff.* family to affect the format-patch command. The latest\naddition was \"diff.relative\"---together with the \"diff.noprefix\"\nthat triggered this thread, some users may feel that it is a bug to\nallow these configuration variables to affect the output from the\n\"log -p\", \"show\", and \"format-patch\" commands. \n\nIt *might* make sense to introduce log.diff.* configuration\nvariables so that when they are set, they are used by\nlog/show/format-patch instead of diff.* counterparts, or something\nalong those lines. I dunno.\n\n"},{"id":"399148","messageId":"20200604193238.GA1911544@spk-laptop","threadId":"53602","inReplyTo":"xmqqeeqxrufz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] format-patch: generate valid patch with diff.noprefix config","fromName":"Laurent Arnoud","fromEmail":"laurent@spkdev.net","sentAt":"2020-06-04T19:32:38Z","receivedAt":"2020-06-04T19:32:44Z","isPatch":true,"sender":{"key":"laurent@spkdev.net","avatar":"https://avatars.githubusercontent.com/u/98590?v=4"},"body":"On Tue, Jun 02, 2020 at 03:09:36PM -0700, Junio C Hamano wrote:\n> Having said all that, we seem to be letting more configurations for\n> the diff.* family to affect the format-patch command. The latest\n> addition was \"diff.relative\"---together with the \"diff.noprefix\"\n> that triggered this thread, some users may feel that it is a bug to\n> allow these configuration variables to affect the output from the\n> \"log -p\", \"show\", and \"format-patch\" commands. \n> \n> It *might* make sense to introduce log.diff.* configuration\n> variables so that when they are set, they are used by\n> log/show/format-patch instead of diff.* counterparts, or something\n> along those lines. I dunno.\n\nI don't know if its a bug but I founded strange that I needed to use an alias\n\"git -c diff.noprefix=false format-patch\" to generate a patch that I can apply\ndirectly with \"git am\". I didn't know the -p option but to send a patch to a\nmailinglist it should have the prefix I guess ?\nI can try to implement this option if you think it can get merged\n\n-- \nLaurent\n"},{"id":"399150","messageId":"xmqq367aposu.fsf@gitster.c.googlers.com","threadId":"53602","inReplyTo":"20200604193238.GA1911544@spk-laptop","subject":"Re: [PATCH] format-patch: generate valid patch with diff.noprefix config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-06-04T20:18:57Z","receivedAt":"2020-06-04T20:19:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Laurent Arnoud writes:\n\n> I don't know if its a bug but I founded strange that I needed to use an alias\n> \"git -c diff.noprefix=false format-patch\" to generate a patch that I can apply\n> directly with \"git am\".\n\nThe same thing can be said about the diff.relative option.\n\nAs I do not use either of these variables myself, I am somewhat\nindifferent, if those who set these variables find the consequences\nof doing so unpleasant. As people often say, if it hurts, then...\n;-)\n\nBecause the recipient of format-patch output who consumes it is\ntypically different from the one who generates it, it probably does\nnot make much sense to attempt linking diff.noprefix=true with the\n-p0 option (there isn't even a configuration for 'git am -p',\nif I am not mistaken).\n\nDepending on the project a user works with, allowing\nlog/show/format-patch to honor certain diff.* configuration\nvariables, without a way to countermand them with more specific\nconfiguration for log/show/format-patch, may smell like a bug. \n\nI am not sure where to draw the line, though. Would we treat only\nformat-patch and no other commands in the log family specially?\nWould we treat each commands in the log family specially and\nseparately, so that you could say \"diff and show uses noprefix, but\n'log -p' and 'whatchanged' uses the standard a/ and b/ prefix and\nformat-patch uses old/ and new/ prefix\" independently?\n\n> I didn't know the -p option but to send a patch to a\n> mailinglist it should have the prefix I guess ?\n\nThe participants in this project would certainly find it unusual\nwhen they see a prefix-less patch.\n\nThere probably are projects older than Git whose convention is to\nuse .noprefix; we didn't want to force them to switch and instead\naccomodated their preference with .noprefix but in hindsight, it may\nhave been a better idea to force one true way to everybody, which\nwould have kept the world simpler. I dunno.\n\n\n"},{"id":"399154","messageId":"20200604232619.GA6329@danh.dev","threadId":"53602","inReplyTo":"xmqq367aposu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] format-patch: generate valid patch with diff.noprefix config","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2020-06-04T23:26:19Z","receivedAt":"2020-06-04T23:26:25Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2020-06-04 13:18:57-0700, Junio C Hamano wrote:\n> Laurent Arnoud writes:\n> \n> > I don't know if its a bug but I founded strange that I needed to use an alias\n> > \"git -c diff.noprefix=false format-patch\" to generate a patch that I can apply\n> > directly with \"git am\".\n> \n> The same thing can be said about the diff.relative option.\n> \n> As I do not use either of these variables myself, I am somewhat\n> indifferent, if those who set these variables find the consequences\n> of doing so unpleasant. As people often say, if it hurts, then...\n> ;-)\n\nFWIW, people need to dig into git-config(1) to know diff.noprefix\nWe don't mention it in git-diff(1). Me neither until today.\n\n> Because the recipient of format-patch output who consumes it is\n> typically different from the one who generates it, it probably does\n> not make much sense to attempt linking diff.noprefix=true with the\n> -p0 option (there isn't even a configuration for 'git am -p',\n> if I am not mistaken).\n\nCould we extend the patch in someway that can hint git-am(1) to\nuse \"-p0\" if patch was generated by --no-prefix?\nI couldn't come up with any idea right now, since all parts of the\npatch is very busy with a lot of information.\nAn RFC 822 header, perhap?\n\nOf course old git-am(1) couldn't understand the extension.\nAnd the new git-am may come up with false positive?\n\n> Depending on the project a user works with, allowing\n> log/show/format-patch to honor certain diff.* configuration\n> variables, without a way to countermand them with more specific\n> configuration for log/show/format-patch, may smell like a bug. \n> \n> I am not sure where to draw the line, though. Would we treat only\n> format-patch and no other commands in the log family specially?\n> Would we treat each commands in the log family specially and\n> separately, so that you could say \"diff and show uses noprefix, but\n> 'log -p' and 'whatchanged' uses the standard a/ and b/ prefix and\n> format-patch uses old/ and new/ prefix\" independently?\n> \n> > I didn't know the -p option but to send a patch to a\n> > mailinglist it should have the prefix I guess ?\n> \n> The participants in this project would certainly find it unusual\n> when they see a prefix-less patch.\n> \n> There probably are projects older than Git whose convention is to\n> use .noprefix; we didn't want to force them to switch and instead\n> accomodated their preference with .noprefix but in hindsight, it may\n> have been a better idea to force one true way to everybody, which\n> would have kept the world simpler. I dunno.\n\nI do use format-patch --no-prefix.\nIt's unlikely that I will use diff.noprefix, though.\nI think other people also have different preferences.\n\n-- \nDanh\n"}]}