[PATCH v3 0/7] Re-roll rr/rebase-checkout-reflog

[PATCH v3 0/7] Re-roll rr/rebase-checkout-reflog

[Ramkumar Ramachandra]

So, this is hopefully the final re-roll.  [6/7] and [7/7] have updated
commit messages and comments describing what this new
base_reflog_action is.  Also, to prevent breakages with another
in-flight topic, the test that Junio contributed to [7/7] uses a
different branch name.

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             |  7 +++++++
 git-rebase--interactive.sh    | 14 +++++++++----
 git-rebase.sh                 | 18 +++++++++++++++--
 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, 123 insertions(+), 34 deletions(-)

-- 
1.8.3.1.449.g41b32a4.dirty

[PATCH v3 1/7] t/t7512-status-help: test "HEAD detached from"

[Ramkumar Ramachandra]

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.449.g41b32a4.dirty

[PATCH v3 2/7] wt-status: remove unused field in grab_1st_switch_cbdata

[Ramkumar Ramachandra]

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.449.g41b32a4.dirty

[PATCH v3 3/7] t/t2012-checkout-last: test "checkout -" after a rebase

[Ramkumar Ramachandra]

  $ 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.449.g41b32a4.dirty

Re: [PATCH v3 3/7] t/t2012-checkout-last: test "checkout -" after a rebase

[Johannes Sixt]

Am 6/19/2013 10:04, schrieb Ramkumar Ramachandra:
> +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 rebase -i master foodle &&

> +	git checkout - &&
> +	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
> +'
> +
>  test_done

-- Hannes

Re: [PATCH v3 3/7] t/t2012-checkout-last: test "checkout -" after a rebase

[Ramkumar Ramachandra]

Johannes Sixt wrote:
> Am 6/19/2013 10:04, schrieb Ramkumar Ramachandra:
>> +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 rebase -i master foodle &&

Good eyes.  Thanks.

[PATCH v3 4/7] status: do not depend on rebase reflog messages

[Ramkumar Ramachandra]

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.449.g41b32a4.dirty

[PATCH v3 5/7] checkout: respect GIT_REFLOG_ACTION

[Ramkumar Ramachandra]

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.449.g41b32a4.dirty

[PATCH v3 6/7] rebase: write better reflog messages

[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

The usage semantics of GIT_REFLOG_ACTION have changed; by appending to
GIT_REFLOG_ACTION directly, we risk appending to a prebuilt string like
"rebase: checkout mool" from a previous append, when we really intended
to append to the string "rebase".  Solve this problem by introducing a
base_reflog_action variable that is different from GIT_REFLOG_ACTION.

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 |  7 +++++++
 git-rebase.sh     | 18 ++++++++++++++++--
 3 files changed, 26 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..7fbeb35 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,9 @@ else
 		return $?
 	fi
 
+	# always reset GIT_REFLOG_ACTION before calling any external
+	# scripts; they have no idea about our base_reflog_action
+	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..6cdf9f8 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -47,6 +47,13 @@ set_reflog_action rebase
 require_work_tree_exists
 cd_to_toplevel
 
+# Should contain a base string like "rebase" (or "rebase -i (start)")
+# to which a more information like ": checkout foom" can be appended
+# to set the final GIT_REFLOG_ACTION.  If you append to
+# GIT_REFLOG_ACTION directly, you risk appending to a prebuilt string
+# like "rebase: checkout mool" from a previous append.
+base_reflog_action="$GIT_REFLOG_ACTION"
+
 LF='
 '
 ok_to_skip_pre_rebase=
@@ -336,7 +343,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 +551,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 +580,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.449.g41b32a4.dirty

Re: [PATCH v3 6/7] rebase: write better reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> @@ -59,6 +63,9 @@ else
>  		return $?
>  	fi
>  
> +	# always reset GIT_REFLOG_ACTION before calling any external
> +	# scripts; they have no idea about our base_reflog_action
> +	GIT_REFLOG_ACTION="$base_reflog_action"
>  	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg"

Why does this reroll still use this base_reflog_action convention?

The original orig_reflog_action you borrowed this may have been an
acceptable local solution inside 
git-rebase--interactive that does not call out to amyting, but
the above comment a good demonstration that shows why this cannot be 
a good general solution that scales across scriptlets.

And I already explained that to you at least twice.

cf. http://article.gmane.org/gmane.comp.version-control.git/228399

    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.

Re: [PATCH v3 6/7] rebase: write better reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>> @@ -59,6 +63,9 @@ else
>>               return $?
>>       fi
>>
>> +     # always reset GIT_REFLOG_ACTION before calling any external
>> +     # scripts; they have no idea about our base_reflog_action
>> +     GIT_REFLOG_ACTION="$base_reflog_action"
>>       git am $git_am_opt --rebasing --resolvemsg="$resolvemsg"
>
> Why does this reroll still use this base_reflog_action convention?

Because it's simple and it makes sense.

> The original orig_reflog_action you borrowed this may have been an
> acceptable local solution inside
> git-rebase--interactive that does not call out to amyting, but
> the above comment a good demonstration that shows why this cannot be
> a good general solution that scales across scriptlets.

Nonsense.  How do I set a custom reflog action when a certain flag is
passed to my script (like git-rebase.sh:333) without *overriding* an
existing GIT_REFLOG_ACTION?  How do I construct cute start/pick/reword
prefixes elegantly (like in
git-rebase--interactive.sh:comment_for_reflog()) without *overriding*
an existing GIT_REFLOG_ACTION?  In both these examples, I'm setting a
GIT_REFLOG_ACTION for the "rest of the code" to use.  I don't care
about the exact command sequence, but I know that they respect
GIT_REFLOG_ACTION; so I'm setting one in advance.

When calling out to an external scriptlet, I want to define my own
reflog message: when I call out to "am" from "rebase -i", it should
write a "rebase -i: " message, and ignoring its own
set_reflog_message().  _That_ can be done using the subshell thing you
proposed.  And I have absolutely no clue why

  (
    export GIT_REFLOG_ACTION
    git am
  )

is "more scalable" than

  GIT_REFLOG_ACTION="$base_reflog_action"
  git am

> And I already explained that to you at least twice.

You just gave set_reflog_action() and GIT_REFLOG_ACTION some sort of
God status, and proposed to make the scripts more ugly and less
extensible.

... and we're discussing absolutely trivial inconsequential rubbish
once again.  In any case, I've given up on arguing with you as it is
clear that I can't possibly win.  Do whatever you want.

Re: [PATCH v3 6/7] rebase: write better reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>>> +     # always reset GIT_REFLOG_ACTION before calling any external
>>> +     # scripts; they have no idea about our base_reflog_action
>>> +     GIT_REFLOG_ACTION="$base_reflog_action"
>>>       git am $git_am_opt --rebasing --resolvemsg="$resolvemsg"
>>
>> Why does this reroll still use this base_reflog_action convention?
>
> Because it's simple and it makes sense.

Without $base_reflog_action hack, you have to make sure
GIT_REFLOG_ACTION is reasonably pristine when you call out other
people.  Even with $base_reflog_action, you still have to do the
same "keep GIT_REFLOG_ACTION pristine" like this one.  And in
addition, you have to maintain $base_reflog_action as if it is a
read-only variable [*1*].

So you are forcing people to maintain _two_ variables, instead of
just _one_, without making anything simpler.

What's so hard to understand why it is a wrong design?

> ... and we're discussing absolutely trivial inconsequential rubbish
> once again.  In any case, I've given up on arguing with you as it is
> clear that I can't possibly win.  Do whatever you want.

It is not about winning or losing.

If you truly think this is "inconsequential", that unfortunately
convinces me that you cannot yet be trusted enough to give you
latitude to design interfaces that span multiple programs X-<.


[Footnote]

*1* The original orig_reflog_action you borrowed from was bad enough
but it had an excuse that it was confined within the leaf level of
the callchain.  It was merely done as a way to stash the vanilla
action name (e.g. "rebase -i" before it is specialized into "rebase
-i pick" etc) away, so that it can easily "lose" the speciailzation
from GIT_REFLOG_ACTION while preparing for the next operation.

Re: [PATCH v3 6/7] rebase: write better reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> So you are forcing people to maintain _two_ variables, instead of
> just _one_, without making anything simpler.
>
> What's so hard to understand why it is a wrong design?

Fine.  Let's say I buy your argument about one-variable versus
two-variables: how do you solve the existing problems that are solved
by overriding GIT_REFLOG_ACTION that I pointed out?

> If you truly think this is "inconsequential", that unfortunately
> convinces me that you cannot yet be trusted enough to give you
> latitude to design interfaces that span multiple programs X-<.

*shrug* I certainly don't think one-variable versus two-variables
warrants this much discussion.

I don't have anything to win or lose: I designed a solution to the
problem which I think is reasonable; you don't.  Fine.  Show me an
alternative that doesn't involve rewriting half of the rebase
infrastructure in a series that fixes checkout-dash.

[PATCH v3 7/7] rebase -i: write better reflog messages

[Ramkumar Ramachandra]

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)

The usage semantics of GIT_REFLOG_ACTION have changed: use
base_reflog_action just like in git-rebase.sh.

[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..f943efa 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 branch-reflog-test H &&
+	git rebase -i --onto I F branch-reflog-test &&
+	cat >expect <<-\EOF &&
+	rebase -i (start): checkout I
+	rebase -i (pick): G
+	rebase -i (pick): H
+	rebase -i (finish): returning to refs/heads/branch-reflog-test
+	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.449.g41b32a4.dirty