git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] remote.c: reject 0-length branch names

From
Glen Choo <chooglen@google.com>
Date
Jun 1, 2022, 16:55 UTC
Message-ID
<kl6lilpke31e.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To
<220601.86leug261j.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 14 quoted lines
> On Tue, May 31 2022, Glen Choo via GitGitGadget wrote:
>
>> From: Glen Choo <chooglen@google.com>
>>
>> Branch names can't be empty, so config keys with an empty branch name,
>> e.g. "branch..remote", are silently ignored.
>>
>> Since these config keys will never be useful, make it a fatal error when
>> remote.c finds a key that starts with "branch." and has an empty
>> subsection.
>
> Perhaps this is fine, but I think this commit message (and I checked the
> CL too) really needs to work a bit harder to convince us that this is
> safe to do.
Fair.
Show 8 quoted lines
> Are we confident that this is just bizarro config that nobody would have
> had in practice? In that case I think it's fine to start dying on it.
>
> But as I understand we previously just ignored this, then if there's any
> doubt about that perhaps we should start with a warning?
>
> Or are we really confident that this is an edge case not worth worrying
> about in that way, and that we can go straight to die()?

The case I want to make is even stronger than that - this is an edge case that _we_ shouldn't worry about, and we should tell the _user_ that their config is bogus.

It truly makes no sense because `branch..remote` fits the schema of `branch.<name>.remote` where <name> is "", but "" isn't a valid branch name (and it never has been AFAIK). So such a key would never be useful to Git, and it would be extremely hacky for a non-Git tool to use such a key.

I'm not sure how a user would generate such a key in the wild (e.g. [1]). Maybe it was a typo, but more worryingly (I don't have evidence for this, but it could happen), it might be misbehavior from `git [branch|config]` that we never noticed because the bogus keys have flown under the radar. If there really is a bug elsewhere, erroring out when we see such keys might also alert us to the bug.

Perhaps I need to capture all of this in the commit message?
[1] https://lore.kernel.org/git/24f547-6285e280-59-40303580@48243747/
Previous: Ævar Arnfjörð BjarmasonNext: Junio C Hamano
Message 6 of 8 in “remote.c: reject 0-length branch names”
  1. 0/2 remote.c: reject 0-length branch namesGlen Choo via GitGitGadget, May 31, 2022
  2. 1/2 remote.c: don't BUG() on 0-length branch namesGlen Choo via GitGitGadget, May 31, 2022
  3. Jeff KingJun 8, 2022
  4. 2/2 remote.c: reject 0-length branch namesGlen Choo via GitGitGadget, May 31, 2022
  5. Ævar Arnfjörð BjarmasonJun 1, 2022
  6. Glen ChooJun 1, 2022
  7. Junio C HamanoJun 1, 2022
  8. Jeff KingJun 8, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.