This replaces sb/submodule-parallel-update.
(which is based on sb/submodule-parallel-fetch)

Thanks to Junio and Jonathan for review!

* s/config_parallel_submodules/parallel_submodules/ as it is not in config any
  more. Also ease up the default setting. 

* use an enum for submodule update strategy

* parsing of submodule.fetchJobs in submodule.c instead of submodule-config.
  -> no need for 2 refactoring patches
  
* This seems to clash with 00/20] refs backend.
> Applied this on top of a merge between the current 'master' and
> 'sb/submodule-parallel-update' topic to untangle the dependency;
> otherwise there is no way for this topic to make progress X-<.

Anything I can do to help with easing the clash?

Thanks,
Stefan

interdiff to v8:

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7e01b85..6c74474 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -257,7 +257,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
 
 static int git_submodule_config(const char *var, const char *value, void *cb)
 {
-       return parse_submodule_config_option(var, value);
+       return submodule_config(var, value, cb);
 }
 
 struct submodule_update_clone {
@@ -316,7 +316,6 @@ static int update_clone_get_next_task(struct child_process 
*cp,
                struct strbuf displaypath_sb = STRBUF_INIT;
                struct strbuf sb = STRBUF_INIT;
                const char *displaypath = NULL;
-               const char *update_module = NULL;
                char *url = NULL;
                int needs_cloning = 0;
 
@@ -340,13 +339,8 @@ static int update_clone_get_next_task(struct child_process 
*cp,
                else
                        displaypath = ce->name;
 
-               if (pp->update)
-                       update_module = pp->update;
-               if (!update_module)
-                       update_module = sub->update;
-               if (!update_module)
-                       update_module = "checkout";
-               if (!strcmp(update_module, "none")) {
+               if ((pp->update && !strcmp(pp->update, "none")) ||
+                   (!pp->update && sub->update == SM_UPDATE_NONE)) {
                        strbuf_addf(err, "Skipping submodule '%s'\n",
                                    displaypath);
                        goto cleanup_and_continue;
@@ -479,9 +473,7 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
        git_config(git_submodule_config, NULL);
 
        if (max_jobs < 0)
-               max_jobs = config_parallel_submodules();
-       if (max_jobs < 0)
-               max_jobs = 1;
+               max_jobs = parallel_submodules();
 
        run_processes_parallel(max_jobs,
                               update_clone_get_next_task,
diff --git a/submodule-config.c b/submodule-config.c
index 339b59d..d6c8d9c 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -32,7 +32,6 @@ enum lookup_type {
 
 static struct submodule_cache cache;
 static int is_cache_init;
-static unsigned long parallel_jobs = -1;
 
 static int config_path_cmp(const struct submodule_entry *a,
                           const struct submodule_entry *b,
@@ -162,10 +161,27 @@ static struct submodule *cache_lookup_name(struct 
submodule_cache *cache,
        return NULL;
 }
 
+static int name_and_item_from_var(const char *var, struct strbuf *name,
+                                 struct strbuf *item)
+{
+       const char *subsection, *key;
+       int subsection_len, parse;
+       parse = parse_config_key(var, "submodule", &subsection,
+                       &subsection_len, &key);
+       if (parse < 0 || !subsection)
+               return 0;
+
+       strbuf_add(name, subsection, subsection_len);
+       strbuf_addstr(item, key);
+
+       return 1;
+}
+
 static struct submodule *lookup_or_create_by_name(struct submodule_cache 
*cache,
                const unsigned char *gitmodules_sha1, const char *name)
 {
        struct submodule *submodule;
+       struct strbuf name_buf = STRBUF_INIT;
 
        submodule = cache_lookup_name(cache, gitmodules_sha1, name);
        if (submodule)
@@ -173,10 +189,13 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
 
        submodule = xmalloc(sizeof(*submodule));
 
-       submodule->name = xstrdup(name);
+       strbuf_addstr(&name_buf, name);
+       submodule->name = strbuf_detach(&name_buf, NULL);
+
        submodule->path = NULL;
        submodule->url = NULL;
-       submodule->update = NULL;
+       submodule->update = SM_UPDATE_UNSPECIFIED;
+       submodule->update_command = NULL;
        submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
        submodule->ignore = NULL;
 
@@ -229,34 +248,22 @@ struct parse_config_parameter {
        int overwrite;
 };
 
-static int parse_generic_submodule_config(const char *key,
-                                         const char *var,
-                                         const char *value,
-                                         struct parse_config_parameter *me)
+static int parse_config(const char *var, const char *value, void *data)
 {
-       if (!strcmp(key, "fetchjobs")) {
-               if (!git_parse_ulong(value, &parallel_jobs)) {
-                       warning(_("Error parsing submodule.fetchJobs; 
Defaulting to 1."));
-                       parallel_jobs = 1;
-               }
-       }
+       struct parse_config_parameter *me = data;
+       struct submodule *submodule;
+       struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
+       int ret = 0;
 
-       return 0;
-}
+       /* this also ensures that we only parse submodule entries */
+       if (!name_and_item_from_var(var, &name, &item))
+               return 0;
 
-static int parse_specific_submodule_config(const char *subsection,
-                                          const char *key,
-                                          const char *var,
-                                          const char *value,
-                                          struct parse_config_parameter *me)
-{
-       int ret = 0;
-       struct submodule *submodule;
        submodule = lookup_or_create_by_name(me->cache,
                                             me->gitmodules_sha1,
-                                            subsection);
+                                            name.buf);
 
-       if (!strcmp(key, "path")) {
+       if (!strcmp(item.buf, "path")) {
                if (!value)
                        ret = config_error_nonbool(var);
                else if (!me->overwrite && submodule->path)
@@ -269,7 +276,7 @@ static int parse_specific_submodule_config(const char 
*subsection,
                        submodule->path = xstrdup(value);
                        cache_put_path(me->cache, submodule);
                }
-       } else if (!strcmp(key, "fetchrecursesubmodules")) {
+       } else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
                /* when parsing worktree configurations we can die early */
                int die_on_error = is_null_sha1(me->gitmodules_sha1);
                if (!me->overwrite &&
@@ -280,7 +287,7 @@ static int parse_specific_submodule_config(const char 
*subsection,
                        submodule->fetch_recurse = parse_fetch_recurse(
                                                                var, value,
                                                                die_on_error);
-       } else if (!strcmp(key, "ignore")) {
+       } else if (!strcmp(item.buf, "ignore")) {
                if (!value)
                        ret = config_error_nonbool(var);
                else if (!me->overwrite && submodule->ignore)
@@ -296,7 +303,7 @@ static int parse_specific_submodule_config(const char 
*subsection,
                        free((void *) submodule->ignore);
                        submodule->ignore = xstrdup(value);
                }
-       } else if (!strcmp(key, "url")) {
+       } else if (!strcmp(item.buf, "url")) {
                if (!value) {
                        ret = config_error_nonbool(var);
                } else if (!me->overwrite && submodule->url) {
@@ -306,39 +313,31 @@ static int parse_specific_submodule_config(const char 
*subsection,
                        free((void *) submodule->url);
                        submodule->url = xstrdup(value);
                }
-       } else if (!strcmp(key, "update")) {
+       } else if (!strcmp(item.buf, "update")) {
                if (!value)
                        ret = config_error_nonbool(var);
-               else if (!me->overwrite && submodule->update)
+               else if (!me->overwrite &&
+                   submodule->update != SM_UPDATE_UNSPECIFIED)
                        warn_multiple_config(me->commit_sha1, submodule->name,
                                             "update");
                else {
-                       free((void *) submodule->update);
-                       submodule->update = xstrdup(value);
+                       submodule->update_command = NULL;
+                       if (!strcmp(value, "none"))
+                               submodule->update = SM_UPDATE_NONE;
+                       else if (!strcmp(value, "checkout"))
+                               submodule->update = SM_UPDATE_CHECKOUT;
+                       else if (!strcmp(value, "rebase"))
+                               submodule->update = SM_UPDATE_REBASE;
+                       else if (!strcmp(value, "merge"))
+                               submodule->update = SM_UPDATE_MERGE;
+                       else if (!skip_prefix(value, "!", 
&submodule->update_command))
+                               die(_("invalid value for %s"), var);
                }
        }
 
-       return ret;
-}
-
-static int parse_config(const char *var, const char *value, void *data)
-{
-       struct parse_config_parameter *me = data;
-       int subsection_len, ret;
-       const char *subsection, *key;
-       char *sub;
-
-       if (parse_config_key(var, "submodule", &subsection,
-                            &subsection_len, &key) < 0)
-               return 0;
-
-       if (!subsection)
-               return parse_generic_submodule_config(key, var, value, me);
+       strbuf_release(&name);
+       strbuf_release(&item);
 
-       sub = xmemdupz(subsection, subsection_len);
-       ret = parse_specific_submodule_config(sub, key,
-                                             var, value, me);
-       free(sub);
        return ret;
 }
 
@@ -487,8 +486,3 @@ void submodule_free(void)
        cache_free(&cache);
        is_cache_init = 0;
 }
-
-unsigned long config_parallel_submodules(void)
-{
-       return parallel_jobs;
-}
diff --git a/submodule-config.h b/submodule-config.h
index bab1e8d..e3bd56e 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -4,6 +4,15 @@
 #include "hashmap.h"
 #include "strbuf.h"
 
+enum submodule_update_type {
+       SM_UPDATE_CHECKOUT,
+       SM_UPDATE_REBASE,
+       SM_UPDATE_MERGE,
+       SM_UPDATE_NONE,
+       SM_UPDATE_COMMAND,
+       SM_UPDATE_UNSPECIFIED
+};
+
 /*
  * Submodule entry containing the information about a certain submodule
  * in a certain revision.
@@ -14,7 +23,8 @@ struct submodule {
        const char *url;
        int fetch_recurse;
        const char *ignore;
-       const char *update;
+       enum submodule_update_type update;
+       const char *update_command;
        /* the sha1 blob id of the responsible .gitmodules file */
        unsigned char gitmodules_sha1[20];
 };
@@ -27,6 +37,4 @@ const struct submodule *submodule_from_path(const unsigned 
char *commit_sha1,
                const char *path);
 void submodule_free(void);
 
-unsigned long config_parallel_submodules(void);
-
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index eb7d54b..fd763f5 100644
--- a/submodule.c
+++ b/submodule.c
@@ -15,6 +15,7 @@
 #include "thread-utils.h"
 
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
+static int parallel_jobs = 1;
 static struct string_list changed_submodule_paths;
 static int initialized_fetch_ref_tips;
 static struct sha1_array ref_tips_before_fetch;
@@ -169,7 +170,13 @@ void set_diffopt_flags_from_submodule_config(struct 
diff_options *diffopt,
 
 int submodule_config(const char *var, const char *value, void *cb)
 {
-       if (starts_with(var, "submodule."))
+       if (!strcmp(var, "submodule.fetchjobs")) {
+               unsigned long val;
+               if (!git_parse_ulong(value, &val) || 0 > val || val >= INT_MAX)
+                       die(_("Error parsing submodule.fetchJobs %s"), value);
+               parallel_jobs = val;
+               return 0;
+       } else if (starts_with(var, "submodule."))
                return parse_submodule_config_option(var, value);
        else if (!strcmp(var, "fetch.recursesubmodules")) {
                config_fetch_recurse_submodules = 
parse_fetch_recurse_submodules_arg(var, value);
@@ -752,9 +759,7 @@ int fetch_populated_submodules(const struct argv_array 
*options,
        /* default value, "--submodule-prefix" and its value are added later */
 
        if (max_parallel_jobs < 0)
-               max_parallel_jobs = config_parallel_submodules();
-       if (max_parallel_jobs < 0)
-               max_parallel_jobs = 1;
+               max_parallel_jobs = parallel_jobs;
 
        calculate_changed_submodule_paths();
        run_processes_parallel(max_parallel_jobs,
@@ -1102,3 +1107,8 @@ void connect_work_tree_and_git_dir(const char *work_tree, 
const char *git_dir)
        strbuf_release(&rel_path);
        free((void *)real_work_tree);
 }
+
+int parallel_submodules(void)
+{
+       return parallel_jobs;
+}
diff --git a/submodule.h b/submodule.h
index cbc0003..95babc5 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,5 +41,6 @@ int find_unpushed_submodules(unsigned char new_sha1[20], 
const char *remotes_nam
                struct string_list *needs_pushing);
 int push_unpushed_submodules(unsigned char new_sha1[20], const char 
*remotes_name);
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+int parallel_submodules(void);
 
 #endif


Stefan Beller (6):
  submodule-config: keep update strategy around
  submodule-config: drop check against NULL
  fetching submodules: respect `submodule.fetchJobs` config option
  git submodule update: have a dedicated helper for cloning
  submodule update: expose parallelism to the user
  clone: allow an explicit argument for parallel submodule clones

 Documentation/config.txt        |   6 +
 Documentation/git-clone.txt     |   6 +-
 Documentation/git-submodule.txt |   7 +-
 builtin/clone.c                 |  19 +++-
 builtin/fetch.c                 |   2 +-
 builtin/submodule--helper.c     | 238 ++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh                |  54 ++++-----
 submodule-config.c              |  28 ++++-
 submodule-config.h              |  11 ++
 submodule.c                     |  17 ++-
 submodule.h                     |   1 +
 t/t5526-fetch-submodules.sh     |  14 +++
 t/t7400-submodule-basic.sh      |   4 +-
 t/t7406-submodule-update.sh     |  27 +++++
 14 files changed, 385 insertions(+), 49 deletions(-)

-- 
2.7.0.rc0.35.gb0f7869.dirty

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

Reply via email to