Re: [PATCH v2] builtin/fetch: skip unnecessary tasks when using --negotiate-only

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 17, 2021, 23:35 UTC
Message-ID: <xmqqilvm24bb.fsf@gitster.g>
In-Reply-To: <20211217000235.68996-1-chooglen@google.com>

Glen Choo <chooglen@google.com> writes:

Show 8 quoted lines

> cmd_fetch() performs certain tasks with the assumption that objects are
> fetched, but `git fetch --negotiate-only` does not fetch objects, as its
> name implies. This is results in behavior that is unnecessary at best,
> and incorrect at worst:
>
> * Submodules are updated if enabled by recurse.submodules=true, but
>   negotiation fetch doesn't actually update the repo, so this doesn't
>   make sense (introduced in [1]).

Hmph.

> * Commit graphs will be written if enabled by
>   fetch.writeCommitGraph=true. But according to
>   Documentation/config/fetch.txt [2], this should only be done if a
>   pack-file is downloaded.

Makes sense, as we haven't changed any reachability, and we have no need to rewrite the graph file.

> * gc is run, but according to [3], we only do this because we expect
>   `git fetch` to introduce objects.

Makes sense. As we haven't added any new objects, there is nothing (other than the passage of time) that adds to the need to collect garbage.

It makes me wonder if we need to do anything upon "fetch --dry-run". I know we add to the object store without making anything reachable, so that the user can do pre-flight checks with the real objects. We do not change the reachability so there is no reason to rewrite the graph file, but we do add cruft to the object store.

Doing something about "--dry-run" is obviously outside the scope of this topic, but it may make sense to think about it while we are thinking about "fetch".

Show 19 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index f7abbc31ff..85091af99b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1996,6 +1996,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_fetch_options, builtin_fetch_usage, 0);
> +
> +	if (negotiate_only) {
> +		/*
> +		 * --negotiate-only should never recurse into
> +		 * submodules, so there is no need to read .gitmodules.
> +		 */
> +		recurse_submodules = RECURSE_SUBMODULES_OFF;
> +		if (!negotiation_tip.nr)
> +			die(_("--negotiate-only needs one or more --negotiate-tip=*"));
> +	}
> +

This means "fetch --negotiate-only --recurse-submodules" silently ignores an explicit wish by the user.

I suspect that this part should be more like this.

	if (negitiate_only &&
	    recurse_submodules != RECURSE_SUBMODULES_OFF) {
		if (recurse_submodules came from the parse_options)
			die(_("'--%s' cannot be used with '--%s'",
			      "recurse-submodules", "negotiate-only"));
		recurse_submodules = RECURSE_SUBMODULES_OFF;
	}

That is, we complain if user gives us a combination we do not support, but we are OK if the configuration is set to do so and silently ignore (because we declare that the combination does not make sense).

By the way, do not move the check about the number of negotiation tips from the original location. That check, or its location, have nothing to do with what you want to do in this patch, which is "do not gc or update the graph file if we are not fetching". It is better to leave unrelated changes out of the patch.

In order to tell if recurse_submodules that is not OFF came from the call to parse_options(), you may need to capture the value of the variable before calling parse_options() and compare it with the current value in the above illustration code snippet I gave.

Having said all that, is it true that recurse-submodules should not be combined with negotiate-only? I naively think it would not be surprising if users expect negotiate-only fetches are done also in the submodules. Whatever we decide the right behaviour should be, we should document it. With your patch without any of my above input, I would expect at least something like

    diff --git i/Documentation/fetch-options.txt w/Documentation/fetch-options.txt
    index e967ff1874..baf2e9c50d 100644
    --- i/Documentation/fetch-options.txt
    +++ w/Documentation/fetch-options.txt
    @@ -73,6 +73,9 @@ configuration variables documented in linkgit:git-config[1], and the
     +
     Internally this is used to implement the `push.negotiate` option, see
     linkgit:git-config[1].
    ++
    +Note that this option silently makes various options that do not make
    +sense to be used together with it (e.g. `--recurse-submodules`) ignored.
     
     --dry-run::
            Show what would be done, without making any changes.

to leave wiggling room for us to silently ignore more. We may know about --recurse-submodules today, but I would not be surprised if we find more.

Show 17 quoted lines

> @@ -2112,6 +2120,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		result = fetch_multiple(&list, max_children);
>  	}
>  
> +	string_list_clear(&list, 0);
> +
> +	/*
> +	 * Skip irrelevant tasks because we know objects were not
> +	 * fetched.
> +	 *
> +	 * NEEDSWORK: as a future optimization, we can return early
> +	 * whenever objects were not fetched e.g. if we already have all
> +	 * of them.
> +	 */
> +	if (negotiate_only)
> +		return result;
> +

I find it somewhat misleading to have the early return before the block for recurse_submodules, as we _are_ already forcing it to not to recurse. It would be more readable if it went before the place where we start doing the post-action clean-ups like reachability graphs and garbage collection.

Show 9 quoted lines

>  	if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
>  		struct strvec options = STRVEC_INIT;
>  		int max_children = max_jobs;
> @@ -2132,8 +2153,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		strvec_clear(&options);
>  	}
>  
> -	string_list_clear(&list, 0);
> -

Namely, here.

>  	prepare_repo_settings(the_repository);

This is existing code, but I wonder why it can be done _SO_ late in the sequence. We've already called the transport API for the negotiate-only communication at this point, but a call to this function is the only thing that gives fetch_negotiation_algorithm member in the_repository its default value, isn't it?

Thanks.