| Message ID | 20180911234951.14129-10-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fetch: make sure submodule oids are fetched | expand |
Stefan Beller <sbeller@google.com> writes: > For Gerrit users that use submodules the invocation of fetch without a > branch is their main use case. That's way under explains this commit. It is totally unclear how that statement of fact relates to the problem this patch is trying to address; it does not even make it clear what problem is being addressed by the patch. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > builtin/fetch.c | 5 ++++- > t/t5526-fetch-submodules.sh | 2 +- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 95c44bf6ffa..ea6ecd123e7 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -887,11 +887,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, > rc |= update_local_ref(ref, what, rm, ¬e, > summary_width); > free(ref); > - } else > + } else { > + check_for_new_submodule_commits(&rm->old_oid); This happens when there is no "ref", which is set only when rm->peer_ref exists, which is set only when we are using remote tracking branch (or more generally storing the fetched rev somewhere in our refs/ hierarchy), e.g. the rev is recorded only in FETCH_HEAD. What does rm->old_oid have in such a case? Is this the tip of the superproject history we just fetched? When we keep record of what we saw in the previous attempt to fetch, we can tell "we have seen their history up to this old commit before, and now we fetched their history up to this new commit" and the question "during that time, which submodules have been modified in the history of the superproject" becomes answerable. When we are not keeping the record of previous fetch, how would we answer that question without going through the whole history? The answer is that check-for-new does not even do the "old branch tip was X and new branch tip is Y, so we can look only at X..Y"; it only cares about the new branch tip of the superproject, and excludes the existing tips of all branches in the superproject (i.e. computing something akin to "Y --not --all" instead of "X..Y"). So, I guess this is probably reasonable. But does the call to "check-for-new submodule" need to be unconditional? In this codepath, do we know when we are not doing a recursive fetch in a superproject? If so, perhaps we can omit the cost of going through all the refs to populate ref_tips_before_fetch array in such a case. > format_display(¬e, '*', > *kind ? kind : "branch", NULL, > *what ? what : "HEAD", > "FETCH_HEAD", summary_width); > + } > + > if (note.len) { > if (verbosity >= 0 && !shown_url) { > fprintf(stderr, _("From %.*s\n"), > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh > index af12c50e7dd..a509eabb044 100755 > --- a/t/t5526-fetch-submodules.sh > +++ b/t/t5526-fetch-submodules.sh > @@ -615,7 +615,7 @@ test_expect_success "fetch new commits on-demand when they are not reachable" ' > git update-ref refs/changes/2 $D && > ( > cd downstream && > - git fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch && > + git fetch --recurse-submodules origin refs/changes/2 && > git -C submodule cat-file -t $C && > git checkout --recurse-submodules FETCH_HEAD > )
diff --git a/builtin/fetch.c b/builtin/fetch.c index 95c44bf6ffa..ea6ecd123e7 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -887,11 +887,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, rc |= update_local_ref(ref, what, rm, ¬e, summary_width); free(ref); - } else + } else { + check_for_new_submodule_commits(&rm->old_oid); format_display(¬e, '*', *kind ? kind : "branch", NULL, *what ? what : "HEAD", "FETCH_HEAD", summary_width); + } + if (note.len) { if (verbosity >= 0 && !shown_url) { fprintf(stderr, _("From %.*s\n"), diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index af12c50e7dd..a509eabb044 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -615,7 +615,7 @@ test_expect_success "fetch new commits on-demand when they are not reachable" ' git update-ref refs/changes/2 $D && ( cd downstream && - git fetch --recurse-submodules --recurse-submodules-default on-demand origin refs/changes/2:refs/heads/my_branch && + git fetch --recurse-submodules origin refs/changes/2 && git -C submodule cat-file -t $C && git checkout --recurse-submodules FETCH_HEAD )
For Gerrit users that use submodules the invocation of fetch without a branch is their main use case. Signed-off-by: Stefan Beller <sbeller@google.com> --- builtin/fetch.c | 5 ++++- t/t5526-fetch-submodules.sh | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-)