[2/2] refs.c: avoid creating extra unwanted reflog entries

Commit Message

Kyle J. McKay Jan. 30, 2021, 10:19 a.m. UTC

Since commit 523fa69c36744ae6 ("reflog: cleanse messages in the refs.c
layer", 2020-07-10, v2.29.0), ref log messages are now being "cleansed"
to make sure they do not end up breaking the ref log files.  A laudable
endeavor.

Unfortunately, that commit had an unintended side effect that causes
the `git symbolic-ref <refname1> <refname2>` command to suddenly start
adding new entries to the ref log for <refname1> whenever it's run.

These new entries have a completely empty message and do not provide
any useful information.  In fact, there was no mention that the change
to "cleanse" ref log messages was intended to add these new ref log
entries at all.

What happened is that when the change to "cleanse" the incoming ref
log message was made, the code started inadvertently transforming
a NULL ref log message pointer into an empty string "".

This created the observed effect that using the `symbolic-ref` command
suddenly started causing ref log entries to be added.

The original code that predated the "cleanse" commit called the
`xstrdup_or_null` function to retain the original NULL pointer and
avoid introducing unwanted extra ref log entries.

After the "cleanse" commit, ref log messages are now funnelled through
a new static function named `normalize_reflog_message`.

Eliminate the unwanted extra blank ref log entries by returning a NULL
pointer when NULL is passed into `normalize_reflog_message` rather
than returning a pointer to an empty string ("").

To reflect this new behavior, rename the function to
`normalize_reflog_message_or_null` in the same spirit as the name
of the `xstrdup_or_null` function that was called pre-"cleanse".

Flip the `test_expect_failure` tests to `test_expect_success`
as they now pass again.

Signed-off-by: Kyle J. McKay <[e-mail]>
---
 refs.c                   | 16 +++++++++-------
 t/t1417-reflog-symref.sh |  6 +++---
 2 files changed, 12 insertions(+), 10 deletions(-)

--

Patch

diff --git a/refs.c b/refs.c
index 03968ad7..790b1ff0 100644
--- a/refs.c
+++ b/refs.c
@@ -835,11 +835,13 @@  static void copy_reflog_msg(struct strbuf *sb, const char *msg)
 	strbuf_rtrim(sb);
 }
 
-static char *normalize_reflog_message(const char *msg)
+static char *normalize_reflog_message_or_null(const char *msg)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	if (msg && *msg)
+	if (!msg)
+		return NULL;
+	if (*msg)
 		copy_reflog_msg(&sb, msg);
 	return strbuf_detach(&sb, NULL);
 }
@@ -1067,7 +1069,7 @@  struct ref_update *ref_transaction_add_update(
 		oidcpy(&update->new_oid, new_oid);
 	if (flags & REF_HAVE_OLD)
 		oidcpy(&update->old_oid, old_oid);
-	update->msg = normalize_reflog_message(msg);
+	update->msg = normalize_reflog_message_or_null(msg);
 	return update;
 }
 
@@ -1951,7 +1953,7 @@  int refs_create_symref(struct ref_store *refs,
 	char *msg;
 	int retval;
 
-	msg = normalize_reflog_message(logmsg);
+	msg = normalize_reflog_message_or_null(logmsg);
 	retval = refs->be->create_symref(refs, ref_target, refs_heads_master,
 					 msg);
 	free(msg);
@@ -2339,7 +2341,7 @@  int refs_delete_refs(struct ref_store *refs, const char *logmsg,
 	char *msg;
 	int retval;
 
-	msg = normalize_reflog_message(logmsg);
+	msg = normalize_reflog_message_or_null(logmsg);
 	retval = refs->be->delete_refs(refs, msg, refnames, flags);
 	free(msg);
 	return retval;
@@ -2357,7 +2359,7 @@  int refs_rename_ref(struct ref_store *refs, const char *oldref,
 	char *msg;
 	int retval;
 
-	msg = normalize_reflog_message(logmsg);
+	msg = normalize_reflog_message_or_null(logmsg);
 	retval = refs->be->rename_ref(refs, oldref, newref, msg);
 	free(msg);
 	return retval;
@@ -2374,7 +2376,7 @@  int refs_copy_existing_ref(struct ref_store *refs, const char *oldref,
 	char *msg;
 	int retval;
 
-	msg = normalize_reflog_message(logmsg);
+	msg = normalize_reflog_message_or_null(logmsg);
 	retval = refs->be->copy_ref(refs, oldref, newref, msg);
 	free(msg);
 	return retval;
diff --git a/t/t1417-reflog-symref.sh b/t/t1417-reflog-symref.sh
index 6149531f..3687b058 100755
--- a/t/t1417-reflog-symref.sh
+++ b/t/t1417-reflog-symref.sh
@@ -53,7 +53,7 @@  test_expect_success setup '
 	test $hcnt -ne $kcnt
 '
 
-test_expect_failure 'HEAD reflog symbolic-ref' '
+test_expect_success 'HEAD reflog symbolic-ref' '
 	hcnt1=$(git reflog show HEAD | wc -l) &&
 	git symbolic-ref HEAD refs/heads/unu &&
 	git symbolic-ref HEAD refs/heads/du &&
@@ -62,7 +62,7 @@  test_expect_failure 'HEAD reflog symbolic-ref' '
 	test $hcnt1 = $hcnt2
 '
 
-test_expect_failure 'refs/heads/KVAR reflog symbolic-ref' '
+test_expect_success 'refs/heads/KVAR reflog symbolic-ref' '
 	kcnt1=$(git reflog show refs/heads/KVAR | wc -l) &&
 	git symbolic-ref refs/heads/KVAR refs/heads/tri &&
 	git symbolic-ref refs/heads/KVAR refs/heads/du &&
@@ -71,7 +71,7 @@  test_expect_failure 'refs/heads/KVAR reflog symbolic-ref' '
 	test $kcnt1 = $kcnt2
 '
 
-test_expect_failure 'double symref reflog symbolic-ref' '
+test_expect_success 'double symref reflog symbolic-ref' '
 	hcnt1=$(git reflog show HEAD | wc -l) &&
 	kcnt1=$(git reflog show refs/heads/KVAR | wc -l) &&
 	git symbolic-ref HEAD refs/heads/KVAR &&