Re: [PATCH 10/10] fetch: retry fetching submodules if sha1 were not fetched

From: Martin Ågren <martin.agren@gmail.com>
Date: Aug 9, 2018, 07:50 UTC
Message-ID: <CAN0heSpsbYWzujzyteWuhto9DTXzvAkP+vt++d7ar3ob6Zx=Gg@mail.gmail.com>
In-Reply-To: <20180808221752.195419-11-sbeller@google.com>

On 9 August 2018 at 00:17, Stefan Beller <sbeller@google.com> wrote:

Show 13 quoted lines

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

Show 10 quoted lines

> 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@google.com>
> ---

Show 11 quoted lines

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

Show 13 quoted lines

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