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