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

["Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com>]

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