Re: Merge made by recursive?

Junio C Hamano [off-list ref] writes:

Jeff King [off-list ref] writes:

quoted

Also, most of the rest of the merge output has been gettext-ized, but
not this message. If we are going to declare it not parseable, should we
also be internationalizing it?

Heh, great minds think alike. I was wondering about that while I took a
brief walk after sending it out.

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".

Either we split the messages into two, one translatable and given to the
end user and the other untranslatable and sent to the reflog, or find a
place in reflog entry where we can hide machine readable and stable
representation of what happened and store both the end-user facing message
and the machine readable one separately. In the longer term I would prefer
the latter.

-- >8 --
Subject: [PATCH] merge: mark the final "Merge made by..." message for l10n

The final "Merge made by ... strategy" message is an end-user facing
message and can be localized.  And mark the test that depends on the exact
wording with test_i18ncmp.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/merge.c               |    2 +-
 t/t7602-merge-octopus-many.sh |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 8bc453d..4083b9b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -902,7 +902,7 @@ static int finish_automerge(struct commit_list *common,
 	strbuf_addch(&merge_msg, '\n');
 	run_prepare_commit_msg();
 	commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
-	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
+	strbuf_addf(&buf, _("Merge made by the '%s' strategy."), wt_strategy);
 	finish(result_commit, buf.buf);
 	strbuf_release(&buf);
 	drop_save();

diff --git a/t/t7602-merge-octopus-many.sh b/t/t7602-merge-octopus-many.sh
index 61f36ba..3f893de 100755
--- a/t/t7602-merge-octopus-many.sh
+++ b/t/t7602-merge-octopus-many.sh

@@ -66,7 +66,7 @@ EOF
 test_expect_success 'merge output uses pretty names' '
 	git reset --hard c1 &&
 	git merge c2 c3 c4 >actual &&
-	test_cmp actual expected
+	test_i18ncmp actual expected
 '
 
 cat >expected <<\EOF

@@ -80,7 +80,7 @@ EOF
 
 test_expect_success 'merge up-to-date output uses pretty names' '
 	git merge c4 c5 >actual &&
-	test_cmp actual expected
+	test_i18ncmp actual expected
 '
 
 cat >expected <<\EOF

@@ -97,7 +97,7 @@ EOF
 test_expect_success 'merge fast-forward output uses pretty names' '
 	git reset --hard c0 &&
 	git merge c1 c2 >actual &&
-	test_cmp actual expected
+	test_i18ncmp actual expected
 '
 
 test_done

-- 
1.7.5.2.483.gc61ca

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

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.

So it seems to me that we could localize everything after the colon and
still do "3 merges ago". The only thing you would be losing is a
machine-readable description of which strategy was used. In practice,
I'm not sure how much it matters.

But for others, like:

  checkout: moving from $old to $new

a localized version loses useful information.

Either we split the messages into two, one translatable and given to the
end user and the other untranslatable and sent to the reflog,

That seems like the safe choice for now, as it still opens the
possibility of localizing reflogs later if people care to do so.  Being
a native English speaker, I have no clue how much people actually care
about localized reflog entries.

place in reflog entry where we can hide machine readable and stable
representation of what happened and store both the end-user facing message
and the machine readable one separately. In the longer term I would prefer
the latter.

If we assume that reflog entries are machine-readable (or at least
_some_ of them that are generated by a few well-known git programs), you
could always translate on the fly while displaying them.

That solves the storage question, and it allows two people with
different language preferences to both read the same reflog. And the
implementation probably wouldn't be too hard, since we'd only have to
match a few well-known strings, and we could display anything we didn't
match as it appears in the reflog.

-Peff

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

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

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!

Actually, this gets written also to the branch's reflog, so saying
"updating HEAD" has a little bit of value. Of course, saying "reset" is
even better, since that is the command to update HEAD.

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.

...and for the reason above, it makes sense to leave this as "updating
HEAD" for the per-branch reflog, since whatever the script put into
GIT_REFLOG_ACTION may be more clear with that information.

So here's what I think we should do.

-- >8 --
Subject: [PATCH] reset: give better 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

However, we must also consider that some scripts may set
GIT_REFLOG_ACTION before calling reset, and we need to show
their reflog action (with our text appended). For example:

  rebase -i (squash): updating HEAD

On top of that, we also set the ORIG_HEAD reflog action
(even though it doesn't generally exist). In that case, the
reset argument is somewhat meaningless, as it has nothing to
do with what's in ORIG_HEAD.

This patch changes the reset reflog code to show:

  $GIT_REFLOG_ACTION: updating {HEAD,ORIG_HEAD}

as before, but only if GIT_REFLOG_ACTION is set. Otherwise,
show:

   reset: moving to $rev

for HEAD, and:

   reset: updating ORIG_HEAD

for ORIG_HEAD (this is still somewhat superfluous, since we
are in the ORIG_HEAD reflog, obviously, but at least we now
mention which command was used to update it).

While we're at it, we can clean up the code a bit:

  1. Use strbufs to make the message.

  1. Use the "rev" parameter instead of showing all options.
     This makes more sense, since it is the only thing
     impacting the writing of the ref.

Signed-off-by: Jeff King <redacted>
---
 builtin/reset.c        |   49 +++++++++++++++--------------------------------
 t/t1412-reflog-loop.sh |    8 +++---
 2 files changed, 20 insertions(+), 37 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 98bca04..27b3426 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -33,25 +33,6 @@ static const char *reset_type_names[] = {
 	N_("mixed"), N_("soft"), N_("hard"), N_("merge"), N_("keep"), NULL
 };
 
-static char *args_to_str(const char **argv)
-{
-	char *buf = NULL;
-	unsigned long len, space = 0, nr = 0;
-
-	for (; *argv; argv++) {
-		len = strlen(*argv);
-		ALLOC_GROW(buf, nr + 1 + len, space);
-		if (nr)
-			buf[nr++] = ' ';
-		memcpy(buf + nr, *argv, len);
-		nr += len;
-	}
-	ALLOC_GROW(buf, nr + 1, space);
-	buf[nr] = '\0';
-
-	return buf;
-}
-
 static inline int is_merge(void)
 {
 	return !access(git_path("MERGE_HEAD"), F_OK);

@@ -215,14 +196,18 @@ static int read_from_tree(const char *prefix, const char **argv,
 	return update_index_refresh(index_fd, lock, refresh_flags);
 }
 
-static void prepend_reflog_action(const char *action, char *buf, size_t size)
+static void set_reflog_message(struct strbuf *sb, const char *action,
+			       const char *rev)
 {
-	const char *sep = ": ";
 	const char *rla = getenv("GIT_REFLOG_ACTION");
-	if (!rla)
-		rla = sep = "";
-	if (snprintf(buf, size, "%s%s%s", rla, sep, action) >= size)
-		warning(_("Reflog action message too long: %.*s..."), 50, buf);
+
+	strbuf_reset(sb);
+	if (rla)
+		strbuf_addf(sb, "%s: %s", rla, action);
+	else if (rev)
+		strbuf_addf(sb, "reset: moving to %s", rev);
+	else
+		strbuf_addf(sb, "reset: %s", action);
 }
 
 static void die_if_unmerged_cache(int reset_type)

@@ -241,7 +226,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
 				*old_orig = NULL, sha1_old_orig[20];
 	struct commit *commit;
-	char *reflog_action, msg[1024];
+	struct strbuf msg = STRBUF_INIT;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, "be quiet, only report errors"),
 		OPT_SET_INT(0, "mixed", &reset_type,

@@ -261,8 +246,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	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);
 
 	/*
 	 * Possible arguments are:

@@ -357,13 +340,13 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		old_orig = sha1_old_orig;
 	if (!get_sha1("HEAD", sha1_orig)) {
 		orig = sha1_orig;
-		prepend_reflog_action("updating ORIG_HEAD", msg, sizeof(msg));
-		update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
+		set_reflog_message(&msg, "updating ORIG_HEAD", NULL);
+		update_ref(msg.buf, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
 	}
 	else if (old_orig)
 		delete_ref("ORIG_HEAD", old_orig, 0);
-	prepend_reflog_action("updating HEAD", msg, sizeof(msg));
-	update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+	set_reflog_message(&msg, "updating HEAD", rev);
+	update_ref_status = update_ref(msg.buf, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 
 	switch (reset_type) {
 	case HARD:

@@ -380,7 +363,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	remove_branch_state();
 
-	free(reflog_action);
+	strbuf_release(&msg);
 
 	return update_ref_status;
 }

diff --git a/t/t1412-reflog-loop.sh b/t/t1412-reflog-loop.sh
index 7f519e5..647d888 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: moving to two
+		topic@{1} reset: moving to one
+		topic@{2} reset: moving to two
+		topic@{3} reset: moving to one
 		topic@{4} branch: Created from HEAD
 	EOF
 	git log -g --format="%gd %gs" topic >actual &&

-- 
1.7.4.5.34.g0787f