On 9 August 2018 at 00:17, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> Currently when git-fetch is asked to recurse into submodules, it dispatches
> a plain "git-fetch -C <submodule-dir>" (and some submodule related options
> such as prefix and recusing strategy, but) without any information of the
> remote or the tip that should be fetched.
>
> This works surprisingly well in some workflows (such as using submodules
> as a third party library), while not so well in other scenarios, such
> as in a Gerrit topic-based workflow, that can tie together changes
> (potentially across repositories) on the server side. One of the parts
> of such a Gerrit workflow is to download a change when wanting to examine
> it, and you'd want to have its submodule changes that are in the same
> topic downloaded as well. However these submodule changes reside in their
> own repository in their on ref (refs/changes/<int>).
s/on/own/
> Retry fetching a submodule if the object id that the superproject points
> to, cannot be found.
>
> Note: This is an RFC and doesn't support fetching to FETCH_HEAD yet, but
> only into a local branch. To make fetching into FETCH_HEAD work, we need
> some refactoring in builtin/fetch.c to adjust the calls to
> 'check_for_new_submodule_commits'.
>
> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx>
> ---
> diff --git a/submodule.c b/submodule.c
> index ec7ea6f8c2d..6cbd0b1a470 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1127,6 +1127,7 @@ struct submodule_parallel_fetch {
> int result;
>
> struct string_list changed_submodule_names;
> + struct string_list retry;
> };
> #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, STRING_LIST_INIT_DUP }
`retry` will effectively be `STRING_LIST_INIT_NODUP`, but making that
explicit would be better and the next addition to the struct would be
easier to get right.
> +retry_next:
> + retry_it = string_list_pop(&spf->retry);
> + if (retry_it) {
> + struct strbuf submodule_prefix = STRBUF_INIT;
> + const struct submodule *sub =
> + submodule_from_name(spf->r,
> + &null_oid,
> + retry_it->string);
> +
> + child_process_init(cp);
> + cp->dir = get_submodule_git_dir(spf->r, sub->path);
> + if (!cp->dir)
> + goto retry_next;
So here you just drop the string list item. Since it's NODUP, and since
the `util` pointers are owned elsewhere(?), this seems fine. Other uses
of `string_list_pop()` might not be so straightforward.
Just a thought, but rather than pop+if+goto, maybe
while ((retry_it = )) {
...
if (!cp->dir) continue;
...
return 1;
}
I haven't commented on any of the submodule stuff, which is probably
where you'd be most interested in comments. I don't use submodules, nor
do I know the code that runs them.. I guess my comments are more "if
those who know something about submodules find this series worthwhile,
you might want to consider my comments as well".
Martin
