Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> 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.
>
> 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.
>
> 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.

OK.  Unlike a missing object referenced by a tree, a commit, or a
tag, where the expected type of the missing object is known to Git,
I would expect that nobody knowsn what type these missing objects at
the tip have.  So do we now require "object X missing" instead of
"commit X missing" in the output?  If we are not giving any type
information even when we know what the type we expect is, then we do
not have to worry about this change introducing a new output logic,
I guess.

> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index b3f4783858..ae7bb15478 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -562,6 +562,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  				break;
>  		}
>  	}
> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +		if (!strcmp(arg, "--allow-missing-tips")) {
> +			if (arg_missing_action == MA_ERROR)
> +				die(_("option '%s' only makes sense with '%s' set to '%s' or '%s'"),
> +				      "--allow-missing-tips", "--missing=", "allow-*", "print");
> +			revs.do_not_die_on_missing_tips = 1;
> +			break;
> +		}
> +	}

It is unfortunate that we need to add yet another dumb loop that
does not even try to understand there may be an option whose value
happens to be the string strcmp() looks for (we already have two
such loops above this hunk).  I have to wonder if we can do better.

An idle piece of idea.  Perhaps we can instruct setup_revisions()
not to die immediately upon seeing a problematic argument, marking
the revs as "broken" instead, and keep going and interpreting as
much as it could, so that it may record the presence of "--missing",
"--exclude-promisor-objects", and "--allow-missing-tips".  Then upon
seeing setup_revisions() return with such an error, we can redo the
call with these bits already on.

Anyway, I digress.

> 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"
> +	do
> +		for action in "allow-any" "print"
> +		do
> +			test_expect_success "--missing=$action --allow-missing-tips with tip '$obj' missing and tip '$tip'" '
> +				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

Style:

                                if test "$tip" = "HEAD"
                                then

> +					git rev-list --objects --no-object-names \
> +						HEAD ^$obj >expect.raw
> +				else
> +					>expect.raw
> +				fi &&
> +
> +				# 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

Ditto.

> +					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 &&
> +
> +				# 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
> +					;;

OK.  We do not say anything more than the object name (and the fact
that it is missing with a single byte '?'), so my earlier worry was
unfounded.  Good.

> +				esac &&
> +
> +				sort actual.raw >actual &&
> +				sort expect.raw >expect &&
> +				test_cmp expect actual
> +			'
> +		done
> +	done
> +done
> +
>  test_done

THanks.