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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> > 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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux