On Wed, Aug 17, 2016 at 2:31 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
>> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>>
>>> +static void prepare_possible_alternates(const char *sm_name,
>>> + struct string_list *reference)
>>> +{
>>> + char *sm_alternate = NULL, *error_strategy = NULL;
>>> + struct submodule_alternate_setup sas = SUBMODULE_ALTERNATE_SETUP_INIT;
>>> +
>>> + git_config_get_string("submodule.alternateLocation", &sm_alternate);
>>> + if (!sm_alternate)
>>> + return;
>>> +
>>> + git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
>>
>> I have to admit that I need to follow the codepath in config.c every
>> time to check, but I _think_ git_config_get_string() gives you your
>> own copy of the value. As this function does not give ownership of
>> sm_alternate or error_strategy to something else, they are leaked
>> every time this function is called, I think.
>>
>>> + sas.submodule_name = sm_name;
>>> + sas.reference = reference;
>>> + if (!strcmp(error_strategy, "die"))
>>> + sas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;
>
> Another thing I noticed but forgot to mention. Can error_strategy
> be NULL here? We are assuming sm_alternate can be, so I presume
> that it is sensible to protect against dereferencing a NULL here,
> too?
Oh!
Of course we need to fix that too. That slipped in because I assumed that those
two variables go in tandem (if sm_alternate is set, then
error_strategy is set as well),
but that is not the case of course.
Which leads to another thing: we need to have a default for error_strategy.
I think "die" is reasonable here. That also needs a documentation update.
>
>>> + if (!strcmp(error_strategy, "info"))
>>> + sas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;
>>> + if (!strcmp(sm_alternate, "superproject"))
>>> + foreach_alt_odb(add_possible_reference_from_superproject, &sas);
>>> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html