v2: * extended commit messages, * plugged a memory leak * rewrote the patch "sha1-array: provide oid_array_filter" to be much more like object_array_fiter * fixed a typo pointed out by Ramsay. The range diff is below. Thanks, Stefan v1: 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, not so well in others, which this series aims to fix. The first patches provide new basic functionality and do some refactoring; the interesting part is in the two last patches. This was discussed in https://public-inbox.org/git/20180808221752.195419-1-sbeller@xxxxxxxxxx/ and I think I addressed all feedback so far. Stefan Beller (9): string-list: add string_list_{pop, last} functions sha1-array: provide oid_array_filter submodule.c: fix indentation submodule.c: sort changed_submodule_names before searching it submodule: move global changed_submodule_names into fetch submodule struct submodule.c: do not copy around submodule list submodule: fetch in submodules git directory instead of in worktree fetch: retry fetching submodules if needed objects were not fetched builtin/fetch: check for submodule updates for non branch fetches builtin/fetch.c | 14 +-- sha1-array.c | 17 ++++ sha1-array.h | 9 ++ string-list.c | 14 +++ string-list.h | 15 +++ submodule.c | 191 ++++++++++++++++++++++++++++-------- t/t5526-fetch-submodules.sh | 23 ++++- 7 files changed, 236 insertions(+), 47 deletions(-) git range-diff origin/sb/submodule-recursive-fetch-gets-the-tip...HEAD 1: 6fecf7cd01a ! 1: 54d31d90734 string-list: add string_list_{pop, last} functions @@ -12,8 +12,8 @@ - string_list_pop() removes the string_list_item at the end of the string list. - - string_list_push() is not added, but string_list_append() can be - used in its place. + - there is no string_list_push(); string_list_append() can be used + in its place. You can use them in this pattern: @@ -26,7 +26,6 @@ Helped-by: Junio C Hamano <gitster@xxxxxxxxx> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/string-list.c b/string-list.c --- a/string-list.c @@ -66,6 +65,10 @@ + */ +void string_list_pop(struct string_list *list, int free_util); + ++/* ++ * Returns the last item of the list. As it returns the raw access, do not ++ * modify the list while holding onto the returned pointer. ++ */ +static inline struct string_list_item *string_list_last(struct string_list *list) +{ + return &list->items[list->nr - 1]; 2: 7007a318a68 < -: ----------- sha1-array: provide oid_array_filter -: ----------- > 2: a2bd6ef2bf0 sha1-array: provide oid_array_filter 3: 807429234ac ! 3: 0300c27cbd7 submodule.c: fix indentation @@ -6,7 +6,6 @@ Fix it while we are here. Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/submodule.c b/submodule.c --- a/submodule.c 4: f6fa5273af9 ! 4: 80cf0221bbe submodule.c: sort changed_submodule_names before searching it @@ -12,7 +12,6 @@ appropriate. Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/submodule.c b/submodule.c --- a/submodule.c 5: adf7a2fd203 ! 5: 7ddb448b748 submodule: move global changed_submodule_names into fetch submodule struct @@ -6,7 +6,6 @@ part of the struct that is passed around for fetching submodules. Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/submodule.c b/submodule.c --- a/submodule.c 6: 56c9398589a ! 6: 7975a7f1e3b submodule.c: do not copy around submodule list @@ -9,12 +9,11 @@ Instead use the result list directly and prune items afterwards using string_list_remove_empty_items. - By doin so we'll have access to the util pointer for longer that + By doing so we'll have access to the util pointer for longer that contains the commits that we need to fetch, which will be useful in a later patch. Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/submodule.c b/submodule.c --- a/submodule.c 7: 9f70a5f32c9 ! 7: 29bc2868f26 submodule: fetch in submodules git directory instead of in worktree @@ -3,14 +3,14 @@ submodule: fetch in submodules git directory instead of in worktree This patch started as a refactoring to make 'get_next_submodule' more - readable, but upon doing so, I realized that git-fetch actually doesn't - need to be run in the worktree. So let's run it in the git dir instead. + readable, but upon doing so, I realized that "git fetch" of the submodule + actually doesn't need to be run in the submodules worktree. So let's run + it in its git dir instead. That should pave the way towards fetching submodules that are currently not checked out. Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/submodule.c b/submodule.c --- a/submodule.c @@ -32,7 +32,7 @@ return spf->default_option; } -+static const char *get_submodule_git_dir(struct repository *r, const char *path) ++static char *get_submodule_git_dir(struct repository *r, const char *path) +{ + struct repository subrepo; + const char *ret; @@ -87,7 +87,11 @@ if (!spf->quiet) strbuf_addf(err, "Fetching submodule %s%s\n", @@ + argv_array_push(&cp->args, default_argv); + argv_array_push(&cp->args, "--submodule-prefix"); argv_array_push(&cp->args, submodule_prefix.buf); ++ ++ free(git_dir); ret = 1; } - strbuf_release(&submodule_path); 8: bab609b4dc1 ! 8: f837c4a0789 fetch: retry fetching submodules if sha1 were not fetched @@ -1,9 +1,9 @@ Author: Stefan Beller <sbeller@xxxxxxxxxx> - fetch: retry fetching submodules if sha1 were not fetched + fetch: retry fetching submodules if needed objects were not fetched Currently when git-fetch is asked to recurse into submodules, it dispatches - a plain "git-fetch -C <submodule-dir>" (and some submodule related options + a plain "git-fetch -C <submodule-dir>" (with some submodule related options such as prefix and recusing strategy, but) without any information of the remote or the tip that should be fetched. @@ -16,16 +16,22 @@ topic downloaded as well. However these submodule changes reside in their own repository in their own ref (refs/changes/<int>). - Retry fetching a submodule if the object id that the superproject points - to, cannot be found. + Retry fetching a submodule by object name 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. + 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. A later patch will fix this. + + builtin/fetch used to only inspect submodules when they were fetched + "on-demand", as in either on/off case it was clear whether the submodule + needs to be fetched. However to know whether we need to try fetching the + object ids, we need to identify the object names, which is done in this + function check_for_new_submodule_commits(), so we'll also run that code + in case the submodule recursion is set to "on". Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/builtin/fetch.c b/builtin/fetch.c --- a/builtin/fetch.c 9: c16d21313f6 ! 9: 71f2bb035b1 builtin/fetch: check for submodule updates for non branch fetches @@ -2,11 +2,29 @@ builtin/fetch: check for submodule updates for non branch fetches - For Gerrit users that use submodules the invocation of fetch without a - branch is their main use case. + Gerrit, the code review tool, has a different workflow than our mailing + list based approach. Usually users upload changes to a Gerrit server and + continuous integration and testing happens by bots. Sometimes however a + user wants to checkout a change locally and look at it locally. For this + use case, Gerrit offers a command line snippet to copy and paste to your + terminal, which looks like + + git fetch https://<host>/gerrit refs/changes/<id> && + git checkout FETCH_HEAD + + For Gerrit changes that contain changing submodule gitlinks, it would be + easy to extend both the fetch and checkout with the '--recurse-submodules' + flag, such that this command line snippet would produce the state of a + change locally. + + However the functionality added in the previous patch, which would + ensure that we fetch the objects in the submodule that the gitlink pointed + at, only works for remote tracking branches so far, not for FETCH_HEAD. + + Make sure that fetching a superproject to its FETCH_HEAD, also respects + the existence checks for objects in the submodule recursion. Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/builtin/fetch.c b/builtin/fetch.c --- a/builtin/fetch.c
