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

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

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

v2 addresses all outstanding comments.

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

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

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


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

Range-diff vs v1:

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

-- 
gitgitgadget