Re: [PATCH v2] Fix status of initialized but not cloned submodules
- From
- Peter Kästle <peter.kaestle@nokia.com>
- Date
- Jan 24, 2020, 11:55 UTC
- 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:
Show 15 quoted lines
> 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
Show 37 quoted lines
>
> 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", "--",