Re: [PATCH 3/8] fetch: fix missing from-reference when fetching HEAD:foo

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3197 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]