{"thread":{"id":"60955","subject":"[PATCH 0/6] reflog: introduce subcommand to list reflogs","startedAt":"2024-02-19T14:35:19Z","lastAt":"2024-04-24T14:53:32Z","messageCount":39,"participants":["Patrick Steinhardt","Junio C Hamano","Teng Long"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"488908","messageId":"cover.1708353264.git.ps@pks.im","threadId":"60955","inReplyTo":null,"subject":"[PATCH 0/6] reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-19T14:35:14Z","receivedAt":"2024-02-19T14:35:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series introduces a new `git reflog list` subcommand that\nlists all reflogs of the current repository. This addresses an issue\nwith discoverability as there is no way for a user to learn about which\nreflogs exist. While this isn't all that bad with the \"files\" backend as\na user could in the worst case figure out which reflogs exist by walking\nthe \".git/logs\" directory, with the \"reftable\" backend it's basically\nimpossible to achieve this.\n\nWhile I think this is sufficient motivation to have such a subcommand\nnowadays already, I think the need for such a thing will grow in the\nfuture. It was noted in multiple threads that we may eventually want to\nlift the artificial limitations in the \"reftable\" backend where reflogs\nare deleted together with their refs. This limitation is inherited from\nthe \"files\" backend, which may otherwise hit issues with directory/file\nconflicts if it didn't delete reflogs.\n\nOnce that limitation is lifted for the \"reftable\" backend though, it\nwill become even more important to give users the tools to discover\nreflogs that do not have a corresponding ref.\n\nThe series is structured as follows:\n\n  - Patches 1-3 extend the dir iterator so that it can sort directory\n    entries lexicographically. This is required such that we can list\n    reflogs with deterministic ordering.\n\n  - Patch 4 refactors the reflog iterator interface to demonstrate that\n    the object ID and flags aren't needed nowadays, and patch 5 builds\n    on top of that and stops resolving the refs altogether. This allows\n    us to also surface reflogs of broken refs.\n\n  - Patch 6 introduces the new subcommand.\n\nThe series depends on Junio's ps/reftable-backend at 8a0bebdeae\n(refs/reftable: fix leak when copying reflog fails, 2024-02-08): the\nchange in behaviour in patches 4 and 5 apply to both backends.\n\nPatrick\n\nPatrick Steinhardt (6):\n  dir-iterator: pass name to `prepare_next_entry_data()` directly\n  dir-iterator: support iteration in sorted order\n  refs/files: sort reflogs returned by the reflog iterator\n  refs: drop unused params from the reflog iterator callback\n  refs: stop resolving ref corresponding to reflogs\n  builtin/reflog: introduce subcommand to list reflogs\n\n Documentation/git-reflog.txt   |  3 ++\n builtin/fsck.c                 |  4 +-\n builtin/reflog.c               | 37 +++++++++++++-\n dir-iterator.c                 | 93 ++++++++++++++++++++++++++--------\n dir-iterator.h                 |  3 ++\n refs.c                         | 23 +++++++--\n refs.h                         | 11 +++-\n refs/files-backend.c           | 22 ++------\n refs/reftable-backend.c        | 12 +----\n revision.c                     |  4 +-\n t/helper/test-ref-store.c      | 18 ++++---\n t/t0600-reffiles-backend.sh    | 24 ++++-----\n t/t1405-main-ref-store.sh      |  8 +--\n t/t1406-submodule-ref-store.sh |  8 +--\n t/t1410-reflog.sh              | 69 +++++++++++++++++++++++++\n 15 files changed, 251 insertions(+), 88 deletions(-)\n\n-- \n2.44.0-rc1\n\n"},{"id":"488909","messageId":"12de25dfe24d61ef54e0ccc0ebd4cc69d73da50c.1708353264.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708353264.git.ps@pks.im","subject":"[PATCH 1/6] dir-iterator: pass name to `prepare_next_entry_data()` directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-19T14:35:18Z","receivedAt":"2024-02-19T14:35:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When adding the next directory entry for `struct dir_iterator` we pass\nthe complete `struct dirent *` to `prepare_next_entry_data()` even\nthough we only need the entry's name.\n\nRefactor the code to pass in the name, only. This prepares for a\nsubsequent commit where we introduce the ability to iterate through\ndir entries in an ordered manner.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n dir-iterator.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 278b04243a..f58a97e089 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -94,15 +94,15 @@ static int pop_level(struct dir_iterator_int *iter)\n \n /*\n  * Populate iter->base with the necessary information on the next iteration\n- * entry, represented by the given dirent de. Return 0 on success and -1\n+ * entry, represented by the given name. Return 0 on success and -1\n  * otherwise, setting errno accordingly.\n  */\n static int prepare_next_entry_data(struct dir_iterator_int *iter,\n-\t\t\t\t   struct dirent *de)\n+\t\t\t\t   const char *name)\n {\n \tint err, saved_errno;\n \n-\tstrbuf_addstr(&iter->base.path, de->d_name);\n+\tstrbuf_addstr(&iter->base.path, name);\n \t/*\n \t * We have to reset these because the path strbuf might have\n \t * been realloc()ed at the previous strbuf_addstr().\n@@ -159,7 +159,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n-\t\tif (prepare_next_entry_data(iter, de)) {\n+\t\tif (prepare_next_entry_data(iter, de->d_name)) {\n \t\t\tif (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n \t\t\t\tgoto error_out;\n \t\t\tcontinue;\n-- \n2.44.0-rc1\n\n"},{"id":"488910","messageId":"8a588175dbf23d1938db45507812aad8f3793dbb.1708353264.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708353264.git.ps@pks.im","subject":"[PATCH 2/6] dir-iterator: support iteration in sorted order","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-19T14:35:22Z","receivedAt":"2024-02-19T14:35:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct dir_iterator` is a helper that allows us to iterate through\ndirectory entries. This iterator returns entries in the exact same order\nas readdir(3P) does -- or in other words, it guarantees no specific\norder at all.\n\nThis is about to become problematic as we are introducing a new reflog\nsubcommand to list reflogs. As the \"files\" backend uses the directory\niterator to enumerate reflogs, returning reflog names and exposing them\nto the user would inherit the indeterministic ordering. Naturally, it\nwould make for a terrible user interface to show a list with no\ndiscernible order. While this could be handled at a higher level by the\nnew subcommand itself by collecting and ordering the reflogs, this would\nbe inefficient and introduce latency when there are many reflogs.\n\nInstead, introduce a new option into the directory iterator that asks\nfor its entries to be yielded in lexicographical order. If set, the\niterator will read all directory entries greedily end sort them before\nwe start to iterate over them.\n\nWhile this will of course also incur overhead as we cannot yield the\ndirectory entries immediately, it should at least be more efficient than\nhaving to sort the complete list of reflogs as we only need to sort one\ndirectory at a time.\n\nThis functionality will be used in a follow-up commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n dir-iterator.c | 87 ++++++++++++++++++++++++++++++++++++++++----------\n dir-iterator.h |  3 ++\n 2 files changed, 73 insertions(+), 17 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex f58a97e089..396c28178f 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -2,9 +2,12 @@\n #include \"dir.h\"\n #include \"iterator.h\"\n #include \"dir-iterator.h\"\n+#include \"string-list.h\"\n \n struct dir_iterator_level {\n \tDIR *dir;\n+\tstruct string_list entries;\n+\tsize_t entries_idx;\n \n \t/*\n \t * The length of the directory part of path at this level\n@@ -72,6 +75,40 @@ static int push_level(struct dir_iterator_int *iter)\n \t\treturn -1;\n \t}\n \n+\tstring_list_init_dup(&level->entries);\n+\tlevel->entries_idx = 0;\n+\n+\t/*\n+\t * When the iterator is sorted we read and sort all directory entries\n+\t * directly.\n+\t */\n+\tif (iter->flags & DIR_ITERATOR_SORTED) {\n+\t\twhile (1) {\n+\t\t\tstruct dirent *de;\n+\n+\t\t\terrno = 0;\n+\t\t\tde = readdir(level->dir);\n+\t\t\tif (!de) {\n+\t\t\t\tif (errno && errno != ENOENT) {\n+\t\t\t\t\twarning_errno(\"error reading directory '%s'\",\n+\t\t\t\t\t\t      iter->base.path.buf);\n+\t\t\t\t\treturn -1;\n+\t\t\t\t}\n+\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tif (is_dot_or_dotdot(de->d_name))\n+\t\t\t\tcontinue;\n+\n+\t\t\tstring_list_append(&level->entries, de->d_name);\n+\t\t}\n+\t\tstring_list_sort(&level->entries);\n+\n+\t\tclosedir(level->dir);\n+\t\tlevel->dir = NULL;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -88,6 +125,7 @@ static int pop_level(struct dir_iterator_int *iter)\n \t\twarning_errno(\"error closing directory '%s'\",\n \t\t\t      iter->base.path.buf);\n \tlevel->dir = NULL;\n+\tstring_list_clear(&level->entries, 0);\n \n \treturn --iter->levels_nr;\n }\n@@ -136,30 +174,43 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \n \t/* Loop until we find an entry that we can give back to the caller. */\n \twhile (1) {\n-\t\tstruct dirent *de;\n \t\tstruct dir_iterator_level *level =\n \t\t\t&iter->levels[iter->levels_nr - 1];\n+\t\tstruct dirent *de;\n+\t\tconst char *name;\n \n \t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n-\t\terrno = 0;\n-\t\tde = readdir(level->dir);\n-\n-\t\tif (!de) {\n-\t\t\tif (errno) {\n-\t\t\t\twarning_errno(\"error reading directory '%s'\",\n-\t\t\t\t\t      iter->base.path.buf);\n-\t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n-\t\t\t\t\tgoto error_out;\n-\t\t\t} else if (pop_level(iter) == 0) {\n-\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\n+\t\tif (level->dir) {\n+\t\t\terrno = 0;\n+\t\t\tde = readdir(level->dir);\n+\t\t\tif (!de) {\n+\t\t\t\tif (errno) {\n+\t\t\t\t\twarning_errno(\"error reading directory '%s'\",\n+\t\t\t\t\t\t      iter->base.path.buf);\n+\t\t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n+\t\t\t\t\t\tgoto error_out;\n+\t\t\t\t} else if (pop_level(iter) == 0) {\n+\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\t\t}\n+\t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n \n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n+\t\t\tif (is_dot_or_dotdot(de->d_name))\n+\t\t\t\tcontinue;\n \n-\t\tif (prepare_next_entry_data(iter, de->d_name)) {\n+\t\t\tname = de->d_name;\n+\t\t} else {\n+\t\t\tif (level->entries_idx >= level->entries.nr) {\n+\t\t\t\tif (pop_level(iter) == 0)\n+\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\tname = level->entries.items[level->entries_idx++].string;\n+\t\t}\n+\n+\t\tif (prepare_next_entry_data(iter, name)) {\n \t\t\tif (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n \t\t\t\tgoto error_out;\n \t\t\tcontinue;\n@@ -188,6 +239,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n \t\t\twarning_errno(\"error closing directory '%s'\",\n \t\t\t\t      iter->base.path.buf);\n \t\t}\n+\n+\t\tstring_list_clear(&level->entries, 0);\n \t}\n \n \tfree(iter->levels);\ndiff --git a/dir-iterator.h b/dir-iterator.h\nindex 479e1ec784..6d438809b6 100644\n--- a/dir-iterator.h\n+++ b/dir-iterator.h\n@@ -54,8 +54,11 @@\n  *   and ITER_ERROR is returned immediately. In both cases, a meaningful\n  *   warning is emitted. Note: ENOENT errors are always ignored so that\n  *   the API users may remove files during iteration.\n+ *\n+ * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.\n  */\n #define DIR_ITERATOR_PEDANTIC (1 << 0)\n+#define DIR_ITERATOR_SORTED   (1 << 1)\n \n struct dir_iterator {\n \t/* The current path: */\n-- \n2.44.0-rc1\n\n"},{"id":"488911","messageId":"e4e4fac05c7f4bcac8ef96bdebb8a68eef40ead4.1708353264.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708353264.git.ps@pks.im","subject":"[PATCH 3/6] refs/files: sort reflogs returned by the reflog iterator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-19T14:35:26Z","receivedAt":"2024-02-19T14:35:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We use a directory iterator to return reflogs via the reflog iterator.\nThis iterator returns entries in the same order as readdir(3P) would and\nwill thus yield reflogs with no discernible order.\n\nSet the new `DIR_ITERATOR_SORTED` flag that was introduced in the\npreceding commit so that the order is deterministic. While the effect of\nthis can only been observed in a test tool, a subsequent commit will\nstart to expose this functionality to users via a new `git reflog list`\nsubcommand.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/files-backend.c           | 4 ++--\n t/t0600-reffiles-backend.sh    | 4 ++--\n t/t1405-main-ref-store.sh      | 2 +-\n t/t1406-submodule-ref-store.sh | 2 +-\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 75dcc21ecb..2ffc63185f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \n \tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n \n-\tditer = dir_iterator_begin(sb.buf, 0);\n+\tditer = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);\n \tif (!diter) {\n \t\tstrbuf_release(&sb);\n \t\treturn empty_ref_iterator_begin();\n@@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n+\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);\n \titer->dir_iterator = diter;\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\ndiff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh\nindex e6a5f1868f..4f860285cc 100755\n--- a/t/t0600-reffiles-backend.sh\n+++ b/t/t0600-reffiles-backend.sh\n@@ -287,7 +287,7 @@ test_expect_success 'for_each_reflog()' '\n \tmkdir -p     .git/worktrees/wt/logs/refs/bisect &&\n \techo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&\n \n-\t$RWT for-each-reflog | cut -d\" \" -f 2- | sort >actual &&\n+\t$RWT for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \tPSEUDO-WT 0x0\n@@ -297,7 +297,7 @@ test_expect_success 'for_each_reflog()' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\t$RMAIN for-each-reflog | cut -d\" \" -f 2- | sort >actual &&\n+\t$RMAIN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \tPSEUDO-MAIN 0x0\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 976bd71efb..cfb583f544 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -74,7 +74,7 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | sort -k2 | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \trefs/heads/main 0x0\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex e6a7f7334b..40332e23cc 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -63,7 +63,7 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | sort | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \trefs/heads/main 0x0\n-- \n2.44.0-rc1\n\n"},{"id":"488912","messageId":"be512ef268b910852ff11df181d89c483ffc18ab.1708353264.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708353264.git.ps@pks.im","subject":"[PATCH 4/6] refs: drop unused params from the reflog iterator callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-19T14:35:31Z","receivedAt":"2024-02-19T14:35:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The ref and reflog iterators share much of the same underlying code to\niterate over the corresponding entries. This results in some weird code\nbecause the reflog iterator also exposes an object ID as well as a flag\nto the callback function. Neither of these fields do refer to the reflog\nthough -- they refer to the corresponding ref with the same name. This\nis quite misleading. In practice at least the object ID cannot really be\nimplemented in any other way as a reflog does not have a specific object\nID in the first place. This is further stressed by the fact that none of\nthe callbacks except for our test helper make use of these fields.\n\nSplit up the infrastucture so that ref and reflog iterators use separate\ncallback signatures. This allows us to drop the nonsensical fields from\nthe reflog iterator.\n\nNote that internally, the backends still use the same shared infra to\niterate over both types. As the backends should never end up being\ncalled directly anyway, this is not much of a problem and thus kept\nas-is for simplicity's sake.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fsck.c                 |  4 +---\n builtin/reflog.c               |  3 +--\n refs.c                         | 23 +++++++++++++++++++----\n refs.h                         | 11 +++++++++--\n refs/files-backend.c           |  8 +-------\n refs/reftable-backend.c        |  8 +-------\n revision.c                     |  4 +---\n t/helper/test-ref-store.c      | 18 ++++++++++++------\n t/t0600-reffiles-backend.sh    | 24 ++++++++++++------------\n t/t1405-main-ref-store.sh      |  8 ++++----\n t/t1406-submodule-ref-store.sh |  8 ++++----\n 11 files changed, 65 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex a7cf94f67e..f892487c9b 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -509,9 +509,7 @@ static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid\n \treturn 0;\n }\n \n-static int fsck_handle_reflog(const char *logname,\n-\t\t\t      const struct object_id *oid UNUSED,\n-\t\t\t      int flag UNUSED, void *cb_data)\n+static int fsck_handle_reflog(const char *logname, void *cb_data)\n {\n \tstruct strbuf refname = STRBUF_INIT;\n \ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex a5a4099f61..3a0c4d4322 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -60,8 +60,7 @@ struct worktree_reflogs {\n \tstruct string_list reflogs;\n };\n \n-static int collect_reflog(const char *ref, const struct object_id *oid UNUSED,\n-\t\t\t  int flags UNUSED, void *cb_data)\n+static int collect_reflog(const char *ref, void *cb_data)\n {\n \tstruct worktree_reflogs *cb = cb_data;\n \tstruct worktree *worktree = cb->worktree;\ndiff --git a/refs.c b/refs.c\nindex fff343c256..6d76000f13 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2516,18 +2516,33 @@ int refs_verify_refname_available(struct ref_store *refs,\n \treturn ret;\n }\n \n-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n+struct do_for_each_reflog_help {\n+\teach_reflog_fn *fn;\n+\tvoid *cb_data;\n+};\n+\n+static int do_for_each_reflog_helper(struct repository *r UNUSED,\n+\t\t\t\t     const char *refname,\n+\t\t\t\t     const struct object_id *oid UNUSED,\n+\t\t\t\t     int flags,\n+\t\t\t\t     void *cb_data)\n+{\n+\tstruct do_for_each_reflog_help *hp = cb_data;\n+\treturn hp->fn(refname, hp->cb_data);\n+}\n+\n+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n-\tstruct do_for_each_ref_help hp = { fn, cb_data };\n+\tstruct do_for_each_reflog_help hp = { fn, cb_data };\n \n \titer = refs->be->reflog_iterator_begin(refs);\n \n \treturn do_for_each_repo_ref_iterator(the_repository, iter,\n-\t\t\t\t\t     do_for_each_ref_helper, &hp);\n+\t\t\t\t\t     do_for_each_reflog_helper, &hp);\n }\n \n-int for_each_reflog(each_ref_fn fn, void *cb_data)\n+int for_each_reflog(each_reflog_fn fn, void *cb_data)\n {\n \treturn refs_for_each_reflog(get_main_ref_store(the_repository), fn, cb_data);\n }\ndiff --git a/refs.h b/refs.h\nindex 303c5fac4d..895579aeb7 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -534,12 +534,19 @@ int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_dat\n /* youngest entry first */\n int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data);\n \n+/*\n+ * The signature for the callback function for the {refs_,}for_each_reflog()\n+ * functions below. The memory pointed to by the refname argument is only\n+ * guaranteed to be valid for the duration of a single callback invocation.\n+ */\n+typedef int each_reflog_fn(const char *refname, void *cb_data);\n+\n /*\n  * Calls the specified function for each reflog file until it returns nonzero,\n  * and returns the value. Reflog file order is unspecified.\n  */\n-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n-int for_each_reflog(each_ref_fn fn, void *cb_data);\n+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);\n+int for_each_reflog(each_reflog_fn fn, void *cb_data);\n \n #define REFNAME_ALLOW_ONELEVEL 1\n #define REFNAME_REFSPEC_PATTERN 2\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2ffc63185f..2b3c99b00d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2116,10 +2116,8 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,\n \n struct files_reflog_iterator {\n \tstruct ref_iterator base;\n-\n \tstruct ref_store *ref_store;\n \tstruct dir_iterator *dir_iterator;\n-\tstruct object_id oid;\n };\n \n static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n@@ -2130,8 +2128,6 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \tint ok;\n \n \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n-\t\tint flags;\n-\n \t\tif (!S_ISREG(diter->st.st_mode))\n \t\t\tcontinue;\n \t\tif (diter->basename[0] == '.')\n@@ -2141,14 +2137,12 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \n \t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n \t\t\t\t\t     diter->relative_path, 0,\n-\t\t\t\t\t     &iter->oid, &flags)) {\n+\t\t\t\t\t     NULL, NULL)) {\n \t\t\terror(\"bad ref for %s\", diter->path.buf);\n \t\t\tcontinue;\n \t\t}\n \n \t\titer->base.refname = diter->relative_path;\n-\t\titer->base.oid = &iter->oid;\n-\t\titer->base.flags = flags;\n \t\treturn ITER_OK;\n \t}\n \ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex a14f2ad7f4..889bb1f1ba 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1637,7 +1637,6 @@ struct reftable_reflog_iterator {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_iterator iter;\n \tstruct reftable_log_record log;\n-\tstruct object_id oid;\n \tchar *last_name;\n \tint err;\n };\n@@ -1648,8 +1647,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t(struct reftable_reflog_iterator *)ref_iterator;\n \n \twhile (!iter->err) {\n-\t\tint flags;\n-\n \t\titer->err = reftable_iterator_next_log(&iter->iter, &iter->log);\n \t\tif (iter->err)\n \t\t\tbreak;\n@@ -1663,7 +1660,7 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t\tcontinue;\n \n \t\tif (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,\n-\t\t\t\t\t     0, &iter->oid, &flags)) {\n+\t\t\t\t\t     0, NULL, NULL)) {\n \t\t\terror(_(\"bad ref for %s\"), iter->log.refname);\n \t\t\tcontinue;\n \t\t}\n@@ -1671,8 +1668,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tfree(iter->last_name);\n \t\titer->last_name = xstrdup(iter->log.refname);\n \t\titer->base.refname = iter->log.refname;\n-\t\titer->base.oid = &iter->oid;\n-\t\titer->base.flags = flags;\n \n \t\tbreak;\n \t}\n@@ -1725,7 +1720,6 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n \titer = xcalloc(1, sizeof(*iter));\n \tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);\n \titer->refs = refs;\n-\titer->base.oid = &iter->oid;\n \n \tret = refs->err;\n \tif (ret)\ndiff --git a/revision.c b/revision.c\nindex 2424c9bd67..ac45c6d8f2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1686,9 +1686,7 @@ static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n \treturn 0;\n }\n \n-static int handle_one_reflog(const char *refname_in_wt,\n-\t\t\t     const struct object_id *oid UNUSED,\n-\t\t\t     int flag UNUSED, void *cb_data)\n+static int handle_one_reflog(const char *refname_in_wt, void *cb_data)\n {\n \tstruct all_refs_cb *cb = cb_data;\n \tstruct strbuf refname = STRBUF_INIT;\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 702ec1f128..7a0f6cac53 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -221,15 +221,21 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)\n \treturn ret;\n }\n \n+static int each_reflog(const char *refname, void *cb_data UNUSED)\n+{\n+\tprintf(\"%s\\n\", refname);\n+\treturn 0;\n+}\n+\n static int cmd_for_each_reflog(struct ref_store *refs,\n \t\t\t       const char **argv UNUSED)\n {\n-\treturn refs_for_each_reflog(refs, each_ref, NULL);\n+\treturn refs_for_each_reflog(refs, each_reflog, NULL);\n }\n \n-static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n-\t\t       const char *committer, timestamp_t timestamp,\n-\t\t       int tz, const char *msg, void *cb_data UNUSED)\n+static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,\n+\t\t\t   const char *committer, timestamp_t timestamp,\n+\t\t\t   int tz, const char *msg, void *cb_data UNUSED)\n {\n \tprintf(\"%s %s %s %\" PRItime \" %+05d%s%s\", oid_to_hex(old_oid),\n \t       oid_to_hex(new_oid), committer, timestamp, tz,\n@@ -241,14 +247,14 @@ static int cmd_for_each_reflog_ent(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n \n-\treturn refs_for_each_reflog_ent(refs, refname, each_reflog, refs);\n+\treturn refs_for_each_reflog_ent(refs, refname, each_reflog_ent, refs);\n }\n \n static int cmd_for_each_reflog_ent_reverse(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n \n-\treturn refs_for_each_reflog_ent_reverse(refs, refname, each_reflog, refs);\n+\treturn refs_for_each_reflog_ent_reverse(refs, refname, each_reflog_ent, refs);\n }\n \n static int cmd_reflog_exists(struct ref_store *refs, const char **argv)\ndiff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh\nindex 4f860285cc..56a3196b83 100755\n--- a/t/t0600-reffiles-backend.sh\n+++ b/t/t0600-reffiles-backend.sh\n@@ -287,23 +287,23 @@ test_expect_success 'for_each_reflog()' '\n \tmkdir -p     .git/worktrees/wt/logs/refs/bisect &&\n \techo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&\n \n-\t$RWT for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RWT for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\tPSEUDO-WT 0x0\n-\trefs/bisect/wt-random 0x0\n-\trefs/heads/main 0x0\n-\trefs/heads/wt-main 0x0\n+\tHEAD\n+\tPSEUDO-WT\n+\trefs/bisect/wt-random\n+\trefs/heads/main\n+\trefs/heads/wt-main\n \tEOF\n \ttest_cmp expected actual &&\n \n-\t$RMAIN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RMAIN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\tPSEUDO-MAIN 0x0\n-\trefs/bisect/random 0x0\n-\trefs/heads/main 0x0\n-\trefs/heads/wt-main 0x0\n+\tHEAD\n+\tPSEUDO-MAIN\n+\trefs/bisect/random\n+\trefs/heads/main\n+\trefs/heads/wt-main\n \tEOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex cfb583f544..3eee758bce 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -74,11 +74,11 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\trefs/heads/main 0x0\n-\trefs/heads/new-main 0x0\n+\tHEAD\n+\trefs/heads/main\n+\trefs/heads/new-main\n \tEOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex 40332e23cc..c01f0f14a1 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -63,11 +63,11 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\trefs/heads/main 0x0\n-\trefs/heads/new-main 0x0\n+\tHEAD\n+\trefs/heads/main\n+\trefs/heads/new-main\n \tEOF\n \ttest_cmp expected actual\n '\n-- \n2.44.0-rc1\n\n"},{"id":"488913","messageId":"a7459b9483660d1a44df500aaee85ad38146eb02.1708353264.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708353264.git.ps@pks.im","subject":"[PATCH 5/6] refs: stop resolving ref corresponding to reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-19T14:35:35Z","receivedAt":"2024-02-19T14:35:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reflog iterator tries to resolve the corresponding ref for every\nreflog that it is about to yield. Historically, this was done due to\nmultiple reasons:\n\n  - It ensures that the refname is safe because we end up calling\n    `check_refname_format()`. Also, non-conformant refnames are skipped\n    altogether.\n\n  - The iterator used to yield the resolved object ID as well as its\n    flags to the callback. This info was never used though, and the\n    corresponding parameters were dropped in the preceding commit.\n\n  - When a ref is corrupt then the reflog is not emitted at all.\n\nWe're about to introduce a new `git reflog list` subcommand that will\nprint all reflogs that the refdb knows about. Skipping over reflogs\nwhose refs are corrupted would be quite counterproductive in this case\nas the user would have no way to learn about reflogs which may still\nexist in their repository to help and rescue such a corrupted ref. Thus,\nthe only remaining reason for why we'd want to resolve the ref is to\nverify its refname.\n\nRefactor the code to call `check_refname_format()` directly instead of\ntrying to resolve the ref. This is significantly more efficient given\nthat we don't have to hit the object database anymore to list reflogs.\nAnd second, it ensures that we end up showing reflogs of broken refs,\nwhich will help to make the reflog more useful.\n\nNote that this really only impacts the case where the corresponding ref\nis corrupt. Reflogs for nonexistent refs would have been returned to the\ncaller beforehand already as we did not pass `RESOLVE_REF_READING` to\nthe function, and thus `refs_resolve_ref_unsafe()` would have returned\nsuccessfully in that case.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/files-backend.c    | 12 ++----------\n refs/reftable-backend.c |  6 ++----\n 2 files changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2b3c99b00d..741148087d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2130,17 +2130,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n \t\tif (!S_ISREG(diter->st.st_mode))\n \t\t\tcontinue;\n-\t\tif (diter->basename[0] == '.')\n+\t\tif (check_refname_format(diter->basename,\n+\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\t\tcontinue;\n-\t\tif (ends_with(diter->basename, \".lock\"))\n-\t\t\tcontinue;\n-\n-\t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n-\t\t\t\t\t     diter->relative_path, 0,\n-\t\t\t\t\t     NULL, NULL)) {\n-\t\t\terror(\"bad ref for %s\", diter->path.buf);\n-\t\t\tcontinue;\n-\t\t}\n \n \t\titer->base.refname = diter->relative_path;\n \t\treturn ITER_OK;\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 889bb1f1ba..efbbf23c72 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1659,11 +1659,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (iter->last_name && !strcmp(iter->log.refname, iter->last_name))\n \t\t\tcontinue;\n \n-\t\tif (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,\n-\t\t\t\t\t     0, NULL, NULL)) {\n-\t\t\terror(_(\"bad ref for %s\"), iter->log.refname);\n+\t\tif (check_refname_format(iter->log.refname,\n+\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\t\tcontinue;\n-\t\t}\n \n \t\tfree(iter->last_name);\n \t\titer->last_name = xstrdup(iter->log.refname);\n-- \n2.44.0-rc1\n\n"},{"id":"488914","messageId":"cddb2de9394a07e405682e9ccdfdf5de92bb9092.1708353264.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708353264.git.ps@pks.im","subject":"[PATCH 6/6] builtin/reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-19T14:35:40Z","receivedAt":"2024-02-19T14:35:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the git-reflog(1) command has subcommands to show reflog entries\nor check for reflog existence, it does not have any subcommands that\nwould allow the user to enumerate all existing reflogs. This makes it\nquite hard to discover which reflogs a repository has. While this can\nbe worked around with the \"files\" backend by enumerating files in the\n\".git/logs\" directory, users of the \"reftable\" backend don't enjoy such\na luxury.\n\nIntroduce a new subcommand `git reflog list` that lists all reflogs the\nrepository knows of to fill this gap.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/git-reflog.txt |  3 ++\n builtin/reflog.c             | 34 ++++++++++++++++++\n t/t1410-reflog.sh            | 69 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 106 insertions(+)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex ec64cbff4c..a929c52982 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git reflog' [show] [<log-options>] [<ref>]\n+'git reflog list'\n 'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]\n \t[--rewrite] [--updateref] [--stale-fix]\n \t[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]\n@@ -39,6 +40,8 @@ actions, and in addition the `HEAD` reflog records branch switching.\n `git reflog show` is an alias for `git log -g --abbrev-commit\n --pretty=oneline`; see linkgit:git-log[1] for more information.\n \n+The \"list\" subcommand lists all refs which have a corresponding reflog.\n+\n The \"expire\" subcommand prunes older reflog entries. Entries older\n than `expire` time, or entries older than `expire-unreachable` time\n and not reachable from the current tip, are removed from the reflog.\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 3a0c4d4322..63cd4d8b29 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -7,11 +7,15 @@\n #include \"wildmatch.h\"\n #include \"worktree.h\"\n #include \"reflog.h\"\n+#include \"refs.h\"\n #include \"parse-options.h\"\n \n #define BUILTIN_REFLOG_SHOW_USAGE \\\n \tN_(\"git reflog [show] [<log-options>] [<ref>]\")\n \n+#define BUILTIN_REFLOG_LIST_USAGE \\\n+\tN_(\"git reflog list\")\n+\n #define BUILTIN_REFLOG_EXPIRE_USAGE \\\n \tN_(\"git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\\n\" \\\n \t   \"                  [--rewrite] [--updateref] [--stale-fix]\\n\" \\\n@@ -29,6 +33,11 @@ static const char *const reflog_show_usage[] = {\n \tNULL,\n };\n \n+static const char *const reflog_list_usage[] = {\n+\tBUILTIN_REFLOG_LIST_USAGE,\n+\tNULL,\n+};\n+\n static const char *const reflog_expire_usage[] = {\n \tBUILTIN_REFLOG_EXPIRE_USAGE,\n \tNULL\n@@ -46,6 +55,7 @@ static const char *const reflog_exists_usage[] = {\n \n static const char *const reflog_usage[] = {\n \tBUILTIN_REFLOG_SHOW_USAGE,\n+\tBUILTIN_REFLOG_LIST_USAGE,\n \tBUILTIN_REFLOG_EXPIRE_USAGE,\n \tBUILTIN_REFLOG_DELETE_USAGE,\n \tBUILTIN_REFLOG_EXISTS_USAGE,\n@@ -238,6 +248,29 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)\n \treturn cmd_log_reflog(argc, argv, prefix);\n }\n \n+static int show_reflog(const char *refname, void *cb_data UNUSED)\n+{\n+\tprintf(\"%s\\n\", refname);\n+\treturn 0;\n+}\n+\n+static int cmd_reflog_list(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tstruct ref_store *ref_store;\n+\n+\targc = parse_options(argc, argv, prefix, options, reflog_list_usage, 0);\n+\tif (argc)\n+\t\treturn error(_(\"%s does not accept arguments: '%s'\"),\n+\t\t\t     \"list\", argv[0]);\n+\n+\tref_store = get_main_ref_store(the_repository);\n+\n+\treturn refs_for_each_reflog(ref_store, show_reflog, NULL);\n+}\n+\n static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n {\n \tstruct cmd_reflog_expire_cb cmd = { 0 };\n@@ -417,6 +450,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)\n \tparse_opt_subcommand_fn *fn = NULL;\n \tstruct option options[] = {\n \t\tOPT_SUBCOMMAND(\"show\", &fn, cmd_reflog_show),\n+\t\tOPT_SUBCOMMAND(\"list\", &fn, cmd_reflog_list),\n \t\tOPT_SUBCOMMAND(\"expire\", &fn, cmd_reflog_expire),\n \t\tOPT_SUBCOMMAND(\"delete\", &fn, cmd_reflog_delete),\n \t\tOPT_SUBCOMMAND(\"exists\", &fn, cmd_reflog_exists),\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex d2f5f42e67..6d8d5a253d 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'list reflogs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit reflog list >actual &&\n+\t\ttest_must_be_empty actual &&\n+\n+\t\ttest_commit A &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit branch b &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/b\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'reflog list returns error with additional args' '\n+\tcat >expect <<-EOF &&\n+\terror: list does not accept arguments: ${SQ}bogus${SQ}\n+\tEOF\n+\ttest_must_fail git reflog list bogus 2>err &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success 'reflog for symref with unborn target can be listed' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit symbolic-ref HEAD refs/heads/unborn &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'reflog with invalid object ID can be listed' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\ttest-tool ref-store main update-ref msg refs/heads/missing \\\n+\t\t\t$(test_oid deadbeef) \"$ZERO_OID\" REF_SKIP_OID_VERIFICATION &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\trefs/heads/missing\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.44.0-rc1\n\n"},{"id":"488937","messageId":"xmqq8r3g10tf.fsf@gitster.g","threadId":"60955","inReplyTo":"8a588175dbf23d1938db45507812aad8f3793dbb.1708353264.git.ps@pks.im","subject":"Re: [PATCH 2/6] dir-iterator: support iteration in sorted order","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-19T23:39:08Z","receivedAt":"2024-02-19T23:39:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The `struct dir_iterator` is a helper that allows us to iterate through\n> directory entries. This iterator returns entries in the exact same order\n> as readdir(3P) does -- or in other words, it guarantees no specific\n> order at all.\n>\n> This is about to become problematic as we are introducing a new reflog\n> subcommand to list reflogs. As the \"files\" backend uses the directory\n> iterator to enumerate reflogs, returning reflog names and exposing them\n> to the user would inherit the indeterministic ordering. Naturally, it\n> would make for a terrible user interface to show a list with no\n> discernible order. While this could be handled at a higher level by the\n> new subcommand itself by collecting and ordering the reflogs, this would\n> be inefficient and introduce latency when there are many reflogs.\n\nI do not quite understand this argument.  Why is sorting at higher\nlevel less (or more, for that matter) efficient than doing so at\nlower level?  We'd need to sort somewhere no matter what, and I of\ncourse have no problem in listing in a deterministic order.\n\n> Instead, introduce a new option into the directory iterator that asks\n> for its entries to be yielded in lexicographical order. If set, the\n> iterator will read all directory entries greedily end sort them before\n> we start to iterate over them.\n\n\"end\" -> \"and\".  And of course without such sorting option, this\ncodepath is allowed to yield entries in any order that is the\neasiest to produce?  That makes sense.\n\n> While this will of course also incur overhead as we cannot yield the\n> directory entries immediately, it should at least be more efficient than\n> having to sort the complete list of reflogs as we only need to sort one\n> directory at a time.\n\nTrue.  The initial latency before we see the first byte of the\noutput often matters more in perceived performance the throughput.\nAs we need to sort to give a reasonable output, that cannot be\navoided.\n\n> This functionality will be used in a follow-up commit.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  dir-iterator.c | 87 ++++++++++++++++++++++++++++++++++++++++----------\n>  dir-iterator.h |  3 ++\n>  2 files changed, 73 insertions(+), 17 deletions(-)\n>\n> diff --git a/dir-iterator.c b/dir-iterator.c\n> index f58a97e089..396c28178f 100644\n> --- a/dir-iterator.c\n> +++ b/dir-iterator.c\n> @@ -2,9 +2,12 @@\n>  #include \"dir.h\"\n>  #include \"iterator.h\"\n>  #include \"dir-iterator.h\"\n> +#include \"string-list.h\"\n>  \n>  struct dir_iterator_level {\n>  \tDIR *dir;\n> +\tstruct string_list entries;\n> +\tsize_t entries_idx;\n\nDoes it deserve a comment that \"dir == NULL\" is used as a signal\nthat we have read the level and sorted its contents into the\n\"entries\" list (and also we have already called closedir(), of\ncourse)?\n\n> @@ -72,6 +75,40 @@ static int push_level(struct dir_iterator_int *iter)\n>  \t\treturn -1;\n>  \t}\n>  \n> +\tstring_list_init_dup(&level->entries);\n> +\tlevel->entries_idx = 0;\n> +\n> +\t/*\n> +\t * When the iterator is sorted we read and sort all directory entries\n> +\t * directly.\n> +\t */\n> +\tif (iter->flags & DIR_ITERATOR_SORTED) {\n> +\t\twhile (1) {\n> +\t\t\tstruct dirent *de;\n> +\n> +\t\t\terrno = 0;\n> +\t\t\tde = readdir(level->dir);\n> +\t\t\tif (!de) {\n> +\t\t\t\tif (errno && errno != ENOENT) {\n> +\t\t\t\t\twarning_errno(\"error reading directory '%s'\",\n> +\t\t\t\t\t\t      iter->base.path.buf);\n> +\t\t\t\t\treturn -1;\n> +\t\t\t\t}\n> +\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tif (is_dot_or_dotdot(de->d_name))\n> +\t\t\t\tcontinue;\n\nThe condition to skip an entry currently is simple enough that \".\"\nand \"..\" are the only ones that are skipped, but it must be kept in\nsync with the condition in dir_iterator_advance().\n\nIf it becomes more complex than it is now (e.g., we may start to\nskip any name that begins with a dot, like \".git\" or \".dummy\"), it\nprobably is a good idea *not* to add the same filtering logic here\nand in dir_iterator_advance().  Instead, keep the filtering here to\nan absolute minumum, and filter the name, whether it came from\nreaddir() or from the .entries string list, in a single copy of\nfiltering logic in dir_iterator_advance() function.\n\nWe could drop the dot-or-dotdot filter here, too, if we want to\nensure that unified filtering will be correctly done over there.\n\n> +\t\t\tstring_list_append(&level->entries, de->d_name);\n> +\t\t}\n> +\t\tstring_list_sort(&level->entries);\n> +\n> +\t\tclosedir(level->dir);\n> +\t\tlevel->dir = NULL;\n> +\t}\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -88,6 +125,7 @@ static int pop_level(struct dir_iterator_int *iter)\n>  \t\twarning_errno(\"error closing directory '%s'\",\n>  \t\t\t      iter->base.path.buf);\n>  \tlevel->dir = NULL;\n> +\tstring_list_clear(&level->entries, 0);\n>  \n>  \treturn --iter->levels_nr;\n>  }\n\nIt is somewhat interesting that the original code already has\nconditional call to closedir() and prepares .dir to be NULL,\nso that we do not have to make it conditional here.\n\n> @@ -136,30 +174,43 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n>  \n>  \t/* Loop until we find an entry that we can give back to the caller. */\n>  \twhile (1) {\n> -\t\tstruct dirent *de;\n>  \t\tstruct dir_iterator_level *level =\n>  \t\t\t&iter->levels[iter->levels_nr - 1];\n> +\t\tstruct dirent *de;\n> +\t\tconst char *name;\n\nNot a huge deal but this is an unnecessary reordering, right?\n\n>  \t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n> +\n> +\t\tif (level->dir) {\n> +\t\t\terrno = 0;\n> +\t\t\tde = readdir(level->dir);\n> +\t\t\tif (!de) {\n> +\t\t\t\tif (errno) {\n> +\t\t\t\t\twarning_errno(\"error reading directory '%s'\",\n> +\t\t\t\t\t\t      iter->base.path.buf);\n> +\t\t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n> +\t\t\t\t\t\tgoto error_out;\n> +\t\t\t\t} else if (pop_level(iter) == 0) {\n> +\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n> +\t\t\t\t}\n> +\t\t\t\tcontinue;\n>  \t\t\t}\n>  \n> +\t\t\tif (is_dot_or_dotdot(de->d_name))\n> +\t\t\t\tcontinue;\n\nThis is the target of the \"if we will end up filtering even more in\nthe future, it would probably be a good idea not to duplicate the\nlogic to decide what gets filtered in this function and in\npush_level()\" comment.  If we wanted to go that route, we can get\nrid of the filtering from push_level(), and move this filter code\noutside this if/else before calling prepare_next_entry_data().\n\nThe fact that .entries.nr represents the number of entries that are\nshown is unusable (because there is an unsorted codepath that does\nnot even populate .entries), so I am not worried about correctness\ngotchas caused by including names in .entries to be filtered out.\nBut an obvious downside is that the size of the list to be sorted\nwill become larger.\n\nOr we could introduce a shared helper function that takes a name and\ndecides if it is to be included, and replace the is_dot_or_dotdot()\ncall here and in the push_level() with calls to that helper.\n\nIn any case, that is primarily a maintainability issue.  The code\nposted as-is is correct.\n\n> +\t\t\tname = de->d_name;\n> +\t\t} else {\n> +\t\t\tif (level->entries_idx >= level->entries.nr) {\n> +\t\t\t\tif (pop_level(iter) == 0)\n> +\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\n> +\t\t\tname = level->entries.items[level->entries_idx++].string;\n> +\t\t}\n> +\n> +\t\tif (prepare_next_entry_data(iter, name)) {\n>  \t\t\tif (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n>  \t\t\t\tgoto error_out;\n>  \t\t\tcontinue;\n> @@ -188,6 +239,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n>  \t\t\twarning_errno(\"error closing directory '%s'\",\n>  \t\t\t\t      iter->base.path.buf);\n>  \t\t}\n> +\n> +\t\tstring_list_clear(&level->entries, 0);\n>  \t}\n>  \n>  \tfree(iter->levels);\n> diff --git a/dir-iterator.h b/dir-iterator.h\n> index 479e1ec784..6d438809b6 100644\n> --- a/dir-iterator.h\n> +++ b/dir-iterator.h\n> @@ -54,8 +54,11 @@\n>   *   and ITER_ERROR is returned immediately. In both cases, a meaningful\n>   *   warning is emitted. Note: ENOENT errors are always ignored so that\n>   *   the API users may remove files during iteration.\n> + *\n> + * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.\n>   */\n>  #define DIR_ITERATOR_PEDANTIC (1 << 0)\n> +#define DIR_ITERATOR_SORTED   (1 << 1)\n>  \n>  struct dir_iterator {\n>  \t/* The current path: */\n"},{"id":"488939","messageId":"xmqq34to0znj.fsf@gitster.g","threadId":"60955","inReplyTo":"e4e4fac05c7f4bcac8ef96bdebb8a68eef40ead4.1708353264.git.ps@pks.im","subject":"Re: [PATCH 3/6] refs/files: sort reflogs returned by the reflog iterator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T00:04:16Z","receivedAt":"2024-02-20T00:04:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> We use a directory iterator to return reflogs via the reflog iterator.\n> This iterator returns entries in the same order as readdir(3P) would and\n> will thus yield reflogs with no discernible order.\n>\n> Set the new `DIR_ITERATOR_SORTED` flag that was introduced in the\n> preceding commit so that the order is deterministic. While the effect of\n> this can only been observed in a test tool, a subsequent commit will\n> start to expose this functionality to users via a new `git reflog list`\n> subcommand.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  refs/files-backend.c           | 4 ++--\n>  t/t0600-reffiles-backend.sh    | 4 ++--\n>  t/t1405-main-ref-store.sh      | 2 +-\n>  t/t1406-submodule-ref-store.sh | 2 +-\n>  4 files changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 75dcc21ecb..2ffc63185f 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n>  \n>  \tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n>  \n> -\tditer = dir_iterator_begin(sb.buf, 0);\n> +\tditer = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);\n>  \tif (!diter) {\n>  \t\tstrbuf_release(&sb);\n>  \t\treturn empty_ref_iterator_begin();\n> @@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n>  \tCALLOC_ARRAY(iter, 1);\n>  \tref_iterator = &iter->base;\n>  \n> -\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n> +\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);\n\nThis caught my attention.  Once we apply this patch, the only way\nbase_ref_iterator_init() can receive 0 for its last parameter\n(i.e. 'ordered') is via the merge_ref_iterator_begin() call in\nfiles_reflog_iterator_begin() that passes 0 as 'ordered'.  If we\nforce files_reflog_iterator_begin() to ask for an ordered\nmerge_ref_iterator, then we will have no unordered ref iterators.\nAm I reading the code right?\n"},{"id":"488940","messageId":"xmqqplwsyotj.fsf@gitster.g","threadId":"60955","inReplyTo":"be512ef268b910852ff11df181d89c483ffc18ab.1708353264.git.ps@pks.im","subject":"Re: [PATCH 4/6] refs: drop unused params from the reflog iterator callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T00:14:16Z","receivedAt":"2024-02-20T00:14:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The ref and reflog iterators share much of the same underlying code to\n> iterate over the corresponding entries. This results in some weird code\n> because the reflog iterator also exposes an object ID as well as a flag\n> to the callback function. Neither of these fields do refer to the reflog\n> though -- they refer to the corresponding ref with the same name. This\n> is quite misleading. In practice at least the object ID cannot really be\n> implemented in any other way as a reflog does not have a specific object\n> ID in the first place. This is further stressed by the fact that none of\n> the callbacks except for our test helper make use of these fields.\n\nInteresting observation.  Of course this will make the callstack\nlonger by another level of indirection ...\n\n> +struct do_for_each_reflog_help {\n> +\teach_reflog_fn *fn;\n> +\tvoid *cb_data;\n> +};\n> +\n> +static int do_for_each_reflog_helper(struct repository *r UNUSED,\n> +\t\t\t\t     const char *refname,\n> +\t\t\t\t     const struct object_id *oid UNUSED,\n> +\t\t\t\t     int flags,\n> +\t\t\t\t     void *cb_data)\n> +{\n> +\tstruct do_for_each_reflog_help *hp = cb_data;\n> +\treturn hp->fn(refname, hp->cb_data);\n> +}\n\n... but I think it would be worth it.\n\n> +/*\n> + * The signature for the callback function for the {refs_,}for_each_reflog()\n> + * functions below. The memory pointed to by the refname argument is only\n> + * guaranteed to be valid for the duration of a single callback invocation.\n> + */\n> +typedef int each_reflog_fn(const char *refname, void *cb_data);\n> +\n>  /*\n>   * Calls the specified function for each reflog file until it returns nonzero,\n>   * and returns the value. Reflog file order is unspecified.\n>   */\n> -int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n> -int for_each_reflog(each_ref_fn fn, void *cb_data);\n> +int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);\n> +int for_each_reflog(each_reflog_fn fn, void *cb_data);\n\nNice simplification.\n"},{"id":"488941","messageId":"xmqqjzn0yote.fsf@gitster.g","threadId":"60955","inReplyTo":"a7459b9483660d1a44df500aaee85ad38146eb02.1708353264.git.ps@pks.im","subject":"Re: [PATCH 5/6] refs: stop resolving ref corresponding to reflogs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T00:14:21Z","receivedAt":"2024-02-20T00:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Refactor the code to call `check_refname_format()` directly instead of\n> trying to resolve the ref. This is significantly more efficient given\n> that we don't have to hit the object database anymore to list reflogs.\n> And second, it ensures that we end up showing reflogs of broken refs,\n> which will help to make the reflog more useful.\n\nAnd the user would notice corrupt ones among those reflogs listed\nwhen using \"rev-list -g\" on the reflog anyway?  Which sounds like a\nsensible thing to do.\n\n> Note that this really only impacts the case where the corresponding ref\n> is corrupt. Reflogs for nonexistent refs would have been returned to the\n> caller beforehand already as we did not pass `RESOLVE_REF_READING` to\n> the function, and thus `refs_resolve_ref_unsafe()` would have returned\n> successfully in that case.\n\nWhat do \"Reflogs for nonexistent refs\" really mean?  With the files\nbackend, if \"git branch -d main\" that removed the \"main\" branch\nsomehow forgot to remove the \".git/logs/refs/heads/main\" file, the\nreflog entries in such a file is for nonexistent ref.  Is that what\nyou meant?  As a tool to help diagnosing and correcting minor repo\nbreakages, finding such a leftover file that should not exist is a\ngood idea, I would think.\n\nWould we see missing reflog for a ref that exists in the iteration?\nI guess we shouldn't, as the reflog iterator that recursively\nenumerates files under \"$GIT_DIR/logs/\" would not see such a missing\nreflog by definition.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 2b3c99b00d..741148087d 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2130,17 +2130,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n>  \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n>  \t\tif (!S_ISREG(diter->st.st_mode))\n>  \t\t\tcontinue;\n> -\t\tif (diter->basename[0] == '.')\n> +\t\tif (check_refname_format(diter->basename,\n> +\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n>  \t\t\tcontinue;\n\nA tangent.\n\nI've never liked the code arrangement in the check_refname_format()\nthat assumes that each level can be separately checked with exactly\nthe same logic, and the only thing ALLOW_ONELEVEL does is to include\npseudorefs and HEAD; this makes such assumption even more ingrained.\nI am not sure what to think about it, but let's keep reading.\n\n> -\t\tif (ends_with(diter->basename, \".lock\"))\n> -\t\t\tcontinue;\n\nThis can safely go, as it is rejected by check_refname_format().\n\n> -\t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n> -\t\t\t\t\t     diter->relative_path, 0,\n> -\t\t\t\t\t     NULL, NULL)) {\n> -\t\t\terror(\"bad ref for %s\", diter->path.buf);\n> -\t\t\tcontinue;\n> -\t\t}\n\nThis is the focus of this step in the series.  We did not abort the\niteration before, but now we no longer issue any error message.\n\n>  \t\titer->base.refname = diter->relative_path;\n>  \t\treturn ITER_OK;\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 889bb1f1ba..efbbf23c72 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1659,11 +1659,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n>  \t\tif (iter->last_name && !strcmp(iter->log.refname, iter->last_name))\n>  \t\t\tcontinue;\n>  \n> -\t\tif (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,\n> -\t\t\t\t\t     0, NULL, NULL)) {\n> -\t\t\terror(_(\"bad ref for %s\"), iter->log.refname);\n> +\t\tif (check_refname_format(iter->log.refname,\n> +\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n>  \t\t\tcontinue;\n> -\t\t}\n\nThis side is much more straight-forward.  Looking good.\n\n>  \n>  \t\tfree(iter->last_name);\n>  \t\titer->last_name = xstrdup(iter->log.refname);\n"},{"id":"488943","messageId":"xmqq7cj0ynys.fsf@gitster.g","threadId":"60955","inReplyTo":"cddb2de9394a07e405682e9ccdfdf5de92bb9092.1708353264.git.ps@pks.im","subject":"Re: [PATCH 6/6] builtin/reflog: introduce subcommand to list reflogs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T00:32:43Z","receivedAt":"2024-02-20T00:32:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> index d2f5f42e67..6d8d5a253d 100755\n> --- a/t/t1410-reflog.sh\n> +++ b/t/t1410-reflog.sh\n> @@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '\n>  \ttest_must_be_empty err\n>  '\n>  \n> +test_expect_success 'list reflogs' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit reflog list >actual &&\n> +\t\ttest_must_be_empty actual &&\n> +\n> +\t\ttest_commit A &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\tHEAD\n> +\t\trefs/heads/main\n> +\t\tEOF\n> +\t\tgit reflog list >actual &&\n> +\t\ttest_cmp expect actual &&\n> +\n> +\t\tgit branch b &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\tHEAD\n> +\t\trefs/heads/b\n> +\t\trefs/heads/main\n> +\t\tEOF\n> +\t\tgit reflog list >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n\nOK.  This is a quite boring baseline.\n\n> +test_expect_success 'reflog list returns error with additional args' '\n> +\tcat >expect <<-EOF &&\n> +\terror: list does not accept arguments: ${SQ}bogus${SQ}\n> +\tEOF\n> +\ttest_must_fail git reflog list bogus 2>err &&\n> +\ttest_cmp expect err\n> +'\n\nMakes sense.\n\n> +test_expect_success 'reflog for symref with unborn target can be listed' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit A &&\n> +\t\tgit symbolic-ref HEAD refs/heads/unborn &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\tHEAD\n> +\t\trefs/heads/main\n> +\t\tEOF\n> +\t\tgit reflog list >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n\nShould this be under REFFILES?  Ah, no, \"git symbolic-ref\" is valid\nunder reftable as well, so there is no need to.\n\nWithout [5/6], would it have failed to show the reflog for HEAD?\n\n> +test_expect_success 'reflog with invalid object ID can be listed' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit A &&\n> +\t\ttest-tool ref-store main update-ref msg refs/heads/missing \\\n> +\t\t\t$(test_oid deadbeef) \"$ZERO_OID\" REF_SKIP_OID_VERIFICATION &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\tHEAD\n> +\t\trefs/heads/main\n> +\t\trefs/heads/missing\n> +\t\tEOF\n> +\t\tgit reflog list >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n\nOK.\n\n>  test_done\n\nIt would have been \"interesting\" to see an example of \"there is a\nreflog but the underlying ref for it is missing\" case, but I think\nthat falls into a minor repository corruption category, so lack of\nsuch a test is also fine.\n\n"},{"id":"488965","messageId":"ZdRkAe5zajmGb95q@tanuki","threadId":"60955","inReplyTo":"xmqq8r3g10tf.fsf@gitster.g","subject":"Re: [PATCH 2/6] dir-iterator: support iteration in sorted order","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T08:34:09Z","receivedAt":"2024-02-20T08:34:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 19, 2024 at 03:39:08PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > The `struct dir_iterator` is a helper that allows us to iterate through\n> > directory entries. This iterator returns entries in the exact same order\n> > as readdir(3P) does -- or in other words, it guarantees no specific\n> > order at all.\n> >\n> > This is about to become problematic as we are introducing a new reflog\n> > subcommand to list reflogs. As the \"files\" backend uses the directory\n> > iterator to enumerate reflogs, returning reflog names and exposing them\n> > to the user would inherit the indeterministic ordering. Naturally, it\n> > would make for a terrible user interface to show a list with no\n> > discernible order. While this could be handled at a higher level by the\n> > new subcommand itself by collecting and ordering the reflogs, this would\n> > be inefficient and introduce latency when there are many reflogs.\n> \n> I do not quite understand this argument.  Why is sorting at higher\n> level less (or more, for that matter) efficient than doing so at\n> lower level?  We'd need to sort somewhere no matter what, and I of\n> course have no problem in listing in a deterministic order.\n\nBy sorting at a lower level we only need to sort the respective\ndirectory entries and can then return them without having to recurse\ninto all subdirectories yet. Sorting at a higher level would require us\nto first collect _all_ reflogs and then sort them.\n\nWill rephrase a bit.\n\n> > Instead, introduce a new option into the directory iterator that asks\n> > for its entries to be yielded in lexicographical order. If set, the\n> > iterator will read all directory entries greedily end sort them before\n> > we start to iterate over them.\n> \n> \"end\" -> \"and\".  And of course without such sorting option, this\n> codepath is allowed to yield entries in any order that is the\n> easiest to produce?  That makes sense.\n> \n> > While this will of course also incur overhead as we cannot yield the\n> > directory entries immediately, it should at least be more efficient than\n> > having to sort the complete list of reflogs as we only need to sort one\n> > directory at a time.\n> \n> True.  The initial latency before we see the first byte of the\n> output often matters more in perceived performance the throughput.\n> As we need to sort to give a reasonable output, that cannot be\n> avoided.\n> \n> > This functionality will be used in a follow-up commit.\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  dir-iterator.c | 87 ++++++++++++++++++++++++++++++++++++++++----------\n> >  dir-iterator.h |  3 ++\n> >  2 files changed, 73 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/dir-iterator.c b/dir-iterator.c\n> > index f58a97e089..396c28178f 100644\n> > --- a/dir-iterator.c\n> > +++ b/dir-iterator.c\n> > @@ -2,9 +2,12 @@\n> >  #include \"dir.h\"\n> >  #include \"iterator.h\"\n> >  #include \"dir-iterator.h\"\n> > +#include \"string-list.h\"\n> >  \n> >  struct dir_iterator_level {\n> >  \tDIR *dir;\n> > +\tstruct string_list entries;\n> > +\tsize_t entries_idx;\n> \n> Does it deserve a comment that \"dir == NULL\" is used as a signal\n> that we have read the level and sorted its contents into the\n> \"entries\" list (and also we have already called closedir(), of\n> course)?\n\nYeah, probably.\n\n> > @@ -72,6 +75,40 @@ static int push_level(struct dir_iterator_int *iter)\n> >  \t\treturn -1;\n> >  \t}\n> >  \n> > +\tstring_list_init_dup(&level->entries);\n> > +\tlevel->entries_idx = 0;\n> > +\n> > +\t/*\n> > +\t * When the iterator is sorted we read and sort all directory entries\n> > +\t * directly.\n> > +\t */\n> > +\tif (iter->flags & DIR_ITERATOR_SORTED) {\n> > +\t\twhile (1) {\n> > +\t\t\tstruct dirent *de;\n> > +\n> > +\t\t\terrno = 0;\n> > +\t\t\tde = readdir(level->dir);\n> > +\t\t\tif (!de) {\n> > +\t\t\t\tif (errno && errno != ENOENT) {\n> > +\t\t\t\t\twarning_errno(\"error reading directory '%s'\",\n> > +\t\t\t\t\t\t      iter->base.path.buf);\n> > +\t\t\t\t\treturn -1;\n> > +\t\t\t\t}\n> > +\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\n> > +\t\t\tif (is_dot_or_dotdot(de->d_name))\n> > +\t\t\t\tcontinue;\n> \n> The condition to skip an entry currently is simple enough that \".\"\n> and \"..\" are the only ones that are skipped, but it must be kept in\n> sync with the condition in dir_iterator_advance().\n> \n> If it becomes more complex than it is now (e.g., we may start to\n> skip any name that begins with a dot, like \".git\" or \".dummy\"), it\n> probably is a good idea *not* to add the same filtering logic here\n> and in dir_iterator_advance().  Instead, keep the filtering here to\n> an absolute minumum, and filter the name, whether it came from\n> readdir() or from the .entries string list, in a single copy of\n> filtering logic in dir_iterator_advance() function.\n> \n> We could drop the dot-or-dotdot filter here, too, if we want to\n> ensure that unified filtering will be correctly done over there.\n\nFair point. As you mention further down below, there are two ways to\napproach it:\n\n  - Just filter at the later stage and accept that we'll allocate memory\n    for entries that are about to be discarded.\n\n  - Create a function `should_include_entry()` that gets called at both\n    code sites.\n\nI don't think the allocation overhead should matter much, but neither\ndoes it hurt to create a common `should_include_entry()` function. And\nas both are trivial to implement I rather lean towards the more\nefficient variant, even though the efficiency gain should be negligible.\n\n> > +\t\t\tstring_list_append(&level->entries, de->d_name);\n> > +\t\t}\n> > +\t\tstring_list_sort(&level->entries);\n> > +\n> > +\t\tclosedir(level->dir);\n> > +\t\tlevel->dir = NULL;\n> > +\t}\n> > +\n> >  \treturn 0;\n> >  }\n> >  \n> > @@ -88,6 +125,7 @@ static int pop_level(struct dir_iterator_int *iter)\n> >  \t\twarning_errno(\"error closing directory '%s'\",\n> >  \t\t\t      iter->base.path.buf);\n> >  \tlevel->dir = NULL;\n> > +\tstring_list_clear(&level->entries, 0);\n> >  \n> >  \treturn --iter->levels_nr;\n> >  }\n> \n> It is somewhat interesting that the original code already has\n> conditional call to closedir() and prepares .dir to be NULL,\n> so that we do not have to make it conditional here.\n> \n> > @@ -136,30 +174,43 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n> >  \n> >  \t/* Loop until we find an entry that we can give back to the caller. */\n> >  \twhile (1) {\n> > -\t\tstruct dirent *de;\n> >  \t\tstruct dir_iterator_level *level =\n> >  \t\t\t&iter->levels[iter->levels_nr - 1];\n> > +\t\tstruct dirent *de;\n> > +\t\tconst char *name;\n> \n> Not a huge deal but this is an unnecessary reordering, right?\n\nRight.\n\n> >  \t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n> > +\n> > +\t\tif (level->dir) {\n> > +\t\t\terrno = 0;\n> > +\t\t\tde = readdir(level->dir);\n> > +\t\t\tif (!de) {\n> > +\t\t\t\tif (errno) {\n> > +\t\t\t\t\twarning_errno(\"error reading directory '%s'\",\n> > +\t\t\t\t\t\t      iter->base.path.buf);\n> > +\t\t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n> > +\t\t\t\t\t\tgoto error_out;\n> > +\t\t\t\t} else if (pop_level(iter) == 0) {\n> > +\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n> > +\t\t\t\t}\n> > +\t\t\t\tcontinue;\n> >  \t\t\t}\n> >  \n> > +\t\t\tif (is_dot_or_dotdot(de->d_name))\n> > +\t\t\t\tcontinue;\n> \n> This is the target of the \"if we will end up filtering even more in\n> the future, it would probably be a good idea not to duplicate the\n> logic to decide what gets filtered in this function and in\n> push_level()\" comment.  If we wanted to go that route, we can get\n> rid of the filtering from push_level(), and move this filter code\n> outside this if/else before calling prepare_next_entry_data().\n> \n> The fact that .entries.nr represents the number of entries that are\n> shown is unusable (because there is an unsorted codepath that does\n> not even populate .entries), so I am not worried about correctness\n> gotchas caused by including names in .entries to be filtered out.\n> But an obvious downside is that the size of the list to be sorted\n> will become larger.\n> \n> Or we could introduce a shared helper function that takes a name and\n> decides if it is to be included, and replace the is_dot_or_dotdot()\n> call here and in the push_level() with calls to that helper.\n> \n> In any case, that is primarily a maintainability issue.  The code\n> posted as-is is correct.\n\nYeah, let's use the proposed helper function. In fact, I think we can\nshare even more code than merely the filtering part: the errno handling\nis a bit special, and the warning is the same across both code sites,\ntoo.\n\nPatrick\n\n> > +\t\t\tname = de->d_name;\n> > +\t\t} else {\n> > +\t\t\tif (level->entries_idx >= level->entries.nr) {\n> > +\t\t\t\tif (pop_level(iter) == 0)\n> > +\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n> > +\t\t\t\tcontinue;\n> > +\t\t\t}\n> > +\n> > +\t\t\tname = level->entries.items[level->entries_idx++].string;\n> > +\t\t}\n> > +\n> > +\t\tif (prepare_next_entry_data(iter, name)) {\n> >  \t\t\tif (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n> >  \t\t\t\tgoto error_out;\n> >  \t\t\tcontinue;\n> > @@ -188,6 +239,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n> >  \t\t\twarning_errno(\"error closing directory '%s'\",\n> >  \t\t\t\t      iter->base.path.buf);\n> >  \t\t}\n> > +\n> > +\t\tstring_list_clear(&level->entries, 0);\n> >  \t}\n> >  \n> >  \tfree(iter->levels);\n> > diff --git a/dir-iterator.h b/dir-iterator.h\n> > index 479e1ec784..6d438809b6 100644\n> > --- a/dir-iterator.h\n> > +++ b/dir-iterator.h\n> > @@ -54,8 +54,11 @@\n> >   *   and ITER_ERROR is returned immediately. In both cases, a meaningful\n> >   *   warning is emitted. Note: ENOENT errors are always ignored so that\n> >   *   the API users may remove files during iteration.\n> > + *\n> > + * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.\n> >   */\n> >  #define DIR_ITERATOR_PEDANTIC (1 << 0)\n> > +#define DIR_ITERATOR_SORTED   (1 << 1)\n> >  \n> >  struct dir_iterator {\n> >  \t/* The current path: */\n"},{"id":"488966","messageId":"ZdRkEbh-8SKNteDm@tanuki","threadId":"60955","inReplyTo":"xmqqplwsyotj.fsf@gitster.g","subject":"Re: [PATCH 4/6] refs: drop unused params from the reflog iterator callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T08:34:25Z","receivedAt":"2024-02-20T08:34:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 19, 2024 at 04:14:16PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > The ref and reflog iterators share much of the same underlying code to\n> > iterate over the corresponding entries. This results in some weird code\n> > because the reflog iterator also exposes an object ID as well as a flag\n> > to the callback function. Neither of these fields do refer to the reflog\n> > though -- they refer to the corresponding ref with the same name. This\n> > is quite misleading. In practice at least the object ID cannot really be\n> > implemented in any other way as a reflog does not have a specific object\n> > ID in the first place. This is further stressed by the fact that none of\n> > the callbacks except for our test helper make use of these fields.\n> \n> Interesting observation.  Of course this will make the callstack\n> longer by another level of indirection ...\n\nIt actually doesn't -- the old code had a `do_for_each_ref_helper()`\nthat did the same wrapping, so the level of indirection is exactly the\nsame. Over there we have it to drop the unused `struct repository`\nparameter.\n\nPatrick\n\n> > +struct do_for_each_reflog_help {\n> > +\teach_reflog_fn *fn;\n> > +\tvoid *cb_data;\n> > +};\n> > +\n> > +static int do_for_each_reflog_helper(struct repository *r UNUSED,\n> > +\t\t\t\t     const char *refname,\n> > +\t\t\t\t     const struct object_id *oid UNUSED,\n> > +\t\t\t\t     int flags,\n> > +\t\t\t\t     void *cb_data)\n> > +{\n> > +\tstruct do_for_each_reflog_help *hp = cb_data;\n> > +\treturn hp->fn(refname, hp->cb_data);\n> > +}\n> \n> ... but I think it would be worth it.\n> \n> > +/*\n> > + * The signature for the callback function for the {refs_,}for_each_reflog()\n> > + * functions below. The memory pointed to by the refname argument is only\n> > + * guaranteed to be valid for the duration of a single callback invocation.\n> > + */\n> > +typedef int each_reflog_fn(const char *refname, void *cb_data);\n> > +\n> >  /*\n> >   * Calls the specified function for each reflog file until it returns nonzero,\n> >   * and returns the value. Reflog file order is unspecified.\n> >   */\n> > -int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n> > -int for_each_reflog(each_ref_fn fn, void *cb_data);\n> > +int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);\n> > +int for_each_reflog(each_reflog_fn fn, void *cb_data);\n> \n> Nice simplification.\n"},{"id":"488967","messageId":"ZdRkGWhUrHQgWbxy@tanuki","threadId":"60955","inReplyTo":"xmqqjzn0yote.fsf@gitster.g","subject":"Re: [PATCH 5/6] refs: stop resolving ref corresponding to reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T08:34:33Z","receivedAt":"2024-02-20T08:34:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 19, 2024 at 04:14:21PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Refactor the code to call `check_refname_format()` directly instead of\n> > trying to resolve the ref. This is significantly more efficient given\n> > that we don't have to hit the object database anymore to list reflogs.\n> > And second, it ensures that we end up showing reflogs of broken refs,\n> > which will help to make the reflog more useful.\n> \n> And the user would notice corrupt ones among those reflogs listed\n> when using \"rev-list -g\" on the reflog anyway?  Which sounds like a\n> sensible thing to do.\n\nYeah. Overall the user experience is still quite lacking when you have\nsuch \"funny\" reflogs. Corrupted ones would result in errors as you\nmentioned, and that's to be expected in my opinion.\n\nThe more dubious behaviour is that `git reflog show $REFLOG` refuses to\nshow the reflog when the corresponding ref is missing. This is something\nI plan to address in a follow-up patch series.\n\n> > Note that this really only impacts the case where the corresponding ref\n> > is corrupt. Reflogs for nonexistent refs would have been returned to the\n> > caller beforehand already as we did not pass `RESOLVE_REF_READING` to\n> > the function, and thus `refs_resolve_ref_unsafe()` would have returned\n> > successfully in that case.\n> \n> What do \"Reflogs for nonexistent refs\" really mean?  With the files\n> backend, if \"git branch -d main\" that removed the \"main\" branch\n> somehow forgot to remove the \".git/logs/refs/heads/main\" file, the\n> reflog entries in such a file is for nonexistent ref.  Is that what\n> you meant?\n\nYes. Would \"Reflogs which do not have a corresponding ref with the same\nname\" be clearer?\n\n> As a tool to help diagnosing and correcting minor repo\n> breakages, finding such a leftover file that should not exist is a\n> good idea, I would think.\n> \n> Would we see missing reflog for a ref that exists in the iteration?\n> I guess we shouldn't, as the reflog iterator that recursively\n> enumerates files under \"$GIT_DIR/logs/\" would not see such a missing\n> reflog by definition.\n\nNo, and I'd claim we shouldn't. The reflog mechanism gives the user\ncontrol over which reflogs should and which shouldn't exist. For one,\n`core.logAllRefUpdates` allows the user to either enable or disable the\nreflog mechanism. If set to \"false\" then no reflogs are created, with\n\"true\" some are created, and with \"always\" we always end up creating\nreflogs. So depending on this setting it's expected that a subset of\nreflogs do not exist.\n\nBut that'also not the whole story yet. Theoretically speaking, reflogs\nhave a subtle opt-in mechanism: once a reflog is created, we will\ncontinue writing to it no matter what `core.logAllRefUpdates` says. So\nit's feasible to have `core.logAllRefUpdates=false`, but then explicitly\ncreate a specific reflog so that you log changes to a specific ref.\n\nWith this behaviour in mind I'd say that we shouldn't log missing\nreflogs.\n\n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index 2b3c99b00d..741148087d 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -2130,17 +2130,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n> >  \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n> >  \t\tif (!S_ISREG(diter->st.st_mode))\n> >  \t\t\tcontinue;\n> > -\t\tif (diter->basename[0] == '.')\n> > +\t\tif (check_refname_format(diter->basename,\n> > +\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n> >  \t\t\tcontinue;\n> \n> A tangent.\n> \n> I've never liked the code arrangement in the check_refname_format()\n> that assumes that each level can be separately checked with exactly\n> the same logic, and the only thing ALLOW_ONELEVEL does is to include\n> pseudorefs and HEAD; this makes such assumption even more ingrained.\n> I am not sure what to think about it, but let's keep reading.\n\nYeah. This code here is basically just copied over from\n`refs_resolve_ref_unsafe()` to ensure that it remains compatible. In a\nfuture patch series we might include a new option `--include-broken`\nthat would also surface broken-but-safe reflog names.\n\nBut going down the tangent even more: one think I've noticed is that the\nway `check_refname_format()` is structured is also wildly inefficient.\nIt's quite astonishing that when iterating over refs, we spend _more_\ntime in `check_refname_format()` than reading the refs from disk,\nparsing them and massaging them into their final representation.\n\nOverall, the whole infra to check refnames could use some improvement.\nBut this has already been discussed in other threads recently.\n\nPatrick\n\n> > -\t\tif (ends_with(diter->basename, \".lock\"))\n> > -\t\t\tcontinue;\n> \n> This can safely go, as it is rejected by check_refname_format().\n> \n> > -\t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n> > -\t\t\t\t\t     diter->relative_path, 0,\n> > -\t\t\t\t\t     NULL, NULL)) {\n> > -\t\t\terror(\"bad ref for %s\", diter->path.buf);\n> > -\t\t\tcontinue;\n> > -\t\t}\n> \n> This is the focus of this step in the series.  We did not abort the\n> iteration before, but now we no longer issue any error message.\n> \n> >  \t\titer->base.refname = diter->relative_path;\n> >  \t\treturn ITER_OK;\n> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> > index 889bb1f1ba..efbbf23c72 100644\n> > --- a/refs/reftable-backend.c\n> > +++ b/refs/reftable-backend.c\n> > @@ -1659,11 +1659,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n> >  \t\tif (iter->last_name && !strcmp(iter->log.refname, iter->last_name))\n> >  \t\t\tcontinue;\n> >  \n> > -\t\tif (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,\n> > -\t\t\t\t\t     0, NULL, NULL)) {\n> > -\t\t\terror(_(\"bad ref for %s\"), iter->log.refname);\n> > +\t\tif (check_refname_format(iter->log.refname,\n> > +\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n> >  \t\t\tcontinue;\n> > -\t\t}\n> \n> This side is much more straight-forward.  Looking good.\n> \n> >  \n> >  \t\tfree(iter->last_name);\n> >  \t\titer->last_name = xstrdup(iter->log.refname);\n"},{"id":"488968","messageId":"ZdRkJkhBp-7hAJzZ@tanuki","threadId":"60955","inReplyTo":"xmqq7cj0ynys.fsf@gitster.g","subject":"Re: [PATCH 6/6] builtin/reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T08:34:46Z","receivedAt":"2024-02-20T08:34:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 19, 2024 at 04:32:43PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\n> > index d2f5f42e67..6d8d5a253d 100755\n> > --- a/t/t1410-reflog.sh\n> > +++ b/t/t1410-reflog.sh\n> > @@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '\n> >  \ttest_must_be_empty err\n> >  '\n> >  \n> > +test_expect_success 'list reflogs' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\tgit reflog list >actual &&\n> > +\t\ttest_must_be_empty actual &&\n> > +\n> > +\t\ttest_commit A &&\n> > +\t\tcat >expect <<-EOF &&\n> > +\t\tHEAD\n> > +\t\trefs/heads/main\n> > +\t\tEOF\n> > +\t\tgit reflog list >actual &&\n> > +\t\ttest_cmp expect actual &&\n> > +\n> > +\t\tgit branch b &&\n> > +\t\tcat >expect <<-EOF &&\n> > +\t\tHEAD\n> > +\t\trefs/heads/b\n> > +\t\trefs/heads/main\n> > +\t\tEOF\n> > +\t\tgit reflog list >actual &&\n> > +\t\ttest_cmp expect actual\n> > +\t)\n> > +'\n> \n> OK.  This is a quite boring baseline.\n> \n> > +test_expect_success 'reflog list returns error with additional args' '\n> > +\tcat >expect <<-EOF &&\n> > +\terror: list does not accept arguments: ${SQ}bogus${SQ}\n> > +\tEOF\n> > +\ttest_must_fail git reflog list bogus 2>err &&\n> > +\ttest_cmp expect err\n> > +'\n> \n> Makes sense.\n> \n> > +test_expect_success 'reflog for symref with unborn target can be listed' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\ttest_commit A &&\n> > +\t\tgit symbolic-ref HEAD refs/heads/unborn &&\n> > +\t\tcat >expect <<-EOF &&\n> > +\t\tHEAD\n> > +\t\trefs/heads/main\n> > +\t\tEOF\n> > +\t\tgit reflog list >actual &&\n> > +\t\ttest_cmp expect actual\n> > +\t)\n> > +'\n> \n> Should this be under REFFILES?  Ah, no, \"git symbolic-ref\" is valid\n> under reftable as well, so there is no need to.\n> \n> Without [5/6], would it have failed to show the reflog for HEAD?\n\nI initially thought so, but no. `refs_resolve_ref_unsafe()` is weird as\nit returns successfully even if a symref cannot be resolved unless you\npass `RESOLVE_REF_READING`, which we didn't.\n\nThe case where it does make a difference is if we had a corrupt ref. So\nif you \"echo garbage >.git/refs/heads/branch\", then the corresponding\nreflog would not have been listed. Even worse, even after this patch\nseries it's still impossible to `git reflog show` the reflog because we\nfail to resolve the ref itself, which basically breaks the whole point\nof the reflog.\n\nThis is something that I plan to address in a follow-up patch series.\n\n> > +test_expect_success 'reflog with invalid object ID can be listed' '\n> > +\ttest_when_finished \"rm -rf repo\" &&\n> > +\tgit init repo &&\n> > +\t(\n> > +\t\tcd repo &&\n> > +\t\ttest_commit A &&\n> > +\t\ttest-tool ref-store main update-ref msg refs/heads/missing \\\n> > +\t\t\t$(test_oid deadbeef) \"$ZERO_OID\" REF_SKIP_OID_VERIFICATION &&\n> > +\t\tcat >expect <<-EOF &&\n> > +\t\tHEAD\n> > +\t\trefs/heads/main\n> > +\t\trefs/heads/missing\n> > +\t\tEOF\n> > +\t\tgit reflog list >actual &&\n> > +\t\ttest_cmp expect actual\n> > +\t)\n> > +'\n> \n> OK.\n> \n> >  test_done\n> \n> It would have been \"interesting\" to see an example of \"there is a\n> reflog but the underlying ref for it is missing\" case, but I think\n> that falls into a minor repository corruption category, so lack of\n> such a test is also fine.\n\nThe reason why I didn't include such a test is that it's by necessity\nspecific to the backend: we don't have any way to delete a ref without\nalso deleting the corresponding reflog. So we'd have to manually delete\nit, which only works with the REFFILES backend.\n\nPatrick\n"},{"id":"488969","messageId":"ZdRkLylHKj44tstQ@tanuki","threadId":"60955","inReplyTo":"xmqq34to0znj.fsf@gitster.g","subject":"Re: [PATCH 3/6] refs/files: sort reflogs returned by the reflog iterator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T08:34:55Z","receivedAt":"2024-02-20T08:34:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Feb 19, 2024 at 04:04:16PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > We use a directory iterator to return reflogs via the reflog iterator.\n> > This iterator returns entries in the same order as readdir(3P) would and\n> > will thus yield reflogs with no discernible order.\n> >\n> > Set the new `DIR_ITERATOR_SORTED` flag that was introduced in the\n> > preceding commit so that the order is deterministic. While the effect of\n> > this can only been observed in a test tool, a subsequent commit will\n> > start to expose this functionality to users via a new `git reflog list`\n> > subcommand.\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  refs/files-backend.c           | 4 ++--\n> >  t/t0600-reffiles-backend.sh    | 4 ++--\n> >  t/t1405-main-ref-store.sh      | 2 +-\n> >  t/t1406-submodule-ref-store.sh | 2 +-\n> >  4 files changed, 6 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index 75dcc21ecb..2ffc63185f 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n> >  \n> >  \tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n> >  \n> > -\tditer = dir_iterator_begin(sb.buf, 0);\n> > +\tditer = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);\n> >  \tif (!diter) {\n> >  \t\tstrbuf_release(&sb);\n> >  \t\treturn empty_ref_iterator_begin();\n> > @@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n> >  \tCALLOC_ARRAY(iter, 1);\n> >  \tref_iterator = &iter->base;\n> >  \n> > -\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n> > +\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);\n> \n> This caught my attention.  Once we apply this patch, the only way\n> base_ref_iterator_init() can receive 0 for its last parameter\n> (i.e. 'ordered') is via the merge_ref_iterator_begin() call in\n> files_reflog_iterator_begin() that passes 0 as 'ordered'.  If we\n> force files_reflog_iterator_begin() to ask for an ordered\n> merge_ref_iterator, then we will have no unordered ref iterators.\n> Am I reading the code right?\n\nAh, true indeed. The \"files\" reflog iterator was the only remaining\niterator that wasn't ordered. I'll include an additional patch on top\nthat drops the `ordered` bit altogether.\n\nPatrick\n"},{"id":"488970","messageId":"cover.1708418805.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708353264.git.ps@pks.im","subject":"[PATCH v2 0/7] reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:06:16Z","receivedAt":"2024-02-20T09:06:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that introduces a new `git\nreflog list` subcommand to list available reflogs in a repository.\n\nChanges compared to v1:\n\n  - Patch 2: Clarified the commit message to hopefully explain better\n    why a higher level implementation of reflog sorting would have\n    increased latency.\n\n  - Patch 2: Introduced a helper function that unifies the logic to\n    yield the next directory entry.\n\n  - Patch 3: Mark the merged reflog iterator as sorted, which I missed\n    in my previous round.\n\n  - Patch 4: This patch is new and simplifies the code to require all\n    ref iterators to be sorted.\n\nJunio, I noticed that you already merged v1 of this patch series to\n`next`. I was a bit surprised to see it merged down this fast, so I\nassume that this is only done due to the pending Git v2.44 release and\nthat you plan to reroll `next` anyway. I thus didn't send follow-up\npatches but resent the whole patch series as v2. If I misinterpreted\nyour intent I'm happy to send the changes as follow-up patches instead.\n\nThe patch series continues to depend on ps/reftable-backend at\n8a0bebdeae (refs/reftable: fix leak when copying reflog fails,\n2024-02-08).\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (7):\n  dir-iterator: pass name to `prepare_next_entry_data()` directly\n  dir-iterator: support iteration in sorted order\n  refs/files: sort reflogs returned by the reflog iterator\n  refs: always treat iterators as ordered\n  refs: drop unused params from the reflog iterator callback\n  refs: stop resolving ref corresponding to reflogs\n  builtin/reflog: introduce subcommand to list reflogs\n\n Documentation/git-reflog.txt   |   3 +\n builtin/fsck.c                 |   4 +-\n builtin/reflog.c               |  37 +++++++++++-\n dir-iterator.c                 | 105 ++++++++++++++++++++++++++++-----\n dir-iterator.h                 |   3 +\n refs.c                         |  27 ++++++---\n refs.h                         |  11 +++-\n refs/debug.c                   |   3 +-\n refs/files-backend.c           |  27 ++-------\n refs/iterator.c                |  26 +++-----\n refs/packed-backend.c          |   2 +-\n refs/ref-cache.c               |   2 +-\n refs/refs-internal.h           |  18 +-----\n refs/reftable-backend.c        |  20 ++-----\n revision.c                     |   4 +-\n t/helper/test-ref-store.c      |  18 ++++--\n t/t0600-reffiles-backend.sh    |  24 ++++----\n t/t1405-main-ref-store.sh      |   8 +--\n t/t1406-submodule-ref-store.sh |   8 +--\n t/t1410-reflog.sh              |  69 ++++++++++++++++++++++\n 20 files changed, 286 insertions(+), 133 deletions(-)\n\nRange-diff against v1:\n1:  12de25dfe2 = 1:  12de25dfe2 dir-iterator: pass name to `prepare_next_entry_data()` directly\n2:  8a588175db ! 2:  788afce189 dir-iterator: support iteration in sorted order\n    @@ Commit message\n         iterator to enumerate reflogs, returning reflog names and exposing them\n         to the user would inherit the indeterministic ordering. Naturally, it\n         would make for a terrible user interface to show a list with no\n    -    discernible order. While this could be handled at a higher level by the\n    -    new subcommand itself by collecting and ordering the reflogs, this would\n    -    be inefficient and introduce latency when there are many reflogs.\n    +    discernible order.\n    +\n    +    While this could be handled at a higher level by the new subcommand\n    +    itself by collecting and ordering the reflogs, this would be inefficient\n    +    because we would first have to collect all reflogs before we can sort\n    +    them, which would introduce additional latency when there are many\n    +    reflogs.\n     \n         Instead, introduce a new option into the directory iterator that asks\n         for its entries to be yielded in lexicographical order. If set, the\n    -    iterator will read all directory entries greedily end sort them before\n    +    iterator will read all directory entries greedily and sort them before\n         we start to iterate over them.\n     \n         While this will of course also incur overhead as we cannot yield the\n    @@ dir-iterator.c\n      \n      struct dir_iterator_level {\n      \tDIR *dir;\n    + \n    ++\t/*\n    ++\t * The directory entries of the current level. This list will only be\n    ++\t * populated when the iterator is ordered. In that case, `dir` will be\n    ++\t * set to `NULL`.\n    ++\t */\n     +\tstruct string_list entries;\n     +\tsize_t entries_idx;\n    - \n    ++\n      \t/*\n      \t * The length of the directory part of path at this level\n    + \t * (including a trailing '/'):\n    +@@ dir-iterator.c: struct dir_iterator_int {\n    + \tunsigned int flags;\n    + };\n    + \n    ++static int next_directory_entry(DIR *dir, const char *path,\n    ++\t\t\t\tstruct dirent **out)\n    ++{\n    ++\tstruct dirent *de;\n    ++\n    ++repeat:\n    ++\terrno = 0;\n    ++\tde = readdir(dir);\n    ++\tif (!de) {\n    ++\t\tif (errno) {\n    ++\t\t\twarning_errno(\"error reading directory '%s'\",\n    ++\t\t\t\t      path);\n    ++\t\t\treturn -1;\n    ++\t\t}\n    ++\n    ++\t\treturn 1;\n    ++\t}\n    ++\n    ++\tif (is_dot_or_dotdot(de->d_name))\n    ++\t\tgoto repeat;\n    ++\n    ++\t*out = de;\n    ++\treturn 0;\n    ++}\n    ++\n    + /*\n    +  * Push a level in the iter stack and initialize it with information from\n    +  * the directory pointed by iter->base->path. It is assumed that this\n     @@ dir-iterator.c: static int push_level(struct dir_iterator_int *iter)\n      \t\treturn -1;\n      \t}\n    @@ dir-iterator.c: static int push_level(struct dir_iterator_int *iter)\n     +\t * directly.\n     +\t */\n     +\tif (iter->flags & DIR_ITERATOR_SORTED) {\n    -+\t\twhile (1) {\n    -+\t\t\tstruct dirent *de;\n    ++\t\tstruct dirent *de;\n     +\n    -+\t\t\terrno = 0;\n    -+\t\t\tde = readdir(level->dir);\n    -+\t\t\tif (!de) {\n    -+\t\t\t\tif (errno && errno != ENOENT) {\n    -+\t\t\t\t\twarning_errno(\"error reading directory '%s'\",\n    -+\t\t\t\t\t\t      iter->base.path.buf);\n    ++\t\twhile (1) {\n    ++\t\t\tint ret = next_directory_entry(level->dir, iter->base.path.buf, &de);\n    ++\t\t\tif (ret < 0) {\n    ++\t\t\t\tif (errno != ENOENT &&\n    ++\t\t\t\t    iter->flags & DIR_ITERATOR_PEDANTIC)\n     +\t\t\t\t\treturn -1;\n    -+\t\t\t\t}\n    -+\n    ++\t\t\t\tcontinue;\n    ++\t\t\t} else if (ret > 0) {\n     +\t\t\t\tbreak;\n     +\t\t\t}\n     +\n    -+\t\t\tif (is_dot_or_dotdot(de->d_name))\n    -+\t\t\t\tcontinue;\n    -+\n     +\t\t\tstring_list_append(&level->entries, de->d_name);\n     +\t\t}\n     +\t\tstring_list_sort(&level->entries);\n    @@ dir-iterator.c: static int pop_level(struct dir_iterator_int *iter)\n      \treturn --iter->levels_nr;\n      }\n     @@ dir-iterator.c: int dir_iterator_advance(struct dir_iterator *dir_iterator)\n    - \n    - \t/* Loop until we find an entry that we can give back to the caller. */\n    - \twhile (1) {\n    --\t\tstruct dirent *de;\n    + \t\tstruct dirent *de;\n      \t\tstruct dir_iterator_level *level =\n      \t\t\t&iter->levels[iter->levels_nr - 1];\n    -+\t\tstruct dirent *de;\n     +\t\tconst char *name;\n      \n      \t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n     -\t\terrno = 0;\n     -\t\tde = readdir(level->dir);\n    --\n    + \n     -\t\tif (!de) {\n     -\t\t\tif (errno) {\n     -\t\t\t\twarning_errno(\"error reading directory '%s'\",\n     -\t\t\t\t\t      iter->base.path.buf);\n    --\t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n    --\t\t\t\t\tgoto error_out;\n    ++\t\tif (level->dir) {\n    ++\t\t\tint ret = next_directory_entry(level->dir, iter->base.path.buf, &de);\n    ++\t\t\tif (ret < 0) {\n    + \t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n    + \t\t\t\t\tgoto error_out;\n     -\t\t\t} else if (pop_level(iter) == 0) {\n     -\t\t\t\treturn dir_iterator_abort(dir_iterator);\n    -+\n    -+\t\tif (level->dir) {\n    -+\t\t\terrno = 0;\n    -+\t\t\tde = readdir(level->dir);\n    -+\t\t\tif (!de) {\n    -+\t\t\t\tif (errno) {\n    -+\t\t\t\t\twarning_errno(\"error reading directory '%s'\",\n    -+\t\t\t\t\t\t      iter->base.path.buf);\n    -+\t\t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n    -+\t\t\t\t\t\tgoto error_out;\n    -+\t\t\t\t} else if (pop_level(iter) == 0) {\n    ++\t\t\t\tcontinue;\n    ++\t\t\t} else if (ret > 0) {\n    ++\t\t\t\tif (pop_level(iter) == 0)\n     +\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n    -+\t\t\t\t}\n     +\t\t\t\tcontinue;\n      \t\t\t}\n     -\t\t\tcontinue;\n    @@ dir-iterator.c: int dir_iterator_advance(struct dir_iterator *dir_iterator)\n      \n     -\t\tif (is_dot_or_dotdot(de->d_name))\n     -\t\t\tcontinue;\n    -+\t\t\tif (is_dot_or_dotdot(de->d_name))\n    -+\t\t\t\tcontinue;\n    - \n    --\t\tif (prepare_next_entry_data(iter, de->d_name)) {\n     +\t\t\tname = de->d_name;\n     +\t\t} else {\n     +\t\t\tif (level->entries_idx >= level->entries.nr) {\n    @@ dir-iterator.c: int dir_iterator_advance(struct dir_iterator *dir_iterator)\n     +\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n     +\t\t\t\tcontinue;\n     +\t\t\t}\n    -+\n    + \n    +-\t\tif (prepare_next_entry_data(iter, de->d_name)) {\n     +\t\t\tname = level->entries.items[level->entries_idx++].string;\n     +\t\t}\n     +\n3:  e4e4fac05c ! 3:  32b24a3d4b refs/files: sort reflogs returned by the reflog iterator\n    @@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r\n      \titer->dir_iterator = diter;\n      \titer->ref_store = ref_store;\n      \tstrbuf_release(&sb);\n    +@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n    + \t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n    + \t} else {\n    + \t\treturn merge_ref_iterator_begin(\n    +-\t\t\t0, reflog_iterator_begin(ref_store, refs->base.gitdir),\n    ++\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n    + \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n    + \t\t\treflog_iterator_select, refs);\n    + \t}\n     \n      ## t/t0600-reffiles-backend.sh ##\n     @@ t/t0600-reffiles-backend.sh: test_expect_success 'for_each_reflog()' '\n-:  ---------- > 4:  4254f23fd4 refs: always treat iterators as ordered\n4:  be512ef268 ! 5:  240334df6c refs: drop unused params from the reflog iterator callback\n    @@ refs/reftable-backend.c: static int reftable_reflog_iterator_advance(struct ref_\n      \t}\n     @@ refs/reftable-backend.c: static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n      \titer = xcalloc(1, sizeof(*iter));\n    - \tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);\n    + \tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);\n      \titer->refs = refs;\n     -\titer->base.oid = &iter->oid;\n      \n5:  a7459b9483 = 6:  7928661318 refs: stop resolving ref corresponding to reflogs\n6:  cddb2de939 = 7:  d7b9cff4c3 builtin/reflog: introduce subcommand to list reflogs\n-- \n2.44.0-rc1\n\n"},{"id":"488971","messageId":"12de25dfe24d61ef54e0ccc0ebd4cc69d73da50c.1708418805.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708418805.git.ps@pks.im","subject":"[PATCH v2 1/7] dir-iterator: pass name to `prepare_next_entry_data()` directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:06:22Z","receivedAt":"2024-02-20T09:06:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When adding the next directory entry for `struct dir_iterator` we pass\nthe complete `struct dirent *` to `prepare_next_entry_data()` even\nthough we only need the entry's name.\n\nRefactor the code to pass in the name, only. This prepares for a\nsubsequent commit where we introduce the ability to iterate through\ndir entries in an ordered manner.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n dir-iterator.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 278b04243a..f58a97e089 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -94,15 +94,15 @@ static int pop_level(struct dir_iterator_int *iter)\n \n /*\n  * Populate iter->base with the necessary information on the next iteration\n- * entry, represented by the given dirent de. Return 0 on success and -1\n+ * entry, represented by the given name. Return 0 on success and -1\n  * otherwise, setting errno accordingly.\n  */\n static int prepare_next_entry_data(struct dir_iterator_int *iter,\n-\t\t\t\t   struct dirent *de)\n+\t\t\t\t   const char *name)\n {\n \tint err, saved_errno;\n \n-\tstrbuf_addstr(&iter->base.path, de->d_name);\n+\tstrbuf_addstr(&iter->base.path, name);\n \t/*\n \t * We have to reset these because the path strbuf might have\n \t * been realloc()ed at the previous strbuf_addstr().\n@@ -159,7 +159,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n-\t\tif (prepare_next_entry_data(iter, de)) {\n+\t\tif (prepare_next_entry_data(iter, de->d_name)) {\n \t\t\tif (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n \t\t\t\tgoto error_out;\n \t\t\tcontinue;\n-- \n2.44.0-rc1\n\n"},{"id":"488972","messageId":"788afce189ce7ca7cb1ddc9706acb710ca73ee80.1708418805.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708418805.git.ps@pks.im","subject":"[PATCH v2 2/7] dir-iterator: support iteration in sorted order","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:06:26Z","receivedAt":"2024-02-20T09:06:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct dir_iterator` is a helper that allows us to iterate through\ndirectory entries. This iterator returns entries in the exact same order\nas readdir(3P) does -- or in other words, it guarantees no specific\norder at all.\n\nThis is about to become problematic as we are introducing a new reflog\nsubcommand to list reflogs. As the \"files\" backend uses the directory\niterator to enumerate reflogs, returning reflog names and exposing them\nto the user would inherit the indeterministic ordering. Naturally, it\nwould make for a terrible user interface to show a list with no\ndiscernible order.\n\nWhile this could be handled at a higher level by the new subcommand\nitself by collecting and ordering the reflogs, this would be inefficient\nbecause we would first have to collect all reflogs before we can sort\nthem, which would introduce additional latency when there are many\nreflogs.\n\nInstead, introduce a new option into the directory iterator that asks\nfor its entries to be yielded in lexicographical order. If set, the\niterator will read all directory entries greedily and sort them before\nwe start to iterate over them.\n\nWhile this will of course also incur overhead as we cannot yield the\ndirectory entries immediately, it should at least be more efficient than\nhaving to sort the complete list of reflogs as we only need to sort one\ndirectory at a time.\n\nThis functionality will be used in a follow-up commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n dir-iterator.c | 99 +++++++++++++++++++++++++++++++++++++++++++-------\n dir-iterator.h |  3 ++\n 2 files changed, 89 insertions(+), 13 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex f58a97e089..de619846f2 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -2,10 +2,19 @@\n #include \"dir.h\"\n #include \"iterator.h\"\n #include \"dir-iterator.h\"\n+#include \"string-list.h\"\n \n struct dir_iterator_level {\n \tDIR *dir;\n \n+\t/*\n+\t * The directory entries of the current level. This list will only be\n+\t * populated when the iterator is ordered. In that case, `dir` will be\n+\t * set to `NULL`.\n+\t */\n+\tstruct string_list entries;\n+\tsize_t entries_idx;\n+\n \t/*\n \t * The length of the directory part of path at this level\n \t * (including a trailing '/'):\n@@ -43,6 +52,31 @@ struct dir_iterator_int {\n \tunsigned int flags;\n };\n \n+static int next_directory_entry(DIR *dir, const char *path,\n+\t\t\t\tstruct dirent **out)\n+{\n+\tstruct dirent *de;\n+\n+repeat:\n+\terrno = 0;\n+\tde = readdir(dir);\n+\tif (!de) {\n+\t\tif (errno) {\n+\t\t\twarning_errno(\"error reading directory '%s'\",\n+\t\t\t\t      path);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\treturn 1;\n+\t}\n+\n+\tif (is_dot_or_dotdot(de->d_name))\n+\t\tgoto repeat;\n+\n+\t*out = de;\n+\treturn 0;\n+}\n+\n /*\n  * Push a level in the iter stack and initialize it with information from\n  * the directory pointed by iter->base->path. It is assumed that this\n@@ -72,6 +106,35 @@ static int push_level(struct dir_iterator_int *iter)\n \t\treturn -1;\n \t}\n \n+\tstring_list_init_dup(&level->entries);\n+\tlevel->entries_idx = 0;\n+\n+\t/*\n+\t * When the iterator is sorted we read and sort all directory entries\n+\t * directly.\n+\t */\n+\tif (iter->flags & DIR_ITERATOR_SORTED) {\n+\t\tstruct dirent *de;\n+\n+\t\twhile (1) {\n+\t\t\tint ret = next_directory_entry(level->dir, iter->base.path.buf, &de);\n+\t\t\tif (ret < 0) {\n+\t\t\t\tif (errno != ENOENT &&\n+\t\t\t\t    iter->flags & DIR_ITERATOR_PEDANTIC)\n+\t\t\t\t\treturn -1;\n+\t\t\t\tcontinue;\n+\t\t\t} else if (ret > 0) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tstring_list_append(&level->entries, de->d_name);\n+\t\t}\n+\t\tstring_list_sort(&level->entries);\n+\n+\t\tclosedir(level->dir);\n+\t\tlevel->dir = NULL;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -88,6 +151,7 @@ static int pop_level(struct dir_iterator_int *iter)\n \t\twarning_errno(\"error closing directory '%s'\",\n \t\t\t      iter->base.path.buf);\n \tlevel->dir = NULL;\n+\tstring_list_clear(&level->entries, 0);\n \n \treturn --iter->levels_nr;\n }\n@@ -139,27 +203,34 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\tstruct dirent *de;\n \t\tstruct dir_iterator_level *level =\n \t\t\t&iter->levels[iter->levels_nr - 1];\n+\t\tconst char *name;\n \n \t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n-\t\terrno = 0;\n-\t\tde = readdir(level->dir);\n \n-\t\tif (!de) {\n-\t\t\tif (errno) {\n-\t\t\t\twarning_errno(\"error reading directory '%s'\",\n-\t\t\t\t\t      iter->base.path.buf);\n+\t\tif (level->dir) {\n+\t\t\tint ret = next_directory_entry(level->dir, iter->base.path.buf, &de);\n+\t\t\tif (ret < 0) {\n \t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n \t\t\t\t\tgoto error_out;\n-\t\t\t} else if (pop_level(iter) == 0) {\n-\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\t\tcontinue;\n+\t\t\t} else if (ret > 0) {\n+\t\t\t\tif (pop_level(iter) == 0)\n+\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n \n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n+\t\t\tname = de->d_name;\n+\t\t} else {\n+\t\t\tif (level->entries_idx >= level->entries.nr) {\n+\t\t\t\tif (pop_level(iter) == 0)\n+\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \n-\t\tif (prepare_next_entry_data(iter, de->d_name)) {\n+\t\t\tname = level->entries.items[level->entries_idx++].string;\n+\t\t}\n+\n+\t\tif (prepare_next_entry_data(iter, name)) {\n \t\t\tif (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n \t\t\t\tgoto error_out;\n \t\t\tcontinue;\n@@ -188,6 +259,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n \t\t\twarning_errno(\"error closing directory '%s'\",\n \t\t\t\t      iter->base.path.buf);\n \t\t}\n+\n+\t\tstring_list_clear(&level->entries, 0);\n \t}\n \n \tfree(iter->levels);\ndiff --git a/dir-iterator.h b/dir-iterator.h\nindex 479e1ec784..6d438809b6 100644\n--- a/dir-iterator.h\n+++ b/dir-iterator.h\n@@ -54,8 +54,11 @@\n  *   and ITER_ERROR is returned immediately. In both cases, a meaningful\n  *   warning is emitted. Note: ENOENT errors are always ignored so that\n  *   the API users may remove files during iteration.\n+ *\n+ * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.\n  */\n #define DIR_ITERATOR_PEDANTIC (1 << 0)\n+#define DIR_ITERATOR_SORTED   (1 << 1)\n \n struct dir_iterator {\n \t/* The current path: */\n-- \n2.44.0-rc1\n\n"},{"id":"488973","messageId":"32b24a3d4b91a6073bb1a677080c74da828811a3.1708418805.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708418805.git.ps@pks.im","subject":"[PATCH v2 3/7] refs/files: sort reflogs returned by the reflog iterator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:06:30Z","receivedAt":"2024-02-20T09:06:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We use a directory iterator to return reflogs via the reflog iterator.\nThis iterator returns entries in the same order as readdir(3P) would and\nwill thus yield reflogs with no discernible order.\n\nSet the new `DIR_ITERATOR_SORTED` flag that was introduced in the\npreceding commit so that the order is deterministic. While the effect of\nthis can only been observed in a test tool, a subsequent commit will\nstart to expose this functionality to users via a new `git reflog list`\nsubcommand.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/files-backend.c           | 6 +++---\n t/t0600-reffiles-backend.sh    | 4 ++--\n t/t1405-main-ref-store.sh      | 2 +-\n t/t1406-submodule-ref-store.sh | 2 +-\n 4 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 75dcc21ecb..a7b7cdef36 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \n \tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n \n-\tditer = dir_iterator_begin(sb.buf, 0);\n+\tditer = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);\n \tif (!diter) {\n \t\tstrbuf_release(&sb);\n \t\treturn empty_ref_iterator_begin();\n@@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n+\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);\n \titer->dir_iterator = diter;\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\n@@ -2246,7 +2246,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n \t} else {\n \t\treturn merge_ref_iterator_begin(\n-\t\t\t0, reflog_iterator_begin(ref_store, refs->base.gitdir),\n+\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n \t\t\treflog_iterator_select, refs);\n \t}\ndiff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh\nindex e6a5f1868f..4f860285cc 100755\n--- a/t/t0600-reffiles-backend.sh\n+++ b/t/t0600-reffiles-backend.sh\n@@ -287,7 +287,7 @@ test_expect_success 'for_each_reflog()' '\n \tmkdir -p     .git/worktrees/wt/logs/refs/bisect &&\n \techo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&\n \n-\t$RWT for-each-reflog | cut -d\" \" -f 2- | sort >actual &&\n+\t$RWT for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \tPSEUDO-WT 0x0\n@@ -297,7 +297,7 @@ test_expect_success 'for_each_reflog()' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\t$RMAIN for-each-reflog | cut -d\" \" -f 2- | sort >actual &&\n+\t$RMAIN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \tPSEUDO-MAIN 0x0\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 976bd71efb..cfb583f544 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -74,7 +74,7 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | sort -k2 | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \trefs/heads/main 0x0\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex e6a7f7334b..40332e23cc 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -63,7 +63,7 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | sort | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \trefs/heads/main 0x0\n-- \n2.44.0-rc1\n\n"},{"id":"488974","messageId":"4254f23fd40076857ec093365a8adbb860803a72.1708418805.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708418805.git.ps@pks.im","subject":"[PATCH v2 4/7] refs: always treat iterators as ordered","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:06:34Z","receivedAt":"2024-02-20T09:06:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commit we have converted the reflog iterator of the\n\"files\" backend to be ordered, which was the only remaining ref iterator\nthat wasn't ordered. Refactor the ref iterator infrastructure so that we\nalways assume iterators to be ordered, thus simplifying the code.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c                  |  4 ----\n refs/debug.c            |  3 +--\n refs/files-backend.c    |  7 +++----\n refs/iterator.c         | 26 ++++++++------------------\n refs/packed-backend.c   |  2 +-\n refs/ref-cache.c        |  2 +-\n refs/refs-internal.h    | 18 ++----------------\n refs/reftable-backend.c |  8 ++++----\n 8 files changed, 20 insertions(+), 50 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fff343c256..dc25606a82 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1594,10 +1594,6 @@ struct ref_iterator *refs_ref_iterator_begin(\n \tif (trim)\n \t\titer = prefix_ref_iterator_begin(iter, \"\", trim);\n \n-\t/* Sanity check for subclasses: */\n-\tif (!iter->ordered)\n-\t\tBUG(\"reference iterator is not ordered\");\n-\n \treturn iter;\n }\n \ndiff --git a/refs/debug.c b/refs/debug.c\nindex 634681ca44..c7531b17f0 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -181,7 +181,6 @@ static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\ttrace_printf_key(&trace_refs, \"iterator_advance: %s (0)\\n\",\n \t\t\tditer->iter->refname);\n \n-\tditer->base.ordered = diter->iter->ordered;\n \tditer->base.refname = diter->iter->refname;\n \tditer->base.oid = diter->iter->oid;\n \tditer->base.flags = diter->iter->flags;\n@@ -222,7 +221,7 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,\n \t\tdrefs->refs->be->iterator_begin(drefs->refs, prefix,\n \t\t\t\t\t\texclude_patterns, flags);\n \tstruct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));\n-\tbase_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable);\n \tditer->iter = res;\n \ttrace_printf_key(&trace_refs, \"ref_iterator_begin: \\\"%s\\\" (0x%x)\\n\",\n \t\t\t prefix, flags);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a7b7cdef36..51d57d98d2 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -879,8 +879,7 @@ static struct ref_iterator *files_ref_iterator_begin(\n \n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable,\n-\t\t\t       overlay_iter->ordered);\n+\tbase_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable);\n \titer->iter0 = overlay_iter;\n \titer->repo = ref_store->repo;\n \titer->flags = flags;\n@@ -2202,7 +2201,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);\n+\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);\n \titer->dir_iterator = diter;\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\n@@ -2246,7 +2245,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n \t} else {\n \t\treturn merge_ref_iterator_begin(\n-\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n+\t\t\treflog_iterator_begin(ref_store, refs->base.gitdir),\n \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n \t\t\treflog_iterator_select, refs);\n \t}\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex 6b680f610e..f9a9a808e0 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -25,11 +25,9 @@ int ref_iterator_abort(struct ref_iterator *ref_iterator)\n }\n \n void base_ref_iterator_init(struct ref_iterator *iter,\n-\t\t\t    struct ref_iterator_vtable *vtable,\n-\t\t\t    int ordered)\n+\t\t\t    struct ref_iterator_vtable *vtable)\n {\n \titer->vtable = vtable;\n-\titer->ordered = !!ordered;\n \titer->refname = NULL;\n \titer->oid = NULL;\n \titer->flags = 0;\n@@ -74,7 +72,7 @@ struct ref_iterator *empty_ref_iterator_begin(void)\n \tstruct empty_ref_iterator *iter = xcalloc(1, sizeof(*iter));\n \tstruct ref_iterator *ref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable);\n \treturn ref_iterator;\n }\n \n@@ -207,7 +205,6 @@ static struct ref_iterator_vtable merge_ref_iterator_vtable = {\n };\n \n struct ref_iterator *merge_ref_iterator_begin(\n-\t\tint ordered,\n \t\tstruct ref_iterator *iter0, struct ref_iterator *iter1,\n \t\tref_iterator_select_fn *select, void *cb_data)\n {\n@@ -222,7 +219,7 @@ struct ref_iterator *merge_ref_iterator_begin(\n \t * references through only if they exist in both iterators.\n \t */\n \n-\tbase_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable, ordered);\n+\tbase_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable);\n \titer->iter0 = iter0;\n \titer->iter1 = iter1;\n \titer->select = select;\n@@ -271,12 +268,9 @@ struct ref_iterator *overlay_ref_iterator_begin(\n \t} else if (is_empty_ref_iterator(back)) {\n \t\tref_iterator_abort(back);\n \t\treturn front;\n-\t} else if (!front->ordered || !back->ordered) {\n-\t\tBUG(\"overlay_ref_iterator requires ordered inputs\");\n \t}\n \n-\treturn merge_ref_iterator_begin(1, front, back,\n-\t\t\t\t\toverlay_iterator_select, NULL);\n+\treturn merge_ref_iterator_begin(front, back, overlay_iterator_select, NULL);\n }\n \n struct prefix_ref_iterator {\n@@ -315,16 +309,12 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \n \t\tif (cmp > 0) {\n \t\t\t/*\n-\t\t\t * If the source iterator is ordered, then we\n+\t\t\t * As the source iterator is ordered, we\n \t\t\t * can stop the iteration as soon as we see a\n \t\t\t * refname that comes after the prefix:\n \t\t\t */\n-\t\t\tif (iter->iter0->ordered) {\n-\t\t\t\tok = ref_iterator_abort(iter->iter0);\n-\t\t\t\tbreak;\n-\t\t\t} else {\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\t\t\tok = ref_iterator_abort(iter->iter0);\n+\t\t\tbreak;\n \t\t}\n \n \t\tif (iter->trim) {\n@@ -396,7 +386,7 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable, iter0->ordered);\n+\tbase_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable);\n \n \titer->iter0 = iter0;\n \titer->prefix = xstrdup(prefix);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a499a91c7e..4e826c05ff 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1111,7 +1111,7 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);\n \n \tif (exclude_patterns)\n \t\tpopulate_excluded_jump_list(iter, snapshot, exclude_patterns);\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex a372a00941..9f9797209a 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -486,7 +486,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n \n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable);\n \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n \n \titer->levels_nr = 1;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 83e0f0bba3..1e8a9f9f13 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -312,13 +312,6 @@ enum do_for_each_ref_flags {\n  */\n struct ref_iterator {\n \tstruct ref_iterator_vtable *vtable;\n-\n-\t/*\n-\t * Does this `ref_iterator` iterate over references in order\n-\t * by refname?\n-\t */\n-\tunsigned int ordered : 1;\n-\n \tconst char *refname;\n \tconst struct object_id *oid;\n \tunsigned int flags;\n@@ -390,11 +383,9 @@ typedef enum iterator_selection ref_iterator_select_fn(\n  * Iterate over the entries from iter0 and iter1, with the values\n  * interleaved as directed by the select function. The iterator takes\n  * ownership of iter0 and iter1 and frees them when the iteration is\n- * over. A derived class should set `ordered` to 1 or 0 based on\n- * whether it generates its output in order by reference name.\n+ * over.\n  */\n struct ref_iterator *merge_ref_iterator_begin(\n-\t\tint ordered,\n \t\tstruct ref_iterator *iter0, struct ref_iterator *iter1,\n \t\tref_iterator_select_fn *select, void *cb_data);\n \n@@ -423,8 +414,6 @@ struct ref_iterator *overlay_ref_iterator_begin(\n  * As an convenience to callers, if prefix is the empty string and\n  * trim is zero, this function returns iter0 directly, without\n  * wrapping it.\n- *\n- * The resulting ref_iterator is ordered if iter0 is.\n  */\n struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \t\t\t\t\t       const char *prefix,\n@@ -435,14 +424,11 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n /*\n  * Base class constructor for ref_iterators. Initialize the\n  * ref_iterator part of iter, setting its vtable pointer as specified.\n- * `ordered` should be set to 1 if the iterator will iterate over\n- * references in order by refname; otherwise it should be set to 0.\n  * This is meant to be called only by the initializers of derived\n  * classes.\n  */\n void base_ref_iterator_init(struct ref_iterator *iter,\n-\t\t\t    struct ref_iterator_vtable *vtable,\n-\t\t\t    int ordered);\n+\t\t\t    struct ref_iterator_vtable *vtable);\n \n /*\n  * Base class destructor for ref_iterators. Destroy the ref_iterator\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex a14f2ad7f4..70a16dfb9e 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -479,7 +479,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_\n \tint ret;\n \n \titer = xcalloc(1, sizeof(*iter));\n-\tbase_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable);\n \titer->prefix = prefix;\n \titer->base.oid = &iter->oid;\n \titer->flags = flags;\n@@ -575,7 +575,7 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto\n \t * single iterator.\n \t */\n \tworktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);\n-\treturn merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,\n+\treturn merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,\n \t\t\t\t\titerator_select, NULL);\n }\n \n@@ -1723,7 +1723,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n \tint ret;\n \n \titer = xcalloc(1, sizeof(*iter));\n-\tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);\n+\tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);\n \titer->refs = refs;\n \titer->base.oid = &iter->oid;\n \n@@ -1758,7 +1758,7 @@ static struct ref_iterator *reftable_be_reflog_iterator_begin(struct ref_store *\n \n \tworktree_iter = reflog_iterator_for_stack(refs, refs->worktree_stack);\n \n-\treturn merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,\n+\treturn merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,\n \t\t\t\t\titerator_select, NULL);\n }\n \n-- \n2.44.0-rc1\n\n"},{"id":"488975","messageId":"240334df6c7d0e95f67fdeddb8b8a381a59245fa.1708418805.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708418805.git.ps@pks.im","subject":"[PATCH v2 5/7] refs: drop unused params from the reflog iterator callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:06:39Z","receivedAt":"2024-02-20T09:06:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The ref and reflog iterators share much of the same underlying code to\niterate over the corresponding entries. This results in some weird code\nbecause the reflog iterator also exposes an object ID as well as a flag\nto the callback function. Neither of these fields do refer to the reflog\nthough -- they refer to the corresponding ref with the same name. This\nis quite misleading. In practice at least the object ID cannot really be\nimplemented in any other way as a reflog does not have a specific object\nID in the first place. This is further stressed by the fact that none of\nthe callbacks except for our test helper make use of these fields.\n\nSplit up the infrastucture so that ref and reflog iterators use separate\ncallback signatures. This allows us to drop the nonsensical fields from\nthe reflog iterator.\n\nNote that internally, the backends still use the same shared infra to\niterate over both types. As the backends should never end up being\ncalled directly anyway, this is not much of a problem and thus kept\nas-is for simplicity's sake.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fsck.c                 |  4 +---\n builtin/reflog.c               |  3 +--\n refs.c                         | 23 +++++++++++++++++++----\n refs.h                         | 11 +++++++++--\n refs/files-backend.c           |  8 +-------\n refs/reftable-backend.c        |  8 +-------\n revision.c                     |  4 +---\n t/helper/test-ref-store.c      | 18 ++++++++++++------\n t/t0600-reffiles-backend.sh    | 24 ++++++++++++------------\n t/t1405-main-ref-store.sh      |  8 ++++----\n t/t1406-submodule-ref-store.sh |  8 ++++----\n 11 files changed, 65 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex a7cf94f67e..f892487c9b 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -509,9 +509,7 @@ static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid\n \treturn 0;\n }\n \n-static int fsck_handle_reflog(const char *logname,\n-\t\t\t      const struct object_id *oid UNUSED,\n-\t\t\t      int flag UNUSED, void *cb_data)\n+static int fsck_handle_reflog(const char *logname, void *cb_data)\n {\n \tstruct strbuf refname = STRBUF_INIT;\n \ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex a5a4099f61..3a0c4d4322 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -60,8 +60,7 @@ struct worktree_reflogs {\n \tstruct string_list reflogs;\n };\n \n-static int collect_reflog(const char *ref, const struct object_id *oid UNUSED,\n-\t\t\t  int flags UNUSED, void *cb_data)\n+static int collect_reflog(const char *ref, void *cb_data)\n {\n \tstruct worktree_reflogs *cb = cb_data;\n \tstruct worktree *worktree = cb->worktree;\ndiff --git a/refs.c b/refs.c\nindex dc25606a82..f9261267f0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2512,18 +2512,33 @@ int refs_verify_refname_available(struct ref_store *refs,\n \treturn ret;\n }\n \n-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n+struct do_for_each_reflog_help {\n+\teach_reflog_fn *fn;\n+\tvoid *cb_data;\n+};\n+\n+static int do_for_each_reflog_helper(struct repository *r UNUSED,\n+\t\t\t\t     const char *refname,\n+\t\t\t\t     const struct object_id *oid UNUSED,\n+\t\t\t\t     int flags,\n+\t\t\t\t     void *cb_data)\n+{\n+\tstruct do_for_each_reflog_help *hp = cb_data;\n+\treturn hp->fn(refname, hp->cb_data);\n+}\n+\n+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n-\tstruct do_for_each_ref_help hp = { fn, cb_data };\n+\tstruct do_for_each_reflog_help hp = { fn, cb_data };\n \n \titer = refs->be->reflog_iterator_begin(refs);\n \n \treturn do_for_each_repo_ref_iterator(the_repository, iter,\n-\t\t\t\t\t     do_for_each_ref_helper, &hp);\n+\t\t\t\t\t     do_for_each_reflog_helper, &hp);\n }\n \n-int for_each_reflog(each_ref_fn fn, void *cb_data)\n+int for_each_reflog(each_reflog_fn fn, void *cb_data)\n {\n \treturn refs_for_each_reflog(get_main_ref_store(the_repository), fn, cb_data);\n }\ndiff --git a/refs.h b/refs.h\nindex 303c5fac4d..895579aeb7 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -534,12 +534,19 @@ int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_dat\n /* youngest entry first */\n int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data);\n \n+/*\n+ * The signature for the callback function for the {refs_,}for_each_reflog()\n+ * functions below. The memory pointed to by the refname argument is only\n+ * guaranteed to be valid for the duration of a single callback invocation.\n+ */\n+typedef int each_reflog_fn(const char *refname, void *cb_data);\n+\n /*\n  * Calls the specified function for each reflog file until it returns nonzero,\n  * and returns the value. Reflog file order is unspecified.\n  */\n-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n-int for_each_reflog(each_ref_fn fn, void *cb_data);\n+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);\n+int for_each_reflog(each_reflog_fn fn, void *cb_data);\n \n #define REFNAME_ALLOW_ONELEVEL 1\n #define REFNAME_REFSPEC_PATTERN 2\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 51d57d98d2..48cc60d71b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2115,10 +2115,8 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,\n \n struct files_reflog_iterator {\n \tstruct ref_iterator base;\n-\n \tstruct ref_store *ref_store;\n \tstruct dir_iterator *dir_iterator;\n-\tstruct object_id oid;\n };\n \n static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n@@ -2129,8 +2127,6 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \tint ok;\n \n \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n-\t\tint flags;\n-\n \t\tif (!S_ISREG(diter->st.st_mode))\n \t\t\tcontinue;\n \t\tif (diter->basename[0] == '.')\n@@ -2140,14 +2136,12 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \n \t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n \t\t\t\t\t     diter->relative_path, 0,\n-\t\t\t\t\t     &iter->oid, &flags)) {\n+\t\t\t\t\t     NULL, NULL)) {\n \t\t\terror(\"bad ref for %s\", diter->path.buf);\n \t\t\tcontinue;\n \t\t}\n \n \t\titer->base.refname = diter->relative_path;\n-\t\titer->base.oid = &iter->oid;\n-\t\titer->base.flags = flags;\n \t\treturn ITER_OK;\n \t}\n \ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 70a16dfb9e..5247e09d58 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1637,7 +1637,6 @@ struct reftable_reflog_iterator {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_iterator iter;\n \tstruct reftable_log_record log;\n-\tstruct object_id oid;\n \tchar *last_name;\n \tint err;\n };\n@@ -1648,8 +1647,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t(struct reftable_reflog_iterator *)ref_iterator;\n \n \twhile (!iter->err) {\n-\t\tint flags;\n-\n \t\titer->err = reftable_iterator_next_log(&iter->iter, &iter->log);\n \t\tif (iter->err)\n \t\t\tbreak;\n@@ -1663,7 +1660,7 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t\tcontinue;\n \n \t\tif (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,\n-\t\t\t\t\t     0, &iter->oid, &flags)) {\n+\t\t\t\t\t     0, NULL, NULL)) {\n \t\t\terror(_(\"bad ref for %s\"), iter->log.refname);\n \t\t\tcontinue;\n \t\t}\n@@ -1671,8 +1668,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tfree(iter->last_name);\n \t\titer->last_name = xstrdup(iter->log.refname);\n \t\titer->base.refname = iter->log.refname;\n-\t\titer->base.oid = &iter->oid;\n-\t\titer->base.flags = flags;\n \n \t\tbreak;\n \t}\n@@ -1725,7 +1720,6 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n \titer = xcalloc(1, sizeof(*iter));\n \tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);\n \titer->refs = refs;\n-\titer->base.oid = &iter->oid;\n \n \tret = refs->err;\n \tif (ret)\ndiff --git a/revision.c b/revision.c\nindex 2424c9bd67..ac45c6d8f2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1686,9 +1686,7 @@ static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n \treturn 0;\n }\n \n-static int handle_one_reflog(const char *refname_in_wt,\n-\t\t\t     const struct object_id *oid UNUSED,\n-\t\t\t     int flag UNUSED, void *cb_data)\n+static int handle_one_reflog(const char *refname_in_wt, void *cb_data)\n {\n \tstruct all_refs_cb *cb = cb_data;\n \tstruct strbuf refname = STRBUF_INIT;\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 702ec1f128..7a0f6cac53 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -221,15 +221,21 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)\n \treturn ret;\n }\n \n+static int each_reflog(const char *refname, void *cb_data UNUSED)\n+{\n+\tprintf(\"%s\\n\", refname);\n+\treturn 0;\n+}\n+\n static int cmd_for_each_reflog(struct ref_store *refs,\n \t\t\t       const char **argv UNUSED)\n {\n-\treturn refs_for_each_reflog(refs, each_ref, NULL);\n+\treturn refs_for_each_reflog(refs, each_reflog, NULL);\n }\n \n-static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n-\t\t       const char *committer, timestamp_t timestamp,\n-\t\t       int tz, const char *msg, void *cb_data UNUSED)\n+static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,\n+\t\t\t   const char *committer, timestamp_t timestamp,\n+\t\t\t   int tz, const char *msg, void *cb_data UNUSED)\n {\n \tprintf(\"%s %s %s %\" PRItime \" %+05d%s%s\", oid_to_hex(old_oid),\n \t       oid_to_hex(new_oid), committer, timestamp, tz,\n@@ -241,14 +247,14 @@ static int cmd_for_each_reflog_ent(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n \n-\treturn refs_for_each_reflog_ent(refs, refname, each_reflog, refs);\n+\treturn refs_for_each_reflog_ent(refs, refname, each_reflog_ent, refs);\n }\n \n static int cmd_for_each_reflog_ent_reverse(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n \n-\treturn refs_for_each_reflog_ent_reverse(refs, refname, each_reflog, refs);\n+\treturn refs_for_each_reflog_ent_reverse(refs, refname, each_reflog_ent, refs);\n }\n \n static int cmd_reflog_exists(struct ref_store *refs, const char **argv)\ndiff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh\nindex 4f860285cc..56a3196b83 100755\n--- a/t/t0600-reffiles-backend.sh\n+++ b/t/t0600-reffiles-backend.sh\n@@ -287,23 +287,23 @@ test_expect_success 'for_each_reflog()' '\n \tmkdir -p     .git/worktrees/wt/logs/refs/bisect &&\n \techo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&\n \n-\t$RWT for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RWT for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\tPSEUDO-WT 0x0\n-\trefs/bisect/wt-random 0x0\n-\trefs/heads/main 0x0\n-\trefs/heads/wt-main 0x0\n+\tHEAD\n+\tPSEUDO-WT\n+\trefs/bisect/wt-random\n+\trefs/heads/main\n+\trefs/heads/wt-main\n \tEOF\n \ttest_cmp expected actual &&\n \n-\t$RMAIN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RMAIN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\tPSEUDO-MAIN 0x0\n-\trefs/bisect/random 0x0\n-\trefs/heads/main 0x0\n-\trefs/heads/wt-main 0x0\n+\tHEAD\n+\tPSEUDO-MAIN\n+\trefs/bisect/random\n+\trefs/heads/main\n+\trefs/heads/wt-main\n \tEOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex cfb583f544..3eee758bce 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -74,11 +74,11 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\trefs/heads/main 0x0\n-\trefs/heads/new-main 0x0\n+\tHEAD\n+\trefs/heads/main\n+\trefs/heads/new-main\n \tEOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex 40332e23cc..c01f0f14a1 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -63,11 +63,11 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\trefs/heads/main 0x0\n-\trefs/heads/new-main 0x0\n+\tHEAD\n+\trefs/heads/main\n+\trefs/heads/new-main\n \tEOF\n \ttest_cmp expected actual\n '\n-- \n2.44.0-rc1\n\n"},{"id":"488976","messageId":"7928661318a635022b65db543bd551018057c11f.1708418805.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708418805.git.ps@pks.im","subject":"[PATCH v2 6/7] refs: stop resolving ref corresponding to reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:06:43Z","receivedAt":"2024-02-20T09:06:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reflog iterator tries to resolve the corresponding ref for every\nreflog that it is about to yield. Historically, this was done due to\nmultiple reasons:\n\n  - It ensures that the refname is safe because we end up calling\n    `check_refname_format()`. Also, non-conformant refnames are skipped\n    altogether.\n\n  - The iterator used to yield the resolved object ID as well as its\n    flags to the callback. This info was never used though, and the\n    corresponding parameters were dropped in the preceding commit.\n\n  - When a ref is corrupt then the reflog is not emitted at all.\n\nWe're about to introduce a new `git reflog list` subcommand that will\nprint all reflogs that the refdb knows about. Skipping over reflogs\nwhose refs are corrupted would be quite counterproductive in this case\nas the user would have no way to learn about reflogs which may still\nexist in their repository to help and rescue such a corrupted ref. Thus,\nthe only remaining reason for why we'd want to resolve the ref is to\nverify its refname.\n\nRefactor the code to call `check_refname_format()` directly instead of\ntrying to resolve the ref. This is significantly more efficient given\nthat we don't have to hit the object database anymore to list reflogs.\nAnd second, it ensures that we end up showing reflogs of broken refs,\nwhich will help to make the reflog more useful.\n\nNote that this really only impacts the case where the corresponding ref\nis corrupt. Reflogs for nonexistent refs would have been returned to the\ncaller beforehand already as we did not pass `RESOLVE_REF_READING` to\nthe function, and thus `refs_resolve_ref_unsafe()` would have returned\nsuccessfully in that case.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/files-backend.c    | 12 ++----------\n refs/reftable-backend.c |  6 ++----\n 2 files changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 48cc60d71b..4726b04baa 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2129,17 +2129,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n \t\tif (!S_ISREG(diter->st.st_mode))\n \t\t\tcontinue;\n-\t\tif (diter->basename[0] == '.')\n+\t\tif (check_refname_format(diter->basename,\n+\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\t\tcontinue;\n-\t\tif (ends_with(diter->basename, \".lock\"))\n-\t\t\tcontinue;\n-\n-\t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n-\t\t\t\t\t     diter->relative_path, 0,\n-\t\t\t\t\t     NULL, NULL)) {\n-\t\t\terror(\"bad ref for %s\", diter->path.buf);\n-\t\t\tcontinue;\n-\t\t}\n \n \t\titer->base.refname = diter->relative_path;\n \t\treturn ITER_OK;\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 5247e09d58..f3200a1886 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1659,11 +1659,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (iter->last_name && !strcmp(iter->log.refname, iter->last_name))\n \t\t\tcontinue;\n \n-\t\tif (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,\n-\t\t\t\t\t     0, NULL, NULL)) {\n-\t\t\terror(_(\"bad ref for %s\"), iter->log.refname);\n+\t\tif (check_refname_format(iter->log.refname,\n+\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\t\tcontinue;\n-\t\t}\n \n \t\tfree(iter->last_name);\n \t\titer->last_name = xstrdup(iter->log.refname);\n-- \n2.44.0-rc1\n\n"},{"id":"488977","messageId":"d7b9cff4c360147e65df17316533fba0b4f2ab7d.1708418805.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708418805.git.ps@pks.im","subject":"[PATCH v2 7/7] builtin/reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-20T09:06:47Z","receivedAt":"2024-02-20T09:06:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the git-reflog(1) command has subcommands to show reflog entries\nor check for reflog existence, it does not have any subcommands that\nwould allow the user to enumerate all existing reflogs. This makes it\nquite hard to discover which reflogs a repository has. While this can\nbe worked around with the \"files\" backend by enumerating files in the\n\".git/logs\" directory, users of the \"reftable\" backend don't enjoy such\na luxury.\n\nIntroduce a new subcommand `git reflog list` that lists all reflogs the\nrepository knows of to fill this gap.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/git-reflog.txt |  3 ++\n builtin/reflog.c             | 34 ++++++++++++++++++\n t/t1410-reflog.sh            | 69 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 106 insertions(+)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex ec64cbff4c..a929c52982 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git reflog' [show] [<log-options>] [<ref>]\n+'git reflog list'\n 'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]\n \t[--rewrite] [--updateref] [--stale-fix]\n \t[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]\n@@ -39,6 +40,8 @@ actions, and in addition the `HEAD` reflog records branch switching.\n `git reflog show` is an alias for `git log -g --abbrev-commit\n --pretty=oneline`; see linkgit:git-log[1] for more information.\n \n+The \"list\" subcommand lists all refs which have a corresponding reflog.\n+\n The \"expire\" subcommand prunes older reflog entries. Entries older\n than `expire` time, or entries older than `expire-unreachable` time\n and not reachable from the current tip, are removed from the reflog.\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 3a0c4d4322..63cd4d8b29 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -7,11 +7,15 @@\n #include \"wildmatch.h\"\n #include \"worktree.h\"\n #include \"reflog.h\"\n+#include \"refs.h\"\n #include \"parse-options.h\"\n \n #define BUILTIN_REFLOG_SHOW_USAGE \\\n \tN_(\"git reflog [show] [<log-options>] [<ref>]\")\n \n+#define BUILTIN_REFLOG_LIST_USAGE \\\n+\tN_(\"git reflog list\")\n+\n #define BUILTIN_REFLOG_EXPIRE_USAGE \\\n \tN_(\"git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\\n\" \\\n \t   \"                  [--rewrite] [--updateref] [--stale-fix]\\n\" \\\n@@ -29,6 +33,11 @@ static const char *const reflog_show_usage[] = {\n \tNULL,\n };\n \n+static const char *const reflog_list_usage[] = {\n+\tBUILTIN_REFLOG_LIST_USAGE,\n+\tNULL,\n+};\n+\n static const char *const reflog_expire_usage[] = {\n \tBUILTIN_REFLOG_EXPIRE_USAGE,\n \tNULL\n@@ -46,6 +55,7 @@ static const char *const reflog_exists_usage[] = {\n \n static const char *const reflog_usage[] = {\n \tBUILTIN_REFLOG_SHOW_USAGE,\n+\tBUILTIN_REFLOG_LIST_USAGE,\n \tBUILTIN_REFLOG_EXPIRE_USAGE,\n \tBUILTIN_REFLOG_DELETE_USAGE,\n \tBUILTIN_REFLOG_EXISTS_USAGE,\n@@ -238,6 +248,29 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)\n \treturn cmd_log_reflog(argc, argv, prefix);\n }\n \n+static int show_reflog(const char *refname, void *cb_data UNUSED)\n+{\n+\tprintf(\"%s\\n\", refname);\n+\treturn 0;\n+}\n+\n+static int cmd_reflog_list(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tstruct ref_store *ref_store;\n+\n+\targc = parse_options(argc, argv, prefix, options, reflog_list_usage, 0);\n+\tif (argc)\n+\t\treturn error(_(\"%s does not accept arguments: '%s'\"),\n+\t\t\t     \"list\", argv[0]);\n+\n+\tref_store = get_main_ref_store(the_repository);\n+\n+\treturn refs_for_each_reflog(ref_store, show_reflog, NULL);\n+}\n+\n static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n {\n \tstruct cmd_reflog_expire_cb cmd = { 0 };\n@@ -417,6 +450,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)\n \tparse_opt_subcommand_fn *fn = NULL;\n \tstruct option options[] = {\n \t\tOPT_SUBCOMMAND(\"show\", &fn, cmd_reflog_show),\n+\t\tOPT_SUBCOMMAND(\"list\", &fn, cmd_reflog_list),\n \t\tOPT_SUBCOMMAND(\"expire\", &fn, cmd_reflog_expire),\n \t\tOPT_SUBCOMMAND(\"delete\", &fn, cmd_reflog_delete),\n \t\tOPT_SUBCOMMAND(\"exists\", &fn, cmd_reflog_exists),\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex d2f5f42e67..6d8d5a253d 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -436,4 +436,73 @@ test_expect_success 'empty reflog' '\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'list reflogs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit reflog list >actual &&\n+\t\ttest_must_be_empty actual &&\n+\n+\t\ttest_commit A &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit branch b &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/b\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'reflog list returns error with additional args' '\n+\tcat >expect <<-EOF &&\n+\terror: list does not accept arguments: ${SQ}bogus${SQ}\n+\tEOF\n+\ttest_must_fail git reflog list bogus 2>err &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success 'reflog for symref with unborn target can be listed' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit symbolic-ref HEAD refs/heads/unborn &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'reflog with invalid object ID can be listed' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\ttest-tool ref-store main update-ref msg refs/heads/missing \\\n+\t\t\t$(test_oid deadbeef) \"$ZERO_OID\" REF_SKIP_OID_VERIFICATION &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\trefs/heads/missing\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.44.0-rc1\n\n"},{"id":"488996","messageId":"xmqq34tnrqxv.fsf@gitster.g","threadId":"60955","inReplyTo":"cover.1708418805.git.ps@pks.im","subject":"Re: [PATCH v2 0/7] reflog: introduce subcommand to list reflogs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T17:22:36Z","receivedAt":"2024-02-20T17:22:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>       struct dir_iterator_level {\n>       \tDIR *dir;\n>     + \n>     ++\t/*\n>     ++\t * The directory entries of the current level. This list will only be\n>     ++\t * populated when the iterator is ordered. In that case, `dir` will be\n>     ++\t * set to `NULL`.\n>     ++\t */\n>      +\tstruct string_list entries;\n>      +\tsize_t entries_idx;\n\nReads well.  Nice.\n\n>     ++static int next_directory_entry(DIR *dir, const char *path,\n>     ++\t\t\t\tstruct dirent **out)\n>     ++{\n>     ++\tstruct dirent *de;\n>     ++\n>     ++repeat:\n>     ++\terrno = 0;\n>     ++\tde = readdir(dir);\n>     ++\tif (!de) {\n>     ++\t\tif (errno) {\n>     ++\t\t\twarning_errno(\"error reading directory '%s'\",\n>     ++\t\t\t\t      path);\n>     ++\t\t\treturn -1;\n>     ++\t\t}\n>     ++\n>     ++\t\treturn 1;\n>     ++\t}\n>     ++\n>     ++\tif (is_dot_or_dotdot(de->d_name))\n>     ++\t\tgoto repeat;\n>     ++\n>     ++\t*out = de;\n>     ++\treturn 0;\n>     ++}\n\nVery nice to encapsulate the common readdir() loop into this helper.\n\n> 3:  e4e4fac05c ! 3:  32b24a3d4b refs/files: sort reflogs returned by the reflog iterator\n>     @@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r\n>       \titer->dir_iterator = diter;\n>       \titer->ref_store = ref_store;\n>       \tstrbuf_release(&sb);\n>     +@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n>     + \t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n>     + \t} else {\n>     + \t\treturn merge_ref_iterator_begin(\n>     +-\t\t\t0, reflog_iterator_begin(ref_store, refs->base.gitdir),\n>     ++\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n>     + \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n>     + \t\t\treflog_iterator_select, refs);\n>     + \t}\n\nThis hunk is new.  Is there a downside to force merged iterators to\nalways be sorted?  The ones that are combined are all sorted so it\nis natural to force sorting like this code does?  It might deserve\nexplaining, and would certainly help future readers who runs \"blame\"\non this code to figure out what made us think always sorting is a\ngood direction forward.\n\n> -:  ---------- > 4:  4254f23fd4 refs: always treat iterators as ordered\n\nThis one is new, and deserves a separate review.\n\n> 4:  be512ef268 ! 5:  240334df6c refs: drop unused params from the reflog iterator callback\n>     @@ refs/reftable-backend.c: static int reftable_reflog_iterator_advance(struct ref_\n>       \t}\n>      @@ refs/reftable-backend.c: static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n>       \titer = xcalloc(1, sizeof(*iter));\n>     - \tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);\n>     + \tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);\n>       \titer->refs = refs;\n>      -\titer->base.oid = &iter->oid;\n>       \n> 5:  a7459b9483 = 6:  7928661318 refs: stop resolving ref corresponding to reflogs\n> 6:  cddb2de939 = 7:  d7b9cff4c3 builtin/reflog: introduce subcommand to list reflogs\n\nLooking good from a cursory read.\n\nThanks for a quick reroll.\n"},{"id":"489067","messageId":"ZdXi8agx5oxKfwrD@tanuki","threadId":"60955","inReplyTo":"xmqq34tnrqxv.fsf@gitster.g","subject":"Re: [PATCH v2 0/7] reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T11:48:01Z","receivedAt":"2024-02-21T11:48:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 20, 2024 at 09:22:36AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> > 3:  e4e4fac05c ! 3:  32b24a3d4b refs/files: sort reflogs returned by the reflog iterator\n> >     @@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r\n> >       \titer->dir_iterator = diter;\n> >       \titer->ref_store = ref_store;\n> >       \tstrbuf_release(&sb);\n> >     +@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n> >     + \t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n> >     + \t} else {\n> >     + \t\treturn merge_ref_iterator_begin(\n> >     +-\t\t\t0, reflog_iterator_begin(ref_store, refs->base.gitdir),\n> >     ++\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n> >     + \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n> >     + \t\t\treflog_iterator_select, refs);\n> >     + \t}\n> \n> This hunk is new.  Is there a downside to force merged iterators to\n> always be sorted?  The ones that are combined are all sorted so it\n> is natural to force sorting like this code does?  It might deserve\n> explaining, and would certainly help future readers who runs \"blame\"\n> on this code to figure out what made us think always sorting is a\n> good direction forward.\n\nNot really -- it merely gets passed down to the base ref iterator to\nindicate that the entries are returned in lexicographic order. But I've\nbeen jumping the gun here: the `reflog_iterator_select()` function does\nnot ensure lexicographic ordering between the two merged iterators right\nnow. I was assuming so because I implemented it in the reftable backend\nlike that. Should've double checked.\n\nIt's an easy fix though, which I'll add as another patch on top. Thanks\nfor making me think twice.\n\nPatrick\n"},{"id":"489068","messageId":"cover.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708353264.git.ps@pks.im","subject":"[PATCH v3 0/8] reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:15Z","receivedAt":"2024-02-21T12:37:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that introduces a new `git\nreflog list` subcommand to list available reflogs in a repository.\n\nThere is only a single change compared to v2. Junio made me double check\nmy assumption that the select function for reflogs yielded by the merged\niterator in the \"files\" backend would return things lexicographically.\nTurns out it didn't -- instead, it returns all worktree refs first, then\nall common refs.\n\nThis isn't only an issue because I prematurely marked the merged iter as\nsorted. It's also an issue because the user-visible ordering would be\nquite weird when executing `git reflog list` in a worktree.\n\nI've thus added another commit on top that extracts the preexisting\nlogic to merge worktree and common refs by name from the \"reftable\"\nbackend and reuses it for the \"files\" reflogs.\n\nPatrick\n\nPatrick Steinhardt (8):\n  dir-iterator: pass name to `prepare_next_entry_data()` directly\n  dir-iterator: support iteration in sorted order\n  refs/files: sort reflogs returned by the reflog iterator\n  refs/files: sort merged worktree and common reflogs\n  refs: always treat iterators as ordered\n  refs: drop unused params from the reflog iterator callback\n  refs: stop resolving ref corresponding to reflogs\n  builtin/reflog: introduce subcommand to list reflogs\n\n Documentation/git-reflog.txt   |   3 +\n builtin/fsck.c                 |   4 +-\n builtin/reflog.c               |  37 ++++++++++-\n dir-iterator.c                 | 105 +++++++++++++++++++++++++++-----\n dir-iterator.h                 |   3 +\n refs.c                         |  27 ++++++---\n refs.h                         |  11 +++-\n refs/debug.c                   |   3 +-\n refs/files-backend.c           |  55 +++--------------\n refs/iterator.c                |  69 +++++++++++++++------\n refs/packed-backend.c          |   2 +-\n refs/ref-cache.c               |   2 +-\n refs/refs-internal.h           |  27 ++++-----\n refs/reftable-backend.c        |  67 +++-----------------\n revision.c                     |   4 +-\n t/helper/test-ref-store.c      |  18 ++++--\n t/t0600-reffiles-backend.sh    |  24 ++++----\n t/t1405-main-ref-store.sh      |   8 +--\n t/t1406-submodule-ref-store.sh |   8 +--\n t/t1410-reflog.sh              | 108 +++++++++++++++++++++++++++++++++\n 20 files changed, 380 insertions(+), 205 deletions(-)\n\nRange-diff against v2:\n1:  12de25dfe2 = 1:  d474f9cf77 dir-iterator: pass name to `prepare_next_entry_data()` directly\n2:  788afce189 = 2:  89cf960d47 dir-iterator: support iteration in sorted order\n3:  32b24a3d4b ! 3:  8ad63eb3f6 refs/files: sort reflogs returned by the reflog iterator\n    @@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r\n      \titer->dir_iterator = diter;\n      \titer->ref_store = ref_store;\n      \tstrbuf_release(&sb);\n    -@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n    - \t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n    - \t} else {\n    - \t\treturn merge_ref_iterator_begin(\n    --\t\t\t0, reflog_iterator_begin(ref_store, refs->base.gitdir),\n    -+\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n    - \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n    - \t\t\treflog_iterator_select, refs);\n    - \t}\n     \n      ## t/t0600-reffiles-backend.sh ##\n     @@ t/t0600-reffiles-backend.sh: test_expect_success 'for_each_reflog()' '\n-:  ---------- > 4:  0b52f6c4af refs/files: sort merged worktree and common reflogs\n4:  4254f23fd4 ! 5:  d44564c8b3 refs: always treat iterators as ordered\n    @@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(st\n     -\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n     +\t\t\treflog_iterator_begin(ref_store, refs->base.gitdir),\n      \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n    - \t\t\treflog_iterator_select, refs);\n    + \t\t\tref_iterator_select, refs);\n      \t}\n     \n      ## refs/iterator.c ##\n    @@ refs/refs-internal.h: enum do_for_each_ref_flags {\n      \tconst char *refname;\n      \tconst struct object_id *oid;\n      \tunsigned int flags;\n    -@@ refs/refs-internal.h: typedef enum iterator_selection ref_iterator_select_fn(\n    +@@ refs/refs-internal.h: enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree,\n       * Iterate over the entries from iter0 and iter1, with the values\n       * interleaved as directed by the select function. The iterator takes\n       * ownership of iter0 and iter1 and frees them when the iteration is\n    @@ refs/reftable-backend.c: static struct ref_iterator *reftable_be_iterator_begin(\n      \tworktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);\n     -\treturn merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,\n     +\treturn merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,\n    - \t\t\t\t\titerator_select, NULL);\n    + \t\t\t\t\tref_iterator_select, NULL);\n      }\n      \n     @@ refs/reftable-backend.c: static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n    @@ refs/reftable-backend.c: static struct ref_iterator *reftable_be_reflog_iterator\n      \n     -\treturn merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,\n     +\treturn merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,\n    - \t\t\t\t\titerator_select, NULL);\n    + \t\t\t\t\tref_iterator_select, NULL);\n      }\n      \n5:  240334df6c = 6:  c06fef8a64 refs: drop unused params from the reflog iterator callback\n6:  7928661318 = 7:  fc96d5bbab refs: stop resolving ref corresponding to reflogs\n7:  d7b9cff4c3 ! 8:  f3f50f3742 builtin/reflog: introduce subcommand to list reflogs\n    @@ t/t1410-reflog.sh: test_expect_success 'empty reflog' '\n     +\t)\n     +'\n     +\n    ++test_expect_success 'list reflogs with worktree' '\n    ++\ttest_when_finished \"rm -rf repo\" &&\n    ++\tgit init repo &&\n    ++\t(\n    ++\t\tcd repo &&\n    ++\n    ++\t\ttest_commit A &&\n    ++\t\tgit worktree add wt &&\n    ++\t\tgit -c core.logAllRefUpdates=always \\\n    ++\t\t\tupdate-ref refs/worktree/main HEAD &&\n    ++\t\tgit -c core.logAllRefUpdates=always \\\n    ++\t\t\tupdate-ref refs/worktree/per-worktree HEAD &&\n    ++\t\tgit -c core.logAllRefUpdates=always -C wt \\\n    ++\t\t\tupdate-ref refs/worktree/per-worktree HEAD &&\n    ++\t\tgit -c core.logAllRefUpdates=always -C wt \\\n    ++\t\t\tupdate-ref refs/worktree/worktree HEAD &&\n    ++\n    ++\t\tcat >expect <<-EOF &&\n    ++\t\tHEAD\n    ++\t\trefs/heads/main\n    ++\t\trefs/heads/wt\n    ++\t\trefs/worktree/main\n    ++\t\trefs/worktree/per-worktree\n    ++\t\tEOF\n    ++\t\tgit reflog list >actual &&\n    ++\t\ttest_cmp expect actual &&\n    ++\n    ++\t\tcat >expect <<-EOF &&\n    ++\t\tHEAD\n    ++\t\trefs/heads/main\n    ++\t\trefs/heads/wt\n    ++\t\trefs/worktree/per-worktree\n    ++\t\trefs/worktree/worktree\n    ++\t\tEOF\n    ++\t\tgit -C wt reflog list >actual &&\n    ++\t\ttest_cmp expect actual\n    ++\t)\n    ++'\n    ++\n     +test_expect_success 'reflog list returns error with additional args' '\n     +\tcat >expect <<-EOF &&\n     +\terror: list does not accept arguments: ${SQ}bogus${SQ}\n-- \n2.44.0-rc1\n\n"},{"id":"489069","messageId":"d474f9cf77232e0b37dd7312ed1a4eceafcc861f.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708518982.git.ps@pks.im","subject":"[PATCH v3 1/8] dir-iterator: pass name to `prepare_next_entry_data()` directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:19Z","receivedAt":"2024-02-21T12:37:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When adding the next directory entry for `struct dir_iterator` we pass\nthe complete `struct dirent *` to `prepare_next_entry_data()` even\nthough we only need the entry's name.\n\nRefactor the code to pass in the name, only. This prepares for a\nsubsequent commit where we introduce the ability to iterate through\ndir entries in an ordered manner.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n dir-iterator.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 278b04243a..f58a97e089 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -94,15 +94,15 @@ static int pop_level(struct dir_iterator_int *iter)\n \n /*\n  * Populate iter->base with the necessary information on the next iteration\n- * entry, represented by the given dirent de. Return 0 on success and -1\n+ * entry, represented by the given name. Return 0 on success and -1\n  * otherwise, setting errno accordingly.\n  */\n static int prepare_next_entry_data(struct dir_iterator_int *iter,\n-\t\t\t\t   struct dirent *de)\n+\t\t\t\t   const char *name)\n {\n \tint err, saved_errno;\n \n-\tstrbuf_addstr(&iter->base.path, de->d_name);\n+\tstrbuf_addstr(&iter->base.path, name);\n \t/*\n \t * We have to reset these because the path strbuf might have\n \t * been realloc()ed at the previous strbuf_addstr().\n@@ -159,7 +159,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n-\t\tif (prepare_next_entry_data(iter, de)) {\n+\t\tif (prepare_next_entry_data(iter, de->d_name)) {\n \t\t\tif (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n \t\t\t\tgoto error_out;\n \t\t\tcontinue;\n-- \n2.44.0-rc1\n\n"},{"id":"489070","messageId":"89cf960d47026cc1a1527e35b1c069c6598ac3e0.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708518982.git.ps@pks.im","subject":"[PATCH v3 2/8] dir-iterator: support iteration in sorted order","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:23Z","receivedAt":"2024-02-21T12:37:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `struct dir_iterator` is a helper that allows us to iterate through\ndirectory entries. This iterator returns entries in the exact same order\nas readdir(3P) does -- or in other words, it guarantees no specific\norder at all.\n\nThis is about to become problematic as we are introducing a new reflog\nsubcommand to list reflogs. As the \"files\" backend uses the directory\niterator to enumerate reflogs, returning reflog names and exposing them\nto the user would inherit the indeterministic ordering. Naturally, it\nwould make for a terrible user interface to show a list with no\ndiscernible order.\n\nWhile this could be handled at a higher level by the new subcommand\nitself by collecting and ordering the reflogs, this would be inefficient\nbecause we would first have to collect all reflogs before we can sort\nthem, which would introduce additional latency when there are many\nreflogs.\n\nInstead, introduce a new option into the directory iterator that asks\nfor its entries to be yielded in lexicographical order. If set, the\niterator will read all directory entries greedily and sort them before\nwe start to iterate over them.\n\nWhile this will of course also incur overhead as we cannot yield the\ndirectory entries immediately, it should at least be more efficient than\nhaving to sort the complete list of reflogs as we only need to sort one\ndirectory at a time.\n\nThis functionality will be used in a follow-up commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n dir-iterator.c | 99 +++++++++++++++++++++++++++++++++++++++++++-------\n dir-iterator.h |  3 ++\n 2 files changed, 89 insertions(+), 13 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex f58a97e089..de619846f2 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -2,10 +2,19 @@\n #include \"dir.h\"\n #include \"iterator.h\"\n #include \"dir-iterator.h\"\n+#include \"string-list.h\"\n \n struct dir_iterator_level {\n \tDIR *dir;\n \n+\t/*\n+\t * The directory entries of the current level. This list will only be\n+\t * populated when the iterator is ordered. In that case, `dir` will be\n+\t * set to `NULL`.\n+\t */\n+\tstruct string_list entries;\n+\tsize_t entries_idx;\n+\n \t/*\n \t * The length of the directory part of path at this level\n \t * (including a trailing '/'):\n@@ -43,6 +52,31 @@ struct dir_iterator_int {\n \tunsigned int flags;\n };\n \n+static int next_directory_entry(DIR *dir, const char *path,\n+\t\t\t\tstruct dirent **out)\n+{\n+\tstruct dirent *de;\n+\n+repeat:\n+\terrno = 0;\n+\tde = readdir(dir);\n+\tif (!de) {\n+\t\tif (errno) {\n+\t\t\twarning_errno(\"error reading directory '%s'\",\n+\t\t\t\t      path);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\treturn 1;\n+\t}\n+\n+\tif (is_dot_or_dotdot(de->d_name))\n+\t\tgoto repeat;\n+\n+\t*out = de;\n+\treturn 0;\n+}\n+\n /*\n  * Push a level in the iter stack and initialize it with information from\n  * the directory pointed by iter->base->path. It is assumed that this\n@@ -72,6 +106,35 @@ static int push_level(struct dir_iterator_int *iter)\n \t\treturn -1;\n \t}\n \n+\tstring_list_init_dup(&level->entries);\n+\tlevel->entries_idx = 0;\n+\n+\t/*\n+\t * When the iterator is sorted we read and sort all directory entries\n+\t * directly.\n+\t */\n+\tif (iter->flags & DIR_ITERATOR_SORTED) {\n+\t\tstruct dirent *de;\n+\n+\t\twhile (1) {\n+\t\t\tint ret = next_directory_entry(level->dir, iter->base.path.buf, &de);\n+\t\t\tif (ret < 0) {\n+\t\t\t\tif (errno != ENOENT &&\n+\t\t\t\t    iter->flags & DIR_ITERATOR_PEDANTIC)\n+\t\t\t\t\treturn -1;\n+\t\t\t\tcontinue;\n+\t\t\t} else if (ret > 0) {\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tstring_list_append(&level->entries, de->d_name);\n+\t\t}\n+\t\tstring_list_sort(&level->entries);\n+\n+\t\tclosedir(level->dir);\n+\t\tlevel->dir = NULL;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -88,6 +151,7 @@ static int pop_level(struct dir_iterator_int *iter)\n \t\twarning_errno(\"error closing directory '%s'\",\n \t\t\t      iter->base.path.buf);\n \tlevel->dir = NULL;\n+\tstring_list_clear(&level->entries, 0);\n \n \treturn --iter->levels_nr;\n }\n@@ -139,27 +203,34 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator)\n \t\tstruct dirent *de;\n \t\tstruct dir_iterator_level *level =\n \t\t\t&iter->levels[iter->levels_nr - 1];\n+\t\tconst char *name;\n \n \t\tstrbuf_setlen(&iter->base.path, level->prefix_len);\n-\t\terrno = 0;\n-\t\tde = readdir(level->dir);\n \n-\t\tif (!de) {\n-\t\t\tif (errno) {\n-\t\t\t\twarning_errno(\"error reading directory '%s'\",\n-\t\t\t\t\t      iter->base.path.buf);\n+\t\tif (level->dir) {\n+\t\t\tint ret = next_directory_entry(level->dir, iter->base.path.buf, &de);\n+\t\t\tif (ret < 0) {\n \t\t\t\tif (iter->flags & DIR_ITERATOR_PEDANTIC)\n \t\t\t\t\tgoto error_out;\n-\t\t\t} else if (pop_level(iter) == 0) {\n-\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\t\tcontinue;\n+\t\t\t} else if (ret > 0) {\n+\t\t\t\tif (pop_level(iter) == 0)\n+\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n \n-\t\tif (is_dot_or_dotdot(de->d_name))\n-\t\t\tcontinue;\n+\t\t\tname = de->d_name;\n+\t\t} else {\n+\t\t\tif (level->entries_idx >= level->entries.nr) {\n+\t\t\t\tif (pop_level(iter) == 0)\n+\t\t\t\t\treturn dir_iterator_abort(dir_iterator);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \n-\t\tif (prepare_next_entry_data(iter, de->d_name)) {\n+\t\t\tname = level->entries.items[level->entries_idx++].string;\n+\t\t}\n+\n+\t\tif (prepare_next_entry_data(iter, name)) {\n \t\t\tif (errno != ENOENT && iter->flags & DIR_ITERATOR_PEDANTIC)\n \t\t\t\tgoto error_out;\n \t\t\tcontinue;\n@@ -188,6 +259,8 @@ int dir_iterator_abort(struct dir_iterator *dir_iterator)\n \t\t\twarning_errno(\"error closing directory '%s'\",\n \t\t\t\t      iter->base.path.buf);\n \t\t}\n+\n+\t\tstring_list_clear(&level->entries, 0);\n \t}\n \n \tfree(iter->levels);\ndiff --git a/dir-iterator.h b/dir-iterator.h\nindex 479e1ec784..6d438809b6 100644\n--- a/dir-iterator.h\n+++ b/dir-iterator.h\n@@ -54,8 +54,11 @@\n  *   and ITER_ERROR is returned immediately. In both cases, a meaningful\n  *   warning is emitted. Note: ENOENT errors are always ignored so that\n  *   the API users may remove files during iteration.\n+ *\n+ * - DIR_ITERATOR_SORTED: sort directory entries alphabetically.\n  */\n #define DIR_ITERATOR_PEDANTIC (1 << 0)\n+#define DIR_ITERATOR_SORTED   (1 << 1)\n \n struct dir_iterator {\n \t/* The current path: */\n-- \n2.44.0-rc1\n\n"},{"id":"489071","messageId":"8ad63eb3f6a9819db57cd4523297cda6e4ead341.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708518982.git.ps@pks.im","subject":"[PATCH v3 3/8] refs/files: sort reflogs returned by the reflog iterator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:27Z","receivedAt":"2024-02-21T12:37:31Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We use a directory iterator to return reflogs via the reflog iterator.\nThis iterator returns entries in the same order as readdir(3P) would and\nwill thus yield reflogs with no discernible order.\n\nSet the new `DIR_ITERATOR_SORTED` flag that was introduced in the\npreceding commit so that the order is deterministic. While the effect of\nthis can only been observed in a test tool, a subsequent commit will\nstart to expose this functionality to users via a new `git reflog list`\nsubcommand.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/files-backend.c           | 4 ++--\n t/t0600-reffiles-backend.sh    | 4 ++--\n t/t1405-main-ref-store.sh      | 2 +-\n t/t1406-submodule-ref-store.sh | 2 +-\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 75dcc21ecb..2ffc63185f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \n \tstrbuf_addf(&sb, \"%s/logs\", gitdir);\n \n-\tditer = dir_iterator_begin(sb.buf, 0);\n+\tditer = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED);\n \tif (!diter) {\n \t\tstrbuf_release(&sb);\n \t\treturn empty_ref_iterator_begin();\n@@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n+\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);\n \titer->dir_iterator = diter;\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\ndiff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh\nindex e6a5f1868f..4f860285cc 100755\n--- a/t/t0600-reffiles-backend.sh\n+++ b/t/t0600-reffiles-backend.sh\n@@ -287,7 +287,7 @@ test_expect_success 'for_each_reflog()' '\n \tmkdir -p     .git/worktrees/wt/logs/refs/bisect &&\n \techo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&\n \n-\t$RWT for-each-reflog | cut -d\" \" -f 2- | sort >actual &&\n+\t$RWT for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \tPSEUDO-WT 0x0\n@@ -297,7 +297,7 @@ test_expect_success 'for_each_reflog()' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\t$RMAIN for-each-reflog | cut -d\" \" -f 2- | sort >actual &&\n+\t$RMAIN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \tPSEUDO-MAIN 0x0\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex 976bd71efb..cfb583f544 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -74,7 +74,7 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | sort -k2 | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \trefs/heads/main 0x0\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex e6a7f7334b..40332e23cc 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -63,7 +63,7 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | sort | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n \tcat >expected <<-\\EOF &&\n \tHEAD 0x1\n \trefs/heads/main 0x0\n-- \n2.44.0-rc1\n\n"},{"id":"489072","messageId":"0b52f6c4afea258133b4553e8c40fffe2143e6c6.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708518982.git.ps@pks.im","subject":"[PATCH v3 4/8] refs/files: sort merged worktree and common reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:31Z","receivedAt":"2024-02-21T12:37:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When iterating through reflogs in a worktree we create a merged iterator\nthat merges reflogs from both refdbs. The resulting refs are ordered so\nthat instead we first return all worktree reflogs before we return all\ncommon refs.\n\nThis is the only remaining case where a ref iterator returns entries in\na non-lexicographic order. The result would look something like the\nfollowing (listed with a command we introduce in a subsequent commit):\n\n```\n$ git reflog list\nHEAD\nrefs/worktree/per-worktree\nrefs/heads/main\nrefs/heads/wt\n```\n\nSo we first print the per-worktree reflogs in lexicographic order, then\nthe common reflogs in lexicographic order. This is confusing and not\nconsistent with how we print per-worktree refs, which are exclusively\nsorted lexicographically.\n\nSort reflogs lexicographically in the same way as we sort normal refs.\nAs this is already implemented properly by the \"reftable\" backend via a\nseparate selection function, we simply pull out that logic and reuse it\nfor the \"files\" backend. As logs are properly sorted now, mark the\nmerged reflog iterator as sorted.\n\nTests will be added in a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/files-backend.c    | 30 ++------------------------\n refs/iterator.c         | 43 +++++++++++++++++++++++++++++++++++++\n refs/refs-internal.h    |  9 ++++++++\n refs/reftable-backend.c | 47 ++---------------------------------------\n 4 files changed, 56 insertions(+), 73 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2ffc63185f..551cafdf76 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2210,32 +2210,6 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \treturn ref_iterator;\n }\n \n-static enum iterator_selection reflog_iterator_select(\n-\tstruct ref_iterator *iter_worktree,\n-\tstruct ref_iterator *iter_common,\n-\tvoid *cb_data UNUSED)\n-{\n-\tif (iter_worktree) {\n-\t\t/*\n-\t\t * We're a bit loose here. We probably should ignore\n-\t\t * common refs if they are accidentally added as\n-\t\t * per-worktree refs.\n-\t\t */\n-\t\treturn ITER_SELECT_0;\n-\t} else if (iter_common) {\n-\t\tif (parse_worktree_ref(iter_common->refname, NULL, NULL,\n-\t\t\t\t       NULL) == REF_WORKTREE_SHARED)\n-\t\t\treturn ITER_SELECT_1;\n-\n-\t\t/*\n-\t\t * The main ref store may contain main worktree's\n-\t\t * per-worktree refs, which should be ignored\n-\t\t */\n-\t\treturn ITER_SKIP_1;\n-\t} else\n-\t\treturn ITER_DONE;\n-}\n-\n static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)\n {\n \tstruct files_ref_store *refs =\n@@ -2246,9 +2220,9 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n \t} else {\n \t\treturn merge_ref_iterator_begin(\n-\t\t\t0, reflog_iterator_begin(ref_store, refs->base.gitdir),\n+\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n-\t\t\treflog_iterator_select, refs);\n+\t\t\tref_iterator_select, refs);\n \t}\n }\n \ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex 6b680f610e..b7ab5dc92f 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -98,6 +98,49 @@ struct merge_ref_iterator {\n \tstruct ref_iterator **current;\n };\n \n+enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree,\n+\t\t\t\t\t    struct ref_iterator *iter_common,\n+\t\t\t\t\t    void *cb_data UNUSED)\n+{\n+\tif (iter_worktree && !iter_common) {\n+\t\t/*\n+\t\t * Return the worktree ref if there are no more common refs.\n+\t\t */\n+\t\treturn ITER_SELECT_0;\n+\t} else if (iter_common) {\n+\t\t/*\n+\t\t * In case we have pending worktree and common refs we need to\n+\t\t * yield them based on their lexicographical order. Worktree\n+\t\t * refs that have the same name as common refs shadow the\n+\t\t * latter.\n+\t\t */\n+\t\tif (iter_worktree) {\n+\t\t\tint cmp = strcmp(iter_worktree->refname,\n+\t\t\t\t\t iter_common->refname);\n+\t\t\tif (cmp < 0)\n+\t\t\t\treturn ITER_SELECT_0;\n+\t\t\telse if (!cmp)\n+\t\t\t\treturn ITER_SELECT_0_SKIP_1;\n+\t\t}\n+\n+\t\t /*\n+\t\t  * We now know that the lexicographically-next ref is a common\n+\t\t  * ref. When the common ref is a shared one we return it.\n+\t\t  */\n+\t\tif (parse_worktree_ref(iter_common->refname, NULL, NULL,\n+\t\t\t\t       NULL) == REF_WORKTREE_SHARED)\n+\t\t\treturn ITER_SELECT_1;\n+\n+\t\t/*\n+\t\t * Otherwise, if the common ref is a per-worktree ref we skip\n+\t\t * it because it would belong to the main worktree, not ours.\n+\t\t */\n+\t\treturn ITER_SKIP_1;\n+\t} else {\n+\t\treturn ITER_DONE;\n+\t}\n+}\n+\n static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator)\n {\n \tstruct merge_ref_iterator *iter =\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 83e0f0bba3..51f612e122 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -386,6 +386,15 @@ typedef enum iterator_selection ref_iterator_select_fn(\n \t\tstruct ref_iterator *iter0, struct ref_iterator *iter1,\n \t\tvoid *cb_data);\n \n+/*\n+ * An implementation of ref_iterator_select_fn that merges worktree and common\n+ * refs. Per-worktree refs from the common iterator are ignored, worktree refs\n+ * override common refs. Refs are selected lexicographically.\n+ */\n+enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree,\n+\t\t\t\t\t    struct ref_iterator *iter_common,\n+\t\t\t\t\t    void *cb_data);\n+\n /*\n  * Iterate over the entries from iter0 and iter1, with the values\n  * interleaved as directed by the select function. The iterator takes\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex a14f2ad7f4..68d32a9101 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -504,49 +504,6 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_\n \treturn iter;\n }\n \n-static enum iterator_selection iterator_select(struct ref_iterator *iter_worktree,\n-\t\t\t\t\t       struct ref_iterator *iter_common,\n-\t\t\t\t\t       void *cb_data UNUSED)\n-{\n-\tif (iter_worktree && !iter_common) {\n-\t\t/*\n-\t\t * Return the worktree ref if there are no more common refs.\n-\t\t */\n-\t\treturn ITER_SELECT_0;\n-\t} else if (iter_common) {\n-\t\t/*\n-\t\t * In case we have pending worktree and common refs we need to\n-\t\t * yield them based on their lexicographical order. Worktree\n-\t\t * refs that have the same name as common refs shadow the\n-\t\t * latter.\n-\t\t */\n-\t\tif (iter_worktree) {\n-\t\t\tint cmp = strcmp(iter_worktree->refname,\n-\t\t\t\t\t iter_common->refname);\n-\t\t\tif (cmp < 0)\n-\t\t\t\treturn ITER_SELECT_0;\n-\t\t\telse if (!cmp)\n-\t\t\t\treturn ITER_SELECT_0_SKIP_1;\n-\t\t}\n-\n-\t\t /*\n-\t\t  * We now know that the lexicographically-next ref is a common\n-\t\t  * ref. When the common ref is a shared one we return it.\n-\t\t  */\n-\t\tif (parse_worktree_ref(iter_common->refname, NULL, NULL,\n-\t\t\t\t       NULL) == REF_WORKTREE_SHARED)\n-\t\t\treturn ITER_SELECT_1;\n-\n-\t\t/*\n-\t\t * Otherwise, if the common ref is a per-worktree ref we skip\n-\t\t * it because it would belong to the main worktree, not ours.\n-\t\t */\n-\t\treturn ITER_SKIP_1;\n-\t} else {\n-\t\treturn ITER_DONE;\n-\t}\n-}\n-\n static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_store,\n \t\t\t\t\t\t       const char *prefix,\n \t\t\t\t\t\t       const char **exclude_patterns,\n@@ -576,7 +533,7 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto\n \t */\n \tworktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);\n \treturn merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,\n-\t\t\t\t\titerator_select, NULL);\n+\t\t\t\t\tref_iterator_select, NULL);\n }\n \n static int reftable_be_read_raw_ref(struct ref_store *ref_store,\n@@ -1759,7 +1716,7 @@ static struct ref_iterator *reftable_be_reflog_iterator_begin(struct ref_store *\n \tworktree_iter = reflog_iterator_for_stack(refs, refs->worktree_stack);\n \n \treturn merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,\n-\t\t\t\t\titerator_select, NULL);\n+\t\t\t\t\tref_iterator_select, NULL);\n }\n \n static int yield_log_record(struct reftable_log_record *log,\n-- \n2.44.0-rc1\n\n"},{"id":"489073","messageId":"d44564c8b3959e5f54c92f925afef67c71615820.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708518982.git.ps@pks.im","subject":"[PATCH v3 5/8] refs: always treat iterators as ordered","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:35Z","receivedAt":"2024-02-21T12:37:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commit we have converted the reflog iterator of the\n\"files\" backend to be ordered, which was the only remaining ref iterator\nthat wasn't ordered. Refactor the ref iterator infrastructure so that we\nalways assume iterators to be ordered, thus simplifying the code.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c                  |  4 ----\n refs/debug.c            |  3 +--\n refs/files-backend.c    |  7 +++----\n refs/iterator.c         | 26 ++++++++------------------\n refs/packed-backend.c   |  2 +-\n refs/ref-cache.c        |  2 +-\n refs/refs-internal.h    | 18 ++----------------\n refs/reftable-backend.c |  8 ++++----\n 8 files changed, 20 insertions(+), 50 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fff343c256..dc25606a82 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1594,10 +1594,6 @@ struct ref_iterator *refs_ref_iterator_begin(\n \tif (trim)\n \t\titer = prefix_ref_iterator_begin(iter, \"\", trim);\n \n-\t/* Sanity check for subclasses: */\n-\tif (!iter->ordered)\n-\t\tBUG(\"reference iterator is not ordered\");\n-\n \treturn iter;\n }\n \ndiff --git a/refs/debug.c b/refs/debug.c\nindex 634681ca44..c7531b17f0 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -181,7 +181,6 @@ static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t\ttrace_printf_key(&trace_refs, \"iterator_advance: %s (0)\\n\",\n \t\t\tditer->iter->refname);\n \n-\tditer->base.ordered = diter->iter->ordered;\n \tditer->base.refname = diter->iter->refname;\n \tditer->base.oid = diter->iter->oid;\n \tditer->base.flags = diter->iter->flags;\n@@ -222,7 +221,7 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix,\n \t\tdrefs->refs->be->iterator_begin(drefs->refs, prefix,\n \t\t\t\t\t\texclude_patterns, flags);\n \tstruct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter));\n-\tbase_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable);\n \tditer->iter = res;\n \ttrace_printf_key(&trace_refs, \"ref_iterator_begin: \\\"%s\\\" (0x%x)\\n\",\n \t\t\t prefix, flags);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 551cafdf76..05bb0c875c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -879,8 +879,7 @@ static struct ref_iterator *files_ref_iterator_begin(\n \n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable,\n-\t\t\t       overlay_iter->ordered);\n+\tbase_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable);\n \titer->iter0 = overlay_iter;\n \titer->repo = ref_store->repo;\n \titer->flags = flags;\n@@ -2202,7 +2201,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,\n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1);\n+\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);\n \titer->dir_iterator = diter;\n \titer->ref_store = ref_store;\n \tstrbuf_release(&sb);\n@@ -2220,7 +2219,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \t\treturn reflog_iterator_begin(ref_store, refs->gitcommondir);\n \t} else {\n \t\treturn merge_ref_iterator_begin(\n-\t\t\t1, reflog_iterator_begin(ref_store, refs->base.gitdir),\n+\t\t\treflog_iterator_begin(ref_store, refs->base.gitdir),\n \t\t\treflog_iterator_begin(ref_store, refs->gitcommondir),\n \t\t\tref_iterator_select, refs);\n \t}\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex b7ab5dc92f..9db8b056d5 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -25,11 +25,9 @@ int ref_iterator_abort(struct ref_iterator *ref_iterator)\n }\n \n void base_ref_iterator_init(struct ref_iterator *iter,\n-\t\t\t    struct ref_iterator_vtable *vtable,\n-\t\t\t    int ordered)\n+\t\t\t    struct ref_iterator_vtable *vtable)\n {\n \titer->vtable = vtable;\n-\titer->ordered = !!ordered;\n \titer->refname = NULL;\n \titer->oid = NULL;\n \titer->flags = 0;\n@@ -74,7 +72,7 @@ struct ref_iterator *empty_ref_iterator_begin(void)\n \tstruct empty_ref_iterator *iter = xcalloc(1, sizeof(*iter));\n \tstruct ref_iterator *ref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable);\n \treturn ref_iterator;\n }\n \n@@ -250,7 +248,6 @@ static struct ref_iterator_vtable merge_ref_iterator_vtable = {\n };\n \n struct ref_iterator *merge_ref_iterator_begin(\n-\t\tint ordered,\n \t\tstruct ref_iterator *iter0, struct ref_iterator *iter1,\n \t\tref_iterator_select_fn *select, void *cb_data)\n {\n@@ -265,7 +262,7 @@ struct ref_iterator *merge_ref_iterator_begin(\n \t * references through only if they exist in both iterators.\n \t */\n \n-\tbase_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable, ordered);\n+\tbase_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable);\n \titer->iter0 = iter0;\n \titer->iter1 = iter1;\n \titer->select = select;\n@@ -314,12 +311,9 @@ struct ref_iterator *overlay_ref_iterator_begin(\n \t} else if (is_empty_ref_iterator(back)) {\n \t\tref_iterator_abort(back);\n \t\treturn front;\n-\t} else if (!front->ordered || !back->ordered) {\n-\t\tBUG(\"overlay_ref_iterator requires ordered inputs\");\n \t}\n \n-\treturn merge_ref_iterator_begin(1, front, back,\n-\t\t\t\t\toverlay_iterator_select, NULL);\n+\treturn merge_ref_iterator_begin(front, back, overlay_iterator_select, NULL);\n }\n \n struct prefix_ref_iterator {\n@@ -358,16 +352,12 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \n \t\tif (cmp > 0) {\n \t\t\t/*\n-\t\t\t * If the source iterator is ordered, then we\n+\t\t\t * As the source iterator is ordered, we\n \t\t\t * can stop the iteration as soon as we see a\n \t\t\t * refname that comes after the prefix:\n \t\t\t */\n-\t\t\tif (iter->iter0->ordered) {\n-\t\t\t\tok = ref_iterator_abort(iter->iter0);\n-\t\t\t\tbreak;\n-\t\t\t} else {\n-\t\t\t\tcontinue;\n-\t\t\t}\n+\t\t\tok = ref_iterator_abort(iter->iter0);\n+\t\t\tbreak;\n \t\t}\n \n \t\tif (iter->trim) {\n@@ -439,7 +429,7 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable, iter0->ordered);\n+\tbase_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable);\n \n \titer->iter0 = iter0;\n \titer->prefix = xstrdup(prefix);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a499a91c7e..4e826c05ff 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1111,7 +1111,7 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);\n \n \tif (exclude_patterns)\n \t\tpopulate_excluded_jump_list(iter, snapshot, exclude_patterns);\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex a372a00941..9f9797209a 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -486,7 +486,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n \n \tCALLOC_ARRAY(iter, 1);\n \tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable);\n \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n \n \titer->levels_nr = 1;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 51f612e122..a9b6e887f8 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -312,13 +312,6 @@ enum do_for_each_ref_flags {\n  */\n struct ref_iterator {\n \tstruct ref_iterator_vtable *vtable;\n-\n-\t/*\n-\t * Does this `ref_iterator` iterate over references in order\n-\t * by refname?\n-\t */\n-\tunsigned int ordered : 1;\n-\n \tconst char *refname;\n \tconst struct object_id *oid;\n \tunsigned int flags;\n@@ -399,11 +392,9 @@ enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree,\n  * Iterate over the entries from iter0 and iter1, with the values\n  * interleaved as directed by the select function. The iterator takes\n  * ownership of iter0 and iter1 and frees them when the iteration is\n- * over. A derived class should set `ordered` to 1 or 0 based on\n- * whether it generates its output in order by reference name.\n+ * over.\n  */\n struct ref_iterator *merge_ref_iterator_begin(\n-\t\tint ordered,\n \t\tstruct ref_iterator *iter0, struct ref_iterator *iter1,\n \t\tref_iterator_select_fn *select, void *cb_data);\n \n@@ -432,8 +423,6 @@ struct ref_iterator *overlay_ref_iterator_begin(\n  * As an convenience to callers, if prefix is the empty string and\n  * trim is zero, this function returns iter0 directly, without\n  * wrapping it.\n- *\n- * The resulting ref_iterator is ordered if iter0 is.\n  */\n struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \t\t\t\t\t       const char *prefix,\n@@ -444,14 +433,11 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n /*\n  * Base class constructor for ref_iterators. Initialize the\n  * ref_iterator part of iter, setting its vtable pointer as specified.\n- * `ordered` should be set to 1 if the iterator will iterate over\n- * references in order by refname; otherwise it should be set to 0.\n  * This is meant to be called only by the initializers of derived\n  * classes.\n  */\n void base_ref_iterator_init(struct ref_iterator *iter,\n-\t\t\t    struct ref_iterator_vtable *vtable,\n-\t\t\t    int ordered);\n+\t\t\t    struct ref_iterator_vtable *vtable);\n \n /*\n  * Base class destructor for ref_iterators. Destroy the ref_iterator\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 68d32a9101..39e9a9d4e2 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -479,7 +479,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_\n \tint ret;\n \n \titer = xcalloc(1, sizeof(*iter));\n-\tbase_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable, 1);\n+\tbase_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable);\n \titer->prefix = prefix;\n \titer->base.oid = &iter->oid;\n \titer->flags = flags;\n@@ -532,7 +532,7 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto\n \t * single iterator.\n \t */\n \tworktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags);\n-\treturn merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,\n+\treturn merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,\n \t\t\t\t\tref_iterator_select, NULL);\n }\n \n@@ -1680,7 +1680,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n \tint ret;\n \n \titer = xcalloc(1, sizeof(*iter));\n-\tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);\n+\tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);\n \titer->refs = refs;\n \titer->base.oid = &iter->oid;\n \n@@ -1715,7 +1715,7 @@ static struct ref_iterator *reftable_be_reflog_iterator_begin(struct ref_store *\n \n \tworktree_iter = reflog_iterator_for_stack(refs, refs->worktree_stack);\n \n-\treturn merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base,\n+\treturn merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base,\n \t\t\t\t\tref_iterator_select, NULL);\n }\n \n-- \n2.44.0-rc1\n\n"},{"id":"489074","messageId":"c06fef8a64759affaafbce35afb54357fd0487b4.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708518982.git.ps@pks.im","subject":"[PATCH v3 6/8] refs: drop unused params from the reflog iterator callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:39Z","receivedAt":"2024-02-21T12:37:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The ref and reflog iterators share much of the same underlying code to\niterate over the corresponding entries. This results in some weird code\nbecause the reflog iterator also exposes an object ID as well as a flag\nto the callback function. Neither of these fields do refer to the reflog\nthough -- they refer to the corresponding ref with the same name. This\nis quite misleading. In practice at least the object ID cannot really be\nimplemented in any other way as a reflog does not have a specific object\nID in the first place. This is further stressed by the fact that none of\nthe callbacks except for our test helper make use of these fields.\n\nSplit up the infrastucture so that ref and reflog iterators use separate\ncallback signatures. This allows us to drop the nonsensical fields from\nthe reflog iterator.\n\nNote that internally, the backends still use the same shared infra to\niterate over both types. As the backends should never end up being\ncalled directly anyway, this is not much of a problem and thus kept\nas-is for simplicity's sake.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fsck.c                 |  4 +---\n builtin/reflog.c               |  3 +--\n refs.c                         | 23 +++++++++++++++++++----\n refs.h                         | 11 +++++++++--\n refs/files-backend.c           |  8 +-------\n refs/reftable-backend.c        |  8 +-------\n revision.c                     |  4 +---\n t/helper/test-ref-store.c      | 18 ++++++++++++------\n t/t0600-reffiles-backend.sh    | 24 ++++++++++++------------\n t/t1405-main-ref-store.sh      |  8 ++++----\n t/t1406-submodule-ref-store.sh |  8 ++++----\n 11 files changed, 65 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex a7cf94f67e..f892487c9b 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -509,9 +509,7 @@ static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid\n \treturn 0;\n }\n \n-static int fsck_handle_reflog(const char *logname,\n-\t\t\t      const struct object_id *oid UNUSED,\n-\t\t\t      int flag UNUSED, void *cb_data)\n+static int fsck_handle_reflog(const char *logname, void *cb_data)\n {\n \tstruct strbuf refname = STRBUF_INIT;\n \ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex a5a4099f61..3a0c4d4322 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -60,8 +60,7 @@ struct worktree_reflogs {\n \tstruct string_list reflogs;\n };\n \n-static int collect_reflog(const char *ref, const struct object_id *oid UNUSED,\n-\t\t\t  int flags UNUSED, void *cb_data)\n+static int collect_reflog(const char *ref, void *cb_data)\n {\n \tstruct worktree_reflogs *cb = cb_data;\n \tstruct worktree *worktree = cb->worktree;\ndiff --git a/refs.c b/refs.c\nindex dc25606a82..f9261267f0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2512,18 +2512,33 @@ int refs_verify_refname_available(struct ref_store *refs,\n \treturn ret;\n }\n \n-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n+struct do_for_each_reflog_help {\n+\teach_reflog_fn *fn;\n+\tvoid *cb_data;\n+};\n+\n+static int do_for_each_reflog_helper(struct repository *r UNUSED,\n+\t\t\t\t     const char *refname,\n+\t\t\t\t     const struct object_id *oid UNUSED,\n+\t\t\t\t     int flags,\n+\t\t\t\t     void *cb_data)\n+{\n+\tstruct do_for_each_reflog_help *hp = cb_data;\n+\treturn hp->fn(refname, hp->cb_data);\n+}\n+\n+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data)\n {\n \tstruct ref_iterator *iter;\n-\tstruct do_for_each_ref_help hp = { fn, cb_data };\n+\tstruct do_for_each_reflog_help hp = { fn, cb_data };\n \n \titer = refs->be->reflog_iterator_begin(refs);\n \n \treturn do_for_each_repo_ref_iterator(the_repository, iter,\n-\t\t\t\t\t     do_for_each_ref_helper, &hp);\n+\t\t\t\t\t     do_for_each_reflog_helper, &hp);\n }\n \n-int for_each_reflog(each_ref_fn fn, void *cb_data)\n+int for_each_reflog(each_reflog_fn fn, void *cb_data)\n {\n \treturn refs_for_each_reflog(get_main_ref_store(the_repository), fn, cb_data);\n }\ndiff --git a/refs.h b/refs.h\nindex 303c5fac4d..895579aeb7 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -534,12 +534,19 @@ int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_dat\n /* youngest entry first */\n int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data);\n \n+/*\n+ * The signature for the callback function for the {refs_,}for_each_reflog()\n+ * functions below. The memory pointed to by the refname argument is only\n+ * guaranteed to be valid for the duration of a single callback invocation.\n+ */\n+typedef int each_reflog_fn(const char *refname, void *cb_data);\n+\n /*\n  * Calls the specified function for each reflog file until it returns nonzero,\n  * and returns the value. Reflog file order is unspecified.\n  */\n-int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n-int for_each_reflog(each_ref_fn fn, void *cb_data);\n+int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data);\n+int for_each_reflog(each_reflog_fn fn, void *cb_data);\n \n #define REFNAME_ALLOW_ONELEVEL 1\n #define REFNAME_REFSPEC_PATTERN 2\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 05bb0c875c..c7aff6b331 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2115,10 +2115,8 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,\n \n struct files_reflog_iterator {\n \tstruct ref_iterator base;\n-\n \tstruct ref_store *ref_store;\n \tstruct dir_iterator *dir_iterator;\n-\tstruct object_id oid;\n };\n \n static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n@@ -2129,8 +2127,6 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \tint ok;\n \n \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n-\t\tint flags;\n-\n \t\tif (!S_ISREG(diter->st.st_mode))\n \t\t\tcontinue;\n \t\tif (diter->basename[0] == '.')\n@@ -2140,14 +2136,12 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \n \t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n \t\t\t\t\t     diter->relative_path, 0,\n-\t\t\t\t\t     &iter->oid, &flags)) {\n+\t\t\t\t\t     NULL, NULL)) {\n \t\t\terror(\"bad ref for %s\", diter->path.buf);\n \t\t\tcontinue;\n \t\t}\n \n \t\titer->base.refname = diter->relative_path;\n-\t\titer->base.oid = &iter->oid;\n-\t\titer->base.flags = flags;\n \t\treturn ITER_OK;\n \t}\n \ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 39e9a9d4e2..4998b676c2 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1594,7 +1594,6 @@ struct reftable_reflog_iterator {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_iterator iter;\n \tstruct reftable_log_record log;\n-\tstruct object_id oid;\n \tchar *last_name;\n \tint err;\n };\n@@ -1605,8 +1604,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t(struct reftable_reflog_iterator *)ref_iterator;\n \n \twhile (!iter->err) {\n-\t\tint flags;\n-\n \t\titer->err = reftable_iterator_next_log(&iter->iter, &iter->log);\n \t\tif (iter->err)\n \t\t\tbreak;\n@@ -1620,7 +1617,7 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t\tcontinue;\n \n \t\tif (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,\n-\t\t\t\t\t     0, &iter->oid, &flags)) {\n+\t\t\t\t\t     0, NULL, NULL)) {\n \t\t\terror(_(\"bad ref for %s\"), iter->log.refname);\n \t\t\tcontinue;\n \t\t}\n@@ -1628,8 +1625,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tfree(iter->last_name);\n \t\titer->last_name = xstrdup(iter->log.refname);\n \t\titer->base.refname = iter->log.refname;\n-\t\titer->base.oid = &iter->oid;\n-\t\titer->base.flags = flags;\n \n \t\tbreak;\n \t}\n@@ -1682,7 +1677,6 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n \titer = xcalloc(1, sizeof(*iter));\n \tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);\n \titer->refs = refs;\n-\titer->base.oid = &iter->oid;\n \n \tret = refs->err;\n \tif (ret)\ndiff --git a/revision.c b/revision.c\nindex 2424c9bd67..ac45c6d8f2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1686,9 +1686,7 @@ static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n \treturn 0;\n }\n \n-static int handle_one_reflog(const char *refname_in_wt,\n-\t\t\t     const struct object_id *oid UNUSED,\n-\t\t\t     int flag UNUSED, void *cb_data)\n+static int handle_one_reflog(const char *refname_in_wt, void *cb_data)\n {\n \tstruct all_refs_cb *cb = cb_data;\n \tstruct strbuf refname = STRBUF_INIT;\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 702ec1f128..7a0f6cac53 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -221,15 +221,21 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv)\n \treturn ret;\n }\n \n+static int each_reflog(const char *refname, void *cb_data UNUSED)\n+{\n+\tprintf(\"%s\\n\", refname);\n+\treturn 0;\n+}\n+\n static int cmd_for_each_reflog(struct ref_store *refs,\n \t\t\t       const char **argv UNUSED)\n {\n-\treturn refs_for_each_reflog(refs, each_ref, NULL);\n+\treturn refs_for_each_reflog(refs, each_reflog, NULL);\n }\n \n-static int each_reflog(struct object_id *old_oid, struct object_id *new_oid,\n-\t\t       const char *committer, timestamp_t timestamp,\n-\t\t       int tz, const char *msg, void *cb_data UNUSED)\n+static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,\n+\t\t\t   const char *committer, timestamp_t timestamp,\n+\t\t\t   int tz, const char *msg, void *cb_data UNUSED)\n {\n \tprintf(\"%s %s %s %\" PRItime \" %+05d%s%s\", oid_to_hex(old_oid),\n \t       oid_to_hex(new_oid), committer, timestamp, tz,\n@@ -241,14 +247,14 @@ static int cmd_for_each_reflog_ent(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n \n-\treturn refs_for_each_reflog_ent(refs, refname, each_reflog, refs);\n+\treturn refs_for_each_reflog_ent(refs, refname, each_reflog_ent, refs);\n }\n \n static int cmd_for_each_reflog_ent_reverse(struct ref_store *refs, const char **argv)\n {\n \tconst char *refname = notnull(*argv++, \"refname\");\n \n-\treturn refs_for_each_reflog_ent_reverse(refs, refname, each_reflog, refs);\n+\treturn refs_for_each_reflog_ent_reverse(refs, refname, each_reflog_ent, refs);\n }\n \n static int cmd_reflog_exists(struct ref_store *refs, const char **argv)\ndiff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh\nindex 4f860285cc..56a3196b83 100755\n--- a/t/t0600-reffiles-backend.sh\n+++ b/t/t0600-reffiles-backend.sh\n@@ -287,23 +287,23 @@ test_expect_success 'for_each_reflog()' '\n \tmkdir -p     .git/worktrees/wt/logs/refs/bisect &&\n \techo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&\n \n-\t$RWT for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RWT for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\tPSEUDO-WT 0x0\n-\trefs/bisect/wt-random 0x0\n-\trefs/heads/main 0x0\n-\trefs/heads/wt-main 0x0\n+\tHEAD\n+\tPSEUDO-WT\n+\trefs/bisect/wt-random\n+\trefs/heads/main\n+\trefs/heads/wt-main\n \tEOF\n \ttest_cmp expected actual &&\n \n-\t$RMAIN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RMAIN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\tPSEUDO-MAIN 0x0\n-\trefs/bisect/random 0x0\n-\trefs/heads/main 0x0\n-\trefs/heads/wt-main 0x0\n+\tHEAD\n+\tPSEUDO-MAIN\n+\trefs/bisect/random\n+\trefs/heads/main\n+\trefs/heads/wt-main\n \tEOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh\nindex cfb583f544..3eee758bce 100755\n--- a/t/t1405-main-ref-store.sh\n+++ b/t/t1405-main-ref-store.sh\n@@ -74,11 +74,11 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\trefs/heads/main 0x0\n-\trefs/heads/new-main 0x0\n+\tHEAD\n+\trefs/heads/main\n+\trefs/heads/new-main\n \tEOF\n \ttest_cmp expected actual\n '\ndiff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh\nindex 40332e23cc..c01f0f14a1 100755\n--- a/t/t1406-submodule-ref-store.sh\n+++ b/t/t1406-submodule-ref-store.sh\n@@ -63,11 +63,11 @@ test_expect_success 'verify_ref(new-main)' '\n '\n \n test_expect_success 'for_each_reflog()' '\n-\t$RUN for-each-reflog | cut -d\" \" -f 2- >actual &&\n+\t$RUN for-each-reflog >actual &&\n \tcat >expected <<-\\EOF &&\n-\tHEAD 0x1\n-\trefs/heads/main 0x0\n-\trefs/heads/new-main 0x0\n+\tHEAD\n+\trefs/heads/main\n+\trefs/heads/new-main\n \tEOF\n \ttest_cmp expected actual\n '\n-- \n2.44.0-rc1\n\n"},{"id":"489075","messageId":"fc96d5bbabe7986d08eb1645549b5591784521e4.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708518982.git.ps@pks.im","subject":"[PATCH v3 7/8] refs: stop resolving ref corresponding to reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:43Z","receivedAt":"2024-02-21T12:37:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reflog iterator tries to resolve the corresponding ref for every\nreflog that it is about to yield. Historically, this was done due to\nmultiple reasons:\n\n  - It ensures that the refname is safe because we end up calling\n    `check_refname_format()`. Also, non-conformant refnames are skipped\n    altogether.\n\n  - The iterator used to yield the resolved object ID as well as its\n    flags to the callback. This info was never used though, and the\n    corresponding parameters were dropped in the preceding commit.\n\n  - When a ref is corrupt then the reflog is not emitted at all.\n\nWe're about to introduce a new `git reflog list` subcommand that will\nprint all reflogs that the refdb knows about. Skipping over reflogs\nwhose refs are corrupted would be quite counterproductive in this case\nas the user would have no way to learn about reflogs which may still\nexist in their repository to help and rescue such a corrupted ref. Thus,\nthe only remaining reason for why we'd want to resolve the ref is to\nverify its refname.\n\nRefactor the code to call `check_refname_format()` directly instead of\ntrying to resolve the ref. This is significantly more efficient given\nthat we don't have to hit the object database anymore to list reflogs.\nAnd second, it ensures that we end up showing reflogs of broken refs,\nwhich will help to make the reflog more useful.\n\nNote that this really only impacts the case where the corresponding ref\nis corrupt. Reflogs for nonexistent refs would have been returned to the\ncaller beforehand already as we did not pass `RESOLVE_REF_READING` to\nthe function, and thus `refs_resolve_ref_unsafe()` would have returned\nsuccessfully in that case.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/files-backend.c    | 12 ++----------\n refs/reftable-backend.c |  6 ++----\n 2 files changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex c7aff6b331..6f98168a81 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2129,17 +2129,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \twhile ((ok = dir_iterator_advance(diter)) == ITER_OK) {\n \t\tif (!S_ISREG(diter->st.st_mode))\n \t\t\tcontinue;\n-\t\tif (diter->basename[0] == '.')\n+\t\tif (check_refname_format(diter->basename,\n+\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\t\tcontinue;\n-\t\tif (ends_with(diter->basename, \".lock\"))\n-\t\t\tcontinue;\n-\n-\t\tif (!refs_resolve_ref_unsafe(iter->ref_store,\n-\t\t\t\t\t     diter->relative_path, 0,\n-\t\t\t\t\t     NULL, NULL)) {\n-\t\t\terror(\"bad ref for %s\", diter->path.buf);\n-\t\t\tcontinue;\n-\t\t}\n \n \t\titer->base.refname = diter->relative_path;\n \t\treturn ITER_OK;\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 4998b676c2..6c11c4a5e3 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1616,11 +1616,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\tif (iter->last_name && !strcmp(iter->log.refname, iter->last_name))\n \t\t\tcontinue;\n \n-\t\tif (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname,\n-\t\t\t\t\t     0, NULL, NULL)) {\n-\t\t\terror(_(\"bad ref for %s\"), iter->log.refname);\n+\t\tif (check_refname_format(iter->log.refname,\n+\t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\t\tcontinue;\n-\t\t}\n \n \t\tfree(iter->last_name);\n \t\titer->last_name = xstrdup(iter->log.refname);\n-- \n2.44.0-rc1\n\n"},{"id":"489076","messageId":"f3f50f37429e39e07bcbbeeaf90d2aa18e279f5d.1708518982.git.ps@pks.im","threadId":"60955","inReplyTo":"cover.1708518982.git.ps@pks.im","subject":"[PATCH v3 8/8] builtin/reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-02-21T12:37:47Z","receivedAt":"2024-02-21T12:37:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While the git-reflog(1) command has subcommands to show reflog entries\nor check for reflog existence, it does not have any subcommands that\nwould allow the user to enumerate all existing reflogs. This makes it\nquite hard to discover which reflogs a repository has. While this can\nbe worked around with the \"files\" backend by enumerating files in the\n\".git/logs\" directory, users of the \"reftable\" backend don't enjoy such\na luxury.\n\nIntroduce a new subcommand `git reflog list` that lists all reflogs the\nrepository knows of to fill this gap.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/git-reflog.txt |   3 +\n builtin/reflog.c             |  34 +++++++++++\n t/t1410-reflog.sh            | 108 +++++++++++++++++++++++++++++++++++\n 3 files changed, 145 insertions(+)\n\ndiff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt\nindex ec64cbff4c..a929c52982 100644\n--- a/Documentation/git-reflog.txt\n+++ b/Documentation/git-reflog.txt\n@@ -10,6 +10,7 @@ SYNOPSIS\n --------\n [verse]\n 'git reflog' [show] [<log-options>] [<ref>]\n+'git reflog list'\n 'git reflog expire' [--expire=<time>] [--expire-unreachable=<time>]\n \t[--rewrite] [--updateref] [--stale-fix]\n \t[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]\n@@ -39,6 +40,8 @@ actions, and in addition the `HEAD` reflog records branch switching.\n `git reflog show` is an alias for `git log -g --abbrev-commit\n --pretty=oneline`; see linkgit:git-log[1] for more information.\n \n+The \"list\" subcommand lists all refs which have a corresponding reflog.\n+\n The \"expire\" subcommand prunes older reflog entries. Entries older\n than `expire` time, or entries older than `expire-unreachable` time\n and not reachable from the current tip, are removed from the reflog.\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 3a0c4d4322..63cd4d8b29 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -7,11 +7,15 @@\n #include \"wildmatch.h\"\n #include \"worktree.h\"\n #include \"reflog.h\"\n+#include \"refs.h\"\n #include \"parse-options.h\"\n \n #define BUILTIN_REFLOG_SHOW_USAGE \\\n \tN_(\"git reflog [show] [<log-options>] [<ref>]\")\n \n+#define BUILTIN_REFLOG_LIST_USAGE \\\n+\tN_(\"git reflog list\")\n+\n #define BUILTIN_REFLOG_EXPIRE_USAGE \\\n \tN_(\"git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\\n\" \\\n \t   \"                  [--rewrite] [--updateref] [--stale-fix]\\n\" \\\n@@ -29,6 +33,11 @@ static const char *const reflog_show_usage[] = {\n \tNULL,\n };\n \n+static const char *const reflog_list_usage[] = {\n+\tBUILTIN_REFLOG_LIST_USAGE,\n+\tNULL,\n+};\n+\n static const char *const reflog_expire_usage[] = {\n \tBUILTIN_REFLOG_EXPIRE_USAGE,\n \tNULL\n@@ -46,6 +55,7 @@ static const char *const reflog_exists_usage[] = {\n \n static const char *const reflog_usage[] = {\n \tBUILTIN_REFLOG_SHOW_USAGE,\n+\tBUILTIN_REFLOG_LIST_USAGE,\n \tBUILTIN_REFLOG_EXPIRE_USAGE,\n \tBUILTIN_REFLOG_DELETE_USAGE,\n \tBUILTIN_REFLOG_EXISTS_USAGE,\n@@ -238,6 +248,29 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)\n \treturn cmd_log_reflog(argc, argv, prefix);\n }\n \n+static int show_reflog(const char *refname, void *cb_data UNUSED)\n+{\n+\tprintf(\"%s\\n\", refname);\n+\treturn 0;\n+}\n+\n+static int cmd_reflog_list(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct option options[] = {\n+\t\tOPT_END()\n+\t};\n+\tstruct ref_store *ref_store;\n+\n+\targc = parse_options(argc, argv, prefix, options, reflog_list_usage, 0);\n+\tif (argc)\n+\t\treturn error(_(\"%s does not accept arguments: '%s'\"),\n+\t\t\t     \"list\", argv[0]);\n+\n+\tref_store = get_main_ref_store(the_repository);\n+\n+\treturn refs_for_each_reflog(ref_store, show_reflog, NULL);\n+}\n+\n static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n {\n \tstruct cmd_reflog_expire_cb cmd = { 0 };\n@@ -417,6 +450,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)\n \tparse_opt_subcommand_fn *fn = NULL;\n \tstruct option options[] = {\n \t\tOPT_SUBCOMMAND(\"show\", &fn, cmd_reflog_show),\n+\t\tOPT_SUBCOMMAND(\"list\", &fn, cmd_reflog_list),\n \t\tOPT_SUBCOMMAND(\"expire\", &fn, cmd_reflog_expire),\n \t\tOPT_SUBCOMMAND(\"delete\", &fn, cmd_reflog_delete),\n \t\tOPT_SUBCOMMAND(\"exists\", &fn, cmd_reflog_exists),\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex d2f5f42e67..5bf883f1e3 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -436,4 +436,112 @@ test_expect_success 'empty reflog' '\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'list reflogs' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit reflog list >actual &&\n+\t\ttest_must_be_empty actual &&\n+\n+\t\ttest_commit A &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tgit branch b &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/b\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'list reflogs with worktree' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\n+\t\ttest_commit A &&\n+\t\tgit worktree add wt &&\n+\t\tgit -c core.logAllRefUpdates=always \\\n+\t\t\tupdate-ref refs/worktree/main HEAD &&\n+\t\tgit -c core.logAllRefUpdates=always \\\n+\t\t\tupdate-ref refs/worktree/per-worktree HEAD &&\n+\t\tgit -c core.logAllRefUpdates=always -C wt \\\n+\t\t\tupdate-ref refs/worktree/per-worktree HEAD &&\n+\t\tgit -c core.logAllRefUpdates=always -C wt \\\n+\t\t\tupdate-ref refs/worktree/worktree HEAD &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\trefs/heads/wt\n+\t\trefs/worktree/main\n+\t\trefs/worktree/per-worktree\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\trefs/heads/wt\n+\t\trefs/worktree/per-worktree\n+\t\trefs/worktree/worktree\n+\t\tEOF\n+\t\tgit -C wt reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'reflog list returns error with additional args' '\n+\tcat >expect <<-EOF &&\n+\terror: list does not accept arguments: ${SQ}bogus${SQ}\n+\tEOF\n+\ttest_must_fail git reflog list bogus 2>err &&\n+\ttest_cmp expect err\n+'\n+\n+test_expect_success 'reflog for symref with unborn target can be listed' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit symbolic-ref HEAD refs/heads/unborn &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'reflog with invalid object ID can be listed' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\ttest-tool ref-store main update-ref msg refs/heads/missing \\\n+\t\t\t$(test_oid deadbeef) \"$ZERO_OID\" REF_SKIP_OID_VERIFICATION &&\n+\t\tcat >expect <<-EOF &&\n+\t\tHEAD\n+\t\trefs/heads/main\n+\t\trefs/heads/missing\n+\t\tEOF\n+\t\tgit reflog list >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.44.0-rc1\n\n"},{"id":"493393","messageId":"20240424073047.53755-1-tenglong.tl@alibaba-inc.com","threadId":"60955","inReplyTo":"d7b9cff4c360147e65df17316533fba0b4f2ab7d.1708418805.git.ps@pks.im","subject":"[PATCH v2 7/7] builtin/reflog: introduce subcommand to list reflogs","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2024-04-24T07:30:47Z","receivedAt":"2024-04-24T07:30:54Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Patrick Steinhardt <ps@pks.im> wrote:\n\n+#define BUILTIN_REFLOG_LIST_USAGE \\\n+\tN_(\"git reflog list\")\n\nDoesn't seem to need a translation here?\n\nThanks.\n"},{"id":"493395","messageId":"Zii8cxOxM-Ggwtu7@tanuki","threadId":"60955","inReplyTo":"20240424073047.53755-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v2 7/7] builtin/reflog: introduce subcommand to list reflogs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-24T08:01:55Z","receivedAt":"2024-04-24T08:02:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 24, 2024 at 03:30:47PM +0800, Teng Long wrote:\n> Patrick Steinhardt <ps@pks.im> wrote:\n> \n> +#define BUILTIN_REFLOG_LIST_USAGE \\\n> +\tN_(\"git reflog list\")\n> \n> Doesn't seem to need a translation here?\n\nI was following the precedent of the other subcommands, which all mark\ntheir usage as needing translation. Whether that is ultimately warranted\nI can't really tell. In any case, if we decide that it's not we should\nalso drop the marker for all the other usages.\n\nPatrick\n"},{"id":"493405","messageId":"xmqqbk5y3j8a.fsf@gitster.g","threadId":"60955","inReplyTo":"Zii8cxOxM-Ggwtu7@tanuki","subject":"Re: [PATCH v2 7/7] builtin/reflog: introduce subcommand to list reflogs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-24T14:53:25Z","receivedAt":"2024-04-24T14:53:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Wed, Apr 24, 2024 at 03:30:47PM +0800, Teng Long wrote:\n>> Patrick Steinhardt <ps@pks.im> wrote:\n>> \n>> +#define BUILTIN_REFLOG_LIST_USAGE \\\n>> +\tN_(\"git reflog list\")\n>> \n>> Doesn't seem to need a translation here?\n>\n> I was following the precedent of the other subcommands, which all mark\n> their usage as needing translation. Whether that is ultimately warranted\n> I can't really tell. In any case, if we decide that it's not we should\n> also drop the marker for all the other usages.\n\nThe motivation for N_() in others (namely, the ones with\n<placeholder>s) is that the literal part like \"git\" \"reflog\"\n\"expire\" and \"--expire=\" cannot be given differently even when the\nuser works in a different locale, but placeholders that explain the\nmeaning of what the user must plug in there like \"<time>\" is easier\nfor the users to be in their language.\n\nThe \"list\" subcommand is currently an oddball that does not happen\nto take an argument or an option with value, and does not use any\n<placeholder>, but for consistency and future-proofing, it is better\nto have it in N_().\n\nThanks.\n\n"}]}