"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Han-Wen Nienhuys <hanwen@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx,
> 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@xxxxxxxxxx>
> ---
> 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.
> 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.
