Re: [PATCH v4] remote rename/remove: gently handle remote.pushDefault config

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2020, 20:11 UTC
Message-ID: <xmqqv9om6rkk.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2002021911260.46@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 11 quoted lines

>> +	struct push_default_info* info = cb;
>> +	if (strcmp(key, "remote.pushdefault") || strcmp(value, info->old_name))
>> +		return 0;
>
> We will have to be careful to not segfault if a user has this in their
> config:
>
> 	[remote]
> 		pushDefault
>
> i.e. we have to insert `!value || ` before the call to `strcmp()`.

True. The primary reader in remote.c::handle_config() uses git_config_string() that complains that the variable is not bool, but we should reat end-user input as something suspicious and protect us against it.

Show 6 quoted lines

> Concretely, I believe that the patched code will misbehave in this
> scenario:
>
> 	git config --global remote.pushDefault january
> 	git config remote.pushDefault february
> 	git remote rename january march

Good to see careful analysis.  Thanks.

Show 5 quoted lines

>> +static void handle_push_default(const char* old_name, const char* new_name)
>
> That name probably wants to convey better that the push default is handled
> in the `mv`/`rm` commands here, not in any other command. Maybe
> `handle_modified_push_default_remote()`?

Also, the asterisk sticks to the variable not the type ;-)

Show 7 quoted lines

>> +{
>> +	struct push_default_info push_default = {
>> +		old_name, CONFIG_SCOPE_UNKNOWN, STRBUF_INIT, -1 };
>
> Personally, I would prefer the closing bracket to be on a new line,
> followed by an empty line to separate the variable declaration from the
> following statements.

Yes, yes.