[PATCH 0/4] Inspect reflog data programmatically in more tests

[PATCH 0/4] Inspect reflog data programmatically in more tests

[Han-Wen Nienhuys via GitGitGadget]

This helps for reftable support, and will help if we want to reconsider
under which conditions reflogs get created/updated.

Han-Wen Nienhuys (4):
  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

 builtin/show-branch.c          |  7 ++-----
 reflog-walk.c                  |  6 ++----
 refs/files-backend.c           | 30 +++++++++++++++---------------
 t/helper/test-ref-store.c      |  5 ++---
 t/t1400-update-ref.sh          | 18 +++++++++++-------
 t/t1405-main-ref-store.sh      |  4 ++--
 t/t1406-submodule-ref-store.sh |  4 ++--
 t/t3202-show-branch.sh         | 15 +++++++++++++++
 8 files changed, 51 insertions(+), 38 deletions(-)


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

[PATCH 1/4] 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 082449293b5..f1e8318592c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -761,6 +761,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;
 
@@ -770,11 +771,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

Re: [PATCH 1/4] show-branch: show reflog message

[Junio C Hamano]

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

> 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

Well spotted.  It may show that nobody pays attention to output from
"show-branch -g" (or nobody runs it in the first place), but it is
good to fix it anyway.

Thanks.

>
> 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 082449293b5..f1e8318592c 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -761,6 +761,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;
>  
> @@ -770,11 +771,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

Re: [PATCH 1/4] show-branch: show reflog message

[Bagas Sanjaya]

On 22/11/21 21.20, Han-Wen Nienhuys via GitGitGadget wrote:
> 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.
> 

Add what the test?

> +# arbitrary reference time: 2009-08-30 19:20:00
> +GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
> +

Why don't you use oneliner `export GIT_TEST_DATE_NOW=1251660000`?

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH 1/4] show-branch: show reflog message

[Elijah Newren]

On Mon, Nov 22, 2021 at 11:44 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> On 22/11/21 21.20, Han-Wen Nienhuys via GitGitGadget wrote:
> > 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.
> >
>
> Add what the test?
>
> > +# arbitrary reference time: 2009-08-30 19:20:00
> > +GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
> > +
>
> Why don't you use oneliner `export GIT_TEST_DATE_NOW=1251660000`?

t/check-non-portable-shell.pl will flag this construct and throw an
error if you put that in your test script.

[PATCH 2/4] 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 f1e8318592c..016ccdafd0f 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -761,7 +761,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;
 
@@ -772,10 +771,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 8ac4b284b6b..3ee59a98d2f 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -244,8 +244,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);
 }
 
@@ -284,10 +282,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 151b0056fe5..583bbc5f8b9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1936,17 +1936,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);
@@ -2038,6 +2036,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)
@@ -2050,6 +2049,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;
@@ -2099,7 +2099,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);
@@ -3059,18 +3059,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

Re: [PATCH 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

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

If this were truly "user-provided", then I'd argue that all backends
should follow whatever the files backend has been doing forever---if
the files added LF implicitly, others should, too, because that is
pretty much what these "user-provided" callbacks have been expecting
to see.

In other words, it would be a bug for newer backends to behave
differently.

But I _think_ these callbacks are all under our control, and if that
is the case, I am fine either way, even though I would have a strong
preference not to have to change the API without a good reason, even
if it is a purely internal one.

So, let's go and see if we can find a good reason in the changes we
can make to the callback functions ;-)

> -			end = strchr(logmsg, '\n');
> -			if (end)
> -				*end = '\0';
> -

We could argue that the lack of LF at the end from the API output
made this caller simpler, which may be a plus.

> diff --git a/reflog-walk.c b/reflog-walk.c
> index 8ac4b284b6b..3ee59a98d2f 100644
> --- a/reflog-walk.c
> +++ b/reflog-walk.c
> @@ -244,8 +244,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 */

Likewise.

> @@ -284,10 +282,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);
>  		}

This is Meh either way.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 151b0056fe5..583bbc5f8b9 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1936,17 +1936,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);
> @@ -2038,6 +2036,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)
> @@ -2050,6 +2049,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;
> @@ -2099,7 +2099,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);
> @@ -3059,18 +3059,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;
>  }

Hmmmm.  I'll defer my judgment to the end.

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

I got an impression from the proposed log message and the changes to
the code (except for the refs/files-backend.c, which I only skimmed)
that the idea is that the refs API stops adding LF at the end, and
the callers got a matching change to compensate for the (now)
missing LF.  If that is the idea behind the change, why do we need
to change any existing test?  The only way any tests need to be
modified due to such a change I can think of is when we forget to or
failed to make compensating change to the callers of the API.

Puzzled...

Ah, the $RUN is hiding what is really going on; it is running the
"test-tool ref-store" helper, and we did not adjust that helper.  So
if we make a compensating change to the test-tool then we do not
have to have these changes at all?  But that point may be moot.

In any case, in order to lose 5 lines from show-branch.c, and 2
lines from reflog-walk.c, I see that we had to touch 30+ lines in
refs/files-backend.c.  I find it a bit hard to sell this as an
improvement to the API, to be honest.

Luckily, it looks to me that this step is mostly unreleated to the
main thrust of these patches in the series, which is "reading
.git/logs/ in the test would work only when testing files backend;
use for-each-ref test helper to recreate what would have been read
by such tests from the files backend's files and inspect that
instead, and that would allow us test other backends for free".

So I suspect that this step can be safely dropped?

Thanks.

Re: [PATCH 2/4] refs: trim newline from reflog message

[Han-Wen Nienhuys]

On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.
>
> If this were truly "user-provided", then I'd argue that all backends
> should follow whatever the files backend has been doing forever---if
> the files added LF implicitly, others should, too, because that is
> pretty much what these "user-provided" callbacks have been expecting
> to see.

I think it's just wrong. If you pass `msg` to a storage API, you
should get `msg` when you read it back, not (msg + "\n").

> Ah, the $RUN is hiding what is really going on; it is running the
> "test-tool ref-store" helper, and we did not adjust that helper.  So
> if we make a compensating change to the test-tool then we do not
> have to have these changes at all?  But that point may be moot.
>
> In any case, in order to lose 5 lines from show-branch.c, and 2
> lines from reflog-walk.c, I see that we had to touch 30+ lines in
> refs/files-backend.c.  I find it a bit hard to sell this as an
> improvement to the API, to be honest.

The test-tool ref-store adds its own '\n', so you always get a blank
line in the output. That serves no purpose, and leads to the

  tail-n2  | head -n1

in order to read the last log line. I think it's silly, and should be dropped.

-- 
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 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

> On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> If this were truly "user-provided", then I'd argue that all backends
>> should follow whatever the files backend has been doing forever---if
>> the files added LF implicitly, others should, too, because that is
>> pretty much what these "user-provided" callbacks have been expecting
>> to see.
>
> I think it's just wrong. If you pass `msg` to a storage API, you
> should get `msg` when you read it back, not (msg + "\n").

If you give a log message "git commit -m 'single line'", you get LF
at the end of the commit message for free.  This is no different.
And you know that this is not a "storage API" that stores the input
in verbatim after looking at refs.c::copy_reflog_msg().

>> Ah, the $RUN is hiding what is really going on; it is running the
>> "test-tool ref-store" helper, and we did not adjust that helper.  So
>> if we make a compensating change to the test-tool then we do not
>> have to have these changes at all?  But that point may be moot.
>>
>> In any case, in order to lose 5 lines from show-branch.c, and 2
>> lines from reflog-walk.c, I see that we had to touch 30+ lines in
>> refs/files-backend.c.  I find it a bit hard to sell this as an
>> improvement to the API, to be honest.
>
> The test-tool ref-store adds its own '\n', so you always get a blank
> line in the output. That serves no purpose, and leads to the

But that is only because test-tool is wrong, no?  If we know that
the API gives the trailing LF, what purpose does it serve to add
another one?

>   tail-n2  | head -n1
>
> in order to read the last log line. I think it's silly, and should be dropped.

Yes, it is silly and should be dropped, but if you drop it on the
tool side, then it may become even easier to do the "instead of
reading from .git/refs/logs files, have the tool dump and inspect
that" step, no?

This being test-tool, I do not mind losing backward compatibility
that forces us a silly "tail -n 2 | head -n 1" pipeline at all.  But
if silliness comes from the test-tool thta does not work well with
the internal API, that is what should be fixed.  Surely you can
change the API and its current callers to compensate for its silliness,
but I do not think that is a good direction to go in.

Thanks.

Re: [PATCH 2/4] refs: trim newline from reflog message

[Han-Wen Nienhuys]

On Tue, Nov 23, 2021 at 6:10 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > On Mon, Nov 22, 2021 at 11:27 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> If this were truly "user-provided", then I'd argue that all backends
> >> should follow whatever the files backend has been doing forever---if
> >> the files added LF implicitly, others should, too, because that is
> >> pretty much what these "user-provided" callbacks have been expecting
> >> to see.
> >
> > I think it's just wrong. If you pass `msg` to a storage API, you
> > should get `msg` when you read it back, not (msg + "\n").
>
> If you give a log message "git commit -m 'single line'", you get LF
> at the end of the commit message for free.  This is no different.
> And you know that this is not a "storage API" that stores the input
> in verbatim after looking at refs.c::copy_reflog_msg().

I'm talking about refs/refs-internal.h. It seems you want to add something like

/* The ref backend should add a '\n' relative to the message supplied
to the delete/symref/update functions. */
typedef int for_each_reflog_ent_fn(struct ref_store *ref_store,
                                   const char *refname,
                                   each_reflog_ent_fn fn,
                                   void *cb_data);

?

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

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 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

> I'm talking about refs/refs-internal.h. It seems you want to add something like
>
> /* The ref backend should add a '\n' relative to the message supplied
> to the delete/symref/update functions. */
> typedef int for_each_reflog_ent_fn(struct ref_store *ref_store,
>                                    const char *refname,
>                                    each_reflog_ent_fn fn,
>                                    void *cb_data);
>
> ?

Sorry, I do not follow.  Doesn't the ref backend already ensure that
the message is not an incomplete line?  If you feed a single
complete line when updating, I do not think the backend should add
any extra LF relative to the given message:

    $ git update-ref -m 'creating a new branch manually
    ' refs/heads/newtest master
    $ git update-ref -m 'updating a new branch manually
    ' refs/heads/newtest master~1
    $ git reflog refs/heads/newtest
    4150a1677b refs/heads/newtest@{0}: updating a new branch manually
    5f439a0ecf refs/heads/newtest@{1}: updating the reference

I think what deserves such a comment more is the prototype for
each_reflog_ent_fn describing what the parameters to that callback
function, to help the callers of the iterator what to expect.  That
is the end-user facing part.

/*
 * Callback to process a reflog entry found by the iteration functions (see
 * below)
 */
typedef int each_reflog_ent_fn(
		struct object_id *old_oid, struct object_id *new_oid,
		const char *committer, timestamp_t timestamp,
		int tz, const char *msg, void *cb_data);

Currently it only says "Callback (see below)" but "below" has only
comments about the difference between refs_for_each_reflog_ent() and
refs_for_each_reflog_entreverse() functions, and does not talk about
what "committer" looks like (i.e. does it give user.name equivalent,
user.name plus <user.email>, or something else?), and what "msg"
looks like (i.e. if a multi-line message was given when a ref was
updated or created, do we get these lines intact? if it gets
mangled, how? if the original was a single-liner incomplete line, do
we lack the final LF?), and how tz is encoded.

I think the rule for "msg" is that:

   a multi-line message, or a message on a single incomplete-line,
   are normalized into a single complete line, and callback gets a
   single complete line.

Once these rules get specified tightly and fully, it is up to the
ref(log) backends how to implement the interface.  If files-backend
wants to keep LF at the end of the message when storing (so that it
can easily use it as record delimiter), it can do so and reuse the
LF at the end of the message as part of the msg parameter to the
callback.  If another backend wants to drop LF at the end of the
message when storing (to save space), it can do so as long as the
callback function gets the LF just like the other backend.

Re: [PATCH 2/4] refs: trim newline from reflog message

[Han-Wen Nienhuys]

On Tue, Nov 23, 2021 at 9:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > I'm talking about refs/refs-internal.h. It seems you want to add something like
> >
> > /* The ref backend should add a '\n' relative to the message supplied
> > to the delete/symref/update functions. */
> > typedef int for_each_reflog_ent_fn(struct ref_store *ref_store,
> >                                    const char *refname,
> >                                    each_reflog_ent_fn fn,
> >                                    void *cb_data);
> >
> > ?
>
> Sorry, I do not follow.  Doesn't the ref backend already ensure that
> the message is not an incomplete line?  If you feed a single
> complete line when updating, I do not think the backend should add
> any extra LF relative to the given message:

But it does, currently. As of

  commit 523fa69c36744ae6779e38614cb9bfb2be552923
  Author: Junio C Hamano <gitster@pobox.com>
  ..
  - We expect that a reflog message consists of a single line.  The
       file format used by the files backend may add a LF after the

it is the job of the files ref backend to add a LF, and the input to
the ref backend is a string without trailing LF. But the files backend
then produces that message with a LF, when reading back the data, eg.

$ git --version
git version 2.34.0.rc2.393.gf8c9666880-goog

$ GIT_TRACE_REFS="1" git branch -m bla blub
..
12:03:59.408705 refs/debug.c:162        rename_ref: refs/heads/bla ->
refs/heads/blub "Branch: renamed refs/heads/bla to refs/heads/blub": 0

$ GIT_TRACE_REFS=1 git reflog show refs/heads/blub
12:04:23.277805 refs/debug.c:294        reflog_ent refs/heads/blub
(ret 0): cd3e606211bb1cf8bc57f7d76bab98cc17a150bc ->
cd3e606211bb1cf8bc57f7d76bab98cc17a150bc, Han-Wen Nienhuys
<hanwen@google.com> 1637751839 "Branch: renamed refs/heads/bla to
refs/heads/blub
"

> I think the rule for "msg" is that:
>
>    a multi-line message, or a message on a single incomplete-line,
>    are normalized into a single complete line, and callback gets a
>    single complete line.
>

That is not how it works today. The files backend verbatimly dumps the
message supplied to it. (Maybe it should crash if there is a '\n' in
the message).

-- 
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 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

>> ...  Doesn't the ref backend already ensure that
>> the message is not an incomplete line?  If you feed a single
>> complete line when updating, I do not think the backend should add
>> any extra LF relative to the given message:
>
> But it does, currently. As of
>
>   commit 523fa69c36744ae6779e38614cb9bfb2be552923
>   Author: Junio C Hamano <gitster@pobox.com>
>   ..
>   - We expect that a reflog message consists of a single line.  The
>        file format used by the files backend may add a LF after the
>
> it is the job of the files ref backend to add a LF, and the input to
> the ref backend is a string without trailing LF. But the files backend
> then produces that message with a LF, when reading back the data, eg.

Ah, perhaps our confusion comes from the fact that I view the ref
API as a whole and do not draw a fine line of distinction between
the "ref API implementation common across backends" vs "what ref
files backend does".  But as the implementor of one backend, you
obviously have to care.

In any case, when I did that commit, I clearly meant that the
normalization implemented by the patch belong to the common part to
be used by all backends for uniform handling of refs.  If a call to
refs API in a repository that is configured to use reftable backend
bypasses the normalization function introduced in that commit, that
is a bug.

So, yes "ref API ensures that the message is not an incomplete line
to turn 0, 1, or multiple lines input into a single line", which is
why the experiments you omitted from your quote (reproduced here)

    $ git update-ref -m 'creating a new branch manually
    ' refs/heads/newtest master
    $ git update-ref -m 'updating a new branch manually
    ' refs/heads/newtest master~1
    $ git reflog refs/heads/newtest
    4150a1677b refs/heads/newtest@{0}: updating a new branch manually
    5f439a0ecf refs/heads/newtest@{1}: updating the reference

to give a complete line when recording the reflog entries does not
result in an extra LF shown on the output.

>> I think the rule for "msg" is that:
>>
>>    a multi-line message, or a message on a single incomplete-line,
>>    are normalized into a single complete line, and callback gets a
>>    single complete line.
>
> That is not how it works today. The files backend verbatimly dumps the
> message supplied to it. (Maybe it should crash if there is a '\n' in
> the message).

As I said, if parts of refs API implementation forgets to call
normalization, it is a bug.  Is there different codepath other than
the one that is exercised with the "git update -m ''" experiment
above and you found such a bug there?  It needs to be fixed.

Thanks.

Re: [PATCH 2/4] refs: trim newline from reflog message

[Han-Wen Nienhuys]

On Wed, Nov 24, 2021 at 7:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> >> ...  Doesn't the ref backend already ensure that
> >> the message is not an incomplete line?  If you feed a single
> >> complete line when updating, I do not think the backend should add
> >> any extra LF relative to the given message:
> >
> > But it does, currently. As of
> >
> >   commit 523fa69c36744ae6779e38614cb9bfb2be552923
> >   Author: Junio C Hamano <gitster@pobox.com>
> >   ..
> >   - We expect that a reflog message consists of a single line.  The
> >        file format used by the files backend may add a LF after the
> >
> > it is the job of the files ref backend to add a LF, and the input to
> > the ref backend is a string without trailing LF. But the files backend
> > then produces that message with a LF, when reading back the data, eg.
>
> Ah, perhaps our confusion comes from the fact that I view the ref
> API as a whole and do not draw a fine line of distinction between
> the "ref API implementation common across backends" vs "what ref
> files backend does".  But as the implementor of one backend, you
> obviously have to care.

Having the read function return something different than what the
write gets still seems odd to me.
How about having copy_reflog_msg() trim trailing space and then always add LF?

The files backend can assert that the string ends in LF, and doesn't
need to add LF itself.

-- 
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 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

>> Ah, perhaps our confusion comes from the fact that I view the ref
>> API as a whole and do not draw a fine line of distinction between
>> the "ref API implementation common across backends" vs "what ref
>> files backend does".  But as the implementor of one backend, you
>> obviously have to care.
>
> Having the read function return something different than what the
> write gets still seems odd to me.
> How about having copy_reflog_msg() trim trailing space and then always add LF?
>
> The files backend can assert that the string ends in LF, and doesn't
> need to add LF itself.

Ehh, let's step back a bit.

Is there anything in the common part and files backend in ref API
that needs to be changed to fix some bug?  I do not see anything
broken that needs to be fixed by "assert that the string ends in LF
and doesn't need to add LF itself" or by "always add LF".

Re: [PATCH 2/4] refs: trim newline from reflog message

[Han-Wen Nienhuys]

On Wed, Nov 24, 2021 at 9:55 PM Junio C Hamano <gitster@pobox.com> wrote:
> Ehh, let's step back a bit.
>
> Is there anything in the common part and files backend in ref API
> that needs to be changed to fix some bug?  I do not see anything
> broken that needs to be fixed by "assert that the string ends in LF
> and doesn't need to add LF itself" or by "always add LF".

Clearly, you and I disagree about what is logical behavior here.

I've reworked the series to fit with your opinion on the matter. PTAL.

-- 
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 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

> On Wed, Nov 24, 2021 at 9:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Ehh, let's step back a bit.
>>
>> Is there anything in the common part and files backend in ref API
>> that needs to be changed to fix some bug?  I do not see anything
>> broken that needs to be fixed by "assert that the string ends in LF
>> and doesn't need to add LF itself" or by "always add LF".
>
> Clearly, you and I disagree about what is logical behavior here.
>
> I've reworked the series to fit with your opinion on the matter. PTAL.

Thanks.

I find both are logical and internally consistent, so I prefer a
choice that requires fewer changes to what is already working.

Re: [PATCH 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

> $ GIT_TRACE_REFS="1" git branch -m bla blub
> ..
> 12:03:59.408705 refs/debug.c:162        rename_ref: refs/heads/bla ->
> refs/heads/blub "Branch: renamed refs/heads/bla to refs/heads/blub": 0
>
> $ GIT_TRACE_REFS=1 git reflog show refs/heads/blub
> 12:04:23.277805 refs/debug.c:294        reflog_ent refs/heads/blub
> (ret 0): cd3e606211bb1cf8bc57f7d76bab98cc17a150bc ->
> cd3e606211bb1cf8bc57f7d76bab98cc17a150bc, Han-Wen Nienhuys
> <hanwen@google.com> 1637751839 "Branch: renamed refs/heads/bla to
> refs/heads/blub
> "
>
>> I think the rule for "msg" is that:
>>
>>    a multi-line message, or a message on a single incomplete-line,
>>    are normalized into a single complete line, and callback gets a
>>    single complete line.
>>
>
> That is not how it works today. The files backend verbatimly dumps the
> message supplied to it. (Maybe it should crash if there is a '\n' in
> the message).

I still am puzzled what you wanted to illustrate with the "git
branch -m bla" trace.  The call graph into the refs API looks like
this:

builtin/branch.c::cmd_branch()
 -> branch.c::create_branch()
    -> refs.c::ref_transaction_update()
       -> refs.c::ref_transaction_add_update()
    -> refs.c::ref_transaction_commit()

and the message the user gave is passed through in "msg" variable
without modification, when calling ref_transaction_update(), which
in turn makes a call to ref_transaction_add_update().  It does this:

    struct ref_update *ref_transaction_add_update(
                    struct ref_transaction *transaction,
                    const char *refname, unsigned int flags,
                    const struct object_id *new_oid,
                    const struct object_id *old_oid,
                    const char *msg)
    {
            struct ref_update *update;

            if (transaction->state != REF_TRANSACTION_OPEN)
                    BUG("update called for transaction that is not open");

            FLEX_ALLOC_STR(update, refname, refname);
            ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc);
            transaction->updates[transaction->nr++] = update;

            update->flags = flags;

            if (flags & REF_HAVE_NEW)
                    oidcpy(&update->new_oid, new_oid);
            if (flags & REF_HAVE_OLD)
                    oidcpy(&update->old_oid, old_oid);
            update->msg = normalize_reflog_message(msg);
            return update;
    }

And normalize_reflog_message() calls copy_reflog_msg() to squash
runs of isspace() bytes (note: that class includes LF) into a single
space, and runs rtrim(), so update->msg will get a single incomplete
line.  As I suspected in a separate message, I think my notion of
what happens in the ref API implementation common to all backends
and what happens in each backend, and hence my statements about the
distinction, were much fuzzier than yours, so I should say that I
consider that all of the above is happening in the common part
across all backends.  If normalize happens in ref-files, it should
happen in reftable backend, too, automatically.

The files-backend has no chance to even see an embedded LF in the
message when the transaction gets committed and the backend is
called, does it?  So I am not sure why we should crash upon seeing a
LF in the message.

In any case, it seems that as a comment to clarify the end-user
facing each_reflog_ent_fn() parameters, what you quoted above from
my message seems correct to me, after following the callgraph like
the above.  A 0-line (i.e. incomplete, like your 'bla' given to "git
branch"), 1-line (i.e. a single complete line, like the message I
gave to "update-ref -m" in my earlier illustration), or multi-line
message given when a reflog entry is created, is normalized and when
the each_reflog_ent_fn() callback is called, it is shown to the
callback function as a single complete line, with a LF at the end.

Phrased without the explanation specific to this discussion, but
with a bit more detail:

  The message given when a reflog entry was created, is normalized
  into a single line by turning each LF into a space, squeezing a
  run of multiple whitespaces into one space, and removing trailing
  whitespaces. The callback gets a single complete line in the 'msg'
  parameter.

perhaps?

Re: [PATCH 2/4] refs: trim newline from reflog message

[Han-Wen Nienhuys]

On Wed, Nov 24, 2021 at 8:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > $ GIT_TRACE_REFS="1" git branch -m bla blub
> > ..
> > 12:03:59.408705 refs/debug.c:162        rename_ref: refs/heads/bla ->
> > refs/heads/blub "Branch: renamed refs/heads/bla to refs/heads/blub": 0
> >
> > $ GIT_TRACE_REFS=1 git reflog show refs/heads/blub
> > 12:04:23.277805 refs/debug.c:294        reflog_ent refs/heads/blub
> > (ret 0): cd3e606211bb1cf8bc57f7d76bab98cc17a150bc ->
> > cd3e606211bb1cf8bc57f7d76bab98cc17a150bc, Han-Wen Nienhuys
> > <hanwen@google.com> 1637751839 "Branch: renamed refs/heads/bla to
> > refs/heads/blub
> > "
> >
> >> I think the rule for "msg" is that:
> >>
> >>    a multi-line message, or a message on a single incomplete-line,
> >>    are normalized into a single complete line, and callback gets a
> >>    single complete line.
> >>
> >
> > That is not how it works today. The files backend verbatimly dumps the
> > message supplied to it. (Maybe it should crash if there is a '\n' in
> > the message).
>
> I still am puzzled what you wanted to illustrate with the "git
> branch -m bla" trace.

I'm trying to illustrate that (from the perspective of the ref backend
API) one call inserts something without a '\n', but the call that
reads the info back, gets the same data back with a '\n'. It looks
confusing and inconsistent to me.

It seems fine to decide that the message should always end in a LF,
but then why not do that in the normalization routine, so it is shared
across all backends?

For the purpose of the debug support (GIT_TRACE_REFS=1), I would
rather prefer if the message was always without LF, because the LF
ruins the visual of the debug output, but that is a minor concern.

-- 
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 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

> I think what deserves such a comment more is the prototype for
> each_reflog_ent_fn describing what the parameters to that callback
> function, to help the callers of the iterator what to expect.  That
> is the end-user facing part.

Perhaps something along this line.

--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
Subject: [PATCH] refs: document callback for reflog-ent iterators

refs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse()
functions take a callback function that gets called with the details
of each reflog entry.  Its parameters were not documented beyond
their names.  Elaborate a bit on each of them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 refs.h | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git c/refs.h w/refs.h
index 48970dfc7e..4fa97d77cf 100644
--- c/refs.h
+++ w/refs.h
@@ -462,7 +462,27 @@ int delete_reflog(const char *refname);
 
 /*
  * Callback to process a reflog entry found by the iteration functions (see
- * below)
+ * below).
+ *
+ * The committer parameter is a single string, in the form
+ * "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" (without double quotes).
+ *
+ * The timestamp parameter gives the seconds since epoch when the reflog
+ * entry was created.
+ *
+ * The tz parameter is an up to 4 digits integer, whose absolute value
+ * gives the hour and minute offset from GMT (the lower 2 digits are
+ * minutes, the higher 2 digits are hours).  A negative tz means west of,
+ * and a positive tz means east of GMT.
+ *
+ * The msg parameter is a single complete line; a reflog message given
+ * to refs_delete_ref, refs_update_ref, etc. is returned to the
+ * callback normalized---each run of whitespaces are squashed into a
+ * single whitespace, trailing whitespace, if exists, is trimmed, and
+ * then a single LF is added at the end.
+ *
+ * The cb_data is a caller-supplied pointer given to the iterator
+ * functions.
  */
 typedef int each_reflog_ent_fn(
 		struct object_id *old_oid, struct object_id *new_oid,

Re: Re* [PATCH 2/4] refs: trim newline from reflog message

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 26 2021, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I think what deserves such a comment more is the prototype for
>> each_reflog_ent_fn describing what the parameters to that callback
>> function, to help the callers of the iterator what to expect.  That
>> is the end-user facing part.
>
> Perhaps something along this line.
>
> --- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---
> Subject: [PATCH] refs: document callback for reflog-ent iterators
>
> refs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse()
> functions take a callback function that gets called with the details
> of each reflog entry.  Its parameters were not documented beyond
> their names.  Elaborate a bit on each of them.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  refs.h | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git c/refs.h w/refs.h
> index 48970dfc7e..4fa97d77cf 100644
> --- c/refs.h
> +++ w/refs.h
> @@ -462,7 +462,27 @@ int delete_reflog(const char *refname);
>  
>  /*
>   * Callback to process a reflog entry found by the iteration functions (see
> - * below)
> + * below).
> + *
> + * The committer parameter is a single string, in the form
> + * "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" (without double quotes).
> + *
> + * The timestamp parameter gives the seconds since epoch when the reflog
> + * entry was created.
> + *
> + * The tz parameter is an up to 4 digits integer, whose absolute value
> + * gives the hour and minute offset from GMT (the lower 2 digits are
> + * minutes, the higher 2 digits are hours).  A negative tz means west of,
> + * and a positive tz means east of GMT.

The Line and Attu islands beg to differ :)

Nitpicking aside, perhaps these two pargaraphs would be better as simply
replaced by:

    Consult "Git internal format" in git-commit-tree(1) for
    details about the "<unix timestamp> <time zone offset>" format, and
    see show_one_reflog_ent() for the parsing function.

Re: Re* [PATCH 2/4] refs: trim newline from reflog message

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Nitpicking aside, perhaps these two pargaraphs would be better as simply
> replaced by:
>
>     Consult "Git internal format" in git-commit-tree(1) for
>     details about the "<unix timestamp> <time zone offset>" format, and
>     see show_one_reflog_ent() for the parsing function.

Much nicer; this is developer facing and reducing risk of
inconsistency by incorrectly duplicating is much more important than
having the info available in a single place.

Thanks.

Re* [PATCH 2/4] refs: trim newline from reflog message

[Junio C Hamano]

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> Nitpicking aside, perhaps these two pargaraphs would be better as simply
>> replaced by:
>>
>>     Consult "Git internal format" in git-commit-tree(1) for
>>     details about the "<unix timestamp> <time zone offset>" format, and
>>     see show_one_reflog_ent() for the parsing function.
>
> Much nicer; this is developer facing and reducing risk of
> inconsistency by incorrectly duplicating is much more important than
> having the info available in a single place.

Having actually read the description, I do not think
"Documentation/date-formats.txt::Git internal format" is a good fit.
We are describing the format of a single string there (e.g. <unix
timestamp> and <time zone offset> form a single string with one SP
in between), but the parameters are integral types.

Specifically because the "Git internal format" is textual, an
example "+0100" given there makes it sufficiently clear that the
offset is a "a sign followed by 4 digits" string, but those who need
to use the value in "int tz" must know that is represented as a
positive one hundred, not sixty, which does not have to be captured
there for the intended audience of date-formats.txt pagelet.

Here is what I came up with

---- >8 -------- >8 -------- >8 -------- >8 -------- >8 ----
Subject: [PATCH v2] refs: document callback for reflog-ent iterators

refs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse()
functions take a callback function that gets called with the details
of each reflog entry.  Its parameters were not documented beyond
their names.  Elaborate a bit on each of them.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * rewrite timestamp and tz, adjusting the internal format's
   description from Documentation/date-formats.txt pagelet, as
   pointed out by Ævar.

 refs.h | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

 The indented patch shows the part that was changed from the
 previous one.

    diff --git i/refs.h w/refs.h
    index 4fa97d77cf..f6fed5c7d8 100644
    --- i/refs.h
    +++ w/refs.h
    @@ -467,13 +467,15 @@ int delete_reflog(const char *refname);
      * The committer parameter is a single string, in the form
      * "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" (without double quotes).
      *
    - * The timestamp parameter gives the seconds since epoch when the reflog
    - * entry was created.
    - *
    - * The tz parameter is an up to 4 digits integer, whose absolute value
    - * gives the hour and minute offset from GMT (the lower 2 digits are
    - * minutes, the higher 2 digits are hours).  A negative tz means west of,
    - * and a positive tz means east of GMT.
    + * The timestamp parameter gives the time when entry was created as the number
    + * of seconds since the UNIX epoch.
    + *
    + * The tz parameter gives the timezone offset for the user who created
    + * the reflog entry, and its value gives a positive or negative offset
    + * from UTC.  Its absolute value is formed by multiplying the hour
    + * part by 100 and adding the minute part.  For example, 1 hour ahead
    + * of UTC, CET == "+0100", is represented as positive one hundred (not
    + * postiive sixty).
      *
      * The msg parameter is a single complete line; a reflog message given
      * to refs_delete_ref, refs_update_ref, etc. is returned to the

diff --git c/refs.h w/refs.h
index 48970dfc7e..f6fed5c7d8 100644
--- c/refs.h
+++ w/refs.h
@@ -462,7 +462,29 @@ int delete_reflog(const char *refname);
 
 /*
  * Callback to process a reflog entry found by the iteration functions (see
- * below)
+ * below).
+ *
+ * The committer parameter is a single string, in the form
+ * "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" (without double quotes).
+ *
+ * The timestamp parameter gives the time when entry was created as the number
+ * of seconds since the UNIX epoch.
+ *
+ * The tz parameter gives the timezone offset for the user who created
+ * the reflog entry, and its value gives a positive or negative offset
+ * from UTC.  Its absolute value is formed by multiplying the hour
+ * part by 100 and adding the minute part.  For example, 1 hour ahead
+ * of UTC, CET == "+0100", is represented as positive one hundred (not
+ * postiive sixty).
+ *
+ * The msg parameter is a single complete line; a reflog message given
+ * to refs_delete_ref, refs_update_ref, etc. is returned to the
+ * callback normalized---each run of whitespaces are squashed into a
+ * single whitespace, trailing whitespace, if exists, is trimmed, and
+ * then a single LF is added at the end.
+ *
+ * The cb_data is a caller-supplied pointer given to the iterator
+ * functions.
  */
 typedef int each_reflog_ent_fn(
 		struct object_id *old_oid, struct object_id *new_oid,

Re: Re* [PATCH 2/4] refs: trim newline from reflog message

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 28 2021, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> Nitpicking aside, perhaps these two pargaraphs would be better as simply
>>> replaced by:
>>>
>>>     Consult "Git internal format" in git-commit-tree(1) for
>>>     details about the "<unix timestamp> <time zone offset>" format, and
>>>     see show_one_reflog_ent() for the parsing function.
>>
>> Much nicer; this is developer facing and reducing risk of
>> inconsistency by incorrectly duplicating is much more important than
>> having the info available in a single place.
>
> Having actually read the description, I do not think
> "Documentation/date-formats.txt::Git internal format" is a good fit.
> We are describing the format of a single string there (e.g. <unix
> timestamp> and <time zone offset> form a single string with one SP
> in between), but the parameters are integral types.
>
> Specifically because the "Git internal format" is textual, an
> example "+0100" given there makes it sufficiently clear that the
> offset is a "a sign followed by 4 digits" string, but those who need
> to use the value in "int tz" must know that is represented as a
> positive one hundred, not sixty, which does not have to be captured
> there for the intended audience of date-formats.txt pagelet.
>
> Here is what I came up with
>
> ---- >8 -------- >8 -------- >8 -------- >8 -------- >8 ----
> Subject: [PATCH v2] refs: document callback for reflog-ent iterators
>
> refs_for_each_reflog_ent() and refs_for_each_reflog_ent_reverse()
> functions take a callback function that gets called with the details
> of each reflog entry.  Its parameters were not documented beyond
> their names.  Elaborate a bit on each of them.
>
> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * rewrite timestamp and tz, adjusting the internal format's
>    description from Documentation/date-formats.txt pagelet, as
>    pointed out by Ævar.
>
>  refs.h | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
>  The indented patch shows the part that was changed from the
>  previous one.
>
>     diff --git i/refs.h w/refs.h
>     index 4fa97d77cf..f6fed5c7d8 100644
>     --- i/refs.h
>     +++ w/refs.h
>     @@ -467,13 +467,15 @@ int delete_reflog(const char *refname);
>       * The committer parameter is a single string, in the form
>       * "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" (without double quotes).
>       *
>     - * The timestamp parameter gives the seconds since epoch when the reflog
>     - * entry was created.
>     - *
>     - * The tz parameter is an up to 4 digits integer, whose absolute value
>     - * gives the hour and minute offset from GMT (the lower 2 digits are
>     - * minutes, the higher 2 digits are hours).  A negative tz means west of,
>     - * and a positive tz means east of GMT.
>     + * The timestamp parameter gives the time when entry was created as the number
>     + * of seconds since the UNIX epoch.
>     + *
>     + * The tz parameter gives the timezone offset for the user who created
>     + * the reflog entry, and its value gives a positive or negative offset
>     + * from UTC.  Its absolute value is formed by multiplying the hour
>     + * part by 100 and adding the minute part.  For example, 1 hour ahead
>     + * of UTC, CET == "+0100", is represented as positive one hundred (not
>     + * postiive sixty).
>       *
>       * The msg parameter is a single complete line; a reflog message given
>       * to refs_delete_ref, refs_update_ref, etc. is returned to the

Thanks, this version of these docs looks good to me!

Re: [PATCH 2/4] refs: trim newline from reflog message

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <hanwen@google.com>
> [...]
> @@ -3059,18 +3059,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;
>  }

I haven't looked deeply into this topic as a whole, but FWIW this
conflicts with a topic I've got locally and was going to submit sometime
after the current batch of my own ref fixes in "next".

That we've got verbose output at all in file-backend.c is a wart, and it
should just be moved out if it entirely, this commit (stacked on top of
various other fixes I've got in the area) does that:
https://github.com/avar/git/commit/eff40d2d81b

With that change the reftable code will need to handle far less of this,
as it'll be handled in builtin/reflog.c, it just needs to behave
properly in the appropriate "expire reflog?" callback. I.e. the backend
tells us "does this expire?", the builtin/reflog.c code consumes that
and does any verbose or non-verbose (or progress etc.) output.

Now, the merge conflict between that and this looks rather trivial, but
I can't help but wonder if we're going in the wrong direction here
API-wise, or maybe "wrong direction" is too strong, but "sticking with
the wrong patterN?".

I think your cleanup works, but wouldn't a better thing be to move this
to callbacks rather than tweaking the fprintf formats?

I.e. in my version the whole body of this function has become:
	
	static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
				     const char *email, timestamp_t timestamp, int tz,
				     const char *message, void *cb_data)
	{
		struct expire_reflog_cb *cb = cb_data;
		reflog_expiry_should_prune_fn *fn = cb->should_prune_fn;
	
		if (cb->rewrite)
			ooid = &cb->last_kept_oid;
	
		if (fn(ooid, noid, email, timestamp, tz, message, cb->policy_cb))
			return 0;
	
		if (cb->dry_run)
			return 0; /* --dry-run */
	
		fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s", oid_to_hex(ooid),
			oid_to_hex(noid), email, timestamp, tz, message);
		oidcpy(&cb->last_kept_oid, noid);
	
		return 0;
	}

The only file-backend specific part of it is that fprintf(). If we moved
towards making that part of it be:

	write_reflog_entry_cb(oid_to_hex(ooid), oid_to_hex(noid), email, timestamp, tz, message);

Then we could lift the whole of this API to a level that makes more
sense for a backed to implement.

The refs/files-backend.c shouldn't need to have one function calll the
"should_prune_fn" *and* write out the data. Instead some code common to
all backends should call the "should prune?", and then call the
backend's "here's an entry for you to write" callback.

But maybe I'm overthinking this whole thing. I'm just wondering if we
have say a sqlite reflog backend as opposed to the file/reftable backend
that wants to store this data in a schema. Such a a backend would need
to unpack the data, as we're sprintf() formatting function parameters
before it gets passed to the backends.

Re: [PATCH 2/4] refs: trim newline from reflog message

[Han-Wen Nienhuys]

On Tue, Nov 23, 2021 at 11:40 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> I think your cleanup works, but wouldn't a better thing be to move this
> to callbacks rather than tweaking the fprintf formats?

sure. In the reftable glue, I have

        if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
                /* XXX - skip writing records that were not changed. */
                err = reftable_addition_commit(add);
        } else {
                /* XXX - print something */
        }

letting the callbacks do the printing means less work for reftable.

> The refs/files-backend.c shouldn't need to have one function calll the
> "should_prune_fn" *and* write out the data. Instead some code common to
> all backends should call the "should prune?", and then call the
> backend's "here's an entry for you to write" callback.

not sure if that will work. For reftable, you have to write something
(a tombstone) if you _do_ want to prune the entry.

> But maybe I'm overthinking this whole thing. I'm just wondering if we
> have say a sqlite reflog backend as opposed to the file/reftable backend
> that wants to store this data in a schema. Such a a backend would need

reftable also stores this in a schema: there are separate fields for
e-mail, timezone, timestamp etc.

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

Re: [PATCH 3/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

There is no v$n designator on the title line, but I have this
feeling that I've seen this patch before.  More importantly, I
remember that I found it unclear what you exactly mean "the" reflog
format.  Is that what the files backend stores on one line in its
file?  The reason I suspect that may be the answer is because I do
not recall documenting "the" reflog format in Documentation/ and
whatever we have historically been writing would be the most
canonical and/or authoritative format.

> reflogs in tests without using inspecting files under .git/logs/

I agree 100% with the goal.  

It seems that one line of .git/logs/HEAD looks like

<new> SP <old> SP <user> SP '<' <email> '>' SP <time> SP <zone> HT <oneline> LF

and being able to extract a line like that for given reflog entry
out of any backend in a consistent way is valuable when testing
different backends.

It seems that is what the new code is writing, so perhaps the first
paragraph can be clarified to indicate as such.

    We have some tests that read from files in .git/logs/ hierarchy
    when checking if correct reflog entries are created, but that is
    too specific to the files backend.  Other backends like reftable
    may not store its reflog entries in such a "one line per entry"
    format.

    Update for-each-reflog-ent test helper to produce output that
    would be identical to lines in a reflog file files backend uses.
    That way, (1) the current tests can be updated to use the test
    helper to read the reflog entries instead of (parts of) reflog
    files, and perform the same inspection for correctness, and (2)
    when the ref backend is swapped to another backend, the updated
    test can be used as-is to check the correctness.

or something along the line?

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

Looks good to me.  We might want to make the printf format
conditional to add \t%s only when msg is not empty, though.
Hopefully such a change would follow the reflog format even more
closely to make 4/4 unnecessary?

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

I am not sure how this new test helps validate the change to the
code.

What was different in the old output was that the timezone was not
in %+05d format, and the field separator before the log message was
not HT.  So if this grepped for HT or +0000 to make sure we are
using the format that is close to what actually is stored in the
files, I would understand this change, but it is unclear what it
proves to make sure that the oldest entry has recreate-main.

In fact, with the code part of the patch reverted, this new test
seems to pass.

>  	tail -n1 actual | grep one
>  '

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

[Han-Wen Nienhuys]

On Mon, Nov 22, 2021 at 11:31 PM Junio C Hamano <gitster@pobox.com> 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
>
> There is no v$n designator on the title line, but I have this
> feeling that I've seen this patch before.  More importantly, I

you have, as part of a RFC to drop "update reflog if reflog exists".
Since we didn't get consensus there, I'm offering up its predecessors
separately.

> or something along the line?

sure. I'll update the message

> > @@ -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);
>
> Looks good to me.  We might want to make the printf format
> conditional to add \t%s only when msg is not empty, though.
> Hopefully such a change would follow the reflog format even more
> closely to make 4/4 unnecessary?

I think the conditional formatting of \t is impractical. It makes things like

  (metadata, msg) = line.split('\t')

in Python require special casing in case msg is empty.

> > 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 &&
>
> I am not sure how this new test helps validate the change to the
> code.

It's for consistency with the preceding test. I can make a separate commit.

-- 
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 3/4] test-ref-store: tweaks to for-each-reflog-ent format

[Junio C Hamano]

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

>> > +     printf("%s %s %s %" PRItime " %+05d\t%s\n", oid_to_hex(old_oid),
>> > +            oid_to_hex(new_oid), committer, timestamp, tz, msg);
>>
>> Looks good to me.  We might want to make the printf format
>> conditional to add \t%s only when msg is not empty, though.
>> Hopefully such a change would follow the reflog format even more
>> closely to make 4/4 unnecessary?
>
> I think the conditional formatting of \t is impractical. It makes things like
>
>   (metadata, msg) = line.split('\t')
>
> in Python require special casing in case msg is empty.

Doesn't it however make it cumobersome (as we saw in 4/4 and Ævar's
reaction to it) to write tests to add trailing whitespace like this,
I am afraid?

Without trailing HT, a self-test of this data dumper would become
trivial---just run it and compare its output with the real file in
.git/refs/logs/ directory, no?

As this is only test-helper, I do not mind the deviation from the
format, even though the log message claims to make it closer, to
always show HT.  And because the consumers of this data are only
test scripts, I do not mind they are sloppier than the real-world
code.

But if this were a pair of real world data producer/consumer, the
consumer would be prepared to see and deal with a line that ought to
have but lacks HT anyway, so I suspect that the amount of code to
parse conditionally added HT is not that large.

>> > 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 &&
>>
>> I am not sure how this new test helps validate the change to the
>> code.
>
> It's for consistency with the preceding test. I can make a separate commit.

Meaning this is an unrelated clean-up of the existing test before
this series started?  Sure, just like the show-branch bugfix, it
would be nicer to have a separate commit for it.

Thanks.

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

[Han-Wen Nienhuys via GitGitGadget]

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

This avoids inspecting the file system, which only works with the files ref
backend.

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 0d4f73acaa8..f91432e7a25 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

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

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:

>  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

So preceding commits added a trailing tab, or was that always the case
with the alternate test utility?

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

[Han-Wen Nienhuys]

On Mon, Nov 22, 2021 at 4:21 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:
>
> >  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
>
> So preceding commits added a trailing tab, or was that always the case
> with the alternate test utility?

log_ref_write_fd() only writes \t into the log message if there is a
non-empty message, and previously this was checking the log file
directly. The test helper tweaked in one of the previous commits
unconditionally prints a \t, which we now have to deal with.

-- 
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 4/4] t1400: use test-helper ref-store to inspect reflog contents

[Junio C Hamano]

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

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> This avoids inspecting the file system, which only works with the files ref
> backend.

Nice.

Between always leaving HT after the timezone and mimicking the
files-backend storage more closely by using HT as a separator
only when there is actually a message, I have no strong preference,
but as a "test-helper", being uniform would be more useful than
being less ugly, so I think I am OK with this change (and the fact
that test-helper unconditonally puts HT in hits output).



>  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

[PATCH v2 0/5] Inspect reflog data programmatically in more tests

[Han-Wen Nienhuys via GitGitGadget]

This helps for reftable support, and will help if we want to reconsider
under which conditions reflogs get created/updated.

Han-Wen Nienhuys (5):
  show-branch: show reflog message
  test-ref-store: don't add newline to reflog message
  t1405: check for_each_reflog_ent_reverse() more thoroughly
  test-ref-store: tweaks to for-each-reflog-ent format
  refs/debug: trim trailing LF from reflog message

 builtin/show-branch.c          | 12 +++++++-----
 refs/debug.c                   | 10 ++++++++--
 t/helper/test-ref-store.c      |  6 +++---
 t/t1400-update-ref.sh          | 13 ++++++++-----
 t/t1405-main-ref-store.sh      |  4 ++--
 t/t1406-submodule-ref-store.sh |  4 ++--
 t/t3202-show-branch.sh         | 15 +++++++++++++++
 7 files changed, 45 insertions(+), 19 deletions(-)


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

Range-diff vs v1:

 1:  fd2595d370a = 1:  9d8394d8c76 show-branch: show reflog message
 2:  dfb63937323 ! 2:  4a86d212589 refs: trim newline from reflog message
     @@ Metadata
      Author: Han-Wen Nienhuys <hanwen@google.com>
      
       ## Commit message ##
     -    refs: trim newline from reflog message
     +    test-ref-store: don't add newline to reflog message
      
     -    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.
     +    The files backend produces a newline for messages automatically, so before we
     +    would print blank lines between entries.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
     - ## builtin/show-branch.c ##
     -@@ builtin/show-branch.c: 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;
     - 
     -@@ builtin/show-branch.c: 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,
     -
     - ## 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);
     - 	}
     + ## 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);
     ++	printf("%s %s %s %" PRItime " %d %s", oid_to_hex(old_oid),
     ++	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
       	return 0;
       }
     + 
      
       ## t/t1405-main-ref-store.sh ##
      @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog()' '
 3:  8a1b094d547 ! 3:  0319503045b 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
     +    t1405: check for_each_reflog_ent_reverse() more thoroughly
      
     -    Follow the reflog format more closely, so it can be used for comparing
     -    reflogs in tests without using inspecting files under .git/logs/
     +    If we are checking for a certain ordering, we should check that there are two
     +    entries. Do this by mirroring the preceding test.
      
          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);
     -+	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;
     - }
     - 
     -
       ## t/t1405-main-ref-store.sh ##
      @@ t/t1405-main-ref-store.sh: test_expect_success 'for_each_reflog_ent()' '
       
 4:  4ba97a4e70a ! 4:  62f5cb8a824 t1400: use test-helper ref-store to inspect reflog contents
     @@ Metadata
      Author: Han-Wen Nienhuys <hanwen@google.com>
      
       ## Commit message ##
     -    t1400: use test-helper ref-store to inspect reflog contents
     +    test-ref-store: tweaks to for-each-reflog-ent format
      
     -    This avoids inspecting the file system, which only works with the files ref
     -    backend.
     +    We have some tests that read from files in .git/logs/ hierarchy
     +    when checking if correct reflog entries are created, but that is
     +    too specific to the files backend.  Other backends like reftable
     +    may not store its reflog entries in such a "one line per entry"
     +    format.
     +
     +    Update for-each-reflog-ent test helper to produce output that
     +    is identical to lines in a reflog file files backend uses.
     +    That way, (1) the current tests can be updated to use the test
     +    helper to read the reflog entries instead of (parts of) reflog
     +    files, and perform the same inspection for correctness, and (2)
     +    when the ref backend is swapped to another backend, the updated
     +    test can be used as-is to check the correctness.
     +
     +    Adapt t1400 to use the for-each-reflog-ent test helper.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
     - ## t/t1400-update-ref.sh ##
     -@@ t/t1400-update-ref.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     - . ./test-lib.sh
     + ## 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", oid_to_hex(old_oid),
     +-	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
     ++	printf("%s %s %s %" PRItime " %+05d%s%s", oid_to_hex(old_oid),
     ++	       oid_to_hex(new_oid), committer, timestamp, tz,
     ++	       *msg == '\n' ? "" : "\t", msg);
     + 	return 0;
     + }
       
     - 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
     --$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
     -+$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB
     +
     + ## t/t1400-update-ref.sh ##
     +@@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
     + $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
       EOF
       test_expect_success "verifying $m's log (logged by touch)" '
      -	test_when_finished "rm -rf .git/$m .git/logs expect" &&
     @@ t/t1400-update-ref.sh: test_expect_success 'symref empty directory removal' '
       '
       
       test_expect_success "create $m (logged by config)" '
     -@@ t/t1400-update-ref.sh: 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
     +@@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
     + $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
       EOF
       test_expect_success "verifying $m's log (logged by config)" '
      -	test_when_finished "rm -f .git/$m .git/logs/$m expect" &&
 -:  ----------- > 5:  0288e743eb2 refs/debug: trim trailing LF from reflog message

-- 
gitgitgadget

[PATCH v2 1/5] 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 082449293b5..f1e8318592c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -761,6 +761,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;
 
@@ -770,11 +771,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 v2 2/5] test-ref-store: don't add newline to reflog message

[Han-Wen Nienhuys via GitGitGadget]

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

The files backend produces a newline for messages automatically, so before we
would print blank lines between entries.

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

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index b314b81a45b..ef244aa6b27 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 " %d %s", 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 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

Re: [PATCH v2 2/5] test-ref-store: don't add newline to reflog message

[Junio C Hamano]

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

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> The files backend produces a newline for messages automatically, so before we
> would print blank lines between entries.

It is misleading to say "The files backend produces" here.  The
promise the API makes to the callback functions is that msg will be
a single complete line, regardless of what ref backend is used, no?

Unless we plan to break the promise to the callback, forcing them to
compensate the differences of behaviour between backends, by making
the reftable backend behave differently.

    The callback for each-reflog-ent iterator functions receives a
    single complete line terminated with a newline at the end in the
    'msg' parameter; by having a newline in the printf() format
    string, we have been emitting a blank line between entries.

    Fix it.

or something, I think.

> -	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 " %d %s", 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 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

And I do agree this makes it much more pleasant than before (not
just this one but all the changes to the tests).

Thanks.

[PATCH v2 3/5] t1405: check for_each_reflog_ent_reverse() more thoroughly

[Han-Wen Nienhuys via GitGitGadget]

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

If we are checking for a certain ordering, we should check that there are two
entries. Do this by mirroring the preceding test.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/t1405-main-ref-store.sh | 1 +
 1 file changed, 1 insertion(+)

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 4/5] test-ref-store: tweaks to for-each-reflog-ent format

[Han-Wen Nienhuys via GitGitGadget]

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

We have some tests that read from files in .git/logs/ hierarchy
when checking if correct reflog entries are created, but that is
too specific to the files backend.  Other backends like reftable
may not store its reflog entries in such a "one line per entry"
format.

Update for-each-reflog-ent test helper to produce output that
is identical to lines in a reflog file files backend uses.
That way, (1) the current tests can be updated to use the test
helper to read the reflog entries instead of (parts of) reflog
files, and perform the same inspection for correctness, and (2)
when the ref backend is swapped to another backend, the updated
test can be used as-is to check the correctness.

Adapt t1400 to use the for-each-reflog-ent test helper.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/helper/test-ref-store.c |  5 +++--
 t/t1400-update-ref.sh     | 13 ++++++++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index ef244aa6b27..2b13d09a0be 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -151,8 +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", oid_to_hex(old_oid),
-	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
+	printf("%s %s %s %" PRItime " %+05d%s%s", oid_to_hex(old_oid),
+	       oid_to_hex(new_oid), committer, timestamp, tz,
+	       *msg == '\n' ? "" : "\t", msg);
 	return 0;
 }
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 0d4f73acaa8..0d46a488032 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -321,8 +321,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
 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)" '
@@ -350,8 +351,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
 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 +469,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

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

[Junio C Hamano]

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

> -	printf("%s %s %s %" PRItime " %d %s", oid_to_hex(old_oid),
> -	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
> +	printf("%s %s %s %" PRItime " %+05d%s%s", oid_to_hex(old_oid),
> +	       oid_to_hex(new_oid), committer, timestamp, tz,
> +	       *msg == '\n' ? "" : "\t", msg);

So, we used to show the timezone as an integer casually, but now we
make sure we use the +/- prefix, with 0 filled 4 digits, that is
consistent with how we record the tz in different places (like the
low-level object header, or the user-visible header lines in commit
and tag objects).

>  	return 0;
>  }
>  
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index 0d4f73acaa8..0d46a488032 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -321,8 +321,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
>  $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
>  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 &&

micronit: lose SP between redirection '>' and its target 'actual'.

> +	test_cmp actual expect

And the +0000 will be checked by comparing with "expect" file.  Nice.

>  '
>  
>  test_expect_success "create $m (logged by config)" '
> @@ -350,8 +351,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
>  $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
>  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 &&

Ditto.

> +	test_cmp actual expect
>  '
>  
>  test_expect_success 'set up for querying the reflog' '
> @@ -467,7 +469,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 &&

This one is good.

> +	test_cmp expect actual
>  '
>  unset h_TEST h_OTHER h_FIXED h_MERGED

Thanks.

[PATCH v2 5/5] refs/debug: trim trailing LF from reflog message

[Han-Wen Nienhuys via GitGitGadget]

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

On iteration, the reflog message is always terminated by a newline. Trim it to
avoid clobbering the console with is this extra newline.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/debug.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index 8667c640237..2631210795b 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -284,15 +284,21 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
 	int ret;
 	char o[GIT_MAX_HEXSZ + 1] = "null";
 	char n[GIT_MAX_HEXSZ + 1] = "null";
+	struct strbuf trimmed = STRBUF_INIT;
 	if (old_oid)
 		oid_to_hex_r(o, old_oid);
 	if (new_oid)
 		oid_to_hex_r(n, new_oid);
 
+	strbuf_addstr(&trimmed, msg);
 	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
 		      dbg->cb_data);
-	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
-		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	strbuf_trim_trailing_newline(&trimmed);
+	trace_printf_key(&trace_refs,
+			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
+			 dbg->refname, ret, o, n, committer,
+			 (long int)timestamp, trimmed.buf);
+	strbuf_release(&trimmed);
 	return ret;
 }
 
-- 
gitgitgadget

Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message

[Junio C Hamano]

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

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> On iteration, the reflog message is always terminated by a newline. Trim it to
> avoid clobbering the console with is this extra newline.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  refs/debug.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/refs/debug.c b/refs/debug.c
> index 8667c640237..2631210795b 100644
> --- a/refs/debug.c
> +++ b/refs/debug.c
> @@ -284,15 +284,21 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
>  	int ret;
>  	char o[GIT_MAX_HEXSZ + 1] = "null";
>  	char n[GIT_MAX_HEXSZ + 1] = "null";
> +	struct strbuf trimmed = STRBUF_INIT;
>  	if (old_oid)
>  		oid_to_hex_r(o, old_oid);
>  	if (new_oid)
>  		oid_to_hex_r(n, new_oid);
>  
> +	strbuf_addstr(&trimmed, msg);
>  	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
>  		      dbg->cb_data);
> -	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
> -		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
> +	strbuf_trim_trailing_newline(&trimmed);

The API promises to have only LF, not CRLF, at the end, so
strbuf_trim_trailing_newline() is a bit overkill (and if payload
happened to end with CR, we would lose it).

> +	trace_printf_key(&trace_refs,
> +			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
> +			 dbg->refname, ret, o, n, committer,
> +			 (long int)timestamp, trimmed.buf);
> +	strbuf_release(&trimmed);
>  	return ret;
>  }

Can we use counted bytes in trace_printf()?  If we can, it would be
simpler to just scan "msg" for LF and then show only the span
between the beginning of the string and the found LF using "%.*s",
perhaps like this?

 refs/debug.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git c/refs/debug.c w/refs/debug.c
index 8667c64023..c97eeb5740 100644
--- c/refs/debug.c
+++ w/refs/debug.c
@@ -284,15 +284,20 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
 	int ret;
 	char o[GIT_MAX_HEXSZ + 1] = "null";
 	char n[GIT_MAX_HEXSZ + 1] = "null";
+	int msglen;
+
 	if (old_oid)
 		oid_to_hex_r(o, old_oid);
 	if (new_oid)
 		oid_to_hex_r(n, new_oid);
 
+	msglen = strchrnul(msg, '\n') - msg;
 	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
 		      dbg->cb_data);
-	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
-		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	trace_printf_key(&trace_refs,
+			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%.*s\"\n",
+			 dbg->refname, ret, o, n, committer, (long int)timestamp,
+			 (int)msglen, msg);
 	return ret;
 }
 

Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message

[Han-Wen Nienhuys]

On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> The API promises to have only LF, not CRLF, at the end, so
> strbuf_trim_trailing_newline() is a bit overkill (and if payload
> happened to end with CR, we would lose it).

it would be best if there was a way to escape characters (ie. "\n" =>
"\\n"). Do we have a function for that?

> > +     trace_printf_key(&trace_refs,
> > +                      "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
> > +                      dbg->refname, ret, o, n, committer,
> > +                      (long int)timestamp, trimmed.buf);
> > +     strbuf_release(&trimmed);
> >       return ret;
> >  }
>
> Can we use counted bytes in trace_printf()?  If we can, it would be
> simpler to just scan "msg" for LF and then show only the span
> between the beginning of the string and the found LF using "%.*s",
> perhaps like this?

I beg to differ - despite this being fewer lines of code, I think
pointer arithmetic is best avoided if possible.

-- 
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 v2 5/5] refs/debug: trim trailing LF from reflog message

[Junio C Hamano]

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

> On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> The API promises to have only LF, not CRLF, at the end, so
>> strbuf_trim_trailing_newline() is a bit overkill (and if payload
>> happened to end with CR, we would lose it).
>
> it would be best if there was a way to escape characters (ie. "\n" =>
> "\\n"). Do we have a function for that?

Mere escaping would not work in a backward compatible way, without a
trick.  It was envisioned that we probably could encode *and* signal
the fact that the message is encoded by appending a trailing SP at
the end of the message.  See the log message of 523fa69c (reflog:
cleanse messages in the refs.c layer, 2020-07-10) for details.

Having said that, that is about introducing a whole new reflog
message format (whose use is signalled by the trailing SP), and I
would prefer it to happen

 (1) after we integrate with reftable, and

 (2) implemented as an option in the normalize_reflog_message()
     function, so that no ref backends has to worry about it.

outside this topic.

> I beg to differ - despite this being fewer lines of code, I think
> pointer arithmetic is best avoided if possible.

I think repeated allocation and deallocation is best avoided, and
that is why I recommended it.  I do not see anything to fear in
poiter arithmetic, as long as it is done clearly (e.g. in a narrow
scope) and correctly.

If trace_printf() does not allow counted bytes "%.*s", the whole
discussion is moot; I didn't go back to check.

Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message

[Junio C Hamano]

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

> Han-Wen Nienhuys <hanwen@google.com> writes:
>
>> On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> The API promises to have only LF, not CRLF, at the end, so
>>> strbuf_trim_trailing_newline() is a bit overkill (and if payload
>>> happened to end with CR, we would lose it).
>>
>> it would be best if there was a way to escape characters (ie. "\n" =>
>> "\\n"). Do we have a function for that?
>
> Mere escaping would not work in a backward compatible way, without a
> trick.  It was envisioned that we probably could encode *and* signal
> the fact that the message is encoded by appending a trailing SP at
> the end of the message.  See the log message of 523fa69c (reflog:
> cleanse messages in the refs.c layer, 2020-07-10) for details.
>
> Having said that, that is about introducing a whole new reflog
> message format (whose use is signalled by the trailing SP), and I
> would prefer it to happen
>
>  (1) after we integrate with reftable, and
>
>  (2) implemented as an option in the normalize_reflog_message()
>      function, so that no ref backends has to worry about it.
>
> outside this topic.

Sorry, I seem to have forgotten to give an answer to your question,
and having warned sufficiently clearly that now is not the time to
do it, I can safely do so.  If we wanted to go the C-quote route,
then the function we would use would be quote.c::quote_c_style().

Even though 523fa69c (reflog: cleanse messages in the refs.c layer,
2020-07-10) muses on use of urlencode, it shouldn't be taken as a
sign that I prefer it over c-quoting, or vice versa for that matter.

It was merely an example encoding picked in the context of
explaining that (1) I wasn't interested in keeping a multi-line
message and spewing back as-is, and (2) but it is possible to loosen
it in the future.

Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message

[Han-Wen Nienhuys]

On Mon, Nov 29, 2021 at 8:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Han-Wen Nienhuys <hanwen@google.com> writes:
>
> > On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> The API promises to have only LF, not CRLF, at the end, so
> >> strbuf_trim_trailing_newline() is a bit overkill (and if payload
> >> happened to end with CR, we would lose it).
> >
> > it would be best if there was a way to escape characters (ie. "\n" =>
> > "\\n"). Do we have a function for that?
>
> Mere escaping would not work in a backward compatible way, without a
> trick.  It was envisioned that we probably could encode *and* signal

I'm talking about the debug output, which isn't subject to
compatibility guarantees.

-- 
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 v2 5/5] refs/debug: trim trailing LF from reflog message

[Junio C Hamano]

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

> On Mon, Nov 29, 2021 at 8:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Han-Wen Nienhuys <hanwen@google.com> writes:
>>
>> > On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>> >>
>> >> The API promises to have only LF, not CRLF, at the end, so
>> >> strbuf_trim_trailing_newline() is a bit overkill (and if payload
>> >> happened to end with CR, we would lose it).
>> >
>> > it would be best if there was a way to escape characters (ie. "\n" =>
>> > "\\n"). Do we have a function for that?
>>
>> Mere escaping would not work in a backward compatible way, without a
>> trick.  It was envisioned that we probably could encode *and* signal
>
> I'm talking about the debug output, which isn't subject to
> compatibility guarantees.

There is no need to show "\n" in the debug output in the first
place, no?  The message part stored in the reflog database is
already normalized to have each runs of whitespaces (including CR
and LF) turned and squashed into a single SP, so the LF at the end
is constant that the debug output can unconditionally strip without
losing information.

Or are we talking about logging the _input_ to be recorded as the
message for a new reflog entry?  Then I agree we can do whatever,
and if you prefer C-style quoting, the helpers in quote.c are your
friends.

Thanks.

Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 29 2021, Han-Wen Nienhuys wrote:

> On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> The API promises to have only LF, not CRLF, at the end, so
>> strbuf_trim_trailing_newline() is a bit overkill (and if payload
>> happened to end with CR, we would lose it).
>
> it would be best if there was a way to escape characters (ie. "\n" =>
> "\\n"). Do we have a function for that?
>
>> > +     trace_printf_key(&trace_refs,
>> > +                      "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
>> > +                      dbg->refname, ret, o, n, committer,
>> > +                      (long int)timestamp, trimmed.buf);
>> > +     strbuf_release(&trimmed);
>> >       return ret;
>> >  }
>>
>> Can we use counted bytes in trace_printf()?  If we can, it would be
>> simpler to just scan "msg" for LF and then show only the span
>> between the beginning of the string and the found LF using "%.*s",
>> perhaps like this?
>
> I beg to differ - despite this being fewer lines of code, I think
> pointer arithmetic is best avoided if possible.

We usually do this with pointer arithmetic, but the %.*s format doesn't
require that, just code like:

    const char *str = "foobar";
    size_t len = strlen(str);
    len -= 1; /* give me less! */
    printf("%.*s", (int)len, str);

So you can also feed it (len - 1) or whatever if you know it to end with
a character you don't want.

It's (more simply done as) pointer arithmetic if you're finding that end
marker with strstr() or whatever, but you can also bend over backwards
and get a "len" instead through other means, and in either case I think
it beats reallocating the whole thing (more for readability than any
optimization reasons).

Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 25 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> This helps for reftable support, and will help if we want to reconsider
> under which conditions reflogs get created/updated.

Having looked at this in a bit more detail than last time
(https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)
I applaud the goals, but to be blunt the specific approach just seems a
bit backwards to me.

As noted in that message I have patches to tweak the "verbose" mode to
be backend-independent, which as we see from your series is one thing in
the files backend that consumes the "message" and assumes things about
newlines.

Similarly from peeking at the reflog code I see it stores name/email
separately in a struct, but the refs.[ch] API wants to pass "name
<email>" as one string.

The below working patch shows that we have three consumers of that
specific format, so we can just xstrfmt() that instead of xstrdup()-ing
it in one case, and tweak the printf in the other two, and then we pass
it as two parameters.

So it's truly backend-independent now, i.e. a "real DB" like reftable,
SQL or whatever would store those in two separate fields.

So on the "seems backwards" above: If you apply this (and it passes all
tests) and look at what "message" is used for you're guaranteed to find
all the callers, and only a handful more of them are using that for
anything.

So I'd really expect a series like this to just change the
strbuf_getwholeline() in files_for_each_reflog_ent() to be a
strbuf_getline(). Then flip everything downstream that expects a '\n' to
either not, or to add if needed.

E.g. the test helper that's now adding a newline would be correct, and
we'd just need to tweak the file backend specific stuff that's now
skipping the addition of newlines to add them instead.

Anyway, changing the callbacks is quite verbose, and I just did this
ident->name/email change expecting that it would be more difficult.

But likewise the big benefit is that we *are* forced to tweak all
callers, so we're more likely to catch any subtle regressions, and it's
thus easier to review once we get past the verbosity.

I also wonder if for name/email/message a bit more tweaking of this to
make them all const char */size_t pairs wouldn't result in a much better
end-state. I.e. now we read into a strbuf in a loop in files-backend.c
and xstrdup() it.

Perhaps reftable is capable of just handing the underlying code pointers
into the mmap()'d file, so we could even skip all (re)allocations? Or if
not, that certainly seems like a sensible thing to anticipate in a
backend-independent interface. We could do that in the file backend if
we were a bit smarter and used mmap() instead of the
fopen()/read-in-a-loop pattern.

Just my 0.02, if you're interested in running with the below assume my
SOB.

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 27b9e78094d..2ccb20ce057 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -487,8 +487,9 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,
 }
 
 static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid,
-		const char *email, timestamp_t timestamp, int tz,
-		const char *message, void *cb_data)
+				  const char *committer_name, const char *committer_email,
+				  timestamp_t timestamp, int tz,
+				  const char *message, void *cb_data)
 {
 	const char *refname = cb_data;
 
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 175c83e7cc2..0cf7d484613 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -295,7 +295,8 @@ static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit
  * Return true iff the specified reflog entry should be expired.
  */
 static int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
-				    const char *email, timestamp_t timestamp, int tz,
+				    const char *committer_name, const char *committer_email,
+				    timestamp_t timestamp, int tz,
 				    const char *message, void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
@@ -659,8 +660,9 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 }
 
 static int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
-		const char *email, timestamp_t timestamp, int tz,
-		const char *message, void *cb_data)
+			    const char *committer_name, const char *committer_email,
+			    timestamp_t timestamp, int tz,
+			    const char *message, void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
 	if (!cb->cmd.expire_total || timestamp < cb->cmd.expire_total)
diff --git a/builtin/stash.c b/builtin/stash.c
index a0ccc8654df..b6963050b6d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -620,7 +620,8 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
 }
 
 static int reject_reflog_ent(struct object_id *ooid, struct object_id *noid,
-			     const char *email, timestamp_t timestamp, int tz,
+			     const char *committer_name, const char *committer_email,
+			     timestamp_t timestamp, int tz,
 			     const char *message, void *cb_data)
 {
 	return 1;
diff --git a/commit.c b/commit.c
index 551de4903c1..ea97f639cc4 100644
--- a/commit.c
+++ b/commit.c
@@ -937,8 +937,9 @@ static void add_one_commit(struct object_id *oid, struct rev_collect *revs)
 }
 
 static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid,
-				  const char *ident, timestamp_t timestamp,
-				  int tz, const char *message, void *cbdata)
+				  const char *author, const char *email,
+				  timestamp_t timestamp, int tz,
+				  const char *message, void *cbdata)
 {
 	struct rev_collect *revs = cbdata;
 
diff --git a/object-name.c b/object-name.c
index fdff4601b2c..56b131fb4a7 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1287,7 +1287,8 @@ struct grab_nth_branch_switch_cbdata {
 };
 
 static int grab_nth_branch_switch(struct object_id *ooid, struct object_id *noid,
-				  const char *email, timestamp_t timestamp, int tz,
+				  const char *author, const char *email,
+				  timestamp_t timestamp, int tz,
 				  const char *message, void *cb_data)
 {
 	struct grab_nth_branch_switch_cbdata *cb = cb_data;
diff --git a/reflog-walk.c b/reflog-walk.c
index 8ac4b284b6b..12865f4cb84 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -20,8 +20,9 @@ struct complete_reflogs {
 };
 
 static int read_one_reflog(struct object_id *ooid, struct object_id *noid,
-		const char *email, timestamp_t timestamp, int tz,
-		const char *message, void *cb_data)
+			   const char *author, const char *email,
+			   timestamp_t timestamp, int tz,
+			   const char *message, void *cb_data)
 {
 	struct complete_reflogs *array = cb_data;
 	struct reflog_info *item;
@@ -30,7 +31,7 @@ static int read_one_reflog(struct object_id *ooid, struct object_id *noid,
 	item = array->items + array->nr;
 	oidcpy(&item->ooid, ooid);
 	oidcpy(&item->noid, noid);
-	item->email = xstrdup(email);
+	item->email = xstrfmt("%s <%s>", author, email);
 	item->timestamp = timestamp;
 	item->tz = tz;
 	item->message = xstrdup(message);
diff --git a/refs.c b/refs.c
index d7cc0a23a3b..8d95fd2a6bb 100644
--- a/refs.c
+++ b/refs.c
@@ -894,8 +894,9 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb,
 }
 
 static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
-		const char *email, timestamp_t timestamp, int tz,
-		const char *message, void *cb_data)
+			   const char *committer_name, const char *committer_email,
+			   timestamp_t timestamp, int tz,
+			   const char *message, void *cb_data)
 {
 	struct read_ref_at_cb *cb = cb_data;
 	int reached_count;
@@ -936,8 +937,9 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
 }
 
 static int read_ref_at_ent_newest(struct object_id *ooid, struct object_id *noid,
-				  const char *email, timestamp_t timestamp,
-				  int tz, const char *message, void *cb_data)
+				  const char *author, const char *committer_email,
+				  timestamp_t timestamp, int tz,
+				  const char *message, void *cb_data)
 {
 	struct read_ref_at_cb *cb = cb_data;
 
@@ -948,8 +950,9 @@ static int read_ref_at_ent_newest(struct object_id *ooid, struct object_id *noid
 }
 
 static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,
-				  const char *email, timestamp_t timestamp,
-				  int tz, const char *message, void *cb_data)
+				  const char *committer_name, const char *committer_email,
+				  timestamp_t timestamp, int tz,
+				  const char *message, void *cb_data)
 {
 	struct read_ref_at_cb *cb = cb_data;
 
diff --git a/refs.h b/refs.h
index d5099d4984e..a34e98a4123 100644
--- a/refs.h
+++ b/refs.h
@@ -463,8 +463,9 @@ int delete_reflog(const char *refname);
  */
 typedef int each_reflog_ent_fn(
 		struct object_id *old_oid, struct object_id *new_oid,
-		const char *committer, timestamp_t timestamp,
-		int tz, const char *msg, void *cb_data);
+		const char *committer_name, const char *committer_email,
+		timestamp_t timestamp, int tz,
+		const char *msg, void *cb_data);
 
 /* Iterate over reflog entries in the log for `refname`. */
 
@@ -807,7 +808,7 @@ typedef void reflog_expiry_prepare_fn(const char *refname,
 				      void *cb_data);
 typedef int reflog_expiry_should_prune_fn(struct object_id *ooid,
 					  struct object_id *noid,
-					  const char *email,
+					  const char *author, const char *email,
 					  timestamp_t timestamp, int tz,
 					  const char *message, void *cb_data);
 typedef void reflog_expiry_cleanup_fn(void *cb_data);
diff --git a/refs/debug.c b/refs/debug.c
index 8667c640237..dc4b417abeb 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -277,8 +277,9 @@ struct debug_reflog {
 
 static int debug_print_reflog_ent(struct object_id *old_oid,
 				  struct object_id *new_oid,
-				  const char *committer, timestamp_t timestamp,
-				  int tz, const char *msg, void *cb_data)
+				  const char *committer, const char *committer_email,
+				  timestamp_t timestamp, int tz,
+				  const char *msg, void *cb_data)
 {
 	struct debug_reflog *dbg = (struct debug_reflog *)cb_data;
 	int ret;
@@ -289,10 +290,11 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
 	if (new_oid)
 		oid_to_hex_r(n, new_oid);
 
-	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
-		      dbg->cb_data);
-	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
-		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	ret = dbg->fn(old_oid, new_oid, committer, committer_email,
+		      timestamp, tz, msg, dbg->cb_data);
+	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s <%s> %ld \"%s\"\n",
+		dbg->refname, ret, o, n, committer, committer_email,
+			 (long int)timestamp, msg);
 	return ret;
 }
 
@@ -374,12 +376,13 @@ static void debug_reflog_expiry_prepare(const char *refname,
 
 static int debug_reflog_expiry_should_prune_fn(struct object_id *ooid,
 					       struct object_id *noid,
-					       const char *email,
+					       const char *committer_name, const char *committer_email,
 					       timestamp_t timestamp, int tz,
 					       const char *message, void *cb_data) {
 	struct debug_reflog_expiry_should_prune *prune = cb_data;
 
-	int result = prune->should_prune(ooid, noid, email, timestamp, tz, message, prune->cb_data);
+	int result = prune->should_prune(ooid, noid, committer_name, committer_email,
+					 timestamp, tz, message, prune->cb_data);
 	trace_printf_key(&trace_refs, "reflog_expire_should_prune: %s %ld: %d\n", message, (long int) timestamp, result);
 	return result;
 }
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 151b0056fe5..d15f3342c31 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1931,7 +1931,10 @@ static int files_delete_reflog(struct ref_store *ref_store,
 static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cb_data)
 {
 	struct object_id ooid, noid;
-	char *email_end, *message;
+	const char *name;
+	char *email;
+	char *email_end;
+	char *message;
 	timestamp_t timestamp;
 	int tz;
 	const char *p = sb->buf;
@@ -1940,6 +1943,8 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
 	if (!sb->len || sb->buf[sb->len - 1] != '\n' ||
 	    parse_oid_hex(p, &ooid, &p) || *p++ != ' ' ||
 	    parse_oid_hex(p, &noid, &p) || *p++ != ' ' ||
+	    !(name = p) ||
+	    !(email = strchr(p, '<')) ||
 	    !(email_end = strchr(p, '>')) ||
 	    email_end[1] != ' ' ||
 	    !(timestamp = parse_timestamp(email_end + 2, &message, 10)) ||
@@ -1948,13 +1953,20 @@ static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *c
 	    !isdigit(message[2]) || !isdigit(message[3]) ||
 	    !isdigit(message[4]) || !isdigit(message[5]))
 		return 0; /* corrupt? */
-	email_end[1] = '\0';
+	/* \0 the SP before "<", marks end of "name"... */
+	*(email - 1) = '\0';
+	/* ... and advance past the opening... "<" */
+	email++;
+	/* ...and stop at the ">" */
+	email_end[0] = '\0';
+
 	tz = strtol(message + 1, NULL, 10);
 	if (message[6] != '\t')
 		message += 6;
 	else
 		message += 7;
-	return fn(&ooid, &noid, p, timestamp, tz, message, cb_data);
+	return fn(&ooid, &noid, name, email, timestamp, tz, message,
+		  cb_data);
 }
 
 static char *find_beginning_of_line(char *bob, char *scan)
@@ -3047,7 +3059,8 @@ struct expire_reflog_cb {
 };
 
 static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
-			     const char *email, timestamp_t timestamp, int tz,
+			     const char *author, const char *email,
+			     timestamp_t timestamp, int tz,
 			     const char *message, void *cb_data)
 {
 	struct expire_reflog_cb *cb = cb_data;
@@ -3056,7 +3069,8 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 	if (cb->flags & EXPIRE_REFLOGS_REWRITE)
 		ooid = &cb->last_kept_oid;
 
-	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
+	if ((*cb->should_prune_fn)(ooid, noid, author, email,
+				   timestamp, tz,
 				   message, policy_cb)) {
 		if (!cb->newlog)
 			printf("would prune %s", message);
@@ -3064,9 +3078,9 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 			printf("prune %s", message);
 	} else {
 		if (cb->newlog) {
-			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
+			fprintf(cb->newlog, "%s %s %s <%s> %"PRItime" %+05d\t%s",
 				oid_to_hex(ooid), oid_to_hex(noid),
-				email, timestamp, tz, message);
+				author, email, timestamp, tz, message);
 			oidcpy(&cb->last_kept_oid, noid);
 		}
 		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
diff --git a/remote.c b/remote.c
index f958543d707..3cd0cdce879 100644
--- a/remote.c
+++ b/remote.c
@@ -2428,8 +2428,9 @@ struct check_and_collect_until_cb_data {
 
 /* Get the timestamp of the latest entry. */
 static int peek_reflog(struct object_id *o_oid, struct object_id *n_oid,
-		       const char *ident, timestamp_t timestamp,
-		       int tz, const char *message, void *cb_data)
+		       const char *committer_name, const char *committer_email,
+		       timestamp_t timestamp, int tz,
+		       const char *message, void *cb_data)
 {
 	timestamp_t *ts = cb_data;
 	*ts = timestamp;
@@ -2438,8 +2439,9 @@ static int peek_reflog(struct object_id *o_oid, struct object_id *n_oid,
 
 static int check_and_collect_until(struct object_id *o_oid,
 				   struct object_id *n_oid,
-				   const char *ident, timestamp_t timestamp,
-				   int tz, const char *message, void *cb_data)
+				   const char *committer_name, const char *committer_email,
+				   timestamp_t timestamp, int tz,
+				   const char *message, void *cb_data)
 {
 	struct commit *commit;
 	struct check_and_collect_until_cb_data *cb = cb_data;
diff --git a/revision.c b/revision.c
index 1981a0859f0..2979f456cb6 100644
--- a/revision.c
+++ b/revision.c
@@ -1597,8 +1597,9 @@ static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)
 }
 
 static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,
-		const char *email, timestamp_t timestamp, int tz,
-		const char *message, void *cb_data)
+				 const char *author, const char *email,
+				 timestamp_t timestamp, int tz,
+				 const char *message, void *cb_data)
 {
 	handle_one_reflog_commit(ooid, cb_data);
 	handle_one_reflog_commit(noid, cb_data);
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index b314b81a45b..727ac008ad8 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -148,12 +148,13 @@ static int cmd_for_each_reflog(struct ref_store *refs, const char **argv)
 }
 
 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)
+		       const char *committer, const char *committer_email,
+		       timestamp_t timestamp, int tz,
+		       const char *msg, void *cb_data)
 {
-	printf("%s %s %s %"PRItime" %d %s\n",
+	printf("%s %s %s <%s> %"PRItime" %d %s\n",
 	       oid_to_hex(old_oid), oid_to_hex(new_oid),
-	       committer, timestamp, tz, msg);
+	       committer, committer_email, timestamp, tz, msg);
 	return 0;
 }
 
diff --git a/wt-status.c b/wt-status.c
index e4f29b2b4c9..a8526a4a100 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -940,7 +940,8 @@ static void wt_longstatus_print_changed(struct wt_status *s)
 }
 
 static int stash_count_refs(struct object_id *ooid, struct object_id *noid,
-			    const char *email, timestamp_t timestamp, int tz,
+			    const char *committer_name, const char *committer_email,
+			    timestamp_t timestamp, int tz,
 			    const char *message, void *cb_data)
 {
 	int *c = cb_data;
@@ -1592,7 +1593,8 @@ struct grab_1st_switch_cbdata {
 };
 
 static int grab_1st_switch(struct object_id *ooid, struct object_id *noid,
-			   const char *email, timestamp_t timestamp, int tz,
+			   const char *committer_name , const char *committer_email,
+			   timestamp_t timestamp, int tz,
 			   const char *message, void *cb_data)
 {
 	struct grab_1st_switch_cbdata *cb = cb_data;

Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests

[Han-Wen Nienhuys]

On Mon, Nov 29, 2021 at 11:14 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > This helps for reftable support, and will help if we want to reconsider
> > under which conditions reflogs get created/updated.
>
> Having looked at this in a bit more detail than last time
> (https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)
> I applaud the goals, but to be blunt the specific approach just seems a
> bit backwards to me.
>
> As noted in that message I have patches to tweak the "verbose" mode to
> be backend-independent, which as we see from your series is one thing in
> the files backend that consumes the "message" and assumes things about
> newlines.

In v2, I went with Jun's suggestion, and left the newlines alone, just
trimming them in refs/debug.c .  I think that makes most of your mail
irrelevant?

> Perhaps reftable is capable of just handing the underlying code pointers
> into the mmap()'d file, so we could even skip all (re)allocations? Or if
> not, that certainly seems like a sensible thing to anticipate in a
> backend-independent interface. We could do that in the file backend if
> we were a bit smarter and used mmap() instead of the
> fopen()/read-in-a-loop pattern.

It sounds like premature optimization. Reading reflogs is not usually
a performance sensitive operation. Also, if you hand out mmap'd
pointers, how would the reftable storage code know when it is safe to
close and munmap the file?

>
> Just my 0.02, if you're interested in running with the below assume my
> SOB.

What is SOB in this context?



-- 
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 v2 0/5] Inspect reflog data programmatically in more tests

[Junio C Hamano]

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

Just this part...

>> Just my 0.02, if you're interested in running with the below assume my
>> SOB.
>
> What is SOB in this context?

"Signed-off-by:"

Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 29 2021, Han-Wen Nienhuys wrote:

> On Mon, Nov 29, 2021 at 11:14 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> > This helps for reftable support, and will help if we want to reconsider
>> > under which conditions reflogs get created/updated.
>>
>> Having looked at this in a bit more detail than last time
>> (https://lore.kernel.org/git/211123.864k83w3y4.gmgdl@evledraar.gmail.com/)
>> I applaud the goals, but to be blunt the specific approach just seems a
>> bit backwards to me.
>>
>> As noted in that message I have patches to tweak the "verbose" mode to
>> be backend-independent, which as we see from your series is one thing in
>> the files backend that consumes the "message" and assumes things about
>> newlines.
>
> In v2, I went with Jun's suggestion, and left the newlines alone, just
> trimming them in refs/debug.c .  I think that makes most of your mail
> irrelevant?

To whatever immediate problem you're trying to solve? Probably. I'm
mainly pointing out that we can make some of these APIs nicer & a bit
more abstract from the details of the file backend.

Or, as is the case with "ident" v.s. "committer_name/committer_email" in
these reflog callbacks no existing caller actually cares about that
format, so if/when we get to that reftable integration (which seems to
have the two as seperate fields) splitting those up in refs.[ch]
probably makes sense.

>> Perhaps reftable is capable of just handing the underlying code pointers
>> into the mmap()'d file, so we could even skip all (re)allocations? Or if
>> not, that certainly seems like a sensible thing to anticipate in a
>> backend-independent interface. We could do that in the file backend if
>> we were a bit smarter and used mmap() instead of the
>> fopen()/read-in-a-loop pattern.
>
> It sounds like premature optimization. Reading reflogs is not usually
> a performance sensitive operation. Also, if you hand out mmap'd
> pointers, how would the reftable storage code know when it is safe to
> close and munmap the file?

The same way we do the fopen/fclose lifetime now, i.e. you could rely on
them for the iteration, which seems to be a common pattern in code that
needs this.

I was aiming more for the lack of premature optimization there,
i.e. instead of byte-twiddling things by injecting \0s we could just
have char */size_t offsets (as noted elsewhere), which would also nicely
allow binary storage formats, if those formats happen to have an
embedded string. Is that true of reftable?

Re: [PATCH v2 0/5] Inspect reflog data programmatically in more tests

[Han-Wen Nienhuys]

On Tue, Nov 30, 2021 at 12:41 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> I was aiming more for the lack of premature optimization there,
> i.e. instead of byte-twiddling things by injecting \0s we could just
> have char */size_t offsets (as noted elsewhere), which would also nicely
> allow binary storage formats, if those formats happen to have an
> embedded string. Is that true of reftable?

reftable zlib compresses the reflog, so you can't mmap strings from the storage.

-- 
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/5] Inspect reflog data programmatically in more tests

[Han-Wen Nienhuys via GitGitGadget]

This helps for reftable support, and will help if we want to reconsider
under which conditions reflogs get created/updated.

v3: pointer arithmetic for trimming newline.

Han-Wen Nienhuys (5):
  show-branch: show reflog message
  test-ref-store: don't add newline to reflog message
  t1405: check for_each_reflog_ent_reverse() more thoroughly
  test-ref-store: tweaks to for-each-reflog-ent format
  refs/debug: trim trailing LF from reflog message

 builtin/show-branch.c          | 12 +++++++-----
 refs/debug.c                   |  7 +++++--
 t/helper/test-ref-store.c      |  6 +++---
 t/t1400-update-ref.sh          | 13 ++++++++-----
 t/t1405-main-ref-store.sh      |  4 ++--
 t/t1406-submodule-ref-store.sh |  4 ++--
 t/t3202-show-branch.sh         | 15 +++++++++++++++
 7 files changed, 42 insertions(+), 19 deletions(-)


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

Range-diff vs v2:

 1:  9d8394d8c76 = 1:  6e94a6fbe05 show-branch: show reflog message
 2:  4a86d212589 ! 2:  062481ffece test-ref-store: don't add newline to reflog message
     @@ Metadata
       ## Commit message ##
          test-ref-store: don't add newline to reflog message
      
     -    The files backend produces a newline for messages automatically, so before we
     -    would print blank lines between entries.
     +    By convention, reflog messages always end in '\n', so
     +    before we would print blank lines between entries.
      
          Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
      
 3:  0319503045b = 3:  46a8e13a5e6 t1405: check for_each_reflog_ent_reverse() more thoroughly
 4:  62f5cb8a824 ! 4:  40ba92dbf0d test-ref-store: tweaks to for-each-reflog-ent format
     @@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 11171502
      -	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-tool ref-store main for-each-reflog-ent $m >actual &&
      +	test_cmp actual expect
       '
       
     @@ t/t1400-update-ref.sh: $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 11171503
      -	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-tool ref-store main for-each-reflog-ent $m >actual &&
      +	test_cmp actual expect
       '
       
 5:  0288e743eb2 ! 5:  751f713a025 refs/debug: trim trailing LF from reflog message
     @@ refs/debug.c: static int debug_print_reflog_ent(struct object_id *old_oid,
       	int ret;
       	char o[GIT_MAX_HEXSZ + 1] = "null";
       	char n[GIT_MAX_HEXSZ + 1] = "null";
     -+	struct strbuf trimmed = STRBUF_INIT;
     ++	char *msgend = strchrnul(msg, '\n');
       	if (old_oid)
       		oid_to_hex_r(o, old_oid);
       	if (new_oid)
     - 		oid_to_hex_r(n, new_oid);
     +@@ refs/debug.c: static int debug_print_reflog_ent(struct object_id *old_oid,
       
     -+	strbuf_addstr(&trimmed, msg);
       	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
       		      dbg->cb_data);
      -	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
      -		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
     -+	strbuf_trim_trailing_newline(&trimmed);
      +	trace_printf_key(&trace_refs,
     -+			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
     ++			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%.*s\"\n",
      +			 dbg->refname, ret, o, n, committer,
     -+			 (long int)timestamp, trimmed.buf);
     -+	strbuf_release(&trimmed);
     ++			 (long int)timestamp, (int)(msgend - msg), msg);
       	return ret;
       }
       

-- 
gitgitgadget

[PATCH v3 1/5] 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 082449293b5..f1e8318592c 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -761,6 +761,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;
 
@@ -770,11 +771,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/5] test-ref-store: don't add newline to reflog message

[Han-Wen Nienhuys via GitGitGadget]

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

By convention, reflog messages always end in '\n', so
before we would print blank lines between entries.

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

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 3986665037a..5ac33dfb598 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -152,9 +152,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 " %d %s", 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 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/5] t1405: check for_each_reflog_ent_reverse() more thoroughly

[Han-Wen Nienhuys via GitGitGadget]

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

If we are checking for a certain ordering, we should check that there are two
entries. Do this by mirroring the preceding test.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/t1405-main-ref-store.sh | 1 +
 1 file changed, 1 insertion(+)

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/5] test-ref-store: tweaks to for-each-reflog-ent format

[Han-Wen Nienhuys via GitGitGadget]

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

We have some tests that read from files in .git/logs/ hierarchy
when checking if correct reflog entries are created, but that is
too specific to the files backend.  Other backends like reftable
may not store its reflog entries in such a "one line per entry"
format.

Update for-each-reflog-ent test helper to produce output that
is identical to lines in a reflog file files backend uses.
That way, (1) the current tests can be updated to use the test
helper to read the reflog entries instead of (parts of) reflog
files, and perform the same inspection for correctness, and (2)
when the ref backend is swapped to another backend, the updated
test can be used as-is to check the correctness.

Adapt t1400 to use the for-each-reflog-ent test helper.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/helper/test-ref-store.c |  5 +++--
 t/t1400-update-ref.sh     | 13 ++++++++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 5ac33dfb598..775e5da5b95 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -152,8 +152,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", oid_to_hex(old_oid),
-	       oid_to_hex(new_oid), committer, timestamp, tz, msg);
+	printf("%s %s %s %" PRItime " %+05d%s%s", oid_to_hex(old_oid),
+	       oid_to_hex(new_oid), committer, timestamp, tz,
+	       *msg == '\n' ? "" : "\t", msg);
 	return 0;
 }
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 0d4f73acaa8..ef8fdcef466 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -321,8 +321,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
 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)" '
@@ -350,8 +351,9 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
 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 +469,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/5] refs/debug: trim trailing LF from reflog message

[Han-Wen Nienhuys via GitGitGadget]

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

On iteration, the reflog message is always terminated by a newline. Trim it to
avoid clobbering the console with is this extra newline.

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 refs/debug.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/refs/debug.c b/refs/debug.c
index 8667c640237..2defaa9c52e 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -284,6 +284,7 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
 	int ret;
 	char o[GIT_MAX_HEXSZ + 1] = "null";
 	char n[GIT_MAX_HEXSZ + 1] = "null";
+	char *msgend = strchrnul(msg, '\n');
 	if (old_oid)
 		oid_to_hex_r(o, old_oid);
 	if (new_oid)
@@ -291,8 +292,10 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
 
 	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
 		      dbg->cb_data);
-	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
-		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	trace_printf_key(&trace_refs,
+			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%.*s\"\n",
+			 dbg->refname, ret, o, n, committer,
+			 (long int)timestamp, (int)(msgend - msg), msg);
 	return ret;
 }
 
-- 
gitgitgadget