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

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

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 2/2] submodule.c: warn about missing submodule git directories

[Stefan Beller]

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 <sbeller@google.com>
---
 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

Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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.

After seeing this warning, as we do not do any remedial action in
this codepath, the user with a repository in this state will keep
seeing the 'missing' message.  Wouldn't we want to give a hint in
addition (e.g. 'you can run "git submodule update $name" to
recover', or something like that)?

The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already
forcing to correct the situation, so there is no need to touch that,
which makes sense, if I understand correctly.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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

Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories

[Stefan Beller]

On Tue, Aug 28, 2018 at 11:56 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > 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.
>
> After seeing this warning, as we do not do any remedial action in
> this codepath, the user with a repository in this state will keep
> seeing the 'missing' message.  Wouldn't we want to give a hint in
> addition (e.g. 'you can run "git submodule update $name" to
> recover', or something like that)?

Not quite, as this is only triggered in the case of 'old_head = NULL', which
is when you have a superproject that is missing the submodule in the
working tree before and wants to have it afterwards.

Looking at the test in the previous patch, I would think a reasonable workflow
in the test is

    git clone --recurse-submodules super1 super1_clone && cd super1_clone
    git checkout --recurse-submodules historic_state
    # see warning, but checkout proceeds

    git fetch --recurse-submodules
    # clones the submodule as it was configured active via the clone

    git checkout --recurse-submodules historic_state
    # this checkout will put the submodule in place
    #  not sure if -f is needed here, though.


I am currently working on the cloning of submodules that are not currently
in the working tree while fetching in the superproject, which would address
the latter part.

> The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already
> forcing to correct the situation, so there is no need to touch that,
> which makes sense, if I understand correctly.

No, that is not executed for now as it depends on 'old_head'.

In case of FORCE we might want to die instead of just warn about that submodule.

Stefan

Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Not quite, as this is ...
>
> Looking at the test in the previous patch, I would think a reasonable workflow
> in the test is ...
> 
>> The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already
>> forcing to correct the situation, so there is no need to touch that,
>> which makes sense, if I understand correctly.
>
> No, that is not executed for now as it depends on 'old_head'.

All explanation worth having in the log message to help future
readers, don't you think?

Thanks.

Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories

[Stefan Beller]

On Wed, Aug 29, 2018 at 10:17 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Stefan Beller <sbeller@google.com> writes:
>
> > Not quite, as this is ...
> >
> > Looking at the test in the previous patch, I would think a reasonable workflow
> > in the test is ...
> >
> >> The MOVE_HEAD_FORCE codepath that follows this hunk is, eh, already
> >> forcing to correct the situation, so there is no need to touch that,
> >> which makes sense, if I understand correctly.
> >
> > No, that is not executed for now as it depends on 'old_head'.
>
> All explanation worth having in the log message to help future
> readers, don't you think?

ok, will do.

Thanks,
Stefan

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

[SZEDER Gábor]

On Mon, Aug 27, 2018 at 03:12:56PM -0700, Stefan Beller wrote:
> 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 <sbeller@google.com>
> ---
>  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 &&

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
> 

Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories

[Jonathan Nieder]

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 <sbeller@google.com>
> ---
>  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

Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories

[Stefan Beller]

On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> 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 <sbeller@google.com>
> > ---
> >  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

Re: [PATCH 2/2] submodule.c: warn about missing submodule git directories

[Jonathan Nieder]

Hi,

Stefan Beller wrote:
> On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>> 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.
[...]
>> 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