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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 30, 2021, 21:14 UTC
Message-ID: <xmqqbl5e3a9h.fsf@gitster.g>
In-Reply-To: <2f11fd7718005d1e94b3139f086134896da202f1.1630334929.git.gitgitgadget@gmail.com>

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

Show 19 quoted lines

> From: Han-Wen Nienhuys <hanwen@google.com>
>
> Before, if we aren't supposed to update reflogs (eg.
> core.logallrefupdates=NONE), we would still write reflog entries if the
> reflog file (.git/logs/REFNAME) existed.
>
> The reftable storage backend cannot distinguish between a non-existing
> reflog, and an empty one. Therefore it cannot mimick this functionality.
>
> In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com,
> we came to the conclusion that this feature is probably a remnant from
> the time that reflogs weren't enabled by default, and it does not need
> to be kept.
>
> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  refs/files-backend.c  | 20 +-------------------
>  t/t1400-update-ref.sh |  7 +++----
>  2 files changed, 4 insertions(+), 23 deletions(-)

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

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

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

Show 35 quoted lines

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index de0cf678f8e..52433f6d0d2 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -269,7 +269,7 @@ test_expect_success "(not) changed .git/$m" '
>  '
>  
>  rm -f .git/logs/refs/heads/main
> -test_expect_success "create $m (logged by touch)" '
> +test_expect_success "create $m" '
>  	test_config core.logAllRefUpdates false &&
>  	GIT_COMMITTER_DATE="2005-05-26 23:30" \
>  	git update-ref --create-reflog HEAD $A -m "Initial Creation" &&
> @@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' '
>  	test_path_is_file .git/logs/HEAD
>  '
>  
> -TAB='	'
>  cat >expect <<EOF
>  $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creation
> -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
> -$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB
>  EOF
> +
>  test_expect_success "verifying $m's log (logged by touch)" '
>  	test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" &&
>  	test-tool ref-store main for-each-reflog-ent $m > actual &&
> @@ -346,6 +344,7 @@ test_expect_success "set $m (logged by config)" '
>  	test $A = $(git show-ref -s --verify $m)
>  '
>  
> +TAB='	'
>  cat >expect <<EOF
>  $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000	Initial Creation
>  $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch

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

THanks.