{"thread":{"id":"28849","subject":"[PATCH] Add abbreviated commit hash to rebase conflict message","startedAt":"2011-11-05T14:02:39Z","lastAt":"2011-11-09T18:25:21Z","messageCount":9,"participants":["Sverre Rabbelier","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"178898","messageId":"1320501759-27236-1-git-send-email-srabbelier@gmail.com","threadId":"28849","inReplyTo":null,"subject":"[PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-05T14:02:39Z","receivedAt":"2011-11-05T14:02:39Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Also move the $msgnum to a more sensible location.\n\nBefore:\n\tPatch failed at 0001 msg\nAfter:\n\tPatch 0001 failed at [da65151] msg\n\nReviewed-by: Eric Herman \nReviewed-by: Fernando Vezzosi \nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Sverre Rabbelier \n---\n git-am.sh | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 9042432..9d70588 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -837,7 +837,8 @@ did you forget to use 'git add'?\"\n \tfi\n \tif test $apply_status != 0\n \tthen\n-\t\teval_gettextln 'Patch failed at $msgnum $FIRSTLINE'\n+\t\tabbrev_commit=$(git log -1 --pretty=%h $commit)\n+\t\teval_gettextln 'Patch $msgnum failed at [$abbrev_commit] $FIRSTLINE'\n \t\tstop_here_user_resolve $this\n \tfi\n \n-- \n1.7.8.rc0.36.g67522.dirty\n"},{"id":"178938","messageId":"7v39e2852t.fsf@alter.siamese.dyndns.org","threadId":"28849","inReplyTo":"1320501759-27236-1-git-send-email-srabbelier@gmail.com","subject":"Re: [PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-06T00:31:06Z","receivedAt":"2011-11-06T00:31:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> Also move the $msgnum to a more sensible location.\n>\n> Before:\n> \tPatch failed at 0001 msg\n> After:\n> \tPatch 0001 failed at [da65151] msg\n\nWe can guess that 7-hexdigit is an abbreviated commit object name but the\nabove description and the title do not tell the most important thing. What\ncommit are you trying to describe, and why is it a good idea to show it?\n\n> Reviewed-by: Eric Herman \n> Reviewed-by: Fernando Vezzosi \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Sverre Rabbelier \n\nI wouldn't have issues if these were Helped-by or Asked-by or something,\nbut a patch with Reviewed-by for which I do not see any trace of\ndiscussion on this list triggers some WTF at least for me.\n\nWhere did these reviews take place? What were their inputs and how was the\npatch improved based on them? Why I should trust the judgements of these\npeople?\n\nWhat happens when threeway is not enabled, and especially when \"git am\" is\nused for applying patches, not within rebase?\n\n> ---\n> git-am.sh | 3 ++-\n> 1 files changed, 2 insertions(+), 1 deletions(-)\n>\n> diff --git a/git-am.sh b/git-am.sh\n> index 9042432..9d70588 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -837,7 +837,8 @@ did you forget to use 'git add'?\"\n> \tfi\n> \tif test $apply_status != 0\n> \tthen\n> -\t\teval_gettextln 'Patch failed at $msgnum $FIRSTLINE'\n> +\t\tabbrev_commit=$(git log -1 --pretty=%h $commit)\n> +\t\teval_gettextln 'Patch $msgnum failed at [$abbrev_commit] $FIRSTLINE'\n"},{"id":"178940","messageId":"CAGdFq_hw1630ELQP3+AEaCmUTEjYq7K1j8ZB-n0_rb1VN=wQgA@mail.gmail.com","threadId":"28849","inReplyTo":"7v39e2852t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-06T00:37:49Z","receivedAt":"2011-11-06T00:37:49Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Nov 6, 2011 at 01:31, Junio C Hamano wrote:\n> We can guess that 7-hexdigit is an abbreviated commit object name but the\n> above description and the title do not tell the most important thing. What\n> commit are you trying to describe, and why is it a good idea to show it?\n\nThe same commit that the title and number are already being displayed\nfor. It's a good idea to show that as that's a lot more convenient way\nto look up the commit that failed to apply than just a rather\narbitrary number and the title.\n\n>> Reviewed-by: Eric Herman \n>> Reviewed-by: Fernando Vezzosi \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> Signed-off-by: Sverre Rabbelier \n>\n> I wouldn't have issues if these were Helped-by or Asked-by or something,\n> but a patch with Reviewed-by for which I do not see any trace of\n> discussion on this list triggers some WTF at least for me.\n>\n> Where did these reviews take place? What were their inputs and how was the\n> patch improved based on them? Why I should trust the judgements of these\n> people?\n\nWe had a little Git hackathon in Amsterdam today, the review was done\nIRL. In this case it consisted of Fernando pointing out that we should\nstick to the git cherry-pick format of displaying the hash/title (with\nthe hash in square brackets before the title), rather than in\nparenthesis after the title like I had before. I wanted to give credit\nto their offline review somehow. If you'd prefer the \"Helped-by\" nomer\nfor this case I'm fine with that.\n\n> What happens when threeway is not enabled, and especially when \"git am\" is\n> used for applying patches, not within rebase?\n\nThe same thing that already happens. I'm not sure what it is, but\nwhatever title/number is shown, the matching hash is now shown as\nwell. This patch does not change that behavior.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"178948","messageId":"7vy5vt7uqo.fsf@alter.siamese.dyndns.org","threadId":"28849","inReplyTo":"CAGdFq_hw1630ELQP3+AEaCmUTEjYq7K1j8ZB-n0_rb1VN=wQgA@mail.gmail.com","subject":"Re: [PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-06T04:14:23Z","receivedAt":"2011-11-06T04:14:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> We had a little Git hackathon in Amsterdam today,...\n\nAh, that was the context I was missing.\n\n>> What happens when threeway is not enabled, and especially when \"git am\" is\n>> used for applying patches, not within rebase?\n>\n> The same thing that already happens. I'm not sure what it is, but\n> whatever title/number is shown, the matching hash is now shown as\n> well. This patch does not change that behavior.\n\nI am puzzled, but that cannot be true. The existing message uses $msgnum\nand $FIRSTLINE but does not use $commit because it does not necessarily\nexist.\n\nWhat a value would the variable contain when I am applying your original\npatch message using \"git am -s\" (or \"git am -s3\")?\n"},{"id":"178976","messageId":"CAGdFq_j7NxojZ+82s0GJ8ZF0oyd5sH8t0kcMOTQGtKbASXqYTA@mail.gmail.com","threadId":"28849","inReplyTo":"7vy5vt7uqo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-06T19:35:09Z","receivedAt":"2011-11-06T19:35:09Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Nov 6, 2011 at 05:14, Junio C Hamano wrote:\n> I am puzzled, but that cannot be true. The existing message uses $msgnum\n> and $FIRSTLINE but does not use $commit because it does not necessarily\n> exist.\n>\n> What a value would the variable contain when I am applying your original\n> patch message using \"git am -s\" (or \"git am -s3\")?\n\nAaah, I understand the concern you raise now. In that case a spurious\n[] would be printed, which I agree is less than desirable. Would\nchecking 'if test -n $commit' be sufficient?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"178982","messageId":"7vaa89573r.fsf@alter.siamese.dyndns.org","threadId":"28849","inReplyTo":"CAGdFq_j7NxojZ+82s0GJ8ZF0oyd5sH8t0kcMOTQGtKbASXqYTA@mail.gmail.com","subject":"Re: [PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-06T20:27:52Z","receivedAt":"2011-11-06T20:27:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Sun, Nov 6, 2011 at 05:14, Junio C Hamano wrote:\n>> I am puzzled, but that cannot be true. The existing message uses $msgnum\n>> and $FIRSTLINE but does not use $commit because it does not necessarily\n>> exist.\n>>\n>> What a value would the variable contain when I am applying your original\n>> patch message using \"git am -s\" (or \"git am -s3\")?\n>\n> Aaah, I understand the concern you raise now. In that case a spurious\n> [] would be printed, which I agree is less than desirable. Would\n> checking 'if test -n $commit' be sufficient?\n\nIn what situation does it make sense to say \"It came from _this_ commit\"?\n\nI think there is a separate variable that allows any part of the script if\nwe are being run as a backend of rebase or not, and that is the condition\nyou are looking for.\n"},{"id":"178984","messageId":"CAGdFq_gS2fV5B26ZBOLs=5L_rnaeORmKW49OxwbP-+vx+ZN8cQ@mail.gmail.com","threadId":"28849","inReplyTo":"7vaa89573r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-11-06T20:42:28Z","receivedAt":"2011-11-06T20:42:28Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Nov 6, 2011 at 21:27, Junio C Hamano wrote:\n> In what situation does it make sense to say \"It came from _this_ commit\"?\n>\n> I think there is a separate variable that allows any part of the script if\n> we are being run as a backend of rebase or not, and that is the condition\n> you are looking for.\n\nThe closest I could find is:\n\n if test -f \"$dotest/rebasing\"\n\nWhich is exactly the case when commit is set. Do you prefer the \"-f\n$dotest/rebasing\" test or the \"-n $commit\" one?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"178987","messageId":"7v4nyg6b9s.fsf@alter.siamese.dyndns.org","threadId":"28849","inReplyTo":"CAGdFq_gS2fV5B26ZBOLs=5L_rnaeORmKW49OxwbP-+vx+ZN8cQ@mail.gmail.com","subject":"Re: [PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-07T00:12:31Z","receivedAt":"2011-11-07T00:12:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sverre Rabbelier writes:\n\n> On Sun, Nov 6, 2011 at 21:27, Junio C Hamano wrote:\n>> In what situation does it make sense to say \"It came from _this_ commit\"?\n>>\n>> I think there is a separate variable that allows any part of the script if\n>> we are being run as a backend of rebase or not, and that is the condition\n>> you are looking for.\n>\n> The closest I could find is:\n>\n> if test -f \"$dotest/rebasing\"\n>\n> Which is exactly the case when commit is set. Do you prefer the \"-f\n> $dotest/rebasing\" test or the \"-n $commit\" one?\n\nGiven the variable scoping rules of vanilla shell script, relying on the\nvariable $commit is a very bad idea to begin with. I think the variable\nalso is used to hold the final commit object name produced by patch\napplication elsewhere in the script in the same loop, and I do not think\nexisting code clears it before each iteration, as each part of the exiting\ncode uses the variable only immediately after that part assigns to the\nvariable for its own purpose, and they all know that nobody uses the\nvariable as a way for long haul communication media between different\nparts of the script. Unless your patch updated that aspect of the\nlifetime rule for the variable, which I doubt you did, using $commit would\nintroduce yet another bug without solving anything, I would think.\n"},{"id":"179215","messageId":"7vmxc5tapa.fsf@alter.siamese.dyndns.org","threadId":"28849","inReplyTo":"7v4nyg6b9s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add abbreviated commit hash to rebase conflict message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-09T18:25:21Z","receivedAt":"2011-11-09T18:25:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Sverre Rabbelier writes:\n>\n>> On Sun, Nov 6, 2011 at 21:27, Junio C Hamano wrote:\n>>> In what situation does it make sense to say \"It came from _this_ commit\"?\n>>>\n>>> I think there is a separate variable that allows any part of the script if\n>>> we are being run as a backend of rebase or not, and that is the condition\n>>> you are looking for.\n>>\n>> The closest I could find is:\n>>\n>> if test -f \"$dotest/rebasing\"\n>>\n>> Which is exactly the case when commit is set. Do you prefer the \"-f\n>> $dotest/rebasing\" test or the \"-n $commit\" one?\n>\n> Given the variable scoping rules of vanilla shell script, relying on the\n> variable $commit is a very bad idea to begin with. I think the variable\n> also is used to hold the final commit object name produced by patch\n> application elsewhere in the script in the same loop, and I do not think\n> existing code clears it before each iteration, as each part of the exiting\n> code uses the variable only immediately after that part assigns to the\n> variable for its own purpose, and they all know that nobody uses the\n> variable as a way for long haul communication media between different\n> parts of the script. Unless your patch updated that aspect of the\n> lifetime rule for the variable, which I doubt you did, using $commit would\n> introduce yet another bug without solving anything, I would think.\n\nI was looking at git-am today for a separate topic. Doesn't it appear to\nyou that $dotest/original-commit is what serves your purpose the best?\n\nThe file is removed before starting to process a new input (i.e. message\nin the mbox), created only after we read the from line and determine it is\nreally the commit we are rebasing, and is left intact until we decide the\npatch was applied correctly and write the result out as a tree.\n\nIt might be a clean-up to get rid of $dotest/original-commit file, rename\nthe variable to $original_commit and initialize it to an empty string\nwhere we currently have 'rm -f \"$dotest/original-commit\"' (and replace the\ncheck 'test -f \"$dotest/original-commit\"' later in the script with a check\n'test -n \"$original_commit\"'), though.\n"}]}