On Wed, Apr 26, 2023 at 12:25:53PM -0700, Glen Choo wrote: > Rearranging the lines slightly, > > Patrick Steinhardt <ps@xxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
