From: Junio C Hamano <gitster@pobox.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org,
Felipe Contreras <felipe.contreras@gmail.com>,
Glen Choo <chooglen@google.com>,
Jonathan Tan <jonathantanmy@google.com>,
Jacob Keller <jacob.e.keller@intel.com>
Subject: Re: [PATCH v4 4/8] fetch: fix missing from-reference when fetching HEAD:foo
Date: Tue, 09 May 2023 12:28:16 -0700 [thread overview]
Message-ID: <xmqqednpxq27.fsf@gitster.g> (raw)
In-Reply-To: <e599ea6d335d16b45fa75b223ea8db93e46c627d.1683636885.git.ps@pks.im> (Patrick Steinhardt's message of "Tue, 9 May 2023 15:02:15 +0200")
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.
next prev parent reply other threads:[~2023-05-09 19:28 UTC|newest]
Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-19 12:31 [PATCH 0/8] fetch: introduce machine-parseable output Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 1/8] fetch: split out tests for output format Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 2/8] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 3/8] fetch: fix missing from-reference when fetching HEAD:foo Patrick Steinhardt
2023-04-26 19:20 ` Jacob Keller
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-26 19:21 ` Jacob Keller
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-26 19:25 ` Glen Choo
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 4/8] fetch: introduce `display_format` enum Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 5/8] fetch: move display format parsing into main function Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 6/8] fetch: move option related variables " Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 7/8] fetch: introduce new `--output-format` option Patrick Steinhardt
2023-04-26 19:40 ` Glen Choo
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-19 12:31 ` [PATCH 8/8] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-04-26 19:52 ` Glen Choo
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-27 23:20 ` Glen Choo
2023-04-28 8:51 ` Patrick Steinhardt
2023-04-28 17:20 ` Glen Choo
2023-05-02 20:55 ` Felipe Contreras
2023-04-24 20:17 ` [PATCH 0/8] fetch: introduce machine-parseable output Felipe Contreras
2023-04-25 9:58 ` Patrick Steinhardt
2023-04-26 19:14 ` Jacob Keller
2023-04-26 20:23 ` Junio C Hamano
2023-04-26 20:30 ` Jacob Keller
2023-04-27 10:58 ` Patrick Steinhardt
2023-04-27 19:46 ` Jacob Keller
2023-04-27 22:49 ` Glen Choo
2023-04-26 20:24 ` Junio C Hamano
2023-04-26 18:54 ` Glen Choo
2023-04-26 21:14 ` Glen Choo
2023-04-26 19:17 ` Jacob Keller
2023-04-27 11:13 ` [PATCH v2 " Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 1/8] fetch: split out tests for output format Patrick Steinhardt
2023-04-29 17:34 ` SZEDER Gábor
2023-05-03 11:21 ` Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 2/8] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 3/8] fetch: fix missing from-reference when fetching HEAD:foo Patrick Steinhardt
2023-04-27 17:26 ` Glen Choo
2023-04-27 19:49 ` Jacob Keller
2023-04-27 11:13 ` [PATCH v2 4/8] fetch: introduce `display_format` enum Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 5/8] fetch: move display format parsing into main function Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 6/8] fetch: move option related variables " Patrick Steinhardt
2023-04-27 21:52 ` Junio C Hamano
2023-04-27 11:13 ` [PATCH v2 7/8] fetch: introduce new `--output-format` option Patrick Steinhardt
2023-04-27 22:01 ` Junio C Hamano
2023-04-28 22:03 ` Glen Choo
2023-05-03 9:12 ` Patrick Steinhardt
2023-04-28 22:31 ` Glen Choo
2023-05-03 9:43 ` Patrick Steinhardt
2023-05-03 11:36 ` Patrick Steinhardt
2023-04-27 11:13 ` [PATCH v2 8/8] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-04-27 19:52 ` Jacob Keller
2023-04-28 22:42 ` Glen Choo
2023-05-03 11:34 ` [PATCH v3 0/8] fetch: introduce machine-parseable output Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 1/8] fetch: fix `--no-recurse-submodules` with multi-remote fetches Patrick Steinhardt
2023-05-08 22:51 ` Glen Choo
2023-05-09 12:41 ` Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 2/8] fetch: split out tests for output format Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 3/8] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 4/8] fetch: fix missing from-reference when fetching HEAD:foo Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 5/8] fetch: introduce `display_format` enum Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 6/8] fetch: move display format parsing into main function Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 7/8] fetch: move option related variables " Patrick Steinhardt
2023-05-03 11:34 ` [PATCH v3 8/8] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-05-08 23:42 ` Glen Choo
2023-05-09 12:41 ` Patrick Steinhardt
2023-05-09 0:03 ` Glen Choo
2023-05-03 16:48 ` [PATCH v3 0/8] fetch: introduce machine-parseable output Junio C Hamano
2023-05-03 16:53 ` Junio C Hamano
2023-05-04 7:57 ` Patrick Steinhardt
2023-05-09 0:06 ` Glen Choo
2023-05-09 12:42 ` Patrick Steinhardt
2023-05-09 13:01 ` [PATCH v4 " Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 1/8] fetch: fix `--no-recurse-submodules` with multi-remote fetches Patrick Steinhardt
2023-05-09 17:49 ` Junio C Hamano
2023-05-09 18:27 ` Glen Choo
2023-05-10 12:34 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 2/8] fetch: split out tests for output format Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 3/8] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-05-09 17:58 ` Junio C Hamano
2023-05-10 12:34 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 4/8] fetch: fix missing from-reference when fetching HEAD:foo Patrick Steinhardt
2023-05-09 19:28 ` Junio C Hamano [this message]
2023-05-10 12:34 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 5/8] fetch: introduce `display_format` enum Patrick Steinhardt
2023-05-09 20:19 ` Junio C Hamano
2023-05-10 12:35 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 6/8] fetch: move display format parsing into main function Patrick Steinhardt
2023-05-09 20:35 ` Junio C Hamano
2023-05-09 22:30 ` Glen Choo
2023-05-10 12:35 ` Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 7/8] fetch: move option related variables " Patrick Steinhardt
2023-05-09 13:02 ` [PATCH v4 8/8] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-05-09 20:43 ` Junio C Hamano
2023-05-10 12:35 ` Patrick Steinhardt
2023-05-10 12:33 ` [PATCH v5 0/9] fetch: introduce machine-parseable output Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 1/9] fetch: fix `--no-recurse-submodules` with multi-remote fetches Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 2/9] fetch: split out tests for output format Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 3/9] fetch: add a test to exercise invalid output formats Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 4/9] fetch: print left-hand side when fetching HEAD:foo Patrick Steinhardt
2023-05-12 0:16 ` Glen Choo
2023-05-13 16:59 ` Jeff King
2023-05-15 5:15 ` Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 5/9] fetch: refactor calculation of the display table width Patrick Steinhardt
2023-05-12 0:49 ` Glen Choo
2023-05-10 12:34 ` [PATCH v5 6/9] fetch: introduce `display_format` enum Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 7/9] fetch: lift up parsing of "fetch.output" config variable Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 8/9] fetch: move option related variables into main function Patrick Steinhardt
2023-05-10 12:34 ` [PATCH v5 9/9] fetch: introduce machine-parseable "porcelain" output format Patrick Steinhardt
2023-05-12 1:02 ` Glen Choo
2023-05-10 18:05 ` [PATCH v5 0/9] fetch: introduce machine-parseable output Junio C Hamano
2023-05-11 11:05 ` Patrick Steinhardt
2023-05-11 16:53 ` Junio C Hamano
2023-05-11 17:24 ` Felipe Contreras
2023-05-12 1:09 ` Glen Choo
2023-05-12 7:16 ` Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqednpxq27.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=chooglen@google.com \
--cc=felipe.contreras@gmail.com \
--cc=git@vger.kernel.org \
--cc=jacob.e.keller@intel.com \
--cc=jonathantanmy@google.com \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).