Re: Merge made by recursive?

On Wed, May 25, 2011 at 05:02:54PM -0400, Jeff King wrote:

On Wed, May 25, 2011 at 01:47:34PM -0700, Junio C Hamano wrote:

quoted

I am reluctant to do this (including the rewording of the end-user facing
message) until we decide what to do with the reflog. Right now, I think no
tool looks at the reflog, but contaminating the reflog with translatable
messages mean that we will never be able to support "3 merges ago" just
like we support "the previous branch".

The reflog messages look like:

  merge $branch: Merge made by recursive.

While peeking in my reflog, I noticed some very confusing entries, which
this patch addresses.

-- >8 --
Subject: [PATCH] reset: give more verbose reflog messages

The reset command creates its reflog entry from argv.
However, it does so after having run parse_options, which
means the only thing left in argv is any non-option
arguments. Thus you would end up with confusing reflog
entries like:

  $ git reset --hard HEAD^
  $ git reset --soft HEAD@{1}
  $ git log -2 -g --oneline
  8e46cad HEAD@{0}: HEAD@{1}: updating HEAD
  1eb9486 HEAD@{1}: HEAD^: updating HEAD

This patch sets up the reflog before argv is munged, so you
get the command name and any other options, like:

  8e46cad HEAD@{0}: reset --soft HEAD@{1}: updating HEAD
  1eb9486 HEAD@{1}: reset --hard HEAD^: updating HEAD

Signed-off-by: Jeff King <redacted>
---
I am not sure if this was the original intent of the code or not; I had
to update a test vector which codified it. Any options like "--hard" or
"--soft" are actually superfluous to the ref update (not to mention
something like "-q"). So another option would be to just take what's
left after parsing options and putting "reset" in front of it, like:

  8e46cad HEAD@{0}: reset: HEAD^: updating HEAD

which is a little more readable. Though if we are going to change it, I
think my preference would actually be:

  8e46cad HEAD@{0}: reset: moving to HEAD^

which reads better. The "updating HEAD" is just pointless. Of course
we're updating HEAD; we're in the HEAD reflog and we're running reset!

However, if GIT_REFLOG_ACTION is already set (by a script calling us),
then we won't say "reset". So for example, I have entries in my reflog
like:

  944af8c HEAD@{311}: rebase -i (squash): updating HEAD

So maybe it makes sense to leave those ones as-is, and adjust only the
case where GIT_REFLOG_ACTION is unset.

 builtin/reset.c        |    5 +++--
 t/t1412-reflog-loop.sh |    8 ++++----
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 98bca04..77103fb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -259,11 +259,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
-						PARSE_OPT_KEEP_DASHDASH);
 	reflog_action = args_to_str(argv);
 	setenv("GIT_REFLOG_ACTION", reflog_action, 0);
 
+	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
+						PARSE_OPT_KEEP_DASHDASH);
+
 	/*
 	 * Possible arguments are:
 	 *

diff --git a/t/t1412-reflog-loop.sh b/t/t1412-reflog-loop.sh
index 7f519e5..a92875f 100755
--- a/t/t1412-reflog-loop.sh
+++ b/t/t1412-reflog-loop.sh

@@ -21,10 +21,10 @@ test_expect_success 'setup reflog with alternating commits' '
 
 test_expect_success 'reflog shows all entries' '
 	cat >expect <<-\EOF
-		topic@{0} two: updating HEAD
-		topic@{1} one: updating HEAD
-		topic@{2} two: updating HEAD
-		topic@{3} one: updating HEAD
+		topic@{0} reset two: updating HEAD
+		topic@{1} reset one: updating HEAD
+		topic@{2} reset two: updating HEAD
+		topic@{3} reset one: updating HEAD
 		topic@{4} branch: Created from HEAD
 	EOF
 	git log -g --format="%gd %gs" topic >actual &&

-- 
1.7.4.5.34.g0787f