{"thread":{"id":"21766","subject":"[PATCH 0/2] format-patch: fix dashdash","startedAt":"2009-11-26T19:11:58Z","lastAt":"2009-11-28T11:18:25Z","messageCount":11,"participants":["Felipe Contreras","Junio C Hamano","Björn Steinbrink"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"128490","messageId":"1259262720-24077-1-git-send-email-felipe.contreras@gmail.com","threadId":"21766","inReplyTo":null,"subject":"[PATCH 0/2] format-patch: fix dashdash","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-11-26T19:11:58Z","receivedAt":"2009-11-26T19:11:58Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This doesn't work\ngit format-patch -- \n\nSo here are some fixes and a test.\n\nFelipe Contreras (2):\n format-patch: fix dashdash usage\n format-patch: add test for dashdash\n\n builtin-log.c | 3 ++-\n t/t4014-format-patch.sh | 4 ++++\n 2 files changed, 6 insertions(+), 1 deletions(-)\n"},{"id":"128491","messageId":"1259262720-24077-2-git-send-email-felipe.contreras@gmail.com","threadId":"21766","inReplyTo":"1259262720-24077-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/2] format-patch: fix dashdash usage","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-11-26T19:11:59Z","receivedAt":"2009-11-26T19:11:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Otherwise 'git format-patch -- ' doesn't\nwork.\n\nSigned-off-by: Felipe Contreras \n---\n builtin-log.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 33fa6ea..1766349 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -976,7 +976,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t */\n \targc = parse_options(argc, argv, prefix, builtin_format_patch_options,\n \t\t\t builtin_format_patch_usage,\n-\t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);\n+\t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n+\t\t\t PARSE_OPT_KEEP_DASHDASH);\n \n \tif (do_signoff) {\n \t\tconst char *committer;\n-- \n1.6.6.rc0.61.geeb75\n"},{"id":"128492","messageId":"1259262720-24077-3-git-send-email-felipe.contreras@gmail.com","threadId":"21766","inReplyTo":"1259262720-24077-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/2] format-patch: add test for dashdash","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-11-26T19:12:00Z","receivedAt":"2009-11-26T19:12:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n t/t4014-format-patch.sh | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7f267f9..d5b002d 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -536,6 +536,10 @@ test_expect_success 'format-patch --signoff' '\n \tgrep \"^Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n '\n \n+test_expect_success 'format-patch -- ' '\n+\tgit format-patch master..side -- file\n+'\n+\n echo \"fatal: --name-only does not make sense\" > expect.name-only\n echo \"fatal: --name-status does not make sense\" > expect.name-status\n echo \"fatal: --check does not make sense\" > expect.check\n-- \n1.6.6.rc0.61.geeb75\n"},{"id":"128495","messageId":"7vd4355aaw.fsf@alter.siamese.dyndns.org","threadId":"21766","inReplyTo":"1259262720-24077-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/2] format-patch: fix dashdash usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-26T19:57:11Z","receivedAt":"2009-11-26T19:57:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> Otherwise 'git format-patch -- ' doesn't\n> work.\n\nInstead of \"doesn't work\", I really wished you wrote something like:\n\n $ git format-patch -- \n\n complains that does not exist in the current work tree and the\n user needs to explicitly specify \"--\", even though the user _did_ give\n a \"--\". This is because it incorrectly removes \"--\" from the command\n line arguments that is later passed to setup_revisions().\n\nRemember that you are trying to help somebody who has to write Release\nNotes out of \"git log\" output.\n\nI actually have a bigger question, though. Does it even make sense to\nallow pathspecs to format-patch? We sure are currently loose and take\nthem, but I doubt it is by design.\n\nThe patch itself looks good and is a candidate 'maint' material, if the\nanswer to the above question is a convincing \"yes, because ...\".\n"},{"id":"128512","messageId":"94a0d4530911261414o533aa108l202d4c6926da361e@mail.gmail.com","threadId":"21766","inReplyTo":"7vd4355aaw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] format-patch: fix dashdash usage","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-11-26T22:14:29Z","receivedAt":"2009-11-26T22:14:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> Otherwise 'git format-patch -- ' doesn't\n>> work.\n>\n> Instead of \"doesn't work\", I really wished you wrote something like:\n>\n>    $ git format-patch -- \n>\n>    complains that does not exist in the current work tree and the\n>    user needs to explicitly specify \"--\", even though the user _did_ give\n>    a \"--\".  This is because it incorrectly removes \"--\" from the command\n>    line arguments that is later passed to setup_revisions().\n\nComplaining is one thing... failing to do anything is another.\n\n> Remember that you are trying to help somebody who has to write Release\n> Notes out of \"git log\" output.\n>\n> I actually have a bigger question, though.  Does it even make sense to\n> allow pathspecs to format-patch?  We sure are currently loose and take\n> them, but I doubt it is by design.\n\nNot everyone has clean branches only with pertinent patches.\n\nI stumbled upon this trying to re-create (cleanly) a \"branch\" that was\nconstantly merged into another \"master\" branch that had a lot more\nstuff. Maybe there was a smarter way to do that with 'git rebase', but\nthat doesn't mean format-patch -- shouldn't work.\n\n> The patch itself looks good and is a candidate 'maint' material, if the\n> answer to the above question is a convincing \"yes, because ...\".\n\nYeah, I also think this should go into 'maint'.\n\n-- \nFelipe Contreras\n"},{"id":"128513","messageId":"7v7htc3mqo.fsf@alter.siamese.dyndns.org","threadId":"21766","inReplyTo":"94a0d4530911261414o533aa108l202d4c6926da361e@mail.gmail.com","subject":"Re: [PATCH 1/2] format-patch: fix dashdash usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-26T23:11:27Z","receivedAt":"2009-11-26T23:11:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano wrote:\n>> Felipe Contreras writes:\n>>\n>>> Otherwise 'git format-patch -- ' doesn't\n>>> work.\n>>\n>> Instead of \"doesn't work\", I really wished you wrote something like:\n>>\n>>    $ git format-patch -- \n>>\n>>    complains that does not exist in the current work tree and the\n>>    user needs to explicitly specify \"--\", even though the user _did_ give\n>>    a \"--\".  This is because it incorrectly removes \"--\" from the command\n>>    line arguments that is later passed to setup_revisions().\n>\n> Complaining is one thing... failing to do anything is another.\n\nOh, I didn't mean to say \"tone down from doesn't-work to complains\".\n\n Instead of \"doesn't work\" and saying nothing else useful to describe\n the nature of the problem you are addressing, I really wished you\n wrote something that has enough details like the sample explaination I\n gave you has.\n\nIs it clearer what I meant? More importantly, did I get the details\nright?\n\n>> I actually have a bigger question, though.  Does it even make sense to\n>> allow pathspecs to format-patch?  We sure are currently loose and take\n>> them, but I doubt it is by design.\n>\n> Not everyone has clean branches only with pertinent patches.\n>\n> I stumbled upon this trying to re-create (cleanly) a \"branch\" that was\n> constantly merged into another \"master\" branch that had a lot more\n> stuff. Maybe there was a smarter way to do that with 'git rebase', but\n> that doesn't mean format-patch -- shouldn't work.\n>\n>> The patch itself looks good and is a candidate 'maint' material, if the\n>> answer to the above question is a convincing \"yes, because ...\".\n>\n> Yeah, I also think this should go into 'maint'.\n\nHmm, I have not seen a clear \"yes, because...\" yet.\n\nFor one thing, Documentation/git-format-patch.txt does not even hint that\nyou can give pathspecs. builtin_format_patch_usage[] doesn't, either. As\nI wrote the initial version of format-patch I can say with some authority\nthat use with pathspecs were never meant to be supported---if it works, it\nworks by accident, giving long enough rope to users to potentially cause\nthemselves harm.\n\nI am inclined to think that we shouldn't encourage use of pathspecs (just\nlike we never encouraged use of options like --name-only that never makes\nsense in the context of the command) but I am undecided if we also should\nforbid the use of pathspecs (just like we did for --name-only recently).\n\nAn appropriate patch that should go to 'maint' _might_ even be something\nlike this in addition to your patch, if we decide to be consistent.\n\n---\n builtin-log.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex 33fa6ea..3a9bc69 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -1036,6 +1036,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \targc = setup_revisions(argc, argv, &rev, \"HEAD\");\n \tif (argc > 1)\n \t\tdie (\"unrecognized argument: %s\", argv[1]);\n+\tif (rev.prune_data)\n+\t\tdie(\"unexpected pathspec\");\n \n \tif (rev.diffopt.output_format & DIFF_FORMAT_NAME)\n \t\tdie(\"--name-only does not make sense\");\n"},{"id":"128514","messageId":"94a0d4530911261523q25147f12h2e6c9e4fe4f6b12b@mail.gmail.com","threadId":"21766","inReplyTo":"7v7htc3mqo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] format-patch: fix dashdash usage","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-11-26T23:23:01Z","receivedAt":"2009-11-26T23:23:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Nov 27, 2009 at 1:11 AM, Junio C Hamano wrote:\n> Is it clearer what I meant?  More importantly, did I get the details\n> right?\n\nYeah, I guess so.\n\n> Hmm, I have not seen a clear \"yes, because...\" yet.\n\nI'll repeat:\nNot everyone has clean branches only with pertinent patches.\n\nThat's why revision filtering options make sense.\n\n> For one thing, Documentation/git-format-patch.txt does not even hint that\n> you can give pathspecs.  builtin_format_patch_usage[] doesn't, either.  As\n> I wrote the initial version of format-patch I can say with some authority\n> that use with pathspecs were never meant to be supported---if it works, it\n> works by accident, giving long enough rope to users to potentially cause\n> themselves harm.\n>\n> I am inclined to think that we shouldn't encourage use of pathspecs (just\n> like we never encouraged use of options like --name-only that never makes\n> sense in the context of the command) but I am undecided if we also should\n> forbid the use of pathspecs (just like we did for --name-only recently).\n\nHow about 'git format-patch --full-diff'? Isn't that a valid way to\nfilter patches just like --author, --grep, and so on?\n\n-- \nFelipe Contreras\n"},{"id":"128515","messageId":"7vws1c27ch.fsf@alter.siamese.dyndns.org","threadId":"21766","inReplyTo":"7v7htc3mqo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] format-patch: fix dashdash usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-26T23:29:18Z","receivedAt":"2009-11-26T23:29:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Felipe Contreras writes:\n> ...\n>>> I actually have a bigger question, though.  Does it even make sense to\n>>> allow pathspecs to format-patch?  We sure are currently loose and take\n>>> them, but I doubt it is by design.\n>>\n>> Not everyone has clean branches only with pertinent patches.\n>>\n>> I stumbled upon this trying to re-create (cleanly) a \"branch\" that was\n>> constantly merged into another \"master\" branch that had a lot more\n>> stuff. Maybe there was a smarter way to do that with 'git rebase', but\n>> that doesn't mean format-patch -- shouldn't work.\n>>\n>>> The patch itself looks good and is a candidate 'maint' material, if the\n>>> answer to the above question is a convincing \"yes, because ...\".\n>>\n>> Yeah, I also think this should go into 'maint'.\n>\n> Hmm, I have not seen a clear \"yes, because...\" yet.\n\nSorry, I guess I did it again of assuming the reader would read my mind.\nLet's try to be a bit more explicit.\n\nYour description defends why you think showing only part of a single\nchange in a patch form is jusitified. What I found unconvincing is that\nit does not even try to justify why it is a good idea to give the full\ndescription that explains the _whole change_, even the part to the set of\nfiles that were omitted by the pathspecs, as an applicable format-patch\noutput. And that is why I suspect that it may be a long rope that harms\nusers.\n\n> For one thing, Documentation/git-format-patch.txt does not even hint that\n> you can give pathspecs. builtin_format_patch_usage[] doesn't, either. As\n> I wrote the initial version of format-patch I can say with some authority\n> that use with pathspecs were never meant to be supported---if it works, it\n> works by accident, giving long enough rope to users to potentially cause\n> themselves harm.\n>\n> I am inclined to think that we shouldn't encourage use of pathspecs (just\n> like we never encouraged use of options like --name-only that never makes\n> sense in the context of the command) but I am undecided if we also should\n> forbid the use of pathspecs (just like we did for --name-only recently).\n\nCompared to --name-only and friends that makes the output unapplicable by\n\"git am\", a patch generated with pathspecs is worse. The output will\napply cleanly and the user can choose not to bother or forget cleaning up\nthe log message from the resulting commits.\n\nOn the other hand, if the pathspec affected only the choice of commits but\nthe command is changed in such a way that patches were always generated\nwith --full-diff option, I can understand its usefulness in the use case\nyou described. Instead of having to do \"format-patch master..branch\" and\nthen pick only the ones that are necessary by visual inspection, you would\nrun it to generate the ones that _might_ need to be applied by giving\npathspec to cover the files all relevant changes _must_ touch, only to cut\ndown the search space of your visual inspection of picking and choosing.\n\nThen at least the ones that you choose to apply are expressed in full and\nthe patch text and the description should match, and I wouldn't find it\nproblematic nor a long rope that harms users.\n\nBut without such an explanation, I can only _guess_ what the intention\nis. That is why I asked for justifications from you, as this was your\nitch.\n"},{"id":"128517","messageId":"20091126233749.GA26228@atjola.homenet","threadId":"21766","inReplyTo":"7v7htc3mqo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] format-patch: fix dashdash usage","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2009-11-26T23:37:49Z","receivedAt":"2009-11-26T23:37:49Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2009.11.26 15:11:27 -0800, Junio C Hamano wrote:\n> Felipe Contreras writes:\n> > On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano wrote:\n> >> I actually have a bigger question, though.  Does it even make sense to\n> >> allow pathspecs to format-patch?  We sure are currently loose and take\n> >> them, but I doubt it is by design.\n> >\n> > Not everyone has clean branches only with pertinent patches.\n> >\n> > I stumbled upon this trying to re-create (cleanly) a \"branch\" that was\n> > constantly merged into another \"master\" branch that had a lot more\n> > stuff. Maybe there was a smarter way to do that with 'git rebase', but\n> > that doesn't mean format-patch -- shouldn't work.\n> >\n> >> The patch itself looks good and is a candidate 'maint' material, if the\n> >> answer to the above question is a convincing \"yes, because ...\".\n> >\n> > Yeah, I also think this should go into 'maint'.\n> \n> Hmm, I have not seen a clear \"yes, because...\" yet.\n> \n> For one thing, Documentation/git-format-patch.txt does not even hint that\n> you can give pathspecs. builtin_format_patch_usage[] doesn't, either. As\n> I wrote the initial version of format-patch I can say with some authority\n> that use with pathspecs were never meant to be supported---if it works, it\n> works by accident, giving long enough rope to users to potentially cause\n> themselves harm.\n> \n> I am inclined to think that we shouldn't encourage use of pathspecs (just\n> like we never encouraged use of options like --name-only that never makes\n> sense in the context of the command) but I am undecided if we also should\n> forbid the use of pathspecs (just like we did for --name-only recently).\n\nA year ago, there was someone who had done a subtree merge and had\ncommits that changed the subtree in the \"supertree\" branch. He wanted to\ngenerate patches to send them to upstream, and ended up using\nformat-patch with --relative and pathspecs.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/101742\n\nI guess this could be done by some \"git rebase -s subtree ...\"\ninvocation though, to first get commits that sit directly on the subtree\nbranch, and then you could turn them into patches as usual... Hmm..\n\nBjörn\n"},{"id":"128519","messageId":"7vhbsg25sb.fsf@alter.siamese.dyndns.org","threadId":"21766","inReplyTo":"94a0d4530911261523q25147f12h2e6c9e4fe4f6b12b@mail.gmail.com","subject":"Re: [PATCH 1/2] format-patch: fix dashdash usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-27T00:03:00Z","receivedAt":"2009-11-27T00:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> How about 'git format-patch --full-diff'? Isn't that a valid way to\n> filter patches just like --author, --grep, and so on?\n\nOur messages obviously crossed and I think we are in agreement that\npathspec that only is used to pick which commit to show and not limits\nwhich parts of the chosen commits are shown might have some uses.\n\nIn any case, your patch we are discussing, with a proper commit log\nmessage (without discussing if it is a good idea to give pathspecs) would\nbe a good first step, regardless of which direction we end up going as the\nnext step.\n\nAs to the \"next step\", my current thinking, unless there are convincing\narguments why there should be a way to also limit the parts of the commits\nare shown, is to\n\n (0) take your patch with an updated message (eh, that is not \"next step\"\n but the \"first step\");\n\n (1) make --full-diff implicit and default of format-patch (--no-full-diff\n could be supported _if_ somebody can argue successfully why limiting\n the diff is also a useful thing to do); and\n\n (2) document clearly that format-patch takes optional pathspecs, and in\n what situation they are useful.\n\nI think (0) is 'maint' material, and with a good documentation update (1)\nand (2) could also be.\n"},{"id":"128618","messageId":"94a0d4530911280318nd6ea6d4s7ee159fe173a3f2f@mail.gmail.com","threadId":"21766","inReplyTo":"7vhbsg25sb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] format-patch: fix dashdash usage","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2009-11-28T11:18:25Z","receivedAt":"2009-11-28T11:18:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Nov 27, 2009 at 2:03 AM, Junio C Hamano wrote:\n>  (0) take your patch with an updated message (eh, that is not \"next step\"\n>     but the \"first step\");\n\nI guess it depends of what is the \"fist step\". To me the \"first step\"\nis realizing there's a problem. First patch submission is already many\nsteps afterwards.\n\nBut sure, I'll do this next step :)\n\n>  (1) make --full-diff implicit and default of format-patch (--no-full-diff\n>     could be supported _if_ somebody can argue successfully why limiting\n>     the diff is also a useful thing to do); and\n\nI started writing this patch but it turns out there's no --no-full-diff.\n\n>  (2) document clearly that format-patch takes optional pathspecs, and in\n>     what situation they are useful.\n\nI guess this would have to be done in multiple places so it might take\nme some time.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}