| Message ID | 596e12f03a296d753ee6fe8face9522edc9e397e.1681906948.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: introduce machine-parseable output | expand |
On 4/19/2023 5:31 AM, Patrick Steinhardt wrote: > 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". > > The logic is kind of intertwined though and not easy to follow: we > precompute both the kind (e.g. "branch") and the what, which is the > abbreviated remote reference name, in `store_updated_refs()` and then > pass it down the call chain to `display_ref_update()`. > > There is a set of different cases here: > > - When the remote reference name is "HEAD" we assume no kind and > will thus instead print "[new ref]". We keep what at the empty > string. > > - When the remote reference name has a well-known prefix then the > kind would be "branch", "tag" or "remote-tracking branch". The > what is the reference with the well-known prefix stripped and in > fact matches the output that `prettify_refname()` would return. > > - Otherwise, we'll again assume no kind and keep the what set to the > fully qualified reference name. > > Now there is a bug with the first case here, where the remote reference > name is "HEAD". As noted, "what" will be set to the empty string. And > that seems to be intentional because we also use this information to > update the FETCH_HEAD, and in case we're updating HEAD we seemingly > don't want to append that to our FETCH_HEAD value. > > But as mentioned, 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 > ``` > > 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 work > as expected: > > ``` > $ git fetch --dry-run origin HEAD > From https://github.com/git/git > * branch HEAD -> FETCH_HEAD > > $ git fetch --dry-run origin master > From https://github.com/git/git > * branch master -> FETCH_HEAD > * branch master -> origin/master > ``` > > 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. > > 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> The commit message here has a lot of context, but I found it a bit hard to parse through, especially relative to the actual fix in code. One suggestion was to load the paragraphs a bit more with the actual problem being solved first, before beginning a lot of the context. We also discussed the block of format changes and felt a bit mixed on whether to include it or not. It does match the coding style guidelines, but there is no actual functional change made to those lines in this series. I think its a good improvement, and it does force some extra context into the diff which makes reading the resulting change easier.
On 4/19/2023 5:31 AM, Patrick Steinhardt wrote: > > +test_expect_success 'fetch output with HEAD and --dry-run' ' > + test_when_finished "rm -rf head" && > + git clone . head && > + > + git -C head fetch --dry-run origin HEAD >actual 2>&1 && > + cat >expect <<-EOF && > + From $(test-tool path-utils real_path .)/. > + * branch HEAD -> FETCH_HEAD > + EOF > + test_cmp expect actual && > + > + git -C head fetch --dry-run origin HEAD:foo >actual 2>&1 && > + cat >expect <<-EOF && > + From $(test-tool path-utils real_path .)/. > + * [new ref] HEAD -> foo > + EOF > + test_cmp expect actual > +' > + The test mentions HEAD and --dry-run, but the bug seems to exist regardless of whether --dry-run is used. I understand the use of --dry-run for testing fetch output so that you can repeatably run git fetch and get the same results. The tests here should probably also have a test that covers fetch without --dry-run though. Thanks, Jake
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.
On Wed, Apr 26, 2023 at 12:20:15PM -0700, Jacob Keller wrote: > On 4/19/2023 5:31 AM, Patrick Steinhardt wrote: [snip] > The commit message here has a lot of context, but I found it a bit hard > to parse through, especially relative to the actual fix in code. > > One suggestion was to load the paragraphs a bit more with the actual > problem being solved first, before beginning a lot of the context. > > We also discussed the block of format changes and felt a bit mixed on > whether to include it or not. It does match the coding style guidelines, > but there is no actual functional change made to those lines in this series. > > I think its a good improvement, and it does force some extra context > into the diff which makes reading the resulting change easier. I initially really struggled to explain to myself why my change actually works, and that shows in the commit message. I've reworded it a bit and took some of the suggested changes that Glen posted to hopefully make this clearer. Patrick
On Wed, Apr 26, 2023 at 12:21:40PM -0700, Jacob Keller wrote: > > > On 4/19/2023 5:31 AM, Patrick Steinhardt wrote: > > > > +test_expect_success 'fetch output with HEAD and --dry-run' ' > > + test_when_finished "rm -rf head" && > > + git clone . head && > > + > > + git -C head fetch --dry-run origin HEAD >actual 2>&1 && > > + cat >expect <<-EOF && > > + From $(test-tool path-utils real_path .)/. > > + * branch HEAD -> FETCH_HEAD > > + EOF > > + test_cmp expect actual && > > + > > + git -C head fetch --dry-run origin HEAD:foo >actual 2>&1 && > > + cat >expect <<-EOF && > > + From $(test-tool path-utils real_path .)/. > > + * [new ref] HEAD -> foo > > + EOF > > + test_cmp expect actual > > +' > > + > > The test mentions HEAD and --dry-run, but the bug seems to exist > regardless of whether --dry-run is used. I understand the use of > --dry-run for testing fetch output so that you can repeatably run git > fetch and get the same results. > > The tests here should probably also have a test that covers fetch > without --dry-run though. Makes sense, will amend! Patrick
On Wed, Apr 26, 2023 at 12:25:53PM -0700, Glen Choo wrote: > 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... I like that and will use a variant of this. [snip] > > 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. True, I'll amend the test. Patrick
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'); @@ -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 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' ' + test_when_finished "rm -rf head" && + git clone . head && + + git -C head fetch --dry-run origin HEAD >actual 2>&1 && + cat >expect <<-EOF && + From $(test-tool path-utils real_path .)/. + * branch HEAD -> FETCH_HEAD + EOF + test_cmp expect actual && + + git -C head fetch --dry-run origin HEAD:foo >actual 2>&1 && + cat >expect <<-EOF && + From $(test-tool path-utils real_path .)/. + * [new ref] HEAD -> foo + EOF + test_cmp expect actual +' + test_expect_success '--no-show-forced-updates' ' mkdir forced-updates && (
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". The logic is kind of intertwined though and not easy to follow: we precompute both the kind (e.g. "branch") and the what, which is the abbreviated remote reference name, in `store_updated_refs()` and then pass it down the call chain to `display_ref_update()`. There is a set of different cases here: - When the remote reference name is "HEAD" we assume no kind and will thus instead print "[new ref]". We keep what at the empty string. - When the remote reference name has a well-known prefix then the kind would be "branch", "tag" or "remote-tracking branch". The what is the reference with the well-known prefix stripped and in fact matches the output that `prettify_refname()` would return. - Otherwise, we'll again assume no kind and keep the what set to the fully qualified reference name. Now there is a bug with the first case here, where the remote reference name is "HEAD". As noted, "what" will be set to the empty string. And that seems to be intentional because we also use this information to update the FETCH_HEAD, and in case we're updating HEAD we seemingly don't want to append that to our FETCH_HEAD value. But as mentioned, 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 ``` 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 work as expected: ``` $ git fetch --dry-run origin HEAD From https://github.com/git/git * branch HEAD -> FETCH_HEAD $ git fetch --dry-run origin master From https://github.com/git/git * branch master -> FETCH_HEAD * branch master -> origin/master ``` 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. 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 | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+), 18 deletions(-)