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

On Wed, Aug 17, 2016 at 3:12 PM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

 Added a default for alternateErrorStrategy and hence fixed the null pointer
 for error_strategy in prepare_possible_alternates(),

Looks much better.

quoted

+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?

quoted

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

...

quoted

+     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

quoted

+     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