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

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

[Han-Wen Nienhuys via GitGitGadget]

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

[PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 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

[PATCH 2/4] t1400: use test-helper ref-store to inspect reflog contents

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 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

[PATCH 3/4] refs: drop force_create argument of create_reflog API

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 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

[PATCH 4/4] RFC: refs: reflog entries aren't written based on reflog existence.

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 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

Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format

[Taylor Blau]

On Mon, Aug 30, 2021 at 02:48:45PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
> 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 &&

Nit; stray debugging.

Thanks,
Taylor

Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format

[Taylor Blau]

On Mon, Aug 30, 2021 at 02:48:45PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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 <hanwen@google.com>
> ---
>  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);

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

Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format

[Junio C Hamano]

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Follow the reflog format more closely, so it can be used for comparing
> reflogs in tests without using inspecting files under .git/logs/

I find the above written in clear language that conveys what it
wants to say unclearly X-<.

I am guessing, from the patch text, that the callback function to
refs_for_each_reflog_ent() and its _reverse() variant takes a "msg"
parameter, which is expected to include a terminating LF at its end,
but some backends omits the LF, and this change is to work around it
by conditionally adding (or refraining from adding) LF in the
each_reflog() function that is used for testing.

Is that what is going on?  Or is it "even within a single backend,
sometimes we get message with terminating LF and sometimes without"?

In any case, it changes the output.  We used to spend two output
lines per record in tests for these two functions, but now we assume
we get one each line per one reflog entry.  Which may be OK as far
as these tests are concerned, but does this leave problems with the
real world callbacks?  Do the real callers to for_each_reflog_ent()
and for_each_reflog_ent_reverse() also now need to cater to this "we
sometimes get LF at the end, we sometimes don't" differences by
having a similar logic as we see in the updated each_reflog() in
this patch?

Shouldn't we be instead making sure that the callback functions get
the msg argument in a consistent way, whether it is with or without
terminating LF, instead of forcing the callers to cope with the
differences like this each_reflog() function does in this patch?

Thanks.

Re: [PATCH 2/4] t1400: use test-helper ref-store to inspect reflog contents

[Junio C Hamano]

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

Nice.  So the file backend may omit the trailing tab for an empty
message at the storage level, but the test-tool consistently gives
the same format for entries with empty and non-empty messages, and
that is shown in the above.  Makes sense.

> +	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
>  '

Ditto.

Looks good to me.

Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format

[Taylor Blau]

On Mon, Aug 30, 2021 at 01:51:56PM -0700, Junio C Hamano wrote:
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > Follow the reflog format more closely, so it can be used for comparing
> > reflogs in tests without using inspecting files under .git/logs/
>
> I find the above written in clear language that conveys what it
> wants to say unclearly X-<.

I agree; there is unfortunately more than meets the eye for this patch,
so untangling it all took me a little while.

> [...]
> Is that what is going on?  Or is it "even within a single backend,
> sometimes we get message with terminating LF and sometimes without"?

Is exactly what I was wondering about in [1]. And...

> Shouldn't we be instead making sure that the callback functions get
> the msg argument in a consistent way, whether it is with or without
> terminating LF, instead of forcing the callers to cope with the
> differences like this each_reflog() function does in this patch?

...that is a much more sensible resolution than what I saw in this
patch. (At the very least, strchr is confusing since we don't expect LF
characters anywhere in the reflog message, and this is really dealing
with how the callback is invoked, not the actual bytes-on-disk).

I would probably prefer that the callback be standardized to either
always or never give a trailing LF.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/YS0+MHk3svGr7d4n@nand.local/

Re: [PATCH 3/4] refs: drop force_create argument of create_reflog API

[Junio C Hamano]

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> There is only one caller, builtin/checkout.c, and it hardcodes
> force_create=1.

This dates back to abd0cd3a (refs: new public ref function:
safe_create_reflog, 2015-07-21) and it seems that we never acquired
the second caller to this helper.

Makes sense.

Re: [PATCH 4/4] RFC: refs: reflog entries aren't written based on reflog existence.

[Taylor Blau]

On Mon, Aug 30, 2021 at 02:48:48PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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.

> 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 <hanwen@google.com>
> ---
>  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)

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

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

Re: [PATCH 4/4] RFC: refs: reflog entries aren't written based on reflog existence.

[Junio C Hamano]

"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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 <hanwen@google.com>
> ---
>  refs/files-backend.c  | 20 +-------------------
>  t/t1400-update-ref.sh |  7 +++----
>  2 files changed, 4 insertions(+), 23 deletions(-)

While I like the loss of lines and reduced complexity...

One potential harm this change will bring to us is what happens to
people who disable core.logAllRefUpdates manually after using the
repository for a while.  Their @{4} will point at the same commit no
matter how many operations are done on the current branch after they
do so.  I wouldn't mind if "git reflog disable" command is given to
the users prominently and core.logAllRefUpdates becomes a mere
implementation detail nobody has to care about---in such a world, we
could set the configuration and drop the existing reflog records at
the same time and nobody will be hurt.

If we keep the current behaviour, what are we harming instead?
Growth of diskspace usage is an obvious one, but disks are cheaper
compared to human brainwave cycles cost.

> 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

Moving the definition of TAB= around in the test is not all that
welcome.  If anything, I'd suggest doing so nearer to the beginning
of the test sequence, before the first test that uses the ref-store
test-tool, because it is just like $GIT_COMMITTER_EMAIL and $RUN
that define constants used throughout the remainder of the test
script.

THanks.

Re: [PATCH 1/4] test-ref-store: tweaks to for-each-reflog-ent format

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>> Shouldn't we be instead making sure that the callback functions get
>> the msg argument in a consistent way, whether it is with or without
>> terminating LF, instead of forcing the callers to cope with the
>> differences like this each_reflog() function does in this patch?
>
> ...that is a much more sensible resolution than what I saw in this
> patch. (At the very least, strchr is confusing since we don't expect LF
> characters anywhere in the reflog message, and this is really dealing
> with how the callback is invoked, not the actual bytes-on-disk).
>
> I would probably prefer that the callback be standardized to either
> always or never give a trailing LF.

True, and I agree that the use of LF by on-disk reflog storage
format is just as the record terminator, so I do not mind if we
decide to standardize on a single incomplete line without LF.

Thanks.

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

[Han-Wen Nienhuys via GitGitGadget]

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 <hanwen@google.com>
      
       ## 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 <hanwen@google.com>
      
     - ## 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 <hanwen@google.com>
      
       ## 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

[PATCH v2 1/5] refs: trim newline from reflog message

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 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

[PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 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

[PATCH v2 3/5] t1400: use test-helper ref-store to inspect reflog contents

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 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

[PATCH v2 4/5] refs: drop force_create argument of create_reflog API

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 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

[PATCH v2 5/5] RFC: refs: reflog entries aren't written based on reflog existence.

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 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

Re: [PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format

[Ævar Arnfjörð Bjarmason]

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

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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 <hanwen@google.com>
> ---
>  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);

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()...

Re: [PATCH v2 1/5] refs: trim newline from reflog message

[Ævar Arnfjörð Bjarmason]

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

> From: Han-Wen Nienhuys <hanwen@google.com>
> [...]
> -				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.

Re: [PATCH v2 4/5] refs: drop force_create argument of create_reflog API

[Ævar Arnfjörð Bjarmason]

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

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> 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?

Re: [PATCH v2 5/5] RFC: refs: reflog entries aren't written based on reflog existence.

[Ævar Arnfjörð Bjarmason]

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?

Re: [PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format

[Han-Wen Nienhuys]

On Tue, Sep 7, 2021 at 12:35 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > -     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

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

[Han-Wen Nienhuys via GitGitGadget]

<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 <hanwen@google.com>
      
       ## 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
     +    <CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com>.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
 -:  ----------- > 7:  1124dbad594 refs: change log_ref_setup calling convention

-- 
gitgitgadget

[PATCH v3 1/7] show-branch: show reflog message

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 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

[PATCH v3 2/7] refs: trim newline from reflog message

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 builtin/show-branch.c          |  5 -----
 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, 21 insertions(+), 29 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 902a0d99850..58a894e81e2 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -762,7 +762,6 @@ 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;
 
@@ -773,10 +772,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				break;
 			}
 
-			end = strchr(logmsg, '\n');
-			if (end)
-				*end = '\0';
-
 			msg = (*logmsg == '\0') ? "(none)" : logmsg;
 			reflog_msg[i] = xstrfmt("(%s) %s",
 						show_date(timestamp, tz,
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

[PATCH v3 3/7] test-ref-store: tweaks to for-each-reflog-ent format

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 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

[PATCH v3 4/7] t1400: use test-helper ref-store to inspect reflog contents

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 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

[PATCH v3 5/7] refs: drop force_create argument of create_reflog API

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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 <hanwen@google.com>
---
 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..04d73b89c9c 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, 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

[PATCH v3 6/7] RFC: refs: reflog entries aren't written based on reflog existence.

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

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.

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
<CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com>.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 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 04d73b89c9c..4aa4d2bbba1 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

[PATCH v3 7/7] refs: change log_ref_setup calling convention

[Han-Wen Nienhuys via GitGitGadget]

From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/files-backend.c | 48 ++++++++++++++++++--------------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4aa4d2bbba1..114d01a9e3c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1535,30 +1535,26 @@ static int open_or_create_logfile(const char *path, void *cb)
 }
 
 /*
- * Create a reflog for a ref. If force_create = 0, only create the
- * reflog for certain refs (those for which should_autocreate_reflog
- * returns non-zero). Otherwise, create it regardless of the reference
- * name. If the logfile already existed or was created, return 0 and
- * set *logfd to the file descriptor opened for appending to the file.
- * If no logfile exists and we decided not to create one, return 0 and
- * set *logfd to -1. On failure, fill in *err, set *logfd to -1, and
- * return -1.
+ * Create a reflog for a ref. If force_create = 0, only create the reflog for
+ * certain refs (those for which should_autocreate_reflog returns non-zero).
+ * Otherwise, create it regardless of the reference name. On success, return the
+ * filedescriptor. If the log should not be written, return 0 On error, return
+ * -1 and fill in *err.
  */
-static int log_ref_setup(struct files_ref_store *refs,
-			 const char *refname, int force_create,
-			 int *logfd, struct strbuf *err)
+static int log_ref_setup(struct files_ref_store *refs, const char *refname,
+			 int force_create, struct strbuf *err)
 {
 	struct strbuf logfile_sb = STRBUF_INIT;
 	char *logfile;
 
-	*logfd = -1;
+	int 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 (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
+	if (raceproof_create_file(logfile, open_or_create_logfile, &logfd)) {
 		if (errno == ENOENT)
 			strbuf_addf(err,
 				    "unable to create directory for '%s': "
@@ -1572,11 +1568,11 @@ static int log_ref_setup(struct files_ref_store *refs,
 				    logfile, strerror(errno));
 	}
 
-	if (*logfd >= 0)
+	if (logfd > 0)
 		adjust_shared_perm(logfile);
 
 	free(logfile);
-	return (*logfd < 0) ? -1 : 0;
+	return logfd;
 }
 
 static int files_create_reflog(struct ref_store *ref_store, const char *refname,
@@ -1584,15 +1580,14 @@ static int files_create_reflog(struct ref_store *ref_store, const char *refname,
 {
 	struct files_ref_store *refs =
 		files_downcast(ref_store, REF_STORE_WRITE, "create_reflog");
-	int fd;
-
-	if (log_ref_setup(refs, refname, 1, &fd, err))
-		return -1;
+	int fd = log_ref_setup(refs, refname, 1, err);
 
-	if (fd >= 0)
+	if (fd > 0) {
 		close(fd);
+		return 0;
+	}
 
-	return 0;
+	return fd;
 }
 
 static int log_ref_write_fd(int fd, const struct object_id *old_oid,
@@ -1624,15 +1619,12 @@ static int files_log_ref_write(struct files_ref_store *refs,
 	if (log_all_ref_updates == LOG_REFS_UNSET)
 		log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;
 
-	result = log_ref_setup(refs, refname,
-			       flags & REF_FORCE_CREATE_REFLOG,
-			       &logfd, err);
+	logfd = log_ref_setup(refs, refname, flags & REF_FORCE_CREATE_REFLOG,
+			      err);
 
-	if (result)
-		return result;
+	if (logfd <= 0)
+		return logfd;
 
-	if (logfd < 0)
-		return 0;
 	result = log_ref_write_fd(logfd, old_oid, new_oid,
 				  git_committer_info(0), msg);
 	if (result) {
-- 
gitgitgadget

Re: [PATCH v2 2/5] test-ref-store: tweaks to for-each-reflog-ent format

[Ævar Arnfjörð Bjarmason]

On Tue, Sep 07 2021, Han-Wen Nienhuys wrote:

> On Tue, Sep 7, 2021 at 12:35 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> > -     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.

It's not "really, really important" or even "really important", just
notes while reading the series.

Patches are read N times, including during review. Since humans read
formatting changes, it's in general are not something we can leave to
machines. We're after all looking at the patch on-list, not diffing two
versions of the generated machine code, or of clang-format's output.

I don't think the intent of the clang-format target is to run it before
patch submission, but to serve as input on suggested formatting changes.

If on master you run:

    git ls-files '*.[ch]' -z | xargs -n 1 -0 clang-format -i

You'll get:

    631 files changed, 48246 insertions(+), 45061 deletions(-)

Which I think speaks for itself in the likelyhood that
"git-clang-format" is going to inject unnecessary churn into submitted
patches.

I think this case is quite small and not worth a re-roll.

Re: [PATCH v3 0/7] Gets rid of "if reflog exists, append to it regardless of config settings"

[Han-Wen Nienhuys]

On Tue, Sep 7, 2021 at 3:37 PM Han-Wen Nienhuys via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> <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.

Hi Junio,

I had the impression that I addressed all outstanding comments (but
not sure). Are you waiting for me to do something before this can go
into 'seen' ?

There is a merge conflict against master, so I'll send a v4 shortly.

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

Re: [PATCH v3 0/7] Gets rid of "if reflog exists, append to it regardless of config settings"

[Junio C Hamano]

Han-Wen Nienhuys <hanwen@google.com> writes:

> I had the impression that I addressed all outstanding comments (but
> not sure). Are you waiting for me to do something before this can go
> into 'seen' ?
>
> There is a merge conflict against master, so I'll send a v4 shortly.

Sorry.  I seem to have looked at and commented on the precursor RFC
of this topic, but nobody other than Ævar seems to have commented on
the second iteration and the topic was completely under my radar,
and I do not remember what it was about.

It would be good to have an update for others to see.

Thanks.

Re: [PATCH v3 0/7] Gets rid of "if reflog exists, append to it regardless of config settings"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Han-Wen Nienhuys <hanwen@google.com> writes:
>
>> I had the impression that I addressed all outstanding comments (but
>> not sure). Are you waiting for me to do something before this can go
>> into 'seen' ?
>>
>> There is a merge conflict against master, so I'll send a v4 shortly.
>
> Sorry.  I seem to have looked at and commented on the precursor RFC
> of this topic, but nobody other than Ævar seems to have commented on
> the second iteration and the topic was completely under my radar,
> and I do not remember what it was about.
>
> It would be good to have an update for others to see.
>
> Thanks.

Not really.  I think the comment on the RFC still stands, and I do
not recall seeing a response to the point.

    One potential harm this change will bring to us is what happens to
    people who disable core.logAllRefUpdates manually after using the
    repository for a while.  Their @{4} will point at the same commit no
    matter how many operations are done on the current branch after they
    do so.  I wouldn't mind if "git reflog disable" command is given to
    the users prominently and core.logAllRefUpdates becomes a mere
    implementation detail nobody has to care about---in such a world, we
    could set the configuration and drop the existing reflog records at
    the same time and nobody will be hurt.

    If we keep the current behaviour, what are we harming instead?
    Growth of diskspace usage is an obvious one, but disks are cheaper
    compared to human brainwave cycles cost.

As it is about the basic design of the "feature" (or misfeature), no
matter how improved the internal implementation details get, I am
skeptical how it is solved (other than "immediately when we notice
core.logAllRefUpdates is disabled, remove all the existing reflog
entries to avoid confusion, in addition to stop appending any more
reflog entries", that is).

Re: [PATCH v3 0/7] Gets rid of "if reflog exists, append to it regardless of config settings"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Not really.  I think the comment on the RFC still stands, and I do
> not recall seeing a response to the point.
>
>     One potential harm this change will bring to us is what happens to
>     people who disable core.logAllRefUpdates manually after using the
>     repository for a while.  Their @{4} will point at the same commit no
>     matter how many operations are done on the current branch after they
>     do so.  I wouldn't mind if "git reflog disable" command is given to
>     the users prominently and core.logAllRefUpdates becomes a mere
>     implementation detail nobody has to care about---in such a world, we
>     could set the configuration and drop the existing reflog records at
>     the same time and nobody will be hurt.
>
>     If we keep the current behaviour, what are we harming instead?
>     Growth of diskspace usage is an obvious one, but disks are cheaper
>     compared to human brainwave cycles cost.

IIRC, the only reason why reftable implementation may want to change
the behaviour we have to avoid getting blamed for breaking is
because it cannot implement "a reflog exists, and we need to record
further ref movements by appending to it, no matter what the
configuration says" when the existing reflog is empty, because its
data structure lacks support for expressing "exists but empty".

I think the behaviour change described in the title of this message
can be limited in the scope to hurt users a lot less, and can still
satisfy the goal of helping reftable not getting blamed for
breakage, perhaps by making the behaviour for an empty but existing
reflog unspecified or implementation defined per backend.

That would allow us to avoid situation where a user can say foo@{1}
but it does not refer to the point where the branch's tip pointed
just before the most recent update to it. As an empty but existing
reflog would not give foo@{$n} for any value of $n, there is much
less risk of confusing users if we did not append new entries to it,
I would hope.

Re: [PATCH v3 0/7] Gets rid of "if reflog exists, append to it regardless of config settings"

[Han-Wen Nienhuys]

On Thu, Oct 7, 2021 at 7:38 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Not really.  I think the comment on the RFC still stands, and I do
> > not recall seeing a response to the point.
> >
> >     One potential harm this change will bring to us is what happens to
> >     people who disable core.logAllRefUpdates manually after using the
> >     repository for a while.  Their @{4} will point at the same commit no
> >     matter how many operations are done on the current branch after they
> >     do so.  I wouldn't mind if "git reflog disable" command is given to
> >     the users prominently and core.logAllRefUpdates becomes a mere
> >     implementation detail nobody has to care about---in such a world, we
> >     could set the configuration and drop the existing reflog records at
> >     the same time and nobody will be hurt.

A git 'reflog disable' command would address your concerns, but it is
a destructive operation, so the cure might be worse than the solution.

> IIRC, the only reason why reftable implementation may want to change
> the behaviour we have to avoid getting blamed for breaking is
> because it cannot implement "a reflog exists, and we need to record
> further ref movements by appending to it, no matter what the
> configuration says" when the existing reflog is empty, because its
> data structure lacks support for expressing "exists but empty".
>
> I think the behaviour change described in the title of this message
> can be limited in the scope to hurt users a lot less, and can still
> satisfy the goal of helping reftable not getting blamed for
> breakage, perhaps by making the behaviour for an empty but existing
> reflog unspecified or implementation defined per backend.

If we accept implementation-dependent features, we could just leave
the whole feature as is. I had expected more breakage, but there is
only one test case in t1400 that needs addressing. If the test
coverage reflects the popularity of the feature, it should be fine to
leave this divergence in, and mark the test with REFFILES.

The commits prior to the RFC should be OK for committing. In
particular, there is a bugfix for the show-branch command. Should I
resend those separately?

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

Re: [PATCH v3 0/7] Gets rid of "if reflog exists, append to it regardless of config settings"

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 11 2021, Han-Wen Nienhuys wrote:

> On Thu, Oct 7, 2021 at 7:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>> > Not really.  I think the comment on the RFC still stands, and I do
>> > not recall seeing a response to the point.
>> >
>> >     One potential harm this change will bring to us is what happens to
>> >     people who disable core.logAllRefUpdates manually after using the
>> >     repository for a while.  Their @{4} will point at the same commit no
>> >     matter how many operations are done on the current branch after they
>> >     do so.  I wouldn't mind if "git reflog disable" command is given to
>> >     the users prominently and core.logAllRefUpdates becomes a mere
>> >     implementation detail nobody has to care about---in such a world, we
>> >     could set the configuration and drop the existing reflog records at
>> >     the same time and nobody will be hurt.
>
> A git 'reflog disable' command would address your concerns, but it is
> a destructive operation, so the cure might be worse than the solution.
>
>> IIRC, the only reason why reftable implementation may want to change
>> the behaviour we have to avoid getting blamed for breaking is
>> because it cannot implement "a reflog exists, and we need to record
>> further ref movements by appending to it, no matter what the
>> configuration says" when the existing reflog is empty, because its
>> data structure lacks support for expressing "exists but empty".
>>
>> I think the behaviour change described in the title of this message
>> can be limited in the scope to hurt users a lot less, and can still
>> satisfy the goal of helping reftable not getting blamed for
>> breakage, perhaps by making the behaviour for an empty but existing
>> reflog unspecified or implementation defined per backend.
>
> If we accept implementation-dependent features, we could just leave
> the whole feature as is. I had expected more breakage, but there is
> only one test case in t1400 that needs addressing. If the test
> coverage reflects the popularity of the feature, it should be fine to
> leave this divergence in, and mark the test with REFFILES.
>
> The commits prior to the RFC should be OK for committing. In
> particular, there is a bugfix for the show-branch command. Should I
> resend those separately?

I've got some follow-up patches to what's sitting in "next" already that
hoist some reffiles-specific stuff into builtin/reflog.c, I haven't
tested but I expect that the behavior change is silent now in the
reftable backend, i.e. it doesn't implement progress/verbose the same
way, presumably.

Between that and 5ac15ad2509 (reflog tests: add --updateref tests,
2021-10-16) & 52106430dc8 (refs/files: remove "name exist?" check in
lock_ref_oid_basic(), 2021-10-16) I wouldn't put too much faith in those
reflog tests.

None of that should be a blocker for your series landing, just say'n. I
don't trust those tests.

IMO the only meaningful way to be confident in testing these sorts of
things with reftable is more of the chaos monkey approach of the
GIT_TEST_* modes, i.e. we now have a WIP mode to do that for reftable
that has known breakages.

We could similarly instrument the test suite to do "git reflog expire"
for each ref at the end of tests, a bunch of things would break, but we
could log the complete -V run and see if what breaks is different under
the two backends.

I've got some WIP patches to add a similar chaos mode using "git gc
--auto", and it turned up some interesting stuff. It's what I used
initially to test what's now landed in ae35e16cd43 (reflog expire: don't
lock reflogs using previously seen OID, 2021-08-23).