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

From: Stefan Beller <sbeller@google.com>
Date: Aug 17, 2016, 22:18 UTC
Message-ID: <CAGZ79kaBX8UX4Hbq3Ff6DeTT6jfZTHcCXPYGOaDyMQTuWUvbyg@mail.gmail.com>
In-Reply-To: <xmqq8tvv82pg.fsf@gitster.mtv.corp.google.com>

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

Show 27 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>>  Added a default for alternateErrorStrategy and hence fixed the null pointer
>>  for error_strategy in prepare_possible_alternates(),
>
> Looks much better.
>
>> +submodule.alternateLocation::
>> +     Specifies how the submodules obtain alternates when submodules are
>> +     cloned. Possible values are `no`, `superproject`.
>> +     By default `no` is assumed, which doesn't add references. When the
>> +     value is set to `superproject` the submodule to be cloned computes
>> +     its alternates location relative to the superprojects alternate.
>
> I am not seeing a code that handles 'no' and any other string that
> is not 'superproject' differently, though.
>
> I can see that "clone" has codepath that writes 'superproject' to
> the variable, but the only thing that seems to care what value the
> variable is set to checks "no".  When somebody sets the variable to
> "yes", shouldn't we at least say "Sorry, I do not understand" and
> preferrably stop before spreading potential damage?  We'd surely end
> up doing something that the user who set the value to "yes" did not
> expect.
>
> There is something still missing?
>

>> +static void prepare_possible_alternates(const char *sm_name,
>> +             struct string_list *reference)
>> +{

...

Show 6 quoted lines

>> +     sas.submodule_name = sm_name;
>> +     sas.reference = reference;
>> +     if (!strcmp(error_strategy, "die"))
>> +             sas.error_mode = SUBMODULE_ALTERNATE_ERROR_DIE;
>> +     if (!strcmp(error_strategy, "info"))
>> +             sas.error_mode = SUBMODULE_ALTERNATE_ERROR_INFO;

(see below first) Here goes the same for alternateErrorStrategy

>> +     if (!strcmp(sm_alternate, "superproject"))
>> +             foreach_alt_odb(add_possible_reference_from_superproject, &sas);

here is where we would add

    else if (!strcmp(sm_alternate, "no")
        ; /* do nothing */
    else
        die(_("What's wrong with you?"));

Initially I did not add that as I considered it not relevant. But I guess it helps as a typo checker as well and it is more comforting if a wrong value yields an error. Also it is consistent with the rest of options.

Thanks again, Stefan