{"thread":{"id":"36408","subject":"[PATCH] git-rebase: Print name of rev when using shorthand","startedAt":"2014-04-13T20:04:34Z","lastAt":"2014-04-16T23:22:46Z","messageCount":6,"participants":["Brian Gesiak","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"238812","messageId":"1397419474-31999-1-git-send-email-modocache@gmail.com","threadId":"36408","inReplyTo":null,"subject":"[PATCH] git-rebase: Print name of rev when using shorthand","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-04-13T20:04:34Z","receivedAt":"2014-04-13T20:04:34Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"The output from a successful invocation of the shorthand command\n\"git rebase -\" is something like \"Fast-forwarded HEAD to @{-1}\",\nwhich includes a relative reference to a revision. Other commands\nthat use the shorthand \"-\", such as \"git checkout -\", typically\ndisplay the symbolic name of the revision.\n\nChange rebase to output the symbolic name of the revision when using\nthe shorthand. For the example above, the new output is\n\"Fast-forwarded HEAD to master\", assuming \"@{-1}\" is a reference to\n\"master\".\n\n- Use \"git name-rev\" to retreive the name of the rev.\n- Update the tests in light of this new behavior.\n\nRequested-by: John Keeping <john@keeping.me.uk>\nSigned-off-by: Brian Gesiak <modocache@gmail.com>\n---\nPrevious discussion on this issue:\nhttp://article.gmane.org/gmane.comp.version-control.git/244340\n\n git-rebase.sh     | 2 +-\n t/t3400-rebase.sh | 4 +---\n 2 files changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 2c75e9f..ab0e081 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -455,7 +455,7 @@ then\n \t*)\tupstream_name=\"$1\"\n \t\tif test \"$upstream_name\" = \"-\"\n \t\tthen\n-\t\t\tupstream_name=\"@{-1}\"\n+\t\t\tupstream_name=`git name-rev --name-only @{-1}`\n \t\tfi\n \t\tshift\n \t\t;;\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 80e0a95..2b99940 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -91,7 +91,7 @@ test_expect_success 'rebase from ambiguous branch name' '\n test_expect_success 'rebase off of the previous branch using \"-\"' '\n \tgit checkout master &&\n \tgit checkout HEAD^ &&\n-\tgit rebase @{-1} >expect.messages &&\n+\tgit rebase master >expect.messages &&\n \tgit merge-base master HEAD >expect.forkpoint &&\n \n \tgit checkout master &&\n@@ -100,8 +100,6 @@ test_expect_success 'rebase off of the previous branch using \"-\"' '\n \tgit merge-base master HEAD >actual.forkpoint &&\n \n \ttest_cmp expect.forkpoint actual.forkpoint &&\n-\t# the next one is dubious---we may want to say \"-\",\n-\t# instead of @{-1}, in the message\n \ttest_i18ncmp expect.messages actual.messages\n '\n \n-- \n1.9.0.259.gc5d75e8.dirty\n"},{"id":"238859","messageId":"xmqqwqerogvr.fsf@gitster.dls.corp.google.com","threadId":"36408","inReplyTo":"1397419474-31999-1-git-send-email-modocache@gmail.com","subject":"Re: [PATCH] git-rebase: Print name of rev when using shorthand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T19:22:48Z","receivedAt":"2014-04-14T19:22:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Gesiak <modocache@gmail.com> writes:\n\n> The output from a successful invocation of the shorthand command\n> \"git rebase -\" is something like \"Fast-forwarded HEAD to @{-1}\",\n> which includes a relative reference to a revision. Other commands\n> that use the shorthand \"-\", such as \"git checkout -\", typically\n> display the symbolic name of the revision.\n>\n> Change rebase to output the symbolic name of the revision when using\n> the shorthand. For the example above, the new output is\n> \"Fast-forwarded HEAD to master\", assuming \"@{-1}\" is a reference to\n> \"master\".\n>\n> - Use \"git name-rev\" to retreive the name of the rev.\n> - Update the tests in light of this new behavior.\n>\n> Requested-by: John Keeping <john@keeping.me.uk>\n> Signed-off-by: Brian Gesiak <modocache@gmail.com>\n> ---\n\nWhat the patch wants to implement sounds sensible, but I do not\nthink name-rev is a right tool for this.  Imagine the case where\nthere are more than one branches whose tip points at the commit you\ncame from.  name-rev will not be able to pick correctly which one to\nreport.\n\nAlso think what happens if you were previously on a detached HEAD?\n\nI think you would want to use something like:\n\n        upstream_name=$(git rev-parse --symbolic-full-name @{-1})\n        if test -n \"$upstream\"\n        then\n                upstream_name=${upstream_name#refs/heads/}\n        else\n                upstream_name=\"@{-1}\"\n        fi\n\nif the change is to be made at that point in the code.\n\nI also wonder if \"git rebase @{-1}\" deserve a similar translation\nlike you are giving \"git rebase -\".\n\n> Previous discussion on this issue:\n> http://article.gmane.org/gmane.comp.version-control.git/244340\n>\n>  git-rebase.sh     | 2 +-\n>  t/t3400-rebase.sh | 4 +---\n>  2 files changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index 2c75e9f..ab0e081 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -455,7 +455,7 @@ then\n>  \t*)\tupstream_name=\"$1\"\n>  \t\tif test \"$upstream_name\" = \"-\"\n>  \t\tthen\n> -\t\t\tupstream_name=\"@{-1}\"\n> +\t\t\tupstream_name=`git name-rev --name-only @{-1}`\n>  \t\tfi\n>  \t\tshift\n>  \t\t;;\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 80e0a95..2b99940 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -91,7 +91,7 @@ test_expect_success 'rebase from ambiguous branch name' '\n>  test_expect_success 'rebase off of the previous branch using \"-\"' '\n>  \tgit checkout master &&\n>  \tgit checkout HEAD^ &&\n> -\tgit rebase @{-1} >expect.messages &&\n> +\tgit rebase master >expect.messages &&\n\nOK.\n\n>  \tgit merge-base master HEAD >expect.forkpoint &&\n>  \n>  \tgit checkout master &&\n> @@ -100,8 +100,6 @@ test_expect_success 'rebase off of the previous branch using \"-\"' '\n>  \tgit merge-base master HEAD >actual.forkpoint &&\n>  \n>  \ttest_cmp expect.forkpoint actual.forkpoint &&\n> -\t# the next one is dubious---we may want to say \"-\",\n> -\t# instead of @{-1}, in the message\n>  \ttest_i18ncmp expect.messages actual.messages\n>  '\n"},{"id":"238918","messageId":"CAN7MxmUikP6pVAj3cpDiSbFxawScTh5zKusPUe8SpkNbH=e6Aw@mail.gmail.com","threadId":"36408","inReplyTo":"xmqqwqerogvr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-rebase: Print name of rev when using shorthand","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-04-16T08:19:41Z","receivedAt":"2014-04-16T08:19:41Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"Thank you for the feedback!\n\n> Imagine the case where there are more than one branches\n> whose tip points at the commit you came from.\n> name-rev will not be able to pick correctly which one to report.\n\nI see. Yes, you're exactly right; the following demonstrates\nthe problem:\n\n$ git checkout -b xylophone master\n$ git checkout -b aardvark master\n$ git name-rev --name-only @{-1} # I'd want \"xylophone\", but this\noutputs \"aardvark\"\n\nSo it appears name-rev is not up to the task here.\n\n> I think you would want to use something like:\n>\n>         upstream_name=$(git rev-parse --symbolic-full-name @{-1})\n>         if test -n \"$upstream\"\n>         then\n>                 upstream_name=${upstream_name#refs/heads/}\n>         else\n>                 upstream_name=\"@{-1}\"\n>         fi\n>\n> if the change is to be made at that point in the code.\n\nI agree, I will re-roll the patch to use this approach.\n\n> I also wonder if \"git rebase @{-1}\" deserve a similar translation\n> like you are giving \"git rebase -\".\n\nPersonally, I've been using the \"-\" shorthand with \"git checkout\"\nfor a year or so, but only learned about \"@{-1}\" a few months ago.\nI think those who use \"@{-1}\" are familiar enough with the concept\nthat they don't need to have the reference translated to a symbolic\nfull name. Users familiar with \"-\" might not be aware of \"@{-1}\",\nhowever, so I'd prefer not to output it as we are currently.\n\nFurthermore, were we to translate \"@{-1}\", does that mean we\nshould also translate \"@{-2}\" or prior? I don't think that's the case,\nbut then only translating \"@{-1}\" would seem inconsistent.\nFrom that point of view I'd prefer to simply translate \"-\",\nnot \"@{-1}\".\n\n- Brian Gesiak\n"},{"id":"238946","messageId":"xmqqk3api4yy.fsf@gitster.dls.corp.google.com","threadId":"36408","inReplyTo":"CAN7MxmUikP6pVAj3cpDiSbFxawScTh5zKusPUe8SpkNbH=e6Aw@mail.gmail.com","subject":"Re: [PATCH] git-rebase: Print name of rev when using shorthand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T17:01:09Z","receivedAt":"2014-04-16T17:01:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Gesiak <modocache@gmail.com> writes:\n\n> Personally, I've been using the \"-\" shorthand with \"git checkout\"\n> for a year or so, but only learned about \"@{-1}\" a few months ago.\n> I think those who use \"@{-1}\" are familiar enough with the concept\n> that they don't need to have the reference translated to a\n> symbolic full name. Users familiar with \"-\" might not be aware of\n> \"@{-1}\", however, so I'd prefer not to output it as we are\n> currently.\n\nI do not understand that reasoning.\n\nThe concept of \"n-th prior checkout\" (aka @{-n}) and \"immediately\nprevious checkout\" (aka \"-\") are equivalent, even though the former\nmay be more generic.\n\nYou seem to be saying that those who understand the former are with\nsuperiour mental capacity in general than those who only know the\nlatter, and they can always remember where they came from.  It\nsounds similar to an absurd claim (pulled out of thin-air only for\nillustration purposes) that French-speaking people are of superiour\nmind and do not need as much help with math as English speakers.\n\n> Furthermore, were we to translate \"@{-1}\", does that mean we\n> should also translate \"@{-2}\" or prior?\n\nSurely, why not.  If a user is so forgetful to need help remembering\nwhere s/he was immediately before, wouldn't it be more helpful to\ngive \"here is where you were\" reminder for older ones to allow them\nto double check they specified the right thing and spot possible\nmistakes?\n\nI can buy \"that would be a lot more work, and I do not want to do it\n(or I do not think I can solve it in a more general way)\", though.\n"},{"id":"238976","messageId":"xmqqa9blgkew.fsf@gitster.dls.corp.google.com","threadId":"36408","inReplyTo":"xmqqk3api4yy.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-rebase: Print name of rev when using shorthand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T19:10:31Z","receivedAt":"2014-04-16T19:10:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Furthermore, were we to translate \"@{-1}\", does that mean we\n>> should also translate \"@{-2}\" or prior?\n>\n> Surely, why not.  If a user is so forgetful to need help remembering\n> where s/he was immediately before, wouldn't it be more helpful to\n> give \"here is where you were\" reminder for older ones to allow them\n> to double check they specified the right thing and spot possible\n> mistakes?\n\nAfter re-reading the proposed log message of your v2, I notice one\nthing:\n\n    The output from a successful invocation of the shorthand command\n    \"git rebase -\" is something like \"Fast-forwarded HEAD to @{-1}\",\n    which includes a relative reference to a revision. Other\n    commands that use the shorthand \"-\", such as \"git checkout -\",\n    typically display the symbolic name of the revision.\n  \nWhile the above is not incorrect per-se, have you considered _why_\nit is a good thing to show the symbolic name in the first place?\n\nGiving the symbolic name 'master' is good because it is possible\nthat the user thought the previous branch was 'frotz', forgetting\nthat another branch was checked out tentatively in between, and the\nuser ended up rebasing on top of a wrong branch.  Telling what that\nprevious branch is is a way to help user spot such a potential\nmistake.  So I am all for making \"rebase -\" report what concrete\nbranch the branch was replayed on top of, and consider it an incomplete\nimprovement if \"rebase @{-1}\" (or \"rebase @{-2}\") did not get the\nsame help---especially when I know that the underlying mechanism you\nwould use to translate @{-1} back to the concrete branch name is the\nsame for both cases anyway.\n\nBy the way, here is a happy tangent.  I was pleasantly surprised to\nsee what this procedure produced:\n\n    $ git checkout -b ef/send-email-absolute-path maint\n    $ git am -s3c a-patch-by-erik-on-different-topic\n    $ git checkout bg/rebase-off-of-previous-branch\n    $ git am -s3c your-v2-patch\n    $ git checkout jch\n    $ git merge --no-edit -\n    $ git merge --no-edit @{-2}\n    $ git log --first-parent -2 | grep \"Merge branch\"\n\nBoth short-hands are turned into concrete branch names, as they\nshould ;-)\n"},{"id":"238991","messageId":"CAN7MxmWHPaxC0vzJxnJjcarpSh2SsdzhuTbt1rSiJhNS0U8K0Q@mail.gmail.com","threadId":"36408","inReplyTo":"xmqqa9blgkew.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] git-rebase: Print name of rev when using shorthand","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-04-16T23:22:46Z","receivedAt":"2014-04-16T23:22:46Z","isPatch":true,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"> The concept of \"n-th prior checkout\" (aka @{-n}) and \"immediately\n> previous checkout\" (aka \"-\") are equivalent, even though the former\n> may be more generic.\n>\n> You seem to be saying that those who understand the former are with\n> superiour mental capacity in general than those who only know the\n> latter, and they can always remember where they came from.\n>\n> ...have you considered _why_ it is a good thing to show the symbolic\n> name in the first place?\n\nI think I failed to express my point here; I don't think people that\nuse \"@{-1}\" have superior mental capacity, but rather simply that\nthey are aware of the \"@{-n}\" method of specifying a previous reference.\nSo in response to the command \"git rebase @{-4}\", displaying the\nresult \"Fast-forwarded HEAD to @{-4}\" does not contain any unknown\nsyntax that may confuse them. They may not remember what \"@{-4}\"\nrefers to, but they are aware of the syntax at least.\n\nOn the other hand, people who use the \"-\" shorthand may or may\nnot be aware of the \"@{-n}\" syntax. In that respect, I think it would\nbe confusing to display \"Fast-forwarded HEAD to @{-1}\" in response\nto the command \"git rebase -\"; the user may not know what \"@{-1}\"\nmeans!\n\nThus my original point was that I felt displaying a symbolic name in\nresponse to \"git rebase -\" was more important than doing so in\nresponse to \"git rebase @{-1}\". The issue isn't about forgetting what\n\"@{-n}\" refers to, it's whether the user even knows what \"@{-n}\" is\nsupposed to mean.\n\nBut in light of your other comments:\n\n>> Furthermore, were we to translate \"@{-1}\", does that mean we\n>> should also translate \"@{-2}\" or prior?\n>\n> Surely, why not.  If a user is so forgetful to need help remembering\n> where s/he was immediately before, wouldn't it be more helpful to\n> give \"here is where you were\" reminder for older ones to allow them\n> to double check they specified the right thing and spot possible\n> mistakes?\n>\n> ...\n>\n> Giving the symbolic name 'master' is good because it is possible\n> that the user thought the previous branch was 'frotz', forgetting\n> that another branch was checked out tentatively in between, and the\n> user ended up rebasing on top of a wrong branch.  Telling what that\n> previous branch is is a way to help user spot such a potential\n> mistake.  So I am all for making \"rebase -\" report what concrete\n> branch the branch was replayed on top of, and consider it an incomplete\n> improvement if \"rebase @{-1}\" (or \"rebase @{-2}\") did not get the\n> same help---especially when I know that the underlying mechanism you\n> would use to translate @{-1} back to the concrete branch name is the\n> same for both cases anyway.\n\nI had not originally thought of this, perhaps because I was preoccupied\nwith preventing users from seeing syntax they might not be aware of.\nBut I definitely agree that displaying symbolic names for all \"@{-n}\"\nis a good way to prevent user error.\n\n> I can buy \"that would be a lot more work, and I do not want to do it\n> (or I do not think I can solve it in a more general way)\", though.\n\nPerish the thought! :)\n\nI will try to re-roll this patch to include symbolic names for \"@{-n}\".\n\nAs usual, thanks for the feedback!\n\n- Brian Gesiak\n"}]}