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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Aug 30, 2021 at 02:48:48PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Before, if we aren't supposed to update reflogs (eg.
> core.logallrefupdates=NONE), we would still write reflog entries if the
> reflog file (.git/logs/REFNAME) existed.
>
> The reftable storage backend cannot distinguish between a non-existing
> reflog, and an empty one. Therefore it cannot mimick this functionality.
>
> In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,

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

> we came to the conclusion that this feature is probably a remnant from
> the time that reflogs weren't enabled by default, and it does not need
> to be kept.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  refs/files-backend.c  | 20 +-------------------
>  t/t1400-update-ref.sh |  7 +++----
>  2 files changed, 4 insertions(+), 23 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index e05ada9286d..25f5a4ce777 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1551,6 +1551,7 @@ static int log_ref_setup(struct files_ref_store *refs,
>  	struct strbuf logfile_sb = STRBUF_INIT;
>  	char *logfile;
>
> +	*logfd = -1;
>  	files_reflog_path(refs, &logfile_sb, refname);
>  	logfile = strbuf_detach(&logfile_sb, NULL);
>
> @@ -1565,26 +1566,8 @@ static int log_ref_setup(struct files_ref_store *refs,
>  			else
>  				strbuf_addf(err, "unable to append to '%s': %s",
>  					    logfile, strerror(errno));
> -
>  			goto error;
>  		}
> -	} else {
> -		*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);
> -		if (*logfd < 0) {
> -			if (errno == ENOENT || errno == EISDIR) {
> -				/*
> -				 * The logfile doesn't already exist,
> -				 * but that is not an error; it only
> -				 * means that we won't write log
> -				 * entries to it.
> -				 */
> -				;
> -			} else {
> -				strbuf_addf(err, "unable to append to '%s': %s",
> -					    logfile, strerror(errno));
> -				goto error;
> -			}
> -		}
>  	}
>
>  	if (*logfd >= 0)

I'm nit-picking, but I think that this conditional could probably be
moved into the if-statement above, since we no longer set logfd anywhere
else. It might read more clearly, and if you're going to resubmit this
series anyway, it would be helpful to pick that up.
> @@ -1592,7 +1575,6 @@ static int log_ref_setup(struct files_ref_store *refs,
>
>  	free(logfile);
>  	return 0;
> -
>  error:
>  	free(logfile);
>  	return -1;
> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index de0cf678f8e..52433f6d0d2 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -269,7 +269,7 @@ test_expect_success "(not) changed .git/$m" '
>  '
>
>  rm -f .git/logs/refs/heads/main
> -test_expect_success "create $m (logged by touch)" '
> +test_expect_success "create $m" '
>  	test_config core.logAllRefUpdates false &&
>  	GIT_COMMITTER_DATE="2005-05-26 23:30" \
>  	git update-ref --create-reflog HEAD $A -m "Initial Creation" &&
> @@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' '
>  	test_path_is_file .git/logs/HEAD
>  '
>
> -TAB='	'
>  cat >expect <<EOF
>  $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation

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

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

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

Thanks,
Taylor