Re: [PATCH] git-rebase: Print name of rev when using shorthand
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-rebase: Print name of rev when using shorthand
Junio C Hamano gits...@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 ;-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-rebase: Print name of rev when using shorthand
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-rebase: Print name of rev when using shorthand
Brian Gesiak modoca...@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 j...@keeping.me.uk
Signed-off-by: Brian Gesiak modoca...@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
'
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
