threads / patch / 49230

patch, 2 partst2013: add test for missing but active submodule

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

## tl;dr

10 messages between Aug 27, 2018 and Sep 7, 2018. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Stefan Beller· Aug 27, 2018, 22:12 UTC · lore
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(+)
Show changes to t/t2013-checkout-submodule.sh +24 −0
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
Stefan Beller· Aug 27, 2018, 22:12 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 2 files +17 −1

submodule.c, t/t2013-checkout-submodule.sh

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
Junio C Hamano· Aug 28, 2018, 18:56 UTC · re: Stefan Beller · lore

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

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.

Show 46 quoted lines
> 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
Stefan Beller· Aug 28, 2018, 21:49 UTC · re: Junio C Hamano · lore

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

On Tue, Aug 28, 2018 at 11:56 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> 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
Junio C Hamano· Aug 29, 2018, 17:16 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
> 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.
Stefan Beller· Aug 29, 2018, 20:32 UTC · re: Junio C Hamano · lore

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

On Wed, Aug 29, 2018 at 10:17 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
>
> 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

Jonathan Nieder· Sep 5, 2018, 19:18 UTC · re: Stefan Beller · lore

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

Hi,
Stefan Beller wrote:
Show 10 quoted lines
> 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

Stefan Beller· Sep 7, 2018, 18:49 UTC · re: Jonathan Nieder · lore

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

On Wed, Sep 5, 2018 at 12:18 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 20 quoted lines
>
> 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
Jonathan Nieder· Sep 7, 2018, 19:53 UTC · re: Stefan Beller · lore

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

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.
[...]
Show 11 quoted lines
>> 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

SZEDER Gábor· Aug 29, 2018, 21:04 UTC · re: Stefan Beller · lore

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

On Mon, Aug 27, 2018 at 03:12:56PM -0700, Stefan Beller wrote:
Show 28 quoted lines
> 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 ...

Show 6 quoted lines
> +	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.

Show 14 quoted lines
> +'
> +
> +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
>

← back to recent threads