Re: [PATCH 8/9] fetch: retry fetching submodules if sha1 were not fetched
Stefan Beller writes:
> Retry fetching a submodule if the object id that the superproject points
> to, cannot be found.
By object name? By attempting to fetch all refs? Or by doing
something else? Fetching by the exact object name is the most
efficient approach, but the server side may not be prepared to
serve such a request, and that is why spelling it out here would
help the readers.
> This doesn't support fetching to FETCH_HEAD yet, but only into a local
> branch.
It is not clear if this sentence is talking about the fetch done at
the superproject level, or what happens in a submodule repository
when this "retrying" happens. Assuming that it is the former,
perhaps
This retrying does not happen when the "git fetch" done at the
superproject is not storing the fetched results in remote
tracking branches (i.e. instead just recording them to
FETCH_HEAD) in this step.
would help the readers understand what you are trying to say.
> 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'
> that is coming in the next patch.
>
> Signed-off-by: Stefan Beller
> ---
> builtin/fetch.c | 9 ++--
> submodule.c | 87 -
> t/t5526-fetch-submodules.sh | 16 +++
> 3 files changed, 104 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 61bec5d213d..95c44bf6ffa 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -700,8 +700,7 @@ static int update_local_ref(struct ref *ref,
> what = _("[new ref]");
> }
>
> - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> - (recurse_submodules != RECURSE_SUBMODULES_ON))
> + if (recurse_submodules != RECURSE_SUBMODULES_OFF)
> check_for_new_submodule_commits(>new_oid);
> r = s_update_ref(msg, ref, 0);
> format_display(display, r ? '!' : '*', what,
> @@ -716,8 +715,7 @@ static int update_local_ref(struct ref *ref,
> strbuf_add_unique_abbrev(, >object.oid,
> DEFAULT_ABBREV);
> strbuf_addstr(, "..");
> strbuf_add_unique_abbrev(, >new_oid,
> DEFAULT_ABBREV);
> - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> - (recurse_submodules != RECURSE_SUBMODULES_ON))
> + if (recurse_submodules != RECURSE_SUBMODULES_OFF)
> check_for_new_submodule_commits(>new_oid);
> r = s_update_ref("fast-forward", ref, 1);
> format_display(display, r ? '!' : ' ', quickref.buf,
> @@ -731,8 +729,7 @@ static int update_local_ref(struct ref *ref,
> strbuf_add_unique_abbrev(, >object.oid,
> DEFAULT_ABBREV);
> strbuf_addstr(, "...");
> strbuf_add_unique_abbrev(, >new_oid,
> DEFAULT_ABBREV);
> - if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
> - (recurse_submodules != RECURSE_SUBMODULES_ON))
> + if (recurse_submodules != RECURSE_SUBMODULES_OFF)
> check_for_new_submodule_commits(>new_oid);
> r = s_update_ref("forced-update", ref, 1);
> format_display(display, r ? '!' : '+', quickref.buf,
All of these used to react to any value set to recurse-submodules
that is not off or on (i.e. on-demand, default, none), but now
unless the value is explicitly set to off, we call into the check.
It is not immediately clear how that change is linked to the
retrying. "When set to 'on', we did not check for new commits, but
now we do" can be read from the patch text but not the reasoning
behind it.
What was the reason why we didn't call "check-for-new" when recurse
is set to "on"? Is it because "we are going to recurse anyway, so
there is no need to check to decide if we need to fetch in
submodule"? And the reason why we now need to call when we are set
to recurse anyway is because check-for-new now learns much more than
just "do we need to cd there and run git-fetch? yes/no?"
The answers to these two questions would help readers in the log
message.
> diff --git a/submodule.c b/submodule.c
> index 1e6781504f0..a75146e89cf 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1127,8 +1127,11 @@ 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 }
> +#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
> + STRING_LIST_INIT_DUP, \
> + STRING_LIST_INIT_NODUP}
>
> static void calculate_changed_submodule_paths(
> struct submodule_parallel_fetch *spf)
> @@ -1259,8 +1262,10 @@ static int get_next_submodule(struct child_process *cp,
> {
> int ret = 0;
> struct
[PATCH 8/9] fetch: retry fetching submodules if sha1 were not fetched
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (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 own ref (refs/changes/).
Retry fetching a submodule if the object id that the superproject points
to, cannot be found.
This 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'
that is coming in the next patch.
Signed-off-by: Stefan Beller
---
builtin/fetch.c | 9 ++--
submodule.c | 87 -
t/t5526-fetch-submodules.sh | 16 +++
3 files changed, 104 insertions(+), 8 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213d..95c44bf6ffa 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -700,8 +700,7 @@ static int update_local_ref(struct ref *ref,
what = _("[new ref]");
}
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
@@ -716,8 +715,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "..");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
@@ -731,8 +729,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "...");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
diff --git a/submodule.c b/submodule.c
index 1e6781504f0..a75146e89cf 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1127,8 +1127,11 @@ 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 }
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
+ STRING_LIST_INIT_DUP, \
+ STRING_LIST_INIT_NODUP}
static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
@@ -1259,8 +1262,10 @@ static int get_next_submodule(struct child_process *cp,
{
int ret = 0;
struct submodule_parallel_fetch *spf = data;
+ struct string_list_item *it;
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
+ int recurse_config;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
const char *git_dir, *default_argv;
@@ -1280,7 +1285,9 @@ static int get_next_submodule(struct child_process *cp,
}
}
- switch (get_fetch_recurse_config(submodule, spf))
+ recurse_config = get_fetch_recurse_config(submodule, spf);
+
+ switch (recurse_config)
{
default:
case RECURSE_SUBMODULES_DEFAULT:
@@ -1319,9 +1326,50 @@ static int get_next_submodule(struct child_process *cp,
strbuf_release(_prefix);
if (ret) {
