From: Karthik Nayak Date: Fri, 16 Jan 2026 21:27:12 +0000 (+0100) Subject: fetch: delay user information post committing of transaction X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ef73355076ff09346afd325cd3ff58edd84efd99;p=thirdparty%2Fgit.git 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 Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- diff --git a/builtin/fetch.c b/builtin/fetch.c index 49495be0b6..afe5d321d1 100644 --- 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, ¤t->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 45595991c8..29e2f17608 100755 --- 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 5883839a04..22bfc0c74d 100755 --- 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 &&