[PATCH 1/2] t2013: add test for missing but active submodule

When cloning a superproject with the option
 --recurse-submodules='.', it is easy to find yourself wanting
a submodule active, but not having that submodule present in
the modules directory.

Signed-off-by: Stefan Beller <redacted>
---
 t/t2013-checkout-submodule.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 6ef15738e44..c69640fc341 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh

@@ -63,6 +63,30 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
 	! test -s actual
 '
 
+test_expect_success 'setup superproject with historic submodule' '
+	test_create_repo super1 &&
+	test_create_repo sub1 &&
+	test_commit -C sub1 sub_content &&
+	git -C super1 submodule add ../sub1 &&
+	git -C super1 commit -a -m "sub1 added" &&
+	test_commit -C super1 historic_state &&
+	git -C super1 rm sub1 &&
+	git -C super1 commit -a -m "deleted sub" &&
+	test_commit -C super1 new_state &&
+	test_path_is_missing super1/sub &&
+
+	# The important part is to ensure sub1 is not in there any more.
+	# There is another series in flight, that may remove an
+	# empty .gitmodules file entirely.
+	test_must_be_empty super1/.gitmodules
+'
+
+test_expect_failure 'checkout old state with deleted submodule' '
+	test_when_finished "rm -rf super1 sub1 super1_clone" &&
+	git clone --recurse-submodules super1 super1_clone &&
+	git -C super1_clone checkout --recurse-submodules historic_state
+'
+
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 test_submodule_switch_recursing_with_args "checkout"
 

-- 
2.18.0

This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head
works with broken submodules, 2017-04-18), which tones down the case of
"broken submodule" in case of a missing git directory of the submodule to
be only a warning.

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c                   | 16 ++++++++++++++++
 t/t2013-checkout-submodule.sh |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 50cbf5f13ed..689439a3d0c 100644
--- a/submodule.c
+++ b/submodule.c

@@ -1641,6 +1641,22 @@ int submodule_move_head(const char *path,
 		} else {
 			char *gitdir = xstrfmt("%s/modules/%s",
 				    get_git_common_dir(), sub->name);
+
+			if (!is_git_directory(gitdir)) {
+				/*
+				 * It is safe to assume we could just clone
+				 * the submodule here, as we passed the
+				 * is_submodule_active test above (i.e. the
+				 * user is interested in this submodule.
+				 *
+				 * However as this code path is exercised
+				 * for operations that typically do not involve
+				 * network operations, let's not do that for now.
+				 */
+				warning(_("Submodule '%s' missing"), path);
+				free(gitdir);
+				return 0;
+			}
 			connect_work_tree_and_git_dir(path, gitdir, 0);
 			free(gitdir);
 

diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index c69640fc341..82ef4576b91 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh

@@ -81,7 +81,7 @@ test_expect_success 'setup superproject with historic submodule' '
 	test_must_be_empty super1/.gitmodules
 '
 
-test_expect_failure 'checkout old state with deleted submodule' '
+test_expect_success 'checkout old state with deleted submodule' '
 	test_when_finished "rm -rf super1 sub1 super1_clone" &&
 	git clone --recurse-submodules super1 super1_clone &&
 	git -C super1_clone checkout --recurse-submodules historic_state

-- 
2.18.0

On Mon, Aug 27, 2018 at 03:12:56PM -0700, Stefan Beller wrote:

These six consecutive commands above all specify the '-C super1'
options ...

+	test_path_is_missing super1/sub &&
+
+	# The important part is to ensure sub1 is not in there any more.
+	# There is another series in flight, that may remove an
+	# empty .gitmodules file entirely.
+	test_must_be_empty super1/.gitmodules

... and both of these two checks use the 'super1/' path prefix.  I
think it would be more readable to simply 'cd super1' first.

+'
+
+test_expect_failure 'checkout old state with deleted submodule' '
+	test_when_finished "rm -rf super1 sub1 super1_clone" &&
+	git clone --recurse-submodules super1 super1_clone &&
+	git -C super1_clone checkout --recurse-submodules historic_state
+'
+
 KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
 test_submodule_switch_recursing_with_args "checkout"
 
-- 
2.18.0

Hi,

Stefan Beller wrote:

This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head
works with broken submodules, 2017-04-18), which tones down the case of
"broken submodule" in case of a missing git directory of the submodule to
be only a warning.

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c                   | 16 ++++++++++++++++
 t/t2013-checkout-submodule.sh |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

I don't understand what workflow this is a part of.

If the submodule is missing, shouldn't we make it non-missing instead
of producing a partial checkout that doesn't build?

Thanks,
Jonathan

On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder [off-list ref] wrote:

Hi,

Stefan Beller wrote:

quoted

This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head
works with broken submodules, 2017-04-18), which tones down the case of
"broken submodule" in case of a missing git directory of the submodule to
be only a warning.

Signed-off-by: Stefan Beller <redacted>
---
 submodule.c                   | 16 ++++++++++++++++
 t/t2013-checkout-submodule.sh |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

I don't understand what workflow this is a part of.

If the submodule is missing, shouldn't we make it non-missing instead
of producing a partial checkout that doesn't build?

No. checkout and friends do not want to touch the network
(unless we are in a partial clone world; that is the user is fully
aware that commands can use the network at totally unexpected
times)

So for that, all we can do is better error messages.

Stefan

Hi,

Stefan Beller wrote:

On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder [off-list ref] wrote:

quoted

Stefan Beller wrote:

quoted

quoted

This is the continuation of f2d48994dc1 (submodule.c: submodule_move_head
works with broken submodules, 2017-04-18), which tones down the case of
"broken submodule" in case of a missing git directory of the submodule to
be only a warning.

[...]

quoted

I don't understand what workflow this is a part of.

If the submodule is missing, shouldn't we make it non-missing instead
of producing a partial checkout that doesn't build?

No. checkout and friends do not want to touch the network
(unless we are in a partial clone world; that is the user is fully
aware that commands can use the network at totally unexpected
times)

So for that, all we can do is better error messages.

Thanks.  This patch doesn't just improve error messages, though, but
it makes the operation report success instead of failing.

Isn't that likely to produce more confusion when I run additional
commands afterward?  In other words, instead of

	$ git checkout --recurse-submodules -B master origin/new-fancy-branch
	Branch 'master' set up to track remote branch 'new-fancy-branch' from 'origin'.
	Switched to a new branch 'master'
	warning: Submodule 'new-fancy-submodule' is missing
	$ git status
[some unclean state]

I would prefer to experience

	$ git checkout --recurse-submodules -B master origin/new-fancy-branch
	fatal: missing submodule 'new-fancy-submodule'
	hint: run "git fetch --recurse-submodules" to fetch it
	$ git status
[clean state]
	$ git fetch --recurse-submodules
[...]
	$ git checkout --recurse-submodules -B master origin/new-fancy-branch
	Branch 'master' set up to track remote branch 'new-fancy-branch' from 'origin'.
	Switched to a new branch 'master'
	$ git status
[clean state]

Thanks,
Jonathan