Re: [PATCH] refs: allow @{n} to work with n-sized reflog
From: Junio C Hamano <hidden>
Date: 2021-01-06 05:56:30
Denton Liu [off-list ref] 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.
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.OK.
quoted hunk ↗ jump to hunk
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?
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.
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
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; + } + refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb); if (!cb.reccnt) {diff --git a/t/t1503-rev-parse-verify.sh b/t/t1503-rev-parse-verify.sh index dc9fe3cbf1..ed4a366e85 100755 --- a/t/t1503-rev-parse-verify.sh +++ b/t/t1503-rev-parse-verify.sh@@ -86,8 +86,8 @@ test_expect_success 'fails silently when using -q' ' test_expect_success 'fails silently when using -q with deleted reflogs' ' ref=$(git rev-parse HEAD) && git update-ref --create-reflog -m "message for refs/test" refs/test "$ref" && - git reflog delete --updateref --rewrite refs/test@{0} && - test_must_fail git rev-parse -q --verify refs/test@{0} >error 2>&1 && + git reflog delete --updateref --rewrite refs/test@{1} && + test_must_fail git rev-parse -q --verify refs/test@{1} >error 2>&1 && test_must_be_empty error '@@ -139,6 +139,19 @@ test_expect_success 'master@{n} for various n' ' test_must_fail git rev-parse --verify master@{$Np1} ' +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 && + git rev-parse --verify newbranch@{1} +' + +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} +' + test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' ' ln -s does-not-exist .git/refs/heads/broken && test_must_fail git rev-parse --verify broken