Re: [PATCHv4 8/8] clone: recursive and reference option triggers submodule alternates

From: Stefan Beller <sbeller@google.com>
Date: Aug 17, 2016, 21:41 UTC
Message-ID: <CAGZ79kaqy16PLm8aZkAzp6PtTPFDuN80CiPPi-bbU9tec3X_oA@mail.gmail.com>
In-Reply-To: <xmqqd1l784lf.fsf@gitster.mtv.corp.google.com>

On Wed, Aug 17, 2016 at 2:31 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 31 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> 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.

Show 6 quoted lines

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