Re: [PATCHv7 8/8] clone: recursive and reference option triggers submodule alternates
On Wed, Aug 17, 2016 at 3:12 PM, Junio C Hamanowrote: > Stefan Beller 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, >> ); 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv7 8/8] clone: recursive and reference option triggers submodule alternates
Stefan Bellerwrites: > 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? > +submodule.alternateErrorStrategy > + Specifies how to treat errors with the alternates for a submodule > + as computed via `submodule.alternateLocation`. Possible values are > + `ignore`, `info`, `die`. Default is `die`. > + > tag.forceSignAnnotated:: > A boolean to specify whether annotated tags created should be GPG > signed. > If `--annotate` is specified on the command line, it takes > diff --git a/builtin/clone.c b/builtin/clone.c > index 0182665..404c5e8 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char > *prefix) > else > fprintf(stderr, _("Cloning into '%s'...\n"), dir); > } > + > + if (option_recursive) { > + if (option_required_reference.nr && > + option_optional_reference.nr) > + die(_("clone --recursive is not compatible with " > + "both --reference and --reference-if-able")); > + else if (option_required_reference.nr) { > + string_list_append(_config, > + "submodule.alternateLocation=superproject"); > + string_list_append(_config, > + "submodule.alternateErrorStrategy=die"); > + } else if (option_optional_reference.nr) { > + string_list_append(_config, > + "submodule.alternateLocation=superproject"); > + string_list_append(_config, > + "submodule.alternateErrorStrategy=info"); > + } > + } > + > init_db(option_template, INIT_DB_QUIET); > write_config(_config); > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 7096848..f8f35c1 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -472,6 +472,96 @@ static int clone_submodule(const char *path, const char > *gitdir, const char *url > return run_command(); > } > > +struct submodule_alternate_setup { > + const char *submodule_name; > + enum SUBMODULE_ALTERNATE_ERROR_MODE { > + SUBMODULE_ALTERNATE_ERROR_DIE, > + SUBMODULE_ALTERNATE_ERROR_INFO, > + SUBMODULE_ALTERNATE_ERROR_IGNORE > + } error_mode; > + struct string_list *reference; > +}; > +#define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \ > + SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL } > + > +static int add_possible_reference_from_superproject( > + struct alternate_object_database *alt, void *sas_cb) > +{ > + struct submodule_alternate_setup *sas = sas_cb; > + > + /* directory name, minus trailing slash */ > + size_t namelen = alt->name - alt->base - 1; > + struct strbuf name = STRBUF_INIT; > + strbuf_add(, alt->base, namelen); > + > + /* > + * If the alternate object store is another repository, try the > + * standard layout with .git/modules//objects > + */ > + if (ends_with(name.buf, ".git/objects")) { > + char *sm_alternate; > + struct strbuf sb = STRBUF_INIT; > + struct strbuf err = STRBUF_INIT; > + strbuf_add(, name.buf, name.len - strlen("objects")); > + /* > + * We need to end the new path with '/' to mark it as a dir, > + * otherwise a submodule name containing '/' will be broken > + * as the last part of a missing submodule reference would > + * be taken as a file name. > + */ > + strbuf_addf(, "modules/%s/", sas->submodule_name); > + > + sm_alternate = compute_alternate_path(sb.buf, ); > + if (sm_alternate) { > +
[PATCHv7 8/8] clone: recursive and reference option triggers submodule alternates
When `--recursive` and `--reference` is given, it is reasonable to expect that the submodules are created with references to the submodules of the given alternate for the superproject. An initial attempt to do this was presented to the mailing list, which used flags that are passed around ("--super-reference") that instructed the submodule clone to look for a reference in the submodules of the referenced superproject. This is not well thought out, as any further `submodule update` should also respect the initial setup. When a new submodule is added to the superproject and the alternate of the superproject does not know about that submodule yet, we rather error out informing the user instead of being unclear if we did or did not use a submodules alternate. To solve this problem introduce new options that store the configuration for what the user wanted originally. Signed-off-by: Stefan Beller--- Added a default for alternateErrorStrategy and hence fixed the null pointer for error_strategy in prepare_possible_alternates(), Thanks, Stefan Documentation/config.txt | 12 ++ builtin/clone.c| 19 + builtin/submodule--helper.c| 93 ++ t/t7408-submodule-reference.sh | 43 +++ 4 files changed, 167 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index bc1c433..e2571ea 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2837,6 +2837,18 @@ submodule.fetchJobs:: in parallel. A value of 0 will give some reasonable default. If unset, it defaults to 1. +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. + +submodule.alternateErrorStrategy + Specifies how to treat errors with the alternates for a submodule + as computed via `submodule.alternateLocation`. Possible values are + `ignore`, `info`, `die`. Default is `die`. + tag.forceSignAnnotated:: A boolean to specify whether annotated tags created should be GPG signed. If `--annotate` is specified on the command line, it takes diff --git a/builtin/clone.c b/builtin/clone.c index 0182665..404c5e8 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix) else fprintf(stderr, _("Cloning into '%s'...\n"), dir); } + + if (option_recursive) { + if (option_required_reference.nr && + option_optional_reference.nr) + die(_("clone --recursive is not compatible with " + "both --reference and --reference-if-able")); + else if (option_required_reference.nr) { + string_list_append(_config, + "submodule.alternateLocation=superproject"); + string_list_append(_config, + "submodule.alternateErrorStrategy=die"); + } else if (option_optional_reference.nr) { + string_list_append(_config, + "submodule.alternateLocation=superproject"); + string_list_append(_config, + "submodule.alternateErrorStrategy=info"); + } + } + init_db(option_template, INIT_DB_QUIET); write_config(_config); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 7096848..f8f35c1 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -472,6 +472,96 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url return run_command(); } +struct submodule_alternate_setup { + const char *submodule_name; + enum SUBMODULE_ALTERNATE_ERROR_MODE { + SUBMODULE_ALTERNATE_ERROR_DIE, + SUBMODULE_ALTERNATE_ERROR_INFO, + SUBMODULE_ALTERNATE_ERROR_IGNORE + } error_mode; + struct string_list *reference; +}; +#define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \ + SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL } + +static int add_possible_reference_from_superproject( + struct alternate_object_database *alt, void *sas_cb) +{ + struct submodule_alternate_setup *sas = sas_cb; + + /* directory name, minus trailing slash */ + size_t namelen = alt->name - alt->base - 1; + struct strbuf name = STRBUF_INIT; + strbuf_add(, alt->base, namelen); + + /* +* If the alternate object store is another repository,