fetch: delay user information post committing of transaction

In Git 2.50 and earlier, we would display failure codes and error
message as part of the status display:

  $ git fetch . v1.0.0:refs/heads/foo
    error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo'
    From .
     ! [new tag]               v1.0.0     -> foo  (unable to update local ref)

With the addition of batched updates, this information is no longer
shown to the user:

  $ git fetch . v1.0.0:refs/heads/foo
    From .
     * [new tag]               v1.0.0     -> foo
    error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo'

Since reference updates are batched and processed together at the end,
information around the outcome is not available during individual
reference parsing.

To overcome this, collate and delay the output to the end. Introduce
`ref_update_display_info` which will hold individual update's
information and also whether the update failed or succeeded. This
finally allows us to iterate over all such updates and print them to the
user. While this brings back the functionality, it does change the order
of the output. Modify the tests to reflect this.

Using an strmap does add some overhead to 'git-fetch(1)', but from
benchmarking this seems to be not too bad:

  Benchmark 1: fetch: many refs (refformat = files, refcount = 1000, revision = master)
    Time (mean ± σ):      51.9 ms ±   2.5 ms    [User: 15.6 ms, System: 36.9 ms]
    Range (min … max):    47.4 ms …  58.3 ms    41 runs

  Benchmark 2: fetch: many refs (refformat = files, refcount = 1000, revision = HEAD)
    Time (mean ± σ):      53.0 ms ±   1.8 ms    [User: 17.6 ms, System: 36.0 ms]
    Range (min … max):    49.4 ms …  57.6 ms    40 runs

  Summary
    fetch: many refs (refformat = files, refcount = 1000, revision = master) ran
      1.02 ± 0.06 times faster than fetch: many refs (refformat = files, refcount = 1000, revision = HEAD)

Another approach would be to move the status printing logic to be
handled post the transaction being committed. That however would require
adding an iterator to the ref transaction that tracks both the outcome
(success/failure) and the original refspec information for each update,
which is more involved infrastructure work compared to the strmap
approach here.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 49495be0b693fb010c17f548fb23af29f5ea6617..afe5d321d183883c56a44e7b395369af5b7c1413 100644 (file)
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -861,12 +861,77 @@ static void display_ref_update(struct display_state *display_state, char code,
        fputs(display_state->buf.buf, f);
 }
 
+struct ref_update_display_info {
+       bool failed;
+       char success_code;
+       char fail_code;
+       const char *summary;
+       const char *fail_detail;
+       const char *success_detail;
+       const char *remote;
+       const char *local;
+       struct object_id old_oid;
+       struct object_id new_oid;
+};
+
+static struct ref_update_display_info *ref_update_display_info_new(
+                                               char success_code,
+                                               char fail_code,
+                                               const char *summary,
+                                               const char *success_detail,
+                                               const char *fail_detail,
+                                               const char *remote,
+                                               const struct object_id *old_oid,
+                                               const struct object_id *new_oid)
+{
+       struct ref_update_display_info *info;
+       CALLOC_ARRAY(info, 1);
+
+       info->success_code = success_code;
+       info->fail_code = fail_code;
+       info->summary = xstrdup(summary);
+       info->success_detail = xstrdup_or_null(success_detail);
+       info->fail_detail = xstrdup_or_null(fail_detail);
+       info->remote = xstrdup(remote);
+
+       oidcpy(&info->old_oid, old_oid);
+       oidcpy(&info->new_oid, new_oid);
+
+       return info;
+}
+
+static void ref_update_display_info_set_failed(struct ref_update_display_info *info)
+{
+       info->failed = true;
+}
+
+static void ref_update_display_info_free(struct ref_update_display_info *info)
+{
+       free((char *)info->summary);
+       free((char *)info->success_detail);
+       free((char *)info->fail_detail);
+       free((char *)info->remote);
+}
+
+static void ref_update_display_info_display(struct ref_update_display_info *info,
+                                           struct display_state *display_state,
+                                           const char *refname, int summary_width)
+{
+       display_ref_update(display_state,
+                          info->failed ? info->fail_code : info->success_code,
+                          info->summary,
+                          info->failed ? info->fail_detail : info->success_detail,
+                          info->remote, refname, &info->old_oid,
+                          &info->new_oid, summary_width);
+}
+
 static int update_local_ref(struct ref *ref,
                            struct ref_transaction *transaction,
                            struct display_state *display_state,
                            const struct ref *remote_ref,
                            int summary_width,
-                           const struct fetch_config *config)
+                           const struct fetch_config *config,
+                           struct strmap *delayed_ref_display)
 {
        struct commit *current = NULL, *updated;
        int fast_forward = 0;
@@ -900,12 +965,19 @@ static int update_local_ref(struct ref *ref,
        if (!is_null_oid(&ref->old_oid) &&
            starts_with(ref->name, "refs/tags/")) {
                if (force || ref->force) {
+                       struct ref_update_display_info *info;
                        int r;
+
                        r = s_update_ref("updating tag", ref, transaction, 0);
-                       display_ref_update(display_state, r ? '!' : 't', _("[tag update]"),
-                                          r ? _("unable to update local ref") : NULL,
-                                          remote_ref->name, ref->name,
-                                          &ref->old_oid, &ref->new_oid, summary_width);
+
+                       info = ref_update_display_info_new('t', '!', _("[tag update]"), NULL,
+                                                          _("unable to update local ref"),
+                                                          remote_ref->name, &ref->old_oid,
+                                                          &ref->new_oid);
+                       if (r)
+                               ref_update_display_info_set_failed(info);
+                       strmap_put(delayed_ref_display, ref->name, info);
+
                        return r;
                } else {
                        display_ref_update(display_state, '!', _("[rejected]"),
@@ -921,6 +993,7 @@ static int update_local_ref(struct ref *ref,
        updated = lookup_commit_reference_gently(the_repository,
                                                 &ref->new_oid, 1);
        if (!current || !updated) {
+               struct ref_update_display_info *info;
                const char *msg;
                const char *what;
                int r;
@@ -941,10 +1014,15 @@ static int update_local_ref(struct ref *ref,
                }
 
                r = s_update_ref(msg, ref, transaction, 0);
-               display_ref_update(display_state, r ? '!' : '*', what,
-                                  r ? _("unable to update local ref") : NULL,
-                                  remote_ref->name, ref->name,
-                                  &ref->old_oid, &ref->new_oid, summary_width);
+
+               info = ref_update_display_info_new('*', '!', what, NULL,
+                                                  _("unable to update local ref"),
+                                                  remote_ref->name, &ref->old_oid,
+                                                  &ref->new_oid);
+               if (r)
+                       ref_update_display_info_set_failed(info);
+               strmap_put(delayed_ref_display, ref->name, info);
+
                return r;
        }
 
@@ -960,6 +1038,7 @@ static int update_local_ref(struct ref *ref,
        }
 
        if (fast_forward) {
+               struct ref_update_display_info *info;
                struct strbuf quickref = STRBUF_INIT;
                int r;
 
@@ -967,23 +1046,36 @@ static int update_local_ref(struct ref *ref,
                strbuf_addstr(&quickref, "..");
                strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
                r = s_update_ref("fast-forward", ref, transaction, 1);
-               display_ref_update(display_state, r ? '!' : ' ', quickref.buf,
-                                  r ? _("unable to update local ref") : NULL,
-                                  remote_ref->name, ref->name,
-                                  &ref->old_oid, &ref->new_oid, summary_width);
+
+               info = ref_update_display_info_new(' ', '!', quickref.buf, NULL,
+                                                  _("unable to update local ref"),
+                                                  remote_ref->name, &ref->old_oid,
+                                                  &ref->new_oid);
+               if (r)
+                       ref_update_display_info_set_failed(info);
+               strmap_put(delayed_ref_display, ref->name, info);
+
                strbuf_release(&quickref);
                return r;
        } else if (force || ref->force) {
+               struct ref_update_display_info *info;
                struct strbuf quickref = STRBUF_INIT;
                int r;
+
                strbuf_add_unique_abbrev(&quickref, &current->object.oid, DEFAULT_ABBREV);
                strbuf_addstr(&quickref, "...");
                strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
                r = s_update_ref("forced-update", ref, transaction, 1);
-               display_ref_update(display_state, r ? '!' : '+', quickref.buf,
-                                  r ? _("unable to update local ref") : _("forced update"),
-                                  remote_ref->name, ref->name,
-                                  &ref->old_oid, &ref->new_oid, summary_width);
+
+               info = ref_update_display_info_new('+', '!', quickref.buf,
+                                                  _("forced update"),
+                                                  _("unable to update local ref"),
+                                                  remote_ref->name, &ref->old_oid,
+                                                  &ref->new_oid);
+               if (r)
+                       ref_update_display_info_set_failed(info);
+               strmap_put(delayed_ref_display, ref->name, info);
+
                strbuf_release(&quickref);
                return r;
        } else {
@@ -1103,7 +1195,8 @@ static int store_updated_refs(struct display_state *display_state,
                              int connectivity_checked,
                              struct ref_transaction *transaction, struct ref *ref_map,
                              struct fetch_head *fetch_head,
-                             const struct fetch_config *config)
+                             const struct fetch_config *config,
+                             struct strmap *delayed_ref_display)
 {
        int rc = 0;
        struct strbuf note = STRBUF_INIT;
@@ -1219,7 +1312,8 @@ static int store_updated_refs(struct display_state *display_state,
 
                        if (ref) {
                                rc |= update_local_ref(ref, transaction, display_state,
-                                                      rm, summary_width, config);
+                                                      rm, summary_width, config,
+                                                      delayed_ref_display);
                                free(ref);
                        } else if (write_fetch_head || dry_run) {
                                /*
@@ -1300,7 +1394,8 @@ static int fetch_and_consume_refs(struct display_state *display_state,
                                  struct ref_transaction *transaction,
                                  struct ref *ref_map,
                                  struct fetch_head *fetch_head,
-                                 const struct fetch_config *config)
+                                 const struct fetch_config *config,
+                                 struct strmap *delayed_ref_display)
 {
        int connectivity_checked = 1;
        int ret;
@@ -1322,7 +1417,8 @@ static int fetch_and_consume_refs(struct display_state *display_state,
 
        trace2_region_enter("fetch", "consume_refs", the_repository);
        ret = store_updated_refs(display_state, connectivity_checked,
-                                transaction, ref_map, fetch_head, config);
+                                transaction, ref_map, fetch_head, config,
+                                delayed_ref_display);
        trace2_region_leave("fetch", "consume_refs", the_repository);
 
 out:
@@ -1493,7 +1589,8 @@ static int backfill_tags(struct display_state *display_state,
                         struct ref_transaction *transaction,
                         struct ref *ref_map,
                         struct fetch_head *fetch_head,
-                        const struct fetch_config *config)
+                        const struct fetch_config *config,
+                        struct strmap *delayed_ref_display)
 {
        int retcode, cannot_reuse;
 
@@ -1515,7 +1612,7 @@ static int backfill_tags(struct display_state *display_state,
        transport_set_option(transport, TRANS_OPT_DEPTH, "0");
        transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
        retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,
-                                        fetch_head, config);
+                                        fetch_head, config, delayed_ref_display);
 
        if (gsecondary) {
                transport_disconnect(gsecondary);
@@ -1641,6 +1738,7 @@ struct ref_rejection_data {
        bool conflict_msg_shown;
        bool case_sensitive_msg_shown;
        const char *remote_name;
+       struct strmap *delayed_ref_display;
 };
 
 static void ref_transaction_rejection_handler(const char *refname,
@@ -1653,6 +1751,7 @@ static void ref_transaction_rejection_handler(const char *refname,
                                              void *cb_data)
 {
        struct ref_rejection_data *data = cb_data;
+       struct ref_update_display_info *info;
 
        if (err == REF_TRANSACTION_ERROR_CASE_CONFLICT && ignore_case &&
            !data->case_sensitive_msg_shown) {
@@ -1681,6 +1780,10 @@ static void ref_transaction_rejection_handler(const char *refname,
                              refname, ref_transaction_error_msg(err));
        }
 
+       info = strmap_get(data->delayed_ref_display, refname);
+       if (info)
+               ref_update_display_info_set_failed(info);
+
        *data->retcode = 1;
 }
 
@@ -1690,6 +1793,7 @@ static void ref_transaction_rejection_handler(const char *refname,
  */
 static int commit_ref_transaction(struct ref_transaction **transaction,
                                  bool is_atomic, const char *remote_name,
+                                 struct strmap *delayed_ref_display,
                                  struct strbuf *err)
 {
        int retcode = ref_transaction_commit(*transaction, err);
@@ -1701,6 +1805,7 @@ static int commit_ref_transaction(struct ref_transaction **transaction,
                        .conflict_msg_shown = 0,
                        .remote_name = remote_name,
                        .retcode = &retcode,
+                       .delayed_ref_display = delayed_ref_display,
                };
 
                ref_transaction_for_each_rejected_update(*transaction,
@@ -1729,6 +1834,10 @@ static int do_fetch(struct transport *transport,
        struct fetch_head fetch_head = { 0 };
        struct strbuf err = STRBUF_INIT;
        int do_set_head = 0;
+       struct strmap delayed_ref_display = STRMAP_INIT;
+       int summary_width = 0;
+       struct strmap_entry *e;
+       struct hashmap_iter iter;
 
        if (tags == TAGS_DEFAULT) {
                if (transport->remote->fetch_tags == 2)
@@ -1853,7 +1962,7 @@ static int do_fetch(struct transport *transport,
        }
 
        if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
-                                  &fetch_head, config)) {
+                                  &fetch_head, config, &delayed_ref_display)) {
                retcode = 1;
                goto cleanup;
        }
@@ -1876,7 +1985,7 @@ static int do_fetch(struct transport *transport,
                         * the transaction and don't commit anything.
                         */
                        if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
-                                         &fetch_head, config))
+                                         &fetch_head, config, &delayed_ref_display))
                                retcode = 1;
                }
 
@@ -1886,8 +1995,12 @@ static int do_fetch(struct transport *transport,
        if (retcode)
                goto cleanup;
 
+       if (verbosity >= 0)
+               summary_width = transport_summary_width(ref_map);
+
        retcode = commit_ref_transaction(&transaction, atomic_fetch,
-                                        transport->remote->name, &err);
+                                        transport->remote->name,
+                                        &delayed_ref_display, &err);
        /*
         * With '--atomic', bail out if the transaction fails. Without '--atomic',
         * continue to fetch head and perform other post-fetch operations.
@@ -1965,7 +2078,17 @@ cleanup:
         */
        if (retcode && !atomic_fetch && transaction)
                commit_ref_transaction(&transaction, false,
-                                      transport->remote->name, &err);
+                                      transport->remote->name,
+                                      &delayed_ref_display, &err);
+
+       /*
+        * Clear any pending information that needs to be shown to the user.
+        */
+       strmap_for_each_entry(&delayed_ref_display, &iter, e) {
+               struct ref_update_display_info *info = e->value;
+               ref_update_display_info_display(info, &display_state, e->key, summary_width);
+               ref_update_display_info_free(info);
+       }
 
        if (retcode) {
                if (err.len) {
@@ -1980,6 +2103,8 @@ cleanup:
 
        if (transaction)
                ref_transaction_free(transaction);
+
+       strmap_clear(&delayed_ref_display, 1);
        display_state_release(&display_state);
        close_fetch_head(&fetch_head);
        strbuf_release(&err);

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 45595991c8d5fedf5fa7b2ffe199240a96aa862a..29e2f17608156127bc672374b6667d1bc010f353 100755 (executable)
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1893,6 +1893,7 @@ test_expect_success 'pushing non-commit objects should report error' '
 
                tagsha=$(git rev-parse test^{tag}) &&
                test_must_fail git push ../dest "$tagsha:refs/heads/branch" 2>err &&
+               test_grep "! \[remote rejected\] $tagsha -> branch (invalid new value provided)" err &&
                test_grep "trying to write non-commit object $tagsha to branch ${SQ}refs/heads/branch${SQ}" err
        )
 '

diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
index 5883839a04e991d6ab93a965698662ced064fe3d..22bfc0c74dec0adf216b952a3f3f0d7f39c082f2 100755 (executable)
--- a/t/t5574-fetch-output.sh
+++ b/t/t5574-fetch-output.sh
@@ -40,8 +40,8 @@ test_expect_success 'fetch aligned output' '
                grep -e "->" actual | cut -c 22- >../actual
        ) &&
        cat >expect <<-\EOF &&
-       main                 -> origin/main
        looooooooooooong-tag -> looooooooooooong-tag
+       main                 -> origin/main
        EOF
        test_cmp expect actual
 '
@@ -55,8 +55,8 @@ test_expect_success 'fetch compact output' '
                grep -e "->" actual | cut -c 22- >../actual
        ) &&
        cat >expect <<-\EOF &&
-       main       -> origin/*
        extraaa    -> *
+       main       -> origin/*
        EOF
        test_cmp expect actual
 '
@@ -103,15 +103,15 @@ do
                cat >expect <<-EOF &&
                - $MAIN_OLD $ZERO_OID refs/forced/deleted-branch
                - $MAIN_OLD $ZERO_OID refs/unforced/deleted-branch
-                 $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
                ! $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/unforced/force-updated
+               * $ZERO_OID $MAIN_OLD refs/forced/new-branch
+               * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
+               + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
+                 $MAIN_OLD $FAST_FORWARD_NEW refs/unforced/fast-forward
                * $ZERO_OID $MAIN_OLD refs/unforced/new-branch
                  $MAIN_OLD $FAST_FORWARD_NEW refs/forced/fast-forward
-               + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
-               * $ZERO_OID $MAIN_OLD refs/forced/new-branch
                  $MAIN_OLD $FAST_FORWARD_NEW refs/remotes/origin/fast-forward
-               + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/remotes/origin/force-updated
-               * $ZERO_OID $MAIN_OLD refs/remotes/origin/new-branch
+               + $FORCE_UPDATED_OLD $FORCE_UPDATED_NEW refs/forced/force-updated
                EOF
 
                # Change the URL of the repository to fetch different references.
@@ -179,8 +179,8 @@ test_expect_success 'fetch porcelain overrides fetch.output config' '
        new_commit=$(git rev-parse HEAD) &&
 
        cat >expect <<-EOF &&
-         $old_commit $new_commit refs/remotes/origin/config-override
        * $ZERO_OID $new_commit refs/tags/new-commit
+         $old_commit $new_commit refs/remotes/origin/config-override
        EOF
 
        git -C porcelain -c fetch.output=compact fetch --porcelain >stdout 2>stderr &&