git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] Fix submodule status of dirs initialized but empty
@ 2020-01-14 14:02 Peter Kästle
  2020-01-21 10:12 ` [PATCH v2] Fix status of initialized but not cloned submodules Peter Kaestle
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Kästle @ 2020-01-14 14:02 UTC (permalink / raw)
  To: git, Christian Couder, Stefan Beller, Prathamesh Chavan,
	Junio C Hamano

Fix submodule status of dirs initialized but empty

Original bash helper for "submodule status" was doing a check for
initialized but not cloned submodules and prefixed them with "-" in case 
.git
was missing inside the submodule directory.
This check was skipped when originally porting the functionality
from bash to C.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
---
  builtin/submodule--helper.c |   15 +++++++++++++--
  1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c72931e..50bb692 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -782,6 +782,9 @@ static void status_submodule(const char *path, const 
struct object_id *ce_oid,
  	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
  	struct rev_info rev;
  	int diff_files_result;
+	struct strbuf buf = STRBUF_INIT;
+	const char *git_dir;
+
   	if (!submodule_from_path(the_repository, &null_oid, path))
  		die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -793,11 +796,19 @@ static void status_submodule(const char *path, 
const struct object_id *ce_oid,
  		print_status(flags, 'U', path, &null_oid, displaypath);
  		goto cleanup;
  	}
-
-	if (!is_submodule_active(the_repository, path)) {
+	
+	strbuf_addf(&buf, "%s/.git", path);
+	git_dir = read_gitfile(buf.buf);
+	if (!git_dir)
+		git_dir = buf.buf;
+
+	if (!is_submodule_active(the_repository, path) ||
+			!is_git_directory(git_dir)) {
  		print_status(flags, '-', path, ce_oid, displaypath);
+		strbuf_release(&buf);
  		goto cleanup;
  	}
+	strbuf_release(&buf);
   	argv_array_pushl(&diff_files_args, "diff-files",
  			 "--ignore-submodules=dirty", "--quiet", "--",
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2] Fix status of initialized but not cloned submodules
  2020-01-14 14:02 [PATCH] Fix submodule status of dirs initialized but empty Peter Kästle
@ 2020-01-21 10:12 ` Peter Kaestle
  2020-01-23 21:12   ` Junio C Hamano
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Kaestle @ 2020-01-21 10:12 UTC (permalink / raw)
  To: git, christian.couder, sbeller, pc44800, gitster; +Cc: Peter Kaestle

Original bash helper for "submodule status" was doing a check for
initialized but not cloned submodules and prefixed the status with
a minus sign in case no .git file or folder was found inside the
submodule directory.
This check was missed when the original port of the functionality
from bash to C was done.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
---
 builtin/submodule--helper.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c72931e..c04241b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -782,6 +782,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
 	struct rev_info rev;
 	int diff_files_result;
+	struct strbuf buf = STRBUF_INIT;
+	const char *git_dir;
+
 
 	if (!submodule_from_path(the_repository, &null_oid, path))
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -794,10 +797,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 		goto cleanup;
 	}
 
-	if (!is_submodule_active(the_repository, path)) {
+	strbuf_addf(&buf, "%s/.git", path);
+	git_dir = read_gitfile(buf.buf);
+	if (!git_dir)
+		git_dir = buf.buf;
+
+	if (!is_submodule_active(the_repository, path) ||
+			!is_git_directory(git_dir)) {
 		print_status(flags, '-', path, ce_oid, displaypath);
+		strbuf_release(&buf);
 		goto cleanup;
 	}
+	strbuf_release(&buf);
 
 	argv_array_pushl(&diff_files_args, "diff-files",
 			 "--ignore-submodules=dirty", "--quiet", "--",
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] Fix status of initialized but not cloned submodules
  2020-01-21 10:12 ` [PATCH v2] Fix status of initialized but not cloned submodules Peter Kaestle
@ 2020-01-23 21:12   ` Junio C Hamano
  2020-01-24 10:34     ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Peter Kaestle
  2020-01-24 11:55     ` [PATCH v2] Fix status of initialized but not cloned submodules Peter Kästle
  0 siblings, 2 replies; 16+ messages in thread
From: Junio C Hamano @ 2020-01-23 21:12 UTC (permalink / raw)
  To: Peter Kaestle; +Cc: git, christian.couder, pc44800

Peter Kaestle <peter.kaestle@nokia.com> writes:

> Original bash helper for "submodule status" was doing a check for
> initialized but not cloned submodules and prefixed the status with
> a minus sign in case no .git file or folder was found inside the
> submodule directory.
> This check was missed when the original port of the functionality
> from bash to C was done.
>
> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
> ---
>  builtin/submodule--helper.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

I wonder if this is easily protectable with an additional test.

In general, do we have a good coverage of "git status" output that
involves submodules in various states?

Thanks.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c72931e..c04241b 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -782,6 +782,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
>  	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
>  	struct rev_info rev;
>  	int diff_files_result;
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *git_dir;
> +
>  
>  	if (!submodule_from_path(the_repository, &null_oid, path))
>  		die(_("no submodule mapping found in .gitmodules for path '%s'"),
> @@ -794,10 +797,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
>  		goto cleanup;
>  	}
>  
> -	if (!is_submodule_active(the_repository, path)) {
> +	strbuf_addf(&buf, "%s/.git", path);
> +	git_dir = read_gitfile(buf.buf);
> +	if (!git_dir)
> +		git_dir = buf.buf;
> +
> +	if (!is_submodule_active(the_repository, path) ||
> +			!is_git_directory(git_dir)) {
>  		print_status(flags, '-', path, ce_oid, displaypath);
> +		strbuf_release(&buf);
>  		goto cleanup;
>  	}
> +	strbuf_release(&buf);
>  
>  	argv_array_pushl(&diff_files_args, "diff-files",
>  			 "--ignore-submodules=dirty", "--quiet", "--",

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3 1/2] Testcase for submodule status on empty dirs
  2020-01-23 21:12   ` Junio C Hamano
@ 2020-01-24 10:34     ` Peter Kaestle
  2020-01-24 10:34       ` [PATCH v3 2/2] Fix status of initialized but not cloned submodules Peter Kaestle
  2020-01-24 17:35       ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Junio C Hamano
  2020-01-24 11:55     ` [PATCH v2] Fix status of initialized but not cloned submodules Peter Kästle
  1 sibling, 2 replies; 16+ messages in thread
From: Peter Kaestle @ 2020-01-24 10:34 UTC (permalink / raw)
  To: Junio C Hamano, git, christian.couder, pc44800; +Cc: Peter Kaestle

Test that submodule status reports initialized but not cloned
submodules as missing.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
---
 t/t7400-submodule-basic.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 7f75bb1..2e84914 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -399,6 +399,14 @@ test_expect_success 'init should register submodule url in .git/config' '
 	test_cmp expect url
 '
 
+test_expect_success 'status should still be "missing" after initializing' '
+	rm -fr init &&
+	mkdir init &&
+	git submodule status >lines &&
+	rm -fr init &&
+	grep "^-$rev1" lines
+'
+
 test_failure_with_unknown_submodule () {
 	test_must_fail git submodule $1 no-such-submodule 2>output.err &&
 	test_i18ngrep "^error: .*no-such-submodule" output.err
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v3 2/2] Fix status of initialized but not cloned submodules
  2020-01-24 10:34     ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Peter Kaestle
@ 2020-01-24 10:34       ` Peter Kaestle
  2020-01-24 17:35       ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Junio C Hamano
  1 sibling, 0 replies; 16+ messages in thread
From: Peter Kaestle @ 2020-01-24 10:34 UTC (permalink / raw)
  To: Junio C Hamano, git, christian.couder, pc44800; +Cc: Peter Kaestle

Original bash helper for "submodule status" was doing a check for
initialized but not cloned submodules and prefixed the status with
a minus sign in case no .git file or folder was found inside the
submodule directory.
This check was missed when the original port of the functionality
from bash to C was done.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
---
 builtin/submodule--helper.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c72931e..c04241b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -782,6 +782,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
 	struct rev_info rev;
 	int diff_files_result;
+	struct strbuf buf = STRBUF_INIT;
+	const char *git_dir;
+
 
 	if (!submodule_from_path(the_repository, &null_oid, path))
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -794,10 +797,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 		goto cleanup;
 	}
 
-	if (!is_submodule_active(the_repository, path)) {
+	strbuf_addf(&buf, "%s/.git", path);
+	git_dir = read_gitfile(buf.buf);
+	if (!git_dir)
+		git_dir = buf.buf;
+
+	if (!is_submodule_active(the_repository, path) ||
+			!is_git_directory(git_dir)) {
 		print_status(flags, '-', path, ce_oid, displaypath);
+		strbuf_release(&buf);
 		goto cleanup;
 	}
+	strbuf_release(&buf);
 
 	argv_array_pushl(&diff_files_args, "diff-files",
 			 "--ignore-submodules=dirty", "--quiet", "--",
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] Fix status of initialized but not cloned submodules
  2020-01-23 21:12   ` Junio C Hamano
  2020-01-24 10:34     ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Peter Kaestle
@ 2020-01-24 11:55     ` Peter Kästle
  2020-01-24 17:16       ` Junio C Hamano
  1 sibling, 1 reply; 16+ messages in thread
From: Peter Kästle @ 2020-01-24 11:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, christian.couder, pc44800

On 23.01.20 22:12, Junio C Hamano wrote:
> Peter Kaestle <peter.kaestle@nokia.com> writes:
> 
>> Original bash helper for "submodule status" was doing a check for
>> initialized but not cloned submodules and prefixed the status with
>> a minus sign in case no .git file or folder was found inside the
>> submodule directory.
>> This check was missed when the original port of the functionality
>> from bash to C was done.
>>
>> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
>> ---
>>   builtin/submodule--helper.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> I wonder if this is easily protectable with an additional test.

Yes, please find it inside patch series v3.


> In general, do we have a good coverage of "git status" output that
> involves submodules in various states?

Originally, without applying the v3 series, I can see those testcases:

1) not-init, not-cloned: 'status should initially be "missing"'
2) init, not-cloned: MISSING
3) not-init, cloned: MISSING
4) init, cloned: 'status should be "up-to-date" after update'
4.1) + modified: 'status should be "modified" after submodule commit'
4.2) + modified, committed: 'status should be "up-to-date" after update'

My patch v3 series adds a testcase for 2).
Is 3) even a possible use-case?

Besides that, I think testcases for man git-submodule's description of 
status on merge conflicts are missing (...[prefix] U if the submodule 
has merge conflicts).
At least, I didn't find any when searching the tests folder with:
$> grep 'grep "^U' *.sh

> 
> Thanks.
> 
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index c72931e..c04241b 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -782,6 +782,9 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
>>   	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
>>   	struct rev_info rev;
>>   	int diff_files_result;
>> +	struct strbuf buf = STRBUF_INIT;
>> +	const char *git_dir;
>> +
>>   
>>   	if (!submodule_from_path(the_repository, &null_oid, path))
>>   		die(_("no submodule mapping found in .gitmodules for path '%s'"),
>> @@ -794,10 +797,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
>>   		goto cleanup;
>>   	}
>>   
>> -	if (!is_submodule_active(the_repository, path)) {
>> +	strbuf_addf(&buf, "%s/.git", path);
>> +	git_dir = read_gitfile(buf.buf);
>> +	if (!git_dir)
>> +		git_dir = buf.buf;
>> +
>> +	if (!is_submodule_active(the_repository, path) ||
>> +			!is_git_directory(git_dir)) {
>>   		print_status(flags, '-', path, ce_oid, displaypath);
>> +		strbuf_release(&buf);
>>   		goto cleanup;
>>   	}
>> +	strbuf_release(&buf);
>>   
>>   	argv_array_pushl(&diff_files_args, "diff-files",
>>   			 "--ignore-submodules=dirty", "--quiet", "--",

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] Fix status of initialized but not cloned submodules
  2020-01-24 11:55     ` [PATCH v2] Fix status of initialized but not cloned submodules Peter Kästle
@ 2020-01-24 17:16       ` Junio C Hamano
  2020-01-27  8:52         ` Peter Kästle
  2020-02-02 23:16         ` Peter Kästle
  0 siblings, 2 replies; 16+ messages in thread
From: Junio C Hamano @ 2020-01-24 17:16 UTC (permalink / raw)
  To: Peter Kästle; +Cc: git, christian.couder, pc44800

Peter Kästle <peter.kaestle@nokia.com> writes:

> Originally, without applying the v3 series, I can see those testcases:
>
> 1) not-init, not-cloned: 'status should initially be "missing"'
> 2) init, not-cloned: MISSING
> 3) not-init, cloned: MISSING
> 4) init, cloned: 'status should be "up-to-date" after update'
> 4.1) + modified: 'status should be "modified" after submodule commit'
> 4.2) + modified, committed: 'status should be "up-to-date" after update'
>
> My patch v3 series adds a testcase for 2).

That's good.

> Is 3) even a possible use-case?

That would be somebody who is about to add a submodule to an
existing project, right?  You have a top-level project, you need
somebody else's project as its submodule or you need to use a
library in your top-level project and you develop the library
yourself, but want to keep that library part separate from its first
user which is your top-level project.  So in the working tree of a
top-level project you either "git init lib/" and start the submodule
project right there, or "git clone $URL_of_upstream_of_lib lib/",
and then plan to do "git submodule add lib", but haven't done so
yet.  .gitmodules does not know about it, but the directory is
already populated and is a valid repository.

> Besides that, I think testcases for man git-submodule's description of
> status on merge conflicts are missing (...[prefix] U if the submodule
> has merge conflicts).

Nice to know.  Thanks.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3 1/2] Testcase for submodule status on empty dirs
  2020-01-24 10:34     ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Peter Kaestle
  2020-01-24 10:34       ` [PATCH v3 2/2] Fix status of initialized but not cloned submodules Peter Kaestle
@ 2020-01-24 17:35       ` Junio C Hamano
  2020-01-27  9:06         ` [PATCH v4 1/2] t7400: add a testcase " Peter Kaestle
  1 sibling, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2020-01-24 17:35 UTC (permalink / raw)
  To: Peter Kaestle; +Cc: git, christian.couder, pc44800

Peter Kaestle <peter.kaestle@nokia.com> writes:

> Subject: Re: [PATCH v3 1/2] Testcase for submodule status on empty dirs
>
> Test that submodule status reports initialized but not cloned
> submodules as missing.

Let's not waste the findings you made and the description you wrote
during the review of the previous round.  After all, the reason why
reviewers ask questions and you write responses is not because we
want to satisify curiosity of the reviewers, but to polish the
history we leave to the future readers, both in the patch text and
its explanations.

How about explaining this step more like this?

    Subject: t7400: add a testcase for submodule status on empty dirs

    We have test coverage for "git submodule status" output in
    various cases, i.e.

        1) not-init, not-cloned: status should initially be "missing"
        2) init, not-cloned: status should be "missing"
        3) not-init, cloned:
        4) init, cloned:'status should be "up-to-date" after update
        4.1) + modified: status should be "modified" after submodule commit
        4.2) + modified, committed: status should be "up-to-date" after update

    but the cases 2) and 3) are not covered.

    Test that submodule status reports initialized but not cloned
    submodules as missing to fill the gap in test coverage.

Thanks.


>
> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
> ---
>  t/t7400-submodule-basic.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 7f75bb1..2e84914 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -399,6 +399,14 @@ test_expect_success 'init should register submodule url in .git/config' '
>  	test_cmp expect url
>  '
>  
> +test_expect_success 'status should still be "missing" after initializing' '
> +	rm -fr init &&
> +	mkdir init &&
> +	git submodule status >lines &&
> +	rm -fr init &&
> +	grep "^-$rev1" lines
> +'
> +
>  test_failure_with_unknown_submodule () {
>  	test_must_fail git submodule $1 no-such-submodule 2>output.err &&
>  	test_i18ngrep "^error: .*no-such-submodule" output.err

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] Fix status of initialized but not cloned submodules
  2020-01-24 17:16       ` Junio C Hamano
@ 2020-01-27  8:52         ` Peter Kästle
  2020-01-27 18:18           ` Junio C Hamano
  2020-02-02 23:16         ` Peter Kästle
  1 sibling, 1 reply; 16+ messages in thread
From: Peter Kästle @ 2020-01-27  8:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, christian.couder, pc44800

Hi,

On 24.01.20 18:16, Junio C Hamano wrote:
> Peter Kästle <peter.kaestle@nokia.com> writes:

[...]

>> Is 3) even a possible use-case?
> 
> That would be somebody who is about to add a submodule to an
> existing project, right?  You have a top-level project, you need
> somebody else's project as its submodule or you need to use a
> library in your top-level project and you develop the library
> yourself, but want to keep that library part separate from its first
> user which is your top-level project.  So in the working tree of a
> top-level project you either "git init lib/" and start the submodule
> project right there, or "git clone $URL_of_upstream_of_lib lib/",
> and then plan to do "git submodule add lib", but haven't done so
> yet.  .gitmodules does not know about it, but the directory is
> already populated and is a valid repository.

You are right, this is a possible scenario for 3).  However the question 
about how the outer git repo should behave is still open to me.  I mean, 
the outer git repo should treat the inner one as standard file structure 
and not as a submodule until the user explicitly calls the "git 
submodule add" command.  And thus "git submodule status" should 
completely skip it.  Calling "git status" in the outer repo should list 
the inner one as "untracked" or?
At least this is, what I would expect.
So from my point of view the only test for 3) we could do is something 
like this:
mkdir innerfoo &&
(cd innerfoo &&
git init) &&
test_must_fail git submodule status | grep "innerfoo"


>> Besides that, I think testcases for man git-submodule's description of
>> status on merge conflicts are missing (...[prefix] U if the submodule
>> has merge conflicts).
> 
> Nice to know.  Thanks.

Maybe I'll have spare time next weekend to create some.  Not to promise 
anything, but sounds like fun.

-- 
best regards,
--peter;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs
  2020-01-24 17:35       ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Junio C Hamano
@ 2020-01-27  9:06         ` Peter Kaestle
  2020-01-27  9:06           ` [PATCH v4 2/2] Fix status of initialized but not cloned submodules Peter Kaestle
  2020-01-27 18:15           ` [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs Junio C Hamano
  0 siblings, 2 replies; 16+ messages in thread
From: Peter Kaestle @ 2020-01-27  9:06 UTC (permalink / raw)
  To: Junio C Hamano, git, christian.couder, pc44800; +Cc: Peter Kaestle

We have test coverage for "git submodule status" output in
various cases, i.e.

  1) not-init, not-cloned: status should initially be "missing"
  2) init, not-cloned: status should be "missing"
  3) not-init, cloned:
  4) init, cloned:'status should be "up-to-date" after update
  4.1) + modified: status should be "modified" after submodule commit
  4.2) + modified, committed: status should be "up-to-date" after update

Case 2) will be covered by this patch.  Case 3) remains uncovered.

Test that submodule status reports initialized but not cloned
submodules as missing to fill the gap in test coverage.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
---
 t/t7400-submodule-basic.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 7f75bb1..2e84914 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -399,6 +399,14 @@ test_expect_success 'init should register submodule url in .git/config' '
 	test_cmp expect url
 '
 
+test_expect_success 'status should still be "missing" after initializing' '
+	rm -fr init &&
+	mkdir init &&
+	git submodule status >lines &&
+	rm -fr init &&
+	grep "^-$rev1" lines
+'
+
 test_failure_with_unknown_submodule () {
 	test_must_fail git submodule $1 no-such-submodule 2>output.err &&
 	test_i18ngrep "^error: .*no-such-submodule" output.err
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v4 2/2] Fix status of initialized but not cloned submodules
  2020-01-27  9:06         ` [PATCH v4 1/2] t7400: add a testcase " Peter Kaestle
@ 2020-01-27  9:06           ` Peter Kaestle
  2020-01-27 18:15           ` [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs Junio C Hamano
  1 sibling, 0 replies; 16+ messages in thread
From: Peter Kaestle @ 2020-01-27  9:06 UTC (permalink / raw)
  To: Junio C Hamano, git, christian.couder, pc44800; +Cc: Peter Kaestle

Original bash helper for "submodule status" was doing a check for
initialized but not cloned submodules and prefixed the status with
a minus sign in case no .git file or folder was found inside the
submodule directory.
This check was missed when the original port of the functionality
from bash to C was done.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
---
 builtin/submodule--helper.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c72931e..4031cf4 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -782,6 +782,8 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 	struct argv_array diff_files_args = ARGV_ARRAY_INIT;
 	struct rev_info rev;
 	int diff_files_result;
+	struct strbuf buf = STRBUF_INIT;
+	const char *git_dir;
 
 	if (!submodule_from_path(the_repository, &null_oid, path))
 		die(_("no submodule mapping found in .gitmodules for path '%s'"),
@@ -794,10 +796,18 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
 		goto cleanup;
 	}
 
-	if (!is_submodule_active(the_repository, path)) {
+	strbuf_addf(&buf, "%s/.git", path);
+	git_dir = read_gitfile(buf.buf);
+	if (!git_dir)
+		git_dir = buf.buf;
+
+	if (!is_submodule_active(the_repository, path) ||
+			!is_git_directory(git_dir)) {
 		print_status(flags, '-', path, ce_oid, displaypath);
+		strbuf_release(&buf);
 		goto cleanup;
 	}
+	strbuf_release(&buf);
 
 	argv_array_pushl(&diff_files_args, "diff-files",
 			 "--ignore-submodules=dirty", "--quiet", "--",
-- 
2.6.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs
  2020-01-27  9:06         ` [PATCH v4 1/2] t7400: add a testcase " Peter Kaestle
  2020-01-27  9:06           ` [PATCH v4 2/2] Fix status of initialized but not cloned submodules Peter Kaestle
@ 2020-01-27 18:15           ` Junio C Hamano
  2020-01-28  8:16             ` Peter Kästle
  2020-02-02 23:32             ` [PATCH] t7400: testcase for submodule status on unregistered inner git repos Peter Kaestle
  1 sibling, 2 replies; 16+ messages in thread
From: Junio C Hamano @ 2020-01-27 18:15 UTC (permalink / raw)
  To: Peter Kaestle; +Cc: git, christian.couder, pc44800

Peter Kaestle <peter.kaestle@nokia.com> writes:

> We have test coverage for "git submodule status" output in
> various cases, i.e.
>
>   1) not-init, not-cloned: status should initially be "missing"
>   2) init, not-cloned: status should be "missing"
>   3) not-init, cloned:
>   4) init, cloned:'status should be "up-to-date" after update
>   4.1) + modified: status should be "modified" after submodule commit
>   4.2) + modified, committed: status should be "up-to-date" after update
>
> Case 2) will be covered by this patch.  Case 3) remains uncovered.
>
> Test that submodule status reports initialized but not cloned
> submodules as missing to fill the gap in test coverage.

Thanks for an update.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] Fix status of initialized but not cloned submodules
  2020-01-27  8:52         ` Peter Kästle
@ 2020-01-27 18:18           ` Junio C Hamano
  0 siblings, 0 replies; 16+ messages in thread
From: Junio C Hamano @ 2020-01-27 18:18 UTC (permalink / raw)
  To: Peter Kästle; +Cc: git, christian.couder, pc44800

Peter Kästle <peter.kaestle@nokia.com> writes:

> ...  I mean, the outer git repo should treat the inner one as standard
> file structure and not as a submodule until the user explicitly calls
> the "git submodule add" command.  And thus "git submodule status"
> should completely skip it.  Calling "git status" in the outer repo
> should list the inner one as "untracked" or?

I agree that is a sensible expectation.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs
  2020-01-27 18:15           ` [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs Junio C Hamano
@ 2020-01-28  8:16             ` Peter Kästle
  2020-02-02 23:32             ` [PATCH] t7400: testcase for submodule status on unregistered inner git repos Peter Kaestle
  1 sibling, 0 replies; 16+ messages in thread
From: Peter Kästle @ 2020-01-28  8:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, christian.couder, pc44800

Hi Junio,

On 27.01.20 19:15, Junio C Hamano wrote:
> Peter Kaestle <peter.kaestle@nokia.com> writes:
> 
>> We have test coverage for "git submodule status" output in
>> various cases, i.e.
>>
>>    1) not-init, not-cloned: status should initially be "missing"
>>    2) init, not-cloned: status should be "missing"
>>    3) not-init, cloned:
>>    4) init, cloned:'status should be "up-to-date" after update
>>    4.1) + modified: status should be "modified" after submodule commit
>>    4.2) + modified, committed: status should be "up-to-date" after update
>>
>> Case 2) will be covered by this patch.  Case 3) remains uncovered.
>>
>> Test that submodule status reports initialized but not cloned
>> submodules as missing to fill the gap in test coverage.
> 
> Thanks for an update.
> 

Sure, welcome.  Will you include those patches, or do you need further 
refinement?
- Thanks a lot for your guidance and mentoring.  It was a pleasure to 
learn from you.

-- 
kind regards,
--peter;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: Re: [PATCH v2] Fix status of initialized but not cloned submodules
  2020-01-24 17:16       ` Junio C Hamano
  2020-01-27  8:52         ` Peter Kästle
@ 2020-02-02 23:16         ` Peter Kästle
  1 sibling, 0 replies; 16+ messages in thread
From: Peter Kästle @ 2020-02-02 23:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Peter Kästle, git, christian.couder, pc44800

On 24.01.20 18:16, Junio C Hamano wrote:
> Peter Kästle <peter.kaestle@nokia.com> writes:
>> Besides that, I think testcases for man git-submodule's description of
>> status on merge conflicts are missing (...[prefix] U if the submodule
>> has merge conflicts).
> 
> Nice to know.  Thanks.

I was wrong.  Testcases for "submodule status" to add prefix U in case 
of submodules with merge conflicts do exist in: t7405-submodule-merge.sh
The two tests are called:
git submodule status should display the merge conflict properly with[..]

-- 
--peter;

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] t7400: testcase for submodule status on unregistered inner git repos
  2020-01-27 18:15           ` [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs Junio C Hamano
  2020-01-28  8:16             ` Peter Kästle
@ 2020-02-02 23:32             ` Peter Kaestle
  1 sibling, 0 replies; 16+ messages in thread
From: Peter Kaestle @ 2020-02-02 23:32 UTC (permalink / raw)
  To: gitster; +Cc: christian.couder, git, pc44800, peter.kaestle, Peter Kaestle

We have test coverage for "git submodule status" output in
various cases, i.e.

  1) not-init, not-cloned: status should initially be "missing"
  2) init, not-cloned: status should be "missing"
  3) not-init, cloned: status should ignore the inner git-repo
  4) init, cloned: status should be "up-to-date" after update
  4.1) + modified: status should be "modified" after submodule commit
  4.2) + modified, committed: status should be "up-to-date" after update

the case 3) is not covered yet.

Test that submodule status reports an inner git repo as unknown, while
it is not added to the superproject.  This covers case (3).

Signed-off-by: Peter Kaestle <peter@piie.net>
---
 t/t7400-submodule-basic.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 29dc6812f1..d926b8c772 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -55,6 +55,21 @@ test_expect_success 'add aborts on repository with no commits' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'status should ignore inner git repo when not added' '
+	rm -fr inner &&
+	mkdir inner &&
+	(
+		cd inner &&
+		git init &&
+		>t &&
+		git add t &&
+		git commit -m "initial"
+	) &&
+	test_must_fail git submodule status inner 2>output.err &&
+	rm -fr inner &&
+	test_i18ngrep "^error: .*did not match any file(s) known to git" output.err
+'
+
 test_expect_success 'setup - repository in init subdirectory' '
 	mkdir init &&
 	(
-- 
2.25.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-02-02 23:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 14:02 [PATCH] Fix submodule status of dirs initialized but empty Peter Kästle
2020-01-21 10:12 ` [PATCH v2] Fix status of initialized but not cloned submodules Peter Kaestle
2020-01-23 21:12   ` Junio C Hamano
2020-01-24 10:34     ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Peter Kaestle
2020-01-24 10:34       ` [PATCH v3 2/2] Fix status of initialized but not cloned submodules Peter Kaestle
2020-01-24 17:35       ` [PATCH v3 1/2] Testcase for submodule status on empty dirs Junio C Hamano
2020-01-27  9:06         ` [PATCH v4 1/2] t7400: add a testcase " Peter Kaestle
2020-01-27  9:06           ` [PATCH v4 2/2] Fix status of initialized but not cloned submodules Peter Kaestle
2020-01-27 18:15           ` [PATCH v4 1/2] t7400: add a testcase for submodule status on empty dirs Junio C Hamano
2020-01-28  8:16             ` Peter Kästle
2020-02-02 23:32             ` [PATCH] t7400: testcase for submodule status on unregistered inner git repos Peter Kaestle
2020-01-24 11:55     ` [PATCH v2] Fix status of initialized but not cloned submodules Peter Kästle
2020-01-24 17:16       ` Junio C Hamano
2020-01-27  8:52         ` Peter Kästle
2020-01-27 18:18           ` Junio C Hamano
2020-02-02 23:16         ` Peter Kästle

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).