Re: [PATCH 2/3] t1405: mark test that checks existence as REFFILES

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2022, 21:03 UTC
Message-ID: <xmqqa6facn9i.fsf@gitster.g>
In-Reply-To: <CAFQ2z_OFRJh9cwxnbDzrshYPGOvJC6Rz1eHTF-aKURno+41Cvw@mail.gmail.com>

Han-Wen Nienhuys <hanwen@google.com> writes:

Show 6 quoted lines

>> Because there is no generic reflog API that says "enable log for
>> this ref", a test that checks this feature with files backend would
>> do "touch .git/refs/heads/frotz".
>
> There is refs_create_reflog(), so the generic reflog API exists. The
> problem is that there is no sensible way to implement it in reftable.

Ah, yes, that's correct.

> One option is (reflog exists == there exists at least one reflog entry
> for the ref).

Because the current callers of refs_create_reflog() does want a reflog created that does not give any entry when iterated, I agree with you that adding a "fake" reflog entry alone is not a sufficient emulation of the API. I think these are all ...

Show 15 quoted lines

> This messes up the test from this patch, because it
> creates a reflog, but because it doesn't populate the reflog, so we
> return false for git-reflog-exists.
>
> It also turns out to mess up the tests in t3420, as follows:
>
> ++ git stash show -p
> error: refs/stash@{0} is not a valid reference
>
> I get
>
>   reflog_exists: refs/stash: 0
>
> and "git stash show -p" aborts with "error: refs/stash@{0} is not a
> valid reference".

... indications of hat.

I wonder if it is simple and easy to add a new reflog entry type used as an implementation detail of the reftable. If we can do so, then, the reftable backend integrated to the ref API can do these things:

 - reflog_exists() can say yes when one reflog entry of any type
   (internal to the reftable implementation) exists for the ref;

 - create_reflog() can add a reflog entry of the "fake" type
   (internal to the reftable implementation);

 - for_each_reflog_ent() and its reverse can learn to skip such a
   fake reflog entry.

As there is no way to ask, via the API, the number of the existing reflog entries, the ref API callers would not be able to tell such an implementation detail that with reftable backend, create_reflog() does not create an empty reflog. To them, a reflog created with the API call would truly be empty as iterators will not return anything.

Or do we have a list of refs kept somewhere in the reftable data structure in a separate chunk? Do we have a bit for each of these refs to record if the log is enabled for it? Then instead of the fake reflog entry, we could implement the necessary semantics a lot more cleanly:

 - reflog_exists() can just peek the bit.

 - create_reflog() can just flip the bit.

 - there is no need to touch the iterators.

 - the equivalent to files_log_ref_write() can decide based on the
   bit (i.e. what reflog_exists() says) whether to log changes to
   the ref.

It is probably a lot more sensible to fail refs_create_reflog() and safe_create_reflog() (which is a thin wrapper around the former), if we cannot implement "a reflog can exist and have no entries yet" semantics.

Outside the test helper, the only place the helper is used is "checkout -l" when should_autocreate_reflog() returns false, which should be rare (as we are updating a branch, it should be either a detached HEAD or a ref under refs/heads/), so it would not be a huge practical downside if we cannot prepare an empty reflog anyway, I would think.

Thanks.