[PATCH] git-rebase: Print name of rev when using shorthand

[PATCH] git-rebase: Print name of rev when using shorthand

[Brian Gesiak]

The output from a successful invocation of the shorthand command
"git rebase -" is something like "Fast-forwarded HEAD to @{-1}",
which includes a relative reference to a revision. Other commands
that use the shorthand "-", such as "git checkout -", typically
display the symbolic name of the revision.

Change rebase to output the symbolic name of the revision when using
the shorthand. For the example above, the new output is
"Fast-forwarded HEAD to master", assuming "@{-1}" is a reference to
"master".

- Use "git name-rev" to retreive the name of the rev.
- Update the tests in light of this new behavior.

Requested-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Brian Gesiak <modocache@gmail.com>
---
Previous discussion on this issue:
http://article.gmane.org/gmane.comp.version-control.git/244340

 git-rebase.sh     | 2 +-
 t/t3400-rebase.sh | 4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 2c75e9f..ab0e081 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -455,7 +455,7 @@ then
 	*)	upstream_name="$1"
 		if test "$upstream_name" = "-"
 		then
-			upstream_name="@{-1}"
+			upstream_name=`git name-rev --name-only @{-1}`
 		fi
 		shift
 		;;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 80e0a95..2b99940 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -91,7 +91,7 @@ test_expect_success 'rebase from ambiguous branch name' '
 test_expect_success 'rebase off of the previous branch using "-"' '
 	git checkout master &&
 	git checkout HEAD^ &&
-	git rebase @{-1} >expect.messages &&
+	git rebase master >expect.messages &&
 	git merge-base master HEAD >expect.forkpoint &&
 
 	git checkout master &&
@@ -100,8 +100,6 @@ test_expect_success 'rebase off of the previous branch using "-"' '
 	git merge-base master HEAD >actual.forkpoint &&
 
 	test_cmp expect.forkpoint actual.forkpoint &&
-	# the next one is dubious---we may want to say "-",
-	# instead of @{-1}, in the message
 	test_i18ncmp expect.messages actual.messages
 '
 
-- 
1.9.0.259.gc5d75e8.dirty

Re: [PATCH] git-rebase: Print name of rev when using shorthand

[Junio C Hamano]

Brian Gesiak <modocache@gmail.com> writes:

> The output from a successful invocation of the shorthand command
> "git rebase -" is something like "Fast-forwarded HEAD to @{-1}",
> which includes a relative reference to a revision. Other commands
> that use the shorthand "-", such as "git checkout -", typically
> display the symbolic name of the revision.
>
> Change rebase to output the symbolic name of the revision when using
> the shorthand. For the example above, the new output is
> "Fast-forwarded HEAD to master", assuming "@{-1}" is a reference to
> "master".
>
> - Use "git name-rev" to retreive the name of the rev.
> - Update the tests in light of this new behavior.
>
> Requested-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Brian Gesiak <modocache@gmail.com>
> ---

What the patch wants to implement sounds sensible, but I do not
think name-rev is a right tool for this.  Imagine the case where
there are more than one branches whose tip points at the commit you
came from.  name-rev will not be able to pick correctly which one to
report.

Also think what happens if you were previously on a detached HEAD?

I think you would want to use something like:

        upstream_name=$(git rev-parse --symbolic-full-name @{-1})
        if test -n "$upstream"
        then
                upstream_name=${upstream_name#refs/heads/}
        else
                upstream_name="@{-1}"
        fi

if the change is to be made at that point in the code.

I also wonder if "git rebase @{-1}" deserve a similar translation
like you are giving "git rebase -".

> Previous discussion on this issue:
> http://article.gmane.org/gmane.comp.version-control.git/244340
>
>  git-rebase.sh     | 2 +-
>  t/t3400-rebase.sh | 4 +---
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2c75e9f..ab0e081 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -455,7 +455,7 @@ then
>  	*)	upstream_name="$1"
>  		if test "$upstream_name" = "-"
>  		then
> -			upstream_name="@{-1}"
> +			upstream_name=`git name-rev --name-only @{-1}`
>  		fi
>  		shift
>  		;;
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 80e0a95..2b99940 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -91,7 +91,7 @@ test_expect_success 'rebase from ambiguous branch name' '
>  test_expect_success 'rebase off of the previous branch using "-"' '
>  	git checkout master &&
>  	git checkout HEAD^ &&
> -	git rebase @{-1} >expect.messages &&
> +	git rebase master >expect.messages &&

OK.

>  	git merge-base master HEAD >expect.forkpoint &&
>  
>  	git checkout master &&
> @@ -100,8 +100,6 @@ test_expect_success 'rebase off of the previous branch using "-"' '
>  	git merge-base master HEAD >actual.forkpoint &&
>  
>  	test_cmp expect.forkpoint actual.forkpoint &&
> -	# the next one is dubious---we may want to say "-",
> -	# instead of @{-1}, in the message
>  	test_i18ncmp expect.messages actual.messages
>  '

Re: [PATCH] git-rebase: Print name of rev when using shorthand

[Brian Gesiak]

Thank you for the feedback!

> Imagine the case where there are more than one branches
> whose tip points at the commit you came from.
> name-rev will not be able to pick correctly which one to report.

I see. Yes, you're exactly right; the following demonstrates
the problem:

$ git checkout -b xylophone master
$ git checkout -b aardvark master
$ git name-rev --name-only @{-1} # I'd want "xylophone", but this
outputs "aardvark"

So it appears name-rev is not up to the task here.

> I think you would want to use something like:
>
>         upstream_name=$(git rev-parse --symbolic-full-name @{-1})
>         if test -n "$upstream"
>         then
>                 upstream_name=${upstream_name#refs/heads/}
>         else
>                 upstream_name="@{-1}"
>         fi
>
> if the change is to be made at that point in the code.

I agree, I will re-roll the patch to use this approach.

> I also wonder if "git rebase @{-1}" deserve a similar translation
> like you are giving "git rebase -".

Personally, I've been using the "-" shorthand with "git checkout"
for a year or so, but only learned about "@{-1}" a few months ago.
I think those who use "@{-1}" are familiar enough with the concept
that they don't need to have the reference translated to a symbolic
full name. Users familiar with "-" might not be aware of "@{-1}",
however, so I'd prefer not to output it as we are currently.

Furthermore, were we to translate "@{-1}", does that mean we
should also translate "@{-2}" or prior? I don't think that's the case,
but then only translating "@{-1}" would seem inconsistent.
From that point of view I'd prefer to simply translate "-",
not "@{-1}".

- Brian Gesiak

Re: [PATCH] git-rebase: Print name of rev when using shorthand

[Junio C Hamano]

Brian Gesiak <modocache@gmail.com> writes:

> Personally, I've been using the "-" shorthand with "git checkout"
> for a year or so, but only learned about "@{-1}" a few months ago.
> I think those who use "@{-1}" are familiar enough with the concept
> that they don't need to have the reference translated to a
> symbolic full name. Users familiar with "-" might not be aware of
> "@{-1}", however, so I'd prefer not to output it as we are
> currently.

I do not understand that reasoning.

The concept of "n-th prior checkout" (aka @{-n}) and "immediately
previous checkout" (aka "-") are equivalent, even though the former
may be more generic.

You seem to be saying that those who understand the former are with
superiour mental capacity in general than those who only know the
latter, and they can always remember where they came from.  It
sounds similar to an absurd claim (pulled out of thin-air only for
illustration purposes) that French-speaking people are of superiour
mind and do not need as much help with math as English speakers.

> Furthermore, were we to translate "@{-1}", does that mean we
> should also translate "@{-2}" or prior?

Surely, why not.  If a user is so forgetful to need help remembering
where s/he was immediately before, wouldn't it be more helpful to
give "here is where you were" reminder for older ones to allow them
to double check they specified the right thing and spot possible
mistakes?

I can buy "that would be a lot more work, and I do not want to do it
(or I do not think I can solve it in a more general way)", though.

Re: [PATCH] git-rebase: Print name of rev when using shorthand

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Furthermore, were we to translate "@{-1}", does that mean we
>> should also translate "@{-2}" or prior?
>
> Surely, why not.  If a user is so forgetful to need help remembering
> where s/he was immediately before, wouldn't it be more helpful to
> give "here is where you were" reminder for older ones to allow them
> to double check they specified the right thing and spot possible
> mistakes?

After re-reading the proposed log message of your v2, I notice one
thing:

    The output from a successful invocation of the shorthand command
    "git rebase -" is something like "Fast-forwarded HEAD to @{-1}",
    which includes a relative reference to a revision. Other
    commands that use the shorthand "-", such as "git checkout -",
    typically display the symbolic name of the revision.
  
While the above is not incorrect per-se, have you considered _why_
it is a good thing to show the symbolic name in the first place?

Giving the symbolic name 'master' is good because it is possible
that the user thought the previous branch was 'frotz', forgetting
that another branch was checked out tentatively in between, and the
user ended up rebasing on top of a wrong branch.  Telling what that
previous branch is is a way to help user spot such a potential
mistake.  So I am all for making "rebase -" report what concrete
branch the branch was replayed on top of, and consider it an incomplete
improvement if "rebase @{-1}" (or "rebase @{-2}") did not get the
same help---especially when I know that the underlying mechanism you
would use to translate @{-1} back to the concrete branch name is the
same for both cases anyway.

By the way, here is a happy tangent.  I was pleasantly surprised to
see what this procedure produced:

    $ git checkout -b ef/send-email-absolute-path maint
    $ git am -s3c a-patch-by-erik-on-different-topic
    $ git checkout bg/rebase-off-of-previous-branch
    $ git am -s3c your-v2-patch
    $ git checkout jch
    $ git merge --no-edit -
    $ git merge --no-edit @{-2}
    $ git log --first-parent -2 | grep "Merge branch"

Both short-hands are turned into concrete branch names, as they
should ;-)

Re: [PATCH] git-rebase: Print name of rev when using shorthand

[Brian Gesiak]

> The concept of "n-th prior checkout" (aka @{-n}) and "immediately
> previous checkout" (aka "-") are equivalent, even though the former
> may be more generic.
>
> You seem to be saying that those who understand the former are with
> superiour mental capacity in general than those who only know the
> latter, and they can always remember where they came from.
>
> ...have you considered _why_ it is a good thing to show the symbolic
> name in the first place?

I think I failed to express my point here; I don't think people that
use "@{-1}" have superior mental capacity, but rather simply that
they are aware of the "@{-n}" method of specifying a previous reference.
So in response to the command "git rebase @{-4}", displaying the
result "Fast-forwarded HEAD to @{-4}" does not contain any unknown
syntax that may confuse them. They may not remember what "@{-4}"
refers to, but they are aware of the syntax at least.

On the other hand, people who use the "-" shorthand may or may
not be aware of the "@{-n}" syntax. In that respect, I think it would
be confusing to display "Fast-forwarded HEAD to @{-1}" in response
to the command "git rebase -"; the user may not know what "@{-1}"
means!

Thus my original point was that I felt displaying a symbolic name in
response to "git rebase -" was more important than doing so in
response to "git rebase @{-1}". The issue isn't about forgetting what
"@{-n}" refers to, it's whether the user even knows what "@{-n}" is
supposed to mean.

But in light of your other comments:

>> Furthermore, were we to translate "@{-1}", does that mean we
>> should also translate "@{-2}" or prior?
>
> Surely, why not.  If a user is so forgetful to need help remembering
> where s/he was immediately before, wouldn't it be more helpful to
> give "here is where you were" reminder for older ones to allow them
> to double check they specified the right thing and spot possible
> mistakes?
>
> ...
>
> Giving the symbolic name 'master' is good because it is possible
> that the user thought the previous branch was 'frotz', forgetting
> that another branch was checked out tentatively in between, and the
> user ended up rebasing on top of a wrong branch.  Telling what that
> previous branch is is a way to help user spot such a potential
> mistake.  So I am all for making "rebase -" report what concrete
> branch the branch was replayed on top of, and consider it an incomplete
> improvement if "rebase @{-1}" (or "rebase @{-2}") did not get the
> same help---especially when I know that the underlying mechanism you
> would use to translate @{-1} back to the concrete branch name is the
> same for both cases anyway.

I had not originally thought of this, perhaps because I was preoccupied
with preventing users from seeing syntax they might not be aware of.
But I definitely agree that displaying symbolic names for all "@{-n}"
is a good way to prevent user error.

> I can buy "that would be a lot more work, and I do not want to do it
> (or I do not think I can solve it in a more general way)", though.

Perish the thought! :)

I will try to re-roll this patch to include symbolic names for "@{-n}".

As usual, thanks for the feedback!

- Brian Gesiak