Re: [PATCH v2 0/6] Fix checkout-dash to work with rebase
Junio C Hamano wrote:
I actually tried to reorder the patches and they seem to work OK as
expected. And I think it makes sense to order them the way I've
been suggesting, so I'll tentatively queue the result of reordering
on 'rr/rebase-checkout-reflog' and push it out as a part of 'pu'.
Please check to see if I introduced a new bug while doing so.
Thanks for the reorder and commit message tweaks. I'm working on the
series you put up on `pu` now.
For example, the one in git reabse does this:
GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: checkout $onto_name
git checkout -q $onto^0 || die could not detach HEAD
git update-ref ORIG_HEAD $orig_head
...
run_specific_rebase
But the specific rebase, e.g. git-rebase--interactive, does this:
case $head_name in
refs/*)
message=$GIT_REFLOG_ACTION: $head_name onto $onto
git update-ref -m $message $head_name $newhead $orig_head
git symbolic-ref \
-m $GIT_REFLOG_ACTION: returning to $head_name \
HEAD $head_name
;;
esac {
I think the message you added to git reabse is only meant for that
specific checkout $onto, but it is set globally. Wouldn't it
affect later use, which expected it to be rebase and nothing else?
Both rebase.sh and rebase--interactive.sh set a sane GIT_REFLOG_ACTION
right on top (using set_reflog_action), so no worries. I'll just
double-check to make sure that no bogus/ incorrect messages are
printed.
Perhaps something like this on top of the entire series may be
sufficient (which will be queued as SQUASH??? at the tip).
I think this takes the wrong approach to the problem. In my opinion,
the correct approach is to actually overshadow die() with a function
that clears GIT_REFLOG_ACTION before calling die().
git grep -C2 'git checkout' -- git-rebase\*.sh
Ugh. I'll check all the codepaths thoroughly before submitting a re-roll.
Thanks.
--
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 v2 0/6] Fix checkout-dash to work with rebase
Ramkumar Ramachandra wrote: Thanks for the reorder and commit message tweaks. I'm working on the series you put up on `pu` now. That said, I do not agree with one of your commit message updates: checkout: respect GIT_REFLOG_ACTION GIT_REFLOG_ACTION is an environment variable specifying the reflog message to write after an action is completed. Several other commands including merge, reset, and commit respect it. Fix the failing tests in t/checkout-last by making checkout respect it too. You can now expect $ git checkout - to work as expected after any operation that internally uses checkout as its implementation detail, e.g. rebase. After the patch you _cannot_ expect checkout-dash to work after any operation that internally uses checkout; it's limited to rebase. Many other scripts (eg. bisect) do _not_ set GIT_REFLOG_ACTION at all. -- 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 v2 0/6] Fix checkout-dash to work with rebase
Junio C Hamano wrote: diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index a58406d..c175ef1 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -934,6 +934,21 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' ' test L = $(git cat-file commit HEAD | sed -ne \$p) ' +test_expect_success 'rebase -i produces readable reflog' ' I don't know if this test is a good idea at all. We never directly check the messages written by a command to the reflog, and I suspect that there is a good reason for this: the format of .git/logs is not guaranteed to be stable, and the messages written by various commands are not guaranteed to be stable either; the only machine-parsing of reflogs we do is very minimal: interpret_nth_prior_checkout() and grab_1st_switch(). A quick $ git grep .git/logs -- t shows that I'm mostly right about this. Why make an exception in the case of rebase -i? In the worst case, the patch atleast needs to be discussed as an independent patch: it's certainly not obvious enough to sneak into this series. I'll submit a re-roll without this hunk. -- 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 v2 0/6] Fix checkout-dash to work with rebase
Ramkumar Ramachandra artag...@gmail.com writes: Junio C Hamano wrote: diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index a58406d..c175ef1 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -934,6 +934,21 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' ' test L = $(git cat-file commit HEAD | sed -ne \$p) ' +test_expect_success 'rebase -i produces readable reflog' ' I don't know if this test is a good idea at all. It is. This catches a change to GIT_REFLOG_ACTION that is meant to only apply to checkout done internally in git-rebase leaks to the later codepath and affects the reflog message left by rebase--interactive. Apply the test change without the do not leak part in the fix-up (queued as a single SQUASH??? commit on 'pu') to what you posted earlier and see how it breaks. # rr/rebase-checkout-reflog with fix-up $ git checkout 33b1cdeb # revert the fix but keep the test $ git checkout HEAD^ -- git-rebase--interactive.sh git-rebase.sh # run the test $ make cd t sh ./t3404-*.sh -v -i The test fails with this: --- expect 2013-06-18 16:09:21.0 + +++ actual 2013-06-18 16:09:21.0 + @@ -1,4 +1,4 @@ -rebase -i (start): checkout I +rebase -i (start): checkout branch-reflog-test: checkout I rebase -i (pick): G rebase -i (pick): H rebase -i (finish): returning to refs/heads/branch-reflog-test checkout branch-reflog-test part is leaking from your setting of GIT_REFLOG_ACTION for git checkout in git rebase; it is only necessary to affect that checkout and it should not affect later commands that write reflog entries, but we see the breakage. And that is why I did the fix-up to make sure your changes only apply to git checkout. If you apply the test part of that fixup to what you posted today, what would you see? I didn't look at the patches very closely, but I wouldn't be surprised if they are still leaking the change meant only to checkout to the later stages and breaking that test (the reason why I would not be surprised is because the impression I am getting from reading your responses is that you do not understand why it is bad not to limit the setting only to checkout). Why make an exception in the case of rebase -i? Because the whole point of last two patches are word reflog messages better for rebase, the improvements made by these two patches are better protected from future changes; it also makes sure that deliberate improvements will show up as changes to the tests. -- 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 v2 0/6] Fix checkout-dash to work with rebase
Junio C Hamano wrote: Apply the test change without the do not leak part in the fix-up (queued as a single SQUASH??? commit on 'pu') to what you posted earlier and see how it breaks. --- expect 2013-06-18 16:09:21.0 + +++ actual 2013-06-18 16:09:21.0 + @@ -1,4 +1,4 @@ -rebase -i (start): checkout I +rebase -i (start): checkout branch-reflog-test: checkout I rebase -i (pick): G rebase -i (pick): H rebase -i (finish): returning to refs/heads/branch-reflog-test If you apply the test part of that fixup to what you posted today, what would you see? Damn it! I didn't realize that your subshell was for a do-not-leak. I'm feeling especially stupid now. Will post a re-roll in a bit. -- 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 v2 0/6] Fix checkout-dash to work with rebase
Junio C Hamano gits...@pobox.com writes:
In other words, the order I was anticipating to see after the
discussion (this is different from saying A series that is not
ordered like this is unacceptable) was:
wt-status: remove unused field in grab_1st_switch_cbdata
This is an unrelated clean-up, and can be done before anything else.
t/checkout-last: checkout - doesn't work after rebase
This spells out what we want to happen at the end and marks the
current breakage.
status: do not depend on rebase reflog messages
This compensates for fallouts from the next change.
checkout: respect GIT_REFLOG_ACTION
And this is the fix, the most important step.
rebase: prepare to write reflog message for checkout
rebase -i: prepare to write reflog message for checkout
And these are icing on the cake, but that cannot be done before
status is fixed.
I actually tried to reorder the patches and they seem to work OK as
expected. And I think it makes sense to order them the way I've
been suggesting, so I'll tentatively queue the result of reordering
on 'rr/rebase-checkout-reflog' and push it out as a part of 'pu'.
Please check to see if I introduced a new bug while doing so.
Regardless of the ordering, however, I suspect two patches that
change the message recorded in reflog in rebase need further
fixing.
For example, the one in git reabse does this:
GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: checkout $onto_name
git checkout -q $onto^0 || die could not detach HEAD
git update-ref ORIG_HEAD $orig_head
...
run_specific_rebase
But the specific rebase, e.g. git-rebase--interactive, does this:
case $head_name in
refs/*)
message=$GIT_REFLOG_ACTION: $head_name onto $onto
git update-ref -m $message $head_name $newhead $orig_head
git symbolic-ref \
-m $GIT_REFLOG_ACTION: returning to $head_name \
HEAD $head_name
;;
esac {
I think the message you added to git reabse is only meant for that
specific checkout $onto, but it is set globally. Wouldn't it
affect later use, which expected it to be rebase and nothing else?
Perhaps something like this on top of the entire series may be
sufficient (which will be queued as SQUASH??? at the tip).
Hint for anybody (not limited to Ram):
There also are other 'git checkout' invocations in git-rebase\*.sh
that are not yet covered by these nicer reflog message patches,
which may want to be fixed up before these two icing-on-cake patches
become ready to be merged; see output of
git grep -C2 'git checkout' -- git-rebase\*.sh
Thanks.
git-rebase--interactive.sh| 15 ++-
git-rebase.sh | 4 ++--
t/t3404-rebase-interactive.sh | 15 +++
3 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a05a6e4..f21ff7c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -837,9 +837,11 @@ comment_for_reflog start
if test ! -z $switch_to
then
- GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: checkout $switch_to
- output git checkout $switch_to -- ||
- die Could not checkout $switch_to
+ (
+ GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: checkout $switch_to
+ export GIT_REFLOG_ACTION
+ output git checkout $switch_to --
+ ) || die Could not checkout $switch_to
fi
orig_head=$(git rev-parse --verify HEAD) || die No HEAD?
@@ -981,7 +983,10 @@ has_action $todo ||
test -d $rewritten || test -n $force_rebase || skip_unnecessary_picks
-GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: checkout $onto_name
-output git checkout $onto || die_abort could not detach HEAD
+(
+ GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: checkout $onto_name
+ export GIT_REFLOG_ACTION
+ output git checkout $onto
+) || die_abort could not detach HEAD
git update-ref ORIG_HEAD $orig_head
do_rest
diff --git a/git-rebase.sh b/git-rebase.sh
index 7d55372..2344eef 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -523,8 +523,8 @@ test $type = interactive run_specific_rebase
# Detach HEAD and reset the tree
say $(gettext First, rewinding head to replay your work on top of it...)
-GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: checkout $onto_name
-git checkout -q $onto^0 || die could not detach HEAD
+GIT_REFLOG_ACTION=$GIT_REFLOG_ACTION: checkout $onto_name \
+ git checkout -q $onto^0 || die could not detach HEAD
git update-ref ORIG_HEAD $orig_head
# If the $onto is a proper descendant of the tip of the branch, then
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index a58406d..c175ef1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -934,6 +934,21 @@ test_expect_success 'rebase --edit-todo can be used to
modify todo' '
test L = $(git cat-file commit HEAD | sed -ne \$p)
'
Re: [PATCH v2 0/6] Fix checkout-dash to work with rebase
Ramkumar Ramachandra artag...@gmail.com writes: So after extensive discussions with Junio, I have updated [5/6] to special-case rebase and rebase -i instead of dropping the HEAD detached from message altogether. Also, [1/6] includes two more tests, as suggested by Junio. Junio: The message is now the constant rebase in progress; onto $ONTO. That message is better than I was anticipating. I was actually assuming that you would leave them as they are i.e. # HEAD detached at xxx, as we agreed that we will see them improved anyway by the other topic. I am still puzzled to see that the change to checkout comes at the very end. With do not depend on rebase reflog messages done before the checkout: respect reflog-action, I was hoping that we can have changes to the actual reflog message made to rebase (patch 2 and 3) can be done at the very end. Was I missing something else? In other words, the order I was anticipating to see after the discussion (this is different from saying A series that is not ordered like this is unacceptable) was: wt-status: remove unused field in grab_1st_switch_cbdata This is an unrelated clean-up, and can be done before anything else. t/checkout-last: checkout - doesn't work after rebase This spells out what we want to happen at the end and marks the current breakage. status: do not depend on rebase reflog messages This compensates for fallouts from the next change. checkout: respect GIT_REFLOG_ACTION And this is the fix, the most important step. rebase: prepare to write reflog message for checkout rebase -i: prepare to write reflog message for checkout And these are icing on the cake, but that cannot be done before status is fixed. -- 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
