From patchwork Mon Feb 26 10:02:26 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13571789 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 78C7E1DDEE for ; Mon, 26 Feb 2024 10:02:28 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708941750; cv=none; b=CgoAOCsGPQjfPwvewBNzEr9A3wuMwCW3CM3IfBnquXWT+K5PzguEbnLIgl4XGms17fBSX0EE4YpjMugAarjfClceeoa9KtADAp1U5pQu/WtXgGxaJ2iPRF4P3RhEsi1vGQhqqyFun+iS4gIqfb1imZwoBWUQ/I7Q2J4TixeTKec= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708941750; c=relaxed/simple; bh=qlR3O3C4XFiY600c0vhJ7nFu4TrBblodSA1AdNM06Go=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=et0T+kFMhpC4HG5XR7sk6JvMsLmf0AmldNnFpqZFqgkUZyrYTeNw6VstwmKndiBNBfbPNXoUG0s6cD0sRIrEHprQC9fXT6rJPovREWKM+AhqeLBLQ93sT+4+lVs6NX6Z2GfCn57pbozakkZUevHG/Gz/kcMZ8Xe3jwcPfjuVe+w= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 21893 invoked by uid 109); 26 Feb 2024 10:02:27 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 26 Feb 2024 10:02:27 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 14096 invoked by uid 111); 26 Feb 2024 10:02:32 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 26 Feb 2024 05:02:32 -0500 Authentication-Results: peff.net; auth=none Date: Mon, 26 Feb 2024 05:02:26 -0500 From: Jeff King To: Junio C Hamano Cc: Patrick Steinhardt , Yasushi SHOJI , Denton Liu , Git Mailing List Subject: [PATCH 1/3] Revert "refs: allow @{n} to work with n-sized reflog" Message-ID: <20240226100226.GA2685600@coredump.intra.peff.net> References: <20240226100010.GA1214708@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240226100010.GA1214708@coredump.intra.peff.net> This reverts commit 6436a20284f33d42103cac93bd82e65bebb31526. The idea of that commit is that if read_ref_at() is counting back to the Nth reflog but the reflog is short by one entry (e.g., because it was pruned), we can find the oid of the missing entry by looking at the "before" oid value of the entry that comes after it (whereas before, we looked at the "after" value of each entry and complained that we couldn't find the one from before the truncation). This works fine for resolving the oid of ref@{n}, as it is used by get_oid_basic(), which does not look at any other aspect of the reflog we found (e.g., its timestamp or message). But there's another caller of read_ref_at(): in show-branch we use it to walk over the reflog, and we do care about the reflog entry. And so that commit broke "show-branch --reflog"; it shows the reflog message for ref@{0} as ref@{1}, ref@{1} as ref@{2}, and so on. For example, in the new test in t3202 we produce: ! [branch@{0}] (0 seconds ago) commit: three ! [branch@{1}] (0 seconds ago) commit: three ! [branch@{2}] (60 seconds ago) commit: two ! [branch@{3}] (2 minutes ago) reset: moving to HEAD^ instead of the correct: ! [branch@{0}] (0 seconds ago) commit: three ! [branch@{1}] (60 seconds ago) commit: two ! [branch@{2}] (2 minutes ago) reset: moving to HEAD^ ! [branch@{3}] (2 minutes ago) commit: one But there's another bug, too: because it is looking at the "old" value of the reflog after the one we're interested in, it has to special-case ref@{0} (since there isn't anything after it). That's why it doesn't show the offset bug in the output above. But this special-case code fails to handle the situation where the reflog is empty or missing; it returns success even though the reflog message out-parameter has been left uninitialized. You can't trigger this through get_oid_basic(), but "show-branch --reflog" will pretty reliably segfault as it tries to access the garbage pointer. Fixing the segfault would be pretty easy. But the off-by-one problem is inherent in this approach. So let's start by reverting the commit to give us a clean slate to work with. This isn't a pure revert; all of the code changes are reverted, but for the tests: 1. We'll flip the cases in t1508 to expect_failure; making these work was the goal of 6436a2028, and we'll want to use them for our replacement approach. 2. There's a test in t3202 for "show-branch --reflog", but it expects the broken output! It was added by f2463490c4 (show-branch: show reflog message, 2021-12-02) which was fixing another bug, and I think the author simply didn't notice that the second line showed the wrong reflog. Rather than fixing that test, let's replace it with one that is more thorough (while still covering the reflog message fix from that commit). We'll use a longer reflog, which lets us see more entries (thus making the "off by one" pattern much more clear). And we'll use a more recent timestamp for "now" so that our relative dates have more resolution. That lets us see that the reflog dates are correct (whereas when you are 4 years away, two entries that are 60 seconds apart will have the same "4 years ago" relative date). Because we're adjusting the repository state, I've moved this new test to the end of the script, leaving the other tests undisturbed. We'll also add a new test which covers the missing reflog case; previously it segfaulted, but now it reports the empty reflog). Reported-by: Yasushi SHOJI Signed-off-by: Jeff King --- refs.c | 48 +++++++++++-------------------------- t/t1508-at-combinations.sh | 4 ++-- t/t3202-show-branch.sh | 49 ++++++++++++++++++++++++++------------ 3 files changed, 50 insertions(+), 51 deletions(-) diff --git a/refs.c b/refs.c index c633abf284..ba1a4db754 100644 --- a/refs.c +++ b/refs.c @@ -1038,55 +1038,40 @@ 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 reached_count; cb->tz = tz; cb->date = timestamp; - /* - * It is not possible for cb->cnt == 0 on the first iteration because - * that special case is handled in read_ref_at(). - */ - if (cb->cnt > 0) - cb->cnt--; - reached_count = cb->cnt == 0 && !is_null_oid(ooid); - if (timestamp <= cb->at_time || reached_count) { + if (timestamp <= cb->at_time || cb->cnt == 0) { set_read_ref_cutoffs(cb, timestamp, tz, message); /* * 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) && !oideq(&cb->ooid, noid)) - warning(_("log for ref %s has gap after %s"), + if (!is_null_oid(&cb->ooid)) { + oidcpy(cb->oid, noid); + if (!oideq(&cb->ooid, noid)) + warning(_("log for ref %s has gap after %s"), cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822))); - if (reached_count) - oidcpy(cb->oid, ooid); - else if (!is_null_oid(&cb->ooid) || cb->date == cb->at_time) + } + else if (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))); + cb->reccnt++; + oidcpy(&cb->ooid, ooid); + oidcpy(&cb->noid, noid); cb->found_it = 1; + return 1; } cb->reccnt++; oidcpy(&cb->ooid, ooid); oidcpy(&cb->noid, noid); - return cb->found_it; -} - -static int read_ref_at_ent_newest(struct object_id *ooid UNUSED, - struct object_id *noid, - const char *email UNUSED, - timestamp_t timestamp, int tz, - const char *message, void *cb_data) -{ - struct read_ref_at_cb *cb = cb_data; - - set_read_ref_cutoffs(cb, timestamp, tz, message); - oidcpy(cb->oid, noid); - /* We just want the first entry */ - return 1; + if (cb->cnt > 0) + cb->cnt--; + return 0; } static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid, @@ -1121,11 +1106,6 @@ 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/t1508-at-combinations.sh b/t/t1508-at-combinations.sh index e841309d0e..3e5f32f604 100755 --- a/t/t1508-at-combinations.sh +++ b/t/t1508-at-combinations.sh @@ -103,14 +103,14 @@ test_expect_success 'create path with @' ' check "@:normal" blob content check "@:fun@ny" blob content -test_expect_success '@{1} works with only one reflog entry' ' +test_expect_failure '@{1} works with only one reflog entry' ' git checkout -B newbranch main && git reflog expire --expire=now refs/heads/newbranch && git commit --allow-empty -m "first after expiration" && test_cmp_rev newbranch~ newbranch@{1} ' -test_expect_success '@{0} works with empty reflog' ' +test_expect_failure '@{0} works with empty reflog' ' git checkout -B newbranch main && git reflog expire --expire=now refs/heads/newbranch && test_cmp_rev newbranch newbranch@{0} diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh index 6a98b2df76..35f35f8091 100755 --- a/t/t3202-show-branch.sh +++ b/t/t3202-show-branch.sh @@ -4,9 +4,6 @@ test_description='test show-branch' . ./test-lib.sh -# arbitrary reference time: 2009-08-30 19:20:00 -GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW - test_expect_success 'error descriptions on empty repository' ' current=$(git branch --show-current) && cat >expect <<-EOF && @@ -187,18 +184,6 @@ test_expect_success 'show branch --merge-base with N arguments' ' test_cmp expect actual ' -test_expect_success 'show branch --reflog=2' ' - sed "s/^> //" >expect <<-\EOF && - > ! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10 - > ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10 - > -- - > + [refs/heads/branch10@{0}] branch10 - > ++ [refs/heads/branch10@{1}] initial - EOF - git show-branch --reflog=2 >actual && - test_cmp actual expect -' - # incompatible options while read combo do @@ -264,4 +249,38 @@ test_expect_success 'error descriptions on orphan branch' ' test_branch_op_in_wt -c new-branch ' +test_expect_success 'setup reflogs' ' + test_commit base && + git checkout -b branch && + test_commit one && + git reset --hard HEAD^ && + test_commit two && + test_commit three +' + +test_expect_success '--reflog shows reflog entries' ' + cat >expect <<-\EOF && + ! [branch@{0}] (0 seconds ago) commit: three + ! [branch@{1}] (60 seconds ago) commit: two + ! [branch@{2}] (2 minutes ago) reset: moving to HEAD^ + ! [branch@{3}] (2 minutes ago) commit: one + ---- + + [branch@{0}] three + ++ [branch@{1}] two + + [branch@{3}] one + ++++ [branch@{2}] base + EOF + # the output always contains relative timestamps; use + # a known time to get deterministic results + GIT_TEST_DATE_NOW=$test_tick \ + git show-branch --reflog branch >actual && + test_cmp expect actual +' + +test_expect_success '--reflog handles missing reflog' ' + git reflog expire --expire=now branch && + test_must_fail git show-branch --reflog branch 2>err && + grep "log .* is empty" err +' + test_done From patchwork Mon Feb 26 10:04:07 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13571792 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9CC41537E4 for ; Mon, 26 Feb 2024 10:04:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708941850; cv=none; b=p5yNlZPHLnxcglV4HQybKIsjjQp1ohSI0Pz14K1dtB3gH9m58FfsCvvVaAnCs/JvaexwU2dp6giuNuOVHbwL6AXkuG95pwyZLsgKZrAXuN4J1lyext6nTB/iYDu5Is+3BowA2hgxomT3GaYaVkf5hJdAlM46uJR0AvSzeSAOATc= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708941850; c=relaxed/simple; bh=aq1Qqz1w3CrFhAbt+aubuW+xpd0CWmpgGp8dOdFWsh0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=tbL3eBndREa9jGOBmHbs8qGXf1rm8UHYZEXwLub2mMMN3XcBYyHpNioUhFoytXZwB7EP+NAh5pjEnfjUDPVBpY4U10kVvAIc3SvDcyOKr/fK51oYMMm7qeVwmNL1bshEz1uAjwy5C0qUMKdXR691YNNz8Fe6vjyQnjlDCZSGr78= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 21928 invoked by uid 109); 26 Feb 2024 10:04:07 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 26 Feb 2024 10:04:07 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 14376 invoked by uid 111); 26 Feb 2024 10:04:12 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 26 Feb 2024 05:04:12 -0500 Authentication-Results: peff.net; auth=none Date: Mon, 26 Feb 2024 05:04:07 -0500 From: Jeff King To: Junio C Hamano Cc: Patrick Steinhardt , Yasushi SHOJI , Denton Liu , Git Mailing List Subject: [PATCH 2/3] get_oid_basic(): special-case ref@{n} for oldest reflog entry Message-ID: <20240226100407.GB2685600@coredump.intra.peff.net> References: <20240226100010.GA1214708@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240226100010.GA1214708@coredump.intra.peff.net> The goal of 6436a20284 (refs: allow @{n} to work with n-sized reflog, 2021-01-07) was that if we have "n" entries in a reflog, we should still be able to resolve ref@{n} by looking at the "old" value of the oldest entry. Commit 6436a20284 tried to put the logic into read_ref_at() by shifting its idea of "n" by one. But we reverted that in the previous commit, since it led to bugs in other callers which cared about the details of the reflog entry we found. Instead, let's put the special case into the caller that resolves @{n}, as it cares only about the oid. read_ref_at() is even kind enough to return the "old" value from the final reflog; it just returns "1" to signal to us that we ran off the end of the reflog. But we can notice in the caller that we read just enough records for that "old" value to be the one we're looking for, and use it. Note that read_ref_at() could notice this case, too, and just return 0. But we don't want to do that, because the caller must be made aware that we only found the oid, not an actual reflog entry (and the call sites in show-branch do care about this). There is one complication, though. When read_ref_at() hits a truncated reflog, it will return the "old" value of the oldest entry only if it is not the null oid. Otherwise, it actually returns the "new" value from that entry! This bit of fudging is due to d1a4489a56 (avoid null SHA1 in oldest reflog, 2008-07-08), where asking for "ref@{20.years.ago}" for a ref created recently will produce the initial value as a convenience (even though technically it did not exist 20 years ago). But this convenience is only useful for time-based cutoffs. For count-based cutoffs, get_oid_basic() has always simply complained about going too far back: $ git rev-parse HEAD@{20} fatal: log for 'HEAD' only has 16 entries and we should continue to do so, rather than returning a nonsense value (there's even a test in t1508 already which covers this). So let's have the d1a4489a56 code kick in only when doing timestamp-based cutoffs. Signed-off-by: Jeff King --- object-name.c | 9 +++++++++ refs.c | 2 +- t/t1508-at-combinations.sh | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/object-name.c b/object-name.c index 3a2ef5d680..511f09bc0f 100644 --- a/object-name.c +++ b/object-name.c @@ -1034,6 +1034,15 @@ static int get_oid_basic(struct repository *r, const char *str, int len, len, str, show_date(co_time, co_tz, DATE_MODE(RFC2822))); } + } else if (nth == co_cnt && !is_null_oid(oid)) { + /* + * We were asked for the Nth reflog (counting + * from 0), but there were only N entries. + * read_ref_at() will have returned "1" to tell + * us it did not find an entry, but it did + * still fill in the oid with the "old" value, + * which we can use. + */ } else { if (flags & GET_OID_QUIETLY) { exit(128); diff --git a/refs.c b/refs.c index ba1a4db754..6b826b002e 100644 --- a/refs.c +++ b/refs.c @@ -1083,7 +1083,7 @@ static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid set_read_ref_cutoffs(cb, timestamp, tz, message); oidcpy(cb->oid, ooid); - if (is_null_oid(cb->oid)) + if (cb->at_time && is_null_oid(cb->oid)) oidcpy(cb->oid, noid); /* We just want the first entry */ return 1; diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh index 3e5f32f604..370bf7137e 100755 --- a/t/t1508-at-combinations.sh +++ b/t/t1508-at-combinations.sh @@ -103,7 +103,7 @@ test_expect_success 'create path with @' ' check "@:normal" blob content check "@:fun@ny" blob content -test_expect_failure '@{1} works with only one reflog entry' ' +test_expect_success '@{1} works with only one reflog entry' ' git checkout -B newbranch main && git reflog expire --expire=now refs/heads/newbranch && git commit --allow-empty -m "first after expiration" && From patchwork Mon Feb 26 10:08:03 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff King X-Patchwork-Id: 13571793 Received: from cloud.peff.net (cloud.peff.net [104.130.231.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E33F753394 for ; Mon, 26 Feb 2024 10:08:04 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=104.130.231.41 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708942086; cv=none; b=B1p38JydGWJEqxXVRvttZZ4+3+4cA0PIknHPnekyIeI4mgGwrG9xasH3bDMKekBEHv7doVBYnpXECi3/lgVyMelLRb0awAUG8taMGoY5TlwU+/RXvMeVOH5H2bncBRq8pGXwACgl6yNzxzdyPqQfnXA6dBGHxfQn/2FqQbMVx8A= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708942086; c=relaxed/simple; bh=L1US0GykuobW6uDsTNjpjMKN2e/FEFC3W6eyUFFNZ0U=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Ixww201urEv5dN2U/wgYN2txEeSYIG1FD1+m4s2UtM2oj7SFBNmIEH0twPUNL5v2/iIm0B1SN0OeKa8RqOWU0ZUEYNJ1k1fGQpj/fd16xVOMXi2m/Fc/MrxLBuGIVnFJKsyIev78l0MndkxmcXorHzFwteS3IMtcC1mmmyN/J1A= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net; spf=pass smtp.mailfrom=peff.net; arc=none smtp.client-ip=104.130.231.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=peff.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=peff.net Received: (qmail 21995 invoked by uid 109); 26 Feb 2024 10:08:04 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 26 Feb 2024 10:08:04 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 15064 invoked by uid 111); 26 Feb 2024 10:08:08 -0000 Received: from coredump.intra.peff.net (HELO coredump.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 26 Feb 2024 05:08:08 -0500 Authentication-Results: peff.net; auth=none Date: Mon, 26 Feb 2024 05:08:03 -0500 From: Jeff King To: Junio C Hamano Cc: Patrick Steinhardt , Yasushi SHOJI , Denton Liu , Git Mailing List Subject: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog Message-ID: <20240226100803.GC2685600@coredump.intra.peff.net> References: <20240226100010.GA1214708@coredump.intra.peff.net> Precedence: bulk X-Mailing-List: git@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240226100010.GA1214708@coredump.intra.peff.net> The previous commit special-cased get_oid_basic()'s handling of ref@{n} for a reflog with n entries. But its special case doesn't work for ref@{0} in an empty reflog, because read_ref_at() dies when it notices the empty reflog! We can make this work by special-casing this in read_ref_at(). It's somewhat gross, for two reasons: 1. We have no reflog entry to describe in the "msg" out-parameter. So we have to leave it uninitialized or make something up. 2. Likewise, we have no oid to put in the "oid" out-parameter. Leaving it untouched is actually the best thing here, as all of the callers will have initialized it with the current ref value via repo_dwim_log(). This is rather subtle, but it is how things worked in 6436a20284 (refs: allow @{n} to work with n-sized reflog, 2021-01-07) before we reverted it. The key difference from 6436a20284 here is that we'll return "1" to indicate that we _didn't_ find the requested reflog entry. Coupled with the special-casing in get_oid_basic() in the previous commit, that's enough to make looking up ref@{0} work, and we can flip 6436a20284's test back to expect_success. It also means that the call in show-branch which segfaulted with 6436a20284 (and which is now tested in t3202) remains OK. The caller notices that we could not find any reflog entry, and so it breaks out of its loop, showing nothing. This is different from the current behavior of producing an error, but it's just as reasonable (and is exactly what we'd do if you asked it to walk starting at ref@{1} but there was only 1 entry). Thus nobody should actually look at the reflog entry info we return. But we'll still put in some fake values just to be on the safe side, since this is such a subtle and confusing interface. Likewise, we'll document what's going on in a comment above the function declaration. If this were a function with a lot of callers, the footgun would probably not be worth it. But it has only ever had two callers in its 18-year existence, and it seems unlikely to grow more. So let's hold our noses and let users enjoy the convenience of a simulated ref@{0}. Signed-off-by: Jeff King --- refs.c | 15 +++++++++++++++ refs.h | 15 ++++++++++++++- t/t1508-at-combinations.sh | 2 +- t/t3202-show-branch.sh | 4 ++-- 4 files changed, 32 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index 6b826b002e..8eaec5eca7 100644 --- a/refs.c +++ b/refs.c @@ -1109,6 +1109,21 @@ int read_ref_at(struct ref_store *refs, const char *refname, refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb); if (!cb.reccnt) { + if (cnt == 0) { + /* + * The caller asked for ref@{0}, and we had no entries. + * It's a bit subtle, but in practice all callers have + * prepped the "oid" field with the current value of + * the ref, which is the most reasonable fallback. + * + * We'll put dummy values into the out-parameters (so + * they're not just uninitialized garbage), and the + * caller can take our return value as a hint that + * we did not find any such reflog. + */ + set_read_ref_cutoffs(&cb, 0, 0, "empty reflog"); + return 1; + } if (flags & GET_OID_QUIETLY) exit(128); else diff --git a/refs.h b/refs.h index 303c5fac4d..37116ad2b2 100644 --- a/refs.h +++ b/refs.h @@ -440,7 +440,20 @@ int refs_create_reflog(struct ref_store *refs, const char *refname, struct strbuf *err); int safe_create_reflog(const char *refname, struct strbuf *err); -/** Reads log for the value of ref during at_time. **/ +/** + * Reads log for the value of ref during at_time (in which case "cnt" should be + * negative) or the reflog "cnt" entries from the top (in which case "at_time" + * should be 0). + * + * If we found the reflog entry in question, returns 0 (and details of the + * entry can be found in the out-parameters). + * + * If we ran out of reflog entries, the out-parameters are filled with the + * details of the oldest entry we did find, and the function returns 1. Note + * that there is one important special case here! If the reflog was empty + * and the caller asked for the 0-th cnt, we will return "1" but leave the + * "oid" field untouched. + **/ int read_ref_at(struct ref_store *refs, const char *refname, unsigned int flags, timestamp_t at_time, int cnt, diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh index 370bf7137e..e841309d0e 100755 --- a/t/t1508-at-combinations.sh +++ b/t/t1508-at-combinations.sh @@ -110,7 +110,7 @@ test_expect_success '@{1} works with only one reflog entry' ' test_cmp_rev newbranch~ newbranch@{1} ' -test_expect_failure '@{0} works with empty reflog' ' +test_expect_success '@{0} works with empty reflog' ' git checkout -B newbranch main && git reflog expire --expire=now refs/heads/newbranch && test_cmp_rev newbranch newbranch@{0} diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh index 35f35f8091..a1139f79e2 100755 --- a/t/t3202-show-branch.sh +++ b/t/t3202-show-branch.sh @@ -279,8 +279,8 @@ test_expect_success '--reflog shows reflog entries' ' test_expect_success '--reflog handles missing reflog' ' git reflog expire --expire=now branch && - test_must_fail git show-branch --reflog branch 2>err && - grep "log .* is empty" err + git show-branch --reflog branch >actual && + test_must_be_empty actual ' test_done