From: Peter Kästle <peter.kaestle@nokia.com>
Date: Fri, 24 Jan 2020 11:55:25 GMT
Subject: Re: [PATCH v2] Fix status of initialized but not cloned submodules
Message-ID: <9192695a-0b3f-d166-7efa-84f57e091c2c@nokia.com>
In-Reply-To: <xmqq4kwl512y.fsf@gitster-ct.c.googlers.com>

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", "--",