Re: Segfault: git show-branch --reflog refs/pullreqs/1

From: Jeff King <peff@peff.net>
Date: Feb 22, 2024, 17:22 UTC
Message-ID: <20240222172252.GA3535450@coredump.intra.peff.net>
In-Reply-To: <xmqq34tkiho9.fsf@gitster.g>

On Thu, Feb 22, 2024 at 08:32:06AM -0800, Junio C Hamano wrote:

Show 24 quoted lines

> > Hum, I dunno. I don't really understand what the benefit of this
> > fallback is. If a user wants to know the latest object ID of the ref
> > they shouldn't ask for `foo@{0}`, they should ask for `foo`. On the
> > other hand, if I want to know "What is the latest entry in the ref's
> > log", I want to ask for `foo@{0}`.
> 
> The usability hack helps small things like "List up to 4 most recent
> states from a branch", e.g.
> 
>     for nth in $(seq 0 3)
>     do
> 	git rev-parse --quiet --verify @$nth || break
> 	git show -s --format="@$nth %h %s" @$nth
>     done
> 
> vs
> 
>     for rev in HEAD @{1} @{2} @{3}
>     do
> 	git rev-parse --quiet --verify "$rev" || break
> 	git show -s --format="$rev %h %s" "$rev"
>     done
> 
> by not forcing you to special case the "current".

In those examples, though, it is useful precisely because you _do_ have a reflog, and ref@{0} is conceptually the top entry (which brought us to the same state as just "ref").

The question to me is more "is ref@{0} useful on its own, even when you do not necessarily have a reflog". That I am less sure of.

Show 11 quoted lines

> Ideally, "foo@{0}" should have meant "the state immediately before
> the current state of foo" so that "foo" is the unambiguous and only
> way to refer to "the current state of foo", but that was not how we
> implemented the reflog, allowing a subtle repository corruption
> where the latest state of a branch according to the reflog and the
> current commit pointed by the branch can diverge.  But that wasn't
> what we did, and instead both "foo@{0}" and "foo" mean to refer to
> "the latest state of foo".  We can take advantage of that misdesign
> and allow "foo@{0}" to refer to the same commit as "foo", at least
> at the get_oid_basic() level, whether a reflog actually exists or
> not, and that would make the whole thing more consistent.

I think there is some confusion here between how get_oid_basic() behaves and how read_ref_at() is used for something like show-branch. In the former case, we only care about getting an oid as output, but in the latter we actually want the reflog entry (because we care about its timestamp, message, and so on).

So in terms of reflog entries, ref@{0} should refer to the most recent entry. And the oid it returns should be the end-result of that entry, which (in a non corrupted repository) is identical to the current ref value. And that "should" is reinforced by stuff like:

  git log -g "%gd %gs"

which shows the most recent entry as HEAD@{0}.

I think 6436a20284 (refs: allow @{n} to work with n-sized reflog, 2021-01-07) confused things mightily by having read_ref_at() with a count of "n" find entry "n-1" instead, and then return the oid for the "old" value. That makes get_oid_basic() work, because it doesn't care about which entry we found, only the oid. But for show-branch, now we are confused about which reflog entry ref@{1}, etc, refers to (but ref@{0} still works because of the weird special-casing done by that commit).

I think we should fix that (and I have the start of some patches to do so). But in that world-view, having read_ref_at() return anything for a count of "0" when there is no reflog does not make sense. There is no such entry!

OTOH, we face the same problem when asking about ref@{N} when there are only N entries. We can provide an oid (based on the "old" value from the oldest entry we did see), but we have to "fake" the reflog entry data (like the messsage), since there wasn't one.

So the open questions to me are:

  - should this faking happen in read_ref_at(), just returning a dummy
    reflog message? Or should we keep read_ref_at() purely about finding
    the entry, and put the special-casing into get_oid_basic(), which
    only cares about the oid result?

  - wherever we put the faking, should we only fake ref@{N} when N > 0?
    Or should we also fake ref@{0} when there is no reflog at all?

If none of this makes sense, it is because I am only now untangling what is going on with 6436a20284. ;) I will try to polish my proposed patches and hopefully that will explain it a bit more clearly (I may not get to it until tomorrow though).

-Peff