From: Denton Liu <liu.denton@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] refs: allow @{n} to work with n-sized reflog
Date: Wed, 6 Jan 2021 00:25:49 -0800 [thread overview]
Message-ID: <X/V0DU+CD6mS36dK@generichostname> (raw)
In-Reply-To: <xmqqk0sqvcby.fsf@gitster.c.googlers.com>
Hi Junio,
On Tue, Jan 05, 2021 at 09:55:29PM -0800, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > This sequence works
> >
> > $ git checkout -b newbranch
> > $ git commit --allow-empty -m one
> > $ git show -s newbranch@{1}
> >
> > and shows the state that was immediately after the newbranch was
> > created.
> >
> > 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.
>
> Yeah, I am often hit and irritated by this behaviour.
Yep, this was inspired by one of your emails ;)
> > diff --git a/refs.c b/refs.c
> > index 13dc2c3291..c35c61a009 100644
> > --- 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);
>
> The code treats two cases identically (i.e. the case where cb->cnt
> was originally zero, and one). Is that intended?
It shouldn't be possible for cb->cnt == 0 on the first iteration
because there's a special-case check at [0]. As a result, it can only be
-1 or >= 1 on the first iteration.
The -1 case happens when we're doing date-based lookup and that's what
this if is intended to handle.
In the case where it's >= 1, we will always enter the if and it will
always pre-decrement. This essentially gets us the n-1 behaviour.
> I thought the code was to special case only <ref>@{0}, but with this
> conditional decrement, cb->cnt==0 would not be usable by the rest
> of the code as the "we must read the new side instead" signal. Is
> that why null-ness of ooid is also tested here? It is hard to tell
> the intention because "at_indexed_ent" does not quite tell me what
> the code wants to use the variable for.
The null-ness of the ooid is needed because on the last entry of the
reflog, ooid will be null so we should skip that.
"at_indexed_ent" is meant to signal when we are indexing the reflog
numerically (as opposed to by date), we have arrived at the correct
entry. If you have a more fitting name, I'm open to suggestions.
> > + if (timestamp <= cb->at_time || at_indexed_ent) {
> > if (cb->msg)
> > *cb->msg = xstrdup(message);
> > if (cb->cutoff_time)
> > @@ -905,28 +909,41 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
> > * we have not yet updated cb->[n|o]oid so they still
> > * hold the values for the previous record.
> > */
> > - if (!is_null_oid(&cb->ooid)) {
> > - oidcpy(cb->oid, noid);
> > - if (!oideq(&cb->ooid, noid))
> > - warning(_("log for ref %s has gap after %s"),
> > + if (!is_null_oid(&cb->ooid) && !oideq(&cb->ooid, noid))
> > + warning(_("log for ref %s has gap after %s"),
> > cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
> > - }
> > - else if (cb->date == cb->at_time)
> > + if (at_indexed_ent)
> > + oidcpy(cb->oid, ooid);
> > + else if (!is_null_oid(&cb->ooid) || cb->date == cb->at_time)
> > oidcpy(cb->oid, noid);
> > else if (!oideq(noid, cb->oid))
> > warning(_("log for ref %s unexpectedly ended on %s"),
> > cb->refname, show_date(cb->date, cb->tz,
> > DATE_MODE(RFC2822)));
> > - oidcpy(&cb->ooid, ooid);
> > - oidcpy(&cb->noid, noid);
> > cb->found_it = 1;
> > - return 1;
> > }
> > oidcpy(&cb->ooid, ooid);
> > oidcpy(&cb->noid, noid);
> > - if (cb->cnt > 0)
> > - cb->cnt--;
> > - return 0;
> > + return cb->found_it;
> > +}
> > +
> > +static int read_ref_at_ent_newest(struct object_id *ooid, struct object_id *noid,
> > + const char *email, timestamp_t timestamp,
> > + int tz, const char *message, void *cb_data)
> > +{
> > + struct read_ref_at_cb *cb = cb_data;
> > +
> > + if (cb->msg)
> > + *cb->msg = xstrdup(message);
> > + if (cb->cutoff_time)
> > + *cb->cutoff_time = timestamp;
> > + if (cb->cutoff_tz)
> > + *cb->cutoff_tz = tz;
> > + if (cb->cutoff_cnt)
> > + *cb->cutoff_cnt = cb->reccnt;
> > + oidcpy(cb->oid, noid);
> > + /* We just want the first entry */
> > + return 1;
> > }
>
> The similarity of this to read_ref_at_ent_oldest is somehow
> striking. Do we really need to invent a new callback?
Unfortunately, yes. The alternative is to add a flag into
`struct read_ref_at_cb` and we could conditionally choose whether or not
to copy noid or ooid but this seems like the lesser of two evils.
The duplicated part,
if (cb->msg)
*cb->msg = xstrdup(message);
if (cb->cutoff_time)
*cb->cutoff_time = timestamp;
if (cb->cutoff_tz)
*cb->cutoff_tz = tz;
if (cb->cutoff_cnt)
*cb->cutoff_cnt = cb->reccnt;
is actually repeated three times -- once in each of the callbacks. I
considered extracting factoring it out into a function but I was on the
fence because the function would still have some duplication since it'd
still require cb, message, timestamp and tz to all be passed in.
> > static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,
> > @@ -967,6 +984,11 @@ int read_ref_at(struct ref_store *refs, const char *refname,
> > cb.cutoff_cnt = cutoff_cnt;
> > cb.oid = oid;
> >
> > + if (cb.cnt == 0) {
> > + refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);
> > + return 0;
> > + }
> > +
[0]
> > refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
> >
> > if (!cb.reccnt) {
Thanks,
Denton
next prev parent reply other threads:[~2021-01-06 8:26 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-02 1:36 [PATCH] refs: allow @{n} to work with n-sized reflog Denton Liu
2021-01-02 22:30 ` Martin Ågren
2021-01-03 1:24 ` Denton Liu
2021-01-05 8:52 ` SZEDER Gábor
2021-01-06 5:55 ` Junio C Hamano
2021-01-06 8:25 ` Denton Liu [this message]
2021-01-06 21:02 ` Junio C Hamano
2021-01-06 9:01 ` [PATCH v2 0/2] " Denton Liu
2021-01-06 9:01 ` [PATCH v2 1/2] refs: factor out set_read_ref_cutoffs() Denton Liu
2021-01-06 9:01 ` [PATCH v2 2/2] refs: allow @{n} to work with n-sized reflog Denton Liu
2021-01-06 9:59 ` SZEDER Gábor
2021-01-07 10:36 ` [PATCH v3 0/2] " Denton Liu
2021-01-07 10:36 ` [PATCH v3 1/2] refs: factor out set_read_ref_cutoffs() Denton Liu
2021-01-07 10:36 ` [PATCH v3 2/2] refs: allow @{n} to work with n-sized reflog Denton Liu
2021-01-10 20:31 ` Simon Ruderich
2021-01-12 6:14 ` [PATCH v3] fixup! " Denton Liu
2021-01-12 6:18 ` Denton Liu
2021-01-12 6:27 ` Junio C Hamano
2021-01-10 14:44 ` [PATCH v3 0/2] " SZEDER Gábor
2021-01-10 20:24 ` Junio C Hamano
2021-01-07 10:43 ` [PATCH v3 3/2] fixup! " Denton Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=X/V0DU+CD6mS36dK@generichostname \
--to=liu.denton@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.