On Wed, Aug 17, 2016 at 3:12 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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)
>> +{
...
>> + 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
--
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