[PATCH v4 00/10] fetch --recurse-submodules: fetch unpopulated submodules
- From
Glen Choo <chooglen@google.com>
- Date
- Mar 4, 2022, 00:57 UTC
- Message-ID
- <20220304005757.70107-1-chooglen@google.com>
- In-Reply-To
- <20220224100842.95827-1-chooglen@google.com>
Original cover letter: https://lore.kernel.org/git/20220210044152.78352-1-chooglen@google.com
This round of patches is now based on master - I prepared the previous rounds on top of gc/branch-recurse-submodules, but now that's merged onto master (and the actual branch for this series, gc/recursive-fetch-with-unused-submodules, is based off master anyway).
This round fixes up the comments from the previous round (thanks everyone!), all of which are fairly small.
= Patch organization
- Patches 1-3 are quality-of-life improvements to the test suite that make it easier to write the tests in patch 9. - Patches 4-6 are preparation for "git fetch" to read .gitmodules from the superproject commit in patch 7. - Patches 7-8 refactor out the logic of "finding which submodules to fetch" and "fetching the submodules", making it easier to tell "git fetch" to fetch unpopulated submodules. - Patch 9 teaches "git fetch" to fetch changed, unpopulated submodules in addition to populated submodules. - Patch 10 is an optional bugfix + cleanup of the "git fetch" code that removes the last caller of the deprecated "add_submodule_odb()".
= Changes
== Since v3 - Numerous style fixes + improved comments. - Fix sed portability issues. - Fix failing test due to default branch name assumptions. - Patch 3: change a test so that it no longer depends on state from the previous test. - Patch 9: fix memory leak when recording super_oid and path + add explanatory comment.
== Since v2
- Numerous small fixes to the code and commit message (thanks to all who
helped spot these :))
- In patch 2, use test_cmp + sed to assert on test output, effectively
reverting the "use grep" approach of v1-2 (see patch 2's description).
- New patch 3: introduce a test helper that creates the expected
superproject commit (instead of copy-pasting the code over and over).
- I did not get rid of "git fetch" inside the test helper (as Jonathan
suggested) though, because that requires a bigger change in the test
setup, and I think the test helper makes the test straightforward
enough.
- New patch 8: refactor some shared logic out into fetch_task_create().
This reduces code duplication between the get_fetch_task_from_*
functions.
- In patch 9, add additional tests for 'submodules with the same name'.
- In patch 9, handle a bug where a submodule that is unpopulated by "git
rm" still has "core.worktree" set and cannot be fetched (see patch 9's
description).
- Remove the "git fetch --update-shallow" patch (I'll try to send it
separately).== Since v1
- Numerous style fixes suggested by Jonathan (thanks!)
- In patch 3, don't prematurely read submodules from the superproject
commit (see:
<kl6l5yplyat6.fsf@chooglen-macbookpro.roam.corp.google.com>).
- In patch 7, stop using "git checkout" and "! grep" in tests.
- In patch 7, stop doing the "find changed submodules" rev walk
unconditionally. Instead, continue to check for .gitmodules, but also
check for submodules in $GIT_DIR/modules.
- I'm not entirely happy with the helper function name, see "---" for
details.
- Move "git fetch --update-shallow" bugfix to patch 8.
- Because the "find changed submodules" rev walk is no longer
unconditional, this fix is no longer needed for tests to pass.
- Rename fetch_populated_submodules() to fetch_submodules().Glen Choo (10): t5526: introduce test helper to assert on fetches t5526: stop asserting on stderr literally t5526: create superproject commits with test helper submodule: make static functions read submodules from commits submodule: inline submodule_commits() into caller submodule: store new submodule commits oid_array in a struct submodule: extract get_fetch_task() submodule: move logic into fetch_task_create() fetch: fetch unpopulated, changed submodules submodule: fix latent check_has_commit() bug
Documentation/fetch-options.txt | 26 +- Documentation/git-fetch.txt | 10 +- builtin/fetch.c | 14 +- submodule.c | 442 +++++++++++++++++--------- submodule.h | 21 +- t/t5526-fetch-submodules.sh | 539 ++++++++++++++++++++++++-------- 6 files changed, 740 insertions(+), 312 deletions(-)
Range-diff against v3:
1: b6d34b0f5c ! 1: 57cd31afc2 t5526: introduce test helper to assert on fetches
@@ t/t5526-fetch-submodules.sh: add_upstream_commit() {
+verify_fetch_result() {
+ ACTUAL_ERR=$1 &&
+ rm -f expect.err.combined &&
-+ if [ -f expect.err.super ]; then
++ if test -f expect.err.super
++ then
+ cat expect.err.super >>expect.err.combined
+ fi &&
-+ if [ -f expect.err.sub ]; then
++ if test -f expect.err.sub
++ then
+ cat expect.err.sub >>expect.err.combined
+ fi &&
-+ if [ -f expect.err.deep ]; then
++ if test -f expect.err.deep
++ then
+ cat expect.err.deep >>expect.err.combined
+ fi &&
+ test_cmp expect.err.combined $ACTUAL_ERR
2: 0b85fa35c2 ! 2: b70c894cff t5526: stop asserting on stderr literally
@@ t/t5526-fetch-submodules.sh: export GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB
pwd=$(pwd)
-+check_sub() {
++check_sub () {
+ NEW_HEAD=$1 &&
-+ cat <<-EOF >$pwd/expect.err.sub
++ cat >$pwd/expect.err.sub <<-EOF
+ Fetching submodule submodule
+ From $pwd/submodule
+ OLD_HEAD..$NEW_HEAD sub -> origin/sub
+ EOF
+}
+
-+check_deep() {
++check_deep () {
+ NEW_HEAD=$1 &&
-+ cat <<-EOF >$pwd/expect.err.deep
++ cat >$pwd/expect.err.deep <<-EOF
+ Fetching submodule submodule/subdir/deepsubmodule
+ From $pwd/deepsubmodule
+ OLD_HEAD..$NEW_HEAD deep -> origin/deep
+ EOF
+}
+
-+check_super() {
++check_super () {
+ NEW_HEAD=$1 &&
-+ cat <<-EOF >$pwd/expect.err.super
++ cat >$pwd/expect.err.super <<-EOF
+ From $pwd/.
+ OLD_HEAD..$NEW_HEAD super -> origin/super
+ EOF
@@ t/t5526-fetch-submodules.sh: pwd=$(pwd)
)
}
+@@ t/t5526-fetch-submodules.sh: add_upstream_commit() {
+ #
+ # If a repo should not be fetched in the test, its corresponding
+ # expect.err file should be rm-ed.
+-verify_fetch_result() {
++verify_fetch_result () {
+ ACTUAL_ERR=$1 &&
+ rm -f expect.err.combined &&
+ if test -f expect.err.super
@@ t/t5526-fetch-submodules.sh: verify_fetch_result() {
- if [ -f expect.err.deep ]; then
+ then
cat expect.err.deep >>expect.err.combined
fi &&
- test_cmp expect.err.combined $ACTUAL_ERR
-+ sed -E 's/[0-9a-f]+\.\./OLD_HEAD\.\./' $ACTUAL_ERR >actual.err.cmp &&
++ sed -e 's/[0-9a-f][0-9a-f]*\.\./OLD_HEAD\.\./' "$ACTUAL_ERR" >actual.err.cmp &&
+ test_cmp expect.err.combined actual.err.cmp
}
3: bb8ef6094a ! 3: 7e2a01164e t5526: create superproject commits with test helper
@@ Commit message
Signed-off-by: Glen Choo <chooglen@google.com>
## t/t5526-fetch-submodules.sh ##
-@@ t/t5526-fetch-submodules.sh: check_super() {
+@@ t/t5526-fetch-submodules.sh: check_super () {
# a file that contains the expected err if that new commit were fetched.
# These output files get concatenated in the right order by
# verify_fetch_result().
-add_upstream_commit() {
-+add_submodule_commits() {
++add_submodule_commits () {
(
cd submodule &&
echo new >> subfile &&
@@ t/t5526-fetch-submodules.sh: add_upstream_commit() {
+#
+# This requires add_submodule_commits() to be called first, otherwise
+# the submodules will not have changed and cannot be "git add"-ed.
-+add_superproject_commits() {
-+(
-+ cd submodule &&
++add_superproject_commits () {
+ (
-+ cd subdir/deepsubmodule &&
-+ git fetch &&
-+ git checkout -q FETCH_HEAD
-+ ) &&
++ cd submodule &&
++ (
++ cd subdir/deepsubmodule &&
++ git fetch &&
++ git checkout -q FETCH_HEAD
++ ) &&
+ git add subdir/deepsubmodule &&
+ git commit -m "new deep submodule"
+ ) &&
4: e83a1713c4 = 4: 88112ee225 submodule: make static functions read submodules from commits
5: e27d402b9a = 5: 007cd97aba submodule: inline submodule_commits() into caller
6: 1c7c8218b8 = 6: f34ea88fe9 submodule: store new submodule commits oid_array in a struct
7: 80cf317722 ! 7: f66ab663c5 submodule: extract get_fetch_task()
@@ submodule.c: struct fetch_task {
struct repository *repo;
const struct submodule *sub;
unsigned free_sub : 1; /* Do we need to free the submodule? */
-+ const char *default_argv;
++ const char *default_argv; /* The default fetch mode. */
struct oid_array *commits; /* Ensure these commits are fetched */
};
8: bf9cfa7054 = 8: 4e3db1bc9d submodule: move logic into fetch_task_create()
9: c7c2ff71b6 ! 9: 9e7b1c1bbe fetch: fetch unpopulated, changed submodules
@@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)
## submodule.c ##
@@ submodule.c: static const char *default_name_or_path(const char *path_or_name)
- * member of the changed submodule string_list_item.
+
+ /*
+ * Holds relevant information for a changed submodule. Used as the .util
+- * member of the changed submodule string_list_item.
++ * member of the changed submodule name string_list_item.
++ *
++ * (super_oid, path) allows the submodule config to be read from _some_
++ * .gitmodules file. We store this information the first time we find a
++ * superproject commit that points to the submodule, but this is
++ * arbitrary - we can choose any (super_oid, path) that matches the
++ * submodule's name.
*/
struct changed_submodule_data {
+ /*
-+ * The first superproject commit in the rev walk that points to the
-+ * submodule.
++ * The first superproject commit in the rev walk that points to
++ * the submodule.
+ */
+ const struct object_id *super_oid;
+ /*
@@ submodule.c: struct changed_submodule_data {
static void collect_changed_submodules_cb(struct diff_queue_struct *q,
@@ submodule.c: static void collect_changed_submodules_cb(struct diff_queue_struct *q,
- if (!item->util)
+ continue;
+
+ item = string_list_insert(changed, name);
+- if (!item->util)
++ if (item->util)
++ cs_data = item->util;
++ else {
item->util = xcalloc(1, sizeof(struct changed_submodule_data));
- cs_data = item->util;
-+ cs_data->super_oid = commit_oid;
-+ cs_data->path = xstrdup(p->two->path);
+- cs_data = item->util;
++ cs_data = item->util;
++ cs_data->super_oid = commit_oid;
++ cs_data->path = xstrdup(p->two->path);
++ }
oid_array_append(&cs_data->new_commits, &p->two->oid);
}
}
@@ submodule.c: void check_for_new_submodule_commits(struct object_id *oid)
+ */
+static int repo_has_absorbed_submodules(struct repository *r)
+{
++ int ret;
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_repo_git_path(&buf, r, "modules/");
-+ return file_exists(buf.buf) && !is_empty_dir(buf.buf);
++ ret = file_exists(buf.buf) && !is_empty_dir(buf.buf);
++ strbuf_release(&buf);
++ return ret;
+}
+
static void calculate_changed_submodule_paths(struct repository *r,
@@ submodule.c: int submodule_touches_in_range(struct repository *r,
struct submodule_parallel_fetch {
- int count;
++ /*
++ * The index of the last index entry processed by
++ * get_fetch_task_from_index().
++ */
+ int index_count;
++ /*
++ * The index of the last string_list entry processed by
++ * get_fetch_task_from_changed().
++ */
+ int changed_count;
struct strvec args;
struct repository *r;
const char *prefix;
@@ submodule.c: struct submodule_parallel_fetch {
+ int quiet;
int result;
++ /*
++ * Names of submodules that have new commits. Generated by
++ * walking the newly fetched superproject commits.
++ */
struct string_list changed_submodule_names;
++ /*
++ * Names of submodules that have already been processed. Lets us
++ * avoid fetching the same submodule more than once.
++ */
+ struct string_list seen_submodule_names;
/* Pending fetches by OIDs */
@@ submodule.c: struct submodule_parallel_fetch {
@@ submodule.c: struct fetch_task {
const struct submodule *sub;
unsigned free_sub : 1; /* Do we need to free the submodule? */
- const char *default_argv;
-+ struct strvec git_args;
+ const char *default_argv; /* The default fetch mode. */
++ struct strvec git_args; /* Args for the child git process. */
struct oid_array *commits; /* Ensure these commits are fetched */
};
@@ submodule.c: get_fetch_task(struct submodule_parallel_fetch *spf, struct strbuf
+
+ spf->changed_count++;
+ /*
-+ * NEEDSWORK: A submodule unpopulated by "git rm" will
-+ * have core.worktree set, but the actual core.worktree
-+ * directory won't exist, causing the child process to
-+ * fail. Forcibly set --work-tree until we get smarter
-+ * handling for core.worktree in unpopulated submodules.
++ * NEEDSWORK: Submodules set/unset a value for
++ * core.worktree when they are populated/unpopulated by
++ * "git checkout" (and similar commands, see
++ * submodule_move_head() and
++ * connect_work_tree_and_git_dir()), but if the
++ * submodule is unpopulated in another way (e.g. "git
++ * rm", "rm -r"), core.worktree will still be set even
++ * though the directory doesn't exist, and the child
++ * process will crash while trying to chdir into the
++ * nonexistent directory.
++ *
++ * In this case, we know that the submodule has no
++ * working tree, so we can work around this by
++ * setting "--work-tree=." (--bare does not work because
++ * worktree settings take precedence over bare-ness).
++ * However, this is not necessarily true in other cases,
++ * so a generalized solution is still necessary.
++ *
++ * Possible solutions:
++ * - teach "git [add|rm]" to unset core.worktree and
++ * discourage users from removing submodules without
++ * using a Git command.
++ * - teach submodule child processes to ignore stale
++ * core.worktree values.
+ */
+ strvec_push(&task->git_args, "--work-tree=.");
+ return task;
@@ submodule.h: int should_update_submodules(void);
## t/t5526-fetch-submodules.sh ##
@@ t/t5526-fetch-submodules.sh: pwd=$(pwd)
- check_sub() {
+ check_sub () {
NEW_HEAD=$1 &&
+ SUPER_HEAD=$2 &&
- cat <<-EOF >$pwd/expect.err.sub
+ cat >$pwd/expect.err.sub <<-EOF
- Fetching submodule submodule
+ Fetching submodule submodule${SUPER_HEAD:+ at commit $SUPER_HEAD}
From $pwd/submodule
OLD_HEAD..$NEW_HEAD sub -> origin/sub
EOF
-@@ t/t5526-fetch-submodules.sh: check_sub() {
+@@ t/t5526-fetch-submodules.sh: check_sub () {
- check_deep() {
+ check_deep () {
NEW_HEAD=$1 &&
+ SUB_HEAD=$2 &&
- cat <<-EOF >$pwd/expect.err.deep
+ cat >$pwd/expect.err.deep <<-EOF
- Fetching submodule submodule/subdir/deepsubmodule
+ Fetching submodule submodule/subdir/deepsubmodule${SUB_HEAD:+ at commit $SUB_HEAD}
From $pwd/deepsubmodule
OLD_HEAD..$NEW_HEAD deep -> origin/deep
EOF
+@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-demand' doesn't recurse when no ne
+ '
+
+ test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necessary (and ignores config)" '
++ add_submodule_commits &&
+ add_superproject_commits &&
+ (
+ cd downstream &&
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-demand' recurses as deep as necess
verify_fetch_result actual.err
'
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ # Use test_cmp manually because verify_fetch_result does not
+ # consider submodule2. All the repos should be fetched, but only
+ # submodule2 should be read from a commit
-+ cat <<-EOF > expect.err.combined &&
++ cat > expect.err.combined <<-EOF &&
+ From $pwd/.
+ OLD_HEAD..$super_head super -> origin/super
+ OLD_HEAD..$super_sub2_only_head super-sub2-only -> origin/super-sub2-only
@@ t/t5526-fetch-submodules.sh: test_expect_success "'--recurse-submodules=on-deman
+ From $pwd/submodule2
+ OLD_HEAD..$sub2_head sub2 -> origin/sub2
+ EOF
-+ sed -E "s/[0-9a-f]+\.\./OLD_HEAD\.\./" actual.err >actual.err.cmp &&
++ sed -e "s/[0-9a-f][0-9a-f]*\.\./OLD_HEAD\.\./" actual.err >actual.err.cmp &&
+ test_cmp expect.err.combined actual.err.cmp
+'
+
@@ t/t5526-fetch-submodules.sh: test_expect_success 'recursive fetch after deinit a
+ mkdir same-name-1 &&
+ (
+ cd same-name-1 &&
-+ git init &&
++ git init -b main &&
+ test_commit --no-tag a
+ ) &&
+ git clone same-name-1 same-name-2 &&
@@ t/t5526-fetch-submodules.sh: test_expect_success 'recursive fetch after deinit a
+ (
+ cd same-name-1 &&
+ mkdir submodule &&
-+ git -C submodule init &&
++ git -C submodule init -b main &&
+ test_commit -C submodule --no-tag a1 &&
+ git submodule add "$pwd/same-name-1/submodule" &&
+ git add submodule &&
@@ t/t5526-fetch-submodules.sh: test_expect_success 'recursive fetch after deinit a
+ (
+ cd same-name-2 &&
+ mkdir submodule &&
-+ git -C submodule init &&
++ git -C submodule init -b main &&
+ test_commit -C submodule --no-tag a2 &&
+ git submodule add "$pwd/same-name-2/submodule" &&
+ git add submodule &&
@@ t/t5526-fetch-submodules.sh: test_expect_success 'recursive fetch after deinit a
+'
+
+test_expect_success 'fetch --recurse-submodules updates name-conflicted, populated submodule' '
-+ test_when_finished "git -C same-name-downstream checkout master" &&
++ test_when_finished "git -C same-name-downstream checkout main" &&
+ (
+ cd same-name-1 &&
+ test_commit -C submodule --no-tag b1 &&
@@ t/t5526-fetch-submodules.sh: test_expect_success 'recursive fetch after deinit a
+ cd same-name-downstream &&
+ # even though the .gitmodules is correct, we cannot
+ # fetch from same-name-2
-+ git checkout same-name-2/master &&
++ git checkout same-name-2/main &&
+ git fetch --recurse-submodules same-name-1 &&
+ test_must_fail git fetch --recurse-submodules same-name-2
+ ) &&
@@ t/t5526-fetch-submodules.sh: test_expect_success 'recursive fetch after deinit a
+ ) &&
+ (
+ cd same-name-downstream &&
-+ git checkout master &&
++ git checkout main &&
+ git rm .gitmodules &&
+ git rm submodule &&
+ git commit -m "no submodules" &&
@@ t/t5526-fetch-submodules.sh: test_expect_success 'recursive fetch after deinit a
+ (
+ cd same-name-downstream/.git/modules/submodule &&
+ # The submodule has core.worktree pointing to the "git
-+ # rm"-ed directory, overwrite the invalid value.
++ # rm"-ed directory, overwrite the invalid value. See
++ # comment in get_fetch_task_from_changed() for more
++ # information.
+ git --work-tree=. cat-file -e $head1 &&
+ test_must_fail git --work-tree=. cat-file -e $head2
+ )
10: e1ac74eee4 = 10: 362ce3c7f8 submodule: fix latent check_has_commit() bugbase-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7
-- 2.33.GIT