[PATCH 0/4] Gets rid of "if reflog exists, append to it regardless of config settings"

As discussed in
CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com

Han-Wen Nienhuys (4):
  test-ref-store: tweaks to for-each-reflog-ent format
  t1400: use test-helper ref-store to inspect reflog contents
  refs: drop force_create argument of create_reflog API
  RFC: refs: reflog entries aren't written based on reflog existence.

 builtin/checkout.c             |  2 +-
 refs.c                         |  9 ++++-----
 refs.h                         |  4 ++--
 refs/debug.c                   |  5 ++---
 refs/files-backend.c           | 25 +++----------------------
 refs/packed-backend.c          |  3 +--
 refs/refs-internal.h           |  2 +-
 t/helper/test-ref-store.c      |  9 ++++-----
 t/t1400-update-ref.sh          | 21 ++++++++++++---------
 t/t1405-main-ref-store.sh      |  7 ++++---
 t/t1406-submodule-ref-store.sh |  6 +++---
 11 files changed, 37 insertions(+), 56 deletions(-)


base-commit: c4203212e360b25a1c69467b5a8437d45a373cac
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1067%2Fhanwen%2Freflog-touch-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1067/hanwen/reflog-touch-v1
Pull-Request: https://github.com/git/git/pull/1067
-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

Follow the reflog format more closely, so it can be used for comparing
reflogs in tests without using inspecting files under .git/logs/

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 t/helper/test-ref-store.c      | 6 +++---
 t/t1405-main-ref-store.sh      | 5 +++--
 t/t1406-submodule-ref-store.sh | 4 ++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index b314b81a45b..d7bbb20e614 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c

@@ -151,9 +151,9 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,
 		       const char *committer, timestamp_t timestamp,
 		       int tz, const char *msg, void *cb_data)
 {
-	printf("%s %s %s %"PRItime" %d %s\n",
-	       oid_to_hex(old_oid), oid_to_hex(new_oid),
-	       committer, timestamp, tz, msg);
+	const char *newline = strchr(msg, '\n') ? "" : "\n";
+	printf("%s %s %s %" PRItime " %+05d\t%s%s", oid_to_hex(old_oid),
+	       oid_to_hex(new_oid), committer, timestamp, tz, msg, newline);
 	return 0;
 }
 

diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 49718b7ea7f..16a99382770 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh

@@ -88,14 +88,15 @@ test_expect_success 'for_each_reflog()' '
 
 test_expect_success 'for_each_reflog_ent()' '
 	$RUN for-each-reflog-ent HEAD >actual &&
+	cat actual &&
 	head -n1 actual | grep one &&
-	tail -n2 actual | head -n1 | grep recreate-main
+	tail -n1 actual | grep recreate-main
 '
 
 test_expect_success 'for_each_reflog_ent_reverse()' '
 	$RUN for-each-reflog-ent-reverse HEAD >actual &&
 	head -n1 actual | grep recreate-main &&
-	tail -n2 actual | head -n1 | grep one
+	tail -n1 actual | grep one
 '
 
 test_expect_success 'reflog_exists(HEAD)' '

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index 0a87058971e..b0365c1fee0 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh

@@ -74,13 +74,13 @@ test_expect_success 'for_each_reflog()' '
 test_expect_success 'for_each_reflog_ent()' '
 	$RUN for-each-reflog-ent HEAD >actual &&
 	head -n1 actual | grep first &&
-	tail -n2 actual | head -n1 | grep main.to.new
+	tail -n1 actual | grep main.to.new
 '
 
 test_expect_success 'for_each_reflog_ent_reverse()' '
 	$RUN for-each-reflog-ent-reverse HEAD >actual &&
 	head -n1 actual | grep main.to.new &&
-	tail -n2 actual | head -n1 | grep first
+	tail -n1 actual | grep first
 '
 
 test_expect_success 'reflog_exists(HEAD)' '

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 t/t1400-update-ref.sh | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 4506cd435b0..de0cf678f8e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -315,14 +315,16 @@ test_expect_success 'symref empty directory removal' '
 	test_path_is_file .git/logs/HEAD
 '
 
+TAB='	'
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
-$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB
 EOF
 test_expect_success "verifying $m's log (logged by touch)" '
-	test_when_finished "rm -rf .git/$m .git/logs expect" &&
-	test_cmp expect .git/logs/$m
+	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
+	test-tool ref-store main for-each-reflog-ent $m > actual &&
+	test_cmp actual expect
 '
 
 test_expect_success "create $m (logged by config)" '

@@ -347,11 +349,12 @@ test_expect_success "set $m (logged by config)" '
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000	Initial Creation
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
-$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000$TAB
 EOF
 test_expect_success "verifying $m's log (logged by config)" '
-	test_when_finished "rm -f .git/$m .git/logs/$m expect" &&
-	test_cmp expect .git/logs/$m
+	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
+	test-tool ref-store main for-each-reflog-ent $m > actual &&
+	test_cmp actual expect
 '
 
 test_expect_success 'set up for querying the reflog' '

@@ -467,7 +470,8 @@ $h_OTHER $h_FIXED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151040 +0000	co
 $h_FIXED $h_MERGED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151100 +0000	commit (merge): Merged initial commit and a later commit.
 EOF
 test_expect_success 'git commit logged updates' '
-	test_cmp expect .git/logs/$m
+	test-tool ref-store main for-each-reflog-ent $m >actual &&
+	test_cmp expect actual
 '
 unset h_TEST h_OTHER h_FIXED h_MERGED
 

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

There is only one caller, builtin/checkout.c, and it hardcodes
force_create=1.

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 builtin/checkout.c             | 2 +-
 refs.c                         | 9 ++++-----
 refs.h                         | 4 ++--
 refs/debug.c                   | 5 ++---
 refs/files-backend.c           | 5 ++---
 refs/packed-backend.c          | 3 +--
 refs/refs-internal.h           | 2 +-
 t/helper/test-ref-store.c      | 3 +--
 t/t1405-main-ref-store.sh      | 2 +-
 t/t1406-submodule-ref-store.sh | 2 +-
 10 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index b5d477919a7..3c6506e0595 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -876,7 +876,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				int ret;
 				struct strbuf err = STRBUF_INIT;
 
-				ret = safe_create_reflog(refname, 1, &err);
+				ret = safe_create_reflog(refname, &err);
 				if (ret) {
 					fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
 						opts->new_orphan_branch, err.buf);

diff --git a/refs.c b/refs.c
index 8b9f7c3a80a..ca59a7cc652 100644
--- a/refs.c
+++ b/refs.c

@@ -2347,16 +2347,15 @@ int reflog_exists(const char *refname)
 }
 
 int refs_create_reflog(struct ref_store *refs, const char *refname,
-		       int force_create, struct strbuf *err)
+		       struct strbuf *err)
 {
-	return refs->be->create_reflog(refs, refname, force_create, err);
+	return refs->be->create_reflog(refs, refname, err);
 }
 
-int safe_create_reflog(const char *refname, int force_create,
-		       struct strbuf *err)
+int safe_create_reflog(const char *refname, struct strbuf *err)
 {
 	return refs_create_reflog(get_main_ref_store(the_repository), refname,
-				  force_create, err);
+				  err);
 }
 
 int refs_delete_reflog(struct ref_store *refs, const char *refname)

diff --git a/refs.h b/refs.h
index 48970dfc7e0..3c457bc19c8 100644
--- a/refs.h
+++ b/refs.h

@@ -416,8 +416,8 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags);
  * Setup reflog before using. Fill in err and return -1 on failure.
  */
 int refs_create_reflog(struct ref_store *refs, const char *refname,
-		       int force_create, struct strbuf *err);
-int safe_create_reflog(const char *refname, int force_create, struct strbuf *err);
+		       struct strbuf *err);
+int safe_create_reflog(const char *refname, struct strbuf *err);
 
 /** Reads log for the value of ref during at_time. **/
 int read_ref_at(struct ref_store *refs,

diff --git a/refs/debug.c b/refs/debug.c
index 1a7a9e11cfa..f6b01b1eba0 100644
--- a/refs/debug.c
+++ b/refs/debug.c

@@ -340,11 +340,10 @@ static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)
 }
 
 static int debug_create_reflog(struct ref_store *ref_store, const char *refname,
-			       int force_create, struct strbuf *err)
+			       struct strbuf *err)
 {
 	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
-	int res = drefs->refs->be->create_reflog(drefs->refs, refname,
-						 force_create, err);
+	int res = drefs->refs->be->create_reflog(drefs->refs, refname, err);
 	trace_printf_key(&trace_refs, "create_reflog: %s: %d\n", refname, res);
 	return res;
 }

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 677b7e4cdd2..e05ada9286d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1598,15 +1598,14 @@ error:
 	return -1;
 }
 
-static int files_create_reflog(struct ref_store *ref_store,
-			       const char *refname, int force_create,
+static int files_create_reflog(struct ref_store *ref_store, const char *refname,
 			       struct strbuf *err)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE, "create_reflog");
 	int fd;
 
-	if (log_ref_setup(refs, refname, force_create, &fd, err))
+	if (log_ref_setup(refs, refname, /*force_create=*/1, &fd, err))
 		return -1;
 
 	if (fd >= 0)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f8aa97d7998..af7038de42d 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c

@@ -1618,8 +1618,7 @@ static int packed_reflog_exists(struct ref_store *ref_store,
 }
 
 static int packed_create_reflog(struct ref_store *ref_store,
-			       const char *refname, int force_create,
-			       struct strbuf *err)
+				const char *refname, struct strbuf *err)
 {
 	BUG("packed reference store does not support reflogs");
 }

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 3155708345f..cc0e56e8c82 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h

@@ -590,7 +590,7 @@ typedef int for_each_reflog_ent_reverse_fn(struct ref_store *ref_store,
 					   void *cb_data);
 typedef int reflog_exists_fn(struct ref_store *ref_store, const char *refname);
 typedef int create_reflog_fn(struct ref_store *ref_store, const char *refname,
-			     int force_create, struct strbuf *err);
+			     struct strbuf *err);
 typedef int delete_reflog_fn(struct ref_store *ref_store, const char *refname);
 typedef int reflog_expire_fn(struct ref_store *ref_store,
 			     const char *refname, const struct object_id *oid,

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index d7bbb20e614..58d5438a290 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c

@@ -181,11 +181,10 @@ static int cmd_reflog_exists(struct ref_store *refs, const char **argv)
 static int cmd_create_reflog(struct ref_store *refs, const char **argv)
 {
 	const char *refname = notnull(*argv++, "refname");
-	int force_create = arg_flags(*argv++, "force-create");
 	struct strbuf err = STRBUF_INIT;
 	int ret;
 
-	ret = refs_create_reflog(refs, refname, force_create, &err);
+	ret = refs_create_reflog(refs, refname, &err);
 	if (err.len)
 		puts(err.buf);
 	return ret;

diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 16a99382770..4ee95fceccd 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh

@@ -109,7 +109,7 @@ test_expect_success 'delete_reflog(HEAD)' '
 '
 
 test_expect_success 'create-reflog(HEAD)' '
-	$RUN create-reflog HEAD 1 &&
+	$RUN create-reflog HEAD &&
 	git reflog exists HEAD
 '
 

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index b0365c1fee0..78d40bdcd8b 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh

@@ -92,7 +92,7 @@ test_expect_success 'delete_reflog() not allowed' '
 '
 
 test_expect_success 'create-reflog() not allowed' '
-	test_must_fail $RUN create-reflog HEAD 1
+	test_must_fail $RUN create-reflog HEAD
 '
 
 test_done

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

Before, if we aren't supposed to update reflogs (eg.
core.logallrefupdates=NONE), we would still write reflog entries if the
reflog file (.git/logs/REFNAME) existed.

The reftable storage backend cannot distinguish between a non-existing
reflog, and an empty one. Therefore it cannot mimick this functionality.

In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,
we came to the conclusion that this feature is probably a remnant from
the time that reflogs weren't enabled by default, and it does not need
to be kept.

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 refs/files-backend.c  | 20 +-------------------
 t/t1400-update-ref.sh |  7 +++----
 2 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e05ada9286d..25f5a4ce777 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1551,6 +1551,7 @@ static int log_ref_setup(struct files_ref_store *refs,
 	struct strbuf logfile_sb = STRBUF_INIT;
 	char *logfile;
 
+	*logfd = -1;
 	files_reflog_path(refs, &logfile_sb, refname);
 	logfile = strbuf_detach(&logfile_sb, NULL);
 

@@ -1565,26 +1566,8 @@ static int log_ref_setup(struct files_ref_store *refs,
 			else
 				strbuf_addf(err, "unable to append to '%s': %s",
 					    logfile, strerror(errno));
-
 			goto error;
 		}
-	} else {
-		*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
-		if (*logfd < 0) {
-			if (errno == ENOENT || errno == EISDIR) {
-				/*
-				 * The logfile doesn't already exist,
-				 * but that is not an error; it only
-				 * means that we won't write log
-				 * entries to it.
-				 */
-				;
-			} else {
-				strbuf_addf(err, "unable to append to '%s': %s",
-					    logfile, strerror(errno));
-				goto error;
-			}
-		}
 	}
 
 	if (*logfd >= 0)

@@ -1592,7 +1575,6 @@ static int log_ref_setup(struct files_ref_store *refs,
 
 	free(logfile);
 	return 0;
-
 error:
 	free(logfile);
 	return -1;

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index de0cf678f8e..52433f6d0d2 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -269,7 +269,7 @@ test_expect_success "(not) changed .git/$m" '
 '
 
 rm -f .git/logs/refs/heads/main
-test_expect_success "create $m (logged by touch)" '
+test_expect_success "create $m" '
 	test_config core.logAllRefUpdates false &&
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
 	git update-ref --create-reflog HEAD $A -m "Initial Creation" &&

@@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' '
 	test_path_is_file .git/logs/HEAD
 '
 
-TAB='	'
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
-$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
-$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB
 EOF
+
 test_expect_success "verifying $m's log (logged by touch)" '
 	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
 	test-tool ref-store main for-each-reflog-ent $m > actual &&

@@ -346,6 +344,7 @@ test_expect_success "set $m (logged by config)" '
 	test $A = $(git show-ref -s --verify $m)
 '
 
+TAB='	'
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000	Initial Creation
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch

-- 
gitgitgadget

On Mon, Aug 30, 2021 at 02:48:45PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

Nit; stray debugging.

Thanks,
Taylor

On Mon, Aug 30, 2021 at 02:48:45PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

Having read the rest of the series, I did scratch my head quite a bit
here, but I think the change is actually quite simple. In the files
backend, show_one_reflog_ent is parsing line-wise, and each line ends
with the LF.

Of course, we don't expect the reflog to have messages that actually
contain a newline because we cleanse them with copy_reflog_message()
before writing.

So really it seems like the files-backend should be calling rstrip on
the message before handing it to the callback. Either that, or we could
call rstrip ourselves (since the generic strchr() makes me think that
the LF could appear anywhere in the string, at least on first read).

Thanks,
Taylor

On Mon, Aug 30, 2021 at 02:48:48PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:

From: Han-Wen Nienhuys <redacted>

Before, if we aren't supposed to update reflogs (eg.
core.logallrefupdates=NONE), we would still write reflog entries if the
reflog file (.git/logs/REFNAME) existed.

The reftable storage backend cannot distinguish between a non-existing
reflog, and an empty one. Therefore it cannot mimick this functionality.

In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,

This looks like a faithful implementation of that discussion, so I don't
see any problems with the change.

I'm nit-picking, but I think that this conditional could probably be
moved into the if-statement above, since we no longer set logfd anywhere
else. It might read more clearly, and if you're going to resubmit this
series anyway, it would be helpful to pick that up.

OK, so "Initial Creation" happened in a state where
"core.logAllRefUpdates=false", but we passed "--create-reflog" to force
the behavior anyway.

Then during "Switch", we had a reflog, but again "core.logAllRefUpdates"
is false, and we didn't pass "--create-reflog", so no more updates are
written to the reflog.

That's exactly the test that I would have expected to exist for this
change, and so this LGTM.

Thanks,
Taylor

As discussed in
CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com

v2 addresses all outstanding comments.

There is one open question: three is a line marked "XXX" in commit 995d450d
("refs: trim newline from reflog message"). I'd think it would print a
double newline before (which seems wrong), but I'm unsure how to get the
codepath to run.

Han-Wen Nienhuys (5):
  refs: trim newline from reflog message
  test-ref-store: tweaks to for-each-reflog-ent format
  t1400: use test-helper ref-store to inspect reflog contents
  refs: drop force_create argument of create_reflog API
  RFC: refs: reflog entries aren't written based on reflog existence.

 builtin/checkout.c             |  2 +-
 builtin/show-branch.c          |  4 +-
 reflog-walk.c                  |  6 +--
 refs.c                         |  9 ++--
 refs.h                         |  4 +-
 refs/debug.c                   |  5 +-
 refs/files-backend.c           | 88 +++++++++++++---------------------
 refs/packed-backend.c          |  3 +-
 refs/refs-internal.h           |  2 +-
 t/helper/test-ref-store.c      |  8 ++--
 t/t1400-update-ref.sh          | 21 ++++----
 t/t1405-main-ref-store.sh      |  6 +--
 t/t1406-submodule-ref-store.sh |  6 +--
 13 files changed, 71 insertions(+), 93 deletions(-)


base-commit: e0a2f5cbc585657e757385ad918f167f519cfb96
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1067%2Fhanwen%2Freflog-touch-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1067/hanwen/reflog-touch-v2
Pull-Request: https://github.com/git/git/pull/1067

Range-diff vs v1:

 1:  d48207d6858 ! 1:  995d450da42 test-ref-store: tweaks to for-each-reflog-ent format
     @@ Metadata
      Author: Han-Wen Nienhuys [off-list ref]
      
       ## Commit message ##
     -    test-ref-store: tweaks to for-each-reflog-ent format
     +    refs: trim newline from reflog message
      
     -    Follow the reflog format more closely, so it can be used for comparing
     -    reflogs in tests without using inspecting files under .git/logs/
     +    Commit 523fa69c ("reflog: cleanse messages in the refs.c layer") standardizes
     +    how write entries into the reflog. This commit standardizes how we get messages
     +    out of the reflog. Before, the files backend implicitly added '\n' to the end of
     +    reflog message on reading, which creates a subtle incompatibility with alternate
     +    ref storage backends, such as reftable.
     +
     +    We address this by stripping LF from the message before we pass it to the
     +    user-provided callback.
      
          Signed-off-by: Han-Wen Nienhuys [off-list ref]
      
     - ## t/helper/test-ref-store.c ##
     -@@ t/helper/test-ref-store.c: static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,
     - 		       const char *committer, timestamp_t timestamp,
     - 		       int tz, const char *msg, void *cb_data)
     - {
     --	printf("%s %s %s %"PRItime" %d %s\n",
     --	       oid_to_hex(old_oid), oid_to_hex(new_oid),
     --	       committer, timestamp, tz, msg);
     -+	const char *newline = strchr(msg, '\n') ? "" : "\n";
     -+	printf("%s %s %s %" PRItime " %+05d\t%s%s", oid_to_hex(old_oid),
     -+	       oid_to_hex(new_oid), committer, timestamp, tz, msg, newline);
     - 	return 0;
     + ## builtin/show-branch.c ##
     +@@ builtin/show-branch.c: int cmd_show_branch(int ac, const char **av, const char *prefix)
     + 				show_one_commit(rev[i], 1);
     + 			}
     + 			else
     +-				puts(reflog_msg[i]);
     ++				puts(reflog_msg[i]); /* XXX - this puts a
     ++							newline. Did we put two
     ++							newlines beforehand? */
     + 
     + 			if (is_head)
     + 				head_at = i;
     +
     + ## reflog-walk.c ##
     +@@ reflog-walk.c: void get_reflog_message(struct strbuf *sb,
     + 
     + 	info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
     + 	len = strlen(info->message);
     +-	if (len > 0)
     +-		len--; /* strip away trailing newline */
     + 	strbuf_add(sb, info->message, len);
       }
       
     +@@ reflog-walk.c: void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
     + 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
     + 		get_reflog_selector(&selector, reflog_info, dmode, force_date, 0);
     + 		if (oneline) {
     +-			printf("%s: %s", selector.buf, info->message);
     ++			printf("%s: %s\n", selector.buf, info->message);
     + 		}
     + 		else {
     +-			printf("Reflog: %s (%s)\nReflog message: %s",
     ++			printf("Reflog: %s (%s)\nReflog message: %s\n",
     + 			       selector.buf, info->email, info->message);
     + 		}
     + 
     +
     + ## refs/files-backend.c ##
     +@@ refs/files-backend.c: static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
     + 	int tz;
     + 	const char *p = sb->buf;
     + 
     +-	/* old SP new SP name <email> SP time TAB msg LF */
     +-	if (!sb->len || sb->buf[sb->len - 1] != '\n' ||
     +-	    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
     ++	/* old SP new SP name <email> SP time TAB msg */
     ++	if (!sb->len || parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
     + 	    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||
     +-	    !(email_end = strchr(p, '>')) ||
     +-	    email_end[1] != ' ' ||
     ++	    !(email_end = strchr(p, '>')) || email_end[1] != ' ' ||
     + 	    !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||
     + 	    !message || message[0] != ' ' ||
     +-	    (message[1] != '+' && message[1] != '-') ||
     +-	    !isdigit(message[2]) || !isdigit(message[3]) ||
     +-	    !isdigit(message[4]) || !isdigit(message[5]))
     ++	    (message[1] != '+' && message[1] != '-') || !isdigit(message[2]) ||
     ++	    !isdigit(message[3]) || !isdigit(message[4]) ||
     ++	    !isdigit(message[5]))
     + 		return 0; /* corrupt? */
     + 	email_end[1] = '\0';
     + 	tz = strtol(message + 1, NULL, 10);
     +@@ refs/files-backend.c: static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
     + 				strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));
     + 				scanp = bp;
     + 				endp = bp + 1;
     ++				strbuf_trim_trailing_newline(&sb);
     + 				ret = show_one_reflog_ent(&sb, fn, cb_data);
     + 				strbuf_reset(&sb);
     + 				if (ret)
     +@@ refs/files-backend.c: static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
     + 				 * Process it, and we can end the loop.
     + 				 */
     + 				strbuf_splice(&sb, 0, 0, buf, endp - buf);
     ++				strbuf_trim_trailing_newline(&sb);
     + 				ret = show_one_reflog_ent(&sb, fn, cb_data);
     + 				strbuf_reset(&sb);
     + 				break;
     +@@ refs/files-backend.c: static int files_for_each_reflog_ent(struct ref_store *ref_store,
     + 	if (!logfp)
     + 		return -1;
     + 
     +-	while (!ret && !strbuf_getwholeline(&sb, logfp, '\n'))
     ++	while (!ret && !strbuf_getline(&sb, logfp))
     + 		ret = show_one_reflog_ent(&sb, fn, cb_data);
     + 	fclose(logfp);
     + 	strbuf_release(&sb);
     +@@ refs/files-backend.c: static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
     + 	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
     + 				   message, policy_cb)) {
     + 		if (!cb->newlog)
     +-			printf("would prune %s", message);
     ++			printf("would prune %s\n", message);
     + 		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
     +-			printf("prune %s", message);
     ++			printf("prune %s\n", message);
     + 	} else {
     + 		if (cb->newlog) {
     +-			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
     +-				oid_to_hex(ooid), oid_to_hex(noid),
     +-				email, timestamp, tz, message);
     ++			fprintf(cb->newlog, "%s %s %s %" PRItime " %+05d\t%s\n",
     ++				oid_to_hex(ooid), oid_to_hex(noid), email,
     ++				timestamp, tz, message);
     + 			oidcpy(&cb->last_kept_oid, noid);
     + 		}
     + 		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
     +-			printf("keep %s", message);
     ++			printf("keep %s\n", message);
     + 	}
     + 	return 0;
     + }
      
       ## t/t1405-main-ref-store.sh ##
      @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog()' '
     - 
       test_expect_success 'for_each_reflog_ent()' '
       	$RUN for-each-reflog-ent HEAD >actual &&
     -+	cat actual &&
       	head -n1 actual | grep one &&
      -	tail -n2 actual | head -n1 | grep recreate-main
      +	tail -n1 actual | grep recreate-main
     @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog()' '
       
       test_expect_success 'for_each_reflog_ent_reverse()' '
       	$RUN for-each-reflog-ent-reverse HEAD >actual &&
     - 	head -n1 actual | grep recreate-main &&
     +-	head -n1 actual | grep recreate-main &&
      -	tail -n2 actual | head -n1 | grep one
      +	tail -n1 actual | grep one
       '
 -:  ----------- > 2:  11b296a55e9 test-ref-store: tweaks to for-each-reflog-ent format
 2:  dab8e115faf ! 3:  9ec09cc64cd t1400: use test-helper ref-store to inspect reflog contents
     @@ Commit message
          Signed-off-by: Han-Wen Nienhuys [off-list ref]
      
       ## t/t1400-update-ref.sh ##
     -@@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '
     - 	test_path_is_file .git/logs/HEAD
     - '
     +@@ t/t1400-update-ref.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     + . ./test-lib.sh
       
     + Z=$ZERO_OID
      +TAB='	'
     + 
     + m=refs/heads/main
     + n_dir=refs/heads/gu
     +@@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '
       cat >expect <<EOF
       $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
       $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
 3:  048e2d17077 = 4:  aa25fd9b7de refs: drop force_create argument of create_reflog API
 4:  2f11fd77180 ! 5:  f6a7c5ad56e RFC: refs: reflog entries aren't written based on reflog existence.
     @@ refs/files-backend.c: static int log_ref_setup(struct files_ref_store *refs,
       	char *logfile;
       
      +	*logfd = -1;
     ++	if (!force_create && !should_autocreate_reflog(refname))
     ++		return 0;
     ++
       	files_reflog_path(refs, &logfile_sb, refname);
       	logfile = strbuf_detach(&logfile_sb, NULL);
       
     -@@ refs/files-backend.c: static int log_ref_setup(struct files_ref_store *refs,
     - 			else
     - 				strbuf_addf(err, "unable to append to '%s': %s",
     - 					    logfile, strerror(errno));
     +-	if (force_create || should_autocreate_reflog(refname)) {
     +-		if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
     +-			if (errno == ENOENT)
     +-				strbuf_addf(err, "unable to create directory for '%s': "
     +-					    "%s", logfile, strerror(errno));
     +-			else if (errno == EISDIR)
     +-				strbuf_addf(err, "there are still logs under '%s'",
     +-					    logfile);
     +-			else
     +-				strbuf_addf(err, "unable to append to '%s': %s",
     +-					    logfile, strerror(errno));
      -
     - 			goto error;
     - 		}
     +-			goto error;
     +-		}
      -	} else {
      -		*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
      -		if (*logfd < 0) {
     @@ refs/files-backend.c: static int log_ref_setup(struct files_ref_store *refs,
      -				goto error;
      -			}
      -		}
     ++	if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
     ++		if (errno == ENOENT)
     ++			strbuf_addf(err,
     ++				    "unable to create directory for '%s': "
     ++				    "%s",
     ++				    logfile, strerror(errno));
     ++		else if (errno == EISDIR)
     ++			strbuf_addf(err, "there are still logs under '%s'",
     ++				    logfile);
     ++		else
     ++			strbuf_addf(err, "unable to append to '%s': %s",
     ++				    logfile, strerror(errno));
       	}
       
       	if (*logfd >= 0)
     -@@ refs/files-backend.c: static int log_ref_setup(struct files_ref_store *refs,
     + 		adjust_shared_perm(logfile);
       
       	free(logfile);
     - 	return 0;
     +-	return 0;
      -
     - error:
     - 	free(logfile);
     - 	return -1;
     +-error:
     +-	free(logfile);
     +-	return -1;
     ++	return (*logfd < 0) ? -1 : 0;
     + }
     + 
     + static int files_create_reflog(struct ref_store *ref_store, const char *refname,
      
       ## t/t1400-update-ref.sh ##
      @@ t/t1400-update-ref.sh: test_expect_success "(not) changed .git/$m" '
     @@ t/t1400-update-ref.sh: test_expect_success "(not) changed .git/$m" '
       	GIT_COMMITTER_DATE="2005-05-26 23:30" \
       	git update-ref --create-reflog HEAD $A -m "Initial Creation" &&
      @@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '
     - 	test_path_is_file .git/logs/HEAD
     - '
       
     --TAB='	'
       cat >expect <<EOF
       $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
      -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
     @@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '
       test_expect_success "verifying $m's log (logged by touch)" '
       	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
       	test-tool ref-store main for-each-reflog-ent $m > actual &&
     -@@ t/t1400-update-ref.sh: test_expect_success "set $m (logged by config)" '
     - 	test $A = $(git show-ref -s --verify $m)
     - '
     - 
     -+TAB='	'
     - cat >expect <<EOF
     - $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000	Initial Creation
     - $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

Commit 523fa69c ("reflog: cleanse messages in the refs.c layer") standardizes
how write entries into the reflog. This commit standardizes how we get messages
out of the reflog. Before, the files backend implicitly added '\n' to the end of
reflog message on reading, which creates a subtle incompatibility with alternate
ref storage backends, such as reftable.

We address this by stripping LF from the message before we pass it to the
user-provided callback.

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 builtin/show-branch.c          |  4 +++-
 reflog-walk.c                  |  6 ++----
 refs/files-backend.c           | 30 +++++++++++++++---------------
 t/t1405-main-ref-store.sh      |  5 ++---
 t/t1406-submodule-ref-store.sh |  4 ++--
 5 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d77ce7aeb38..acf710313a0 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c

@@ -888,7 +888,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				show_one_commit(rev[i], 1);
 			}
 			else
-				puts(reflog_msg[i]);
+				puts(reflog_msg[i]); /* XXX - this puts a
+							newline. Did we put two
+							newlines beforehand? */
 
 			if (is_head)
 				head_at = i;

diff --git a/reflog-walk.c b/reflog-walk.c
index e9cd3283694..944cce929c8 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c

@@ -245,8 +245,6 @@ void get_reflog_message(struct strbuf *sb,
 
 	info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
 	len = strlen(info->message);
-	if (len > 0)
-		len--; /* strip away trailing newline */
 	strbuf_add(sb, info->message, len);
 }
 

@@ -285,10 +283,10 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
 		get_reflog_selector(&selector, reflog_info, dmode, force_date, 0);
 		if (oneline) {
-			printf("%s: %s", selector.buf, info->message);
+			printf("%s: %s\n", selector.buf, info->message);
 		}
 		else {
-			printf("Reflog: %s (%s)\nReflog message: %s",
+			printf("Reflog: %s (%s)\nReflog message: %s\n",
 			       selector.buf, info->email, info->message);
 		}
 

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 677b7e4cdd2..206c9f8b932 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1897,17 +1897,15 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
 	int tz;
 	const char *p = sb->buf;
 
-	/* old SP new SP name <email> SP time TAB msg LF */
-	if (!sb->len || sb->buf[sb->len - 1] != '\n' ||
-	    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
+	/* old SP new SP name <email> SP time TAB msg */
+	if (!sb->len || parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
 	    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||
-	    !(email_end = strchr(p, '>')) ||
-	    email_end[1] != ' ' ||
+	    !(email_end = strchr(p, '>')) || email_end[1] != ' ' ||
 	    !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||
 	    !message || message[0] != ' ' ||
-	    (message[1] != '+' && message[1] != '-') ||
-	    !isdigit(message[2]) || !isdigit(message[3]) ||
-	    !isdigit(message[4]) || !isdigit(message[5]))
+	    (message[1] != '+' && message[1] != '-') || !isdigit(message[2]) ||
+	    !isdigit(message[3]) || !isdigit(message[4]) ||
+	    !isdigit(message[5]))
 		return 0; /* corrupt? */
 	email_end[1] = '\0';
 	tz = strtol(message + 1, NULL, 10);

@@ -1999,6 +1997,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 				strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));
 				scanp = bp;
 				endp = bp + 1;
+				strbuf_trim_trailing_newline(&sb);
 				ret = show_one_reflog_ent(&sb, fn, cb_data);
 				strbuf_reset(&sb);
 				if (ret)

@@ -2011,6 +2010,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 				 * Process it, and we can end the loop.
 				 */
 				strbuf_splice(&sb, 0, 0, buf, endp - buf);
+				strbuf_trim_trailing_newline(&sb);
 				ret = show_one_reflog_ent(&sb, fn, cb_data);
 				strbuf_reset(&sb);
 				break;

@@ -2060,7 +2060,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
 	if (!logfp)
 		return -1;
 
-	while (!ret && !strbuf_getwholeline(&sb, logfp, '\n'))
+	while (!ret && !strbuf_getline(&sb, logfp))
 		ret = show_one_reflog_ent(&sb, fn, cb_data);
 	fclose(logfp);
 	strbuf_release(&sb);

@@ -3020,18 +3020,18 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
 				   message, policy_cb)) {
 		if (!cb->newlog)
-			printf("would prune %s", message);
+			printf("would prune %s\n", message);
 		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("prune %s", message);
+			printf("prune %s\n", message);
 	} else {
 		if (cb->newlog) {
-			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
-				oid_to_hex(ooid), oid_to_hex(noid),
-				email, timestamp, tz, message);
+			fprintf(cb->newlog, "%s %s %s %" PRItime " %+05d\t%s\n",
+				oid_to_hex(ooid), oid_to_hex(noid), email,
+				timestamp, tz, message);
 			oidcpy(&cb->last_kept_oid, noid);
 		}
 		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
-			printf("keep %s", message);
+			printf("keep %s\n", message);
 	}
 	return 0;
 }

diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 49718b7ea7f..a600bedf2cd 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh

@@ -89,13 +89,12 @@ test_expect_success 'for_each_reflog()' '
 test_expect_success 'for_each_reflog_ent()' '
 	$RUN for-each-reflog-ent HEAD >actual &&
 	head -n1 actual | grep one &&
-	tail -n2 actual | head -n1 | grep recreate-main
+	tail -n1 actual | grep recreate-main
 '
 
 test_expect_success 'for_each_reflog_ent_reverse()' '
 	$RUN for-each-reflog-ent-reverse HEAD >actual &&
-	head -n1 actual | grep recreate-main &&
-	tail -n2 actual | head -n1 | grep one
+	tail -n1 actual | grep one
 '
 
 test_expect_success 'reflog_exists(HEAD)' '

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index 0a87058971e..b0365c1fee0 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh

@@ -74,13 +74,13 @@ test_expect_success 'for_each_reflog()' '
 test_expect_success 'for_each_reflog_ent()' '
 	$RUN for-each-reflog-ent HEAD >actual &&
 	head -n1 actual | grep first &&
-	tail -n2 actual | head -n1 | grep main.to.new
+	tail -n1 actual | grep main.to.new
 '
 
 test_expect_success 'for_each_reflog_ent_reverse()' '
 	$RUN for-each-reflog-ent-reverse HEAD >actual &&
 	head -n1 actual | grep main.to.new &&
-	tail -n2 actual | head -n1 | grep first
+	tail -n1 actual | grep first
 '
 
 test_expect_success 'reflog_exists(HEAD)' '

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

Follow the reflog format more closely, so it can be used for comparing
reflogs in tests without using inspecting files under .git/logs/

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 t/helper/test-ref-store.c | 5 ++---
 t/t1405-main-ref-store.sh | 1 +
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index b314b81a45b..0fcad9b3812 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c

@@ -151,9 +151,8 @@ static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,
 		       const char *committer, timestamp_t timestamp,
 		       int tz, const char *msg, void *cb_data)
 {
-	printf("%s %s %s %"PRItime" %d %s\n",
-	       oid_to_hex(old_oid), oid_to_hex(new_oid),
-	       committer, timestamp, tz, msg);
+	printf("%s %s %s %" PRItime " %+05d\t%s\n", oid_to_hex(old_oid),
+	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
 	return 0;
 }
 

diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index a600bedf2cd..76b15458409 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh

@@ -94,6 +94,7 @@ test_expect_success 'for_each_reflog_ent()' '
 
 test_expect_success 'for_each_reflog_ent_reverse()' '
 	$RUN for-each-reflog-ent-reverse HEAD >actual &&
+	head -n1 actual | grep recreate-main &&
 	tail -n1 actual | grep one
 '
 

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 t/t1400-update-ref.sh | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 4506cd435b0..8ced98e0fe8 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -10,6 +10,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 Z=$ZERO_OID
+TAB='	'
 
 m=refs/heads/main
 n_dir=refs/heads/gu

@@ -318,11 +319,12 @@ test_expect_success 'symref empty directory removal' '
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
-$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB
 EOF
 test_expect_success "verifying $m's log (logged by touch)" '
-	test_when_finished "rm -rf .git/$m .git/logs expect" &&
-	test_cmp expect .git/logs/$m
+	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
+	test-tool ref-store main for-each-reflog-ent $m > actual &&
+	test_cmp actual expect
 '
 
 test_expect_success "create $m (logged by config)" '

@@ -347,11 +349,12 @@ test_expect_success "set $m (logged by config)" '
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000	Initial Creation
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
-$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000$TAB
 EOF
 test_expect_success "verifying $m's log (logged by config)" '
-	test_when_finished "rm -f .git/$m .git/logs/$m expect" &&
-	test_cmp expect .git/logs/$m
+	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
+	test-tool ref-store main for-each-reflog-ent $m > actual &&
+	test_cmp actual expect
 '
 
 test_expect_success 'set up for querying the reflog' '

@@ -467,7 +470,8 @@ $h_OTHER $h_FIXED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151040 +0000	co
 $h_FIXED $h_MERGED $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117151100 +0000	commit (merge): Merged initial commit and a later commit.
 EOF
 test_expect_success 'git commit logged updates' '
-	test_cmp expect .git/logs/$m
+	test-tool ref-store main for-each-reflog-ent $m >actual &&
+	test_cmp expect actual
 '
 unset h_TEST h_OTHER h_FIXED h_MERGED
 

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

There is only one caller, builtin/checkout.c, and it hardcodes
force_create=1.

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 builtin/checkout.c             | 2 +-
 refs.c                         | 9 ++++-----
 refs.h                         | 4 ++--
 refs/debug.c                   | 5 ++---
 refs/files-backend.c           | 5 ++---
 refs/packed-backend.c          | 3 +--
 refs/refs-internal.h           | 2 +-
 t/helper/test-ref-store.c      | 3 +--
 t/t1405-main-ref-store.sh      | 2 +-
 t/t1406-submodule-ref-store.sh | 2 +-
 10 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index b5d477919a7..3c6506e0595 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -876,7 +876,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				int ret;
 				struct strbuf err = STRBUF_INIT;
 
-				ret = safe_create_reflog(refname, 1, &err);
+				ret = safe_create_reflog(refname, &err);
 				if (ret) {
 					fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
 						opts->new_orphan_branch, err.buf);

diff --git a/refs.c b/refs.c
index 8b9f7c3a80a..ca59a7cc652 100644
--- a/refs.c
+++ b/refs.c

@@ -2347,16 +2347,15 @@ int reflog_exists(const char *refname)
 }
 
 int refs_create_reflog(struct ref_store *refs, const char *refname,
-		       int force_create, struct strbuf *err)
+		       struct strbuf *err)
 {
-	return refs->be->create_reflog(refs, refname, force_create, err);
+	return refs->be->create_reflog(refs, refname, err);
 }
 
-int safe_create_reflog(const char *refname, int force_create,
-		       struct strbuf *err)
+int safe_create_reflog(const char *refname, struct strbuf *err)
 {
 	return refs_create_reflog(get_main_ref_store(the_repository), refname,
-				  force_create, err);
+				  err);
 }
 
 int refs_delete_reflog(struct ref_store *refs, const char *refname)

diff --git a/refs.h b/refs.h
index 48970dfc7e0..3c457bc19c8 100644
--- a/refs.h
+++ b/refs.h

@@ -416,8 +416,8 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags);
  * Setup reflog before using. Fill in err and return -1 on failure.
  */
 int refs_create_reflog(struct ref_store *refs, const char *refname,
-		       int force_create, struct strbuf *err);
-int safe_create_reflog(const char *refname, int force_create, struct strbuf *err);
+		       struct strbuf *err);
+int safe_create_reflog(const char *refname, struct strbuf *err);
 
 /** Reads log for the value of ref during at_time. **/
 int read_ref_at(struct ref_store *refs,

diff --git a/refs/debug.c b/refs/debug.c
index 1a7a9e11cfa..f6b01b1eba0 100644
--- a/refs/debug.c
+++ b/refs/debug.c

@@ -340,11 +340,10 @@ static int debug_reflog_exists(struct ref_store *ref_store, const char *refname)
 }
 
 static int debug_create_reflog(struct ref_store *ref_store, const char *refname,
-			       int force_create, struct strbuf *err)
+			       struct strbuf *err)
 {
 	struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store;
-	int res = drefs->refs->be->create_reflog(drefs->refs, refname,
-						 force_create, err);
+	int res = drefs->refs->be->create_reflog(drefs->refs, refname, err);
 	trace_printf_key(&trace_refs, "create_reflog: %s: %d\n", refname, res);
 	return res;
 }

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 206c9f8b932..b710d43be16 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1598,15 +1598,14 @@ error:
 	return -1;
 }
 
-static int files_create_reflog(struct ref_store *ref_store,
-			       const char *refname, int force_create,
+static int files_create_reflog(struct ref_store *ref_store, const char *refname,
 			       struct strbuf *err)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE, "create_reflog");
 	int fd;
 
-	if (log_ref_setup(refs, refname, force_create, &fd, err))
+	if (log_ref_setup(refs, refname, /*force_create=*/1, &fd, err))
 		return -1;
 
 	if (fd >= 0)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index f8aa97d7998..af7038de42d 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c

@@ -1618,8 +1618,7 @@ static int packed_reflog_exists(struct ref_store *ref_store,
 }
 
 static int packed_create_reflog(struct ref_store *ref_store,
-			       const char *refname, int force_create,
-			       struct strbuf *err)
+				const char *refname, struct strbuf *err)
 {
 	BUG("packed reference store does not support reflogs");
 }

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 3155708345f..cc0e56e8c82 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h

@@ -590,7 +590,7 @@ typedef int for_each_reflog_ent_reverse_fn(struct ref_store *ref_store,
 					   void *cb_data);
 typedef int reflog_exists_fn(struct ref_store *ref_store, const char *refname);
 typedef int create_reflog_fn(struct ref_store *ref_store, const char *refname,
-			     int force_create, struct strbuf *err);
+			     struct strbuf *err);
 typedef int delete_reflog_fn(struct ref_store *ref_store, const char *refname);
 typedef int reflog_expire_fn(struct ref_store *ref_store,
 			     const char *refname, const struct object_id *oid,

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 0fcad9b3812..a65fda66ddc 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c

@@ -180,11 +180,10 @@ static int cmd_reflog_exists(struct ref_store *refs, const char **argv)
 static int cmd_create_reflog(struct ref_store *refs, const char **argv)
 {
 	const char *refname = notnull(*argv++, "refname");
-	int force_create = arg_flags(*argv++, "force-create");
 	struct strbuf err = STRBUF_INIT;
 	int ret;
 
-	ret = refs_create_reflog(refs, refname, force_create, &err);
+	ret = refs_create_reflog(refs, refname, &err);
 	if (err.len)
 		puts(err.buf);
 	return ret;

diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
index 76b15458409..3cb5e23d6db 100755
--- a/t/t1405-main-ref-store.sh
+++ b/t/t1405-main-ref-store.sh

@@ -108,7 +108,7 @@ test_expect_success 'delete_reflog(HEAD)' '
 '
 
 test_expect_success 'create-reflog(HEAD)' '
-	$RUN create-reflog HEAD 1 &&
+	$RUN create-reflog HEAD &&
 	git reflog exists HEAD
 '
 

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index b0365c1fee0..78d40bdcd8b 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh

@@ -92,7 +92,7 @@ test_expect_success 'delete_reflog() not allowed' '
 '
 
 test_expect_success 'create-reflog() not allowed' '
-	test_must_fail $RUN create-reflog HEAD 1
+	test_must_fail $RUN create-reflog HEAD
 '
 
 test_done

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

Before, if we aren't supposed to update reflogs (eg.
core.logallrefupdates=NONE), we would still write reflog entries if the
reflog file (.git/logs/REFNAME) existed.

The reftable storage backend cannot distinguish between a non-existing
reflog, and an empty one. Therefore it cannot mimick this functionality.

In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,
we came to the conclusion that this feature is probably a remnant from
the time that reflogs weren't enabled by default, and it does not need
to be kept.

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 refs/files-backend.c  | 53 ++++++++++++++-----------------------------
 t/t1400-update-ref.sh |  5 ++--
 2 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index b710d43be16..5ba68584aba 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c

@@ -1551,51 +1551,32 @@ static int log_ref_setup(struct files_ref_store *refs,
 	struct strbuf logfile_sb = STRBUF_INIT;
 	char *logfile;
 
+	*logfd = -1;
+	if (!force_create && !should_autocreate_reflog(refname))
+		return 0;
+
 	files_reflog_path(refs, &logfile_sb, refname);
 	logfile = strbuf_detach(&logfile_sb, NULL);
 
-	if (force_create || should_autocreate_reflog(refname)) {
-		if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
-			if (errno == ENOENT)
-				strbuf_addf(err, "unable to create directory for '%s': "
-					    "%s", logfile, strerror(errno));
-			else if (errno == EISDIR)
-				strbuf_addf(err, "there are still logs under '%s'",
-					    logfile);
-			else
-				strbuf_addf(err, "unable to append to '%s': %s",
-					    logfile, strerror(errno));
-
-			goto error;
-		}
-	} else {
-		*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
-		if (*logfd < 0) {
-			if (errno == ENOENT || errno == EISDIR) {
-				/*
-				 * The logfile doesn't already exist,
-				 * but that is not an error; it only
-				 * means that we won't write log
-				 * entries to it.
-				 */
-				;
-			} else {
-				strbuf_addf(err, "unable to append to '%s': %s",
-					    logfile, strerror(errno));
-				goto error;
-			}
-		}
+	if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
+		if (errno == ENOENT)
+			strbuf_addf(err,
+				    "unable to create directory for '%s': "
+				    "%s",
+				    logfile, strerror(errno));
+		else if (errno == EISDIR)
+			strbuf_addf(err, "there are still logs under '%s'",
+				    logfile);
+		else
+			strbuf_addf(err, "unable to append to '%s': %s",
+				    logfile, strerror(errno));
 	}
 
 	if (*logfd >= 0)
 		adjust_shared_perm(logfile);
 
 	free(logfile);
-	return 0;
-
-error:
-	free(logfile);
-	return -1;
+	return (*logfd < 0) ? -1 : 0;
 }
 
 static int files_create_reflog(struct ref_store *ref_store, const char *refname,

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 8ced98e0fe8..446b568cef3 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh

@@ -270,7 +270,7 @@ test_expect_success "(not) changed .git/$m" '
 '
 
 rm -f .git/logs/refs/heads/main
-test_expect_success "create $m (logged by touch)" '
+test_expect_success "create $m" '
 	test_config core.logAllRefUpdates false &&
 	GIT_COMMITTER_DATE="2005-05-26 23:30" \
 	git update-ref --create-reflog HEAD $A -m "Initial Creation" &&

@@ -318,9 +318,8 @@ test_expect_success 'symref empty directory removal' '
 
 cat >expect <<EOF
 $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
-$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
-$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB
 EOF
+
 test_expect_success "verifying $m's log (logged by touch)" '
 	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
 	test-tool ref-store main for-each-reflog-ent $m > actual &&

-- 
gitgitgadget

On Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:

Nit: Would be a more readable diff if this wasn't a
line-wrap-while-at-it change in addition to changing the format string.

I.e. the last 4x parameters aren't changed, so leaving them on their own
line & just changing the string & the two oid_to_hex()...

On Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:

From: Han-Wen Nienhuys <redacted>
[...]
-				puts(reflog_msg[i]);
+				puts(reflog_msg[i]); /* XXX - this puts a
+							newline. Did we put two
+							newlines beforehand? */

I recently added some tests for show-branch, see the tip of
ab/show-branch-tests. This seems like it would be clearer to both you &
reviewers if we first checked & test_cmp'd the reflog behavior of "git
show-branch" in some "here's how it works now test", then made whatever
non-changes to the format done here.

Presumably this newline mystery would become clear once we'd have
coverage of its reflog codepaths.

On Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:

From: Han-Wen Nienhuys <redacted>

There is only one caller, builtin/checkout.c, and it hardcodes
force_create=1.

Was it ever needed? A glance at abd0cd3a301 (refs: new public ref
function: safe_create_reflog, 2015-07-21) suggests probably not, but
then there's 0f2a71d9923 (refs: add REF_FORCE_CREATE_REFLOG flag,
2015-07-21) ...
 

[...]
-static int files_create_reflog(struct ref_store *ref_store,
-			       const char *refname, int force_create,
+static int files_create_reflog(struct ref_store *ref_store, const char *refname,
 			       struct strbuf *err)
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE, "create_reflog");
 	int fd;
 
-	if (log_ref_setup(refs, refname, force_create, &fd, err))
+	if (log_ref_setup(refs, refname, /*force_create=*/1, &fd, err))

We can lose the inline comment here & let the function definition speak
for itself, we usually don't inline comment boolean flags.

In any case, having not dug (but presumably you have) some overview of
how we ended up having this not-required flag would be nice. I.e. was it
always this dead-end, or did we replace it with REF_FORCE_CREATE_REFLOG
at some point etc?

On Mon, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote:

In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,

Nit: use <message-id> for quoting, not message-id.

we came to the conclusion that this feature is probably a remnant from
the time that reflogs weren't enabled by default, and it does not need
to be kept.

Maybe some summary of the flexibily either Jeff King or Junio mentioned
we were losing (i.e. we can't selectively enable per-ref now), but that
we think it's OK because...

For the implementation:

+	*logfd = -1;

Weird, more on this later...

+	if (!force_create && !should_autocreate_reflog(refname))
+		return 0;

OK, so we can early abort.

 	files_reflog_path(refs, &logfile_sb, refname);
 	logfile = strbuf_detach(&logfile_sb, NULL);
 
-	if (force_create || should_autocreate_reflog(refname)) {
-		if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
-			if (errno == ENOENT)
-				strbuf_addf(err, "unable to create directory for '%s': "
-					    "%s", logfile, strerror(errno));

Here we use one indent/wrapping style...

-			else if (errno == EISDIR)
-				strbuf_addf(err, "there are still logs under '%s'",
-					    logfile);
-			else
-				strbuf_addf(err, "unable to append to '%s': %s",
-					    logfile, strerror(errno));
-
-			goto error;
-		}
-	} else {
-		*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
-		if (*logfd < 0) {
-			if (errno == ENOENT || errno == EISDIR) {
-				/*
-				 * The logfile doesn't already exist,
-				 * but that is not an error; it only
-				 * means that we won't write log
-				 * entries to it.
-				 */
-				;
-			} else {
-				strbuf_addf(err, "unable to append to '%s': %s",
-					    logfile, strerror(errno));
-				goto error;
-			}
-		}
+	if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
+		if (errno == ENOENT)
+			strbuf_addf(err,
+				    "unable to create directory for '%s': "
+				    "%s",
+				    logfile, strerror(errno));

...but here it's changed while we're at it, this patch would be easier
to follow IMO if we just left the formatting alone (or did it as another
step). I'm aware that it ends us at over 79 columns, but that was the
case before...

+		else if (errno == EISDIR)
+			strbuf_addf(err, "there are still logs under '%s'",
+				    logfile);
+		else
+			strbuf_addf(err, "unable to append to '%s': %s",
+				    logfile, strerror(errno));
 	}
 
 	if (*logfd >= 0)
 		adjust_shared_perm(logfile);
 
 	free(logfile);
-	return 0;
-
-error:
-	free(logfile);
-	return -1;
+	return (*logfd < 0) ? -1 : 0;

On "more on this later": Since we just return -1, 0 or a valid fd now,
can't we just return the "fd" here and let the callers sort out -1, 0
and >0?

On Tue, Sep 7, 2021 at 12:35 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

-     printf("%s %s %s %"PRItime" %d %s\n",
-            oid_to_hex(old_oid), oid_to_hex(new_oid),
-            committer, timestamp, tz, msg);
+     printf("%s %s %s %" PRItime " %+05d\t%s\n", oid_to_hex(old_oid),
+            oid_to_hex(new_oid), committer, timestamp, tz, msg);

Nit: Would be a more readable diff if this wasn't a
line-wrap-while-at-it change in addition to changing the format string.

I.e. the last 4x parameters aren't changed, so leaving them on their own
line & just changing the string & the two oid_to_hex()...

This is clang-format's output, and the new code takes up less lines vertically.

If you think this is really, really important, I can change it, but I
think it's a better use of everyone's time to leave mechanical tasks
(like formatting) to the machines.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

<As discussed in
CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com

v3:

 * fix show-branch
 * add some more context to commit messages
 * change calling convention for log_ref_setup; could fold into predecessor
   if needed too.

Han-Wen Nienhuys (7):
  show-branch: show reflog message
  refs: trim newline from reflog message
  test-ref-store: tweaks to for-each-reflog-ent format
  t1400: use test-helper ref-store to inspect reflog contents
  refs: drop force_create argument of create_reflog API
  RFC: refs: reflog entries aren't written based on reflog existence.
  refs: change log_ref_setup calling convention

 builtin/checkout.c             |   2 +-
 builtin/show-branch.c          |   7 +-
 reflog-walk.c                  |   6 +-
 refs.c                         |   9 ++-
 refs.h                         |   4 +-
 refs/debug.c                   |   5 +-
 refs/files-backend.c           | 128 +++++++++++++--------------------
 refs/packed-backend.c          |   3 +-
 refs/refs-internal.h           |   2 +-
 t/helper/test-ref-store.c      |   8 +--
 t/t1400-update-ref.sh          |  21 +++---
 t/t1405-main-ref-store.sh      |   6 +-
 t/t1406-submodule-ref-store.sh |   6 +-
 t/t3202-show-branch.sh         |  15 ++++
 14 files changed, 101 insertions(+), 121 deletions(-)


base-commit: e0a2f5cbc585657e757385ad918f167f519cfb96
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1067%2Fhanwen%2Freflog-touch-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1067/hanwen/reflog-touch-v3
Pull-Request: https://github.com/git/git/pull/1067

Range-diff vs v2:

 -:  ----------- > 1:  e158882812f show-branch: show reflog message
 1:  995d450da42 ! 2:  d16d94164c1 refs: trim newline from reflog message
     @@ Commit message
      
       ## builtin/show-branch.c ##
      @@ builtin/show-branch.c: int cmd_show_branch(int ac, const char **av, const char *prefix)
     - 				show_one_commit(rev[i], 1);
     + 			char *logmsg;
     + 			char *nth_desc;
     + 			const char *msg;
     +-			char *end;
     + 			timestamp_t timestamp;
     + 			int tz;
     + 
     +@@ builtin/show-branch.c: int cmd_show_branch(int ac, const char **av, const char *prefix)
     + 				break;
       			}
     - 			else
     --				puts(reflog_msg[i]);
     -+				puts(reflog_msg[i]); /* XXX - this puts a
     -+							newline. Did we put two
     -+							newlines beforehand? */
       
     - 			if (is_head)
     - 				head_at = i;
     +-			end = strchr(logmsg, '\n');
     +-			if (end)
     +-				*end = '\0';
     +-
     + 			msg = (*logmsg == '\0') ? "(none)" : logmsg;
     + 			reflog_msg[i] = xstrfmt("(%s) %s",
     + 						show_date(timestamp, tz,
      
       ## reflog-walk.c ##
      @@ reflog-walk.c: void get_reflog_message(struct strbuf *sb,
 2:  11b296a55e9 = 3:  e273963216c test-ref-store: tweaks to for-each-reflog-ent format
 3:  9ec09cc64cd = 4:  52093fce57c t1400: use test-helper ref-store to inspect reflog contents
 4:  aa25fd9b7de ! 5:  ce0047028dd refs: drop force_create argument of create_reflog API
     @@ Commit message
          There is only one caller, builtin/checkout.c, and it hardcodes
          force_create=1.
      
     +    This argument was introduced in abd0cd3a301 (refs: new public ref function:
     +    safe_create_reflog, 2015-07-21), which promised to immediately use it in a
     +    follow-on commit, but that never happened.
     +
          Signed-off-by: Han-Wen Nienhuys [off-list ref]
      
       ## builtin/checkout.c ##
     @@ refs/files-backend.c: error:
       	int fd;
       
      -	if (log_ref_setup(refs, refname, force_create, &fd, err))
     -+	if (log_ref_setup(refs, refname, /*force_create=*/1, &fd, err))
     ++	if (log_ref_setup(refs, refname, 1, &fd, err))
       		return -1;
       
       	if (fd >= 0)
 5:  f6a7c5ad56e ! 6:  7a030cfd3e2 RFC: refs: reflog entries aren't written based on reflog existence.
     @@ Commit message
          The reftable storage backend cannot distinguish between a non-existing
          reflog, and an empty one. Therefore it cannot mimick this functionality.
      
     -    In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,
     -    we came to the conclusion that this feature is probably a remnant from
     -    the time that reflogs weren't enabled by default, and it does not need
     -    to be kept.
     +    With this feature, it is possible to mark only specific branches as subject to
     +    reflog updates. When introduced, it presumably served as a cheap substitute for
     +    introducing branch.$NAME.logRefUpdate configuration setting.
     +
     +    Reflogs are small and don't impact the runtime of normal operations, so this
     +    flexibility is not very useful. Since it incurs complexity for alternate ref
     +    backends, we remove it.
     +
     +    Further background to this change is in
     +    [off-list ref].
      
          Signed-off-by: Han-Wen Nienhuys [off-list ref]
      
 -:  ----------- > 7:  1124dbad594 refs: change log_ref_setup calling convention

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

Before, --reflog option would look for '\t' in the reflog message. As refs.c
already parses the reflog line, the '\t' was never found, and show-branch
--reflog would always say "(none)" as reflog message

Add test.

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 builtin/show-branch.c  | 12 +++++++-----
 t/t3202-show-branch.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d77ce7aeb38..902a0d99850 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c

@@ -762,6 +762,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			char *logmsg;
 			char *nth_desc;
 			const char *msg;
+			char *end;
 			timestamp_t timestamp;
 			int tz;
 

@@ -771,11 +772,12 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				reflog = i;
 				break;
 			}
-			msg = strchr(logmsg, '\t');
-			if (!msg)
-				msg = "(none)";
-			else
-				msg++;
+
+			end = strchr(logmsg, '\n');
+			if (end)
+				*end = '\0';
+
+			msg = (*logmsg == '\0') ? "(none)" : logmsg;
 			reflog_msg[i] = xstrfmt("(%s) %s",
 						show_date(timestamp, tz,
 							  DATE_MODE(RELATIVE)),

diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index ad9902a06b9..d4d64401e4b 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh

@@ -4,6 +4,9 @@ test_description='test show-branch'
 
 . ./test-lib.sh
 
+# arbitrary reference time: 2009-08-30 19:20:00
+GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
+
 test_expect_success 'setup' '
 	test_commit initial &&
 	for i in $(test_seq 1 10)

@@ -146,4 +149,16 @@ test_expect_success 'show branch --merge-base with N arguments' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show branch --reflog=2' '
+	sed "s/^>	//" >expect <<-\EOF &&
+	>	! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10
+	>	 ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10
+	>	--
+	>	+  [refs/heads/branch10@{0}] branch10
+	>	++ [refs/heads/branch10@{1}] initial
+	EOF
+	git show-branch --reflog=2 >actual &&
+	test_cmp actual expect
+'
+
 test_done

-- 
gitgitgadget

From: Han-Wen Nienhuys <redacted>

There is only one caller, builtin/checkout.c, and it hardcodes
force_create=1.

This argument was introduced in abd0cd3a301 (refs: new public ref function:
safe_create_reflog, 2015-07-21), which promised to immediately use it in a
follow-on commit, but that never happened.

Signed-off-by: Han-Wen Nienhuys <redacted>
---
 builtin/checkout.c             | 2 +-
 refs.c                         | 9 ++++-----
 refs.h                         | 4 ++--
 refs/debug.c                   | 5 ++---
 refs/files-backend.c           | 5 ++---
 refs/packed-backend.c          | 3 +--
 refs/refs-internal.h           | 2 +-
 t/helper/test-ref-store.c      | 3 +--
 t/t1405-main-ref-store.sh      | 2 +-
 t/t1406-submodule-ref-store.sh | 2 +-
 10 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index b5d477919a7..3c6506e0595 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -876,7 +876,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				int ret;
 				struct strbuf err = STRBUF_INIT;
 
-				ret = safe_create_reflog(refname, 1, &err);
+				ret = safe_create_reflog(refname, &err);
 				if (ret) {
 					fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
 						opts->new_orphan_branch, err.buf);

diff --git a/refs.c b/refs.c
index 8b9f7c3a80a..ca59a7cc652 100644
--- a/refs.c
+++ b/refs.c

@@ -2347,16 +2347,15 @@ int reflog_exists(const char *refname)
 }
 
 int refs_create_reflog(struct ref_store *refs, const char *refname,
-		       int force_create, struct strbuf *err)
+		       struct strbuf *err)
 {
-	return refs->be->create_reflog(refs, refname, force_create, err);
+	return refs->be->create_reflog(refs, refname, err);
 }
 
-int safe_create_reflog(const char *refname, int force_create,
-		       struct strbuf *err)
+int safe_create_reflog(const char *refname, struct strbuf *err)
 {
 	return refs_create_reflog(get_main_ref_store(the_repository), refname,
-				  force_create, err);
+				  err);
 }
 
 int refs_delete_reflog(struct ref_store *refs, const char *refname)