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$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox