@matheusbsbs / matheus-bagliano / commits / aa72e73a2e

Revert "refs: allow @{n} to work with n-sized reflog"

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 <yasushi.shoji@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Feb 26, 2024 at 05:02 UTC aa72e73a2e8f0924462aad5a2a45f12c267b1408
3 files changed +50 -51
refs.c
+14 -34
@@ -1038,55 +1038,40 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
1038 const char *message, void *cb_data)
1039 {
1040 struct read_ref_at_cb *cb = cb_data;
1041 - int reached_count;
1041
1042 cb->tz = tz;
1043 cb->date = timestamp;
1044
1046 - /*
1047 - * It is not possible for cb->cnt == 0 on the first iteration because
1048 - * that special case is handled in read_ref_at().
1049 - */
1050 - if (cb->cnt > 0)
1051 - cb->cnt--;
1052 - reached_count = cb->cnt == 0 && !is_null_oid(ooid);
1053 - if (timestamp <= cb->at_time || reached_count) {
1045 + if (timestamp <= cb->at_time || cb->cnt == 0) {
1046 set_read_ref_cutoffs(cb, timestamp, tz, message);
1047 /*
1048 * we have not yet updated cb->[n|o]oid so they still
1049 * hold the values for the previous record.
1050 */
1059 - if (!is_null_oid(&cb->ooid) && !oideq(&cb->ooid, noid))
1060 - warning(_("log for ref %s has gap after %s"),
1051 + if (!is_null_oid(&cb->ooid)) {
1052 + oidcpy(cb->oid, noid);
1053 + if (!oideq(&cb->ooid, noid))
1054 + warning(_("log for ref %s has gap after %s"),
1055 cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
1062 - if (reached_count)
1063 - oidcpy(cb->oid, ooid);
1064 - else if (!is_null_oid(&cb->ooid) || cb->date == cb->at_time)
1056 + }
1057 + else if (cb->date == cb->at_time)
1058 oidcpy(cb->oid, noid);
1059 else if (!oideq(noid, cb->oid))
1060 warning(_("log for ref %s unexpectedly ended on %s"),
1061 cb->refname, show_date(cb->date, cb->tz,
1062 DATE_MODE(RFC2822)));
1063 + cb->reccnt++;
1064 + oidcpy(&cb->ooid, ooid);
1065 + oidcpy(&cb->noid, noid);
1066 cb->found_it = 1;
1067 + return 1;
1068 }
1069 cb->reccnt++;
1070 oidcpy(&cb->ooid, ooid);
1071 oidcpy(&cb->noid, noid);
1075 - return cb->found_it;
1076 -}
1077 -
1078 -static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,
1079 - struct object_id *noid,
1080 - const char *email UNUSED,
1081 - timestamp_t timestamp, int tz,
1082 - const char *message, void *cb_data)
1083 -{
1084 - struct read_ref_at_cb *cb = cb_data;
1085 -
1086 - set_read_ref_cutoffs(cb, timestamp, tz, message);
1087 - oidcpy(cb->oid, noid);
1088 - /* We just want the first entry */
1089 - return 1;
1072 + if (cb->cnt > 0)
1073 + cb->cnt--;
1074 + return 0;
1075 }
1076
1077 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,
1106 cb.cutoff_cnt = cutoff_cnt;
1107 cb.oid = oid;
1108
1124 - if (cb.cnt == 0) {
1125 - refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);
1126 - return 0;
1127 - }
1128 -
1109 refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
1110
1111 if (!cb.reccnt) {
t/t1508-at-combinations.sh
+2 -2
@@ -103,14 +103,14 @@ test_expect_success 'create path with @' '
103 check "@:normal" blob content
104 check "@:fun@ny" blob content
105
106 -test_expect_success '@{1} works with only one reflog entry' '
106 +test_expect_failure '@{1} works with only one reflog entry' '
107 git checkout -B newbranch main &&
108 git reflog expire --expire=now refs/heads/newbranch &&
109 git commit --allow-empty -m "first after expiration" &&
110 test_cmp_rev newbranch~ newbranch@{1}
111 '
112
113 -test_expect_success '@{0} works with empty reflog' '
113 +test_expect_failure '@{0} works with empty reflog' '
114 git checkout -B newbranch main &&
115 git reflog expire --expire=now refs/heads/newbranch &&
116 test_cmp_rev newbranch newbranch@{0}
t/t3202-show-branch.sh
+34 -15
@@ -4,9 +4,6 @@ test_description='test show-branch'
4
5 . ./test-lib.sh
6
7 -# arbitrary reference time: 2009-08-30 19:20:00
8 -GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW
9 -
7 test_expect_success 'error descriptions on empty repository' '
8 current=$(git branch --show-current) &&
9 cat >expect <<-EOF &&
@@ -187,18 +184,6 @@ test_expect_success 'show branch --merge-base with N arguments' '
184 test_cmp expect actual
185 '
186
190 -test_expect_success 'show branch --reflog=2' '
191 - sed "s/^> //" >expect <<-\EOF &&
192 - > ! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10
193 - > ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10
194 - > --
195 - > + [refs/heads/branch10@{0}] branch10
196 - > ++ [refs/heads/branch10@{1}] initial
197 - EOF
198 - git show-branch --reflog=2 >actual &&
199 - test_cmp actual expect
200 -'
201 -
187 # incompatible options
188 while read combo
189 do
@@ -264,4 +249,38 @@ test_expect_success 'error descriptions on orphan branch' '
249 test_branch_op_in_wt -c new-branch
250 '
251
252 +test_expect_success 'setup reflogs' '
253 + test_commit base &&
254 + git checkout -b branch &&
255 + test_commit one &&
256 + git reset --hard HEAD^ &&
257 + test_commit two &&
258 + test_commit three
259 +'
260 +
261 +test_expect_success '--reflog shows reflog entries' '
262 + cat >expect <<-\EOF &&
263 + ! [branch@{0}] (0 seconds ago) commit: three
264 + ! [branch@{1}] (60 seconds ago) commit: two
265 + ! [branch@{2}] (2 minutes ago) reset: moving to HEAD^
266 + ! [branch@{3}] (2 minutes ago) commit: one
267 + ----
268 + + [branch@{0}] three
269 + ++ [branch@{1}] two
270 + + [branch@{3}] one
271 + ++++ [branch@{2}] base
272 + EOF
273 + # the output always contains relative timestamps; use
274 + # a known time to get deterministic results
275 + GIT_TEST_DATE_NOW=$test_tick \
276 + git show-branch --reflog branch >actual &&
277 + test_cmp expect actual
278 +'
279 +
280 +test_expect_success '--reflog handles missing reflog' '
281 + git reflog expire --expire=now branch &&
282 + test_must_fail git show-branch --reflog branch 2>err &&
283 + grep "log .* is empty" err
284 +'
285 +
286 test_done