Junio C Hamano [off-list ref] writes:
Not really. I think the comment on the RFC still stands, and I do
not recall seeing a response to the point.
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.
IIRC, the only reason why reftable implementation may want to change
the behaviour we have to avoid getting blamed for breaking is
because it cannot implement "a reflog exists, and we need to record
further ref movements by appending to it, no matter what the
configuration says" when the existing reflog is empty, because its
data structure lacks support for expressing "exists but empty".
I think the behaviour change described in the title of this message
can be limited in the scope to hurt users a lot less, and can still
satisfy the goal of helping reftable not getting blamed for
breakage, perhaps by making the behaviour for an empty but existing
reflog unspecified or implementation defined per backend.
That would allow us to avoid situation where a user can say foo@{1}
but it does not refer to the point where the branch's tip pointed
just before the most recent update to it. As an empty but existing
reflog would not give foo@{$n} for any value of $n, there is much
less risk of confusing users if we did not append new entries to it,
I would hope.
On Thu, Oct 7, 2021 at 7:38 PM Junio C Hamano [off-list ref] wrote:
quoted
Not really. I think the comment on the RFC still stands, and I do
not recall seeing a response to the point.
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.
A git 'reflog disable' command would address your concerns, but it is
a destructive operation, so the cure might be worse than the solution.
IIRC, the only reason why reftable implementation may want to change
the behaviour we have to avoid getting blamed for breaking is
because it cannot implement "a reflog exists, and we need to record
further ref movements by appending to it, no matter what the
configuration says" when the existing reflog is empty, because its
data structure lacks support for expressing "exists but empty".
I think the behaviour change described in the title of this message
can be limited in the scope to hurt users a lot less, and can still
satisfy the goal of helping reftable not getting blamed for
breakage, perhaps by making the behaviour for an empty but existing
reflog unspecified or implementation defined per backend.
If we accept implementation-dependent features, we could just leave
the whole feature as is. I had expected more breakage, but there is
only one test case in t1400 that needs addressing. If the test
coverage reflects the popularity of the feature, it should be fine to
leave this divergence in, and mark the test with REFFILES.
The commits prior to the RFC should be OK for committing. In
particular, there is a bugfix for the show-branch command. Should I
resend those separately?
--
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
On Thu, Nov 11 2021, Han-Wen Nienhuys wrote:
On Thu, Oct 7, 2021 at 7:38 PM Junio C Hamano [off-list ref] wrote:
quoted
quoted
Not really. I think the comment on the RFC still stands, and I do
not recall seeing a response to the point.
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.
A git 'reflog disable' command would address your concerns, but it is
a destructive operation, so the cure might be worse than the solution.
quoted
IIRC, the only reason why reftable implementation may want to change
the behaviour we have to avoid getting blamed for breaking is
because it cannot implement "a reflog exists, and we need to record
further ref movements by appending to it, no matter what the
configuration says" when the existing reflog is empty, because its
data structure lacks support for expressing "exists but empty".
I think the behaviour change described in the title of this message
can be limited in the scope to hurt users a lot less, and can still
satisfy the goal of helping reftable not getting blamed for
breakage, perhaps by making the behaviour for an empty but existing
reflog unspecified or implementation defined per backend.
If we accept implementation-dependent features, we could just leave
the whole feature as is. I had expected more breakage, but there is
only one test case in t1400 that needs addressing. If the test
coverage reflects the popularity of the feature, it should be fine to
leave this divergence in, and mark the test with REFFILES.
The commits prior to the RFC should be OK for committing. In
particular, there is a bugfix for the show-branch command. Should I
resend those separately?
I've got some follow-up patches to what's sitting in "next" already that
hoist some reffiles-specific stuff into builtin/reflog.c, I haven't
tested but I expect that the behavior change is silent now in the
reftable backend, i.e. it doesn't implement progress/verbose the same
way, presumably.
Between that and 5ac15ad2509 (reflog tests: add --updateref tests,
2021-10-16) & 52106430dc8 (refs/files: remove "name exist?" check in
lock_ref_oid_basic(), 2021-10-16) I wouldn't put too much faith in those
reflog tests.
None of that should be a blocker for your series landing, just say'n. I
don't trust those tests.
IMO the only meaningful way to be confident in testing these sorts of
things with reftable is more of the chaos monkey approach of the
GIT_TEST_* modes, i.e. we now have a WIP mode to do that for reftable
that has known breakages.
We could similarly instrument the test suite to do "git reflog expire"
for each ref at the end of tests, a bunch of things would break, but we
could log the complete -V run and see if what breaks is different under
the two backends.
I've got some WIP patches to add a similar chaos mode using "git gc
--auto", and it turned up some interesting stuff. It's what I used
initially to test what's now landed in ae35e16cd43 (reflog expire: don't
lock reflogs using previously seen OID, 2021-08-23).