{"thread":{"id":"60970","subject":"Segfault: git show-branch --reflog refs/pullreqs/1","startedAt":"2024-02-21T01:48:37Z","lastAt":"2024-02-27T17:04:02Z","messageCount":27,"participants":["Yasushi SHOJI","Jeff King","Patrick Steinhardt","Kristoffer Haugsbakk","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"489046","messageId":"CAELBRWK-bZTV0qx6_34HAgpmYwy+5Zo2E0M+4B6yZJJ3CqweTw@mail.gmail.com","threadId":"60970","inReplyTo":null,"subject":"Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Yasushi SHOJI","fromEmail":"yasushi.shoji@gmail.com","sentAt":"2024-02-21T01:48:25Z","receivedAt":"2024-02-21T01:48:37Z","isPatch":false,"sender":{"key":"yasushi.shoji@gmail.com","avatar":"https://avatars.githubusercontent.com/u/10152?v=4"},"body":"Hi all,\n\nDoes anyone see a segfault on `git show-branch --reflog refs/pullreqs/1`?\n\nIt seems files_reflog_path() creates a wrong path with the above command\nusing REF_WORKTREE_SHARED.\n\nrefs/pullreqs is from GitHub if you have the following line in .git/config:\n\nfetch = +refs/pull/*/head:refs/pullreqs/*\n\n(gdb) bt\n#0 __pthread_kill_implementation (threadid=,\nsigno=signo@entry=6, no_tid=no_tid@entry=0)\n at ./nptl/pthread_kill.c:44\n#1 0x00007ffff7e101cf in __pthread_kill_internal (signo=6,\nthreadid=) at ./nptl/pthread_kill.c:78\n#2 0x00007ffff7dc2472 in __GI_raise (sig=sig@entry=6) at\n../sysdeps/posix/raise.c:26\n#3 0x00007ffff7dac4b2 in __GI_abort () at ./stdlib/abort.c:79\n#4 0x00007ffff7dad1ed in __libc_message (fmt=fmt@entry=0x7ffff7f1f78c\n\"%s\\n\") at ../sysdeps/posix/libc_fatal.c:150\n#5 0x00007ffff7e19ae5 in malloc_printerr\n(str=str@entry=0x7ffff7f1d22c \"free(): invalid pointer\") at\n./malloc/malloc.c:5658\n#6 0x00007ffff7e1b864 in _int_free (av=, p=, have_lock=have_lock@entry=0) at ./malloc/malloc.c:4432\n#7 0x00007ffff7e1e1df in __GI___libc_free (mem=) at\n./malloc/malloc.c:3367\n#8 0x0000555555625ae2 in cmd_show_branch (ac=,\nav=, prefix=)\n at builtin/show-branch.c:801\n#9 0x0000555555572250 in run_builtin (argv=0x7fffffffe080, argc=3,\np=0x5555558ca198 ) at git.c:469\n#10 handle_builtin (argc=3, argv=0x7fffffffe080) at git.c:724\n#11 0x00005555555731b4 in run_argv (argcp=argcp@entry=0x7fffffffde6c,\nargv=argv@entry=0x7fffffffde60) at git.c:788\n#12 0x0000555555573d38 in cmd_main (argc=,\nargc@entry=4, argv=, argv@entry=0x7fffffffe078)\n at git.c:923\n#13 0x0000555555571f10 in main (argc=4, argv=0x7fffffffe078) at common-main.c:62\n\nBest,\n-- \n yashi\n"},{"id":"489056","messageId":"20240221084250.GA25385@coredump.intra.peff.net","threadId":"60970","inReplyTo":"CAELBRWK-bZTV0qx6_34HAgpmYwy+5Zo2E0M+4B6yZJJ3CqweTw@mail.gmail.com","subject":"Re: Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-21T08:42:50Z","receivedAt":"2024-02-21T08:42:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2024 at 10:48:25AM +0900, Yasushi SHOJI wrote:\n\n> Does anyone see a segfault on `git show-branch --reflog refs/pullreqs/1`?\n> \n> It seems files_reflog_path() creates a wrong path with the above command\n> using REF_WORKTREE_SHARED.\n\nI can trigger a segfault, but I think the issue is simply a ref that has\nno reflog. Here's a simple reproduction:\n\n $ git init\n $ git commit --allow-empty -m foo\n $ rm -rf .git/logs\n $ git show-branch --reflog\n Segmentation fault\n\nThe bug is in read_ref_at(). When asked for the reflog at position \"0\",\nit calls refs_for_each_reflog_ent_reverse() with a special callback, but\ndoes not check that it actually found anything! So we return \"0\" for\nsuccess, but all of the returned fields are garbage (including the\npointer to reflog message, which is where I see the segfault).\n\nThe bug was introduced by 6436a20284 (refs: allow @{n} to work with\nn-sized reflog, 2021-01-07). Probably the fix is something like:\n\ndiff --git a/refs.c b/refs.c\nindex 03968ad787..c2a48f8188 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -945,6 +945,8 @@ static int read_ref_at_ent_newest(struct object_id *ooid, struct object_id *noid\n \n \tset_read_ref_cutoffs(cb, timestamp, tz, message);\n \toidcpy(cb->oid, noid);\n+\tcb->reccnt++;\n+\tcb->found_it = 1;\n \t/* We just want the first entry */\n \treturn 1;\n }\n@@ -980,12 +982,10 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n \tcb.cutoff_cnt = cutoff_cnt;\n \tcb.oid = oid;\n \n-\tif (cb.cnt == 0) {\n+\tif (cb.cnt == 0)\n \t\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);\n-\t\treturn 0;\n-\t}\n-\n-\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);\n+\telse\n+\t\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);\n \n \tif (!cb.reccnt) {\n \t\tif (flags & GET_OID_QUIETLY)\n\nbut that breaks t1508.35, which explicitly tests for branch@{0} to work\nwith an empty reflog file (added by that same commit). The code in\nget_oid_basic() to parse reflogs doesn't suffer from the same bugs: it\nchecks up front that the reflog file exists, it preloads the output oid\nwith the current ref value, and it doesn't look at other fields (like\nthe reflog message).\n\nSo I'm not sure if read_ref_at() needs to be made safer, or if\ncmd_show_branch() needs to learn the same tricks as get_oid_basic().\nThose are the only two callers of read_ref_at().\n\nBeyond that confusion, I noticed we do not have many tests for\nshow-branch, and none for \"--reflog\". So I thought to add a basic one\nwhere we _do_ have an actual reflog to show. But wow, this has been\nbroken for some time. I found at least two issues trying to run a test\nlike:\n\ndiff --git a/t/t3207-show-branch-reflog.sh b/t/t3207-show-branch-reflog.sh\nnew file mode 100755\nindex 0000000000..7f52c8dcb1\n--- /dev/null\n+++ b/t/t3207-show-branch-reflog.sh\n@@ -0,0 +1,34 @@\n+#!/bin/sh\n+\n+test_description='show-branch reflog tests'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit base &&\n+\tgit checkout -b branch &&\n+\ttest_commit one &&\n+\tgit reset --hard HEAD^ &&\n+\ttest_commit two &&\n+\ttest_commit three\n+'\n+\n+test_expect_success '--reflog shows reflog entries' '\n+\tcat >expect <<-\\EOF &&\n+\t! [branch@{0}] (0 seconds ago) commit: three\n+\t ! [branch@{1}] (60 seconds ago) commit: two\n+\t ! [branch@{2}] (2 minutes ago) reset: moving to HEAD^\n+\t ! [branch@{3}] (2 minutes ago) commit: one\n+\t----\n+\t+ [refs/heads/branch@{0}] three\n+\t++ [refs/heads/branch@{1}] two\n+\t + [refs/heads/branch@{3}] one\n+\t++++ [refs/heads/branch@{2}] base\n+\tEOF\n+\t# the output always contains relative timestamps; use\n+\t# a known time to get deterministic results\n+\tGIT_TEST_DATE_NOW=$test_tick \\\n+\tgit show-branch --reflog branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n\nThe first is that \"show-branch\" does not print the correct reflog\nmessage, and you get output like this:\n\n ! [branch@{0}] (0 seconds ago) (none)\n ! [branch@{1}] (0 seconds ago) (none)\n ! [branch@{2}] (60 seconds ago) (none)\n ! [branch@{3}] (2 minutes ago) (none)\n\nOnce upon a time, read_ref_at() returned the whole reflog line, and\nshow-branch had to find the tab-separator. But since 4207ed285f (refs.c:\nchange read_ref_at to use the reflog iterators, 2014-06-03), it returns\njust the actual message (curiously, with the newline still attached). So\nwe need something like this to fix it:\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d6d2dabeca..b678b9fedb 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -761,7 +761,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\tfor (i = 0; i < reflog; i++) {\n \t\t\tchar *logmsg;\n \t\t\tchar *nth_desc;\n-\t\t\tconst char *msg;\n+\t\t\tchar *eol;\n \t\t\ttimestamp_t timestamp;\n \t\t\tint tz;\n \n@@ -771,15 +771,13 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\t\treflog = i;\n \t\t\t\tbreak;\n \t\t\t}\n-\t\t\tmsg = strchr(logmsg, '\\t');\n-\t\t\tif (!msg)\n-\t\t\t\tmsg = \"(none)\";\n-\t\t\telse\n-\t\t\t\tmsg++;\n+\t\t\teol = strchr(logmsg, '\\n');\n+\t\t\tif (eol)\n+\t\t\t\t*eol = '\\0';\n \t\t\treflog_msg[i] = xstrfmt(\"(%s) %s\",\n \t\t\t\t\t\tshow_date(timestamp, tz,\n \t\t\t\t\t\t\t DATE_MODE(RELATIVE)),\n-\t\t\t\t\t\tmsg);\n+\t\t\t\t\t\tlogmsg);\n \t\t\tfree(logmsg);\n \n \t\t\tnth_desc = xstrfmt(\"%s@{%d}\", *av, base+i);\n\nEasy enough. But the output is still subtly wrong! Now we're back to\n6436a20284 (refs: allow @{n} to work with n-sized reflog, 2021-01-07)\nagain. Before that commit, applying the fix above gives the expected\noutput from my test:\n\n ! [branch@{0}] (0 seconds ago) commit: three\n ! [branch@{1}] (60 seconds ago) commit: two\n ! [branch@{2}] (2 minutes ago) reset: moving to HEAD^\n ! [branch@{3}] (2 minutes ago) commit: one\n\nbut afterwards, entries higher than one are all shifted (so 1 is a\nduplicate of 0, 2 is the old 1, and so on):\n\n ! [branch@{0}] (0 seconds ago) commit: three\n ! [branch@{1}] (0 seconds ago) commit: three\n ! [branch@{2}] (60 seconds ago) commit: two\n ! [branch@{3}] (2 minutes ago) reset: moving to HEAD^\n\nI am still trying to wrap my head around how it can get such wrong\nresults for show-branch when asking for \"git rev-parse branch@{0}\", etc,\nare correct. I think it is that \"rev-parse branch@{0}\" is only looking\nat the output oid and does not consider the reflog message at all. So I\nthink it is subtly broken, but in a way that happens to work for that\ncaller. But I'm not sure of the correct fix. At least not at this time\nof night.\n\nCc-ing folks involved in 6436a20284.\n\n-Peff\n"},{"id":"489061","messageId":"ZdXH39p_Bh0mCsj3@tanuki","threadId":"60970","inReplyTo":"CAELBRWK-bZTV0qx6_34HAgpmYwy+5Zo2E0M+4B6yZJJ3CqweTw@mail.gmail.com","subject":"Re: Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T09:52:31Z","receivedAt":"2024-02-21T09:52:37Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 21, 2024 at 10:48:25AM +0900, Yasushi SHOJI wrote:\n> Hi all,\n> \n> Does anyone see a segfault on `git show-branch --reflog refs/pullreqs/1`?\n\nTIL. I didn't even know that git-show-branch(1) is a thing.\n\nBy default, no refs but \"HEAD\" and refs starting with \"refs/heads/*\"\nhave a reflog. Thus, your ref \"refs/pullreqs/1\" won't have a reflog, and\ngit-show-branch(1) then happily segfaults when it didn't find a reflog\nat all. This is of course a bug that needs fixing.\n\nI'll send a patch in a bit, thanks for your report!\n\nPatrick\n"},{"id":"489062","messageId":"cover.1708509190.git.ps@pks.im","threadId":"60970","inReplyTo":"CAELBRWK-bZTV0qx6_34HAgpmYwy+5Zo2E0M+4B6yZJJ3CqweTw@mail.gmail.com","subject":"[PATCH 0/2] Detect empty or missing reflogs with `ref@{0}`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T09:56:32Z","receivedAt":"2024-02-21T09:56:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series addresses some shortcomings when parsing `ref@{n}`\nsyntax via `read_ref_at()` when the reflog is missing or empty:\n\n - First, as reported by Yasushi, git-show-branch(1) would segfault\n because the function does not report when the 0th entry wasn't\n found.\n\n - Second, `ref@{0}` would fall back to return the object ID of ref\n itself in case the reflog is empty or missing. This behaviour is\n quite confusing and only works by chance.\n\nThe series addresses both of these issues by detecting and reporting the\ncase where the reflog is empty or missing.\n\nPatrick\n\nPatrick Steinhardt (2):\n object-name: detect and report empty reflogs\n builtin/show-branch: detect empty reflogs\n\n builtin/show-branch.c | 2 ++\n object-name.c | 10 ++++++----\n refs.c | 3 ++-\n t/t1506-rev-parse-diagnosis.sh | 8 ++++++++\n t/t1508-at-combinations.sh | 29 +++++++++++++++++++++++++----\n t/t3202-show-branch.sh | 25 +++++++++++++++++++++++++\n 6 files changed, 68 insertions(+), 9 deletions(-)\n\n-- \n2.44.0-rc1\n\n"},{"id":"489063","messageId":"0fac6ebb098c7e8cdc87cb75f2dcffdc4b1ccfaa.1708509190.git.ps@pks.im","threadId":"60970","inReplyTo":"cover.1708509190.git.ps@pks.im","subject":"[PATCH 1/2] object-name: detect and report empty reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T09:56:36Z","receivedAt":"2024-02-21T09:56:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `ref@{n}` syntax allows the user to request the n'th reflog entry\nfor a ref. This syntax is zero-indexed, meaning that entry zero refers\nto the first entry in the reflog. The behaviour here is quite confusing\nthough and depends on the state of the corresponding reflog:\n\n - If the reflog is not empty, we return the first reflog entry.\n\n - If the reflog is empty, we return the object ID of the ref.\n\n - If the reflog is missing, we return an error.\n\nThis is inconsistent and quite misleading.\n\nThis behaviour goes back to 6436a20284 (refs: allow @{n} to work with\nn-sized reflog, 2021-01-07), which fixed a bug that wouldn't allow a\nuser to return the n'th reflog entry with an n-sized reflog. With this\ncommit, `read_ref_at()` started to special case reading the first entry\nof the reflog via a separate `read_ref_at_ent_newest()` function. The\nproblem here is that we forgot to check whether the callback was invoked\nat all, and thus we don't notice empty reflogs.\n\nThe commit in question added a test for `ref@{0}` when the reflog is\nempty. But that test only works by chance: while `read_ref_at()` won't\ninitialize the object ID passed in by the pointer, all callers of this\nfunction happen to call `repo_ref_dwim()` and thus pre-populate the\nobject ID. Thus, the consequence is that we indeed return the object ID\nof the refname when the reflog is empty.\n\nThis behaviour is documented nowhere, and the fact that we return a\nsomewhat sensible result to the caller by sheer luck further stresses\nthe point that this behaviour is only accidental, even if there is a\ntest covering it.\n\nFurthermore, this behaviour causes problems for the git-show-branch(1)\ncommand. When executing `git show-branch --reflog` for a ref that either\nhas no or an empty reflog we run into a segfault. This is because the\n`read_ref_at()` function doesn't report the error to us, and thus parts\nof its out-parameters are not initialized.\n\nStart to detect and report empty or missing reflogs in `read_ref_at()`\nand report them to the caller. This results in a change in behaviour\nwhen asking for `ref@{0}` with an empty or missing reflog because we now\ndie instead of returning the object ID of the ref itself. This adapted\nbehaviour should lead to less surprises as we now really only report\nobject IDs to the caller that actually come from the reflog, thus making\nthe user experience a whole lot more consistent.\n\nThis change also fixes the segfault in git-show-branch(1). Note that\nthis commit does not add a test yet -- this will be handled in the next\ncommit.\n\nReported-by: Yasushi SHOJI \nSigned-off-by: Patrick Steinhardt \n---\n object-name.c | 10 ++++++----\n refs.c | 3 ++-\n t/t1506-rev-parse-diagnosis.sh | 8 ++++++++\n t/t1508-at-combinations.sh | 29 +++++++++++++++++++++++++----\n 4 files changed, 41 insertions(+), 9 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 3a2ef5d680..e2a6c9d2ec 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -994,8 +994,8 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \tif (reflog_len) {\n \t\tint nth, i;\n \t\ttimestamp_t at_time;\n-\t\ttimestamp_t co_time;\n-\t\tint co_tz, co_cnt;\n+\t\ttimestamp_t co_time = 0;\n+\t\tint co_tz = 0, co_cnt = 0;\n \n \t\t/* Is it asking for N-th entry, or approxidate? */\n \t\tfor (i = nth = 0; 0 <= nth && i < reflog_len; i++) {\n@@ -1020,6 +1020,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\t\t\treturn -1;\n \t\t\t}\n \t\t}\n+\n \t\tif (read_ref_at(get_main_ref_store(r),\n \t\t\t\treal_ref, flags, at_time, nth, oid, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n@@ -1035,9 +1036,10 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\t\t\t\t\tshow_date(co_time, co_tz, DATE_MODE(RFC2822)));\n \t\t\t\t}\n \t\t\t} else {\n-\t\t\t\tif (flags & GET_OID_QUIETLY) {\n+\t\t\t\tif (flags & GET_OID_QUIETLY)\n \t\t\t\t\texit(128);\n-\t\t\t\t}\n+\t\t\t\tif (!co_cnt)\n+\t\t\t\t\tdie(_(\"log for '%.*s' is empty\"), len, str);\n \t\t\t\tdie(_(\"log for '%.*s' only has %d entries\"),\n \t\t\t\t len, str, co_cnt);\n \t\t\t}\ndiff --git a/refs.c b/refs.c\nindex c633abf284..a2369e7835 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1084,6 +1084,7 @@ static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,\n \tstruct read_ref_at_cb *cb = cb_data;\n \n \tset_read_ref_cutoffs(cb, timestamp, tz, message);\n+\tcb->found_it = 1;\n \toidcpy(cb->oid, noid);\n \t/* We just want the first entry */\n \treturn 1;\n@@ -1123,7 +1124,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n \n \tif (cb.cnt == 0) {\n \t\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);\n-\t\treturn 0;\n+\t\treturn !cb.found_it;\n \t}\n \n \trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex ef40511d89..9d147c4ade 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -140,6 +140,14 @@ test_expect_success 'incorrect file in :path and :N:path' '\n \ttest_grep \"path .disk-only.txt. exists on disk, but not in the index\" error\n '\n \n+test_expect_success '@{0} reference with empty reflog' '\n+\tgit checkout -B empty-reflog main &&\n+\tgit reflog expire --expire=now refs/heads/empty-reflog &&\n+\ttest_must_fail git rev-parse empty-reflog@{0} >output 2>error &&\n+\ttest_must_be_empty output &&\n+\ttest_grep \"log for ${SQ}empty-reflog${SQ} is empty\" error\n+'\n+\n test_expect_success 'invalid @{n} reference' '\n \ttest_must_fail git rev-parse main@{99999} >output 2>error &&\n \ttest_must_be_empty output &&\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex e841309d0e..020106a1cc 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -110,10 +110,31 @@ test_expect_success '@{1} works with only one reflog entry' '\n \ttest_cmp_rev newbranch~ newbranch@{1}\n '\n \n-test_expect_success '@{0} works with empty reflog' '\n-\tgit checkout -B newbranch main &&\n-\tgit reflog expire --expire=now refs/heads/newbranch &&\n-\ttest_cmp_rev newbranch newbranch@{0}\n+test_expect_success '@{0} fails with empty reflog' '\n+\tgit checkout -B empty-reflog main &&\n+\tgit reflog expire --expire=now refs/heads/empty-reflog &&\n+\tcat >expect <<-EOF &&\n+\tfatal: Needed a single revision\n+\tEOF\n+\ttest_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success '@{0} fails with missing reflog' '\n+\tgit -c core.logAllRefUpdates=false checkout -B missing-reflog main &&\n+\tcat >expect <<-EOF &&\n+\tfatal: Needed a single revision\n+\tEOF\n+\ttest_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success '@{0} favors first reflog entry with diverged reflog' '\n+\tgit checkout -B diverged-reflog main &&\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\tgit reflog delete diverged-reflog@{0} &&\n+\ttest_cmp_rev diverged-reflog~ diverged-reflog@{0}\n '\n \n test_done\n-- \n2.44.0-rc1\n\n"},{"id":"489064","messageId":"6107efeffa3a9e87ed95f4fe592d9b480887c510.1708509190.git.ps@pks.im","threadId":"60970","inReplyTo":"cover.1708509190.git.ps@pks.im","subject":"[PATCH 2/2] builtin/show-branch: detect empty reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T09:56:40Z","receivedAt":"2024-02-21T09:56:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `--reflog=n` option for git-show-branch(1) allows the user to list\nthe reflog of a specific branch, where `n` specifies how many reflog\nentries to show. When there are less than `n` entries we handle this\ngracefully and simply report less entries.\n\nThere is a special case though when the ref either has no reflog or when\nits reflog is empty. In this case, we end up printing nothing at all\nwhile returning successfully. This is rather confusing as there is no\nindicator why we didn't print anything.\n\nAdapt the behaviour so that we die instead of leaving no user visible\ntraces. This change in behaviour should be fine given that this case\nused to segfault before the preceding commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/show-branch.c | 2 ++\n t/t3202-show-branch.sh | 25 +++++++++++++++++++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex b01ec761d2..8837415031 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -785,6 +785,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\tif (read_ref_at(get_main_ref_store(the_repository),\n \t\t\t\t\tref, flags, 0, base + i, &oid, &logmsg,\n \t\t\t\t\t×tamp, &tz, NULL)) {\n+\t\t\t\tif (!i)\n+\t\t\t\t\tdie(_(\"log for %s is empty\"), ref);\n \t\t\t\treflog = i;\n \t\t\t\tbreak;\n \t\t\t}\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex 6a98b2df76..e5b74cc55f 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -264,4 +264,29 @@ test_expect_success 'error descriptions on orphan branch' '\n \ttest_branch_op_in_wt -c new-branch\n '\n \n+test_expect_success 'show-branch --reflog with empty reflog' '\n+\tgit checkout -B empty-reflog &&\n+\tgit reflog expire --expire=now refs/heads/empty-reflog &&\n+\n+\tcat >expect <<-EOF &&\n+\tfatal: log for refs/heads/empty-reflog is empty\n+\tEOF\n+\ttest_must_fail git show-branch --reflog=1 empty-reflog 2>err &&\n+\ttest_cmp expect err &&\n+\ttest_must_fail git show-branch --reflog empty-reflog 2>err &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success 'show-branch --reflog with missing reflog' '\n+\tgit -c core.logAllRefUpdates=false checkout -B missing-reflog &&\n+\n+\tcat >expect <<-EOF &&\n+\tfatal: log for refs/heads/missing-reflog is empty\n+\tEOF\n+\ttest_must_fail git show-branch --reflog=1 missing-reflog 2>err &&\n+\ttest_cmp expect err &&\n+\ttest_must_fail git show-branch --reflog missing-reflog 2>err &&\n+\ttest_cmp expect err\n+'\n+\n test_done\n-- \n2.44.0-rc1\n\n"},{"id":"489065","messageId":"ZdXLBIv1vLjhQUgx@tanuki","threadId":"60970","inReplyTo":"20240221084250.GA25385@coredump.intra.peff.net","subject":"Re: Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T10:05:56Z","receivedAt":"2024-02-21T10:06:02Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 21, 2024 at 03:42:50AM -0500, Jeff King wrote:\n> On Wed, Feb 21, 2024 at 10:48:25AM +0900, Yasushi SHOJI wrote:\n> \n> > Does anyone see a segfault on `git show-branch --reflog refs/pullreqs/1`?\n> > \n> > It seems files_reflog_path() creates a wrong path with the above command\n> > using REF_WORKTREE_SHARED.\n> I am still trying to wrap my head around how it can get such wrong\n> results for show-branch when asking for \"git rev-parse branch@{0}\", etc,\n> are correct. I think it is that \"rev-parse branch@{0}\" is only looking\n> at the output oid and does not consider the reflog message at all. So I\n> think it is subtly broken, but in a way that happens to work for that\n> caller. But I'm not sure of the correct fix. At least not at this time\n> of night.\n> \n> Cc-ing folks involved in 6436a20284.\n\nAh, our mails crossed, but we came to the same conclusion. Things indeed\nare subtly broken here and work just by chance because all callers pre\ninitialize the object ID. So in the case where the reflog is missing or\nempty we'd use that pre-initialized object ID because `read_ref_at()`\ndoes not indicate the failure to the callers.\n\nI think that this behaviour is not sensible in the first place. When\nasking for the reflog, we should only ever return object IDs parsed from\nthe reflog. Falling back to parsing the ref itself does not make much\nsense. I've thus sent a patch series that changes the behaviour here.\n\nPatrick\n"},{"id":"489066","messageId":"5856eabc-63cf-48a0-94a2-7f6d62906aa5@app.fastmail.com","threadId":"60970","inReplyTo":"0fac6ebb098c7e8cdc87cb75f2dcffdc4b1ccfaa.1708509190.git.ps@pks.im","subject":"Re: [PATCH 1/2] object-name: detect and report empty reflogs","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-02-21T10:37:00Z","receivedAt":"2024-02-21T10:37:22Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Wed, Feb 21, 2024, at 10:56, Patrick Steinhardt wrote:\n> The `ref@{n}` syntax allows the user to request the n'th reflog entry\n> for a ref. This syntax is zero-indexed, meaning that entry zero refers\n> to the first entry in the reflog. The behaviour here is quite confusing\n> though and depends on the state of the corresponding reflog:\n\nMaybe this is obvious to other readers, but I sometimes get tripped up\nwhen reading about such logs: what’s the “first entry”? The oldest one\nor newest one? How about:\n\n “ The `ref@{n}` syntax allows the user to request the n'th reflog entry\n for a ref, starting from `ref@{0}` which points to the commit that\n `ref` points to (zero-indexed). The behaviour here is quite confusing\n though and depends on the state of the corresponding reflog:\n\n-- \nKristoffer Haugsbakk\n"},{"id":"489088","messageId":"CAPig+cQff2LUnsXf+NMuAczN0sVNbPU9fOSFapeMSDBpMYkedQ@mail.gmail.com","threadId":"60970","inReplyTo":"0fac6ebb098c7e8cdc87cb75f2dcffdc4b1ccfaa.1708509190.git.ps@pks.im","subject":"Re: [PATCH 1/2] object-name: detect and report empty reflogs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-21T16:48:40Z","receivedAt":"2024-02-21T16:48:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 21, 2024 at 5:03 AM Patrick Steinhardt wrote:\n> [...]\n> Start to detect and report empty or missing reflogs in `read_ref_at()`\n> and report them to the caller. This results in a change in behaviour\n> when asking for `ref@{0}` with an empty or missing reflog because we now\n> die instead of returning the object ID of the ref itself. This adapted\n> behaviour should lead to less surprises as we now really only report\n> object IDs to the caller that actually come from the reflog, thus making\n> the user experience a whole lot more consistent.\n> [...]\n> Reported-by: Yasushi SHOJI \n> Signed-off-by: Patrick Steinhardt \n> ---\n> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\n> @@ -110,10 +110,31 @@ test_expect_success '@{1} works with only one reflog entry' '\n> +test_expect_success '@{0} fails with empty reflog' '\n> + git checkout -B empty-reflog main &&\n> + git reflog expire --expire=now refs/heads/empty-reflog &&\n> + cat >expect <<-EOF &&\n> + fatal: Needed a single revision\n> + EOF\n\nTypically, we would use <<-\\EOF rather than <<-EOF to signal to the\nreader that no variable interpolation is happening in the here-doc\nbody.\n\nA simpler alternative in this case would, of course, be to use `echo`:\n\n echo \"fatal: Needed a single revision\" >expect &&\n\n> + test_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&\n> + test_cmp expect err\n> +'\n> +\n> +test_expect_success '@{0} fails with missing reflog' '\n> + git -c core.logAllRefUpdates=false checkout -B missing-reflog main &&\n> + cat >expect <<-EOF &&\n> + fatal: Needed a single revision\n> + EOF\n\nDitto.\n\n> + test_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&\n> + test_cmp expect err\n> +'\n\nProbably neither comment is worth a reroll, but if you reroll for some\nother reason, perhaps consider them.\n"},{"id":"489090","messageId":"20240221173121.GA634809@coredump.intra.peff.net","threadId":"60970","inReplyTo":"0fac6ebb098c7e8cdc87cb75f2dcffdc4b1ccfaa.1708509190.git.ps@pks.im","subject":"Re: [PATCH 1/2] object-name: detect and report empty reflogs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-21T17:31:21Z","receivedAt":"2024-02-21T17:31:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2024 at 10:56:36AM +0100, Patrick Steinhardt wrote:\n\n> This behaviour goes back to 6436a20284 (refs: allow @{n} to work with\n> n-sized reflog, 2021-01-07), which fixed a bug that wouldn't allow a\n> user to return the n'th reflog entry with an n-sized reflog. With this\n> commit, `read_ref_at()` started to special case reading the first entry\n> of the reflog via a separate `read_ref_at_ent_newest()` function. The\n> problem here is that we forgot to check whether the callback was invoked\n> at all, and thus we don't notice empty reflogs.\n\nI'm on the fence no whether the current @{0} behavior is sensible and\nshould be preserved. I agree it mostly worked by luck, but the presence\nof the test makes me think at least the intention was there.\n\nBut assuming that is a good direction, there's one thing that puzzles me\nabout your patch:\n\n> @@ -1084,6 +1084,7 @@ static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,\n> \tstruct read_ref_at_cb *cb = cb_data;\n> \n> \tset_read_ref_cutoffs(cb, timestamp, tz, message);\n> +\tcb->found_it = 1;\n> \toidcpy(cb->oid, noid);\n> \t/* We just want the first entry */\n> \treturn 1;\n\nOK, so we note whether the callback was invoked, which is good...\n\n> @@ -1123,7 +1124,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n> \n> \tif (cb.cnt == 0) {\n> \t\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);\n> -\t\treturn 0;\n> +\t\treturn !cb.found_it;\n> \t}\n\n...but here we just return without an error message. Whereas later in\nthe function, we have logic to produce the \"log for %s is empty\"\nmessage. So now we will produce a message if you ask for branch@{1} in\nan empty reflog, but not for branch@{0}, and the caller is responsible\nfor printing an error in the latter case.\n\nIf we instead set reccnt for branch@{0} as we would for branch@{1}, then\nwe can fall through and share the error handling (like it was before\n6436a20284, when they used the same callback):\n\ndiff --git a/refs.c b/refs.c\nindex c633abf284..7d5e7a9ba6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1084,6 +1084,8 @@ static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,\n \tstruct read_ref_at_cb *cb = cb_data;\n \n \tset_read_ref_cutoffs(cb, timestamp, tz, message);\n+\tcb->reccnt++;\n+\tcb->found_it = 1;\n \toidcpy(cb->oid, noid);\n \t/* We just want the first entry */\n \treturn 1;\n@@ -1121,12 +1123,10 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n \tcb.cutoff_cnt = cutoff_cnt;\n \tcb.oid = oid;\n \n-\tif (cb.cnt == 0) {\n+\tif (cb.cnt == 0)\n \t\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);\n-\t\treturn 0;\n-\t}\n-\n-\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);\n+\telse\n+\t\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);\n \n \tif (!cb.reccnt) {\n \t\tif (flags & GET_OID_QUIETLY)\n\nAnd it all just works without having to touch get_oid_basic() or\ncmd_show_branch() at all. Do note that one of the tests needs to be\nupdated to account for the slightly different format of the error\nmessage; but again, I think that is showing off the inconsistency in\nhaving the error message in two places.\n\n-Peff\n"},{"id":"489093","messageId":"20240221173545.GB634809@coredump.intra.peff.net","threadId":"60970","inReplyTo":"6107efeffa3a9e87ed95f4fe592d9b480887c510.1708509190.git.ps@pks.im","subject":"Re: [PATCH 2/2] builtin/show-branch: detect empty reflogs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-21T17:35:45Z","receivedAt":"2024-02-21T17:35:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2024 at 10:56:40AM +0100, Patrick Steinhardt wrote:\n\n> diff --git a/builtin/show-branch.c b/builtin/show-branch.c\n> index b01ec761d2..8837415031 100644\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n> @@ -785,6 +785,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n> \t\t\tif (read_ref_at(get_main_ref_store(the_repository),\n> \t\t\t\t\tref, flags, 0, base + i, &oid, &logmsg,\n> \t\t\t\t\t×tamp, &tz, NULL)) {\n> +\t\t\t\tif (!i)\n> +\t\t\t\t\tdie(_(\"log for %s is empty\"), ref);\n> \t\t\t\treflog = i;\n> \t\t\t\tbreak;\n> \t\t\t}\n\nThere's another call to read_ref_at() earlier in the function that does\nnot check the return code. I think it is OK, though, because it always\npasses a timestamp rather than a count, so it never hits the\nspecial-case added by 6436a20284.\n\nBut I think this highlights my confusion with the first patch; most of\nthe time read_ref_at() will print the appropriate message for us and\ndie, but for this one weird case we have to do it ourselves. It makes\nmore sense to me for it to behave consistently for an empty reflog,\nwhether we were looking for branch@{0} or branch@{1}.\n\n-Peff\n"},{"id":"489096","messageId":"20240221173839.GC634809@coredump.intra.peff.net","threadId":"60970","inReplyTo":"ZdXLBIv1vLjhQUgx@tanuki","subject":"Re: Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-21T17:38:39Z","receivedAt":"2024-02-21T17:38:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 21, 2024 at 11:05:56AM +0100, Patrick Steinhardt wrote:\n\n> > I am still trying to wrap my head around how it can get such wrong\n> > results for show-branch when asking for \"git rev-parse branch@{0}\", etc,\n> > are correct. I think it is that \"rev-parse branch@{0}\" is only looking\n> > at the output oid and does not consider the reflog message at all. So I\n> > think it is subtly broken, but in a way that happens to work for that\n> > caller. But I'm not sure of the correct fix. At least not at this time\n> > of night.\n> > \n> > Cc-ing folks involved in 6436a20284.\n> \n> Ah, our mails crossed, but we came to the same conclusion. Things indeed\n> are subtly broken here and work just by chance because all callers pre\n> initialize the object ID. So in the case where the reflog is missing or\n> empty we'd use that pre-initialized object ID because `read_ref_at()`\n> does not indicate the failure to the callers.\n> \n> I think that this behaviour is not sensible in the first place. When\n> asking for the reflog, we should only ever return object IDs parsed from\n> the reflog. Falling back to parsing the ref itself does not make much\n> sense. I've thus sent a patch series that changes the behaviour here.\n\nI left some comments on your patches. But assuming we are OK to change\nthe branch@{0} behavior for the empty log, the approach is sound.\n\nThat still leaves us with the bug in showing the message (which is\neasily fixed), and the weird off-by-one output caused by 6436a20284.\nObviously the segfault fix can be taken independently of the rest, but I\nwonder if some deeper refactoring of what 6436a20284 did will be\nnecessary.\n\n-Peff\n"},{"id":"489097","messageId":"xmqqv86hogpi.fsf@gitster.g","threadId":"60970","inReplyTo":"20240221084250.GA25385@coredump.intra.peff.net","subject":"Re: Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-21T17:44:09Z","receivedAt":"2024-02-21T17:44:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> with an empty reflog file (added by that same commit). The code in\n> get_oid_basic() to parse reflogs doesn't suffer from the same bugs: it\n> checks up front that the reflog file exists, it preloads the output oid\n> with the current ref value, and it doesn't look at other fields (like\n> the reflog message).\n\nIt is a usability hack to allow foo@{0} to resolve successfully,\ninstead of causing a failure, when there is no reflog entries for\nfoo, I would think. As to the \"show-branch -g\", the intent is to\nsee the recent evolution of the branch, so I am fine if we do not\ngive any output when no reflog entries exist (i.e. \"no evolution\nbehind the current state---it just is\"), or just one entry output\nfor \"foo@{0}\" to say \"there is only one recent state\".\n\nEven though it may feel wrong to successfully resolve foo@{0} when\nreflog for foo does not exist at the mechanical level (read: the\nimplementors of reflog mechanism may find the usability hack a bad\nidea), I suspect at the end-user level it may be closer to what\npeople expect out of foo@{0} (i.e. \"give me the latest\").\n\nThanks.\n\n"},{"id":"489126","messageId":"ZdcNtxw04MtybTWZ@tanuki","threadId":"60970","inReplyTo":"xmqqv86hogpi.fsf@gitster.g","subject":"Re: Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-22T09:02:47Z","receivedAt":"2024-02-22T09:02:54Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 21, 2024 at 09:44:09AM -0800, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > with an empty reflog file (added by that same commit). The code in\n> > get_oid_basic() to parse reflogs doesn't suffer from the same bugs: it\n> > checks up front that the reflog file exists, it preloads the output oid\n> > with the current ref value, and it doesn't look at other fields (like\n> > the reflog message).\n> \n> It is a usability hack to allow foo@{0} to resolve successfully,\n> instead of causing a failure, when there is no reflog entries for\n> foo, I would think. As to the \"show-branch -g\", the intent is to\n> see the recent evolution of the branch, so I am fine if we do not\n> give any output when no reflog entries exist (i.e. \"no evolution\n> behind the current state---it just is\"), or just one entry output\n> for \"foo@{0}\" to say \"there is only one recent state\".\n> \n> Even though it may feel wrong to successfully resolve foo@{0} when\n> reflog for foo does not exist at the mechanical level (read: the\n> implementors of reflog mechanism may find the usability hack a bad\n> idea), I suspect at the end-user level it may be closer to what\n> people expect out of foo@{0} (i.e. \"give me the latest\").\n\nHum, I dunno. I don't really understand what the benefit of this\nfallback is. If a user wants to know the latest object ID of the ref\nthey shouldn't ask for `foo@{0}`, they should ask for `foo`. On the\nother hand, if I want to know \"What is the latest entry in the ref's\nlog\", I want to ask for `foo@{0}`.\n\nFor one, I think that this is adding complexity to the user interface.\nIf you were to explain this feature to a user who has never encountered\nit, you need to now also explain special cases: \"It gives you the latest\nreflog entry, except when the reflog doesn't exist or is missing, then\nwe return the object ID of the corresponding ref.\" This is a lot more\nmental overhead than \"It gives you the latest reflog entry.\"\n\nWe also have to consider a potential future where we stop deleting\nreflogs together with their ref in the \"reftable\" backend. What do we\nreturn when the reflog is empty and the ref is missing? \"It gives you\nthe latest reflog entry, except when the reflog doesn't exist or is\nmissing, then we give you the ref except if that is missing, too\". It's\ngetting even harder to explain now.\n\nAnother angle to me is scripting. If I really want to get the latest\nreflog entry, then I now have to execute two commands. First I have to\ncheck whether the reflog is non-empty, and only then can I ask for the\nlatest reflog entry. Otherwise, I might not get a reflog entry but the\nresolved ref instead. And I wouldn't even know how to check whether the\nreflog is non-empty.\n\nSo overall, I think the interface is a lot easier to understand and use\ncorrectly if we didn't have this fallback.\n\nPatrick\n"},{"id":"489159","messageId":"xmqq34tkiho9.fsf@gitster.g","threadId":"60970","inReplyTo":"ZdcNtxw04MtybTWZ@tanuki","subject":"Re: Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-22T16:32:06Z","receivedAt":"2024-02-22T16:32:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Even though it may feel wrong to successfully resolve foo@{0} when\n>> reflog for foo does not exist at the mechanical level (read: the\n>> implementors of reflog mechanism may find the usability hack a bad\n>> idea), I suspect at the end-user level it may be closer to what\n>> people expect out of foo@{0} (i.e. \"give me the latest\").\n>\n> Hum, I dunno. I don't really understand what the benefit of this\n> fallback is. If a user wants to know the latest object ID of the ref\n> they shouldn't ask for `foo@{0}`, they should ask for `foo`. On the\n> other hand, if I want to know \"What is the latest entry in the ref's\n> log\", I want to ask for `foo@{0}`.\n\nThe usability hack helps small things like \"List up to 4 most recent\nstates from a branch\", e.g.\n\n for nth in $(seq 0 3)\n do\n\tgit rev-parse --quiet --verify @$nth || break\n\tgit show -s --format=\"@$nth %h %s\" @$nth\n done\n\nvs\n\n for rev in HEAD @{1} @{2} @{3}\n do\n\tgit rev-parse --quiet --verify \"$rev\" || break\n\tgit show -s --format=\"$rev %h %s\" \"$rev\"\n done\n\nby not forcing you to special case the \"current\".\n\nIdeally, \"foo@{0}\" should have meant \"the state immediately before\nthe current state of foo\" so that \"foo\" is the unambiguous and only\nway to refer to \"the current state of foo\", but that was not how we\nimplemented the reflog, allowing a subtle repository corruption\nwhere the latest state of a branch according to the reflog and the\ncurrent commit pointed by the branch can diverge. But that wasn't\nwhat we did, and instead both \"foo@{0}\" and \"foo\" mean to refer to\n\"the latest state of foo\". We can take advantage of that misdesign\nand allow \"foo@{0}\" to refer to the same commit as \"foo\", at least\nat the get_oid_basic() level, whether a reflog actually exists or\nnot, and that would make the whole thing more consistent.\n\nIn any case, I do not know how this \"usability\" actually helps in\nthe field, and I wouldn't personally shed tears if it gets removed.\nThe above is just an explanation why it exists.\n\nThanks.\n"},{"id":"489173","messageId":"20240222172252.GA3535450@coredump.intra.peff.net","threadId":"60970","inReplyTo":"xmqq34tkiho9.fsf@gitster.g","subject":"Re: Segfault: git show-branch --reflog refs/pullreqs/1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-22T17:22:52Z","receivedAt":"2024-02-22T17:22:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 22, 2024 at 08:32:06AM -0800, Junio C Hamano wrote:\n\n> > Hum, I dunno. I don't really understand what the benefit of this\n> > fallback is. If a user wants to know the latest object ID of the ref\n> > they shouldn't ask for `foo@{0}`, they should ask for `foo`. On the\n> > other hand, if I want to know \"What is the latest entry in the ref's\n> > log\", I want to ask for `foo@{0}`.\n> \n> The usability hack helps small things like \"List up to 4 most recent\n> states from a branch\", e.g.\n> \n> for nth in $(seq 0 3)\n> do\n> \tgit rev-parse --quiet --verify @$nth || break\n> \tgit show -s --format=\"@$nth %h %s\" @$nth\n> done\n> \n> vs\n> \n> for rev in HEAD @{1} @{2} @{3}\n> do\n> \tgit rev-parse --quiet --verify \"$rev\" || break\n> \tgit show -s --format=\"$rev %h %s\" \"$rev\"\n> done\n> \n> by not forcing you to special case the \"current\".\n\nIn those examples, though, it is useful precisely because you _do_ have\na reflog, and ref@{0} is conceptually the top entry (which brought us to\nthe same state as just \"ref\").\n\nThe question to me is more \"is ref@{0} useful on its own, even when you\ndo not necessarily have a reflog\". That I am less sure of.\n\n> Ideally, \"foo@{0}\" should have meant \"the state immediately before\n> the current state of foo\" so that \"foo\" is the unambiguous and only\n> way to refer to \"the current state of foo\", but that was not how we\n> implemented the reflog, allowing a subtle repository corruption\n> where the latest state of a branch according to the reflog and the\n> current commit pointed by the branch can diverge. But that wasn't\n> what we did, and instead both \"foo@{0}\" and \"foo\" mean to refer to\n> \"the latest state of foo\". We can take advantage of that misdesign\n> and allow \"foo@{0}\" to refer to the same commit as \"foo\", at least\n> at the get_oid_basic() level, whether a reflog actually exists or\n> not, and that would make the whole thing more consistent.\n\nI think there is some confusion here between how get_oid_basic() behaves\nand how read_ref_at() is used for something like show-branch. In the\nformer case, we only care about getting an oid as output, but in the\nlatter we actually want the reflog entry (because we care about its\ntimestamp, message, and so on).\n\nSo in terms of reflog entries, ref@{0} should refer to the most recent\nentry. And the oid it returns should be the end-result of that entry,\nwhich (in a non corrupted repository) is identical to the current ref\nvalue. And that \"should\" is reinforced by stuff like:\n\n git log -g \"%gd %gs\"\n\nwhich shows the most recent entry as HEAD@{0}.\n\nI think 6436a20284 (refs: allow @{n} to work with n-sized reflog,\n2021-01-07) confused things mightily by having read_ref_at() with a\ncount of \"n\" find entry \"n-1\" instead, and then return the oid for the\n\"old\" value. That makes get_oid_basic() work, because it doesn't care\nabout which entry we found, only the oid. But for show-branch, now we\nare confused about which reflog entry ref@{1}, etc, refers to (but\nref@{0} still works because of the weird special-casing done by that\ncommit).\n\nI think we should fix that (and I have the start of some patches to do\nso). But in that world-view, having read_ref_at() return anything for a\ncount of \"0\" when there is no reflog does not make sense. There is no\nsuch entry!\n\nOTOH, we face the same problem when asking about ref@{N} when there are\nonly N entries. We can provide an oid (based on the \"old\" value from the\noldest entry we did see), but we have to \"fake\" the reflog entry data\n(like the messsage), since there wasn't one.\n\nSo the open questions to me are:\n\n - should this faking happen in read_ref_at(), just returning a dummy\n reflog message? Or should we keep read_ref_at() purely about finding\n the entry, and put the special-casing into get_oid_basic(), which\n only cares about the oid result?\n\n - wherever we put the faking, should we only fake ref@{N} when N > 0?\n Or should we also fake ref@{0} when there is no reflog at all?\n\nIf none of this makes sense, it is because I am only now untangling what\nis going on with 6436a20284. ;) I will try to polish my proposed patches\nand hopefully that will explain it a bit more clearly (I may not get to\nit until tomorrow though).\n\n-Peff\n"},{"id":"489361","messageId":"20240226100010.GA1214708@coredump.intra.peff.net","threadId":"60970","inReplyTo":"20240222172252.GA3535450@coredump.intra.peff.net","subject":"[PATCH 0/3] show-branch --reflog fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-26T10:00:10Z","receivedAt":"2024-02-26T10:00:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 22, 2024 at 12:22:52PM -0500, Jeff King wrote:\n\n> If none of this makes sense, it is because I am only now untangling what\n> is going on with 6436a20284. ;) I will try to polish my proposed patches\n> and hopefully that will explain it a bit more clearly (I may not get to\n> it until tomorrow though).\n\nOK, so here's what I came up with. One thing I did not realize, as I\nwas writing my patches directly atop 6436a20284, is that we actually\nfixed the reflog message bug back in f2463490c4 (show-branch: show\nreflog message, 2021-12-02). And several tests have been added since\nthen.\n\nSo I gave up on trying to build on top of the source of the bug, and\njust rebased onto the tip of master. It should apply to recent \"maint\"\nas well, I'd think.\n\n [1/3]: Revert \"refs: allow @{n} to work with n-sized reflog\"\n [2/3]: get_oid_basic(): special-case ref@{n} for oldest reflog entry\n [3/3]: read_ref_at(): special-case ref@{0} for an empty reflog\n\n object-name.c | 9 ++++++\n refs.c | 65 +++++++++++++++++++-----------------------\n refs.h | 15 +++++++++-\n t/t3202-show-branch.sh | 49 +++++++++++++++++++++----------\n 4 files changed, 87 insertions(+), 51 deletions(-)\n"},{"id":"489362","messageId":"20240226100226.GA2685600@coredump.intra.peff.net","threadId":"60970","inReplyTo":"20240226100010.GA1214708@coredump.intra.peff.net","subject":"[PATCH 1/3] Revert \"refs: allow @{n} to work with n-sized reflog\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-26T10:02:26Z","receivedAt":"2024-02-26T10:02:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This reverts commit 6436a20284f33d42103cac93bd82e65bebb31526.\n\nThe idea of that commit is that if read_ref_at() is counting back to the\nNth reflog but the reflog is short by one entry (e.g., because it was\npruned), we can find the oid of the missing entry by looking at the\n\"before\" oid value of the entry that comes after it (whereas before, we\nlooked at the \"after\" value of each entry and complained that we\ncouldn't find the one from before the truncation).\n\nThis works fine for resolving the oid of ref@{n}, as it is used by\nget_oid_basic(), which does not look at any other aspect of the reflog\nwe found (e.g., its timestamp or message). But there's another caller of\nread_ref_at(): in show-branch we use it to walk over the reflog, and we\ndo care about the reflog entry. And so that commit broke \"show-branch\n--reflog\"; it shows the reflog message for ref@{0} as ref@{1}, ref@{1}\nas ref@{2}, and so on.\n\nFor example, in the new test in t3202 we produce:\n\n ! [branch@{0}] (0 seconds ago) commit: three\n ! [branch@{1}] (0 seconds ago) commit: three\n ! [branch@{2}] (60 seconds ago) commit: two\n ! [branch@{3}] (2 minutes ago) reset: moving to HEAD^\n\ninstead of the correct:\n\n ! [branch@{0}] (0 seconds ago) commit: three\n ! [branch@{1}] (60 seconds ago) commit: two\n ! [branch@{2}] (2 minutes ago) reset: moving to HEAD^\n ! [branch@{3}] (2 minutes ago) commit: one\n\nBut there's another bug, too: because it is looking at the \"old\" value\nof the reflog after the one we're interested in, it has to special-case\nref@{0} (since there isn't anything after it). That's why it doesn't\nshow the offset bug in the output above. But this special-case code\nfails to handle the situation where the reflog is empty or missing; it\nreturns success even though the reflog message out-parameter has been\nleft uninitialized. You can't trigger this through get_oid_basic(), but\n\"show-branch --reflog\" will pretty reliably segfault as it tries to\naccess the garbage pointer.\n\nFixing the segfault would be pretty easy. But the off-by-one problem is\ninherent in this approach. So let's start by reverting the commit to\ngive us a clean slate to work with.\n\nThis isn't a pure revert; all of the code changes are reverted, but for\nthe tests:\n\n 1. We'll flip the cases in t1508 to expect_failure; making these work\n was the goal of 6436a2028, and we'll want to use them for our\n replacement approach.\n\n 2. There's a test in t3202 for \"show-branch --reflog\", but it expects\n the broken output! It was added by f2463490c4 (show-branch: show\n reflog message, 2021-12-02) which was fixing another bug, and I\n think the author simply didn't notice that the second line showed\n the wrong reflog.\n\n Rather than fixing that test, let's replace it with one that is\n more thorough (while still covering the reflog message fix from\n that commit). We'll use a longer reflog, which lets us see more\n entries (thus making the \"off by one\" pattern much more clear). And\n we'll use a more recent timestamp for \"now\" so that our relative\n dates have more resolution. That lets us see that the reflog dates\n are correct (whereas when you are 4 years away, two entries that\n are 60 seconds apart will have the same \"4 years ago\" relative\n date). Because we're adjusting the repository state, I've moved\n this new test to the end of the script, leaving the other tests\n undisturbed.\n\n We'll also add a new test which covers the missing reflog case;\n previously it segfaulted, but now it reports the empty reflog).\n\nReported-by: Yasushi SHOJI \nSigned-off-by: Jeff King \n---\n refs.c | 48 +++++++++++--------------------------\n t/t1508-at-combinations.sh | 4 ++--\n t/t3202-show-branch.sh | 49 ++++++++++++++++++++++++++------------\n 3 files changed, 50 insertions(+), 51 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c633abf284..ba1a4db754 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1038,55 +1038,40 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n \t\t\t const char *message, void *cb_data)\n {\n \tstruct read_ref_at_cb *cb = cb_data;\n-\tint reached_count;\n \n \tcb->tz = tz;\n \tcb->date = timestamp;\n \n-\t/*\n-\t * It is not possible for cb->cnt == 0 on the first iteration because\n-\t * that special case is handled in read_ref_at().\n-\t */\n-\tif (cb->cnt > 0)\n-\t\tcb->cnt--;\n-\treached_count = cb->cnt == 0 && !is_null_oid(ooid);\n-\tif (timestamp <= cb->at_time || reached_count) {\n+\tif (timestamp <= cb->at_time || cb->cnt == 0) {\n \t\tset_read_ref_cutoffs(cb, timestamp, tz, message);\n \t\t/*\n \t\t * we have not yet updated cb->[n|o]oid so they still\n \t\t * hold the values for the previous record.\n \t\t */\n-\t\tif (!is_null_oid(&cb->ooid) && !oideq(&cb->ooid, noid))\n-\t\t\twarning(_(\"log for ref %s has gap after %s\"),\n+\t\tif (!is_null_oid(&cb->ooid)) {\n+\t\t\toidcpy(cb->oid, noid);\n+\t\t\tif (!oideq(&cb->ooid, noid))\n+\t\t\t\twarning(_(\"log for ref %s has gap after %s\"),\n \t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n-\t\tif (reached_count)\n-\t\t\toidcpy(cb->oid, ooid);\n-\t\telse if (!is_null_oid(&cb->ooid) || cb->date == cb->at_time)\n+\t\t}\n+\t\telse if (cb->date == cb->at_time)\n \t\t\toidcpy(cb->oid, noid);\n \t\telse if (!oideq(noid, cb->oid))\n \t\t\twarning(_(\"log for ref %s unexpectedly ended on %s\"),\n \t\t\t\tcb->refname, show_date(cb->date, cb->tz,\n \t\t\t\t\t\t DATE_MODE(RFC2822)));\n+\t\tcb->reccnt++;\n+\t\toidcpy(&cb->ooid, ooid);\n+\t\toidcpy(&cb->noid, noid);\n \t\tcb->found_it = 1;\n+\t\treturn 1;\n \t}\n \tcb->reccnt++;\n \toidcpy(&cb->ooid, ooid);\n \toidcpy(&cb->noid, noid);\n-\treturn cb->found_it;\n-}\n-\n-static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,\n-\t\t\t\t struct object_id *noid,\n-\t\t\t\t const char *email UNUSED,\n-\t\t\t\t timestamp_t timestamp, int tz,\n-\t\t\t\t const char *message, void *cb_data)\n-{\n-\tstruct read_ref_at_cb *cb = cb_data;\n-\n-\tset_read_ref_cutoffs(cb, timestamp, tz, message);\n-\toidcpy(cb->oid, noid);\n-\t/* We just want the first entry */\n-\treturn 1;\n+\tif (cb->cnt > 0)\n+\t\tcb->cnt--;\n+\treturn 0;\n }\n \n static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,\n@@ -1121,11 +1106,6 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n \tcb.cutoff_cnt = cutoff_cnt;\n \tcb.oid = oid;\n \n-\tif (cb.cnt == 0) {\n-\t\trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);\n-\t\treturn 0;\n-\t}\n-\n \trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);\n \n \tif (!cb.reccnt) {\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex e841309d0e..3e5f32f604 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -103,14 +103,14 @@ test_expect_success 'create path with @' '\n check \"@:normal\" blob content\n check \"@:fun@ny\" blob content\n \n-test_expect_success '@{1} works with only one reflog entry' '\n+test_expect_failure '@{1} works with only one reflog entry' '\n \tgit checkout -B newbranch main &&\n \tgit reflog expire --expire=now refs/heads/newbranch &&\n \tgit commit --allow-empty -m \"first after expiration\" &&\n \ttest_cmp_rev newbranch~ newbranch@{1}\n '\n \n-test_expect_success '@{0} works with empty reflog' '\n+test_expect_failure '@{0} works with empty reflog' '\n \tgit checkout -B newbranch main &&\n \tgit reflog expire --expire=now refs/heads/newbranch &&\n \ttest_cmp_rev newbranch newbranch@{0}\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex 6a98b2df76..35f35f8091 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -4,9 +4,6 @@ test_description='test show-branch'\n \n . ./test-lib.sh\n \n-# arbitrary reference time: 2009-08-30 19:20:00\n-GIT_TEST_DATE_NOW=1251660000; export GIT_TEST_DATE_NOW\n-\n test_expect_success 'error descriptions on empty repository' '\n \tcurrent=$(git branch --show-current) &&\n \tcat >expect <<-EOF &&\n@@ -187,18 +184,6 @@ test_expect_success 'show branch --merge-base with N arguments' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'show branch --reflog=2' '\n-\tsed \"s/^>\t//\" >expect <<-\\EOF &&\n-\t>\t! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10\n-\t>\t ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10\n-\t>\t--\n-\t>\t+ [refs/heads/branch10@{0}] branch10\n-\t>\t++ [refs/heads/branch10@{1}] initial\n-\tEOF\n-\tgit show-branch --reflog=2 >actual &&\n-\ttest_cmp actual expect\n-'\n-\n # incompatible options\n while read combo\n do\n@@ -264,4 +249,38 @@ test_expect_success 'error descriptions on orphan branch' '\n \ttest_branch_op_in_wt -c new-branch\n '\n \n+test_expect_success 'setup reflogs' '\n+\ttest_commit base &&\n+\tgit checkout -b branch &&\n+\ttest_commit one &&\n+\tgit reset --hard HEAD^ &&\n+\ttest_commit two &&\n+\ttest_commit three\n+'\n+\n+test_expect_success '--reflog shows reflog entries' '\n+\tcat >expect <<-\\EOF &&\n+\t! [branch@{0}] (0 seconds ago) commit: three\n+\t ! [branch@{1}] (60 seconds ago) commit: two\n+\t ! [branch@{2}] (2 minutes ago) reset: moving to HEAD^\n+\t ! [branch@{3}] (2 minutes ago) commit: one\n+\t----\n+\t+ [branch@{0}] three\n+\t++ [branch@{1}] two\n+\t + [branch@{3}] one\n+\t++++ [branch@{2}] base\n+\tEOF\n+\t# the output always contains relative timestamps; use\n+\t# a known time to get deterministic results\n+\tGIT_TEST_DATE_NOW=$test_tick \\\n+\tgit show-branch --reflog branch >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--reflog handles missing reflog' '\n+\tgit reflog expire --expire=now branch &&\n+\ttest_must_fail git show-branch --reflog branch 2>err &&\n+\tgrep \"log .* is empty\" err\n+'\n+\n test_done\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489363","messageId":"20240226100407.GB2685600@coredump.intra.peff.net","threadId":"60970","inReplyTo":"20240226100010.GA1214708@coredump.intra.peff.net","subject":"[PATCH 2/3] get_oid_basic(): special-case ref@{n} for oldest reflog entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-26T10:04:07Z","receivedAt":"2024-02-26T10:04:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The goal of 6436a20284 (refs: allow @{n} to work with n-sized reflog,\n2021-01-07) was that if we have \"n\" entries in a reflog, we should still\nbe able to resolve ref@{n} by looking at the \"old\" value of the oldest\nentry.\n\nCommit 6436a20284 tried to put the logic into read_ref_at() by shifting\nits idea of \"n\" by one. But we reverted that in the previous commit,\nsince it led to bugs in other callers which cared about the details of\nthe reflog entry we found. Instead, let's put the special case into the\ncaller that resolves @{n}, as it cares only about the oid.\n\nread_ref_at() is even kind enough to return the \"old\" value from the\nfinal reflog; it just returns \"1\" to signal to us that we ran off the\nend of the reflog. But we can notice in the caller that we read just\nenough records for that \"old\" value to be the one we're looking for, and\nuse it.\n\nNote that read_ref_at() could notice this case, too, and just return 0.\nBut we don't want to do that, because the caller must be made aware that\nwe only found the oid, not an actual reflog entry (and the call sites in\nshow-branch do care about this).\n\nThere is one complication, though. When read_ref_at() hits a truncated\nreflog, it will return the \"old\" value of the oldest entry only if it is\nnot the null oid. Otherwise, it actually returns the \"new\" value from\nthat entry! This bit of fudging is due to d1a4489a56 (avoid null SHA1 in\noldest reflog, 2008-07-08), where asking for \"ref@{20.years.ago}\" for a\nref created recently will produce the initial value as a convenience\n(even though technically it did not exist 20 years ago).\n\nBut this convenience is only useful for time-based cutoffs. For\ncount-based cutoffs, get_oid_basic() has always simply complained about\ngoing too far back:\n\n $ git rev-parse HEAD@{20}\n fatal: log for 'HEAD' only has 16 entries\n\nand we should continue to do so, rather than returning a nonsense value\n(there's even a test in t1508 already which covers this). So let's have\nthe d1a4489a56 code kick in only when doing timestamp-based cutoffs.\n\nSigned-off-by: Jeff King \n---\n object-name.c | 9 +++++++++\n refs.c | 2 +-\n t/t1508-at-combinations.sh | 2 +-\n 3 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 3a2ef5d680..511f09bc0f 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1034,6 +1034,15 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\t\t\t\t\tlen, str,\n \t\t\t\t\t\tshow_date(co_time, co_tz, DATE_MODE(RFC2822)));\n \t\t\t\t}\n+\t\t\t} else if (nth == co_cnt && !is_null_oid(oid)) {\n+\t\t\t\t/*\n+\t\t\t\t * We were asked for the Nth reflog (counting\n+\t\t\t\t * from 0), but there were only N entries.\n+\t\t\t\t * read_ref_at() will have returned \"1\" to tell\n+\t\t\t\t * us it did not find an entry, but it did\n+\t\t\t\t * still fill in the oid with the \"old\" value,\n+\t\t\t\t * which we can use.\n+\t\t\t\t */\n \t\t\t} else {\n \t\t\t\tif (flags & GET_OID_QUIETLY) {\n \t\t\t\t\texit(128);\ndiff --git a/refs.c b/refs.c\nindex ba1a4db754..6b826b002e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1083,7 +1083,7 @@ static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid\n \n \tset_read_ref_cutoffs(cb, timestamp, tz, message);\n \toidcpy(cb->oid, ooid);\n-\tif (is_null_oid(cb->oid))\n+\tif (cb->at_time && is_null_oid(cb->oid))\n \t\toidcpy(cb->oid, noid);\n \t/* We just want the first entry */\n \treturn 1;\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 3e5f32f604..370bf7137e 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -103,7 +103,7 @@ test_expect_success 'create path with @' '\n check \"@:normal\" blob content\n check \"@:fun@ny\" blob content\n \n-test_expect_failure '@{1} works with only one reflog entry' '\n+test_expect_success '@{1} works with only one reflog entry' '\n \tgit checkout -B newbranch main &&\n \tgit reflog expire --expire=now refs/heads/newbranch &&\n \tgit commit --allow-empty -m \"first after expiration\" &&\n-- \n2.44.0.rc2.424.gbdbf4d014b\n\n"},{"id":"489364","messageId":"20240226100803.GC2685600@coredump.intra.peff.net","threadId":"60970","inReplyTo":"20240226100010.GA1214708@coredump.intra.peff.net","subject":"[PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-26T10:08:03Z","receivedAt":"2024-02-26T10:08:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit special-cased get_oid_basic()'s handling of ref@{n}\nfor a reflog with n entries. But its special case doesn't work for\nref@{0} in an empty reflog, because read_ref_at() dies when it notices\nthe empty reflog!\n\nWe can make this work by special-casing this in read_ref_at(). It's\nsomewhat gross, for two reasons:\n\n 1. We have no reflog entry to describe in the \"msg\" out-parameter. So\n we have to leave it uninitialized or make something up.\n\n 2. Likewise, we have no oid to put in the \"oid\" out-parameter. Leaving\n it untouched is actually the best thing here, as all of the callers\n will have initialized it with the current ref value via\n repo_dwim_log(). This is rather subtle, but it is how things worked\n in 6436a20284 (refs: allow @{n} to work with n-sized reflog,\n 2021-01-07) before we reverted it.\n\nThe key difference from 6436a20284 here is that we'll return \"1\" to\nindicate that we _didn't_ find the requested reflog entry. Coupled with\nthe special-casing in get_oid_basic() in the previous commit, that's\nenough to make looking up ref@{0} work, and we can flip 6436a20284's\ntest back to expect_success.\n\nIt also means that the call in show-branch which segfaulted with\n6436a20284 (and which is now tested in t3202) remains OK. The caller\nnotices that we could not find any reflog entry, and so it breaks out of\nits loop, showing nothing. This is different from the current behavior\nof producing an error, but it's just as reasonable (and is exactly what\nwe'd do if you asked it to walk starting at ref@{1} but there was only 1\nentry).\n\nThus nobody should actually look at the reflog entry info we return. But\nwe'll still put in some fake values just to be on the safe side, since\nthis is such a subtle and confusing interface. Likewise, we'll document\nwhat's going on in a comment above the function declaration. If this\nwere a function with a lot of callers, the footgun would probably not be\nworth it. But it has only ever had two callers in its 18-year existence,\nand it seems unlikely to grow more. So let's hold our noses and let\nusers enjoy the convenience of a simulated ref@{0}.\n\nSigned-off-by: Jeff King \n---\n refs.c | 15 +++++++++++++++\n refs.h | 15 ++++++++++++++-\n t/t1508-at-combinations.sh | 2 +-\n t/t3202-show-branch.sh | 4 ++--\n 4 files changed, 32 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6b826b002e..8eaec5eca7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1109,6 +1109,21 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n \trefs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);\n \n \tif (!cb.reccnt) {\n+\t\tif (cnt == 0) {\n+\t\t\t/*\n+\t\t\t * The caller asked for ref@{0}, and we had no entries.\n+\t\t\t * It's a bit subtle, but in practice all callers have\n+\t\t\t * prepped the \"oid\" field with the current value of\n+\t\t\t * the ref, which is the most reasonable fallback.\n+\t\t\t *\n+\t\t\t * We'll put dummy values into the out-parameters (so\n+\t\t\t * they're not just uninitialized garbage), and the\n+\t\t\t * caller can take our return value as a hint that\n+\t\t\t * we did not find any such reflog.\n+\t\t\t */\n+\t\t\tset_read_ref_cutoffs(&cb, 0, 0, \"empty reflog\");\n+\t\t\treturn 1;\n+\t\t}\n \t\tif (flags & GET_OID_QUIETLY)\n \t\t\texit(128);\n \t\telse\ndiff --git a/refs.h b/refs.h\nindex 303c5fac4d..37116ad2b2 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -440,7 +440,20 @@ int refs_create_reflog(struct ref_store *refs, const char *refname,\n \t\t struct strbuf *err);\n int safe_create_reflog(const char *refname, struct strbuf *err);\n \n-/** Reads log for the value of ref during at_time. **/\n+/**\n+ * Reads log for the value of ref during at_time (in which case \"cnt\" should be\n+ * negative) or the reflog \"cnt\" entries from the top (in which case \"at_time\"\n+ * should be 0).\n+ *\n+ * If we found the reflog entry in question, returns 0 (and details of the\n+ * entry can be found in the out-parameters).\n+ *\n+ * If we ran out of reflog entries, the out-parameters are filled with the\n+ * details of the oldest entry we did find, and the function returns 1. Note\n+ * that there is one important special case here! If the reflog was empty\n+ * and the caller asked for the 0-th cnt, we will return \"1\" but leave the\n+ * \"oid\" field untouched.\n+ **/\n int read_ref_at(struct ref_store *refs,\n \t\tconst char *refname, unsigned int flags,\n \t\ttimestamp_t at_time, int cnt,\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 370bf7137e..e841309d0e 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -110,7 +110,7 @@ test_expect_success '@{1} works with only one reflog entry' '\n \ttest_cmp_rev newbranch~ newbranch@{1}\n '\n \n-test_expect_failure '@{0} works with empty reflog' '\n+test_expect_success '@{0} works with empty reflog' '\n \tgit checkout -B newbranch main &&\n \tgit reflog expire --expire=now refs/heads/newbranch &&\n \ttest_cmp_rev newbranch newbranch@{0}\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex 35f35f8091..a1139f79e2 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -279,8 +279,8 @@ test_expect_success '--reflog shows reflog entries' '\n \n test_expect_success '--reflog handles missing reflog' '\n \tgit reflog expire --expire=now branch &&\n-\ttest_must_fail git show-branch --reflog branch 2>err &&\n-\tgrep \"log .* is empty\" err\n+\tgit show-branch --reflog branch >actual &&\n+\ttest_must_be_empty actual\n '\n \n test_done\n-- \n2.44.0.rc2.424.gbdbf4d014b\n"},{"id":"489365","messageId":"20240226101027.GA2685773@coredump.intra.peff.net","threadId":"60970","inReplyTo":"20240226100803.GC2685600@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-26T10:10:27Z","receivedAt":"2024-02-26T10:10:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2024 at 05:08:03AM -0500, Jeff King wrote:\n\n> Thus nobody should actually look at the reflog entry info we return. But\n> we'll still put in some fake values just to be on the safe side, since\n> this is such a subtle and confusing interface. Likewise, we'll document\n> what's going on in a comment above the function declaration. If this\n> were a function with a lot of callers, the footgun would probably not be\n> worth it. But it has only ever had two callers in its 18-year existence,\n> and it seems unlikely to grow more. So let's hold our noses and let\n> users enjoy the convenience of a simulated ref@{0}.\n\nObviously I'm sympathetic to Patrick's position that this empty-reflog\nspecial case is kind of gross. ;)\n\nThat's one of the reasons I split this out from patch 2; we can see\nexactly what must be done to make each case work. And in fact I had\noriginally started to write a patch that simply changed t1508 to expect\nfailure. I could still be persuaded to go that way if anybody feels\nstrongly.\n\n-Peff\n"},{"id":"489380","messageId":"xmqqjzmrgqs1.fsf@gitster.g","threadId":"60970","inReplyTo":"20240226100407.GB2685600@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] get_oid_basic(): special-case ref@{n} for oldest reflog entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T15:59:42Z","receivedAt":"2024-02-26T15:59:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> the reflog entry we found. Instead, let's put the special case into the\n> caller that resolves @{n}, as it cares only about the oid.\n\nThanks. I am quite puzzled why we didn't do this obvious thing from\nthe start.\n\n> There is one complication, though. When read_ref_at() hits a truncated\n> reflog, it will return the \"old\" value of the oldest entry only if it is\n> not the null oid. Otherwise, it actually returns the \"new\" value from\n> that entry! This bit of fudging is due to d1a4489a56 (avoid null SHA1 in\n> oldest reflog, 2008-07-08), where asking for \"ref@{20.years.ago}\" for a\n> ref created recently will produce the initial value as a convenience\n> (even though technically it did not exist 20 years ago).\n>\n> But this convenience is only useful for time-based cutoffs. For\n> count-based cutoffs, get_oid_basic() has always simply complained about\n> going too far back:\n>\n> $ git rev-parse HEAD@{20}\n> fatal: log for 'HEAD' only has 16 entries\n>\n> and we should continue to do so, rather than returning a nonsense value\n> (there's even a test in t1508 already which covers this). So let's have\n> the d1a4489a56 code kick in only when doing timestamp-based cutoffs.\n\nMakes perfect sense.\n\n> Signed-off-by: Jeff King \n> ---\n> object-name.c | 9 +++++++++\n> refs.c | 2 +-\n> t/t1508-at-combinations.sh | 2 +-\n> 3 files changed, 11 insertions(+), 2 deletions(-)\n"},{"id":"489391","messageId":"xmqqmsrncf3r.fsf@gitster.g","threadId":"60970","inReplyTo":"20240226100803.GC2685600@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T17:25:28Z","receivedAt":"2024-02-26T17:25:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> \tif (!cb.reccnt) {\n> +\t\tif (cnt == 0) {\n\nStyle \"if (!cnt)\" ? In this particular case I do not think it\nactually is an improvement, though, simply because zero is really\nspecial in this logic.\n\n> +\t\t\t/*\n> +\t\t\t * The caller asked for ref@{0}, and we had no entries.\n> +\t\t\t * It's a bit subtle, but in practice all callers have\n> +\t\t\t * prepped the \"oid\" field with the current value of\n> +\t\t\t * the ref, which is the most reasonable fallback.\n> +\t\t\t *\n> +\t\t\t * We'll put dummy values into the out-parameters (so\n> +\t\t\t * they're not just uninitialized garbage), and the\n> +\t\t\t * caller can take our return value as a hint that\n> +\t\t\t * we did not find any such reflog.\n> +\t\t\t */\n> +\t\t\tset_read_ref_cutoffs(&cb, 0, 0, \"empty reflog\");\n> +\t\t\treturn 1;\n> +\t\t}\n\nThe dummy value I 100% agree with ;-).\n\nYou mentioned the convenience special case for time-based reflog\nquery for a time older than (e.g. @{20.years.ago}) the reflog\nitself, and perhaps this one should be treated as its counterpart,\nthat is only useful for count-based access.\n\n"},{"id":"489392","messageId":"xmqqh6hvcf3n.fsf@gitster.g","threadId":"60970","inReplyTo":"20240226101027.GA2685773@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-26T17:25:32Z","receivedAt":"2024-02-26T17:25:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That's one of the reasons I split this out from patch 2; we can see\n> exactly what must be done to make each case work. And in fact I had\n> originally started to write a patch that simply changed t1508 to expect\n> failure. I could still be persuaded to go that way if anybody feels\n> strongly.\n\nI do not feel strongly either way myself. It just is interesting\nthat the older end of the history is with @{20.years.ago} special\ncase that is only for time-based query, while the newer end of the\nhistory is with @{0} special case.\n"},{"id":"489468","messageId":"20240227080501.GF3263678@coredump.intra.peff.net","threadId":"60970","inReplyTo":"xmqqmsrncf3r.fsf@gitster.g","subject":"Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-27T08:05:01Z","receivedAt":"2024-02-27T08:05:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2024 at 09:25:28AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > \tif (!cb.reccnt) {\n> > +\t\tif (cnt == 0) {\n> \n> Style \"if (!cnt)\" ? In this particular case I do not think it\n> actually is an improvement, though, simply because zero is really\n> special in this logic.\n\nYeah, I didn't really choose \"== 0\" as a conscious decision. But I do\ntend to write \"!cnt\" when available, so I think I sub-consciously chose\nthis as a better description of the intent.\n\n> \n> > +\t\t\t/*\n> > +\t\t\t * The caller asked for ref@{0}, and we had no entries.\n> > +\t\t\t * It's a bit subtle, but in practice all callers have\n> > +\t\t\t * prepped the \"oid\" field with the current value of\n> > +\t\t\t * the ref, which is the most reasonable fallback.\n> > +\t\t\t *\n> > +\t\t\t * We'll put dummy values into the out-parameters (so\n> > +\t\t\t * they're not just uninitialized garbage), and the\n> > +\t\t\t * caller can take our return value as a hint that\n> > +\t\t\t * we did not find any such reflog.\n> > +\t\t\t */\n> > +\t\t\tset_read_ref_cutoffs(&cb, 0, 0, \"empty reflog\");\n> > +\t\t\treturn 1;\n> > +\t\t}\n> \n> The dummy value I 100% agree with ;-).\n> \n> You mentioned the convenience special case for time-based reflog\n> query for a time older than (e.g. @{20.years.ago}) the reflog\n> itself, and perhaps this one should be treated as its counterpart,\n> that is only useful for count-based access.\n\nI think the true counterpart to that would be extending what's added by\npatch 2 to get_oid_basic() to stop checking that we hit the count\nexactly.\n\nLet me try to lay out my thinking. If you _do_ have a reflog and the\nrequest (whether time or count-based) goes too far back, read_ref_at()\nwill give you the oldest entry and return \"1\". And then in\nget_oid_basic():\n\n - if it was a time based cutoff (like @{20.years.ago}), we will issue\n a warning (\"log only goes back to 2024-01-01\") but return the value\n anyway.\n\n - before this series, if it is a count based cutoff (like @{9999}), we\n fail and say \"there are only have N entries\".\n\n - after this series, we special case asking for @{9999} when there are\n 9998 entries by returning the \"old\" oid but not issuing any warning\n (we do not need to, because we found the right answer for what it\n would have been had that old entry not been pruned).\n\n - what we _could_ do (but this series does not), and what would be the\n true counterpart to the @{20.years.ago} case, is to allow @{9999}\n for a reflog with only 20 entries, returning the old value from 20\n (or the new value if it was a creation!?) and issuing a warning\n saying \"well, it only went back 20, but here you go\".\n\nI'm not so sure about that last one. It is pretty subjective, but\nsomehow asking for timestamps feels more \"fuzzy\" to me, and Git\nreturning a fuzzy answer is OK. Whereas asking for item 9999 in a list\nwith 20 items and getting back an answer feels more absolutely wrong. I\ncould be persuaded if there were a concrete use case, but I can't really\nthink of one. It seems more likely to confuse and hinder a user than to\nhelp them.\n\n-Peff\n"},{"id":"489469","messageId":"20240227080732.GG3263678@coredump.intra.peff.net","threadId":"60970","inReplyTo":"xmqqh6hvcf3n.fsf@gitster.g","subject":"Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-27T08:07:32Z","receivedAt":"2024-02-27T08:07:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 26, 2024 at 09:25:32AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > That's one of the reasons I split this out from patch 2; we can see\n> > exactly what must be done to make each case work. And in fact I had\n> > originally started to write a patch that simply changed t1508 to expect\n> > failure. I could still be persuaded to go that way if anybody feels\n> > strongly.\n> \n> I do not feel strongly either way myself. It just is interesting\n> that the older end of the history is with @{20.years.ago} special\n> case that is only for time-based query, while the newer end of the\n> history is with @{0} special case.\n\nI laid out my thinking on the 20.years.ago special case in another\nreply, but I wanted to say one more thing. The special case here in\npatch 3 is making @{0} work for the empty reflog, but there is no\nmatching special case for time-based timestamps. If you have an empty\nreflog and ask for @{20.years.ago}, you will get the usual \"nope, the\nreflog is empty\" response (as opposed to having a non-empty reflog that\ncuts off before 20 years ago).\n\nObviously we could make that work, but I think the point is that \"@{0}\"\nis special magic for \"the current value\" in a way that a timestamp isn't\nreally.\n\n-Peff\n"},{"id":"489548","messageId":"xmqqil29vnyh.fsf@gitster.g","threadId":"60970","inReplyTo":"20240227080501.GF3263678@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] read_ref_at(): special-case ref@{0} for an empty reflog","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-27T17:03:50Z","receivedAt":"2024-02-27T17:04:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Let me try to lay out my thinking. If you _do_ have a reflog and the\n> request (whether time or count-based) goes too far back, read_ref_at()\n> will give you the oldest entry and return \"1\". And then in\n> get_oid_basic():\n> ...\n> - what we _could_ do (but this series does not), and what would be the\n> true counterpart to the @{20.years.ago} case, is to allow @{9999}\n> for a reflog with only 20 entries, returning the old value from 20\n> (or the new value if it was a creation!?) and issuing a warning\n> saying \"well, it only went back 20, but here you go\".\n\nAh, I wasn't drawing *that* similarity. My thinking was more like\n\n - When you have two entries in reflog, ref@{0} will use and find\n the latest entry whose value is the same as the ref itself.\n\n - When you have one entry, @{0} will use and find the latest entry\n whose value is the same as the ref itself.\n\n - When you have zero entry, @{0} can do the same by taking\n advantage of the fact that its value is supposed to be the same\n as the ref itself anyway.\n\nthat happens near the youngest end of a reflog, contrasting with the\n@{20.years.ago} that happens near the oldest end.\n\n> I'm not so sure about that last one. It is pretty subjective, but\n> somehow asking for timestamps feels more \"fuzzy\" to me, and Git\n> returning a fuzzy answer is OK. Whereas asking for item 9999 in a list\n> with 20 items and getting back an answer feels more absolutely wrong. I\n> could be persuaded if there were a concrete use case, but I can't really\n> think of one. It seems more likely to confuse and hinder a user than to\n> help them.\n\nI do not think anybody misses @{9999} not giving the oldest\navailable, simply because \"oldest\" is a concept that fits better\nwith time-based queries than count-based queries.\n"}]}