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

From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Bert Wesarg <bert.wesarg@googlemail.com>,
	git@vger.kernel.org, Matthew Rogers <mattr94@gmail.com>
Subject: Re: [PATCH v4] remote rename/remove: gently handle remote.pushDefault config
Date: Tue, 04 Feb 2020 12:11:23 -0800	[thread overview]
Message-ID: <xmqqv9om6rkk.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2002021911260.46@tvgsbejvaqbjf.bet> (Johannes Schindelin's message of "Sun, 2 Feb 2020 21:54:31 +0100 (CET)")

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

>> +	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.

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

>> +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 ;-)

>> +{
>> +	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.