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

Jonathan Tan [off-list ref] 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?

Hi,

Junio C Hamano wrote:

Jonathan Tan [off-list ref] writes:

quoted

-	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