Re: [PATCH 05/15] submodule--helper: don't overlay config in update-clone

[Brandon Williams]

On 07/25, Stefan Beller wrote:
> On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams  wrote:
> > Don't rely on overlaying the repository's config on top of the
> > submodule-config, instead query the repository's config directly for the
> > url and the update strategy configuration.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> ...
> 
> > +struct submodule_update_strategy 
> > submodule_strategy_with_config_overlayed(struct repository *repo,
> > + 
> > const struct submodule *sub)
> > +{
> > +   struct submodule_update_strategy strat = sub->update_strategy;
> > +   const char *update;
> > +   char *key;
> > +
> > +   key = xstrfmt("submodule.%s.update", sub->name);
> > +   if (!repo_config_get_string_const(repo, key, )) {
> > +   strat.command = NULL;
> > +   if (!strcmp(update, "none")) {
> > +   strat.type = SM_UPDATE_NONE;
> > +   } else if (!strcmp(update, "checkout")) {
> > +   strat.type = SM_UPDATE_CHECKOUT;
> > +   } else if (!strcmp(update, "rebase")) {
> > +   strat.type = SM_UPDATE_REBASE;
> > +   } else if (!strcmp(update, "merge")) {
> > +   strat.type = SM_UPDATE_MERGE;
> > +   } else if (skip_prefix(update, "!", )) {
> > +   strat.type = SM_UPDATE_COMMAND;
> > +   strat.command = update;
> > +   } else {
> > +   die("invalid submodule update strategy '%s'", 
> > update);
> > +   }
> > +   }
> 
> Can this be simplified by reusing
> parse_submodule_update_strategy(value, dest)
> ?

It would result in a memory leak if we did.  Really I'd like to just
remove this entirely. The only reason this needs to be done is for
checkout, which if we don't have respect the update config it can be
removed.

-- 
Brandon Williams

Re: [PATCH 05/15] submodule--helper: don't overlay config in update-clone

[Stefan Beller]

On Tue, Jul 25, 2017 at 2:39 PM, Brandon Williams  wrote:
> Don't rely on overlaying the repository's config on top of the
> submodule-config, instead query the repository's config directly for the
> url and the update strategy configuration.
>
> Signed-off-by: Brandon Williams 
> ---
...

> +struct submodule_update_strategy 
> submodule_strategy_with_config_overlayed(struct repository *repo,
> + 
> const struct submodule *sub)
> +{
> +   struct submodule_update_strategy strat = sub->update_strategy;
> +   const char *update;
> +   char *key;
> +
> +   key = xstrfmt("submodule.%s.update", sub->name);
> +   if (!repo_config_get_string_const(repo, key, )) {
> +   strat.command = NULL;
> +   if (!strcmp(update, "none")) {
> +   strat.type = SM_UPDATE_NONE;
> +   } else if (!strcmp(update, "checkout")) {
> +   strat.type = SM_UPDATE_CHECKOUT;
> +   } else if (!strcmp(update, "rebase")) {
> +   strat.type = SM_UPDATE_REBASE;
> +   } else if (!strcmp(update, "merge")) {
> +   strat.type = SM_UPDATE_MERGE;
> +   } else if (skip_prefix(update, "!", )) {
> +   strat.type = SM_UPDATE_COMMAND;
> +   strat.command = update;
> +   } else {
> +   die("invalid submodule update strategy '%s'", update);
> +   }
> +   }

Can this be simplified by reusing
parse_submodule_update_strategy(value, dest)
?

[PATCH 05/15] submodule--helper: don't overlay config in update-clone

[Brandon Williams]

Don't rely on overlaying the repository's config on top of the
submodule-config, instead query the repository's config directly for the
url and the update strategy configuration.

Signed-off-by: Brandon Williams 
---
 builtin/submodule--helper.c | 14 ++
 submodule.c | 30 ++
 submodule.h |  3 +++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f71f4270d..25f471ba1 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -780,6 +780,8 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
   struct strbuf *out)
 {
const struct submodule *sub = NULL;
+   const char *url = NULL;
+   struct submodule_update_strategy update;
struct strbuf displaypath_sb = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
const char *displaypath = NULL;
@@ -808,9 +810,10 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
goto cleanup;
}
 
+   update = submodule_strategy_with_config_overlayed(the_repository, sub);
if (suc->update.type == SM_UPDATE_NONE
|| (suc->update.type == SM_UPDATE_UNSPECIFIED
-   && sub->update_strategy.type == SM_UPDATE_NONE)) {
+   && update.type == SM_UPDATE_NONE)) {
strbuf_addf(out, _("Skipping submodule '%s'"), displaypath);
strbuf_addch(out, '\n');
goto cleanup;
@@ -822,6 +825,11 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
goto cleanup;
}
 
+   strbuf_reset();
+   strbuf_addf(, "submodule.%s.url", sub->name);
+   if (repo_config_get_string_const(the_repository, sb.buf, ))
+   url = sub->url;
+
strbuf_reset();
strbuf_addf(, "%s/.git", ce->name);
needs_cloning = !file_exists(sb.buf);
@@ -851,7 +859,7 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_push(>args, "--depth=1");
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
-   argv_array_pushl(>args, "--url", sub->url, NULL);
+   argv_array_pushl(>args, "--url", url, NULL);
if (suc->references.nr) {
struct string_list_item *item;
for_each_string_list_item(item, >references)
@@ -1025,9 +1033,7 @@ static int update_clone(int argc, const char **argv, 
const char *prefix)
if (pathspec.nr)
suc.warn_if_uninitialized = 1;
 
-   /* Overlay the parsed .gitmodules file with .git/config */
gitmodules_config();
-   git_config(submodule_config, NULL);
 
run_processes_parallel(max_jobs,
   update_clone_get_next_task,
diff --git a/submodule.c b/submodule.c
index fd391aea6..8b9e48a61 100644
--- a/submodule.c
+++ b/submodule.c
@@ -440,6 +440,36 @@ const char *submodule_strategy_to_string(const struct 
submodule_update_strategy
return NULL;
 }
 
+struct submodule_update_strategy 
submodule_strategy_with_config_overlayed(struct repository *repo,
+ const 
struct submodule *sub)
+{
+   struct submodule_update_strategy strat = sub->update_strategy;
+   const char *update;
+   char *key;
+
+   key = xstrfmt("submodule.%s.update", sub->name);
+   if (!repo_config_get_string_const(repo, key, )) {
+   strat.command = NULL;
+   if (!strcmp(update, "none")) {
+   strat.type = SM_UPDATE_NONE;
+   } else if (!strcmp(update, "checkout")) {
+   strat.type = SM_UPDATE_CHECKOUT;
+   } else if (!strcmp(update, "rebase")) {
+   strat.type = SM_UPDATE_REBASE;
+   } else if (!strcmp(update, "merge")) {
+   strat.type = SM_UPDATE_MERGE;
+   } else if (skip_prefix(update, "!", )) {
+   strat.type = SM_UPDATE_COMMAND;
+   strat.command = update;
+   } else {
+   die("invalid submodule update strategy '%s'", update);
+   }
+   }
+   free(key);
+
+   return strat;
+}
+
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
  const char *arg)
 {
diff --git a/submodule.h b/submodule.h
index e402b004f..f17ca1e34 100644
--- a/submodule.h
+++ b/submodule.h
@@ -6,6 +6,7 @@ struct diff_options;
 struct argv_array;
 struct oid_array;
 struct remote;
+struct submodule;
 
 enum {
RECURSE_SUBMODULES_ONLY = -5,
@@ -65,6 +66,8 @@ extern void die_path_inside_submodule(const struct 
index_state *istate,
 extern int