Re: [PATCH 1/2] remote.c: don't BUG() on 0-length branch names

From: Jeff King <peff@peff.net>
Date: Jun 8, 2022, 23:27 UTC
Message-ID: <YqEwa37mNBqT7M+X@coredump.intra.peff.net>
In-Reply-To: <df6e4db6072e90afc92505a73792cf3c3221d5e4.1654038754.git.gitgitgadget@gmail.com>

On Tue, May 31, 2022 at 11:12:33PM +0000, Glen Choo via GitGitGadget wrote:

> Fix the bug by removing the convenience strlen functionality, so that
> 0 means that the string is 0-length. We still insert a bogus branch name
> into the hash map, but this will be fixed in a later commit.

I think this is a good change, regardless of whether we take the second commit or not. These kind of "automagically run strlen() sometimes" interfaces are subtle, and I think have bitten us before.

Show 14 quoted lines

> diff --git a/remote.c b/remote.c
> index 42a4e7106e1..cf7015ae8ab 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -195,9 +195,6 @@ static struct branch *find_branch(struct remote_state *remote_state,
>  	struct branches_hash_key lookup;
>  	struct hashmap_entry lookup_entry, *e;
>  
> -	if (!len)
> -		len = strlen(name);
> -
>  	lookup.str = name;
>  	lookup.len = len;
>  	hashmap_entry_init(&lookup_entry, memhash(name, len));

This changes the behavior of find_branch() without changing its signature. So any topics in flight that use it might be subtly broken. I think that's probably OK in this instance, since it's a file-local static, and was added relatively recently (i.e., the blast radius is pretty small and unlikely).

-Peff