Re: [PATCH v3 2/2] revision: un-regress --exclude-promisor-objects
From: Emily Shaffer <hidden>
Date: 2020-03-25 20:50:43
On Sat, Jan 11, 2020 at 02:34:56PM -0800, Jonathan Tan wrote:
Before commit 4cf67869b2 ("list-objects.c: don't segfault for missing
cmdline objects", 2018-12-06),
git rev-list --exclude-promisor-objects $A_MISSING_PROMISOR_OBJECT
succeeds. But after that commit, this invocation produces a non-zero
result.
Restore this functionality: since get_reference() already does what we
need, we can just use its return value; skip the arg if the return value
is NULL, and use it otherwise (if the arg is invalid, get_reference()
already dies). With this commit, --exclude-promisor-objects treats both
promisor objects passed through the CLI and promisor objects found
through traversal in the same say: it excludes them, so it does not
matter whether they're missing or not.Since the return code is changing I'm kind of worried about what other impacts this will have. What's the call tree for handle_revision_arg look like? It looks like it's called in a couple places by revision.c and by builtin/pack-objects.c, but because of the way args are packed up in struct rev_info, it's hard to tell when those callers will be affected or not.
quoted hunk ↗ jump to hunk
--- revision.c | 2 +- t/t0410-partial-clone.sh | 12 +++--------- 2 files changed, 4 insertions(+), 10 deletions(-)diff --git a/revision.c b/revision.c index 91ca194388..0659a09b02 100644 --- a/revision.c +++ b/revision.c@@ -1917,7 +1917,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi verify_non_filename(revs->prefix, arg); object = get_reference(revs, arg, &oid, flags ^ local_flags); if (!object) - return revs->ignore_missing ? 0 : -1; + return 0; add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags); add_pending_object_with_path(revs, object, arg, oc.mode, oc.path); free(oc.path);diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index a3988bd4b8..b251985e82 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh@@ -397,7 +397,7 @@ test_expect_success 'rev-list stops traversal at promisor commit, tree, and blob grep $(git -C repo rev-parse bar) out # sanity check that some walking was done ' -test_expect_success 'rev-list dies for missing objects on cmd line' ' +test_expect_success 'rev-list accepts missing and promised objects on command line ' ' rm -rf repo && test_create_repo repo && test_commit -C repo foo &&@@ -416,15 +416,9 @@ test_expect_success 'rev-list dies for missing objects on cmd line' ' git -C repo config extensions.partialclone "arbitrary string" && for OBJ in "$COMMIT" "$TREE" "$BLOB"; do - test_must_fail git -C repo rev-list --objects \ + git -C repo rev-list --objects \ --exclude-promisor-objects "$OBJ" && - test_must_fail git -C repo rev-list --objects-edge-aggressive \ - --exclude-promisor-objects "$OBJ" && - - # Do not die or crash when --ignore-missing is passed. - git -C repo rev-list --ignore-missing --objects \ - --exclude-promisor-objects "$OBJ" && - git -C repo rev-list --ignore-missing --objects-edge-aggressive \ + git -C repo rev-list --objects-edge-aggressive \ --exclude-promisor-objects "$OBJ"
It seems to me the -ignore-missing tests should still pass and therefore shouldn't be removed, no? But then I looked a little harder, and it looks like before the test said, "This call fails, but with --ignore-missing it does not fail" - and now the test just says "This call does not fail". So it looks OK to me.
done ' -- 2.25.0.rc1.283.g88dfdc4193-goog