[PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct
The information that is printed for update_submodules in 'submodule--helper update-clone' and consumed by 'git submodule update' is stored as a string per submodule. This made sense at the time of 48308681b07 (git submodule update: have a dedicated helper for cloning, 2016-02-29), but as we want to migrate the rest of the submodule update into C, we're better off having access to the raw information in a helper struct. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 37 +++-- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 96929ba1096..fb54936efc7 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1444,6 +1444,12 @@ static int module_clone(int argc, const char **argv, const char *prefix) return 0; } +struct update_clone_data { + const struct submodule *sub; + struct object_id oid; + unsigned just_cloned; +}; + struct submodule_update_clone { /* index into 'list', the list of submodules to look into for cloning */ int current; @@ -1463,8 +1469,9 @@ struct submodule_update_clone { const char *recursive_prefix; const char *prefix; - /* Machine-readable status lines to be consumed by git-submodule.sh */ - struct string_list projectlines; + /* to be consumed by git-submodule.sh */ + struct update_clone_data *update_clone; + int update_clone_nr; int update_clone_alloc; /* If we want to stop as fast as possible and return an error */ unsigned quickstop : 1; @@ -1478,7 +1485,7 @@ struct submodule_update_clone { #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ NULL, NULL, NULL, \ - STRING_LIST_INIT_DUP, 0, NULL, 0, 0} + NULL, 0, 0, 0, NULL, 0, 0, 0} static void next_submodule_warn_missing(struct submodule_update_clone *suc, @@ -1572,10 +1579,12 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_addf(, "%s/.git", ce->name); needs_cloning = !file_exists(sb.buf); - strbuf_reset(); - strbuf_addf(, "dummy %s %d\t%s\n", - oid_to_hex(>oid), needs_cloning, ce->name); - string_list_append(>projectlines, sb.buf); + ALLOC_GROW(suc->update_clone, suc->update_clone_nr + 1, + suc->update_clone_alloc); + oidcpy(>update_clone[suc->update_clone_nr].oid, >oid); + suc->update_clone[suc->update_clone_nr].just_cloned = needs_cloning; + suc->update_clone[suc->update_clone_nr].sub = sub; + suc->update_clone_nr++; if (!needs_cloning) goto cleanup; @@ -1718,7 +1727,8 @@ static int gitmodules_update_clone_config(const char *var, const char *value, static int update_submodules(struct submodule_update_clone *suc) { - struct string_list_item *item; + int i; + struct strbuf sb = STRBUF_INIT; run_processes_parallel(suc->max_jobs, update_clone_get_next_task, @@ -1737,9 +1747,16 @@ static int update_submodules(struct submodule_update_clone *suc) if (suc->quickstop) return 1; - for_each_string_list_item(item, >projectlines) - fprintf(stdout, "%s", item->string); + for (i = 0; i < suc->update_clone_nr; i++) { + strbuf_addf(, "dummy %s %d\t%s\n", + oid_to_hex(>update_clone[i].oid), + suc->update_clone[i].just_cloned, + suc->update_clone[i].sub->path); + fprintf(stdout, "%s", sb.buf); + strbuf_reset(); + } + strbuf_release(); return 0; } -- 2.18.0.233.g985f88cf7e-goog
Re: [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct
On Mon, Jul 16, 2018 at 12:37 PM Junio C Hamano wrote: > > Stefan Beller writes: > > > The information that is printed for update_submodules in > > 'submodule--helper update-clone' and consumed by 'git submodule update' > > is stored as a string per submodule. This made sense at the time of > > 48308681b07 (git submodule update: have a dedicated helper for cloning, > > 2016-02-29), but as we want to migrate the rest of the submodule update > > into C, we're better off having access to the raw information in a helper > > struct. > > The reasoning above makes sense, but I have a few niggles on the > naming. > > * Anything you'd place to keep track of the state is "information", >so a whole "_information" suffix to the structure name does not >add much value. We've seen our fair share of (meaningless) >"_data" suffix used in many places but I think the overly long >"_information" that is equally meaningless trumps them. I think >we also have "_info", but if we are not going to have a beter >name, let's not be original and stick to "_data" like other >existing codepath. An alternative with more meaningful name is >of course better, though, but it is not all that much worth to >spend too many braincycles on it. agreed. > * Is the fact that these parameters necessary to help populating >the submodule repository are sent on a line to external process >the most important aspect of the submodule_lines[] array? As In terms of what the submodule--helper does, it is. It is not the most important aspect in the big picture, or once we finish the migration to not have shell interpret its output. >this step is a preparation to migrate out of that line/text >oriented IPC, I think line-ness is the least important and >interesting thing to name the variable with. ok. > If I were writing this patch, perhaps I'd do > > struct update_clone_data *update_clone; > int update_clone_nr, update_clone_alloc; > > following my gut, but since you've been thinking about submodule > longer than I have, perhaps you can come up with a better name. That makes sense. We do not need to mention 'submodule' as that ought to be obvious from the file name already and 'update_clone' is just enough to describe what we are doing. Although there is already struct submodule_update_clone, which would be the better candidate for 'update_clone' and this new struct would be used as a helper in that struct, so update_clone_data I'll think of adding a patch to rename that already existing struct (submodule_update_clone -> update_clone) and renaming this to update_clone_data. > > > @@ -1463,8 +1469,9 @@ struct submodule_update_clone { > > const char *recursive_prefix; > > const char *prefix; > > > > - /* Machine-readable status lines to be consumed by git-submodule.sh */ > > - struct string_list projectlines; > > + /* to be consumed by git-submodule.sh */ > > + struct submodule_update_clone_information *submodule_lines; > > + int submodule_lines_nr; int submodule_lines_alloc; > > > > /* If we want to stop as fast as possible and return an error */ > > unsigned quickstop : 1; > > @@ -1478,7 +1485,7 @@ struct submodule_update_clone { > > #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ > > SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ > > NULL, NULL, NULL, \ > > - STRING_LIST_INIT_DUP, 0, NULL, 0, 0} > > + NULL, 0, 0, 0, NULL, 0, 0, 0} > > The structure definition and this macro definition are nearby, so it > is not crucial, but its probably not a bad idea to switch to C99 > initializers for a thing like this to make it more readable, once > the code around this area stabilizes back again sufficiently (IOW, > let's not distract ourselves in the middle of adding a new feature > like this one). Are we still in the phase of "test balloon" or do we now accept C99 initializers all over the code base? But I agree to defer the conversion for now. Thanks, Stefan
Re: [PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct
Stefan Beller writes: > The information that is printed for update_submodules in > 'submodule--helper update-clone' and consumed by 'git submodule update' > is stored as a string per submodule. This made sense at the time of > 48308681b07 (git submodule update: have a dedicated helper for cloning, > 2016-02-29), but as we want to migrate the rest of the submodule update > into C, we're better off having access to the raw information in a helper > struct. The reasoning above makes sense, but I have a few niggles on the naming. * Anything you'd place to keep track of the state is "information", so a whole "_information" suffix to the structure name does not add much value. We've seen our fair share of (meaningless) "_data" suffix used in many places but I think the overly long "_information" that is equally meaningless trumps them. I think we also have "_info", but if we are not going to have a beter name, let's not be original and stick to "_data" like other existing codepath. An alternative with more meaningful name is of course better, though, but it is not all that much worth to spend too many braincycles on it. * Is the fact that these parameters necessary to help populating the submodule repository are sent on a line to external process the most important aspect of the submodule_lines[] array? As this step is a preparation to migrate out of that line/text oriented IPC, I think line-ness is the least important and interesting thing to name the variable with. If I were writing this patch, perhaps I'd do struct update_clone_data *update_clone; int update_clone_nr, update_clone_alloc; following my gut, but since you've been thinking about submodule longer than I have, perhaps you can come up with a better name. > @@ -1463,8 +1469,9 @@ struct submodule_update_clone { > const char *recursive_prefix; > const char *prefix; > > - /* Machine-readable status lines to be consumed by git-submodule.sh */ > - struct string_list projectlines; > + /* to be consumed by git-submodule.sh */ > + struct submodule_update_clone_information *submodule_lines; > + int submodule_lines_nr; int submodule_lines_alloc; > > /* If we want to stop as fast as possible and return an error */ > unsigned quickstop : 1; > @@ -1478,7 +1485,7 @@ struct submodule_update_clone { > #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ > SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ > NULL, NULL, NULL, \ > - STRING_LIST_INIT_DUP, 0, NULL, 0, 0} > + NULL, 0, 0, 0, NULL, 0, 0, 0} The structure definition and this macro definition are nearby, so it is not crucial, but its probably not a bad idea to switch to C99 initializers for a thing like this to make it more readable, once the code around this area stabilizes back again sufficiently (IOW, let's not distract ourselves in the middle of adding a new feature like this one).
[PATCH 4/6] builtin/submodule--helper: store update_clone information in a struct
The information that is printed for update_submodules in 'submodule--helper update-clone' and consumed by 'git submodule update' is stored as a string per submodule. This made sense at the time of 48308681b07 (git submodule update: have a dedicated helper for cloning, 2016-02-29), but as we want to migrate the rest of the submodule update into C, we're better off having access to the raw information in a helper struct. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 37 +++-- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 96929ba1096..c9c3fe2bf28 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1444,6 +1444,12 @@ static int module_clone(int argc, const char **argv, const char *prefix) return 0; } +struct submodule_update_clone_information { + const struct submodule *sub; + struct object_id oid; + unsigned just_cloned; +}; + struct submodule_update_clone { /* index into 'list', the list of submodules to look into for cloning */ int current; @@ -1463,8 +1469,9 @@ struct submodule_update_clone { const char *recursive_prefix; const char *prefix; - /* Machine-readable status lines to be consumed by git-submodule.sh */ - struct string_list projectlines; + /* to be consumed by git-submodule.sh */ + struct submodule_update_clone_information *submodule_lines; + int submodule_lines_nr; int submodule_lines_alloc; /* If we want to stop as fast as possible and return an error */ unsigned quickstop : 1; @@ -1478,7 +1485,7 @@ struct submodule_update_clone { #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \ SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \ NULL, NULL, NULL, \ - STRING_LIST_INIT_DUP, 0, NULL, 0, 0} + NULL, 0, 0, 0, NULL, 0, 0, 0} static void next_submodule_warn_missing(struct submodule_update_clone *suc, @@ -1572,10 +1579,12 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, strbuf_addf(, "%s/.git", ce->name); needs_cloning = !file_exists(sb.buf); - strbuf_reset(); - strbuf_addf(, "dummy %s %d\t%s\n", - oid_to_hex(>oid), needs_cloning, ce->name); - string_list_append(>projectlines, sb.buf); + ALLOC_GROW(suc->submodule_lines, suc->submodule_lines_nr + 1, +suc->submodule_lines_alloc); + oidcpy(>submodule_lines[suc->submodule_lines_nr].oid, >oid); + suc->submodule_lines[suc->submodule_lines_nr].just_cloned = needs_cloning; + suc->submodule_lines[suc->submodule_lines_nr].sub = sub; + suc->submodule_lines_nr++; if (!needs_cloning) goto cleanup; @@ -1718,7 +1727,8 @@ static int gitmodules_update_clone_config(const char *var, const char *value, static int update_submodules(struct submodule_update_clone *suc) { - struct string_list_item *item; + int i; + struct strbuf sb = STRBUF_INIT; run_processes_parallel(suc->max_jobs, update_clone_get_next_task, @@ -1737,9 +1747,16 @@ static int update_submodules(struct submodule_update_clone *suc) if (suc->quickstop) return 1; - for_each_string_list_item(item, >projectlines) - fprintf(stdout, "%s", item->string); + for (i = 0; i < suc->submodule_lines_nr; i++) { + strbuf_addf(, "dummy %s %d\t%s\n", + oid_to_hex(>submodule_lines[i].oid), + suc->submodule_lines[i].just_cloned, + suc->submodule_lines[i].sub->path); + fprintf(stdout, "%s", sb.buf); + strbuf_reset(); + } + strbuf_release(); return 0; } -- 2.18.0.203.gfac676dfb9-goog