| Message ID | e599ea6d335d16b45fa75b223ea8db93e46c627d.1683636885.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: introduce machine-parseable output | expand |
Patrick Steinhardt <ps@pks.im> writes: > But it is not intended when displaying the updated references and would > cause us to miss the left-hand side of the displayed reference update: > > ``` > $ git fetch origin HEAD:foo > From https://github.com/git/git > * [new ref] -> foo > ``` > The HEAD string is clearly missing from the left-hand side of the arrow, > which is further stressed by the point that the following commands show > the left-hand side as expected: > > ``` > $ git fetch origin HEAD > From https://github.com/git/git > * branch HEAD -> FETCH_HEAD I do not mind being explicit and showing HEAD in this case for the sake of consistency. But speaking for the past developers, it was deliberate to omit what is common from the output to make it more terse, IIRC, and I think it is unfair to call it a "BUG". Back when we wrote git-fetch-script, the output was a lot more verbose, and through efforts like 165f3902 (git-fetch: more terse fetch output, 2007-11-03) and numerous others over time, we got to the current output. > Note that this patch also changes formatting of the block that computes > the "kind" and "what" variables. This is done on purpose so that it is > part of the diff, hopefully making the change easier to comprehend. Just to help readers, "kind" is the category like branch, tag, etc. and "what" is the concrete name like 'master' and 'foo'. > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 08d7fc7233..6aecf549e8 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -918,12 +918,14 @@ static void display_ref_update(struct display_state *display_state, char code, > } > > width = (summary_width + strlen(summary) - gettext_width(summary)); > + remote = prettify_refname(remote); > + local = prettify_refname(local); > > strbuf_addf(&display_state->buf, " %c %-*s ", code, width, summary); > if (!display_state->compact_format) > - print_remote_to_local(display_state, remote, prettify_refname(local)); > + print_remote_to_local(display_state, remote, local); > else > - print_compact(display_state, remote, prettify_refname(local)); > + print_compact(display_state, remote, local); > if (error) > strbuf_addf(&display_state->buf, " (%s)", error); > strbuf_addch(&display_state->buf, '\n'); > @@ -934,7 +936,7 @@ static void display_ref_update(struct display_state *display_state, char code, > static int update_local_ref(struct ref *ref, > struct ref_transaction *transaction, > struct display_state *display_state, > - const char *remote, const struct ref *remote_ref, > + const struct ref *remote_ref, > int summary_width) > { > struct commit *current = NULL, *updated; > @@ -946,7 +948,7 @@ static int update_local_ref(struct ref *ref, > if (oideq(&ref->old_oid, &ref->new_oid)) { > if (verbosity > 0) > display_ref_update(display_state, '=', _("[up to date]"), NULL, > - remote, ref->name, summary_width); > + remote_ref->name, ref->name, summary_width); Makes sense. The variable "remote" (now removed) holds what to write to FETCH_HEAD to be used to formulate a merge message by the caller, but this function is purely to report the ref updates and has no need to have access to that information. > @@ -1252,14 +1254,13 @@ static int store_updated_refs(struct display_state *display_state, > if (!strcmp(rm->name, "HEAD")) { > kind = ""; > what = ""; > - } > - else if (skip_prefix(rm->name, "refs/heads/", &what)) > + } else if (skip_prefix(rm->name, "refs/heads/", &what)) { > kind = "branch"; > - else if (skip_prefix(rm->name, "refs/tags/", &what)) > + } else if (skip_prefix(rm->name, "refs/tags/", &what)) { > kind = "tag"; > - else if (skip_prefix(rm->name, "refs/remotes/", &what)) > + } else if (skip_prefix(rm->name, "refs/remotes/", &what)) { > kind = "remote-tracking branch"; > - else { > + } else { > kind = ""; > what = rm->name; > } This is a bit noisier than necessary. It took me a while until I realized that this hunk is a no-op. > @@ -1277,7 +1278,7 @@ static int store_updated_refs(struct display_state *display_state, > display_state->url_len); > > if (ref) { > - rc |= update_local_ref(ref, transaction, display_state, what, > + rc |= update_local_ref(ref, transaction, display_state, > rm, summary_width); > free(ref); Good. > @@ -1288,7 +1289,7 @@ static int store_updated_refs(struct display_state *display_state, > */ > display_ref_update(display_state, '*', > *kind ? kind : "branch", NULL, > - *what ? what : "HEAD", > + rm->name, > "FETCH_HEAD", summary_width); Good, too. The original cleared "what" and then to compensate for that had a logic to turn it back to "HEAD", but that is all gone by passing rm->name down. I think we could pass "rm" and leave it to display_ref_update() what string to use, if we wanted to further refine the output later. Then somebody in the future may even want to see "HEAD" to be shown as an empty string and that can all be done in display_ref_update(). It would fix the inconsistency that "git fetch origin HEAD" reports "HEAD -> FETCH_HEAD" by hiding "HEAD" just like the case where fetching "HEAD:foo" does, going in the other direction, I would think. Thanks.
On Tue, May 09, 2023 at 12:28:16PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > But it is not intended when displaying the updated references and would > > cause us to miss the left-hand side of the displayed reference update: > > > > ``` > > $ git fetch origin HEAD:foo > > From https://github.com/git/git > > * [new ref] -> foo > > ``` > > The HEAD string is clearly missing from the left-hand side of the arrow, > > which is further stressed by the point that the following commands show > > the left-hand side as expected: > > > > ``` > > $ git fetch origin HEAD > > From https://github.com/git/git > > * branch HEAD -> FETCH_HEAD > > I do not mind being explicit and showing HEAD in this case for the > sake of consistency. > > But speaking for the past developers, it was deliberate to omit what > is common from the output to make it more terse, IIRC, and I think > it is unfair to call it a "BUG". > > Back when we wrote git-fetch-script, the output was a lot more > verbose, and through efforts like 165f3902 (git-fetch: more terse > fetch output, 2007-11-03) and numerous others over time, we got to > the current output. That's fair. It's still not quite clear whether this behaviour is in fact intentional though. Quoting 165f3902 (git-fetch: more terse fetch output, 2007-11-03), the weird corner case is not documented: This makes the fetch output much more terse and prettier on a 80 column display, based on a consensus reached on the mailing list. Here's an example output: Receiving objects: 100% (5439/5439), 1.60 MiB | 636 KiB/s, done. Resolving deltas: 100% (4604/4604), done. From git://git.kernel.org/pub/scm/git/git ! [rejected] html -> origin/html (non fast forward) 136e631..f45e867 maint -> origin/maint (fast forward) 9850e2e..44dd7e0 man -> origin/man (fast forward) 3e4bb08..e3d6d56 master -> origin/master (fast forward) fa3665c..536f64a next -> origin/next (fast forward) + 4f6d9d6...768326f pu -> origin/pu (forced update) * [new branch] todo -> origin/todo I've reformulated the commit message to talk about an inconsistency instead of a bug. Patrick
diff --git a/builtin/fetch.c b/builtin/fetch.c index 08d7fc7233..6aecf549e8 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -918,12 +918,14 @@ static void display_ref_update(struct display_state *display_state, char code, } width = (summary_width + strlen(summary) - gettext_width(summary)); + remote = prettify_refname(remote); + local = prettify_refname(local); strbuf_addf(&display_state->buf, " %c %-*s ", code, width, summary); if (!display_state->compact_format) - print_remote_to_local(display_state, remote, prettify_refname(local)); + print_remote_to_local(display_state, remote, local); else - print_compact(display_state, remote, prettify_refname(local)); + print_compact(display_state, remote, local); if (error) strbuf_addf(&display_state->buf, " (%s)", error); strbuf_addch(&display_state->buf, '\n'); @@ -934,7 +936,7 @@ static void display_ref_update(struct display_state *display_state, char code, static int update_local_ref(struct ref *ref, struct ref_transaction *transaction, struct display_state *display_state, - const char *remote, const struct ref *remote_ref, + const struct ref *remote_ref, int summary_width) { struct commit *current = NULL, *updated; @@ -946,7 +948,7 @@ static int update_local_ref(struct ref *ref, if (oideq(&ref->old_oid, &ref->new_oid)) { if (verbosity > 0) display_ref_update(display_state, '=', _("[up to date]"), NULL, - remote, ref->name, summary_width); + remote_ref->name, ref->name, summary_width); return 0; } @@ -959,7 +961,7 @@ static int update_local_ref(struct ref *ref, */ display_ref_update(display_state, '!', _("[rejected]"), _("can't fetch into checked-out branch"), - remote, ref->name, summary_width); + remote_ref->name, ref->name, summary_width); return 1; } @@ -970,12 +972,12 @@ static int update_local_ref(struct ref *ref, 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, summary_width); + remote_ref->name, ref->name, summary_width); return r; } else { display_ref_update(display_state, '!', _("[rejected]"), _("would clobber existing tag"), - remote, ref->name, summary_width); + remote_ref->name, ref->name, summary_width); return 1; } } @@ -1008,7 +1010,7 @@ 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, summary_width); + remote_ref->name, ref->name, summary_width); return r; } @@ -1030,7 +1032,7 @@ static int update_local_ref(struct ref *ref, 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, summary_width); + remote_ref->name, ref->name, summary_width); strbuf_release(&quickref); return r; } else if (force || ref->force) { @@ -1042,12 +1044,12 @@ static int update_local_ref(struct ref *ref, 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, summary_width); + remote_ref->name, ref->name, summary_width); strbuf_release(&quickref); return r; } else { display_ref_update(display_state, '!', _("[rejected]"), _("non-fast-forward"), - remote, ref->name, summary_width); + remote_ref->name, ref->name, summary_width); return 1; } } @@ -1252,14 +1254,13 @@ static int store_updated_refs(struct display_state *display_state, if (!strcmp(rm->name, "HEAD")) { kind = ""; what = ""; - } - else if (skip_prefix(rm->name, "refs/heads/", &what)) + } else if (skip_prefix(rm->name, "refs/heads/", &what)) { kind = "branch"; - else if (skip_prefix(rm->name, "refs/tags/", &what)) + } else if (skip_prefix(rm->name, "refs/tags/", &what)) { kind = "tag"; - else if (skip_prefix(rm->name, "refs/remotes/", &what)) + } else if (skip_prefix(rm->name, "refs/remotes/", &what)) { kind = "remote-tracking branch"; - else { + } else { kind = ""; what = rm->name; } @@ -1277,7 +1278,7 @@ static int store_updated_refs(struct display_state *display_state, display_state->url_len); if (ref) { - rc |= update_local_ref(ref, transaction, display_state, what, + rc |= update_local_ref(ref, transaction, display_state, rm, summary_width); free(ref); } else if (write_fetch_head || dry_run) { @@ -1288,7 +1289,7 @@ static int store_updated_refs(struct display_state *display_state, */ display_ref_update(display_state, '*', *kind ? kind : "branch", NULL, - *what ? what : "HEAD", + rm->name, "FETCH_HEAD", summary_width); } } diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh index a09750d225..6e0f7e0046 100755 --- a/t/t5574-fetch-output.sh +++ b/t/t5574-fetch-output.sh @@ -56,6 +56,35 @@ test_expect_success 'fetch compact output' ' test_cmp expect actual ' +test_expect_success 'fetch output with HEAD' ' + test_when_finished "rm -rf head" && + git clone . head && + + git -C head fetch --dry-run origin HEAD >actual.out 2>actual.err && + cat >expect <<-EOF && + From $(test-tool path-utils real_path .)/. + * branch HEAD -> FETCH_HEAD + EOF + test_must_be_empty actual.out && + test_cmp expect actual.err && + + git -C head fetch origin HEAD >actual.out 2>actual.err && + test_must_be_empty actual.out && + test_cmp expect actual.err && + + git -C head fetch --dry-run origin HEAD:foo >actual.out 2>actual.err && + cat >expect <<-EOF && + From $(test-tool path-utils real_path .)/. + * [new ref] HEAD -> foo + EOF + test_must_be_empty actual.out && + test_cmp expect actual.err && + + git -C head fetch origin HEAD:foo >actual.out 2>actual.err && + test_must_be_empty actual.out && + test_cmp expect actual.err +' + test_expect_success '--no-show-forced-updates' ' mkdir forced-updates && (
`store_updated_refs()` parses the remote reference for two purposes: - It gets used as a note when writing FETCH_HEAD. - It is passed through to `display_ref_update()` to display updated references in the following format: ``` * branch master -> master ``` In most cases, the parsed remote reference is the prettified reference name and can thus be used for both cases. But if the remote reference is HEAD, the parsed remote reference becomes empty. This is intended when we write the FETCH_HEAD, where we skip writing the note in that case. But it is not intended when displaying the updated references and would cause us to miss the left-hand side of the displayed reference update: ``` $ git fetch origin HEAD:foo From https://github.com/git/git * [new ref] -> foo ``` The HEAD string is clearly missing from the left-hand side of the arrow, which is further stressed by the point that the following commands show the left-hand side as expected: ``` $ git fetch origin HEAD From https://github.com/git/git * branch HEAD -> FETCH_HEAD $ git fetch origin master From https://github.com/git/git * branch master -> FETCH_HEAD * branch master -> origin/master ``` The logic of how we compute the remote reference name that we ultimately pass to `display_ref_update()` is not easy to follow. There are three different cases here: - When the remote reference name is "HEAD" we set the remote reference name to the empty string. This is the case that causes the bug to occur, where we would indeed want to print "HEAD" instead of the empty string. This is what `prettify_refname()` would return. - When the remote reference name has a well-known prefix then we strip this prefix. This matches what `prettify_refname()` does. - Otherwise, we keep the fully qualified reference name. This also matches what `prettify_refname()` does. As the return value of `prettify_refname()` would do the correct thing for us in all three cases, we can fix the bug by passing through the full remote reference name to `display_ref_update()`, which learns to call `prettify_refname()`. At the same time, this also simplifies the code a bit. Note that this patch also changes formatting of the block that computes the "kind" and "what" variables. This is done on purpose so that it is part of the diff, hopefully making the change easier to comprehend. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/fetch.c | 37 +++++++++++++++++++------------------ t/t5574-fetch-output.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 18 deletions(-)