From: Glen Choo <chooglen@google.com>
To: Patrick Steinhardt <ps@pks.im>, git@vger.kernel.org
Cc: Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH 3/8] fetch: fix missing from-reference when fetching HEAD:foo
Date: Wed, 26 Apr 2023 12:25:53 -0700 [thread overview]
Message-ID: <kl6l5y9io37i.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <596e12f03a296d753ee6fe8face9522edc9e397e.1681906948.git.ps@pks.im>
Rearranging the lines slightly,
Patrick Steinhardt <ps@pks.im> writes:
> When displaying reference updates, we print a line that looks similar to
> the following:
>
> ```
> * branch master -> master
> ```
>
> The "branch" bit changes depending on what kind of reference we're
> updating, while both of the right-hand references are computed by
> stripping well-known prefixes like "refs/heads/" or "refs/tags".
>
> [...]
> we also use this value to display reference updates.
> And while the call to `display_ref_update()` correctly figures out that
> we meant "HEAD" when `what` is empty, the call to `update_local_ref()`
> doesn't. `update_local_ref()` will then call `display_ref_update()` with
> the empty string and cause the following broken output:
>
> ```
> $ git fetch --dry-run origin HEAD:foo
> From https://github.com/git/git
> * [new ref] -> foo
> ```
>
> [...]
>
> Fix this bug by instead unconditionally passing the full reference name
> to `display_ref_update()` which learns to call `prettify_refname()` on
> it. This does fix the above bug and is otherwise functionally the same
> as `prettify_refname()` would only ever strip the well-known prefixes
> just as intended. So at the same time, this also simplifies the code a
> bit.
The bug fix is obviously good. I'm surprised we hadn't caught this
sooner.
As a nitpicky comment, the commit message goes into a lot of detail,
which makes it tricky to read on its own (though the level of detail
makes it easy to match to the diff, making the diff quite easy to
follow). I would have found this easier to read by summarizing the
high-level mental model before diving into the background, e.g.
store_updated_refs() parses the remote ref name to create a 'note' to
write to FETCH_HEAD. This note is usually the prettified ref name, so
it is used to diplay ref updates (display_ref_update()). But if the
remote ref is HEAD, the note is the empty string [insert bug
description]. Instead, use the note only as a note and have
display_ref_update() prettify the ref name itself...
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c310d89878..7c64f0c562 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');
As expected, we now prettify the name isntead of trusting the 'note'
that came in the parameter...
> @@ -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;
> }
> }
...
> @@ -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);
> }
> }
and we stop passing the 'note' as a parameter. Looks good.
> @@ -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;
> }
I really appreciate that this makes the patch easier to read. I don't
really appreciate this sort of churn, but it _is_ following
CodingGuidelines:
- When there are multiple arms to a conditional and some of them
require braces, enclose even a single line block in braces for
consistency. E.g.:
if (foo) {
doit();
} else {
one();
two();
three();
}
(I initialy thought it wasn't. Thanks to other Review Club participants
for pointing this out).
> diff --git a/t/t5574-fetch-output.sh b/t/t5574-fetch-output.sh
> index 0e45c27007..55f0f05b6a 100755
> --- a/t/t5574-fetch-output.sh
> +++ b/t/t5574-fetch-output.sh
> @@ -54,6 +54,25 @@ test_expect_success 'fetch compact output' '
> test_cmp expect actual
> '
>
> +test_expect_success 'fetch output with HEAD and --dry-run' '
The commit message and diff didn't imply that this is a --dry-run only
bug. I tested locally, and it seems to reproduce without --dry-run too,
so I think we should drop "--dry-run" from this test name. In a later
patch, you also add a test for porcelain output with --dry-run, but
since this test seems designed for just this bug, I think we can drop
the later test.
next prev parent reply other threads:[~2023-04-26 19:26 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 [this message]
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
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=kl6l5y9io37i.fsf@chooglen-macbookpro.roam.corp.google.com \
--to=chooglen@google.com \
--cc=git@vger.kernel.org \
--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).