Re: [PATCH] transport: no warning if no server wait-for-done

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Aug 06 2021, Jonathan Tan wrote:

> When the push.negotiate configuration variable was implemented in
> 477673d6f3 ("send-pack: support push negotiation", 2021-05-05), Git was
> taught to print warning messages whenever that variable was set to true
> but push negotiation didn't happen. However, the lack of push
> negotiation is more similar to things like the usage of protocol v2 - in
> which the new feature is desired for performance, but if the feature
> is not there, the user does not intend to take any action - than to
> things like the usage of --filter - in which the new feature is desired
> for a certain outcome, and if the outcome does not happen, there is a
> high chance that the user would need to do something else (in this case,
> for example, reclone with "--depth" if the user needs the disk space).
>
> Therefore, when pushing with push.negotiate set to true, do not warn if
> wait-for-done is not supported for any reason (e.g. if the server is
> using an older version of Git or if protocol v2 is deactivated on the
> server). This is done by using an internal-use-only parameter to "git
> fetch".

Tangentally related (the alternative was to start a thread on some
2018-era patch of yours): Is it intentional that when you supply a
gibberish OID or a nonexisting one as an explicit negotiation tip we
don't even warn about it?

Looking at the history of fetch-pack.c I suspect not. It goes back to
ec06283844a (fetch-pack: introduce negotiator API, 2018-06-14), i.e. the
"o && o->type == OBJ_COMMIT" check, now "if (c)" (as in could we look up
a commit) on "master". That in turn seems to go back as far as
9534f40bc42 (Be careful when dereferencing tags., 2005-11-02).

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/fetch.c       |  8 +++++++-
>  send-pack.c           | 11 +++--------
>  t/t5516-fetch-push.sh |  3 +--
>  transport.c           |  6 ++++--
>  transport.h           |  6 ++++++
>  5 files changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 25740c13df..940d650aba 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -84,6 +84,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
>  static int fetch_write_commit_graph = -1;
>  static int stdin_refspecs = 0;
>  static int negotiate_only;
> +static int negotiate_only_failure_ok;
>  
>  static int git_fetch_config(const char *k, const char *v, void *cb)
>  {
> @@ -208,6 +209,8 @@ static struct option builtin_fetch_options[] = {
>  			N_("report that we have only objects reachable from this object")),
>  	OPT_BOOL(0, "negotiate-only", &negotiate_only,
>  		 N_("do not fetch a packfile; instead, print ancestors of negotiation tips")),
> +	OPT_BOOL(0, "negotiate-only-failure-ok", &negotiate_only_failure_ok,
> +		 N_("for internal use only: if --negotiate-only fails, do not print a warning message")),
>  	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
>  	OPT_BOOL(0, "auto-maintenance", &enable_auto_gc,
>  		 N_("run 'maintenance --auto' after fetching")),
> @@ -2059,8 +2062,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		gtransport = prepare_transport(remote, 1);
>  		if (gtransport->smart_options) {
>  			gtransport->smart_options->acked_commits = &acked_commits;
> +			gtransport->smart_options->negotiate_only_failure_ok =
> +				negotiate_only_failure_ok;
>  		} else {
> -			warning(_("Protocol does not support --negotiate-only, exiting."));
> +			if (!negotiate_only_failure_ok)
> +				warning(_("Protocol does not support --negotiate-only, exiting."));
>  			return 1;

But we still want to "return 1" here and not proceed with the fetch?, ah
yes, because we run this via send-pack.c below...

>  		}
>  		if (server_options.nr)
> diff --git a/send-pack.c b/send-pack.c
> index 5a79e0e711..020fd0b265 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -424,7 +424,8 @@ static void get_commons_through_negotiation(const char *url,
>  	child.git_cmd = 1;
>  	child.no_stdin = 1;
>  	child.out = -1;
> -	strvec_pushl(&child.args, "fetch", "--negotiate-only", NULL);
> +	strvec_pushl(&child.args, "fetch", "--negotiate-only",
> +		     "--negotiate-only-failure-ok", NULL);
>  	for (ref = remote_refs; ref; ref = ref->next)
>  		strvec_pushf(&child.args, "--negotiation-tip=%s", oid_to_hex(&ref->new_oid));
>  	strvec_push(&child.args, url);
> @@ -447,13 +448,7 @@ static void get_commons_through_negotiation(const char *url,
>  		oid_array_append(commons, &oid);
>  	} while (1);
>  
> -	if (finish_command(&child)) {
> -		/*
> -		 * The information that push negotiation provides is useful but
> -		 * not mandatory.
> -		 */
> -		warning(_("push negotiation failed; proceeding anyway with push"));
> -	}
> +	finish_command(&child);
>  }
>  
>  int send_pack(struct send_pack_args *args,
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 0916f76302..60b377edf2 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -222,8 +222,7 @@ test_expect_success 'push with negotiation proceeds anyway even if negotiation f
>  	git -C testrepo config receive.hideRefs refs/remotes/origin/first_commit &&
>  	GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" \
>  		git -c push.negotiate=1 push testrepo refs/heads/main:refs/remotes/origin/main 2>err &&
> -	grep_wrote 5 event && # 2 commits, 2 trees, 1 blob
> -	test_i18ngrep "push negotiation failed" err
> +	grep_wrote 5 event # 2 commits, 2 trees, 1 blob
>  '
>  
>  test_expect_success 'push without wildcard' '
> diff --git a/transport.c b/transport.c
> index 17e9629710..913fc0f8e4 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -397,10 +397,12 @@ static int fetch_refs_via_pack(struct transport *transport,
>  
>  	if (data->options.acked_commits) {
>  		if (data->version < protocol_v2) {
> -			warning(_("--negotiate-only requires protocol v2"));
> +			if (!data->options.negotiate_only_failure_ok)
> +				warning(_("--negotiate-only requires protocol v2"));
>  			ret = -1;
>  		} else if (!server_supports_feature("fetch", "wait-for-done", 0)) {
> -			warning(_("server does not support wait-for-done"));
> +			if (!data->options.negotiate_only_failure_ok)
> +				warning(_("server does not support wait-for-done"));
>  			ret = -1;
>  		} else {
>  			negotiate_using_fetch(data->options.negotiation_tips,
> diff --git a/transport.h b/transport.h
> index 1cbab11373..98c90b46df 100644
> --- a/transport.h
> +++ b/transport.h
> @@ -53,6 +53,12 @@ struct git_transport_options {
>  	 * common commits to this oidset instead of fetching any packfiles.
>  	 */
>  	struct oidset *acked_commits;
> +
> +	/*
> +	 * If the server does not support wait-for-done, do not print any
> +	 * warning messages.
> +	 */
> +	unsigned negotiate_only_failure_ok : 1;
>  };
>  
>  enum transport_family {

I find myself wondering if a new option for this, or if --negotiate-only
shouldn't just pay attention to the existing --quiet and --verbose
options, depending. We already pass that down to this level through the
transport struct you're changing.

So since we're running a one-off special command here why not just pass
--quiet and check args.quiet in fetch_refs_via_pack() before emitting
the warning()? Ditto for fetch itself...