| Message ID | xmqqk0alyqyj.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0353c6881890db1302f0f1bdf85c6076eed61113 |
| Headers | show |
| Series | [v2] fetch: do not run a redundant fetch from submodule | expand |
This version looks good to me, thanks :) Reviewed-by: Glen Choo <chooglen@google.com> Junio C Hamano <gitster@pobox.com> writes: > t5617 is much cleanly organized than t5526, and we may want to clean > up the latter after dust settles. Yeah, t5526 has so many tests for the 'core' functionality that it's hard to fit something 'tangential' like "--all". I might touch it again soon, so I'll keep this in mind. > diff --git a/builtin/fetch.c b/builtin/fetch.c > index e3791f09ed..8b15c40bb2 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -2261,7 +2265,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > result = fetch_multiple(&list, max_children); > } > > - if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { > + > + /* > + * This is only needed after fetch_one(), which does not fetch > + * submodules by itself. > + * > + * When we fetch from multiple remotes, fetch_multiple() has > + * already updated submodules to grab commits necessary for > + * the fetched history from each remote, so there is no need > + * to fetch submodules from here. > + */ > + if (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { > struct strvec options = STRVEC_INIT; > int max_children = max_jobs; Looks good; the comment is easier to understand than my suggestion for sure. > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh > index 43dada8544..a301b56db8 100755 > --- a/t/t5526-fetch-submodules.sh > +++ b/t/t5526-fetch-submodules.sh > @@ -1125,4 +1125,31 @@ test_expect_success 'fetch --recurse-submodules updates name-conflicted, unpopul > ) > ' > > +test_expect_success 'fetch --all with --recurse-submodules' ' > + test_when_finished "rm -fr src_clone" && > + git clone --recurse-submodules src src_clone && > + ( > + cd src_clone && > + git config submodule.recurse true && > + git config fetch.parallel 0 && > + git fetch --all 2>../fetch-log > + ) && > + grep "^Fetching submodule sub$" fetch-log >fetch-subs && > + test_line_count = 1 fetch-subs > +' > + > +test_expect_success 'fetch --all with --recurse-submodules with multiple' ' > + test_when_finished "rm -fr src_clone" && > + git clone --recurse-submodules src src_clone && > + ( > + cd src_clone && > + git remote add secondary ../src && > + git config submodule.recurse true && > + git config fetch.parallel 0 && > + git fetch --all 2>../fetch-log > + ) && > + grep "Fetching submodule sub" fetch-log >fetch-subs && > + test_line_count = 2 fetch-subs > +' > + Also looks good.
Glen Choo <chooglen@google.com> writes: >> + >> + /* >> + * This is only needed after fetch_one(), which does not fetch >> + * submodules by itself. >> + * >> + * When we fetch from multiple remotes, fetch_multiple() has >> + * already updated submodules to grab commits necessary for >> + * the fetched history from each remote, so there is no need >> + * to fetch submodules from here. >> + */ >> + if (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { >> struct strvec options = STRVEC_INIT; >> int max_children = max_jobs; > > Looks good; the comment is easier to understand than my suggestion for > sure. Thanks. Today's code has diverged too much from the original code I wrote long time ago (before submodules), and I needed an extra set of eyeballs to double check and tell me that what I (wishfully) wrote how the code works with submodules is in line with today's code ;-)
diff --git a/builtin/fetch.c b/builtin/fetch.c index e3791f09ed..8b15c40bb2 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -2187,6 +2187,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) else if (argc > 1) die(_("fetch --all does not make sense with refspecs")); (void) for_each_remote(get_one_remote_for_fetch, &list); + + /* do not do fetch_multiple() of one */ + if (list.nr == 1) + remote = remote_get(list.items[0].string); } else if (argc == 0) { /* No arguments -- use default remote */ remote = remote_get(NULL); @@ -2261,7 +2265,17 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) result = fetch_multiple(&list, max_children); } - if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { + + /* + * This is only needed after fetch_one(), which does not fetch + * submodules by itself. + * + * When we fetch from multiple remotes, fetch_multiple() has + * already updated submodules to grab commits necessary for + * the fetched history from each remote, so there is no need + * to fetch submodules from here. + */ + if (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) { struct strvec options = STRVEC_INIT; int max_children = max_jobs; diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index 43dada8544..a301b56db8 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -1125,4 +1125,31 @@ test_expect_success 'fetch --recurse-submodules updates name-conflicted, unpopul ) ' +test_expect_success 'fetch --all with --recurse-submodules' ' + test_when_finished "rm -fr src_clone" && + git clone --recurse-submodules src src_clone && + ( + cd src_clone && + git config submodule.recurse true && + git config fetch.parallel 0 && + git fetch --all 2>../fetch-log + ) && + grep "^Fetching submodule sub$" fetch-log >fetch-subs && + test_line_count = 1 fetch-subs +' + +test_expect_success 'fetch --all with --recurse-submodules with multiple' ' + test_when_finished "rm -fr src_clone" && + git clone --recurse-submodules src src_clone && + ( + cd src_clone && + git remote add secondary ../src && + git config submodule.recurse true && + git config fetch.parallel 0 && + git fetch --all 2>../fetch-log + ) && + grep "Fetching submodule sub" fetch-log >fetch-subs && + test_line_count = 2 fetch-subs +' + test_done