patchFix submodule status of dirs initialized but empty

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(-)

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(-)

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

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.

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(+)

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(-)

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

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.

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(+)

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(-)

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

Thanks for an update.

Hi Junio,

On 27.01.20 19:15, Junio C Hamano wrote:

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.

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(+)

On 23.01.20 22:12, Junio C Hamano wrote:

Yes, please find it inside patch series v3.

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

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

That's good.

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.

Nice to know.  Thanks.

Hi,

On 24.01.20 18:16, Junio C Hamano wrote:

[...]

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"

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

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

I agree that is a sensible expectation.

On 24.01.20 18:16, Junio C Hamano wrote:

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