Thread (7 messages) 7 messages, 2 authors, 2024-02-26

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

From: Jeff King <hidden>
Date: 2024-02-22 17:22:59

Possibly related (same subject, not in this thread)

On Thu, Feb 22, 2024 at 08:32:06AM -0800, Junio C Hamano wrote:
quoted
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.
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help