Re: [PATCH v2 4/4] rev-list: allow missing tips with --missing=[print|allow*]

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2024, 17:44 UTC
Message-ID: <xmqqeddmonq0.fsf@gitster.g>
In-Reply-To: <20240208135055.2705260-5-christian.couder@gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 6 quoted lines

>  The form '--missing=print' is like 'allow-any', but will also print a
>  list of the missing objects.  Object IDs are prefixed with a ``?'' character.
> ++
> +If some tips passed to the traversal are missing, they will be
> +considered as missing too, and the traversal will ignore them. In case
> +we cannot get their Object ID though, an error will be raised.

Makes sense.

Show 11 quoted lines

> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index b3f4783858..ec9556f135 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -545,6 +545,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  	 *
>  	 * Let "--missing" to conditionally set fetch_if_missing.
>  	 */
> +	/*
> +	 * NEEDSWORK: These dump loops to look for some options early
> +	 * are ugly. We really need setup_revisions() to have a

I would drop the "dump" and "ugly" from here. It OK to make value judgement with such words in casual conversations on a proposed patch, but when we make a request to future developers to fix our mess, we should be more objective and stick to the techincal facts, so that they have better understanding on why we think this area needs more work.

Perhaps something like:

    These loops that attempt to find presence of options without
    understanding what the options they are skipping are broken
    (e.g., it would not know "--grep --exclude-promisor-objects" is
    not triggering "--exclude-promisor-objects" option).

Everything after "We really need" is good (modulo possible grammos), I think. Thanks for writing it.

Show 20 quoted lines

> +	 * mechanism to allow and disallow some sets of options for
> +	 * different commands (like rev-list, replay, etc). Such
> +	 * mechanism should do an early parsing of option and be able
> +	 * to manage the `--exclude-promisor-objects` and `--missing=...`
> +	 * options below.
> +	 */
>  	for (i = 1; i < argc; i++) {
>  		const char *arg = argv[i];
>  		if (!strcmp(arg, "--exclude-promisor-objects")) {
> @@ -753,8 +762,12 @@ 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) {
>  		oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE);
> +		/* Already add missing tips */
> +		oidset_insert_from_set(&missing_objects, &revs.missing_commits);
> +		oidset_clear(&revs.missing_commits);
> +	}

It is unclear what "already" here refers to, at least to me.

Thanks.