From bf708add2eaf37d53fa8a908b5d8772806c54d1b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:37:58 -0400 Subject: refs-internal.h: move DO_FOR_EACH_* flags next to each other There are currently two DO_FOR_EACH_* flags, which must not have their bits overlap. Yet they're defined hundreds of lines apart. Let's move them next to each other to make it clear that they are related and are a complete set (which matters if you are adding a new flag and would like to know what the next available bit is). Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/refs-internal.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'refs') diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 3155708345..7b30910974 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -248,6 +248,14 @@ int refs_rename_ref_available(struct ref_store *refs, /* Include broken references in a do_for_each_ref*() iteration: */ #define DO_FOR_EACH_INCLUDE_BROKEN 0x01 +/* + * Only include per-worktree refs in a do_for_each_ref*() iteration. + * Normally this will be used with a files ref_store, since that's + * where all reference backends will presumably store their + * per-worktree refs. + */ +#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02 + /* * Reference iterators * @@ -498,14 +506,6 @@ int do_for_each_repo_ref_iterator(struct repository *r, struct ref_iterator *iter, each_repo_ref_fn fn, void *cb_data); -/* - * Only include per-worktree refs in a do_for_each_ref*() iteration. - * Normally this will be used with a files ref_store, since that's - * where all reference backends will presumably store their - * per-worktree refs. - */ -#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02 - struct ref_store; /* refs backends */ -- cgit v1.3-6-g1900 From 9aab952e855be1a567d4d0585afbdbde624e274f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:39:44 -0400 Subject: refs-internal.h: reorganize DO_FOR_EACH_* flag documentation The documentation for the DO_FOR_EACH_* flags is sprinkled over the refs-internal.h file. We define the two flags in one spot, and then describe them in more detail far away from there, in the definitions of refs_ref_iterator_begin() and ref_iterator_advance_fn(). Let's try to organize this a bit better: - convert the #defines to an enum. This makes it clear that they are related, and that the enum shows the complete set of flags. - combine all descriptions for each flag in a single spot, next to the flag's definition - use the enum rather than a bare int for functions which take the flags. This helps readers realize which flags can be used. - clarify the mention of flags for ref_iterator_advance_fn(). It does not take flags itself, but is meant to depend on ones set up earlier. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs.c | 10 ++++++---- refs/refs-internal.h | 46 +++++++++++++++++++++++++++------------------- 2 files changed, 33 insertions(+), 23 deletions(-) (limited to 'refs') diff --git a/refs.c b/refs.c index 8b9f7c3a80..c28bd6a818 100644 --- a/refs.c +++ b/refs.c @@ -1413,7 +1413,8 @@ int head_ref(each_ref_fn fn, void *cb_data) struct ref_iterator *refs_ref_iterator_begin( struct ref_store *refs, - const char *prefix, int trim, int flags) + const char *prefix, int trim, + enum do_for_each_ref_flags flags) { struct ref_iterator *iter; @@ -1479,7 +1480,8 @@ static int do_for_each_ref_helper(struct repository *r, } static int do_for_each_ref(struct ref_store *refs, const char *prefix, - each_ref_fn fn, int trim, int flags, void *cb_data) + each_ref_fn fn, int trim, + enum do_for_each_ref_flags flags, void *cb_data) { struct ref_iterator *iter; struct do_for_each_ref_help hp = { fn, cb_data }; @@ -1516,7 +1518,7 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data) int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken) { - unsigned int flag = 0; + enum do_for_each_ref_flags flag = 0; if (broken) flag = DO_FOR_EACH_INCLUDE_BROKEN; @@ -1528,7 +1530,7 @@ int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken) { - unsigned int flag = 0; + enum do_for_each_ref_flags flag = 0; if (broken) flag = DO_FOR_EACH_INCLUDE_BROKEN; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 7b30910974..2c4e1739f2 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -245,16 +245,30 @@ int refs_rename_ref_available(struct ref_store *refs, /* We allow "recursive" symbolic refs. Only within reason, though */ #define SYMREF_MAXDEPTH 5 -/* Include broken references in a do_for_each_ref*() iteration: */ -#define DO_FOR_EACH_INCLUDE_BROKEN 0x01 - /* - * Only include per-worktree refs in a do_for_each_ref*() iteration. - * Normally this will be used with a files ref_store, since that's - * where all reference backends will presumably store their - * per-worktree refs. + * These flags are passed to refs_ref_iterator_begin() (and do_for_each_ref(), + * which feeds it). */ -#define DO_FOR_EACH_PER_WORKTREE_ONLY 0x02 +enum do_for_each_ref_flags { + /* + * Include broken references in a do_for_each_ref*() iteration, which + * would normally be omitted. This includes both refs that point to + * missing objects (a true repository corruption), ones with illegal + * names (which we prefer not to expose to callers), as well as + * dangling symbolic refs (i.e., those that point to a non-existent + * ref; this is not a corruption, but as they have no valid oid, we + * omit them from normal iteration results). + */ + DO_FOR_EACH_INCLUDE_BROKEN = (1 << 0), + + /* + * Only include per-worktree refs in a do_for_each_ref*() iteration. + * Normally this will be used with a files ref_store, since that's + * where all reference backends will presumably store their + * per-worktree refs. + */ + DO_FOR_EACH_PER_WORKTREE_ONLY = (1 << 1), +}; /* * Reference iterators @@ -357,16 +371,12 @@ int is_empty_ref_iterator(struct ref_iterator *ref_iterator); * Return an iterator that goes over each reference in `refs` for * which the refname begins with prefix. If trim is non-zero, then * trim that many characters off the beginning of each refname. - * The output is ordered by refname. The following flags are supported: - * - * DO_FOR_EACH_INCLUDE_BROKEN: include broken references in - * the iteration. - * - * DO_FOR_EACH_PER_WORKTREE_ONLY: only produce REF_TYPE_PER_WORKTREE refs. + * The output is ordered by refname. */ struct ref_iterator *refs_ref_iterator_begin( struct ref_store *refs, - const char *prefix, int trim, int flags); + const char *prefix, int trim, + enum do_for_each_ref_flags flags); /* * A callback function used to instruct merge_ref_iterator how to @@ -454,10 +464,8 @@ void base_ref_iterator_free(struct ref_iterator *iter); /* * backend-specific implementation of ref_iterator_advance. For symrefs, the * function should set REF_ISSYMREF, and it should also dereference the symref - * to provide the OID referent. If DO_FOR_EACH_INCLUDE_BROKEN is set, symrefs - * with non-existent referents and refs pointing to non-existent object names - * should also be returned. If DO_FOR_EACH_PER_WORKTREE_ONLY, only - * REF_TYPE_PER_WORKTREE refs should be returned. + * to provide the OID referent. It should respect do_for_each_ref_flags + * that were passed to refs_ref_iterator_begin(). */ typedef int ref_iterator_advance_fn(struct ref_iterator *ref_iterator); -- cgit v1.3-6-g1900 From 8dccb2244c81258cf9f3480c4102d14e30978194 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:41:32 -0400 Subject: refs: add DO_FOR_EACH_OMIT_DANGLING_SYMREFS flag When the DO_FOR_EACH_INCLUDE_BROKEN flag is used, we include both actual corrupt refs (illegal names, missing objects), but also symrefs that point to nothing. This latter is not really a corruption, but just something that may happen normally. For example, the symref at refs/remotes/origin/HEAD may point to a tracking branch which is later deleted. (The local HEAD may also be unborn, of course, but we do not access it through ref iteration). Most callers of for_each_ref() etc, do not care. They don't pass INCLUDE_BROKEN, so don't see it at all. But for those which do pass it, this somewhat-normal state causes extra warnings (e.g., from for-each-ref) or even aborts operations (destructive repacks with GIT_REF_PARANOIA set). This patch just introduces the flag and the mechanism; there are no callers yet (and hence no tests). Two things to note on the implementation: - we actually skip any symref that does not resolve to a ref. This includes ones which point to an invalidly-named ref. You could argue this is a more serious breakage than simple dangling. But the overall effect is the same (we could not follow the symref), as well as the impact on things like REF_PARANOIA (either way, a symref we can't follow won't impact reachability, because we'll see the ref itself during iteration). The underlying resolution function doesn't distinguish these two cases (they both get REF_ISBROKEN). - we change the iterator in refs/files-backend.c where we check INCLUDE_BROKEN. There's a matching spot in refs/packed-backend.c, but we don't know need to do anything there. The packed backend does not support symrefs at all. The resulting set of flags might be a bit easier to follow if we broke this down into "INCLUDE_CORRUPT_REFS" and "INCLUDE_DANGLING_SYMREFS". But there are a few reasons not do so: - adding a new OMIT_DANGLING_SYMREFS flag lets us leave existing callers intact, without changing their behavior (and some of them really do want to see the dangling symrefs; e.g., t5505 has a test which expects us to report when a symref becomes dangling) - they're not actually independent. You cannot say "include dangling symrefs" without also including refs whose objects are not reachable, because dangling symrefs by definition do not have an object. We could tweak the implementation to distinguish this, but in practice nobody wants to ask for that. Adding the OMIT flag keeps the implementation simple and makes sure we don't regress the current behavior. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano --- refs/files-backend.c | 5 +++++ refs/refs-internal.h | 6 ++++++ 2 files changed, 11 insertions(+) (limited to 'refs') diff --git a/refs/files-backend.c b/refs/files-backend.c index 74c0385873..1148c0cf09 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -744,6 +744,11 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator) ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE) continue; + if ((iter->flags & DO_FOR_EACH_OMIT_DANGLING_SYMREFS) && + (iter->iter0->flags & REF_ISSYMREF) && + (iter->iter0->flags & REF_ISBROKEN)) + continue; + if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && !ref_resolves_to_object(iter->iter0->refname, iter->iter0->oid, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 2c4e1739f2..96911fb26e 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -268,6 +268,12 @@ enum do_for_each_ref_flags { * per-worktree refs. */ DO_FOR_EACH_PER_WORKTREE_ONLY = (1 << 1), + + /* + * Omit dangling symrefs from output; this only has an effect with + * INCLUDE_BROKEN, since they are otherwise not included at all. + */ + DO_FOR_EACH_OMIT_DANGLING_SYMREFS = (1 << 2), }; /* -- cgit v1.3-6-g1900