Re: [PATCH] refs: allow @{n} to work with n-sized reflog

From: Martin Ågren <martin.agren@gmail.com>
Date: Jan 2, 2021, 22:30 UTC
Message-ID: <CAN0heSqotLJp3T5XbmeibVcg0RSyqH=69c1tV8-k153-_=CTxg@mail.gmail.com>
In-Reply-To: <0c6885f15f5ce0be28142d9c69724362e72481a9.1609551262.git.liu.denton@gmail.com>

On Sat, 2 Jan 2021 at 02:41, Denton Liu <liu.denton@gmail.com> wrote:

Show 17 quoted lines

>
> But then if you do
>
>         $ git reflog expire --expire=now refs/heads/newbranch
>         $ git commit --allow=empty -m two
>         $ git show -s newbranch@{1}
>
> you'd be scolded with
>
>         fatal: log for 'newbranch' only has 1 entries
>
> While it is true that it has only 1 entry, we have enough
> information in that single entry that records the transition between
> the state in which the tip of the branch was pointing at commit
> 'one' to the new commit 'two' built on it, so we should be able to
> answer "what object newbranch was pointing at?". But we refuse to
> do so.

The basic idea seems to make sense to me...

> Make @{0} the special case where we use the new side to look up that
> entry. Otherwise, look up @{n} using the old side of the (n-1)th entry
> of the reflog.

Show 17 quoted lines

> --- a/refs.c
> +++ b/refs.c
> @@ -887,12 +887,16 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
>                 const char *message, void *cb_data)
>  {
>         struct read_ref_at_cb *cb = cb_data;
> +       int at_indexed_ent;
>
>         cb->reccnt++;
>         cb->tz = tz;
>         cb->date = timestamp;
>
> -       if (timestamp <= cb->at_time || cb->cnt == 0) {
> +       if (cb->cnt > 0)
> +               cb->cnt--;
> +       at_indexed_ent = cb->cnt == 0 && !is_null_oid(ooid);
> +       if (timestamp <= cb->at_time || at_indexed_ent) {

... but I can't really say anything about the implementation.

> +test_expect_success '@{1} works with only one reflog entry' '
> +       git checkout -B newbranch &&
> +       git reflog expire --expire=now refs/heads/newbranch &&
> +       git commit --allow-empty -mexpired &&

Minor nit: not sure about "expired" -- maybe "first after expiration".

> +       git rev-parse --verify newbranch@{1}
> +'

Should this capture the output and compare it to, e.g., `git rev-parse newbranch^`?

Show 5 quoted lines

> +test_expect_success '@{0} works with empty reflog' '
> +       git checkout -B newbranch &&
> +       git reflog expire --expire=now refs/heads/newbranch &&
> +       git rev-parse --verify newbranch@{0}
> +'

Same here, but comparing to `git rev-parse newbranch`? Both of these checks seem worthwhile to make sure that we don't just answer *something*, but that we actually get the right answer, as per your "redefinition".

Speaking of redefinition, does this warrant an update of the documentation? That's a genuine question -- having browsed git-reflog(1) and gitrevisions(7) a bit, I'm not sure.

Martin