| Message ID | 1579601532-10694-1-git-send-email-peter.kaestle@nokia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Fix status of initialized but not cloned submodules | expand |
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", "--",
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", "--",
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.
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.
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.
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[..]
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", "--",
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(-)