From b4724242fa3342d939e7a0c4102d3695091db1f6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:32:17 -0400 Subject: t7900: clean up some more broken refs The "incremental-repack task" test replaces the object directory with a known state. As a result, some of our refs point to objects that are not included in that state. Commit 3cf5f221be (t7900: clean up some broken refs, 2021-01-19) cleaned up some of those (that were causing warnings to stderr from the maintenance process). But there are a few more that were missed. These aren't hurting anything for now, but it's certainly an unexpected state to leave the test repository in, and it will become a problem if repack ever gets more picky about broken refs. Let's clean up those additional refs (which are all in refs/remotes, with nothing there that isn't broken), and add an extra "for-each-ref" call to assert that we've got everything. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 36a4218..31245f6 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -277,7 +277,7 @@ test_expect_success 'incremental-repack task' ' # Delete refs that have not been repacked in these packs. git for-each-ref --format="delete %(refname)" \ - refs/prefetch refs/tags >refs && + refs/prefetch refs/tags refs/remotes >refs && git update-ref --stdin packs-before && test_line_count = 3 packs-before && + # make sure we do not have any broken refs that were + # missed in the deletion above + git for-each-ref && + # the job repacks the two into a new pack, but does not # delete the old ones. git maintenance run --task=incremental-repack && -- cgit v0.10.2-6-g49f6 From e9de7a52a5cb1d6647a33a3ce0eedb947e04f3fc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:33:00 -0400 Subject: t5516: don't use HEAD ref for invalid ref-deletion tests A few tests in t5516 want to assert that we can delete a corrupted ref whose pointed-to object is missing. They do so by using the "main" branch, which is also pointed to by HEAD. This does work, but only because of a subtle assumption about the implementation. We do not block the deletion because of the invalid ref, but we _also_ do not notice that the deleted branch is pointed to by HEAD. And so the safety rule of "do not allow HEAD to be deleted in a non-bare repository" does not kick in, and the test passes. Let's instead use a non-HEAD branch. That still tests what we care about here (deleting a corrupt ref), but without implicitly depending on our failure to notice that we're deleting HEAD. That will future proof the test against that behavior changing. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 4db8edd..b13553e 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -662,10 +662,10 @@ test_expect_success 'push does not update local refs on failure' ' test_expect_success 'allow deleting an invalid remote ref' ' - mk_test testrepo heads/main && + mk_test testrepo heads/branch && rm -f testrepo/.git/objects/??/* && - git push testrepo :refs/heads/main && - (cd testrepo && test_must_fail git rev-parse --verify refs/heads/main) + git push testrepo :refs/heads/branch && + (cd testrepo && test_must_fail git rev-parse --verify refs/heads/branch) ' @@ -706,25 +706,25 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho ' test_expect_success 'deleting dangling ref triggers hooks with correct args' ' - mk_test_with_hooks testrepo heads/main && + mk_test_with_hooks testrepo heads/branch && rm -f testrepo/.git/objects/??/* && - git push testrepo :refs/heads/main && + git push testrepo :refs/heads/branch && ( cd testrepo/.git && cat >pre-receive.expect <<-EOF && - $ZERO_OID $ZERO_OID refs/heads/main + $ZERO_OID $ZERO_OID refs/heads/branch EOF cat >update.expect <<-EOF && - refs/heads/main $ZERO_OID $ZERO_OID + refs/heads/branch $ZERO_OID $ZERO_OID EOF cat >post-receive.expect <<-EOF && - $ZERO_OID $ZERO_OID refs/heads/main + $ZERO_OID $ZERO_OID refs/heads/branch EOF cat >post-update.expect <<-EOF && - refs/heads/main + refs/heads/branch EOF test_cmp pre-receive.expect pre-receive.actual && -- cgit v0.10.2-6-g49f6 From da5e0c6a00fd01112b10f3e47cf8044a37ec329d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:34:04 -0400 Subject: t5600: provide detached HEAD for corruption failures When checking how git-clone behaves when it fails, we stimulate some failures by trying to do a clone from a local repository whose objects have been removed. Because these clones use local optimizations, there's a subtle dependency in how the corruption is handled on the sending side. If upload-pack does not show us the broken refs (which it does not currently), then we see only HEAD (which is itself broken), and clone that as a detached HEAD. When we try to write the ref, we notice that we never got the object and bail. But if upload-pack _does_ show us the broken refs (which it may in a future patch), then we'll realize that HEAD is a symref and just write that. You'd think we'd fail when writing out the refs themselves, but we don't; we do a bulk write and skip the connectivity check because of our --local optimizations. For the non-bare case, we do notice the problem when we try to checkout. But for a bare repository, we unexpectedly complete the clone successfully! At first glance this may seem like a bug. But the whole point of those local optimizations is to give up some safety for speed. If you want to be careful, you should be using "--no-local", which would notice that the pack did not transfer sufficient objects. We could do that in these tests, but part of the point is for them to fail at specific moments (and indeed, we have a later test that checks for transport failure). However, we can make this less subtle and future-proof it against changes on the upload-pack side by just having an explicit detached HEAD in the corrupted repo. Now we'll fail as expected during the ref write if any ref _or_ HEAD is corrupt, whether we're --bare or not. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh index 5bf1026..34b3df4 100755 --- a/t/t5600-clone-fail-cleanup.sh +++ b/t/t5600-clone-fail-cleanup.sh @@ -35,7 +35,9 @@ test_expect_success 'create a repo to clone' ' ' test_expect_success 'create objects in repo for later corruption' ' - test_commit -C foo file + test_commit -C foo file && + git -C foo checkout --detach && + test_commit -C foo detached ' # source repository given to git clone should be relative to the -- cgit v0.10.2-6-g49f6 From 2ac0cbc9b0bf9236b34eff4aa0160f5cfc3606c4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:35:29 -0400 Subject: t5312: drop "verbose" helper t5312 has several uses of the "verbose" helper, as described in 8ad1652418 (t5304: use helper to report failure of "test foo = bar", 2014-10-10). Back then the "-x" trace option for tests was new, and was not as pleasant to use (e.g., some tests failed under "-x", we did not support BASH_XTRACEFD, etc). These days it is clear that "-x" is the preferred way to get extra output, and we don't need to mark up individual tests. Let's get rid of the uses of "verbose" here, as one step toward eradicating it totally. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index 11423b3..165cc80 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -31,21 +31,21 @@ test_expect_success 'create history reachable only from a bogus-named ref' ' test_expect_success 'pruning does not drop bogus object' ' test_when_finished "git hash-object -w -t commit saved" && test_might_fail git prune --expire=now && - verbose git cat-file -e $bogus + git cat-file -e $bogus ' test_expect_success 'put bogus object into pack' ' git tag reachable $bogus && git repack -ad && git tag -d reachable && - verbose git cat-file -e $bogus + git cat-file -e $bogus ' test_expect_success 'destructive repack keeps packed object' ' test_might_fail git repack -Ad --unpack-unreachable=now && - verbose git cat-file -e $bogus && + git cat-file -e $bogus && test_might_fail git repack -ad && - verbose git cat-file -e $bogus + git cat-file -e $bogus ' # subsequent tests will have different corruptions @@ -78,7 +78,7 @@ test_expect_success 'create history with missing tip commit' ' test_expect_success 'pruning with a corrupted tip does not drop history' ' test_when_finished "git hash-object -w -t commit saved" && test_might_fail git prune --expire=now && - verbose git cat-file -e $recoverable + git cat-file -e $recoverable ' test_expect_success 'pack-refs does not silently delete broken loose ref' ' -- cgit v0.10.2-6-g49f6 From f805844676ac4d6f5def5c2ace1d0430c410e21e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:36:18 -0400 Subject: t5312: create bogus ref as necessary Some tests in t5312 create an illegally-named ref, and then see how various operations handle it. But between those operations, we also do some more setup (e.g., repacking), and we are subtly depending on how those setup steps react to the illegal ref. To future-proof us against those behaviors changing, let's instead create and clean up our bogus ref on demand in the tests that need it. This has two small extra advantages: - the tests are more stand-alone; we do not need an extra test to clean up the ref before moving on to other parts of the script - the creation and cleanup is together in one helper function. Because these depend on touching the refs in the filesystem directly, they may need to be tweaked for a world with alternate backends (they have not been noticed so far in the reftable work because with a non-file backend the tests don't fail; they simply become uninteresting noops because the broken ref isn't read at all). Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index 165cc80..0704f3e 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -18,18 +18,23 @@ test_expect_success 'disable reflogs' ' git reflog expire --expire=all --all ' +create_bogus_ref () { + test_when_finished 'rm -f .git/refs/heads/bogus..name' && + echo $bogus >.git/refs/heads/bogus..name +} + test_expect_success 'create history reachable only from a bogus-named ref' ' test_tick && git commit --allow-empty -m main && base=$(git rev-parse HEAD) && test_tick && git commit --allow-empty -m bogus && bogus=$(git rev-parse HEAD) && git cat-file commit $bogus >saved && - echo $bogus >.git/refs/heads/bogus..name && git reset --hard HEAD^ ' test_expect_success 'pruning does not drop bogus object' ' test_when_finished "git hash-object -w -t commit saved" && + create_bogus_ref && test_might_fail git prune --expire=now && git cat-file -e $bogus ' @@ -42,17 +47,13 @@ test_expect_success 'put bogus object into pack' ' ' test_expect_success 'destructive repack keeps packed object' ' + create_bogus_ref && test_might_fail git repack -Ad --unpack-unreachable=now && git cat-file -e $bogus && test_might_fail git repack -ad && git cat-file -e $bogus ' -# subsequent tests will have different corruptions -test_expect_success 'clean up bogus ref' ' - rm .git/refs/heads/bogus..name -' - # We create two new objects here, "one" and "two". Our # main branch points to "two", which is deleted, # corrupting the repository. But we'd like to make sure -- cgit v0.10.2-6-g49f6 From 078eecbcbe04bf77e8b9afee01a58c905c3b3c50 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:36:42 -0400 Subject: t5312: test non-destructive repack In t5312, we create a state with a broken ref, and then make sure that destructive repacks don't silently ignore the breakage (where a destructive repack is one that might drop objects). But we don't check the behavior of non-destructive repacks at all (i.e., ones where we'd keep unreachable objects). So let's add a test to confirm the current behavior, which is that they are allowed (i.e., ignoring the breakage and considering any objects it points to as unreachable). This may change in the future, but we'd like for the test suite to alert us to that fact. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index 0704f3e..c7010fb 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -46,6 +46,11 @@ test_expect_success 'put bogus object into pack' ' git cat-file -e $bogus ' +test_expect_success 'non-destructive repack ignores bogus name' ' + create_bogus_ref && + git repack -adk +' + test_expect_success 'destructive repack keeps packed object' ' create_bogus_ref && test_might_fail git repack -Ad --unpack-unreachable=now && -- cgit v0.10.2-6-g49f6 From 5b062e1f79b5121296678d91b2bbd032ca86a866 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:37:11 -0400 Subject: t5312: be more assertive about command failure When repacking or pruning in a corrupted repository, our tests in t5312 argue that it is OK to complete the operation or bail, as long as we don't actually delete the objects pointed to by the corruption. This isn't a wrong line of reasoning, but the tests are a bit permissive by using test_might_fail. The fact is that we _do_ bail currently, and if we ever stopped doing so, that would be worthy of a human investigating. So let's switch these to test_must_fail. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index c7010fb..1522a4b 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -7,6 +7,9 @@ if we see, for example, a ref with a bogus name, it is OK either to bail out or to proceed using it as a reachable tip, but it is _not_ OK to proceed as if it did not exist. Otherwise we might silently delete objects that cannot be recovered. + +Note that we do assert command failure in these cases, because that is +what currently happens. If that changes, these tests should be revisited. ' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME @@ -35,7 +38,7 @@ test_expect_success 'create history reachable only from a bogus-named ref' ' test_expect_success 'pruning does not drop bogus object' ' test_when_finished "git hash-object -w -t commit saved" && create_bogus_ref && - test_might_fail git prune --expire=now && + test_must_fail git prune --expire=now && git cat-file -e $bogus ' @@ -53,9 +56,9 @@ test_expect_success 'non-destructive repack ignores bogus name' ' test_expect_success 'destructive repack keeps packed object' ' create_bogus_ref && - test_might_fail git repack -Ad --unpack-unreachable=now && + test_must_fail git repack -Ad --unpack-unreachable=now && git cat-file -e $bogus && - test_might_fail git repack -ad && + test_must_fail git repack -ad && git cat-file -e $bogus ' @@ -83,7 +86,7 @@ test_expect_success 'create history with missing tip commit' ' test_expect_success 'pruning with a corrupted tip does not drop history' ' test_when_finished "git hash-object -w -t commit saved" && - test_might_fail git prune --expire=now && + test_must_fail git prune --expire=now && git cat-file -e $recoverable ' -- cgit v0.10.2-6-g49f6 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 diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 3155708..7b30910 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -249,6 +249,14 @@ int refs_rename_ref_available(struct ref_store *refs, #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 * * A reference iterator encapsulates the state of an in-progress @@ -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 v0.10.2-6-g49f6 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 diff --git a/refs.c b/refs.c index 8b9f7c3..c28bd6a 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 7b30910..2c4e173 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 v0.10.2-6-g49f6 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 diff --git a/refs/files-backend.c b/refs/files-backend.c index 74c0385..1148c0c 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 2c4e173..96911fb 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 v0.10.2-6-g49f6 From 6d751be4b66180679e8bc03fc22b14b4245067a8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:42:38 -0400 Subject: refs: omit dangling symrefs when using GIT_REF_PARANOIA Dangling symrefs aren't actually a corruption problem. It's perfectly fine for refs/remotes/origin/HEAD to point to an unborn branch. And in particular, if you are trying to establish reachability, a symref that points nowhere doesn't matter either way. Any ref it could point to will be examined during the rest of the traversal. It's possible that a symref pointing nowhere _could_ be a sign that the ref it was meant to point to was deleted accidentally (e.g., via corruption). But there is no particular reason to think that is true for any given case, and in the meantime, GIT_REF_PARANOIA kicking in automatically for some operations means they'll fail unnecessarily. So let's loosen it just a bit. The new test in t5312 shows off an example that is safe, but currently fails (and no longer does after this patch). Note that we don't do anything if the caller explicitly asked for DO_FOR_EACH_INCLUDE_BROKEN. In that case they may be looking for dangling symrefs themselves, and setting GIT_REF_PARANOIA should not _loosen_ things from what the caller asked for. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/refs.c b/refs.c index c28bd6a..e725108 100644 --- a/refs.c +++ b/refs.c @@ -1418,10 +1418,14 @@ struct ref_iterator *refs_ref_iterator_begin( { struct ref_iterator *iter; - if (ref_paranoia < 0) - ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); - if (ref_paranoia) - flags |= DO_FOR_EACH_INCLUDE_BROKEN; + if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) { + if (ref_paranoia < 0) + ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); + if (ref_paranoia) { + flags |= DO_FOR_EACH_INCLUDE_BROKEN; + flags |= DO_FOR_EACH_OMIT_DANGLING_SYMREFS; + } + } iter = refs->be->iterator_begin(refs, prefix, flags); diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index 1522a4b..d8ec5a7 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -62,6 +62,13 @@ test_expect_success 'destructive repack keeps packed object' ' git cat-file -e $bogus ' +test_expect_success 'destructive repack not confused by dangling symref' ' + test_when_finished "git symbolic-ref -d refs/heads/dangling" && + git symbolic-ref refs/heads/dangling refs/heads/does-not-exist && + git repack -ad && + test_must_fail git cat-file -e $bogus +' + # We create two new objects here, "one" and "two". Our # main branch points to "two", which is deleted, # corrupting the repository. But we'd like to make sure -- cgit v0.10.2-6-g49f6 From 968f12fdac2601086dea7e10db17f1c50d704a07 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:46:13 -0400 Subject: refs: turn on GIT_REF_PARANOIA by default The original point of the GIT_REF_PARANOIA flag was to include broken refs in iterations, so that possibly-destructive operations would not silently ignore them (and would generally instead try to operate on the oids and fail when the objects could not be accessed). We already turned this on by default for some dangerous operations, like "repack -ad" (where missing a reachability tip would mean dropping the associated history). But it was not on for general use, even though it could easily result in the spreading of corruption (e.g., imagine cloning a repository which simply omits some of its refs because their objects are missing; the result quietly succeeds even though you did not clone everything!). This patch turns on GIT_REF_PARANOIA by default. So a clone as mentioned above would actually fail (upload-pack tells us about the broken ref, and when we ask for the objects, pack-objects fails to deliver them). This may be inconvenient when working with a corrupted repository, but: - we are better off to err on the side of complaining about corruption, and then provide mechanisms for explicitly loosening safety. - this is only one type of corruption anyway. If we are missing any other objects in the history that _aren't_ ref tips, then we'd behave similarly (happily show the ref, but then barf when we started traversing). We retain the GIT_REF_PARANOIA variable, but simply default it to "1" instead of "0". That gives the user an escape hatch for loosening this when working with a corrupt repository. It won't work across a remote connection to upload-pack (because we can't necessarily set environment variables on the remote), but there the client has other options (e.g., choosing which refs to fetch). As a bonus, this also makes ref iteration faster in general (because we don't have to call has_object_file() for each ref), though probably not noticeably so in the general case. In a repo with a million refs, it shaved a few hundred milliseconds off of upload-pack's advertisement; that's noticeable, but most repos are not nearly that large. The possible downside here is that any operation which iterates refs but doesn't ever open their objects may now quietly claim to have X when the object is corrupted (e.g., "git rev-list new-branch --not --all" will treat a broken ref as uninteresting). But again, that's not really any different than corruption below the ref level. We might have refs/heads/old-branch as non-corrupt, but we are not actively checking that we have the entire reachable history. Or the pointed-to object could even be corrupted on-disk (but our "do we have it" check would still succeed). In that sense, this is merely bringing ref-corruption in line with general object corruption. One alternative implementation would be to actually check for broken refs, and then _immediately die_ if we see any. That would cause the "rev-list --not --all" case above to abort immediately. But in many ways that's the worst of all worlds: - it still spends time looking up the objects an extra time - it still doesn't catch corruption below the ref level - it's even more inconvenient; with the current implementation of GIT_REF_PARANOIA for something like upload-pack, we can make the advertisement and let the client choose a non-broken piece of history. If we bail as soon as we see a broken ref, they cannot even see the advertisement. The test changes here show some of the fallout. A non-destructive "git repack -adk" now fails by default (but we can override it). Deleting a broken ref now actually tells the hooks the correct "before" state, rather than a confusing null oid. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/Documentation/git.txt b/Documentation/git.txt index abace9e..d63c65e 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -867,15 +867,16 @@ for full details. end user, to be recorded in the body of the reflog. `GIT_REF_PARANOIA`:: - If set to `1`, include broken or badly named refs when iterating - over lists of refs. In a normal, non-corrupted repository, this - does nothing. However, enabling it may help git to detect and - abort some operations in the presence of broken refs. Git sets - this variable automatically when performing destructive - operations like linkgit:git-prune[1]. You should not need to set - it yourself unless you want to be paranoid about making sure - an operation has touched every ref (e.g., because you are - cloning a repository to make a backup). + If set to `0`, ignore broken or badly named refs when iterating + over lists of refs. Normally Git will try to include any such + refs, which may cause some operations to fail. This is usually + preferable, as potentially destructive operations (e.g., + linkgit:git-prune[1]) are better off aborting rather than + ignoring broken refs (and thus considering the history they + point to as not worth saving). The default value is `1` (i.e., + be paranoid about detecting and aborting all operations). You + should not normally need to set this to `0`, but it may be + useful when trying to salvage data from a corrupted repository. `GIT_ALLOW_PROTOCOL`:: If set to a colon-separated list of protocols, behave as if diff --git a/refs.c b/refs.c index e725108..ac19c68 100644 --- a/refs.c +++ b/refs.c @@ -1420,7 +1420,7 @@ struct ref_iterator *refs_ref_iterator_begin( if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) { if (ref_paranoia < 0) - ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0); + ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 1); if (ref_paranoia) { flags |= DO_FOR_EACH_INCLUDE_BROKEN; flags |= DO_FOR_EACH_OMIT_DANGLING_SYMREFS; diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index d8ec5a7..ea889c0 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -49,11 +49,17 @@ test_expect_success 'put bogus object into pack' ' git cat-file -e $bogus ' -test_expect_success 'non-destructive repack ignores bogus name' ' +test_expect_success 'non-destructive repack bails on bogus ref' ' create_bogus_ref && - git repack -adk + test_must_fail git repack -adk ' +test_expect_success 'GIT_REF_PARANOIA=0 overrides safety' ' + create_bogus_ref && + GIT_REF_PARANOIA=0 git repack -adk +' + + test_expect_success 'destructive repack keeps packed object' ' create_bogus_ref && test_must_fail git repack -Ad --unpack-unreachable=now && diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index b13553e..8212ca5 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -707,20 +707,21 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho test_expect_success 'deleting dangling ref triggers hooks with correct args' ' mk_test_with_hooks testrepo heads/branch && + orig=$(git -C testrepo rev-parse refs/heads/branch) && rm -f testrepo/.git/objects/??/* && git push testrepo :refs/heads/branch && ( cd testrepo/.git && cat >pre-receive.expect <<-EOF && - $ZERO_OID $ZERO_OID refs/heads/branch + $orig $ZERO_OID refs/heads/branch EOF cat >update.expect <<-EOF && - refs/heads/branch $ZERO_OID $ZERO_OID + refs/heads/branch $orig $ZERO_OID EOF cat >post-receive.expect <<-EOF && - $ZERO_OID $ZERO_OID refs/heads/branch + $orig $ZERO_OID refs/heads/branch EOF cat >post-update.expect <<-EOF && -- cgit v0.10.2-6-g49f6 From 5d1f5b8cd4bec8fbb405e32b1208955c93240f17 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:46:37 -0400 Subject: repack, prune: drop GIT_REF_PARANOIA settings Now that GIT_REF_PARANOIA is the default, we don't need to selectively enable it for destructive operations. In fact, it's harmful to do so, because it overrides any GIT_REF_PARANOIA=0 setting that the user may have provided (because they're trying to work around some corruption). With these uses gone, we can further clean up the ref_paranoia global, and make it a static variable inside the refs code. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/builtin/prune.c b/builtin/prune.c index 02c6ab7..485c9a3 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -143,7 +143,6 @@ int cmd_prune(int argc, const char **argv, const char *prefix) expire = TIME_MAX; save_commit_buffer = 0; read_replace_refs = 0; - ref_paranoia = 1; repo_init_revisions(the_repository, &revs, prefix); argc = parse_options(argc, argv, prefix, options, prune_usage, 0); diff --git a/builtin/repack.c b/builtin/repack.c index c1a2090..cb9f4bf 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -586,15 +586,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix) strvec_pushf(&cmd.args, "--unpack-unreachable=%s", unpack_unreachable); - strvec_push(&cmd.env_array, "GIT_REF_PARANOIA=1"); } else if (pack_everything & LOOSEN_UNREACHABLE) { strvec_push(&cmd.args, "--unpack-unreachable"); } else if (keep_unreachable) { strvec_push(&cmd.args, "--keep-unreachable"); strvec_push(&cmd.args, "--pack-loose-unreachable"); - } else { - strvec_push(&cmd.env_array, "GIT_REF_PARANOIA=1"); } } } else if (geometry) { diff --git a/cache.h b/cache.h index f6295f3..f445008 100644 --- a/cache.h +++ b/cache.h @@ -995,14 +995,6 @@ extern int core_apply_sparse_checkout; extern int core_sparse_checkout_cone; /* - * Include broken refs in all ref iterations, which will - * generally choke dangerous operations rather than letting - * them silently proceed without taking the broken ref into - * account. - */ -extern int ref_paranoia; - -/* * Returns the boolean value of $GIT_OPTIONAL_LOCKS (or the default value). */ int use_optional_locks(void); diff --git a/environment.c b/environment.c index b4ba4fa..7923ab2 100644 --- a/environment.c +++ b/environment.c @@ -31,7 +31,6 @@ int prefer_symlink_refs; int is_bare_repository_cfg = -1; /* unspecified */ int warn_ambiguous_refs = 1; int warn_on_object_refname_ambiguity = 1; -int ref_paranoia = -1; int repository_format_precious_objects; int repository_format_worktree_config; const char *git_commit_encoding; diff --git a/refs.c b/refs.c index ac19c68..d0f4e87 100644 --- a/refs.c +++ b/refs.c @@ -1419,6 +1419,8 @@ struct ref_iterator *refs_ref_iterator_begin( struct ref_iterator *iter; if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) { + static int ref_paranoia = -1; + if (ref_paranoia < 0) ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 1); if (ref_paranoia) { -- cgit v0.10.2-6-g49f6 From 1763334caf6c060f38b3310960b38cd3b1d54687 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:48:05 -0400 Subject: ref-filter: stop setting FILTER_REFS_INCLUDE_BROKEN Of the ref-filter callers, for-each-ref and git-branch both set the INCLUDE_BROKEN flag (but git-tag does not, which is a weird inconsistency). But now that GIT_REF_PARANOIA is on by default, that produces almost the same outcome for all three. The one exception is that GIT_REF_PARANOIA will omit dangling symrefs. That's a better behavior for these tools, as they would never include such a symref in the main output anyway (they can't, as it doesn't point to an object). Instead they issue a warning to stderr. But that warning is somewhat useless; a dangling symref is a perfectly reasonable thing to have in your repository, and is not a sign of corruption. It's much friendlier to just quietly ignore it. And in terms of robustness, the warning gains us little. It does not impact the exit code of either tool. So while the warning _might_ clue in a user that they have an unexpected broken symref, it would not help any kind of scripted use. This patch converts for-each-ref and git-branch to stop using the INCLUDE_BROKEN flag. That gives them more reasonable behavior, and harmonizes them with git-tag. We have to change one test to adapt to the situation. t1430 tries to trigger all of the REF_ISBROKEN behaviors from the underlying ref code. It uses for-each-ref to do so (because there isn't any other mechanism). That will no longer issue a warning about the symref which points to an invalid name, as it's considered dangling (and we can instead be sure that it's _not_ mentioned on stderr). Note that we do still complain about the illegally named "broken..symref"; its problem is not that it's dangling, but the name of the symref itself is illegal. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/builtin/branch.c b/builtin/branch.c index 03c7b72..0b7ed82 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -427,7 +427,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin memset(&array, 0, sizeof(array)); - filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN); + filter_refs(&array, filter, filter->kind); if (filter->verbose) maxwidth = calc_maxwidth(&array, strlen(remote_prefix)); diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 89cb630..642b4b8 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -77,7 +77,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) filter.name_patterns = argv; filter.match_as_path = 1; - filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN); + filter_refs(&array, &filter, FILTER_REFS_ALL); ref_array_sort(sorting, &array); if (!maxcount || array.nr < maxcount) diff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh index b1839e0..fa3aeb8 100755 --- a/t/t1430-bad-ref-name.sh +++ b/t/t1430-bad-ref-name.sh @@ -170,7 +170,7 @@ test_expect_success 'for-each-ref emits warnings for broken names' ' ! grep -e "badname" output && ! grep -e "broken\.\.\.symref" output && test_i18ngrep "ignoring ref with broken name refs/heads/broken\.\.\.ref" error && - test_i18ngrep "ignoring broken ref refs/heads/badname" error && + test_i18ngrep ! "ignoring broken ref refs/heads/badname" error && test_i18ngrep "ignoring ref with broken name refs/heads/broken\.\.\.symref" error ' -- cgit v0.10.2-6-g49f6 From 2d653c50364aeccb604f6b4680190824debf637a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:48:20 -0400 Subject: ref-filter: drop broken-ref code entirely Now that none of our callers passes the INCLUDE_BROKEN flag, we can drop it entirely, along with the code to plumb it through to the for_each_fullref_in() functions. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/ref-filter.c b/ref-filter.c index 93ce2a6..e59bb4c 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2405,13 +2405,10 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int { struct ref_filter_cbdata ref_cbdata; int ret = 0; - unsigned int broken = 0; ref_cbdata.array = array; ref_cbdata.filter = filter; - if (type & FILTER_REFS_INCLUDE_BROKEN) - broken = 1; filter->kind = type & FILTER_REFS_KIND_MASK; init_contains_cache(&ref_cbdata.contains_cache); @@ -2428,13 +2425,13 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int * of filter_ref_kind(). */ if (filter->kind == FILTER_REFS_BRANCHES) - ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata, broken); + ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata, 0); else if (filter->kind == FILTER_REFS_REMOTES) - ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, broken); + ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, 0); else if (filter->kind == FILTER_REFS_TAGS) - ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, broken); + ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, 0); else if (filter->kind & FILTER_REFS_ALL) - ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, broken); + ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, 0); if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD)) head_ref(ref_filter_handler, &ref_cbdata); } diff --git a/ref-filter.h b/ref-filter.h index c15dee8..b636f43 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -13,7 +13,6 @@ #define QUOTE_PYTHON 4 #define QUOTE_TCL 8 -#define FILTER_REFS_INCLUDE_BROKEN 0x0001 #define FILTER_REFS_TAGS 0x0002 #define FILTER_REFS_BRANCHES 0x0004 #define FILTER_REFS_REMOTES 0x0008 -- cgit v0.10.2-6-g49f6 From 67985e4e4aa85f11593b1aec35cf7cd7e9d02fba Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Sep 2021 14:48:48 -0400 Subject: refs: drop "broken" flag from for_each_fullref_in() No callers pass in anything but "0" here. Likewise to our sibling functions. Note that some of them ferry along the flag, but none of their callers pass anything but "0" either. Nor is anybody likely to change that. Callers which really want to see all of the raw refs use for_each_rawref(). And anybody interested in iterating a subset of the refs will likely be happy to use the now-default behavior of showing broken refs, but omitting dangling symlinks. So we can get rid of this whole feature. Signed-off-by: Jeff King Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 22c4e1a..8480a59 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -863,8 +863,8 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) continue; } if (!strcmp(arg, "--bisect")) { - for_each_fullref_in("refs/bisect/bad", show_reference, NULL, 0); - for_each_fullref_in("refs/bisect/good", anti_reference, NULL, 0); + for_each_fullref_in("refs/bisect/bad", show_reference, NULL); + for_each_fullref_in("refs/bisect/good", anti_reference, NULL); continue; } if (opt_with_value(arg, "--branches", &arg)) { diff --git a/ls-refs.c b/ls-refs.c index be09568..7fe9675 100644 --- a/ls-refs.c +++ b/ls-refs.c @@ -171,7 +171,7 @@ int ls_refs(struct repository *r, struct packet_reader *request) if (!data.prefixes.nr) strvec_push(&data.prefixes, ""); for_each_fullref_in_prefixes(get_git_namespace(), data.prefixes.v, - send_ref, &data, 0); + send_ref, &data); packet_fflush(stdout); strvec_clear(&data.prefixes); strbuf_release(&data.buf); diff --git a/ref-filter.c b/ref-filter.c index e59bb4c..e15f0c4 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2100,8 +2100,7 @@ static int filter_pattern_match(struct ref_filter *filter, const char *refname) */ static int for_each_fullref_in_pattern(struct ref_filter *filter, each_ref_fn cb, - void *cb_data, - int broken) + void *cb_data) { if (!filter->match_as_path) { /* @@ -2109,7 +2108,7 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter, * prefixes like "refs/heads/" etc. are stripped off, * so we have to look at everything: */ - return for_each_fullref_in("", cb, cb_data, broken); + return for_each_fullref_in("", cb, cb_data); } if (filter->ignore_case) { @@ -2118,16 +2117,16 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter, * so just return everything and let the caller * sort it out. */ - return for_each_fullref_in("", cb, cb_data, broken); + return for_each_fullref_in("", cb, cb_data); } if (!filter->name_patterns[0]) { /* no patterns; we have to look at everything */ - return for_each_fullref_in("", cb, cb_data, broken); + return for_each_fullref_in("", cb, cb_data); } return for_each_fullref_in_prefixes(NULL, filter->name_patterns, - cb, cb_data, broken); + cb, cb_data); } /* @@ -2425,13 +2424,13 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int * of filter_ref_kind(). */ if (filter->kind == FILTER_REFS_BRANCHES) - ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata, 0); + ret = for_each_fullref_in("refs/heads/", ref_filter_handler, &ref_cbdata); else if (filter->kind == FILTER_REFS_REMOTES) - ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata, 0); + ret = for_each_fullref_in("refs/remotes/", ref_filter_handler, &ref_cbdata); else if (filter->kind == FILTER_REFS_TAGS) - ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata, 0); + ret = for_each_fullref_in("refs/tags/", ref_filter_handler, &ref_cbdata); else if (filter->kind & FILTER_REFS_ALL) - ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata, 0); + ret = for_each_fullref_in_pattern(filter, ref_filter_handler, &ref_cbdata); if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD)) head_ref(ref_filter_handler, &ref_cbdata); } diff --git a/refs.c b/refs.c index d0f4e87..2be0d0f 100644 --- a/refs.c +++ b/refs.c @@ -1522,25 +1522,16 @@ int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data) return refs_for_each_ref_in(get_main_ref_store(the_repository), prefix, fn, cb_data); } -int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsigned int broken) +int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data) { - enum do_for_each_ref_flags flag = 0; - - if (broken) - flag = DO_FOR_EACH_INCLUDE_BROKEN; return do_for_each_ref(get_main_ref_store(the_repository), - prefix, fn, 0, flag, cb_data); + prefix, fn, 0, 0, cb_data); } int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix, - each_ref_fn fn, void *cb_data, - unsigned int broken) + each_ref_fn fn, void *cb_data) { - enum do_for_each_ref_flags flag = 0; - - if (broken) - flag = DO_FOR_EACH_INCLUDE_BROKEN; - return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data); + return do_for_each_ref(refs, prefix, fn, 0, 0, cb_data); } int for_each_replace_ref(struct repository *r, each_repo_ref_fn fn, void *cb_data) @@ -1632,8 +1623,7 @@ static void find_longest_prefixes(struct string_list *out, int for_each_fullref_in_prefixes(const char *namespace, const char **patterns, - each_ref_fn fn, void *cb_data, - unsigned int broken) + each_ref_fn fn, void *cb_data) { struct string_list prefixes = STRING_LIST_INIT_DUP; struct string_list_item *prefix; @@ -1648,7 +1638,7 @@ int for_each_fullref_in_prefixes(const char *namespace, for_each_string_list_item(prefix, &prefixes) { strbuf_addstr(&buf, prefix->string); - ret = for_each_fullref_in(buf.buf, fn, cb_data, broken); + ret = for_each_fullref_in(buf.buf, fn, cb_data); if (ret) break; strbuf_setlen(&buf, namespace_len); diff --git a/refs.h b/refs.h index 48970df..10e7696 100644 --- a/refs.h +++ b/refs.h @@ -342,10 +342,8 @@ int for_each_ref(each_ref_fn fn, void *cb_data); int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data); int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix, - each_ref_fn fn, void *cb_data, - unsigned int broken); -int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, - unsigned int broken); + each_ref_fn fn, void *cb_data); +int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data); /** * iterate all refs in "patterns" by partitioning patterns into disjoint sets @@ -354,8 +352,7 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, * callers should be prepared to ignore references that they did not ask for. */ int for_each_fullref_in_prefixes(const char *namespace, const char **patterns, - each_ref_fn fn, void *cb_data, - unsigned int broken); + each_ref_fn fn, void *cb_data); /** * iterate refs from the respective area. */ diff --git a/revision.c b/revision.c index 0dabb5a..b7a2baa 100644 --- a/revision.c +++ b/revision.c @@ -2548,7 +2548,7 @@ static int for_each_bisect_ref(struct ref_store *refs, each_ref_fn fn, struct strbuf bisect_refs = STRBUF_INIT; int status; strbuf_addf(&bisect_refs, "refs/bisect/%s", term); - status = refs_for_each_fullref_in(refs, bisect_refs.buf, fn, cb_data, 0); + status = refs_for_each_fullref_in(refs, bisect_refs.buf, fn, cb_data); strbuf_release(&bisect_refs); return status; } -- cgit v0.10.2-6-g49f6