@matheusbsbs / matheus-bagliano / commits / 7b644c8c5a

rev-list: allow missing tips with --missing=[print|allow*]

In 9830926c7d (rev-list: add commit object support in `--missing` option, 2023-10-27) we fixed the `--missing` option in `git rev-list` so that it works with with missing commits, not just blobs/trees. Unfortunately, such a command would still fail with a "fatal: bad object <oid>" if it is passed a missing commit, blob or tree as an argument (before the rev walking even begins). When such a command is used to find the dependencies of some objects, for example the dependencies of quarantined objects (see the "QUARANTINE ENVIRONMENT" section in the git-receive-pack(1) documentation), it would be better if the command would instead consider such missing objects, especially commits, in the same way as other missing objects. If, for example `--missing=print` is used, it would be nice for some use cases if the missing tips passed as arguments were reported in the same way as other missing objects instead of the command just failing. We could introduce a new option to make it work like this, but most users are likely to prefer the command to have this behavior as the default one. Introducing a new option would require another dumb loop to look for that option early, which isn't nice. Also we made `git rev-list` work with missing commits very recently and the command is most often passed commits as arguments. So let's consider this as a bug fix related to these recent changes. While at it let's add a NEEDSWORK comment to say that we should get rid of the existing ugly dumb loops that parse the `--exclude-promisor-objects` and `--missing=...` options early. Helped-by: Linus Arver <linusa@google.com> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Christian Couder committed Feb 14, 2024 at 15:25 UTC 7b644c8c5a67cdda4a15ac91976eb29f5e29b796

4 files changed +88 -4

Documentation/rev-list-options.txt

@@ -1019,6 +1019,10 @@ Unexpected missing objects will raise an error.

1019 +

1020 The form '--missing=print' is like 'allow-any', but will also print a

1021 list of the missing objects. Object IDs are prefixed with a ``?'' character.

1022 ++

1023 +If some tips passed to the traversal are missing, they will be

1024 +considered as missing too, and the traversal will ignore them. In case

1025 +we cannot get their Object ID though, an error will be raised.

1026

1027 --exclude-promisor-objects::

1028 (For internal use only.) Prefilter object traversal at

builtin/rev-list.c

+17 -1

@@ -545,6 +545,18 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)

545 *

546 * Let "--missing" to conditionally set fetch_if_missing.

547 */

548 + /*

549 + * NEEDSWORK: These loops that attempt to find presence of

550 + * options without understanding that the options they are

551 + * skipping are broken (e.g., it would not know "--grep

552 + * --exclude-promisor-objects" is not triggering

553 + * "--exclude-promisor-objects" option). We really need

554 + * setup_revisions() to have a mechanism to allow and disallow

555 + * some sets of options for different commands (like rev-list,

556 + * replay, etc). Such a mechanism should do an early parsing

557 + * of options and be able to manage the `--missing=...` and

558 + * `--exclude-promisor-objects` options below.

559 + */

560 for (i = 1; i < argc; i++) {

561 const char *arg = argv[i];

562 if (!strcmp(arg, "--exclude-promisor-objects")) {

@@ -753,8 +765,12 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)

765

766 if (arg_print_omitted)

767 oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);

756 - if (arg_missing_action == MA_PRINT)

768 + if (arg_missing_action == MA_PRINT) {

769 oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);

770 + /* Add missing tips */

771 + oidset_insert_from_set(&missing_objects, &revs.missing_commits);

772 + oidset_clear(&revs.missing_commits);

773 + }

774

775 traverse_commit_list_filtered(

776 &revs, show_commit, show_object, &info,

revision.c

+11 -3

@@ -388,6 +388,10 @@ static struct object *get_reference(struct rev_info *revs, const char *name,

388 return NULL;

389 if (revs->exclude_promisor_objects && is_promisor_object(oid))

390 return NULL;

391 + if (revs->do_not_die_on_missing_objects) {

392 + oidset_insert(&revs->missing_commits, oid);

393 + return NULL;

394 + }

395 die("bad object %s", name);

396 }

397 object->flags |= flags;

@@ -1947,6 +1951,7 @@ void repo_init_revisions(struct repository *r,

1951 init_display_notes(&revs->notes_opt);

1952 list_objects_filter_init(&revs->filter);

1953 init_ref_exclusions(&revs->ref_excludes);

1954 + oidset_init(&revs->missing_commits, 0);

1955 }

1956

1957 static void add_pending_commit_list(struct rev_info *revs,

@@ -2178,13 +2183,18 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl

2183 if (revarg_opt & REVARG_COMMITTISH)

2184 get_sha1_flags |= GET_OID_COMMITTISH;

2185

2186 + /*

2187 + * Even if revs->do_not_die_on_missing_objects is set, we

2188 + * should error out if we can't even get an oid, as

2189 + * `--missing=print` should be able to report missing oids.

2190 + */

2191 if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))

2192 return revs->ignore_missing ? 0 : -1;

2193 if (!cant_be_filename)

2194 verify_non_filename(revs->prefix, arg);

2195 object = get_reference(revs, arg, &oid, flags ^ local_flags);

2196 if (!object)

2187 - return revs->ignore_missing ? 0 : -1;

2197 + return (revs->ignore_missing || revs->do_not_die_on_missing_objects) ? 0 : -1;

2198 add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);

2199 add_pending_object_with_path(revs, object, arg, oc.mode, oc.path);

2200 free(oc.path);

@@ -3830,8 +3840,6 @@ int prepare_revision_walk(struct rev_info *revs)

3840 FOR_EACH_OBJECT_PROMISOR_ONLY);

3841 }

3842

3833 - oidset_init(&revs->missing_commits, 0);

3834 -

3843 if (!revs->reflog_info)

3844 prepare_to_use_bloom_filter(revs);

3845 if (!revs->unsorted_input)

t/t6022-rev-list-missing.sh

+56

@@ -78,4 +78,60 @@ do

78 done

79 done

81 +for missing_tip in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"

82 +do

83 + # We want to check that things work when both

84 + # - all the tips passed are missing (case existing_tip = ""), and

85 + # - there is one missing tip and one existing tip (case existing_tip = "HEAD")

86 + for existing_tip in "" "HEAD"

87 + do

88 + for action in "allow-any" "print"

89 + do

90 + test_expect_success "--missing=$action with tip '$missing_tip' missing and tip '$existing_tip'" '

91 + oid="$(git rev-parse $missing_tip)" &&

92 + path=".git/objects/$(test_oid_to_path $oid)" &&

93 +

94 + # Before the object is made missing, we use rev-list to

95 + # get the expected oids.

96 + if test "$existing_tip" = "HEAD"

97 + then

98 + git rev-list --objects --no-object-names \

99 + HEAD ^$missing_tip >expect.raw

100 + else

101 + >expect.raw

102 + fi &&

103 +

104 + # Blobs are shared by all commits, so even though a commit/tree

105 + # might be skipped, its blob must be accounted for.

106 + if test "$existing_tip" = "HEAD" && test $missing_tip != "HEAD:1.t"

107 + then

108 + echo $(git rev-parse HEAD:1.t) >>expect.raw &&

109 + echo $(git rev-parse HEAD:2.t) >>expect.raw

110 + fi &&

111 +

112 + mv "$path" "$path.hidden" &&

113 + test_when_finished "mv $path.hidden $path" &&

114 +

115 + git rev-list --missing=$action --objects --no-object-names \

116 + $oid $existing_tip >actual.raw &&

117 +

118 + # When the action is to print, we should also add the missing

119 + # oid to the expect list.

120 + case $action in

121 + allow-any)

122 + ;;

123 + print)

124 + grep ?$oid actual.raw &&

125 + echo ?$oid >>expect.raw

126 + ;;

127 + esac &&

128 +

129 + sort actual.raw >actual &&

130 + sort expect.raw >expect &&

131 + test_cmp expect actual

132 + '

133 + done

134 + done

135 +done

136 +

137 test_done