{"thread":{"id":"31949","subject":"[PATCH] replace: parse revision argument for -d","startedAt":"2012-10-26T13:33:27Z","lastAt":"2012-11-14T13:15:19Z","messageCount":22,"participants":["Michael J Gruber","Christian Couder","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"201943","messageId":"807340e40adb1fc97cd97161fe1fabd292bc79c3.1351258394.git.git@drmicha.warpmail.net","threadId":"31949","inReplyTo":null,"subject":"[PATCH] replace: parse revision argument for -d","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-10-26T13:33:27Z","receivedAt":"2012-10-26T13:33:27Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"'git replace' parses the revision arguments when it creates replacements\n(so that a sha1 can be abbreviated, e.g.) but not when deleting\nreplacements.\n\nThis sucks.\n\nMake it parse the argument to 'replace -d' in the same way.\n\nJust in case someone lost the replacement object before deleting the\nreplacement, take the argument literally if it can not be resolved to a\nfull sha1.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n builtin/replace.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex e3aaf70..80e2039 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -46,24 +46,29 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n \n static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n {\n-\tconst char **p;\n+\tconst char **p, *q;\n \tchar ref[PATH_MAX];\n \tint had_error = 0;\n \tunsigned char sha1[20];\n \n \tfor (p = argv; *p; p++) {\n-\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n+\t\tq = *p;\n+\t\tif (get_sha1(q, sha1))\n+\t\t\twarning(\"Failed to resolve '%s' as a valid ref; taking it literally.\", q);\n+\t\telse\n+\t\t\tq = sha1_to_hex(sha1);\n+\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", q)\n \t\t\t\t\t>= sizeof(ref)) {\n-\t\t\terror(\"replace ref name too long: %.*s...\", 50, *p);\n+\t\t\terror(\"replace ref name too long: %.*s...\", 50, q);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (read_ref(ref, sha1)) {\n-\t\t\terror(\"replace ref '%s' not found.\", *p);\n+\t\t\terror(\"replace ref '%s' not found.\", q);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (fn(*p, ref, sha1))\n+\t\tif (fn(q, ref, sha1))\n \t\t\thad_error = 1;\n \t}\n \treturn had_error;\n-- \n1.8.0.370.g8cbad08\n"},{"id":"201955","messageId":"CAP8UFD3MNwpwzD-eNEHDLRSkLskXfW-tF061U=Nxc-OrTf4znw@mail.gmail.com","threadId":"31949","inReplyTo":"807340e40adb1fc97cd97161fe1fabd292bc79c3.1351258394.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] replace: parse revision argument for -d","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2012-10-26T15:25:50Z","receivedAt":"2012-10-26T15:25:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Fri, Oct 26, 2012 at 3:33 PM, Michael J Gruber\n<git@drmicha.warpmail.net> wrote:\n> 'git replace' parses the revision arguments when it creates replacements\n> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n> replacements.\n>\n> This sucks.\n>\n> Make it parse the argument to 'replace -d' in the same way.\n\nNit: there could be more than one argument to 'replace -d', so perhaps\n\"each argument\" is better.\n\n> Just in case someone lost the replacement object before deleting the\n> replacement, take the argument literally if it can not be resolved to a\n\nHere too.\n\n> full sha1.\n>\n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n>  builtin/replace.c | 15 ++++++++++-----\n>  1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index e3aaf70..80e2039 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -46,24 +46,29 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n>\n>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n>  {\n> -       const char **p;\n> +       const char **p, *q;\n>         char ref[PATH_MAX];\n>         int had_error = 0;\n>         unsigned char sha1[20];\n>\n>         for (p = argv; *p; p++) {\n> -               if (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n> +               q = *p;\n> +               if (get_sha1(q, sha1))\n> +                       warning(\"Failed to resolve '%s' as a valid ref; taking it literally.\", q);\n> +               else\n> +                       q = sha1_to_hex(sha1);\n> +               if (snprintf(ref, sizeof(ref), \"refs/replace/%s\", q)\n>                                         >= sizeof(ref)) {\n> -                       error(\"replace ref name too long: %.*s...\", 50, *p);\n> +                       error(\"replace ref name too long: %.*s...\", 50, q);\n>                         had_error = 1;\n>                         continue;\n>                 }\n>                 if (read_ref(ref, sha1)) {\n> -                       error(\"replace ref '%s' not found.\", *p);\n> +                       error(\"replace ref '%s' not found.\", q);\n>                         had_error = 1;\n>                         continue;\n>                 }\n> -               if (fn(*p, ref, sha1))\n> +               if (fn(q, ref, sha1))\n>                         had_error = 1;\n>         }\n>         return had_error;\n\nLooks good to me.\n\nThanks,\nChristian.\n"},{"id":"202093","messageId":"20121029065836.GC5102@sigill.intra.peff.net","threadId":"31949","inReplyTo":"807340e40adb1fc97cd97161fe1fabd292bc79c3.1351258394.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] replace: parse revision argument for -d","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-29T06:58:36Z","receivedAt":"2012-10-29T06:58:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote:\n\n>  \tfor (p = argv; *p; p++) {\n> -\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n> +\t\tq = *p;\n> +\t\tif (get_sha1(q, sha1))\n> +\t\t\twarning(\"Failed to resolve '%s' as a valid ref; taking it literally.\", q);\n> +\t\telse\n> +\t\t\tq = sha1_to_hex(sha1);\n\nDoesn't get_sha1 already handle this for 40-byte sha1s (and for anything\nelse, it would not work anyway)?\n\n-Peff\n"},{"id":"202104","messageId":"508E4637.2060903@drmicha.warpmail.net","threadId":"31949","inReplyTo":"20121029065836.GC5102@sigill.intra.peff.net","subject":"Re: [PATCH] replace: parse revision argument for -d","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-10-29T09:02:47Z","receivedAt":"2012-10-29T09:02:47Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 29.10.2012 07:58:\n> On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote:\n> \n>>  \tfor (p = argv; *p; p++) {\n>> -\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n>> +\t\tq = *p;\n>> +\t\tif (get_sha1(q, sha1))\n>> +\t\t\twarning(\"Failed to resolve '%s' as a valid ref; taking it literally.\", q);\n>> +\t\telse\n>> +\t\t\tq = sha1_to_hex(sha1);\n> \n> Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything\n> else, it would not work anyway)?\n\nWhat is \"this\"???\n\nSo far, \"git replace -d <rev>\" only accepts a full sha1, because it uses\nit literally as a ref name \"resf/replace/<rev>\" without resolving anything.\n\nThe patch makes it so that <rev> gets resolved to a sha1 even if it is\nabbreviated, and then it gets used.\n\nOr do you mean the warning?\n\nMichael\n"},{"id":"202105","messageId":"20121029090419.GA29464@sigill.intra.peff.net","threadId":"31949","inReplyTo":"508E4637.2060903@drmicha.warpmail.net","subject":"Re: [PATCH] replace: parse revision argument for -d","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-29T09:04:19Z","receivedAt":"2012-10-29T09:04:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 29, 2012 at 10:02:47AM +0100, Michael J Gruber wrote:\n\n> Jeff King venit, vidit, dixit 29.10.2012 07:58:\n> > On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote:\n> > \n> >>  \tfor (p = argv; *p; p++) {\n> >> -\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n> >> +\t\tq = *p;\n> >> +\t\tif (get_sha1(q, sha1))\n> >> +\t\t\twarning(\"Failed to resolve '%s' as a valid ref; taking it literally.\", q);\n> >> +\t\telse\n> >> +\t\t\tq = sha1_to_hex(sha1);\n> > \n> > Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything\n> > else, it would not work anyway)?\n> \n> What is \"this\"???\n> \n> So far, \"git replace -d <rev>\" only accepts a full sha1, because it uses\n> it literally as a ref name \"resf/replace/<rev>\" without resolving anything.\n> \n> The patch makes it so that <rev> gets resolved to a sha1 even if it is\n> abbreviated, and then it gets used.\n> \n> Or do you mean the warning?\n\nSorry, yeah, I meant the warning and fallback.\n\nIf I understand correctly, the fallback will never work unless we are\nfed a 40-byte sha1. But get_sha1 should always return a 40-byte sha1\nwithout doing any further processing.\n\n-Peff\n"},{"id":"202108","messageId":"508E55B2.6060502@drmicha.warpmail.net","threadId":"31949","inReplyTo":"20121029090419.GA29464@sigill.intra.peff.net","subject":"Re: [PATCH] replace: parse revision argument for -d","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-10-29T10:08:50Z","receivedAt":"2012-10-29T10:08:50Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 29.10.2012 10:04:\n> On Mon, Oct 29, 2012 at 10:02:47AM +0100, Michael J Gruber wrote:\n> \n>> Jeff King venit, vidit, dixit 29.10.2012 07:58:\n>>> On Fri, Oct 26, 2012 at 03:33:27PM +0200, Michael J Gruber wrote:\n>>>\n>>>>  \tfor (p = argv; *p; p++) {\n>>>> -\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n>>>> +\t\tq = *p;\n>>>> +\t\tif (get_sha1(q, sha1))\n>>>> +\t\t\twarning(\"Failed to resolve '%s' as a valid ref; taking it literally.\", q);\n>>>> +\t\telse\n>>>> +\t\t\tq = sha1_to_hex(sha1);\n>>>\n>>> Doesn't get_sha1 already handle this for 40-byte sha1s (and for anything\n>>> else, it would not work anyway)?\n>>\n>> What is \"this\"???\n>>\n>> So far, \"git replace -d <rev>\" only accepts a full sha1, because it uses\n>> it literally as a ref name \"resf/replace/<rev>\" without resolving anything.\n>>\n>> The patch makes it so that <rev> gets resolved to a sha1 even if it is\n>> abbreviated, and then it gets used.\n>>\n>> Or do you mean the warning?\n> \n> Sorry, yeah, I meant the warning and fallback.\n> \n> If I understand correctly, the fallback will never work unless we are\n> fed a 40-byte sha1. But get_sha1 should always return a 40-byte sha1\n> without doing any further processing.\n\nYou do understand correctly, and I misunderstood get_sha1(). I does not\ncheck for the presence of that sha1 in the object db, so that it\nsucceeds no matter what, that is: if it's valid hex. So I should\nprobably rewrite the error handling: no more need to check for lengths.\n\nMichael\n"},{"id":"202121","messageId":"24b0f81315ddab8cc37133d5b3bec8aec90ed652.1351516888.git.git@drmicha.warpmail.net","threadId":"31949","inReplyTo":"508E55B2.6060502@drmicha.warpmail.net","subject":"[PATCHv2] replace: parse revision argument for -d","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-10-29T13:23:27Z","receivedAt":"2012-10-29T13:23:27Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"'git replace' parses the revision arguments when it creates replacements\n(so that a sha1 can be abbreviated, e.g.) but not when deleting\nreplacements.\n\nMake it parse the arguments to 'replace -d' in the same way.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\nv2 has the simplified error check as per Jeff, and a reworded message.\nComes with a free test case, too.\n\n builtin/replace.c  | 14 ++++++++------\n t/t6050-replace.sh | 11 +++++++++++\n 2 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex e3aaf70..7b00055 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -46,24 +46,26 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n \n static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n {\n-\tconst char **p;\n+\tconst char **p, *q;\n \tchar ref[PATH_MAX];\n \tint had_error = 0;\n \tunsigned char sha1[20];\n \n \tfor (p = argv; *p; p++) {\n-\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n-\t\t\t\t\t>= sizeof(ref)) {\n-\t\t\terror(\"replace ref name too long: %.*s...\", 50, *p);\n+\t\tq = *p;\n+\t\tif (get_sha1(q, sha1)) {\n+\t\t\terror(\"Failed to resolve '%s' as a valid ref.\", q);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tq = sha1_to_hex(sha1);\n+\t\tsnprintf(ref, sizeof(ref), \"refs/replace/%s\", q);\n \t\tif (read_ref(ref, sha1)) {\n-\t\t\terror(\"replace ref '%s' not found.\", *p);\n+\t\t\terror(\"replace ref '%s' not found.\", q);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (fn(*p, ref, sha1))\n+\t\tif (fn(q, ref, sha1))\n \t\t\thad_error = 1;\n \t}\n \treturn had_error;\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 5c87f28..decdc33 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -140,6 +140,17 @@ test_expect_success '\"git replace\" replacing' '\n      test \"$HASH2\" = \"$(git replace)\"\n '\n \n+test_expect_success '\"git replace\" resolves sha1' '\n+     SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&\n+     git replace -d $SHORTHASH2 &&\n+     git replace $SHORTHASH2 $R &&\n+     git show $HASH2 | grep \"O Thor\" &&\n+     test_must_fail git replace $HASH2 $R &&\n+     git replace -f $HASH2 $R &&\n+     test_must_fail git replace -f &&\n+     test \"$HASH2\" = \"$(git replace)\"\n+'\n+\n # This creates a side branch where the bug in H2\n # does not appear because P2 is created by applying\n # H2 and squashing H5 into it.\n-- \n1.8.0.370.g8cbad08\n"},{"id":"202709","messageId":"20121109164808.GE19725@sigill.intra.peff.net","threadId":"31949","inReplyTo":"24b0f81315ddab8cc37133d5b3bec8aec90ed652.1351516888.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2] replace: parse revision argument for -d","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-09T16:48:08Z","receivedAt":"2012-11-09T16:48:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 29, 2012 at 02:23:27PM +0100, Michael J Gruber wrote:\n\n> 'git replace' parses the revision arguments when it creates replacements\n> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n> replacements.\n> \n> Make it parse the arguments to 'replace -d' in the same way.\n> \n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n> v2 has the simplified error check as per Jeff, and a reworded message.\n> Comes with a free test case, too.\n\nI noticed this today in my pile of \"to look at\" patches. Sorry for being\nslow.\n\n>  \tfor (p = argv; *p; p++) {\n> -\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n> -\t\t\t\t\t>= sizeof(ref)) {\n> -\t\t\terror(\"replace ref name too long: %.*s...\", 50, *p);\n> +\t\tq = *p;\n> +\t\tif (get_sha1(q, sha1)) {\n> +\t\t\terror(\"Failed to resolve '%s' as a valid ref.\", q);\n>  \t\t\thad_error = 1;\n>  \t\t\tcontinue;\n>  \t\t}\n\nLooks reasonable.\n\n> +\t\tq = sha1_to_hex(sha1);\n> +\t\tsnprintf(ref, sizeof(ref), \"refs/replace/%s\", q);\n>  \t\tif (read_ref(ref, sha1)) {\n> -\t\t\terror(\"replace ref '%s' not found.\", *p);\n> +\t\t\terror(\"replace ref '%s' not found.\", q);\n\nI worry a little about assuming that \"q\", which points to a static\ninternal buffer of sha1_to_hex, is still valid after calling read_ref.\nWe'll end up in resolve_ref, which might need to do considerable work\n(e.g., loading the whole packed refs file). Just grepping for\nsha1_to_hex, I don't think it is a problem currently, but it might be\nworth copying the value (you could even point into the \"ref\" buffer to\navoid dealing with an extra allocation).\n\nOther than that, it looks good to me.\n\n-Peff\n"},{"id":"202947","messageId":"50A0B896.8050700@drmicha.warpmail.net","threadId":"31949","inReplyTo":"20121109164808.GE19725@sigill.intra.peff.net","subject":"Re: [PATCHv2] replace: parse revision argument for -d","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-11-12T08:51:34Z","receivedAt":"2012-11-12T08:51:34Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 09.11.2012 17:48:\n> On Mon, Oct 29, 2012 at 02:23:27PM +0100, Michael J Gruber wrote:\n> \n>> 'git replace' parses the revision arguments when it creates replacements\n>> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n>> replacements.\n>>\n>> Make it parse the arguments to 'replace -d' in the same way.\n>>\n>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n>> ---\n>> v2 has the simplified error check as per Jeff, and a reworded message.\n>> Comes with a free test case, too.\n> \n> I noticed this today in my pile of \"to look at\" patches. Sorry for being\n> slow.\n\nNo problem. This is not urgent, and it takes some effort to look at code\namongst all those black-and-white discussions. [It even takes effort to\nrefrain from responding when you words are being twisted around...]\n\n>>  \tfor (p = argv; *p; p++) {\n>> -\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n>> -\t\t\t\t\t>= sizeof(ref)) {\n>> -\t\t\terror(\"replace ref name too long: %.*s...\", 50, *p);\n>> +\t\tq = *p;\n>> +\t\tif (get_sha1(q, sha1)) {\n>> +\t\t\terror(\"Failed to resolve '%s' as a valid ref.\", q);\n>>  \t\t\thad_error = 1;\n>>  \t\t\tcontinue;\n>>  \t\t}\n> \n> Looks reasonable.\n> \n>> +\t\tq = sha1_to_hex(sha1);\n>> +\t\tsnprintf(ref, sizeof(ref), \"refs/replace/%s\", q);\n>>  \t\tif (read_ref(ref, sha1)) {\n>> -\t\t\terror(\"replace ref '%s' not found.\", *p);\n>> +\t\t\terror(\"replace ref '%s' not found.\", q);\n> \n> I worry a little about assuming that \"q\", which points to a static\n> internal buffer of sha1_to_hex, is still valid after calling read_ref.\n> We'll end up in resolve_ref, which might need to do considerable work\n> (e.g., loading the whole packed refs file). Just grepping for\n> sha1_to_hex, I don't think it is a problem currently, but it might be\n> worth copying the value (you could even point into the \"ref\" buffer to\n> avoid dealing with an extra allocation).\n\nWe could just leave '*p' as it is (after all, that was the user input),\nor use 'ref+strlen(\"refs/replace/\")'.\n\nI wasn't aware of the volatile nature of the return value. Thanks for\nspotting!\n\nMichael\n"},{"id":"202953","messageId":"a35a8f44b908bded0b475b02e7917011fb3bf90b.1352728712.git.git@drmicha.warpmail.net","threadId":"31949","inReplyTo":"50A0B896.8050700@drmicha.warpmail.net","subject":"[PATCHv3] replace: parse revision argument for -d","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-11-12T14:18:02Z","receivedAt":"2012-11-12T14:18:02Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"'git replace' parses the revision arguments when it creates replacements\n(so that a sha1 can be abbreviated, e.g.) but not when deleting\nreplacements.\n\nMake it parse the argument to 'replace -d' in the same way.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n\nNotes:\n    v3 safeguards the hex buffer against reuse\n builtin/replace.c  | 16 ++++++++++------\n t/t6050-replace.sh | 11 +++++++++++\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex e3aaf70..33e6ec3 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -46,24 +46,28 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n \n static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n {\n-\tconst char **p;\n+\tconst char **p, *q;\n \tchar ref[PATH_MAX];\n \tint had_error = 0;\n \tunsigned char sha1[20];\n \n \tfor (p = argv; *p; p++) {\n-\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n-\t\t\t\t\t>= sizeof(ref)) {\n-\t\t\terror(\"replace ref name too long: %.*s...\", 50, *p);\n+\t\tq = *p;\n+\t\tif (get_sha1(q, sha1)) {\n+\t\t\terror(\"Failed to resolve '%s' as a valid ref.\", q);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tq = sha1_to_hex(sha1);\n+\t\tsnprintf(ref, sizeof(ref), \"refs/replace/%s\", q);\n+\t\t/* read_ref() may reuse the buffer */\n+\t\tq = ref + strlen(\"refs/replace/\");\n \t\tif (read_ref(ref, sha1)) {\n-\t\t\terror(\"replace ref '%s' not found.\", *p);\n+\t\t\terror(\"replace ref '%s' not found.\", q);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (fn(*p, ref, sha1))\n+\t\tif (fn(q, ref, sha1))\n \t\t\thad_error = 1;\n \t}\n \treturn had_error;\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 5c87f28..decdc33 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -140,6 +140,17 @@ test_expect_success '\"git replace\" replacing' '\n      test \"$HASH2\" = \"$(git replace)\"\n '\n \n+test_expect_success '\"git replace\" resolves sha1' '\n+     SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&\n+     git replace -d $SHORTHASH2 &&\n+     git replace $SHORTHASH2 $R &&\n+     git show $HASH2 | grep \"O Thor\" &&\n+     test_must_fail git replace $HASH2 $R &&\n+     git replace -f $HASH2 $R &&\n+     test_must_fail git replace -f &&\n+     test \"$HASH2\" = \"$(git replace)\"\n+'\n+\n # This creates a side branch where the bug in H2\n # does not appear because P2 is created by applying\n # H2 and squashing H5 into it.\n-- \n1.8.0.311.gdd08018\n"},{"id":"202997","messageId":"20121112204254.GH4623@sigill.intra.peff.net","threadId":"31949","inReplyTo":"a35a8f44b908bded0b475b02e7917011fb3bf90b.1352728712.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv3] replace: parse revision argument for -d","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-12T20:42:55Z","receivedAt":"2012-11-12T20:42:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 12, 2012 at 03:18:02PM +0100, Michael J Gruber wrote:\n\n> 'git replace' parses the revision arguments when it creates replacements\n> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n> replacements.\n> \n> Make it parse the argument to 'replace -d' in the same way.\n> \n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n> \n> Notes:\n>     v3 safeguards the hex buffer against reuse\n\nThanks, I don't see any other functional problems.\n\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index e3aaf70..33e6ec3 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -46,24 +46,28 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n>  \n>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n>  {\n> -\tconst char **p;\n> +\tconst char **p, *q;\n\nI find this readable today, but I wonder if in six months we will wonder\nwhat in the world \"q\" means. Maybe \"short_refname\" or something would be\nappropriate?\n\n-Peff\n"},{"id":"203067","messageId":"50A22026.60506@drmicha.warpmail.net","threadId":"31949","inReplyTo":"20121112204254.GH4623@sigill.intra.peff.net","subject":"Re: [PATCHv3] replace: parse revision argument for -d","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-11-13T10:25:42Z","receivedAt":"2012-11-13T10:25:42Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Jeff King venit, vidit, dixit 12.11.2012 21:42:\n> On Mon, Nov 12, 2012 at 03:18:02PM +0100, Michael J Gruber wrote:\n> \n>> 'git replace' parses the revision arguments when it creates replacements\n>> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n>> replacements.\n>>\n>> Make it parse the argument to 'replace -d' in the same way.\n>>\n>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n>> ---\n>>\n>> Notes:\n>>     v3 safeguards the hex buffer against reuse\n> \n> Thanks, I don't see any other functional problems.\n> \n>> diff --git a/builtin/replace.c b/builtin/replace.c\n>> index e3aaf70..33e6ec3 100644\n>> --- a/builtin/replace.c\n>> +++ b/builtin/replace.c\n>> @@ -46,24 +46,28 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n>>  \n>>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n>>  {\n>> -\tconst char **p;\n>> +\tconst char **p, *q;\n> \n> I find this readable today, but I wonder if in six months we will wonder\n> what in the world \"q\" means. Maybe \"short_refname\" or something would be\n> appropriate?\n\nThat would be sooo inappropriate! ;)\n\nMaybe \"full_hex\"?\n\nI should also do away with the first replacement which really made sense\nin v1 only.\n\nv4 to follow.\n\nMichael\n"},{"id":"203068","messageId":"50A2213B.4060505@drmicha.warpmail.net","threadId":"31949","inReplyTo":"a35a8f44b908bded0b475b02e7917011fb3bf90b.1352728712.git.git@drmicha.warpmail.net","subject":"Notes in format-patch (was: Re: [PATCHv3] replace: parse revision argument for -d)","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-11-13T10:30:19Z","receivedAt":"2012-11-13T10:30:19Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:\n> 'git replace' parses the revision arguments when it creates replacements\n> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n> replacements.\n> \n> Make it parse the argument to 'replace -d' in the same way.\n> \n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n> \n> Notes:\n>     v3 safeguards the hex buffer against reuse\n>  builtin/replace.c  | 16 ++++++++++------\n>  t/t6050-replace.sh | 11 +++++++++++\n>  2 files changed, 21 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/replace.c b/builtin/replace.c\n\nBy the way - Junio, is that the intented outcome of \"format-patch\n--notes\"? I would rather put the newline between the note and the\ndiffstat (and omit the one after the ---) but may have goofed up a rebase:\n\n...\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\nNotes:\n    v3 safeguards the hex buffer against reuse\n\n builtin/replace.c  | 16 ++++++++++------\n...\n"},{"id":"203069","messageId":"848bc8f48885acd63b4f6d29eea0543e73d86451.1352802239.git.git@drmicha.warpmail.net","threadId":"31949","inReplyTo":"50A22026.60506@drmicha.warpmail.net","subject":"[PATCHv4] replace: parse revision argument for -d","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-11-13T10:34:11Z","receivedAt":"2012-11-13T10:34:11Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"'git replace' parses the revision arguments when it creates replacements\n(so that a sha1 can be abbreviated, e.g.) but not when deleting\nreplacements.\n\nMake it parse the argument to 'replace -d' in the same way.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n---\n\nNotes:\n    v4 names the aux variable more concisely and does away with a superfluous\n    assignment.\n builtin/replace.c  | 15 +++++++++------\n t/t6050-replace.sh | 11 +++++++++++\n 2 files changed, 20 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex e3aaf70..398ccd5 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -46,24 +46,27 @@ typedef int (*each_replace_name_fn)(const char *name, const char *ref,\n \n static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n {\n-\tconst char **p;\n+\tconst char **p, *full_hex;\n \tchar ref[PATH_MAX];\n \tint had_error = 0;\n \tunsigned char sha1[20];\n \n \tfor (p = argv; *p; p++) {\n-\t\tif (snprintf(ref, sizeof(ref), \"refs/replace/%s\", *p)\n-\t\t\t\t\t>= sizeof(ref)) {\n-\t\t\terror(\"replace ref name too long: %.*s...\", 50, *p);\n+\t\tif (get_sha1(*p, sha1)) {\n+\t\t\terror(\"Failed to resolve '%s' as a valid ref.\", *p);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tfull_hex = sha1_to_hex(sha1);\n+\t\tsnprintf(ref, sizeof(ref), \"refs/replace/%s\", full_hex);\n+\t\t/* read_ref() may reuse the buffer */\n+\t\tfull_hex = ref + strlen(\"refs/replace/\");\n \t\tif (read_ref(ref, sha1)) {\n-\t\t\terror(\"replace ref '%s' not found.\", *p);\n+\t\t\terror(\"replace ref '%s' not found.\", full_hex);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (fn(*p, ref, sha1))\n+\t\tif (fn(full_hex, ref, sha1))\n \t\t\thad_error = 1;\n \t}\n \treturn had_error;\ndiff --git a/t/t6050-replace.sh b/t/t6050-replace.sh\nindex 5c87f28..decdc33 100755\n--- a/t/t6050-replace.sh\n+++ b/t/t6050-replace.sh\n@@ -140,6 +140,17 @@ test_expect_success '\"git replace\" replacing' '\n      test \"$HASH2\" = \"$(git replace)\"\n '\n \n+test_expect_success '\"git replace\" resolves sha1' '\n+     SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&\n+     git replace -d $SHORTHASH2 &&\n+     git replace $SHORTHASH2 $R &&\n+     git show $HASH2 | grep \"O Thor\" &&\n+     test_must_fail git replace $HASH2 $R &&\n+     git replace -f $HASH2 $R &&\n+     test_must_fail git replace -f &&\n+     test \"$HASH2\" = \"$(git replace)\"\n+'\n+\n # This creates a side branch where the bug in H2\n # does not appear because P2 is created by applying\n # H2 and squashing H5 into it.\n-- \n1.8.0.311.gdd08018\n"},{"id":"203076","messageId":"20121113133729.GA20361@sigill.intra.peff.net","threadId":"31949","inReplyTo":"848bc8f48885acd63b4f6d29eea0543e73d86451.1352802239.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv4] replace: parse revision argument for -d","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T13:37:29Z","receivedAt":"2012-11-13T13:37:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 11:34:11AM +0100, Michael J Gruber wrote:\n\n> 'git replace' parses the revision arguments when it creates replacements\n> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n> replacements.\n> \n> Make it parse the argument to 'replace -d' in the same way.\n> \n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> ---\n> \n> Notes:\n>     v4 names the aux variable more concisely and does away with a superfluous\n>     assignment.\n\nThanks. I agree the name \"full_hex\" is much better. Patch looks good to\nme at this point.\n\n-Peff\n"},{"id":"203074","messageId":"20121113133834.GB20361@sigill.intra.peff.net","threadId":"31949","inReplyTo":"50A2213B.4060505@drmicha.warpmail.net","subject":"Re: Notes in format-patch (was: Re: [PATCHv3] replace: parse revision argument for -d)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T13:38:35Z","receivedAt":"2012-11-13T13:38:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 11:30:19AM +0100, Michael J Gruber wrote:\n\n> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:\n> > 'git replace' parses the revision arguments when it creates replacements\n> > (so that a sha1 can be abbreviated, e.g.) but not when deleting\n> > replacements.\n> > \n> > Make it parse the argument to 'replace -d' in the same way.\n> > \n> > Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> > ---\n> > \n> > Notes:\n> >     v3 safeguards the hex buffer against reuse\n> >  builtin/replace.c  | 16 ++++++++++------\n> >  t/t6050-replace.sh | 11 +++++++++++\n> >  2 files changed, 21 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/builtin/replace.c b/builtin/replace.c\n> \n> By the way - Junio, is that the intented outcome of \"format-patch\n> --notes\"? I would rather put the newline between the note and the\n> diffstat (and omit the one after the ---) but may have goofed up a rebase:\n\nI do not know was intended, but the above quoted output is hard to read,\nand your suggested change looks way better.\n\n-Peff\n"},{"id":"203103","messageId":"7vhaotfou6.fsf@alter.siamese.dyndns.org","threadId":"31949","inReplyTo":"50A2213B.4060505@drmicha.warpmail.net","subject":"Re: Notes in format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T16:29:53Z","receivedAt":"2012-11-13T16:29:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:\n>> 'git replace' parses the revision arguments when it creates replacements\n>> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n>> replacements.\n>> \n>> Make it parse the argument to 'replace -d' in the same way.\n>> \n>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n>> ---\n>> \n>> Notes:\n>>     v3 safeguards the hex buffer against reuse\n>>  builtin/replace.c  | 16 ++++++++++------\n>>  t/t6050-replace.sh | 11 +++++++++++\n>>  2 files changed, 21 insertions(+), 6 deletions(-)\n>> \n>> diff --git a/builtin/replace.c b/builtin/replace.c\n>\n> By the way - Junio, is that the intented outcome of \"format-patch\n> --notes\"? I would rather put the newline between the note and the\n> diffstat...\n\nI do not mind (actually I personally would prefer to see) a blank\nline between the three-dash and \"Notes:\", but I agree that we should\nhave a blank line before the diffstat block.\n"},{"id":"203114","messageId":"7vzk2le918.fsf@alter.siamese.dyndns.org","threadId":"31949","inReplyTo":"7vhaotfou6.fsf@alter.siamese.dyndns.org","subject":"Re: Notes in format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T16:56:35Z","receivedAt":"2012-11-13T16:56:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n>\n>> Michael J Gruber venit, vidit, dixit 12.11.2012 15:18:\n>>> 'git replace' parses the revision arguments when it creates replacements\n>>> (so that a sha1 can be abbreviated, e.g.) but not when deleting\n>>> replacements.\n>>> \n>>> Make it parse the argument to 'replace -d' in the same way.\n>>> \n>>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n>>> ---\n>>> \n>>> Notes:\n>>>     v3 safeguards the hex buffer against reuse\n>>>  builtin/replace.c  | 16 ++++++++++------\n>>>  t/t6050-replace.sh | 11 +++++++++++\n>>>  2 files changed, 21 insertions(+), 6 deletions(-)\n>>> \n>>> diff --git a/builtin/replace.c b/builtin/replace.c\n>>\n>> By the way - Junio, is that the intented outcome of \"format-patch\n>> --notes\"? I would rather put the newline between the note and the\n>> diffstat...\n>\n> I do not mind (actually I personally would prefer to see) a blank\n> line between the three-dash and \"Notes:\", but I agree that we should\n> have a blank line before the diffstat block.\n\nAs the topic seems to be already in Peff's next, here is a trivial\nfix for this in incremental form.\n\n-- >8 --\nSubject: format-patch: add a blank line between notes and diffstat\n\nThe last line of the note text comes immediately before the diffstat\nblock, making the latter unnecessarily harder to view.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n log-tree.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git i/log-tree.c w/log-tree.c\nindex 712a22b..9303fd8 100644\n--- i/log-tree.c\n+++ w/log-tree.c\n@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)\n \t\t\topt->shown_dashes = 1;\n \t\t}\n \t\tstrbuf_addstr(&msgbuf, ctx.notes_message);\n+\t\tstrbuf_addch(&msgbuf, '\\n');\n \t}\n \n \tif (opt->show_log_size) {\n"},{"id":"203121","messageId":"7vr4nxe7nf.fsf@alter.siamese.dyndns.org","threadId":"31949","inReplyTo":"7vzk2le918.fsf@alter.siamese.dyndns.org","subject":"Re: Notes in format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T17:26:28Z","receivedAt":"2012-11-13T17:26:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> As the topic seems to be already in Peff's next, here is a trivial\n> fix for this in incremental form.\n>\n> -- >8 --\n> Subject: format-patch: add a blank line between notes and diffstat\n>\n> The last line of the note text comes immediately before the diffstat\n> block, making the latter unnecessarily harder to view.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  log-tree.c | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git i/log-tree.c w/log-tree.c\n> index 712a22b..9303fd8 100644\n> --- i/log-tree.c\n> +++ w/log-tree.c\n> @@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)\n>  \t\t\topt->shown_dashes = 1;\n>  \t\t}\n>  \t\tstrbuf_addstr(&msgbuf, ctx.notes_message);\n> +\t\tstrbuf_addch(&msgbuf, '\\n');\n>  \t}\n>  \n>  \tif (opt->show_log_size) {\n\n... and it is broken X-<.\n\nThe blank line should be added before the diffstat, not after the\nnotes message (t3307 shows a case where we give notes without\ndiffstat, and we shouldn't be adding an extra blank line in that\ncase.\n"},{"id":"203125","messageId":"7vfw4de5oc.fsf@alter.siamese.dyndns.org","threadId":"31949","inReplyTo":"7vr4nxe7nf.fsf@alter.siamese.dyndns.org","subject":"Re: Notes in format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T18:09:07Z","receivedAt":"2012-11-13T18:09:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ... and it is broken X-<.\n>\n> The blank line should be added before the diffstat, not after the\n> notes message (t3307 shows a case where we give notes without\n> diffstat, and we shouldn't be adding an extra blank line in that\n> case.\n\nSecond try.\n\n-- >8 --\nSubject: format-patch: add a blank line between notes and diffstat\n\nThe last line of the note text comes immediately before the diffstat\nblock, making the latter unnecessarily harder to view.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n log-tree.c | 31 +++++++++++++++++++++----------\n 1 file changed, 21 insertions(+), 10 deletions(-)\n\ndiff --git c/log-tree.c w/log-tree.c\nindex 712a22b..4f86def 100644\n--- c/log-tree.c\n+++ w/log-tree.c\n@@ -727,15 +727,16 @@ int log_tree_diff_flush(struct rev_info *opt)\n \t}\n \n \tif (opt->loginfo && !opt->no_commit_id) {\n-\t\t/* When showing a verbose header (i.e. log message),\n-\t\t * and not in --pretty=oneline format, we would want\n-\t\t * an extra newline between the end of log and the\n-\t\t * output for readability.\n-\t\t */\n \t\tshow_log(opt);\n \t\tif ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&\n \t\t    opt->verbose_header &&\n \t\t    opt->commit_format != CMIT_FMT_ONELINE) {\n+\t\t\t/*\n+\t\t\t * When showing a verbose header (i.e. log message),\n+\t\t\t * and not in --pretty=oneline format, we would want\n+\t\t\t * an extra newline between the end of log and the\n+\t\t\t * diff/diffstat output for readability.\n+\t\t\t */\n \t\t\tint pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;\n \t\t\tif (opt->diffopt.output_prefix) {\n \t\t\t\tstruct strbuf *msg = NULL;\n@@ -743,11 +744,21 @@ int log_tree_diff_flush(struct rev_info *opt)\n \t\t\t\t\topt->diffopt.output_prefix_data);\n \t\t\t\tfwrite(msg->buf, msg->len, 1, stdout);\n \t\t\t}\n-\t\t\tif (!opt->shown_dashes) {\n-\t\t\t\tif ((pch & opt->diffopt.output_format) == pch)\n-\t\t\t\t\tprintf(\"---\");\n-\t\t\t\tputchar('\\n');\n-\t\t\t}\n+\n+\t\t\t/*\n+\t\t\t * We may have shown three-dashes line early\n+\t\t\t * between notes and the log message, in which\n+\t\t\t * case we only want a blank line after the\n+\t\t\t * notes without (an extra) three-dashes line.\n+\t\t\t * Otherwise, we show the three-dashes line if\n+\t\t\t * we are showing the patch with diffstat, but\n+\t\t\t * in that case, there is no extra blank line\n+\t\t\t * after the three-dashes line.\n+\t\t\t */\n+\t\t\tif (!opt->shown_dashes &&\n+\t\t\t    (pch & opt->diffopt.output_format) == pch)\n+\t\t\t\tprintf(\"---\");\n+\t\t\tputchar('\\n');\n \t\t}\n \t}\n \tdiff_flush(&opt->diffopt);\n"},{"id":"203177","messageId":"50A361BD.2010806@drmicha.warpmail.net","threadId":"31949","inReplyTo":"7vfw4de5oc.fsf@alter.siamese.dyndns.org","subject":"Re: Notes in format-patch","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2012-11-14T09:17:49Z","receivedAt":"2012-11-14T09:17:49Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 13.11.2012 19:09:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> ... and it is broken X-<.\n>>\n>> The blank line should be added before the diffstat, not after the\n>> notes message (t3307 shows a case where we give notes without\n>> diffstat, and we shouldn't be adding an extra blank line in that\n>> case.\n> \n> Second try.\n> \n> -- >8 --\n> Subject: format-patch: add a blank line between notes and diffstat\n> \n> The last line of the note text comes immediately before the diffstat\n> block, making the latter unnecessarily harder to view.\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> \n>  log-tree.c | 31 +++++++++++++++++++++----------\n>  1 file changed, 21 insertions(+), 10 deletions(-)\n\nThanks, that patch works. I'm curious, though, where the empty line\nbetween the --- and your diffstat comes from. Do you have an empty note?\nI'm not getting any (origin/next+your patch).\n\nThe fact that we don't usually have that empty line was the reason why I\npreferred to have no empty line between the --- and the \"Note:\".\n\nMichael\n"},{"id":"203185","messageId":"7v7gpocom0.fsf@alter.siamese.dyndns.org","threadId":"31949","inReplyTo":"50A361BD.2010806@drmicha.warpmail.net","subject":"Re: Notes in format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-14T13:15:19Z","receivedAt":"2012-11-14T13:15:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Junio C Hamano venit, vidit, dixit 13.11.2012 19:09:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>>> ... and it is broken X-<.\n>>>\n>>> The blank line should be added before the diffstat, not after the\n>>> notes message (t3307 shows a case where we give notes without\n>>> diffstat, and we shouldn't be adding an extra blank line in that\n>>> case.\n>> \n>> Second try.\n>> \n>> -- >8 --\n>> Subject: format-patch: add a blank line between notes and diffstat\n>> \n>> The last line of the note text comes immediately before the diffstat\n>> block, making the latter unnecessarily harder to view.\n>> \n>> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n>> ---\n>> \n>>  log-tree.c | 31 +++++++++++++++++++++----------\n>>  1 file changed, 21 insertions(+), 10 deletions(-)\n>\n> Thanks, that patch works. I'm curious, though, where the empty line\n> between the --- and your diffstat comes from.\n\nThe message you are responding to is *not* an output from\nformat-patch but was written in my MUA.\n\nThe way I work when I show \"this should work\" patch is to:\n\n (1) Think, edit in my working tree, compile, eyeball \"git diff HEAD\",\n     think again, and test;\n\n (2) Hit \"Reply All\" to the message I am going to give \"this should\n     work\" response to, and start composing the response;\n\n (3) Run \"git diff --stat -p HEAD\" to have its output appended at\n     the end of the message I started to compose in the previous\n     step;\n\n (4) Write everything that should come before the output I appended\n     in the previous step, i.e. \"-- >8 --\", in-body \"Subject:\", log,\n     sign-off, and three-dash lines;\n\n (5) Send it out; and\n\n (6) Run \"git reset --hard\" and move on.\n\nThe blank line was added in step (4), not step (3), which does not\neven have any commit log message, as the patch does not come from\nany existing commit.  Later I may pick it up and apply to a topic\nbranch just like I do for patches from other people and that is the\npoint when such a patch becomes a commit for the first time.\n"}]}