Christian Couder <christian.couder@xxxxxxxxx> writes:
> 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 now works with commits too.
>
> 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.
Hmm, but according to the manpage for rev-list, it only accepts commits
as arguments. Conflict...?
Also it took me a second to realize that you are talking about
missing objects passed in at the command line to `git rev-list` and not
the objects that this command encounters during the normal rev walking
process. It would have been a touch nicer to say something more
explicit, like
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 missing commits, not just blobs/trees.
Unfortunately, it would still fail with a "fatal: bad
object <oid>" if it is passed a missing commit 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, it would be
> better if the command would instead consider such missing objects,
> especially commits, in the same way as other missing objects.
Could you define what quarantined objects are (it's not in the manpage
for rev-list)? But also, I'm not sure how much additional value this
paragraph is adding on top of what you already said in the first two
paragraphs.
> 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.
>
> Let's introduce a new `--allow-missing-tips` option to make it work
> like this.
I assume "tips" means "the starting commits which are passed into this
command from which it begins the rev walk". Might be worth including in
the commit message.
But also, it's curious that you chose the word "allow" when we already
have "--ignore-missing". I assume this is because you still want the
missing tips to still be displayed in the output, and not ignored
(omitted from processing altogether). Perhaps "--report-missing-tips"
is more explicit?
> @@ -753,9 +765,19 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>
> if (arg_print_omitted)
> oidset_init(&omitted_objects, DEFAULT_OIDSET_SIZE);
> - if (arg_missing_action == MA_PRINT)
> + if (arg_missing_action == MA_PRINT) {
> + struct oidset_iter iter;
> + struct object_id *oid;
> +
> oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
>
> + /* Already add missing commits */
Did you mean "Add already-missing commits"? Perhaps even more explicit
would be "Add missing tips"?
> + oidset_iter_init(&revs.missing_commits, &iter);
> + while ((oid = oidset_iter_next(&iter)))
> + oidset_insert(&missing_objects, oid);
> + oidset_clear(&revs.missing_commits);
> + }
Aside: I am surprised that there is no helper function already that
simply copies things in one oidset into another oidset.
> traverse_commit_list_filtered(
> &revs, show_commit, show_object, &info,
> (arg_print_omitted ? &omitted_objects : NULL));
> diff --git a/revision.c b/revision.c
> index 4c5cd7c3ce..9f25faa249 100644
> --- a/revision.c
> +++ b/revision.c
>
> [...]
>
> @@ -2184,7 +2189,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl
> verify_non_filename(revs->prefix, arg);
> object = get_reference(revs, arg, &oid, flags ^ local_flags);
> if (!object)
> - return revs->ignore_missing ? 0 : -1;
> + return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;
> 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);
With a few more context lines, the diff looks like
--8<---------------cut here---------------start------------->8---
if (get_oid_with_context(revs->repo, arg, get_sha1_flags, &oid, &oc))
return revs->ignore_missing ? 0 : -1;
if (!cant_be_filename)
verify_non_filename(revs->prefix, arg);
object = get_reference(revs, arg, &oid, flags ^ local_flags);
if (!object)
- return revs->ignore_missing ? 0 : -1;
+ return (revs->ignore_missing || revs->do_not_die_on_missing_tips) ? 0 : -1;
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);
return 0;
--8<---------------cut here---------------end--------------->8---
and it's not obvious to me why you only touched the "if (!object)" case
but not the "if (get_oid_with_context(...))" case. Are there any
subtleties here that would not be obvious to reviewers?
> @@ -3830,8 +3835,6 @@ int prepare_revision_walk(struct rev_info *revs)
> FOR_EACH_OBJECT_PROMISOR_ONLY);
> }
>
> - oidset_init(&revs->missing_commits, 0);
> -
> if (!revs->reflog_info)
> prepare_to_use_bloom_filter(revs);
> if (!revs->unsorted_input)
> diff --git a/revision.h b/revision.h
> index 94c43138bc..67435a5d8a 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -227,6 +227,14 @@ struct rev_info {
> */
> do_not_die_on_missing_objects:1,
>
> + /*
> + * When the do_not_die_on_missing_objects flag above is set,
> + * a rev walk could still die with "fatal: bad object <oid>"
> + * if one of the tips it is passed is missing. With this flag
> + * such a tip will be reported as missing too.
> + */
> + do_not_die_on_missing_tips:1,
> +
> /* for internal use only */
> exclude_promisor_objects:1;
IIUC, we won't get to even do the rev walk though if all tips are
missing. So perhaps a more precise comment would be
/*
* When the do_not_die_on_missing_objects flag above is set,
* we could prematurely die with "fatal: bad object <oid>"
* if one of the tips it is passed is missing before even getting to
* the rev walk. With this flag such a tip will be reported as
* missing in the output after the rev walk completes.
*/
> diff --git a/t/t6022-rev-list-missing.sh b/t/t6022-rev-list-missing.sh
> index 527aa94f07..283e8fc2c2 100755
> --- a/t/t6022-rev-list-missing.sh
> +++ b/t/t6022-rev-list-missing.sh
> @@ -77,4 +77,55 @@ do
> done
> done
>
> +for obj in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
> +do
> + for tip in "" "HEAD"
So far from the patch diff it was not obvious that you wanted to check
for the empty string as a "tip".
> + do
> + for action in "allow-any" "print"
> + do
> + test_expect_success "--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'" '
If I run this test without the new --allow-missing-tips flag, I get
fatal: bad object 82335b23aa7234320d6f8055243c852e4b6a5ca3
not ok 11 - --missing=allow-any --allow-missing-tips with tip 'HEAD~1' missing and tip ''
and I think the last "tip ''" part is misleading because IIUC it's not
actually passed in as a tip (see my comment below about shell quoting).
> + oid="$(git rev-parse $obj)" &&
> + path=".git/objects/$(test_oid_to_path $oid)" &&
> +
> + # Before the object is made missing, we use rev-list to
> + # get the expected oids.
> + if [ "$tip" = "HEAD" ]; then
> + git rev-list --objects --no-object-names \
> + HEAD ^$obj >expect.raw
> + else
> + >expect.raw
> + fi &&
OK so you are using this empty string to clear the expect.raw file. If
that's true then what stops us from doing this at the start of every
iteration?
> + # Blobs are shared by all commits, so even though a commit/tree
> + # might be skipped, its blob must be accounted for.
> + if [ "$tip" = "HEAD" ] && [ $obj != "HEAD:1.t" ]; then
> + echo $(git rev-parse HEAD:1.t) >>expect.raw &&
> + echo $(git rev-parse HEAD:2.t) >>expect.raw
> + fi &&
> +
> + mv "$path" "$path.hidden" &&
> + test_when_finished "mv $path.hidden $path" &&
> +
> + git rev-list --missing=$action --allow-missing-tips \
> + --objects --no-object-names $oid $tip >actual.raw &&
The fact that $tip is not quoted here means that this is equivalent to
--objects --no-object-names $oid >actual.raw &&
and not
--objects --no-object-names $oid "" >actual.raw &&
for the case where tip is the empty string. I wonder if we could avoid
being nuanced here with subtle shell quoting rules, especially because
these are tests (where making everything as explicit as possible is
desirable).
> + # When the action is to print, we should also add the missing
> + # oid to the expect list.
> + case $action in
> + allow-any)
> + ;;
> + print)
> + grep ?$oid actual.raw &&
> + echo ?$oid >>expect.raw
> + ;;
> + esac &&
> +
> + sort actual.raw >actual &&
> + sort expect.raw >expect &&
> + test_cmp expect actual
> + '
> + done
> + done
> +done
> +
Wait, but where are we actually making the $tip commit's object
_missing_? Hmm...
Ah, actually the tips are just the $obj variables. So it seems like you
could have done
for tip in "HEAD~1" "HEAD~1^{tree}" "HEAD:1.t"
in the beginning to make it more obvious.
Also, given how most of this is identical from the loop already in t6022
just above it, it would help to add a comment at the top of this one
explaining how it's different than the one above it.
Thanks.
> test_done
> --
> 2.43.0.496.gd667eb0d7d.dirty
