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@xxxxxx> 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.
