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

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

[Jonathan Tan]

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".

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;
 		}
 		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 {
-- 
2.32.0.605.g8dce9f2422-goog

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

[Ævar Arnfjörð Bjarmason]

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...

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

[Jonathan Tan]

> 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).

This sounds unintentional, yes. Other than replying to an old email, you
could also write a top-level patch :-)

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> 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".

Hmph, if this were a hard error, instead of "print warning
messages", the above discussion is entirely reasonable.  But I am
not sure if squelching the warning unconditionally, especially given
that the feature is relatively young, is a good idea.

I suspect that you are trying to hide the "failed" message that may
sound more alarming then it should be from the end users, but if
that is the case, wouldn't it be a better solution to reword the
message to tone it down?

> -	if (finish_command(&child)) {
> -		/*
> -		 * The information that push negotiation provides is useful but
> -		 * not mandatory.
> -		 */
> -		warning(_("push negotiation failed; proceeding anyway with push"));
> -	}

Perhaps like "optional ancestry negotiation failed---pushing
normally" or some phrasing that assures the users that pushing
without negotiation is perfectly normal?

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

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:

>> -	if (finish_command(&child)) {
>> -		/*
>> -		 * The information that push negotiation provides is useful but
>> -		 * not mandatory.
>> -		 */
>> -		warning(_("push negotiation failed; proceeding anyway with push"));
>> -	}
>
> Perhaps like "optional ancestry negotiation failed---pushing
> normally" or some phrasing that assures the users that pushing
> without negotiation is perfectly normal?

The question is what the user will do with this information.

Will they contact the service provider to ask them to turn on push
negotiation?

Will they turn off push negotiation because they don't want to waste a
round trip?

Does what they will do depend on _why_ push negotiation failed?  If it
failed because the server didn't declare the capability and the user
has set push.negotate to true to represent "I want to live in the
future by using push negotiation wherever it's available", then the
message is noise.  If it failed due to a bug, then the message is more
relevant to the user --- e.g., should we use a different exit status
to distinguish between these two cases?

Thanks,
Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>>> -	if (finish_command(&child)) {
>>> -		/*
>>> -		 * The information that push negotiation provides is useful but
>>> -		 * not mandatory.
>>> -		 */
>>> -		warning(_("push negotiation failed; proceeding anyway with push"));
>>> -	}
>>
>> Perhaps like "optional ancestry negotiation failed---pushing
>> normally" or some phrasing that assures the users that pushing
>> without negotiation is perfectly normal?
>
> The question is what the user will do with this information.
>
> Will they contact the service provider to ask them to turn on push
> negotiation?
>
> Will they turn off push negotiation because they don't want to waste a
> round trip?
>
> Does what they will do depend on _why_ push negotiation failed?  If it
> failed because the server didn't declare the capability and the user
> has set push.negotate to true to represent "I want to live in the
> future by using push negotiation wherever it's available", then the
> message is noise.  If it failed due to a bug, then the message is more
> relevant to the user --- e.g., should we use a different exit status
> to distinguish between these two cases?

I was hoping that the "child" command being run there gives enough
clue before the warning message so that the user would already know
(that is where my "assures the users" primarily comes from---even we
cannot hide that the negotiation poll has failed, the users would be
helped by being told that it is OK).

If "child" does not give enough clue to this codepath (via the exit
code) or to the end user (via error messages), we probably would
want to fix that for that approach to work.

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

[Jonathan Tan]

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> >>> -	if (finish_command(&child)) {
> >>> -		/*
> >>> -		 * The information that push negotiation provides is useful but
> >>> -		 * not mandatory.
> >>> -		 */
> >>> -		warning(_("push negotiation failed; proceeding anyway with push"));
> >>> -	}
> >>
> >> Perhaps like "optional ancestry negotiation failed---pushing
> >> normally" or some phrasing that assures the users that pushing
> >> without negotiation is perfectly normal?
> >
> > The question is what the user will do with this information.
> >
> > Will they contact the service provider to ask them to turn on push
> > negotiation?
> >
> > Will they turn off push negotiation because they don't want to waste a
> > round trip?
> >
> > Does what they will do depend on _why_ push negotiation failed?  If it
> > failed because the server didn't declare the capability and the user
> > has set push.negotate to true to represent "I want to live in the
> > future by using push negotiation wherever it's available", then the
> > message is noise.  If it failed due to a bug, then the message is more
> > relevant to the user --- e.g., should we use a different exit status
> > to distinguish between these two cases?
> 
> I was hoping that the "child" command being run there gives enough
> clue before the warning message so that the user would already know
> (that is where my "assures the users" primarily comes from---even we
> cannot hide that the negotiation poll has failed, the users would be
> helped by being told that it is OK).
> 
> If "child" does not give enough clue to this codepath (via the exit
> code) or to the end user (via error messages), we probably would
> want to fix that for that approach to work.

The idea of this patch is to treat it like a user wanting to use
protocol v2 - if the remote doesn't support it, then the user is
perfectly fine falling back, and any extra message would be noise
because the user wouldn't take action on it anyway.

If we think that the user needs to know when push negotiation fails (for
example, if we think "the user enabled it so the user expects it to
happen"), then I agree that we should leave the warning messages in and
also print a message saying that push is still happening anyway (which
was the approach in the original patch set), and maybe update the
message to something like Junio suggested [1] to clarify what's going
on. But I would think that the user doesn't care - for example, I could
imagine someone globally enabling it and having pushes become
automatically faster as more and more remotes support it.

[1] https://lore.kernel.org/git/xmqqh7fyfrtl.fsf@gitster.g/