[PATCH 0/2] Eliminate extraneous ref log entries

STALE2041d

3 messages, 1 author, 2021-01-30 · open the first message on its own page

[PATCH 0/2] Eliminate extraneous ref log entries

From: Kyle J. McKay <hidden>
Date: 2021-01-30 10:27:09

Since Git version v2.29.0, the `git symbolic-ref` command has started
adding extraneous entries to the ref log of the symbolic ref it's
updating.

This change was inadvertently introduced in commit 523fa69c36744ae6
("reflog: cleanse messages in the refs.c layer", 2020-07-10, v2.29.0).

A bug report [1] was made about a failing test in the TopGit test
suite.  Further investigations into the cause led to this patch set.

1/2 - adds new tests to monitor this behavior
2/2 - corrects the problem

The tests added in 1/2 are marked `test_expect_failure` and then
changed to `test_expect_success` in 2/2.

-Kyle

[1]: <https://github.com/mackyle/topgit/issues/17>

Kyle J. McKay (2):
  t/t1417: test symbolic-ref effects on ref logs
  refs.c: avoid creating extra unwanted reflog entries

 refs.c                   | 16 +++----
 t/t1417-reflog-symref.sh | 91 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 7 deletions(-)
 create mode 100755 t/t1417-reflog-symref.sh

--

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

From: Kyle J. McKay <hidden>
Date: 2021-01-30 10:27:10

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 <redacted>
---
 refs.c                   | 16 +++++++++-------
 t/t1417-reflog-symref.sh |  6 +++---
 2 files changed, 12 insertions(+), 10 deletions(-)

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 &&

--

[PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs

From: Kyle J. McKay <hidden>
Date: 2021-01-30 10:27:10

The git command `git symbolic-ref <refname1> <refname2>` updates
<refname1> to be a "symbolic" pointer to <refname2>.  No matter
what future value <refname2> takes on, <refname1> will continue
to refer to that future value since it's "symbolic".

Since commit 523fa69c36744ae6 ("reflog: cleanse messages in the refs.c
layer", 2020-07-10, v2.29.0), the effect of using the aforementioned
"symbolic-ref" command on ref logs has changed in an unexpected way.

Add a new set of tests to exercise and demonstrate this change
in preparation for correcting it (at which point the failing tests
will be flipped from `test_expect_failure` to `test_expect_success`).

The new test file can be used unchanged to examine this behavior
in much older Git versions (likely to as far back as v2.6.0).

Signed-off-by: Kyle J. McKay <redacted>
---
 t/t1417-reflog-symref.sh | 91 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)
 create mode 100755 t/t1417-reflog-symref.sh

diff --git a/t/t1417-reflog-symref.sh b/t/t1417-reflog-symref.sh
new file mode 100755
index 00000000..6149531f
--- /dev/null
+++ b/t/t1417-reflog-symref.sh

@@ -0,0 +1,91 @@
+#!/bin/sh
+#
+# Copyright (c) 2021 Kyle J. McKay
+#
+
+test_description='Test symbolic-ref effects on reflogs'
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_commit 'initial' &&
+	git checkout -b unu &&
+	test_commit 'one' &&
+	git checkout -b du &&
+	test_commit 'two' &&
+	git checkout -b tri &&
+	test_commit 'three' &&
+	git checkout du &&
+	test_commit 'twofour' &&
+	git checkout -b KVAR du &&
+	test_commit 'four' &&
+	unu="$(git rev-parse --verify unu)" &&
+	du="$(git rev-parse --verify du)" &&
+	tri="$(git rev-parse --verify tri)" &&
+	kvar="$(git rev-parse --verify KVAR)" &&
+	test -n "$unu" &&
+	test -n "$du" &&
+	test -n "$tri" &&
+	test -n "$kvar" &&
+	test "$unu" != "$du" &&
+	test "$unu" != "$tri" &&
+	test "$unu" != "$kvar" &&
+	test "$du" != "$tri" &&
+	test "$du" != "$kvar" &&
+	test "$tri" != "$kvar" &&
+	git symbolic-ref refs/heads/KVAR refs/heads/du &&
+	kvarsym="$(git rev-parse --verify KVAR)" &&
+	test "$kvarsym" = "$du" &&
+	test "$kvarsym" != "$kvar" &&
+	git reflog exists HEAD &&
+	git reflog exists refs/heads/KVAR &&
+	git symbolic-ref HEAD >/dev/null &&
+	git symbolic-ref refs/heads/KVAR &&
+	git checkout unu &&
+	hcnt=$(git reflog show HEAD | wc -l) &&
+	kcnt=$(git reflog show refs/heads/KVAR | wc -l) &&
+	test -n "$hcnt" &&
+	test -n "$kcnt" &&
+	test $hcnt -gt 1 &&
+	test $kcnt -gt 1 &&
+	test $hcnt -ne $kcnt
+'
+
+test_expect_failure '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 &&
+	git symbolic-ref HEAD refs/heads/tri &&
+	hcnt2=$(git reflog show HEAD | wc -l) &&
+	test $hcnt1 = $hcnt2
+'
+
+test_expect_failure '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 &&
+	git symbolic-ref refs/heads/KVAR refs/heads/unu &&
+	kcnt2=$(git reflog show refs/heads/KVAR | wc -l) &&
+	test $kcnt1 = $kcnt2
+'
+
+test_expect_failure '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 &&
+	git symbolic-ref refs/heads/KVAR refs/heads/du &&
+	git symbolic-ref refs/heads/KVAR refs/heads/unu &&
+	git symbolic-ref refs/heads/KVAR refs/heads/tri &&
+	git symbolic-ref HEAD refs/heads/du &&
+	git symbolic-ref HEAD refs/heads/tri &&
+	git symbolic-ref HEAD refs/heads/unu &&
+	hcnt2=$(git reflog show HEAD | wc -l) &&
+	kcnt2=$(git reflog show refs/heads/KVAR | wc -l) &&
+	test $hcnt1 = $hcnt2 &&
+	test $kcnt1 = $kcnt2 &&
+	test $hcnt2 != $kcnt2
+'
+
+test_done

--

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help