From e69e8ffef7369ca0d24c570d2657e41cf5e45936 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 21 Feb 2024 13:37:27 +0100 Subject: refs/files: sort reflogs returned by the reflog iterator We use a directory iterator to return reflogs via the reflog iterator. This iterator returns entries in the same order as readdir(3P) would and will thus yield reflogs with no discernible order. Set the new `DIR_ITERATOR_SORTED` flag that was introduced in the preceding commit so that the order is deterministic. While the effect of this can only been observed in a test tool, a subsequent commit will start to expose this functionality to users via a new `git reflog list` subcommand. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 75dcc21ecb..2ffc63185f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2193,7 +2193,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store, strbuf_addf(&sb, "%s/logs", gitdir); - diter = dir_iterator_begin(sb.buf, 0); + diter = dir_iterator_begin(sb.buf, DIR_ITERATOR_SORTED); if (!diter) { strbuf_release(&sb); return empty_ref_iterator_begin(); @@ -2202,7 +2202,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store, CALLOC_ARRAY(iter, 1); ref_iterator = &iter->base; - base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0); + base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1); iter->dir_iterator = diter; iter->ref_store = ref_store; strbuf_release(&sb); -- cgit 1.2.3-korg From 6f227800176d7ed1d1c50d64ef5d5e485f5fbee3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 21 Feb 2024 13:37:31 +0100 Subject: refs/files: sort merged worktree and common reflogs When iterating through reflogs in a worktree we create a merged iterator that merges reflogs from both refdbs. The resulting refs are ordered so that instead we first return all worktree reflogs before we return all common refs. This is the only remaining case where a ref iterator returns entries in a non-lexicographic order. The result would look something like the following (listed with a command we introduce in a subsequent commit): ``` $ git reflog list HEAD refs/worktree/per-worktree refs/heads/main refs/heads/wt ``` So we first print the per-worktree reflogs in lexicographic order, then the common reflogs in lexicographic order. This is confusing and not consistent with how we print per-worktree refs, which are exclusively sorted lexicographically. Sort reflogs lexicographically in the same way as we sort normal refs. As this is already implemented properly by the "reftable" backend via a separate selection function, we simply pull out that logic and reuse it for the "files" backend. As logs are properly sorted now, mark the merged reflog iterator as sorted. Tests will be added in a subsequent commit. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 30 ++---------------------------- refs/iterator.c | 43 +++++++++++++++++++++++++++++++++++++++++++ refs/refs-internal.h | 9 +++++++++ refs/reftable-backend.c | 47 ++--------------------------------------------- 4 files changed, 56 insertions(+), 73 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index 2ffc63185f..551cafdf76 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2210,32 +2210,6 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store, return ref_iterator; } -static enum iterator_selection reflog_iterator_select( - struct ref_iterator *iter_worktree, - struct ref_iterator *iter_common, - void *cb_data UNUSED) -{ - if (iter_worktree) { - /* - * We're a bit loose here. We probably should ignore - * common refs if they are accidentally added as - * per-worktree refs. - */ - return ITER_SELECT_0; - } else if (iter_common) { - if (parse_worktree_ref(iter_common->refname, NULL, NULL, - NULL) == REF_WORKTREE_SHARED) - return ITER_SELECT_1; - - /* - * The main ref store may contain main worktree's - * per-worktree refs, which should be ignored - */ - return ITER_SKIP_1; - } else - return ITER_DONE; -} - static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store) { struct files_ref_store *refs = @@ -2246,9 +2220,9 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st return reflog_iterator_begin(ref_store, refs->gitcommondir); } else { return merge_ref_iterator_begin( - 0, reflog_iterator_begin(ref_store, refs->base.gitdir), + 1, reflog_iterator_begin(ref_store, refs->base.gitdir), reflog_iterator_begin(ref_store, refs->gitcommondir), - reflog_iterator_select, refs); + ref_iterator_select, refs); } } diff --git a/refs/iterator.c b/refs/iterator.c index 6b680f610e..b7ab5dc92f 100644 --- a/refs/iterator.c +++ b/refs/iterator.c @@ -98,6 +98,49 @@ struct merge_ref_iterator { struct ref_iterator **current; }; +enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree, + struct ref_iterator *iter_common, + void *cb_data UNUSED) +{ + if (iter_worktree && !iter_common) { + /* + * Return the worktree ref if there are no more common refs. + */ + return ITER_SELECT_0; + } else if (iter_common) { + /* + * In case we have pending worktree and common refs we need to + * yield them based on their lexicographical order. Worktree + * refs that have the same name as common refs shadow the + * latter. + */ + if (iter_worktree) { + int cmp = strcmp(iter_worktree->refname, + iter_common->refname); + if (cmp < 0) + return ITER_SELECT_0; + else if (!cmp) + return ITER_SELECT_0_SKIP_1; + } + + /* + * We now know that the lexicographically-next ref is a common + * ref. When the common ref is a shared one we return it. + */ + if (parse_worktree_ref(iter_common->refname, NULL, NULL, + NULL) == REF_WORKTREE_SHARED) + return ITER_SELECT_1; + + /* + * Otherwise, if the common ref is a per-worktree ref we skip + * it because it would belong to the main worktree, not ours. + */ + return ITER_SKIP_1; + } else { + return ITER_DONE; + } +} + static int merge_ref_iterator_advance(struct ref_iterator *ref_iterator) { struct merge_ref_iterator *iter = diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 83e0f0bba3..51f612e122 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -386,6 +386,15 @@ typedef enum iterator_selection ref_iterator_select_fn( struct ref_iterator *iter0, struct ref_iterator *iter1, void *cb_data); +/* + * An implementation of ref_iterator_select_fn that merges worktree and common + * refs. Per-worktree refs from the common iterator are ignored, worktree refs + * override common refs. Refs are selected lexicographically. + */ +enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree, + struct ref_iterator *iter_common, + void *cb_data); + /* * Iterate over the entries from iter0 and iter1, with the values * interleaved as directed by the select function. The iterator takes diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index a14f2ad7f4..68d32a9101 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -504,49 +504,6 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_ return iter; } -static enum iterator_selection iterator_select(struct ref_iterator *iter_worktree, - struct ref_iterator *iter_common, - void *cb_data UNUSED) -{ - if (iter_worktree && !iter_common) { - /* - * Return the worktree ref if there are no more common refs. - */ - return ITER_SELECT_0; - } else if (iter_common) { - /* - * In case we have pending worktree and common refs we need to - * yield them based on their lexicographical order. Worktree - * refs that have the same name as common refs shadow the - * latter. - */ - if (iter_worktree) { - int cmp = strcmp(iter_worktree->refname, - iter_common->refname); - if (cmp < 0) - return ITER_SELECT_0; - else if (!cmp) - return ITER_SELECT_0_SKIP_1; - } - - /* - * We now know that the lexicographically-next ref is a common - * ref. When the common ref is a shared one we return it. - */ - if (parse_worktree_ref(iter_common->refname, NULL, NULL, - NULL) == REF_WORKTREE_SHARED) - return ITER_SELECT_1; - - /* - * Otherwise, if the common ref is a per-worktree ref we skip - * it because it would belong to the main worktree, not ours. - */ - return ITER_SKIP_1; - } else { - return ITER_DONE; - } -} - static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_store, const char *prefix, const char **exclude_patterns, @@ -576,7 +533,7 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto */ worktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags); return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base, - iterator_select, NULL); + ref_iterator_select, NULL); } static int reftable_be_read_raw_ref(struct ref_store *ref_store, @@ -1759,7 +1716,7 @@ static struct ref_iterator *reftable_be_reflog_iterator_begin(struct ref_store * worktree_iter = reflog_iterator_for_stack(refs, refs->worktree_stack); return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base, - iterator_select, NULL); + ref_iterator_select, NULL); } static int yield_log_record(struct reftable_log_record *log, -- cgit 1.2.3-korg From 5e01d838412d6679c40c929bbb2591669ae393d4 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 21 Feb 2024 13:37:35 +0100 Subject: refs: always treat iterators as ordered In the preceding commit we have converted the reflog iterator of the "files" backend to be ordered, which was the only remaining ref iterator that wasn't ordered. Refactor the ref iterator infrastructure so that we always assume iterators to be ordered, thus simplifying the code. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 4 ---- refs/debug.c | 3 +-- refs/files-backend.c | 7 +++---- refs/iterator.c | 26 ++++++++------------------ refs/packed-backend.c | 2 +- refs/ref-cache.c | 2 +- refs/refs-internal.h | 18 ++---------------- refs/reftable-backend.c | 8 ++++---- 8 files changed, 20 insertions(+), 50 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index fff343c256..dc25606a82 100644 --- a/refs.c +++ b/refs.c @@ -1594,10 +1594,6 @@ struct ref_iterator *refs_ref_iterator_begin( if (trim) iter = prefix_ref_iterator_begin(iter, "", trim); - /* Sanity check for subclasses: */ - if (!iter->ordered) - BUG("reference iterator is not ordered"); - return iter; } diff --git a/refs/debug.c b/refs/debug.c index 634681ca44..c7531b17f0 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -181,7 +181,6 @@ static int debug_ref_iterator_advance(struct ref_iterator *ref_iterator) trace_printf_key(&trace_refs, "iterator_advance: %s (0)\n", diter->iter->refname); - diter->base.ordered = diter->iter->ordered; diter->base.refname = diter->iter->refname; diter->base.oid = diter->iter->oid; diter->base.flags = diter->iter->flags; @@ -222,7 +221,7 @@ debug_ref_iterator_begin(struct ref_store *ref_store, const char *prefix, drefs->refs->be->iterator_begin(drefs->refs, prefix, exclude_patterns, flags); struct debug_ref_iterator *diter = xcalloc(1, sizeof(*diter)); - base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable, 1); + base_ref_iterator_init(&diter->base, &debug_ref_iterator_vtable); diter->iter = res; trace_printf_key(&trace_refs, "ref_iterator_begin: \"%s\" (0x%x)\n", prefix, flags); diff --git a/refs/files-backend.c b/refs/files-backend.c index 551cafdf76..05bb0c875c 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -879,8 +879,7 @@ static struct ref_iterator *files_ref_iterator_begin( CALLOC_ARRAY(iter, 1); ref_iterator = &iter->base; - base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable, - overlay_iter->ordered); + base_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable); iter->iter0 = overlay_iter; iter->repo = ref_store->repo; iter->flags = flags; @@ -2202,7 +2201,7 @@ static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store, CALLOC_ARRAY(iter, 1); ref_iterator = &iter->base; - base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 1); + base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable); iter->dir_iterator = diter; iter->ref_store = ref_store; strbuf_release(&sb); @@ -2220,7 +2219,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st return reflog_iterator_begin(ref_store, refs->gitcommondir); } else { return merge_ref_iterator_begin( - 1, reflog_iterator_begin(ref_store, refs->base.gitdir), + reflog_iterator_begin(ref_store, refs->base.gitdir), reflog_iterator_begin(ref_store, refs->gitcommondir), ref_iterator_select, refs); } diff --git a/refs/iterator.c b/refs/iterator.c index b7ab5dc92f..9db8b056d5 100644 --- a/refs/iterator.c +++ b/refs/iterator.c @@ -25,11 +25,9 @@ int ref_iterator_abort(struct ref_iterator *ref_iterator) } void base_ref_iterator_init(struct ref_iterator *iter, - struct ref_iterator_vtable *vtable, - int ordered) + struct ref_iterator_vtable *vtable) { iter->vtable = vtable; - iter->ordered = !!ordered; iter->refname = NULL; iter->oid = NULL; iter->flags = 0; @@ -74,7 +72,7 @@ struct ref_iterator *empty_ref_iterator_begin(void) struct empty_ref_iterator *iter = xcalloc(1, sizeof(*iter)); struct ref_iterator *ref_iterator = &iter->base; - base_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable, 1); + base_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable); return ref_iterator; } @@ -250,7 +248,6 @@ static struct ref_iterator_vtable merge_ref_iterator_vtable = { }; struct ref_iterator *merge_ref_iterator_begin( - int ordered, struct ref_iterator *iter0, struct ref_iterator *iter1, ref_iterator_select_fn *select, void *cb_data) { @@ -265,7 +262,7 @@ struct ref_iterator *merge_ref_iterator_begin( * references through only if they exist in both iterators. */ - base_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable, ordered); + base_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable); iter->iter0 = iter0; iter->iter1 = iter1; iter->select = select; @@ -314,12 +311,9 @@ struct ref_iterator *overlay_ref_iterator_begin( } else if (is_empty_ref_iterator(back)) { ref_iterator_abort(back); return front; - } else if (!front->ordered || !back->ordered) { - BUG("overlay_ref_iterator requires ordered inputs"); } - return merge_ref_iterator_begin(1, front, back, - overlay_iterator_select, NULL); + return merge_ref_iterator_begin(front, back, overlay_iterator_select, NULL); } struct prefix_ref_iterator { @@ -358,16 +352,12 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator) if (cmp > 0) { /* - * If the source iterator is ordered, then we + * As the source iterator is ordered, we * can stop the iteration as soon as we see a * refname that comes after the prefix: */ - if (iter->iter0->ordered) { - ok = ref_iterator_abort(iter->iter0); - break; - } else { - continue; - } + ok = ref_iterator_abort(iter->iter0); + break; } if (iter->trim) { @@ -439,7 +429,7 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0, CALLOC_ARRAY(iter, 1); ref_iterator = &iter->base; - base_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable, iter0->ordered); + base_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable); iter->iter0 = iter0; iter->prefix = xstrdup(prefix); diff --git a/refs/packed-backend.c b/refs/packed-backend.c index a499a91c7e..4e826c05ff 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1111,7 +1111,7 @@ static struct ref_iterator *packed_ref_iterator_begin( CALLOC_ARRAY(iter, 1); ref_iterator = &iter->base; - base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable, 1); + base_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable); if (exclude_patterns) populate_excluded_jump_list(iter, snapshot, exclude_patterns); diff --git a/refs/ref-cache.c b/refs/ref-cache.c index a372a00941..9f9797209a 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -486,7 +486,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache, CALLOC_ARRAY(iter, 1); ref_iterator = &iter->base; - base_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable, 1); + base_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable); ALLOC_GROW(iter->levels, 10, iter->levels_alloc); iter->levels_nr = 1; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 51f612e122..a9b6e887f8 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -312,13 +312,6 @@ enum do_for_each_ref_flags { */ struct ref_iterator { struct ref_iterator_vtable *vtable; - - /* - * Does this `ref_iterator` iterate over references in order - * by refname? - */ - unsigned int ordered : 1; - const char *refname; const struct object_id *oid; unsigned int flags; @@ -399,11 +392,9 @@ enum iterator_selection ref_iterator_select(struct ref_iterator *iter_worktree, * Iterate over the entries from iter0 and iter1, with the values * interleaved as directed by the select function. The iterator takes * ownership of iter0 and iter1 and frees them when the iteration is - * over. A derived class should set `ordered` to 1 or 0 based on - * whether it generates its output in order by reference name. + * over. */ struct ref_iterator *merge_ref_iterator_begin( - int ordered, struct ref_iterator *iter0, struct ref_iterator *iter1, ref_iterator_select_fn *select, void *cb_data); @@ -432,8 +423,6 @@ struct ref_iterator *overlay_ref_iterator_begin( * As an convenience to callers, if prefix is the empty string and * trim is zero, this function returns iter0 directly, without * wrapping it. - * - * The resulting ref_iterator is ordered if iter0 is. */ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0, const char *prefix, @@ -444,14 +433,11 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0, /* * Base class constructor for ref_iterators. Initialize the * ref_iterator part of iter, setting its vtable pointer as specified. - * `ordered` should be set to 1 if the iterator will iterate over - * references in order by refname; otherwise it should be set to 0. * This is meant to be called only by the initializers of derived * classes. */ void base_ref_iterator_init(struct ref_iterator *iter, - struct ref_iterator_vtable *vtable, - int ordered); + struct ref_iterator_vtable *vtable); /* * Base class destructor for ref_iterators. Destroy the ref_iterator diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 68d32a9101..39e9a9d4e2 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -479,7 +479,7 @@ static struct reftable_ref_iterator *ref_iterator_for_stack(struct reftable_ref_ int ret; iter = xcalloc(1, sizeof(*iter)); - base_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable, 1); + base_ref_iterator_init(&iter->base, &reftable_ref_iterator_vtable); iter->prefix = prefix; iter->base.oid = &iter->oid; iter->flags = flags; @@ -532,7 +532,7 @@ static struct ref_iterator *reftable_be_iterator_begin(struct ref_store *ref_sto * single iterator. */ worktree_iter = ref_iterator_for_stack(refs, refs->worktree_stack, prefix, flags); - return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base, + return merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base, ref_iterator_select, NULL); } @@ -1680,7 +1680,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl int ret; iter = xcalloc(1, sizeof(*iter)); - base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1); + base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable); iter->refs = refs; iter->base.oid = &iter->oid; @@ -1715,7 +1715,7 @@ static struct ref_iterator *reftable_be_reflog_iterator_begin(struct ref_store * worktree_iter = reflog_iterator_for_stack(refs, refs->worktree_stack); - return merge_ref_iterator_begin(1, &worktree_iter->base, &main_iter->base, + return merge_ref_iterator_begin(&worktree_iter->base, &main_iter->base, ref_iterator_select, NULL); } -- cgit 1.2.3-korg From 31f898397bb2f44692b8bcc4fd64fffaf3b59c48 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 21 Feb 2024 13:37:39 +0100 Subject: refs: drop unused params from the reflog iterator callback The ref and reflog iterators share much of the same underlying code to iterate over the corresponding entries. This results in some weird code because the reflog iterator also exposes an object ID as well as a flag to the callback function. Neither of these fields do refer to the reflog though -- they refer to the corresponding ref with the same name. This is quite misleading. In practice at least the object ID cannot really be implemented in any other way as a reflog does not have a specific object ID in the first place. This is further stressed by the fact that none of the callbacks except for our test helper make use of these fields. Split up the infrastucture so that ref and reflog iterators use separate callback signatures. This allows us to drop the nonsensical fields from the reflog iterator. Note that internally, the backends still use the same shared infra to iterate over both types. As the backends should never end up being called directly anyway, this is not much of a problem and thus kept as-is for simplicity's sake. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/fsck.c | 4 +--- builtin/reflog.c | 3 +-- refs.c | 23 +++++++++++++++++++---- refs.h | 11 +++++++++-- refs/files-backend.c | 8 +------- refs/reftable-backend.c | 8 +------- revision.c | 4 +--- t/helper/test-ref-store.c | 18 ++++++++++++------ t/t0600-reffiles-backend.sh | 24 ++++++++++++------------ t/t1405-main-ref-store.sh | 8 ++++---- t/t1406-submodule-ref-store.sh | 8 ++++---- 11 files changed, 65 insertions(+), 54 deletions(-) (limited to 'refs/files-backend.c') diff --git a/builtin/fsck.c b/builtin/fsck.c index a7cf94f67e..f892487c9b 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -509,9 +509,7 @@ static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid return 0; } -static int fsck_handle_reflog(const char *logname, - const struct object_id *oid UNUSED, - int flag UNUSED, void *cb_data) +static int fsck_handle_reflog(const char *logname, void *cb_data) { struct strbuf refname = STRBUF_INIT; diff --git a/builtin/reflog.c b/builtin/reflog.c index a5a4099f61..3a0c4d4322 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -60,8 +60,7 @@ struct worktree_reflogs { struct string_list reflogs; }; -static int collect_reflog(const char *ref, const struct object_id *oid UNUSED, - int flags UNUSED, void *cb_data) +static int collect_reflog(const char *ref, void *cb_data) { struct worktree_reflogs *cb = cb_data; struct worktree *worktree = cb->worktree; diff --git a/refs.c b/refs.c index dc25606a82..f9261267f0 100644 --- a/refs.c +++ b/refs.c @@ -2512,18 +2512,33 @@ int refs_verify_refname_available(struct ref_store *refs, return ret; } -int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data) +struct do_for_each_reflog_help { + each_reflog_fn *fn; + void *cb_data; +}; + +static int do_for_each_reflog_helper(struct repository *r UNUSED, + const char *refname, + const struct object_id *oid UNUSED, + int flags, + void *cb_data) +{ + struct do_for_each_reflog_help *hp = cb_data; + return hp->fn(refname, hp->cb_data); +} + +int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data) { struct ref_iterator *iter; - struct do_for_each_ref_help hp = { fn, cb_data }; + struct do_for_each_reflog_help hp = { fn, cb_data }; iter = refs->be->reflog_iterator_begin(refs); return do_for_each_repo_ref_iterator(the_repository, iter, - do_for_each_ref_helper, &hp); + do_for_each_reflog_helper, &hp); } -int for_each_reflog(each_ref_fn fn, void *cb_data) +int for_each_reflog(each_reflog_fn fn, void *cb_data) { return refs_for_each_reflog(get_main_ref_store(the_repository), fn, cb_data); } diff --git a/refs.h b/refs.h index 303c5fac4d..895579aeb7 100644 --- a/refs.h +++ b/refs.h @@ -534,12 +534,19 @@ int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_dat /* youngest entry first */ int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data); +/* + * The signature for the callback function for the {refs_,}for_each_reflog() + * functions below. The memory pointed to by the refname argument is only + * guaranteed to be valid for the duration of a single callback invocation. + */ +typedef int each_reflog_fn(const char *refname, void *cb_data); + /* * Calls the specified function for each reflog file until it returns nonzero, * and returns the value. Reflog file order is unspecified. */ -int refs_for_each_reflog(struct ref_store *refs, each_ref_fn fn, void *cb_data); -int for_each_reflog(each_ref_fn fn, void *cb_data); +int refs_for_each_reflog(struct ref_store *refs, each_reflog_fn fn, void *cb_data); +int for_each_reflog(each_reflog_fn fn, void *cb_data); #define REFNAME_ALLOW_ONELEVEL 1 #define REFNAME_REFSPEC_PATTERN 2 diff --git a/refs/files-backend.c b/refs/files-backend.c index 05bb0c875c..c7aff6b331 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2115,10 +2115,8 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store, struct files_reflog_iterator { struct ref_iterator base; - struct ref_store *ref_store; struct dir_iterator *dir_iterator; - struct object_id oid; }; static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) @@ -2129,8 +2127,6 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) int ok; while ((ok = dir_iterator_advance(diter)) == ITER_OK) { - int flags; - if (!S_ISREG(diter->st.st_mode)) continue; if (diter->basename[0] == '.') @@ -2140,14 +2136,12 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) if (!refs_resolve_ref_unsafe(iter->ref_store, diter->relative_path, 0, - &iter->oid, &flags)) { + NULL, NULL)) { error("bad ref for %s", diter->path.buf); continue; } iter->base.refname = diter->relative_path; - iter->base.oid = &iter->oid; - iter->base.flags = flags; return ITER_OK; } diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 39e9a9d4e2..4998b676c2 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1594,7 +1594,6 @@ struct reftable_reflog_iterator { struct reftable_ref_store *refs; struct reftable_iterator iter; struct reftable_log_record log; - struct object_id oid; char *last_name; int err; }; @@ -1605,8 +1604,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator) (struct reftable_reflog_iterator *)ref_iterator; while (!iter->err) { - int flags; - iter->err = reftable_iterator_next_log(&iter->iter, &iter->log); if (iter->err) break; @@ -1620,7 +1617,7 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator) continue; if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname, - 0, &iter->oid, &flags)) { + 0, NULL, NULL)) { error(_("bad ref for %s"), iter->log.refname); continue; } @@ -1628,8 +1625,6 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator) free(iter->last_name); iter->last_name = xstrdup(iter->log.refname); iter->base.refname = iter->log.refname; - iter->base.oid = &iter->oid; - iter->base.flags = flags; break; } @@ -1682,7 +1677,6 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl iter = xcalloc(1, sizeof(*iter)); base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable); iter->refs = refs; - iter->base.oid = &iter->oid; ret = refs->err; if (ret) diff --git a/revision.c b/revision.c index 2424c9bd67..ac45c6d8f2 100644 --- a/revision.c +++ b/revision.c @@ -1686,9 +1686,7 @@ static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid, return 0; } -static int handle_one_reflog(const char *refname_in_wt, - const struct object_id *oid UNUSED, - int flag UNUSED, void *cb_data) +static int handle_one_reflog(const char *refname_in_wt, void *cb_data) { struct all_refs_cb *cb = cb_data; struct strbuf refname = STRBUF_INIT; diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 702ec1f128..7a0f6cac53 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -221,15 +221,21 @@ static int cmd_verify_ref(struct ref_store *refs, const char **argv) return ret; } +static int each_reflog(const char *refname, void *cb_data UNUSED) +{ + printf("%s\n", refname); + return 0; +} + static int cmd_for_each_reflog(struct ref_store *refs, const char **argv UNUSED) { - return refs_for_each_reflog(refs, each_ref, NULL); + return refs_for_each_reflog(refs, each_reflog, NULL); } -static int each_reflog(struct object_id *old_oid, struct object_id *new_oid, - const char *committer, timestamp_t timestamp, - int tz, const char *msg, void *cb_data UNUSED) +static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid, + const char *committer, timestamp_t timestamp, + int tz, const char *msg, void *cb_data UNUSED) { printf("%s %s %s %" PRItime " %+05d%s%s", oid_to_hex(old_oid), oid_to_hex(new_oid), committer, timestamp, tz, @@ -241,14 +247,14 @@ static int cmd_for_each_reflog_ent(struct ref_store *refs, const char **argv) { const char *refname = notnull(*argv++, "refname"); - return refs_for_each_reflog_ent(refs, refname, each_reflog, refs); + return refs_for_each_reflog_ent(refs, refname, each_reflog_ent, refs); } static int cmd_for_each_reflog_ent_reverse(struct ref_store *refs, const char **argv) { const char *refname = notnull(*argv++, "refname"); - return refs_for_each_reflog_ent_reverse(refs, refname, each_reflog, refs); + return refs_for_each_reflog_ent_reverse(refs, refname, each_reflog_ent, refs); } static int cmd_reflog_exists(struct ref_store *refs, const char **argv) diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh index 4f860285cc..56a3196b83 100755 --- a/t/t0600-reffiles-backend.sh +++ b/t/t0600-reffiles-backend.sh @@ -287,23 +287,23 @@ test_expect_success 'for_each_reflog()' ' mkdir -p .git/worktrees/wt/logs/refs/bisect && echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random && - $RWT for-each-reflog | cut -d" " -f 2- >actual && + $RWT for-each-reflog >actual && cat >expected <<-\EOF && - HEAD 0x1 - PSEUDO-WT 0x0 - refs/bisect/wt-random 0x0 - refs/heads/main 0x0 - refs/heads/wt-main 0x0 + HEAD + PSEUDO-WT + refs/bisect/wt-random + refs/heads/main + refs/heads/wt-main EOF test_cmp expected actual && - $RMAIN for-each-reflog | cut -d" " -f 2- >actual && + $RMAIN for-each-reflog >actual && cat >expected <<-\EOF && - HEAD 0x1 - PSEUDO-MAIN 0x0 - refs/bisect/random 0x0 - refs/heads/main 0x0 - refs/heads/wt-main 0x0 + HEAD + PSEUDO-MAIN + refs/bisect/random + refs/heads/main + refs/heads/wt-main EOF test_cmp expected actual ' diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh index cfb583f544..3eee758bce 100755 --- a/t/t1405-main-ref-store.sh +++ b/t/t1405-main-ref-store.sh @@ -74,11 +74,11 @@ test_expect_success 'verify_ref(new-main)' ' ' test_expect_success 'for_each_reflog()' ' - $RUN for-each-reflog | cut -d" " -f 2- >actual && + $RUN for-each-reflog >actual && cat >expected <<-\EOF && - HEAD 0x1 - refs/heads/main 0x0 - refs/heads/new-main 0x0 + HEAD + refs/heads/main + refs/heads/new-main EOF test_cmp expected actual ' diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh index 40332e23cc..c01f0f14a1 100755 --- a/t/t1406-submodule-ref-store.sh +++ b/t/t1406-submodule-ref-store.sh @@ -63,11 +63,11 @@ test_expect_success 'verify_ref(new-main)' ' ' test_expect_success 'for_each_reflog()' ' - $RUN for-each-reflog | cut -d" " -f 2- >actual && + $RUN for-each-reflog >actual && cat >expected <<-\EOF && - HEAD 0x1 - refs/heads/main 0x0 - refs/heads/new-main 0x0 + HEAD + refs/heads/main + refs/heads/new-main EOF test_cmp expected actual ' -- cgit 1.2.3-korg From 59c50a96c5d2e58f981e0ceceaeadd941d5a19b7 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 21 Feb 2024 13:37:43 +0100 Subject: refs: stop resolving ref corresponding to reflogs The reflog iterator tries to resolve the corresponding ref for every reflog that it is about to yield. Historically, this was done due to multiple reasons: - It ensures that the refname is safe because we end up calling `check_refname_format()`. Also, non-conformant refnames are skipped altogether. - The iterator used to yield the resolved object ID as well as its flags to the callback. This info was never used though, and the corresponding parameters were dropped in the preceding commit. - When a ref is corrupt then the reflog is not emitted at all. We're about to introduce a new `git reflog list` subcommand that will print all reflogs that the refdb knows about. Skipping over reflogs whose refs are corrupted would be quite counterproductive in this case as the user would have no way to learn about reflogs which may still exist in their repository to help and rescue such a corrupted ref. Thus, the only remaining reason for why we'd want to resolve the ref is to verify its refname. Refactor the code to call `check_refname_format()` directly instead of trying to resolve the ref. This is significantly more efficient given that we don't have to hit the object database anymore to list reflogs. And second, it ensures that we end up showing reflogs of broken refs, which will help to make the reflog more useful. Note that this really only impacts the case where the corresponding ref is corrupt. Reflogs for nonexistent refs would have been returned to the caller beforehand already as we did not pass `RESOLVE_REF_READING` to the function, and thus `refs_resolve_ref_unsafe()` would have returned successfully in that case. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 12 ++---------- refs/reftable-backend.c | 6 ++---- 2 files changed, 4 insertions(+), 14 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index c7aff6b331..6f98168a81 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2129,17 +2129,9 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) while ((ok = dir_iterator_advance(diter)) == ITER_OK) { if (!S_ISREG(diter->st.st_mode)) continue; - if (diter->basename[0] == '.') + if (check_refname_format(diter->basename, + REFNAME_ALLOW_ONELEVEL)) continue; - if (ends_with(diter->basename, ".lock")) - continue; - - if (!refs_resolve_ref_unsafe(iter->ref_store, - diter->relative_path, 0, - NULL, NULL)) { - error("bad ref for %s", diter->path.buf); - continue; - } iter->base.refname = diter->relative_path; return ITER_OK; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 4998b676c2..6c11c4a5e3 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1616,11 +1616,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator) if (iter->last_name && !strcmp(iter->log.refname, iter->last_name)) continue; - if (!refs_resolve_ref_unsafe(&iter->refs->base, iter->log.refname, - 0, NULL, NULL)) { - error(_("bad ref for %s"), iter->log.refname); + if (check_refname_format(iter->log.refname, + REFNAME_ALLOW_ONELEVEL)) continue; - } free(iter->last_name); iter->last_name = xstrdup(iter->log.refname); -- cgit 1.2.3-korg