git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog
@ 2013-06-18 18:55 Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 1/7] t/t7512-status-help: test "HEAD detached from" Ramkumar Ramachandra
                   ` (6 more replies)
  0 siblings, 7 replies; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 18:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

In this iteration, I've redone [6/7] and [7/7] to address the "do not
leak GIT_REFLOG_ACTION" problem that Junio talked about.  Predictably,
I couldn't convince myself to take Junio's subshell approach.
Instead, I've reworked the entire logic to have two variables:
GIT_REFLOG_ACTION and base_reflog_action.  It is now elegant and
correct.

I'm elated that I got the chance to do this right.

Thanks.

Junio C Hamano (1):
  t/t7512-status-help: test "HEAD detached from"

Ramkumar Ramachandra (6):
  wt-status: remove unused field in grab_1st_switch_cbdata
  t/t2012-checkout-last: test "checkout -" after a rebase
  status: do not depend on rebase reflog messages
  checkout: respect GIT_REFLOG_ACTION
  rebase: write better reflog messages
  rebase -i: write better reflog messages

 builtin/checkout.c            | 11 +++++++---
 git-am.sh                     |  4 +++-
 git-rebase--am.sh             |  5 +++++
 git-rebase--interactive.sh    | 14 +++++++++----
 git-rebase.sh                 | 13 ++++++++++--
 t/t2012-checkout-last.sh      | 34 +++++++++++++++++++++++++++++++
 t/t3404-rebase-interactive.sh | 15 ++++++++++++++
 t/t7512-status-help.sh        | 47 ++++++++++++++++++++++++-------------------
 wt-status.c                   |  7 ++++---
 9 files changed, 116 insertions(+), 34 deletions(-)

-- 
1.8.3.1.455.g5932b31

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2 1/7] t/t7512-status-help: test "HEAD detached from"
  2013-06-18 18:55 [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog Ramkumar Ramachandra
@ 2013-06-18 18:55 ` Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 2/7] wt-status: remove unused field in grab_1st_switch_cbdata Ramkumar Ramachandra
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 18:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

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

b397ea (status: show more info than "currently not on any branch",
2013-03-13) wanted to make sure that after a checkout to detach HEAD,
the user can see where the HEAD was originally detached from.  The last
test added by that commit to t/status-help shows one example,
immediately after HEAD is detached via a checkout.  Enhance that test to
test the "HEAD detached from" message is displayed when the user further
resets to another commit.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t7512-status-help.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index bf08d4e..bafa5e7 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -667,7 +667,7 @@ test_expect_success 'status when cherry-picking after resolving conflicts' '
 	test_i18ncmp expected actual
 '
 
-test_expect_success 'status showing detached from a tag' '
+test_expect_success 'status showing detached at and from a tag' '
 	test_commit atag tagging &&
 	git checkout atag &&
 	cat >expected <<-\EOF
@@ -675,6 +675,14 @@ test_expect_success 'status showing detached from a tag' '
 	nothing to commit (use -u to show untracked files)
 	EOF
 	git status --untracked-files=no >actual &&
+	test_i18ncmp expected actual &&
+
+	git reset --hard HEAD^ &&
+	cat >expected <<-\EOF
+	# HEAD detached from atag
+	nothing to commit (use -u to show untracked files)
+	EOF
+	git status --untracked-files=no >actual &&
 	test_i18ncmp expected actual
 '
 
-- 
1.8.3.1.455.g5932b31

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 2/7] wt-status: remove unused field in grab_1st_switch_cbdata
  2013-06-18 18:55 [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 1/7] t/t7512-status-help: test "HEAD detached from" Ramkumar Ramachandra
@ 2013-06-18 18:55 ` Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 3/7] t/t2012-checkout-last: test "checkout -" after a rebase Ramkumar Ramachandra
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 18:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

The struct grab_1st_switch_cbdata has the field "found", which is
set in grab_1st_switch() when a match is found.  This information is
redundant and unused by any code.  The return value of the function
serves to communicate this information anyway.

Remove the field.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wt-status.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index bf84a86..2511270 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1035,7 +1035,6 @@ got_nothing:
 }
 
 struct grab_1st_switch_cbdata {
-	int found;
 	struct strbuf buf;
 	unsigned char nsha1[20];
 };
@@ -1059,7 +1058,6 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
 	for (end = target; *end && *end != '\n'; end++)
 		;
 	strbuf_add(&cb->buf, target, end - target);
-	cb->found = 1;
 	return 1;
 }
 
-- 
1.8.3.1.455.g5932b31

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 3/7] t/t2012-checkout-last: test "checkout -" after a rebase
  2013-06-18 18:55 [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 1/7] t/t7512-status-help: test "HEAD detached from" Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 2/7] wt-status: remove unused field in grab_1st_switch_cbdata Ramkumar Ramachandra
@ 2013-06-18 18:55 ` Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 4/7] status: do not depend on rebase reflog messages Ramkumar Ramachandra
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 18:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

  $ git checkout -

does not work as expected after a rebase.  This is because the
reflog records "checkout" made by "rebase" as its implementation
detail the same way as end-user initiated "checkout", and makes it
count as the branch that was previously checked out.

Add four failing tests documenting this bug: two for a normal rebase,
and another two for an interactive rebase.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t2012-checkout-last.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index b44de9d..6ad6edf 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -116,4 +116,38 @@ test_expect_success 'master...' '
 	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify master^)"
 '
 
+test_expect_failure '"checkout -" works after a rebase A' '
+	git checkout master &&
+	git checkout other &&
+	git rebase master &&
+	git checkout - &&
+	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
+'
+
+test_expect_failure '"checkout -" works after a rebase A B' '
+	git branch moodle master~1 &&
+	git checkout master &&
+	git checkout other &&
+	git rebase master moodle &&
+	git checkout - &&
+	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
+'
+
+test_expect_failure '"checkout -" works after a rebase -i A' '
+	git checkout master &&
+	git checkout other &&
+	git rebase -i master &&
+	git checkout - &&
+	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
+'
+
+test_expect_failure '"checkout -" works after a rebase -i A B' '
+	git branch foodle master~1 &&
+	git checkout master &&
+	git checkout other &&
+	git rebase master foodle &&
+	git checkout - &&
+	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
+'
+
 test_done
-- 
1.8.3.1.455.g5932b31

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 4/7] status: do not depend on rebase reflog messages
  2013-06-18 18:55 [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog Ramkumar Ramachandra
                   ` (2 preceding siblings ...)
  2013-06-18 18:55 ` [PATCH v2 3/7] t/t2012-checkout-last: test "checkout -" after a rebase Ramkumar Ramachandra
@ 2013-06-18 18:55 ` Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 5/7] checkout: respect GIT_REFLOG_ACTION Ramkumar Ramachandra
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 18:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

b397ea4 (status: show more info than "currently not on any branch",
2013-03-13) attempted to make the output of 'git status' richer in
the case of a detached HEAD.  Before this patch, with a detached
HEAD, we saw:

  $ git status
  # Not currently on any branch.

But after the patch, we see:

  $ git checkout v1.8.2
  $ git status
  # HEAD detached at v1.8.2

It works by digging the reflog for the most recent message of the
form "checkout: moving from xxxx to yyyy".  It then asserts that
HEAD and "yyyy" are the same, and displays this message.  When they
aren't equal, it displays:

  $ git status
  # HEAD detached from fe11db

so that the user can see where the HEAD was first detached.

In case of a rebase [-i] operation in progress, this message depends on
the implementation of rebase writing "checkout: " messages to the
reflog, but that is an implementation detail of "rebase".  To remove
this dependency so that rebase can be updated to write better reflog
messages, replace this "HEAD detached from" message with:

  # rebase in progress; onto $ONTO

Changes to the commit object name in the expected output for some of the
tests shows that what the test expected "status" to show during "rebase
-i" was not consistent with the output during a vanilla "rebase", which
showed on top of what commit the series is being replayed.  Now we
consistently show something meaningful to the end user.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t7512-status-help.sh | 37 +++++++++++++++++--------------------
 wt-status.c            |  5 ++++-
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index bafa5e7..d6c66d7 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -77,7 +77,7 @@ test_expect_success 'status when rebase in progress before resolving conflicts'
 	ONTO=$(git rev-parse --short HEAD^^) &&
 	test_must_fail git rebase HEAD^ --onto HEAD^^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently rebasing branch '\''rebase_conflicts'\'' on '\''$ONTO'\''.
 	#   (fix conflicts and then run "git rebase --continue")
 	#   (use "git rebase --skip" to skip this patch)
@@ -104,7 +104,7 @@ test_expect_success 'status when rebase in progress before rebase --continue' '
 	echo three >main.txt &&
 	git add main.txt &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently rebasing branch '\''rebase_conflicts'\'' on '\''$ONTO'\''.
 	#   (all conflicts fixed: run "git rebase --continue")
 	#
@@ -136,7 +136,7 @@ test_expect_success 'status during rebase -i when conflicts unresolved' '
 	ONTO=$(git rev-parse --short rebase_i_conflicts) &&
 	test_must_fail git rebase -i rebase_i_conflicts &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO'\''.
 	#   (fix conflicts and then run "git rebase --continue")
 	#   (use "git rebase --skip" to skip this patch)
@@ -162,7 +162,7 @@ test_expect_success 'status during rebase -i after resolving conflicts' '
 	test_must_fail git rebase -i rebase_i_conflicts &&
 	git add main.txt &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO'\''.
 	#   (all conflicts fixed: run "git rebase --continue")
 	#
@@ -188,10 +188,9 @@ test_expect_success 'status when rebasing -i in edit mode' '
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short HEAD~2) &&
-	TGT=$(git rev-parse --short two_rebase_i) &&
 	git rebase -i HEAD~2 &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $TGT
+	# rebase in progress; onto $ONTO
 	# You are currently editing a commit while rebasing branch '\''rebase_i_edit'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -216,9 +215,8 @@ test_expect_success 'status when splitting a commit' '
 	ONTO=$(git rev-parse --short HEAD~3) &&
 	git rebase -i HEAD~3 &&
 	git reset HEAD^ &&
-	TGT=$(git rev-parse --short HEAD) &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $TGT
+	# rebase in progress; onto $ONTO
 	# You are currently splitting a commit while rebasing branch '\''split_commit'\'' on '\''$ONTO'\''.
 	#   (Once your working directory is clean, run "git rebase --continue")
 	#
@@ -246,11 +244,10 @@ test_expect_success 'status after editing the last commit with --amend during a
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	TGT=$(git rev-parse --short three_amend) &&
 	git rebase -i HEAD~3 &&
 	git commit --amend -m "foo" &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $TGT
+	# rebase in progress; onto $ONTO
 	# You are currently editing a commit while rebasing branch '\''amend_last'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -280,7 +277,7 @@ test_expect_success 'status: (continue first edit) second edit' '
 	git rebase -i HEAD~3 &&
 	git rebase --continue &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -302,7 +299,7 @@ test_expect_success 'status: (continue first edit) second edit and split' '
 	git rebase --continue &&
 	git reset HEAD^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (Once your working directory is clean, run "git rebase --continue")
 	#
@@ -329,7 +326,7 @@ test_expect_success 'status: (continue first edit) second edit and amend' '
 	git rebase --continue &&
 	git commit --amend -m "foo" &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -351,7 +348,7 @@ test_expect_success 'status: (amend first edit) second edit' '
 	git commit --amend -m "a" &&
 	git rebase --continue &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -374,7 +371,7 @@ test_expect_success 'status: (amend first edit) second edit and split' '
 	git rebase --continue &&
 	git reset HEAD^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (Once your working directory is clean, run "git rebase --continue")
 	#
@@ -402,7 +399,7 @@ test_expect_success 'status: (amend first edit) second edit and amend' '
 	git rebase --continue &&
 	git commit --amend -m "d" &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -426,7 +423,7 @@ test_expect_success 'status: (split first edit) second edit' '
 	git commit -m "e" &&
 	git rebase --continue &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -451,7 +448,7 @@ test_expect_success 'status: (split first edit) second edit and split' '
 	git rebase --continue &&
 	git reset HEAD^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (Once your working directory is clean, run "git rebase --continue")
 	#
@@ -481,7 +478,7 @@ test_expect_success 'status: (split first edit) second edit and amend' '
 	git rebase --continue &&
 	git commit --amend -m "h" &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -601,7 +598,7 @@ test_expect_success 'status when rebase conflicts with statushints disabled' '
 	ONTO=$(git rev-parse --short HEAD^^) &&
 	test_must_fail git rebase HEAD^ --onto HEAD^^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# rebase in progress; onto $ONTO
 	# You are currently rebasing branch '\''statushints_disabled'\'' on '\''$ONTO'\''.
 	#
 	# Unmerged paths:
diff --git a/wt-status.c b/wt-status.c
index 2511270..85a00f1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1174,7 +1174,10 @@ void wt_status_print(struct wt_status *s)
 			branch_name += 11;
 		else if (!strcmp(branch_name, "HEAD")) {
 			branch_status_color = color(WT_STATUS_NOBRANCH, s);
-			if (state.detached_from) {
+			if (state.rebase_in_progress || state.rebase_interactive_in_progress) {
+				on_what = _("rebase in progress; onto ");
+				branch_name = state.onto;
+			} else if (state.detached_from) {
 				unsigned char sha1[20];
 				branch_name = state.detached_from;
 				if (!get_sha1("HEAD", sha1) &&
-- 
1.8.3.1.455.g5932b31

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 5/7] checkout: respect GIT_REFLOG_ACTION
  2013-06-18 18:55 [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog Ramkumar Ramachandra
                   ` (3 preceding siblings ...)
  2013-06-18 18:55 ` [PATCH v2 4/7] status: do not depend on rebase reflog messages Ramkumar Ramachandra
@ 2013-06-18 18:55 ` Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 6/7] rebase: write better reflog messages Ramkumar Ramachandra
  2013-06-18 18:55 ` [PATCH v2 7/7] rebase -i: " Ramkumar Ramachandra
  6 siblings, 0 replies; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 18:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

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 a rebase [-i].  It will also work with any
other scripts provided they set an appropriate GIT_REFLOG_ACTION if they
internally use "git checkout".

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c       | 11 ++++++++---
 t/t2012-checkout-last.sh |  8 ++++----
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f5b50e5..1e2af85 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -587,7 +587,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				   struct branch_info *new)
 {
 	struct strbuf msg = STRBUF_INIT;
-	const char *old_desc;
+	const char *old_desc, *reflog_msg;
 	if (opts->new_branch) {
 		if (opts->new_orphan_branch) {
 			if (opts->new_branch_log && !log_all_ref_updates) {
@@ -620,8 +620,13 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 	old_desc = old->name;
 	if (!old_desc && old->commit)
 		old_desc = sha1_to_hex(old->commit->object.sha1);
-	strbuf_addf(&msg, "checkout: moving from %s to %s",
-		    old_desc ? old_desc : "(invalid)", new->name);
+
+	reflog_msg = getenv("GIT_REFLOG_ACTION");
+	if (!reflog_msg)
+		strbuf_addf(&msg, "checkout: moving from %s to %s",
+			old_desc ? old_desc : "(invalid)", new->name);
+	else
+		strbuf_insert(&msg, 0, reflog_msg, strlen(reflog_msg));
 
 	if (!strcmp(new->name, "HEAD") && !new->path && !opts->force_detach) {
 		/* Nothing to do. */
diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index 6ad6edf..e7ba8c5 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -116,7 +116,7 @@ test_expect_success 'master...' '
 	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify master^)"
 '
 
-test_expect_failure '"checkout -" works after a rebase A' '
+test_expect_success '"checkout -" works after a rebase A' '
 	git checkout master &&
 	git checkout other &&
 	git rebase master &&
@@ -124,7 +124,7 @@ test_expect_failure '"checkout -" works after a rebase A' '
 	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
 '
 
-test_expect_failure '"checkout -" works after a rebase A B' '
+test_expect_success '"checkout -" works after a rebase A B' '
 	git branch moodle master~1 &&
 	git checkout master &&
 	git checkout other &&
@@ -133,7 +133,7 @@ test_expect_failure '"checkout -" works after a rebase A B' '
 	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
 '
 
-test_expect_failure '"checkout -" works after a rebase -i A' '
+test_expect_success '"checkout -" works after a rebase -i A' '
 	git checkout master &&
 	git checkout other &&
 	git rebase -i master &&
@@ -141,7 +141,7 @@ test_expect_failure '"checkout -" works after a rebase -i A' '
 	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
 '
 
-test_expect_failure '"checkout -" works after a rebase -i A B' '
+test_expect_success '"checkout -" works after a rebase -i A B' '
 	git branch foodle master~1 &&
 	git checkout master &&
 	git checkout other &&
-- 
1.8.3.1.455.g5932b31

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 6/7] rebase: write better reflog messages
  2013-06-18 18:55 [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog Ramkumar Ramachandra
                   ` (4 preceding siblings ...)
  2013-06-18 18:55 ` [PATCH v2 5/7] checkout: respect GIT_REFLOG_ACTION Ramkumar Ramachandra
@ 2013-06-18 18:55 ` Ramkumar Ramachandra
  2013-06-18 20:35   ` Junio C Hamano
  2013-06-19  5:39   ` Johannes Sixt
  2013-06-18 18:55 ` [PATCH v2 7/7] rebase -i: " Ramkumar Ramachandra
  6 siblings, 2 replies; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 18:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

Now that the "checkout" invoked internally from "rebase" knows to honor
GIT_REFLOG_ACTION, we can start to use it to write a better reflog
message when "rebase anotherbranch", "rebase --onto branch",
etc. internally checks out the new fork point.  We will write:

  rebase: checkout master

instead of the old

  rebase

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-am.sh         |  4 +++-
 git-rebase--am.sh |  5 +++++
 git-rebase.sh     | 13 +++++++++++--
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index 1cf3d1d..74ef9ca 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -46,6 +46,8 @@ set_reflog_action am
 require_work_tree
 cd_to_toplevel
 
+base_reflog_action="$GIT_REFLOG_ACTION"
+
 git var GIT_COMMITTER_IDENT >/dev/null ||
 	die "$(gettext "You need to set your committer info first")"
 
@@ -884,7 +886,7 @@ did you forget to use 'git add'?"
 		fi &&
 		git commit-tree $tree ${parent:+-p} $parent <"$dotest/final-commit"
 	) &&
-	git update-ref -m "$GIT_REFLOG_ACTION: $FIRSTLINE" HEAD $commit $parent ||
+	git update-ref -m "$base_reflog_action: $FIRSTLINE" HEAD $commit $parent ||
 	stop_here $this
 
 	if test -f "$dotest/original-commit"; then
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 34e3102..69fae7a 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -5,11 +5,13 @@
 
 case "$action" in
 continue)
+	GIT_REFLOG_ACTION="$base_reflog_action"
 	git am --resolved --resolvemsg="$resolvemsg" &&
 	move_to_original_branch
 	return
 	;;
 skip)
+	GIT_REFLOG_ACTION="$base_reflog_action"
 	git am --skip --resolvemsg="$resolvemsg" &&
 	move_to_original_branch
 	return
@@ -40,9 +42,11 @@ else
 		rm -f "$GIT_DIR/rebased-patches"
 		case "$head_name" in
 		refs/heads/*)
+			GIT_REFLOG_ACTION="$base_reflog_action: checkout $head_name"
 			git checkout -q "$head_name"
 			;;
 		*)
+			GIT_REFLOG_ACTION="$base_reflog_action: checkout $orig_head"
 			git checkout -q "$orig_head"
 			;;
 		esac
@@ -59,6 +63,7 @@ else
 		return $?
 	fi
 
+	GIT_REFLOG_ACTION="$base_reflog_action"
 	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
 	ret=$?
 
diff --git a/git-rebase.sh b/git-rebase.sh
index d0c11a9..1c72120 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -47,6 +47,8 @@ set_reflog_action rebase
 require_work_tree_exists
 cd_to_toplevel
 
+base_reflog_action="$GIT_REFLOG_ACTION"
+
 LF='
 '
 ok_to_skip_pre_rebase=
@@ -336,7 +338,8 @@ then
 	# Only interactive rebase uses detailed reflog messages
 	if test "$type" = interactive && test "$GIT_REFLOG_ACTION" = rebase
 	then
-		GIT_REFLOG_ACTION="rebase -i ($action)"
+		GIT_REFLOG_ACTION="rebase -i ($1)"
+		base_reflog_action="$GIT_REFLOG_ACTION"
 		export GIT_REFLOG_ACTION
 	fi
 fi
@@ -543,7 +546,11 @@ then
 	if test -z "$force_rebase"
 	then
 		# Lazily switch to the target branch if needed...
-		test -z "$switch_to" || git checkout "$switch_to" --
+		if test -n "$switch_to"
+		then
+			GIT_REFLOG_ACTION="$base_reflog_action: checkout $switch_to"
+			git checkout "$switch_to" --
+		fi
 		say "$(eval_gettext "Current branch \$branch_name is up to date.")"
 		exit 0
 	else
@@ -568,6 +575,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="$base_reflog_action: checkout $onto_name"
 git checkout -q "$onto^0" || die "could not detach HEAD"
 git update-ref ORIG_HEAD $orig_head
 
-- 
1.8.3.1.455.g5932b31

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 7/7] rebase -i: write better reflog messages
  2013-06-18 18:55 [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog Ramkumar Ramachandra
                   ` (5 preceding siblings ...)
  2013-06-18 18:55 ` [PATCH v2 6/7] rebase: write better reflog messages Ramkumar Ramachandra
@ 2013-06-18 18:55 ` Ramkumar Ramachandra
  2013-06-18 20:36   ` Junio C Hamano
  6 siblings, 1 reply; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 18:55 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano

Now that the "checkout" invoked internally from "rebase -i" knows to
honor GIT_REFLOG_ACTION, we can start to use it to write a better reflog
message when "rebase -i anotherbranch", "rebase -i --onto branch",
etc. internally checks out the new fork point.  We will write:

  rebase -i (start): checkout master

instead of the old

  rebase -i (start)

[jc: add rebase-reflog test]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-rebase--interactive.sh    | 14 ++++++++++----
 t/t3404-rebase-interactive.sh | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..8429c87 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -95,12 +95,15 @@ commit_message () {
 	git cat-file commit "$1" | sed "1,/^$/d"
 }
 
-orig_reflog_action="$GIT_REFLOG_ACTION"
+base_reflog_action="$GIT_REFLOG_ACTION"
 
 comment_for_reflog () {
-	case "$orig_reflog_action" in
+	case "$base_reflog_action" in
+	# if GIT_REFLOG_ACTION was set by caller git-rebase, overwrite
+	# it with rebase -i.
 	''|rebase*)
 		GIT_REFLOG_ACTION="rebase -i ($1)"
+		base_reflog_action="$GIT_REFLOG_ACTION"
 		export GIT_REFLOG_ACTION
 		;;
 	esac
@@ -326,6 +329,7 @@ pick_one_preserving_merges () {
 		if [ "$1" != "-n" ]
 		then
 			# detach HEAD to current parent
+			GIT_REFLOG_ACTION="$base_reflog_action: checkout $first_parent"
 			output git checkout $first_parent 2> /dev/null ||
 				die "Cannot move HEAD to $first_parent"
 		fi
@@ -608,10 +612,10 @@ do_next () {
 	newhead=$(git rev-parse HEAD) &&
 	case $head_name in
 	refs/*)
-		message="$GIT_REFLOG_ACTION: $head_name onto $onto" &&
+		message="$base_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" \
+		  -m "$base_reflog_action: returning to $head_name" \
 		  HEAD $head_name
 		;;
 	esac && {
@@ -838,6 +842,7 @@ comment_for_reflog start
 
 if test ! -z "$switch_to"
 then
+	GIT_REFLOG_ACTION="$base_reflog_action: checkout $switch_to"
 	output git checkout "$switch_to" -- ||
 		die "Could not checkout $switch_to"
 fi
@@ -981,6 +986,7 @@ has_action "$todo" ||
 
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
+GIT_REFLOG_ACTION="$base_reflog_action: checkout $onto_name"
 output git checkout $onto || die_abort "could not detach HEAD"
 git update-ref ORIG_HEAD $orig_head
 do_rest
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 79e8d3c..f7d0147 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' '
+	git reset --hard &&
+	git branch -f branch1 H &&
+	git rebase -i --onto I F branch1 &&
+	cat >expect <<-\EOF &&
+	rebase -i (start): checkout I
+	rebase -i (pick): G
+	rebase -i (pick): H
+	rebase -i (finish): returning to refs/heads/branch1
+	EOF
+	tail -n 4 .git/logs/HEAD |
+	sed -e "s/.*	//" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase -i respects core.commentchar' '
 	git reset --hard &&
 	git checkout E^0 &&
-- 
1.8.3.1.455.g5932b31

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/7] rebase: write better reflog messages
  2013-06-18 18:55 ` [PATCH v2 6/7] rebase: write better reflog messages Ramkumar Ramachandra
@ 2013-06-18 20:35   ` Junio C Hamano
  2013-06-19  5:39   ` Johannes Sixt
  1 sibling, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2013-06-18 20:35 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Now that the "checkout" invoked internally from "rebase" knows to honor
> GIT_REFLOG_ACTION, we can start to use it to write a better reflog
> message when "rebase anotherbranch", "rebase --onto branch",
> etc. internally checks out the new fork point.  We will write:
>
>   rebase: checkout master
>
> instead of the old
>
>   rebase
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

So the approach taken by this round is to change everybody's
assumption so far that they can start from a clean and usable
GIT_REFLOG_ACTION when creating their own message, and stop
depending on what is left in GIT_REFLOG_ACTION.

That would certainly allow this patch to leave whatever cruft in
GIT_REFLOG_ACTION, because everybody else will now create the value
to be assigned to that variable from scratch based on a new and
different variable.  All existing "everybody else" is converted to
adjust to the new reality.

A one-shot assignment "VAR=VAL git checkout" is sometimes cumbersome
to arrange, especially when what calls the "git checkout" is wrapped
in a shell function like "output", I think this is an OK approach.

If we are adopting that convention, however, the new variable that
holds the name of the overall program, base_reflog_action, needs to
be a bit more prominently documented in the code, to let the other
people know that is the new convention to follow.

Something like...

	# Use this as the prefix when setting and exporting
        # GIT_REFLOG_ACTION variable.
	base_reflog_action=am

And the fact that we are declaring the new convention and expecting
everybody to stick to it should be in the log message.

Thanks.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 7/7] rebase -i: write better reflog messages
  2013-06-18 18:55 ` [PATCH v2 7/7] rebase -i: " Ramkumar Ramachandra
@ 2013-06-18 20:36   ` Junio C Hamano
  2013-06-18 20:38     ` Ramkumar Ramachandra
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2013-06-18 20:36 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 79e8d3c..f7d0147 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' '
> +	git reset --hard &&
> +	git branch -f branch1 H &&

Why did this have to change back from branch-reflog-test to branch1
(which I used by mistake in "how about this" patch, but fixed in the
version queued on 'pu')?

The reason I did not use 'branch1' in the version I queued on 'pu'
is because rr/rebase-sha1-by-string-query, when merged on top of
this, makes an assumption on where branch1 is in the test and
fliping its tip here will break it.

Have you tested this with combination of that topic before sending
it out?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 7/7] rebase -i: write better reflog messages
  2013-06-18 20:36   ` Junio C Hamano
@ 2013-06-18 20:38     ` Ramkumar Ramachandra
  2013-06-18 20:55       ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-18 20:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

Junio C Hamano wrote:
> Why did this have to change back from branch-reflog-test to branch1
> (which I used by mistake in "how about this" patch, but fixed in the
> version queued on 'pu')?
>
> The reason I did not use 'branch1' in the version I queued on 'pu'
> is because rr/rebase-sha1-by-string-query, when merged on top of
> this, makes an assumption on where branch1 is in the test and
> fliping its tip here will break it.

Oh, okay.  Then just tweak before applying, if you don't mind?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 7/7] rebase -i: write better reflog messages
  2013-06-18 20:38     ` Ramkumar Ramachandra
@ 2013-06-18 20:55       ` Junio C Hamano
  2013-06-19  4:07         ` Ramkumar Ramachandra
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2013-06-18 20:55 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Why did this have to change back from branch-reflog-test to branch1
>> (which I used by mistake in "how about this" patch, but fixed in the
>> version queued on 'pu')?
>>
>> The reason I did not use 'branch1' in the version I queued on 'pu'
>> is because rr/rebase-sha1-by-string-query, when merged on top of
>> this, makes an assumption on where branch1 is in the test and
>> fliping its tip here will break it.
>
> Oh, okay.  Then just tweak before applying, if you don't mind?

Sorry, you've already used my time allotment for this week.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 7/7] rebase -i: write better reflog messages
  2013-06-18 20:55       ` Junio C Hamano
@ 2013-06-19  4:07         ` Ramkumar Ramachandra
  0 siblings, 0 replies; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-19  4:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List

Junio C Hamano wrote:
>> Oh, okay.  Then just tweak before applying, if you don't mind?
>
> Sorry, you've already used my time allotment for this week.

No worries, I'll re-roll.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/7] rebase: write better reflog messages
  2013-06-18 18:55 ` [PATCH v2 6/7] rebase: write better reflog messages Ramkumar Ramachandra
  2013-06-18 20:35   ` Junio C Hamano
@ 2013-06-19  5:39   ` Johannes Sixt
  2013-06-19  6:09     ` Ramkumar Ramachandra
  2013-06-19 16:58     ` Junio C Hamano
  1 sibling, 2 replies; 19+ messages in thread
From: Johannes Sixt @ 2013-06-19  5:39 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

Am 6/18/2013 20:55, schrieb Ramkumar Ramachandra:
> Now that the "checkout" invoked internally from "rebase" knows to honor
> GIT_REFLOG_ACTION, we can start to use it to write a better reflog
> message when "rebase anotherbranch", "rebase --onto branch",
> etc. internally checks out the new fork point.  We will write:
> 
>   rebase: checkout master
> 
> instead of the old
> 
>   rebase

> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index 34e3102..69fae7a 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -5,11 +5,13 @@
>  
>  case "$action" in
>  continue)
> +	GIT_REFLOG_ACTION="$base_reflog_action"

I haven't followed the topic closely, but I wonder why there are so many
explicit assignments to GIT_REFLOG_ACTION. Is calling set_reflog_action
(defined in git-sh-setup) the wrong thing to do?

-- Hannes

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/7] rebase: write better reflog messages
  2013-06-19  5:39   ` Johannes Sixt
@ 2013-06-19  6:09     ` Ramkumar Ramachandra
  2013-06-19  6:24       ` Johannes Sixt
  2013-06-19 16:58     ` Junio C Hamano
  1 sibling, 1 reply; 19+ messages in thread
From: Ramkumar Ramachandra @ 2013-06-19  6:09 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Git List, Junio C Hamano

Johannes Sixt wrote:
> I haven't followed the topic closely, but I wonder why there are so many
> explicit assignments to GIT_REFLOG_ACTION. Is calling set_reflog_action
> (defined in git-sh-setup) the wrong thing to do?

Does this answer your question?

set_reflog_action() {
	if [ -z "${GIT_REFLOG_ACTION:+set}" ]
	then
		GIT_REFLOG_ACTION="$*"
		export GIT_REFLOG_ACTION
	fi
}

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/7] rebase: write better reflog messages
  2013-06-19  6:09     ` Ramkumar Ramachandra
@ 2013-06-19  6:24       ` Johannes Sixt
  0 siblings, 0 replies; 19+ messages in thread
From: Johannes Sixt @ 2013-06-19  6:24 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano

Am 6/19/2013 8:09, schrieb Ramkumar Ramachandra:
> Johannes Sixt wrote:
>> I haven't followed the topic closely, but I wonder why there are so many
>> explicit assignments to GIT_REFLOG_ACTION. Is calling set_reflog_action
>> (defined in git-sh-setup) the wrong thing to do?
> 
> Does this answer your question?
> 
> set_reflog_action() {
> 	if [ -z "${GIT_REFLOG_ACTION:+set}" ]
> 	then
> 		GIT_REFLOG_ACTION="$*"
> 		export GIT_REFLOG_ACTION
> 	fi
> }

Please don't state the obvious, that does not help. Of course, this does
not answer my question. I was rather hinting that it may be wrong to set
GIT_REFLOG_ACTION explicitly.

I thought that the convention is not to modify GIT_REFLOG_ACTION if it is
already set. set_reflog_action does just that.

-- Hannes

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/7] rebase: write better reflog messages
  2013-06-19  5:39   ` Johannes Sixt
  2013-06-19  6:09     ` Ramkumar Ramachandra
@ 2013-06-19 16:58     ` Junio C Hamano
  2013-06-19 17:53       ` Junio C Hamano
  1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2013-06-19 16:58 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Ramkumar Ramachandra, Git List

Johannes Sixt <j.sixt@viscovery.net> writes:

> Am 6/18/2013 20:55, schrieb Ramkumar Ramachandra:
>> Now that the "checkout" invoked internally from "rebase" knows to honor
>> GIT_REFLOG_ACTION, we can start to use it to write a better reflog
>> message when "rebase anotherbranch", "rebase --onto branch",
>> etc. internally checks out the new fork point.  We will write:
>> 
>>   rebase: checkout master
>> 
>> instead of the old
>> 
>>   rebase
>
>> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
>> index 34e3102..69fae7a 100644
>> --- a/git-rebase--am.sh
>> +++ b/git-rebase--am.sh
>> @@ -5,11 +5,13 @@
>>  
>>  case "$action" in
>>  continue)
>> +	GIT_REFLOG_ACTION="$base_reflog_action"
>
> I haven't followed the topic closely, but I wonder why there are so many
> explicit assignments to GIT_REFLOG_ACTION. Is calling set_reflog_action
> (defined in git-sh-setup) the wrong thing to do?

Excellent question, and I think this illustrates why the recent
reroll that uses an approach to use base_reflog_action is not
complete and needs further work (to put it mildly).

set_reflog_action is designed to be used once at the very top of a
program, like this in "git am", for example:

	set_reflog_action am

The helper function sets the given string to GIT_REFLOG_ACTION only
when GIT_REFLOG_ACTION is not yet set.  Thanks to this, "git am",
when run as the top-level program, will use "am" in GIT_REFLOG_ACTION
and the reflog entries made by whatever it does will say "am did this".

Because of the conditional assignment, when "git am" is run as a
subprogram (i.e. an implementation detail) of "git rebase", the call
to the helper function at the beginning will *not* have any effect.
So "git rebase" can do this:

	set_reflog_action rebase
	... do its own preparation, like checking out "onto" commit
        ... decide to do "format-patch" to "am" pipeline
        	git format-patch --stdout >mbox
		git am mbox

and the reflog entries made inside "git am" invocation will say
"rebase", not "am".

The approach to introduce base_reflog_action may be valid, but if we
were to go that route, set_reflog_action needs to learn the new
convention.  Perhaps by doing something like this:

 1. set_reflog_action to set GIT_REFLOG_NAME and GIT_REFLOG_ACTION;
    Program names like "am", "rebase" will be set to this value.

 2. If the program does not want to say anything more than its
    program name in the reflog, it does not have to do anything.
    GIT_REFLOG_ACTION that is set upfront (or inherited from the
    calling program) will be written in the reflog.

 3. If the program wants to say more than just its name, it needs to
    arrange GIT_REFLOG_ACTION not to be clobbered.  It can do so in
    various ways:

   a) A one-shot invocation of any program that writes to reflog can
      do:

	GIT_REFLOG_ACTION="$GIT_REFLOG_NAME: custom message" \
        	git cmd

      An important thing is that GIT_REFLOG_ACTION is left as the
      original, i.e. "am" or "rebase", so that calls to programs that
      write reflog using the default (i.e. program name only) will
      not be affected.

   b) Or it can temporarily change it and revert it back (necessary
      to use shell function like "output" that cannot use the above
      "single shot export" technique):

	GIT_REFLOG_ACTION="$GIT_REFLOG_NAME: custom message"
        output git cmd
 	GIT_REFLOG_ACTION=$GIT_REFLOG_NAME

But after writing it down this way, I realize that introduction of
base_reflog_action (or GIT_REFLOG_NAME which is a moral equivalent)
is not helping us at all.  As long as calls to "git" command in the
second category exists in these scripts, GIT_REFLOG_ACTION *must* be
kept pristine after set_reflog_action sets it, so we can get rid of
this new variable, and rewrite 3.a and 3.b like so:

    3-a)

	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: custom message" \
        	git cmd

    3-b)

	SAVED=$GIT_REFLOG_ACTION
	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: custom message"
        output git cmd
 	GIT_REFLOG_ACTION=$SAVED

	    or

	(
                GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: custom message"
                output git cmd
	)

That essentially boils down to the very original suggestion I made
before Ram introduced the base_reflog_action.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/7] rebase: write better reflog messages
  2013-06-19 16:58     ` Junio C Hamano
@ 2013-06-19 17:53       ` Junio C Hamano
  2013-06-19 18:29         ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2013-06-19 17:53 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Ramkumar Ramachandra, Git List

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

> Excellent question, and I think this illustrates why the recent
> reroll that uses an approach to use base_reflog_action is not
> complete and needs further work (to put it mildly).
> ...
> That essentially boils down to the very original suggestion I made
> before Ram introduced the base_reflog_action.

So how about doing something like this?

Incidentally, I noticed that GIT_LITERAL_PATHSPECS:: heading in the
enumeration of environment variables is marked-up differently from
others, which is a low-hanging fruit somebody may want to fix.

 Documentation/git-sh-setup.txt |  8 +++++---
 Documentation/git.txt          | 10 ++++++++++
 git-sh-setup.sh                | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt
index 5d709d0..4f67c4c 100644
--- a/Documentation/git-sh-setup.txt
+++ b/Documentation/git-sh-setup.txt
@@ -41,9 +41,11 @@ usage::
 	die with the usage message.
 
 set_reflog_action::
-	set the message that will be recorded to describe the
-	end-user action in the reflog, when the script updates a
-	ref.
+	Set GIT_REFLOG_ACTION environment to a given string (typically
+	the name of the program) unless it is already set.  Whenever
+	the script runs a `git` command that updates refs, a reflog
+	entry is created using the value of this string to leave the
+	record of what command updated the ref.
 
 git_editor::
 	runs an editor of user's choice (GIT_EDITOR, core.editor, VISUAL or
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 2e23cbb..e2bdcc9 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -846,6 +846,16 @@ GIT_LITERAL_PATHSPECS::
 	literal paths to Git (e.g., paths previously given to you by
 	`git ls-tree`, `--raw` diff output, etc).
 
+'GIT_REFLOG_ACTION'::
+	When a ref is updated, reflog entries are created to keep
+	track of the reason why the ref was updated (which is
+	typically the name of the high-level command that updated
+	the ref), in addition to the old and new values of the ref.
+	A scripted Porcelain command can use set_reflog_action
+	helper function in `git-sh-setup` to set its name to this
+	variable when it is invoked as the top level command by the
+	end user, to be recorded in the body of the reflog.
+
 
 Discussion[[Discussion]]
 ------------------------
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 2f78359..e5379bc 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -103,6 +103,40 @@ $LONG_USAGE"
 	esac
 fi
 
+# Set the name of the end-user facing command in the reflog when the
+# script may update refs.  When GIT_REFLOG_ACTION is already set, this
+# will not overwrite it, so that a scripted Porcelain (e.g. "git
+# rebase") can set it to its own name (e.g. "rebase") and then call
+# another scripted Porcelain (e.g. "git am") and a call to this
+# function in the latter will keep the name of the end-user facing
+# program (e.g. "rebase") in GIT_REFLOG_ACTION, ensuring whatever it
+# does will be record as actions done as part of the end-user facing
+# operation (e.g. "rebase").
+#
+# NOTE NOTE NOTE: consequently, after assigning a specific message to
+# GIT_REFLOG_ACTION when calling a "git" command to record a custom
+# reflog message, do not leave that custom value in GIT_REFLOG_ACTION,
+# after you are done.  Other callers of "git" commands that rely on
+# writing the default "program name" in reflog expect the variable to
+# contain the value set by this function.
+#
+# To use a custom reflog message, do either one of these three:
+#
+# (a) use a single-shot export form:
+#     GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: preparing frotz" \
+#         git command-that-updates-a-ref
+#
+# (b) save the original away and restore:
+#     SAVED_ACTION=$GIT_REFLOG_ACTION
+#     GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: preparing frotz"
+#     git command-that-updates-a-ref
+#     GIT_REFLOG_ACITON=$SAVED_ACTION
+#
+# (c) assign the variable in a subshell:
+#     (
+#         GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: preparing frotz"
+#         git command-that-updates-a-ref
+#     )
 set_reflog_action() {
 	if [ -z "${GIT_REFLOG_ACTION:+set}" ]
 	then

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 6/7] rebase: write better reflog messages
  2013-06-19 17:53       ` Junio C Hamano
@ 2013-06-19 18:29         ` Junio C Hamano
  0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2013-06-19 18:29 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Ramkumar Ramachandra, Git List

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Excellent question, and I think this illustrates why the recent
>> reroll that uses an approach to use base_reflog_action is not
>> complete and needs further work (to put it mildly).
>> ...
>> That essentially boils down to the very original suggestion I made
>> before Ram introduced the base_reflog_action.
>
> So how about doing something like this?

Having said all that, although I think the "something like this"
patch is an improvement in that it spells out the rules regarding
the use of GIT_REFLOG_ACTION environment variable, which was not
documented so far, I think the environment variable is showing its
flaws.

It was a good mechanism in simpler times, back when "git commit",
"git fetch" and "git merge" were its primary users.  They didn't do
many ref updates, and having a way to record that "this update was
done by a 'merge' command initiated by the end user" was perfectly
adequate.

For a command like "rebase" that can do many ref updates, having to
set a custom message and export the variable in each and every step
is cumbersome, and keeping the same prefix across becomes even more
so.

The $orig_reflog_action used inside git-rebase--interactive is a
reasonable local solution for the "keeping the same prefix" problem,
but it is a _local_ solution that does not scale.  In the end, it
updates the GIT_REFLOG_ACTION variable, so the script has to be very
careful to make sure the variable has a sensible value before
calling any ref-updating "git" command.  It will have to set it back
to $orig_reflog_action if it ever wants to call another scripted
Porcelain.

Among the C infrastructure, commit, fetch, merge and reset are the
only ones that pay attention to GIT_REFLOG_ACTION, and we will be
adding checkout to the mix.  If we originally did not make the
mistake of using GIT_REFLOG_ACTION as a whole message, and instead
used it to convey _only_ the prefix (i.e. "rebase", "am", etc.) to
subprocesses in order to remember what the end-user initiated
command was, and used a command line argument to give the actual
messages, we would have been in much better shape.  E.g. a
"checkout" call inside "git rebase" may become

	git checkout \
            --reflog-message="$GIT_REFLOG_ACTION: detaching" \
	    $onto^0

and nobody other than set_reflog_action shell function would be
setting GIT_REFLOG_ACTION variable.

Oh well.

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2013-06-19 18:29 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-18 18:55 [PATCH v2 0/7] Re-roll rr/rebase-checkout-reflog Ramkumar Ramachandra
2013-06-18 18:55 ` [PATCH v2 1/7] t/t7512-status-help: test "HEAD detached from" Ramkumar Ramachandra
2013-06-18 18:55 ` [PATCH v2 2/7] wt-status: remove unused field in grab_1st_switch_cbdata Ramkumar Ramachandra
2013-06-18 18:55 ` [PATCH v2 3/7] t/t2012-checkout-last: test "checkout -" after a rebase Ramkumar Ramachandra
2013-06-18 18:55 ` [PATCH v2 4/7] status: do not depend on rebase reflog messages Ramkumar Ramachandra
2013-06-18 18:55 ` [PATCH v2 5/7] checkout: respect GIT_REFLOG_ACTION Ramkumar Ramachandra
2013-06-18 18:55 ` [PATCH v2 6/7] rebase: write better reflog messages Ramkumar Ramachandra
2013-06-18 20:35   ` Junio C Hamano
2013-06-19  5:39   ` Johannes Sixt
2013-06-19  6:09     ` Ramkumar Ramachandra
2013-06-19  6:24       ` Johannes Sixt
2013-06-19 16:58     ` Junio C Hamano
2013-06-19 17:53       ` Junio C Hamano
2013-06-19 18:29         ` Junio C Hamano
2013-06-18 18:55 ` [PATCH v2 7/7] rebase -i: " Ramkumar Ramachandra
2013-06-18 20:36   ` Junio C Hamano
2013-06-18 20:38     ` Ramkumar Ramachandra
2013-06-18 20:55       ` Junio C Hamano
2013-06-19  4:07         ` Ramkumar Ramachandra

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).