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