BUG in fetching non-checked out submodule

BUG in fetching non-checked out submodule

[Ralf Thielow]

Hi,

I have the current 'master' branch of git installed and get
the following error when fetching a submodule that is not
checked out.

I've bisected this error down to commit
1b7ac4e6d4 (submodules: fix of regression on fetching of
non-init subsub-repo, 2020-11-12)

$ git version
git version 2.29.2.435.g72ffeb997e

$ git config --get submodule.recurse
true

$ git submodule status
-855827c583bc30645ba427885caa40c5b81764d2 sha1collisiondetection

$ git fetch
Fetching submodule sha1collisiondetection
Fetching submodule sha1collisiondetection/sha1collisiondetection
Fetching submodule
sha1collisiondetection/sha1collisiondetection/sha1collisiondetection
Fetching submodule
sha1collisiondetection/sha1collisiondetection/sha1collisiondetection/sha1collisiondetection
...

$ git submodule update --checkout
Submodule path 'sha1collisiondetection': checked out
'855827c583bc30645ba427885caa40c5b81764d2'

$ git fetch
Fetching submodule sha1collisiondetection
$

Ralf

Re: BUG in fetching non-checked out submodule

[Philippe Blain]

Hi Ralf,

> Le 2 déc. 2020 à 10:56, Ralf Thielow <ralf.thielow@gmail.com> a écrit :
> 
> Hi,
> 
> I have the current 'master' branch of git installed and get
> the following error when fetching a submodule that is not
> checked out.
> 
> I've bisected this error down to commit
> 1b7ac4e6d4 (submodules: fix of regression on fetching of
> non-init subsub-repo, 2020-11-12)

Thanks for bisecting it. That commit wanted to fix a different bug
related to nested submodules, and the route taken was simply
reverting an earlier commit (a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28).

As you discovered, it breaks other scenarios.

> 
> $ git version
> git version 2.29.2.435.g72ffeb997e
> 
> $ git config --get submodule.recurse
> true

Yeah, I think the test suite could make more efforts
to run more tests with that setting turned 'on', but
it would require significants efforts since it changes 
the behaviour of several commands.

Meta question: is there an easy way to run the whole test
suite with specific config options turned on ?

> 
> $ git submodule status
> -855827c583bc30645ba427885caa40c5b81764d2 sha1collisiondetection
> 
> $ git fetch
> Fetching submodule sha1collisiondetection
> Fetching submodule sha1collisiondetection/sha1collisiondetection
> Fetching submodule
> sha1collisiondetection/sha1collisiondetection/sha1collisiondetection
> Fetching submodule
> sha1collisiondetection/sha1collisiondetection/sha1collisiondetection/sha1collisiondetection
> ...
> 
> $ git submodule update --checkout
> Submodule path 'sha1collisiondetection': checked out
> '855827c583bc30645ba427885caa40c5b81764d2'

Ok, you don't add '--init' but the submodule gets checked out,
so it looks like you have 'submodule.active' set to a pathspec 
that matches 'sha1collisiondetection'. Did you clone the git repo
with '--recurse-submodules', which would add '.' as the value of
'submodule.active' ? Or maybe you manually configured that value
in your global gitconfig ?

Thanks for the report,

Philippe.

Re: BUG in fetching non-checked out submodule

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Thanks for bisecting it. That commit wanted to fix a different bug
> related to nested submodules, and the route taken was simply
> reverting an earlier commit (a62387b (submodule.c: fetch in
> submodules git directory instead of in worktree, 2018-11-28).
>
> As you discovered, it breaks other scenarios.
>
>> 
>> $ git version
>> git version 2.29.2.435.g72ffeb997e
>> 
>> $ git config --get submodule.recurse
>> true

I think the current situation is probably worse.  

As a short-term fix, we should revert 1b7ac4e6d4 until we can come
up with a real fix, probably.

> Yeah, I think the test suite could make more efforts
> to run more tests with that setting turned 'on', but
> it would require significants efforts since it changes 
> the behaviour of several commands.

I am not sure if the question is about amount of efforts.

A configuration variable is there to change the behaviour of
commands, so a test of a command that has been running happily and
producing a set of expected outcome with a configuration unset
should break the expectation when the configuration is set ---
otherwise there is no point in having a configuration variable.

> Meta question: is there an easy way to run the whole test
> suite with specific config options turned on ?

Hence, I do not think it even makes sense to have such an "easy
way".  If the "fetch" command, for example, is expected to change
behaviour depending on the value of submodule.recurse, a test
written for the case where the variable is not set should produce
different outcome when the variable is set.

What we need may be a better test coverage.  submodule.recurse is a
later addition, and all tests written earlier do test how the commands
behave without the configuration being set.  If one wants to change
the behaviour of these commands when the configuration is set, new
tests to specify what the expected behaviour need to be added.

> Thanks for the report,

Yup, thanks for helping out.

Re: BUG in fetching non-checked out submodule

[Peter Kästle]

Hi,

On 03.12.20 00:06, Junio C Hamano wrote:
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
> 
>> Thanks for bisecting it. That commit wanted to fix a different bug
>> related to nested submodules, and the route taken was simply
>> reverting an earlier commit (a62387b (submodule.c: fetch in
>> submodules git directory instead of in worktree, 2018-11-28).
>>
>> As you discovered, it breaks other scenarios.
>>
>>>
>>> $ git version
>>> git version 2.29.2.435.g72ffeb997e
>>>
>>> $ git config --get submodule.recurse
>>> true
> 
> I think the current situation is probably worse.
> 
> As a short-term fix, we should revert 1b7ac4e6d4 until we can come
> up with a real fix, probably.

Junio: This is why I originally intended to commit the test case for the 
testsuite separated from the revert and wanted to start a discussion 
about the actual real fix for the issue:
https://public-inbox.org/git/1604413399-63090-1-git-send-email-peter.kaestle@nokia.com/

My proposal would be to revert 1b7ac4e6d4 and isolate the test case 
"test_expect_success 'setup nested submodule fetch test' '" make it 
"test_expect_failure" and apply it instead, until we come up with a real 
solution.

>> Yeah, I think the test suite could make more efforts
>> to run more tests with that setting turned 'on', but
>> it would require significants efforts since it changes
>> the behaviour of several commands.
> 
> I am not sure if the question is about amount of efforts.
> 
> A configuration variable is there to change the behaviour of
> commands, so a test of a command that has been running happily and
> producing a set of expected outcome with a configuration unset
> should break the expectation when the configuration is set ---
> otherwise there is no point in having a configuration variable.
> 
>> Meta question: is there an easy way to run the whole test
>> suite with specific config options turned on ?
> 
> Hence, I do not think it even makes sense to have such an "easy
> way".  If the "fetch" command, for example, is expected to change
> behaviour depending on the value of submodule.recurse, a test
> written for the case where the variable is not set should produce
> different outcome when the variable is set.
> 
> What we need may be a better test coverage.  submodule.recurse is a
> later addition, and all tests written earlier do test how the commands
> behave without the configuration being set.  If one wants to change
> the behaviour of these commands when the configuration is set, new
> tests to specify what the expected behaviour need to be added.

Can we start a new test suite for tests on which this variable is set? 
The case of Ralf can be used as first test case.

Ralf: could you please send a command sequence, which recreates the 
required repository setup from scratch and is able to trigger your 
observation?  Then we can convert this into a test case.

Thanks.

-- 
best regards
--peter;

Re: BUG in fetching non-checked out submodule

[Philippe Blain]

Hello Peter,

Le jeu. 3 déc. 2020, à 02 h 54, Peter Kästle <peter.kaestle@nokia.com> a écrit :
>
> Hi,
>
> On 03.12.20 00:06, Junio C Hamano wrote:
> > Philippe Blain <levraiphilippeblain@gmail.com> writes:
> >
> >> Thanks for bisecting it. That commit wanted to fix a different bug
> >> related to nested submodules, and the route taken was simply
> >> reverting an earlier commit (a62387b (submodule.c: fetch in
> >> submodules git directory instead of in worktree, 2018-11-28).
> >>
> >> As you discovered, it breaks other scenarios.
> >>
> >>>
> >>> $ git version
> >>> git version 2.29.2.435.g72ffeb997e
> >>>
> >>> $ git config --get submodule.recurse
> >>> true
> >
> > I think the current situation is probably worse.
> >
> > As a short-term fix, we should revert 1b7ac4e6d4 until we can come
> > up with a real fix, probably.
>
> Junio: This is why I originally intended to commit the test case for the
> testsuite separated from the revert and wanted to start a discussion
> about the actual real fix for the issue:
> https://public-inbox.org/git/1604413399-63090-1-git-send-email-peter.kaestle@nokia.com/
>
> My proposal would be to revert 1b7ac4e6d4 and isolate the test case
> "test_expect_success 'setup nested submodule fetch test' '" make it
> "test_expect_failure" and apply it instead, until we come up with a real
> solution.


I think I have the real solution. I did some debugging and I think it
is quite easy:
In 'get_next_submodule', 'get_submodule_repo_for(spf->r, task->sub)'
fails to get a repo pointer for the submodule repository, since it is
not initialized. That
is normal. Then we go in the "else" branch, and hit this code:

/*
* An empty directory is normal,
* the submodule is not initialized
*/
if (S_ISGITLINK(ce->ce_mode) &&
!is_empty_dir(ce->name)) {

'is_empty_dir' receives ce->name, but the current working directory is the
Git directory of 'middle', so clearly is_empty_dir returns false, as

/path/to/git/t/trash
directory.t5526-fetch-submodules/B/.git/modules/middle/inner

is a non-existent path. The path that we should send is the worktree
of inner, ie.
the concatenation of spf->r->worktree and ce->name. This would give

/path/to/git/t/trash directory.t5526-fetch-submodules/B/middle/inner,

which is an empty directory since the inner submodule is not initialized,
and we would not get the "Could not access submodule inner" error
that you wanted to solve.


Cheers,

Philippe.

Re: BUG in fetching non-checked out submodule

[Peter Kästle]

Hi Philippe,

On 03.12.20 16:25, Philippe Blain wrote:
>> On 03.12.20 00:06, Junio C Hamano wrote:
>>> Philippe Blain <levraiphilippeblain@gmail.com> writes:
>>>
>>>> Thanks for bisecting it. That commit wanted to fix a different bug
>>>> related to nested submodules, and the route taken was simply
>>>> reverting an earlier commit (a62387b (submodule.c: fetch in
>>>> submodules git directory instead of in worktree, 2018-11-28).
>>>>
>>>> As you discovered, it breaks other scenarios.
>>>>
>>>>>
>>>>> $ git version
>>>>> git version 2.29.2.435.g72ffeb997e
>>>>>
>>>>> $ git config --get submodule.recurse
>>>>> true
>>>
>>> I think the current situation is probably worse.
>>>
>>> As a short-term fix, we should revert 1b7ac4e6d4 until we can come
>>> up with a real fix, probably.
>>
>> Junio: This is why I originally intended to commit the test case for the
>> testsuite separated from the revert and wanted to start a discussion
>> about the actual real fix for the issue:
>> https://public-inbox.org/git/1604413399-63090-1-git-send-email-peter.kaestle@nokia.com/
>>
>> My proposal would be to revert 1b7ac4e6d4 and isolate the test case
>> "test_expect_success 'setup nested submodule fetch test' '" make it
>> "test_expect_failure" and apply it instead, until we come up with a real
>> solution.
> 
> 
> I think I have the real solution. I did some debugging and I think it
> is quite easy:
> In 'get_next_submodule', 'get_submodule_repo_for(spf->r, task->sub)'
> fails to get a repo pointer for the submodule repository, since it is
> not initialized. That
> is normal. Then we go in the "else" branch, and hit this code:
> 
> /*
> * An empty directory is normal,
> * the submodule is not initialized
> */
> if (S_ISGITLINK(ce->ce_mode) &&
> !is_empty_dir(ce->name)) {
> 
> 'is_empty_dir' receives ce->name, but the current working directory is the
> Git directory of 'middle', so clearly is_empty_dir returns false, as
> 
> /path/to/git/t/trash
> directory.t5526-fetch-submodules/B/.git/modules/middle/inner
> 
> is a non-existent path. The path that we should send is the worktree
> of inner, ie.
> the concatenation of spf->r->worktree and ce->name. This would give
> 
> /path/to/git/t/trash directory.t5526-fetch-submodules/B/middle/inner,
> 
> which is an empty directory since the inner submodule is not initialized,
> and we would not get the "Could not access submodule inner" error
> that you wanted to solve.


On quick glance this sounds plausible, but to fully understand it I need 
to put some effort in reading this code again.  I hope to do so 
tomorrow.  We can then compile a new set of patches including this real 
fix and Ralf's and my test case.

Thanks for digging into it.

-- 
kind regards
--peter;

Re: BUG in fetching non-checked out submodule

[Junio C Hamano]

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

> On quick glance this sounds plausible, but to fully understand it I
> need to put some effort in reading this code again.  I hope to do so 
> tomorrow.  We can then compile a new set of patches including this
> real fix and Ralf's and my test case.
>
> Thanks for digging into it.

Yeah, thanks, both for noticing problem so quickly and started
digging to find the real solution.

By the way, as to the "if this were originally two patches, we could
have saved the test that expects failure" you raised, I do not think
it is a good idea to optimize for cases where changes turn out
problematic and have to get reverted.

[PATCH] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kaestle]

A regression has been introduced by a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28).

The scenario in which it triggers is when one has a remote repository
with a subrepository inside a subrepository like this:
superproject/middle_repo/inner_repo

Person A and B have both a clone of it, while Person B is not working
with the inner_repo and thus does not have it initialized in his working
copy.

Now person A introduces a change to the inner_repo and propagates it
through the middle_repo and the superproject.

Once person A pushed the changes and person B wants to fetch them using
"git fetch" on superproject level, B's git call will return with error
saying:

Could not access submodule 'inner_repo'
Errors during submodule fetch:
         middle_repo

Expectation is that in this case the inner submodule will be recognized
as uninitialized subrepository and skipped by the git fetch command.

This used to work correctly before 'a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28)'.

Starting with a62387b the code wants to evaluate "is_empty_dir()" inside
.git/modules for a directory only existing in the worktree, delivering
then of course wrong return value.

This patch ensures is_empty_dir() is getting the correct path of the
uninitialized submodule by concatenation of the actual worktree and the
name of the uninitialized submodule.

Furthermore a regression test case is added, which tests for recursive
fetches on a superproject with uninitialized sub repositories.  This
issue was leading to an infinite loop when doing a revert of a62387b.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
CC: Junio C Hamano <gitster@pobox.com>
CC: Philippe Blain <levraiphilippeblain@gmail.com>
CC: Ralf Thielow <ralf.thielow@gmail.com>
---
 submodule.c                 |  7 ++-
 t/t5526-fetch-submodules.sh | 94 +++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index b3bb59f066..b561445329 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,
 			strbuf_release(&submodule_prefix);
 			return 1;
 		} else {
+			struct strbuf empty_submodule_path = STRBUF_INIT;
 
 			fetch_task_release(task);
 			free(task);
@@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,
 			 * An empty directory is normal,
 			 * the submodule is not initialized
 			 */
+			strbuf_addf(&empty_submodule_path, "%s/%s/",
+							spf->r->worktree,
+							ce->name);
 			if (S_ISGITLINK(ce->ce_mode) &&
-			    !is_empty_dir(ce->name)) {
+			    !is_empty_dir(empty_submodule_path.buf)) {
 				spf->result = 1;
 				strbuf_addf(err,
 					    _("Could not access submodule '%s'\n"),
 					    ce->name);
 			}
+			strbuf_release(&empty_submodule_path);
 		}
 	}
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index dd8e423d25..04eb587862 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -719,4 +719,98 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
 	)
 '
 
+add_commit_push () {
+	dir="$1"
+	msg="$2"
+	shift 2
+	git -C "$dir" add "$@" &&
+	git -C "$dir" commit -a -m "$msg" &&
+	git -C "$dir" push
+}
+
+compare_refs_in_dir () {
+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail='!' &&
+		shift
+	fi &&
+	git -C "$1" rev-parse --verify "$2" >expect &&
+	git -C "$3" rev-parse --verify "$4" >actual &&
+	eval $fail test_cmp expect actual
+}
+
+
+test_expect_success 'setup nested submodule fetch test' '
+	# does not depend on any previous test setups
+
+	for repo in outer middle inner
+	do
+		(
+			git init --bare $repo &&
+			git clone $repo ${repo}_content &&
+			echo "$repo" >"${repo}_content/file" &&
+			add_commit_push ${repo}_content "initial" file
+		) || return 1
+	done &&
+
+	git clone outer A &&
+	git -C A submodule add "$pwd/middle" &&
+	git -C A/middle/ submodule add "$pwd/inner" &&
+	add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
+	add_commit_push A/ "adding middle sub" .gitmodules middle &&
+
+	git clone outer B &&
+	git -C B/ submodule update --init middle &&
+
+	compare_refs_in_dir A HEAD B HEAD &&
+	compare_refs_in_dir A/middle HEAD B/middle HEAD &&
+	test -f B/file &&
+	test -f B/middle/file &&
+	! test -f B/middle/inner/file &&
+
+	echo "change on inner repo of A" >"A/middle/inner/file" &&
+	add_commit_push A/middle/inner "change on inner" file &&
+	add_commit_push A/middle "change on inner" inner &&
+	add_commit_push A "change on inner" middle
+'
+
+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
+	# depends on previous test for setup
+
+	git -C B/ fetch &&
+	compare_refs_in_dir A origin/master B origin/master
+'
+
+
+test_expect_success 'setup recursive fetch with uninit submodule' '
+	# does not depend on any previous test setups
+
+	git init main &&
+	git init sub &&
+
+	touch sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+	git -C sub rev-parse HEAD >expect &&
+
+	git -C main submodule add ../sub &&
+	git -C main submodule init &&
+	git -C main submodule update --checkout &&
+	git -C main submodule status |
+		sed -e "s/^ //" -e "s/ sub .*$//" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'recursive fetch with uninit submodule' '
+	# depends on previous test for setup
+
+	git -C main submodule deinit -f sub &&
+	! git -C main fetch --recurse-submodules |&
+		grep -v -m1 "Fetching submodule sub$" &&
+	git -C main submodule status |
+		sed -e "s/^-//" -e "s/ sub$//" >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.29.2

Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo

[Eric Sunshine]

On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle <peter.kaestle@nokia.com> wrote:
> [...]
> Furthermore a regression test case is added, which tests for recursive
> fetches on a superproject with uninitialized sub repositories.  This
> issue was leading to an infinite loop when doing a revert of a62387b.

Just a few small comments (nothing comprehensive) from a quick scan of
the patch...

Mostly they are just minor style issues, not necessarily worth a
re-roll, but there is one actionable item.

> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
> ---
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> @@ -719,4 +719,98 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
> +add_commit_push () {
> +       dir="$1"
> +       msg="$2"
> +       shift 2

We typically recommend including these assignments in the &&-chain to
future-proof against someone later inserting code above them and not
realizing that that code is not part of the &&-chain, in which case if
the new code fails, the failure might go unnoticed.

> +       git -C "$dir" add "$@" &&
> +       git -C "$dir" commit -a -m "$msg" &&
> +       git -C "$dir" push
> +}
> +
> +compare_refs_in_dir () {
> +       fail= &&
> +       if test "x$1" = 'x!'
> +       then
> +               fail='!' &&
> +               shift
> +       fi &&
> +       git -C "$1" rev-parse --verify "$2" >expect &&
> +       git -C "$3" rev-parse --verify "$4" >actual &&
> +       eval $fail test_cmp expect actual
> +}

We have a test_cmp_rev() similar to this but it doesn't support -C as
some of our other test functions do. I briefly wondered if it would
make sense to extend it to understand -C, but even that wouldn't help
this case since compare_refs_in_dir() introduced here involves two
distinct directories. The need here is so special-purpose that it
likely would not make sense to upgrade test_cmp_rev() to accommodate
it. Okay.

> +test_expect_success 'setup nested submodule fetch test' '
> +       # does not depend on any previous test setups
> +
> +       for repo in outer middle inner
> +       do
> +               (
> +                       git init --bare $repo &&
> +                       git clone $repo ${repo}_content &&
> +                       echo "$repo" >"${repo}_content/file" &&
> +                       add_commit_push ${repo}_content "initial" file
> +               ) || return 1
> +       done &&

What is the purpose of the subshell here? Is it to ensure that commits
in each repo have identical timestamps? Or is it just for making the
&& and || expression more clear? If the latter, we normally don't
bother with the parentheses.

> +       git clone outer A &&
> +       git -C A submodule add "$pwd/middle" &&
> +       git -C A/middle/ submodule add "$pwd/inner" &&
> +       add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
> +       add_commit_push A/ "adding middle sub" .gitmodules middle &&
> +
> +       git clone outer B &&
> +       git -C B/ submodule update --init middle &&
> +
> +       compare_refs_in_dir A HEAD B HEAD &&
> +       compare_refs_in_dir A/middle HEAD B/middle HEAD &&
> +       test -f B/file &&
> +       test -f B/middle/file &&
> +       ! test -f B/middle/inner/file &&

These days we typically use test_path_exists() (or
test_path_is_file()) and test_path_is_missing() rather than bare
`test`.

> +test_expect_success 'setup recursive fetch with uninit submodule' '
> +       # does not depend on any previous test setups
> +
> +       git init main &&
> +       git init sub &&
> +
> +       touch sub/file &&

Unless the timestamp of the file is significant to the test, in which
case `touch` is used, we normally create empty files like this:

    >sub/file &&

> +test_expect_success 'recursive fetch with uninit submodule' '
> +       git -C main submodule deinit -f sub &&
> +       ! git -C main fetch --recurse-submodules |&
> +               grep -v -m1 "Fetching submodule sub$" &&

We want the test scripts to be portable, thus avoid Bashisms such as `|&`.

We also avoid placing a Git command upstream in a pipe since doing so
causes the exit code of the Git command to be lost. Instead, we would
normally send the Git output to a file and then send that file to
whatever would be downstream of the Git command in the pipe. So, a
mechanical rewrite of the above (without thinking too hard about it)
might be:

    git -C main fetch --recurse-submodules >out 2>&1 &&
    ! grep -v -m1 "Fetching submodule sub$" &&

> +       git -C main submodule status |
> +               sed -e "s/^-//" -e "s/ sub$//" >actual &&

Same comment about avoiding Git upstream in a pipe, so perhaps:

    git -C main submodule status >out &&
    sed -e "s/^-//" -e "s/ sub$//" out >actual &&

> +       test_cmp expect actual
> +'

Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kästle]

Hi Eric,

On 04.12.20 19:06, Eric Sunshine wrote:
> On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle <peter.kaestle@nokia.com> wrote:
>> [...]
>> Furthermore a regression test case is added, which tests for recursive
>> fetches on a superproject with uninitialized sub repositories.  This
>> issue was leading to an infinite loop when doing a revert of a62387b.
> 
> Just a few small comments (nothing comprehensive) from a quick scan of
> the patch...
> 
> Mostly they are just minor style issues, not necessarily worth a
> re-roll, but there is one actionable item.

thanks for your comments.  A new patch will follow soon.


>> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
>> ---
>> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
>> @@ -719,4 +719,98 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
>> +add_commit_push () {
>> +       dir="$1"
>> +       msg="$2"
>> +       shift 2
> 
> We typically recommend including these assignments in the &&-chain to
> future-proof against someone later inserting code above them and not
> realizing that that code is not part of the &&-chain, in which case if
> the new code fails, the failure might go unnoticed.

ok

> 
>> +       git -C "$dir" add "$@" &&
>> +       git -C "$dir" commit -a -m "$msg" &&
>> +       git -C "$dir" push
>> +}
>> +
>> +compare_refs_in_dir () {
>> +       fail= &&
>> +       if test "x$1" = 'x!'
>> +       then
>> +               fail='!' &&
>> +               shift
>> +       fi &&
>> +       git -C "$1" rev-parse --verify "$2" >expect &&
>> +       git -C "$3" rev-parse --verify "$4" >actual &&
>> +       eval $fail test_cmp expect actual
>> +}
> 
> We have a test_cmp_rev() similar to this but it doesn't support -C as
> some of our other test functions do. I briefly wondered if it would
> make sense to extend it to understand -C, but even that wouldn't help
> this case since compare_refs_in_dir() introduced here involves two
> distinct directories. The need here is so special-purpose that it
> likely would not make sense to upgrade test_cmp_rev() to accommodate
> it. Okay.

Yes, I saw that there's a similar function and I tried to modify this 
one first.  Unfortunately this didn't work without touching much 
unaffected test code.  So I propose to continue with this additional 
function.


>> +test_expect_success 'setup nested submodule fetch test' '
>> +       # does not depend on any previous test setups
>> +
>> +       for repo in outer middle inner
>> +       do
>> +               (
>> +                       git init --bare $repo &&
>> +                       git clone $repo ${repo}_content &&
>> +                       echo "$repo" >"${repo}_content/file" &&
>> +                       add_commit_push ${repo}_content "initial" file
>> +               ) || return 1
>> +       done &&
> 
> What is the purpose of the subshell here? Is it to ensure that commits
> in each repo have identical timestamps? Or is it just for making the
> && and || expression more clear? If the latter, we normally don't
> bother with the parentheses.

It was intended to make the correlation of && and || clear.  I have 
experienced many cases in the past where things were screwed because it 
was not clearly understood by everybody.  I'll propose next patch 
without this subshell.

>> +       git clone outer A &&
>> +       git -C A submodule add "$pwd/middle" &&
>> +       git -C A/middle/ submodule add "$pwd/inner" &&
>> +       add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
>> +       add_commit_push A/ "adding middle sub" .gitmodules middle &&
>> +
>> +       git clone outer B &&
>> +       git -C B/ submodule update --init middle &&
>> +
>> +       compare_refs_in_dir A HEAD B HEAD &&
>> +       compare_refs_in_dir A/middle HEAD B/middle HEAD &&
>> +       test -f B/file &&
>> +       test -f B/middle/file &&
>> +       ! test -f B/middle/inner/file &&
> 
> These days we typically use test_path_exists() (or
> test_path_is_file()) and test_path_is_missing() rather than bare
> `test`.

ok.


>> +test_expect_success 'setup recursive fetch with uninit submodule' '
>> +       # does not depend on any previous test setups
>> +
>> +       git init main &&
>> +       git init sub &&
>> +
>> +       touch sub/file &&
> 
> Unless the timestamp of the file is significant to the test, in which
> case `touch` is used, we normally create empty files like this:
> 
>      >sub/file &&

ok.


> 
>> +test_expect_success 'recursive fetch with uninit submodule' '
>> +       git -C main submodule deinit -f sub &&
>> +       ! git -C main fetch --recurse-submodules |&
>> +               grep -v -m1 "Fetching submodule sub$" &&
> 
> We want the test scripts to be portable, thus avoid Bashisms such as `|&`. > We also avoid placing a Git command upstream in a pipe since doing so
> causes the exit code of the Git command to be lost. Instead, we would
> normally send the Git output to a file and then send that file to
> whatever would be downstream of the Git command in the pipe. So, a
> mechanical rewrite of the above (without thinking too hard about it)
> might be:
> 
>      git -C main fetch --recurse-submodules >out 2>&1 &&
>      ! grep -v -m1 "Fetching submodule sub$" &&

In general I agree, but for this special test case, it's required to 
have the two commands connected by a pipe, as the grep needs to kill the 
git call in error case.  Otherwise for this regression git would go for 
an infinite recursion loop.

Of course, we can go for a "git 2>&1 | grep" solution.


>> +       git -C main submodule status |
>> +               sed -e "s/^-//" -e "s/ sub$//" >actual &&
> 
> Same comment about avoiding Git upstream in a pipe, so perhaps:
> 
>      git -C main submodule status >out &&
>      sed -e "s/^-//" -e "s/ sub$//" out >actual &&
> 
>> +       test_cmp expect actual
>> +'

ok.

-- 
kind regards
--peter;

Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo

[Eric Sunshine]

On Mon, Dec 7, 2020 at 3:29 AM Peter Kästle <peter.kaestle@nokia.com> wrote:
> On 04.12.20 19:06, Eric Sunshine wrote:
> > On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle <peter.kaestle@nokia.com> wrote:
> >> +       ! git -C main fetch --recurse-submodules |&
> >> +               grep -v -m1 "Fetching submodule sub$" &&
> >
> > We want the test scripts to be portable, thus avoid Bashisms such as `|&`.
> >      git -C main fetch --recurse-submodules >out 2>&1 &&
> >      ! grep -v -m1 "Fetching submodule sub$" &&
>
> In general I agree, but for this special test case, it's required to
> have the two commands connected by a pipe, as the grep needs to kill the
> git call in error case.  Otherwise for this regression git would go for
> an infinite recursion loop.
>
> Of course, we can go for a "git 2>&1 | grep" solution.

In that case, an in-code comment explaining why the output of `git`
must be piped to `grep` would be helpful since it is not uncommon for
people to "modernize" test code as they are working in the vicinity,
and removing such pipes is one of the modernizations often made. A
comment would help avoid such a change.

[PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kaestle]

A regression has been introduced by a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28).

The scenario in which it triggers is when one has a remote repository
with a subrepository inside a subrepository like this:
superproject/middle_repo/inner_repo

Person A and B have both a clone of it, while Person B is not working
with the inner_repo and thus does not have it initialized in his working
copy.

Now person A introduces a change to the inner_repo and propagates it
through the middle_repo and the superproject.

Once person A pushed the changes and person B wants to fetch them using
"git fetch" on superproject level, B's git call will return with error
saying:

Could not access submodule 'inner_repo'
Errors during submodule fetch:
         middle_repo

Expectation is that in this case the inner submodule will be recognized
as uninitialized subrepository and skipped by the git fetch command.

This used to work correctly before 'a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28)'.

Starting with a62387b the code wants to evaluate "is_empty_dir()" inside
.git/modules for a directory only existing in the worktree, delivering
then of course wrong return value.

This patch ensures is_empty_dir() is getting the correct path of the
uninitialized submodule by concatenation of the actual worktree and the
name of the uninitialized submodule.

Furthermore a regression test case is added, which tests for recursive
fetches on a superproject with uninitialized sub repositories.  This
issue was leading to an infinite loop when doing a revert of a62387b.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
CC: Junio C Hamano <gitster@pobox.com>
CC: Philippe Blain <levraiphilippeblain@gmail.com>
CC: Ralf Thielow <ralf.thielow@gmail.com>
CC: Eric Sunshine <sunshine@sunshineco.com>
---
 submodule.c                 |   7 ++-
 t/t5526-fetch-submodules.sh | 104 ++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index b3bb59f066..b561445329 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,
 			strbuf_release(&submodule_prefix);
 			return 1;
 		} else {
+			struct strbuf empty_submodule_path = STRBUF_INIT;
 
 			fetch_task_release(task);
 			free(task);
@@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,
 			 * An empty directory is normal,
 			 * the submodule is not initialized
 			 */
+			strbuf_addf(&empty_submodule_path, "%s/%s/",
+							spf->r->worktree,
+							ce->name);
 			if (S_ISGITLINK(ce->ce_mode) &&
-			    !is_empty_dir(ce->name)) {
+			    !is_empty_dir(empty_submodule_path.buf)) {
 				spf->result = 1;
 				strbuf_addf(err,
 					    _("Could not access submodule '%s'\n"),
 					    ce->name);
 			}
+			strbuf_release(&empty_submodule_path);
 		}
 	}
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index dd8e423d25..666dd1e2b7 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -719,4 +719,108 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
 	)
 '
 
+add_commit_push () {
+	dir="$1" &&
+	msg="$2" &&
+	shift 2 &&
+	git -C "$dir" add "$@" &&
+	git -C "$dir" commit -a -m "$msg" &&
+	git -C "$dir" push
+}
+
+compare_refs_in_dir () {
+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail='!' &&
+		shift
+	fi &&
+	git -C "$1" rev-parse --verify "$2" >expect &&
+	git -C "$3" rev-parse --verify "$4" >actual &&
+	eval $fail test_cmp expect actual
+}
+
+
+test_expect_success 'setup nested submodule fetch test' '
+	# does not depend on any previous test setups
+
+	for repo in outer middle inner
+	do
+		git init --bare $repo &&
+		git clone $repo ${repo}_content &&
+		echo "$repo" >"${repo}_content/file" &&
+		add_commit_push ${repo}_content "initial" file ||
+		return 1
+	done &&
+
+	git clone outer A &&
+	git -C A submodule add "$pwd/middle" &&
+	git -C A/middle/ submodule add "$pwd/inner" &&
+	add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
+	add_commit_push A/ "adding middle sub" .gitmodules middle &&
+
+	git clone outer B &&
+	git -C B/ submodule update --init middle &&
+
+	compare_refs_in_dir A HEAD B HEAD &&
+	compare_refs_in_dir A/middle HEAD B/middle HEAD &&
+	test_path_is_file B/file &&
+	test_path_is_file B/middle/file &&
+	test_path_is_missing B/middle/inner/file &&
+
+	echo "change on inner repo of A" >"A/middle/inner/file" &&
+	add_commit_push A/middle/inner "change on inner" file &&
+	add_commit_push A/middle "change on inner" inner &&
+	add_commit_push A "change on inner" middle
+'
+
+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
+	# depends on previous test for setup
+
+	git -C B/ fetch &&
+	compare_refs_in_dir A origin/master B origin/master
+'
+
+
+test_expect_success 'setup recursive fetch with uninit submodule' '
+	# does not depend on any previous test setups
+
+	git init main &&
+	git init sub &&
+
+	>sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+	git -C sub rev-parse HEAD >expect &&
+
+	git -C main submodule add ../sub &&
+	git -C main submodule init &&
+	git -C main submodule update --checkout &&
+	git -C main submodule status >out &&
+	sed -e "s/^ //" -e "s/ sub .*$//" out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'recursive fetch with uninit submodule' '
+	# depends on previous test for setup
+
+	git -C main submodule deinit -f sub &&
+
+	# In a regression the following git call will run into infinite recursion.
+	# To handle that, we connect the grep command to the git call by a pipe
+	# so that grep can kill the infinite recusion when detected.
+	# The recursion creates git output like:
+	# Fetching submodule sub
+	# Fetching submodule sub/sub              <-- [1]
+	# Fetching submodule sub/sub/sub
+	# ...
+	# [1] grep will trigger here and kill git by exiting and closing its stdin
+
+	! git -C main fetch --recurse-submodules 2>&1 |
+		grep -v -m1 "Fetching submodule sub$" &&
+	git -C main submodule status >out &&
+	sed -e "s/^-//" -e "s/ sub$//" out >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.29.2

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Philippe Blain]

Hi Peter,

> Le 7 déc. 2020 à 08:46, Peter Kaestle <peter.kaestle@nokia.com> a écrit :
> 
> A regression has been introduced by a62387b (submodule.c: fetch in
> submodules git directory instead of in worktree, 2018-11-28).
> 
> The scenario in which it triggers is when one has a remote repository
> with a subrepository inside a subrepository like this:
> superproject/middle_repo/inner_repo

The correct terminology is "submodule", not "subrepository". 

Also, (minor point) I would just write "when one has a repository", 
as its simpler (the repository by itself is not "remote", it is only "remote" 
in relation the repositories that are cloned from it).

> Person A and B have both a clone of it, while Person B is not working
> with the inner_repo and thus does not have it initialized in his working
> copy.
> 
> Now person A introduces a change to the inner_repo and propagates it
> through the middle_repo and the superproject.
> 
> Once person A pushed the changes and person B wants to fetch them using
> "git fetch" on superproject level,

s/on/at the/

> B's git call will return with error
> saying:
> 
> Could not access submodule 'inner_repo'
> Errors during submodule fetch:
>         middle_repo
> 
> Expectation is that in this case the inner submodule will be recognized
> as uninitialized subrepository and skipped by the git fetch command.

here again, terminology: "as an uninitialized submodule" 

> This used to work correctly before 'a62387b (submodule.c: fetch in
> submodules git directory instead of in worktree, 2018-11-28)'.
> 
> Starting with a62387b the code wants to evaluate "is_empty_dir()" inside
> .git/modules for a directory only existing in the worktree, delivering
> then of course wrong return value.
> 
> This patch ensures is_empty_dir() is getting the correct path of the
> uninitialized submodule by concatenation of the actual worktree and the
> name of the uninitialized submodule.
> 
> Furthermore a regression test case is added, which tests for recursive
> fetches on a superproject with uninitialized sub repositories.
>  This
> issue was leading to an infinite loop when doing a revert of a62387b.

I would maybe add more details here, something like the following 
(we can cite your previous attempt, because it was merged to 'master'):

The first attempt to fix this regression, in 1b7ac4e6d4 (submodules: 
fix of regression on fetching of non-init subsub-repo, 2020-11-12), by simply
reverting a62387b, resulted in
an infinite loop of submodule fetches in the simpler case of a recursive fetch of a superproject with
uninitialized submodules, and so this commit was reverted in 7091499bc0 (Revert 
"submodules: fix of regression on fetching of non-init subsub-repo", 2020-12-02).
To prevent future breakages, also add a regression test for this scenario.

> 
> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
> CC: Junio C Hamano <gitster@pobox.com>
> CC: Philippe Blain <levraiphilippeblain@gmail.com>
> CC: Ralf Thielow <ralf.thielow@gmail.com>
> CC: Eric Sunshine <sunshine@sunshineco.com>
> ---
> submodule.c                 |   7 ++-
> t/t5526-fetch-submodules.sh | 104 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 110 insertions(+), 1 deletion(-)
> 
> diff --git a/submodule.c b/submodule.c
> index b3bb59f066..b561445329 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,
> 			strbuf_release(&submodule_prefix);
> 			return 1;
> 		} else {
> +			struct strbuf empty_submodule_path = STRBUF_INIT;
> 
> 			fetch_task_release(task);
> 			free(task);
> @@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,
> 			 * An empty directory is normal,
> 			 * the submodule is not initialized
> 			 */
> +			strbuf_addf(&empty_submodule_path, "%s/%s/",
> +							spf->r->worktree,
> +							ce->name);
> 			if (S_ISGITLINK(ce->ce_mode) &&
> -			    !is_empty_dir(ce->name)) {
> +			    !is_empty_dir(empty_submodule_path.buf)) {
> 				spf->result = 1;
> 				strbuf_addf(err,
> 					    _("Could not access submodule '%s'\n"),
> 					    ce->name);
> 			}
> +			strbuf_release(&empty_submodule_path);
> 		}
> 	}


Maybe a personal preference, but I would have gone for something a little simpler, like the following:


diff --git a/submodule.c b/submodule.c
index b3bb59f066..4200865174 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1486,7 +1486,7 @@ static int get_next_submodule(struct child_process *cp,
                         * the submodule is not initialized
                         */
                        if (S_ISGITLINK(ce->ce_mode) &&
-                           !is_empty_dir(ce->name)) {
+                           !is_empty_dir(repo_worktree_path(spf->r, "%s", ce->name))) {
                                spf->result = 1;
                                strbuf_addf(err,
                                            _("Could not access submodule '%s'\n"),

> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index dd8e423d25..666dd1e2b7 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -719,4 +719,108 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
> 	)
> '
> 
> +add_commit_push () {
> +	dir="$1" &&
> +	msg="$2" &&
> +	shift 2 &&
> +	git -C "$dir" add "$@" &&
> +	git -C "$dir" commit -a -m "$msg" &&
> +	git -C "$dir" push
> +}
> +
> +compare_refs_in_dir () {
> +	fail= &&
> +	if test "x$1" = 'x!'
> +	then
> +		fail='!' &&
> +		shift
> +	fi &&
> +	git -C "$1" rev-parse --verify "$2" >expect &&
> +	git -C "$3" rev-parse --verify "$4" >actual &&
> +	eval $fail test_cmp expect actual
> +}
> +
> +
> +test_expect_success 'setup nested submodule fetch test' '
> +	# does not depend on any previous test setups
> +
> +	for repo in outer middle inner
> +	do
> +		git init --bare $repo &&
> +		git clone $repo ${repo}_content &&
> +		echo "$repo" >"${repo}_content/file" &&
> +		add_commit_push ${repo}_content "initial" file ||
> +		return 1
> +	done &&
> +
> +	git clone outer A &&
> +	git -C A submodule add "$pwd/middle" &&
> +	git -C A/middle/ submodule add "$pwd/inner" &&
> +	add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
> +	add_commit_push A/ "adding middle sub" .gitmodules middle &&
> +
> +	git clone outer B &&
> +	git -C B/ submodule update --init middle &&
> +
> +	compare_refs_in_dir A HEAD B HEAD &&
> +	compare_refs_in_dir A/middle HEAD B/middle HEAD &&
> +	test_path_is_file B/file &&
> +	test_path_is_file B/middle/file &&
> +	test_path_is_missing B/middle/inner/file &&
> +
> +	echo "change on inner repo of A" >"A/middle/inner/file" &&
> +	add_commit_push A/middle/inner "change on inner" file &&
> +	add_commit_push A/middle "change on inner" inner &&
> +	add_commit_push A "change on inner" middle
> +'
> +
> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
> +	# depends on previous test for setup
> +
> +	git -C B/ fetch &&
> +	compare_refs_in_dir A origin/master B origin/master
> +'
> +
> +
> +test_expect_success 'setup recursive fetch with uninit submodule' '
> +	# does not depend on any previous test setups
> +
> +	git init main &&
> +	git init sub &&
> +
> +	>sub/file &&
> +	git -C sub add file &&
> +	git -C sub commit -m "add file" &&
> +	git -C sub rev-parse HEAD >expect &&
> +
> +	git -C main submodule add ../sub &&
> +	git -C main submodule init &&
> +	git -C main submodule update --checkout &&

These two steps are unnecessary as they are implicitly done by 'git submodule add'.
I think we could reflect real life a little bit more by cloning the superproject, and running
the 'recursive fetch with uninit submodule' test below in the clone.

> +	git -C main submodule status >out &&
> +	sed -e "s/^ //" -e "s/ sub .*$//" out >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'recursive fetch with uninit submodule' '
> +	# depends on previous test for setup
> +
> +	git -C main submodule deinit -f sub &&

Here you are deiniting the submodule, such that 
the Git directory will stay in .git/modules/sub. This is not the same thing
as a submodule that was never initialized ("uninitialized"), for which .git/modules/sub
will not yet exist. So maybe we could harden the tests by also testing
for that scenario ? I don't know... maybe the infinite loop only happens
if .git/modules/sub actually already exists. If so, the test name should be
"recursive fetch with deinitialized submodule", I think.

> +
> +	# In a regression the following git call will run into infinite recursion.
> +	# To handle that, we connect the grep command to the git call by a pipe
> +	# so that grep can kill the infinite recusion when detected.
> +	# The recursion creates git output like:
> +	# Fetching submodule sub
> +	# Fetching submodule sub/sub              <-- [1]
> +	# Fetching submodule sub/sub/sub
> +	# ...
> +	# [1] grep will trigger here and kill git by exiting and closing its stdin
> +
> +	! git -C main fetch --recurse-submodules 2>&1 |
> +		grep -v -m1 "Fetching submodule sub$" &&
> +	git -C main submodule status >out &&
> +	sed -e "s/^-//" -e "s/ sub$//" out >actual &&
> +	test_cmp expect actual
> +'
> +
> test_done


Thanks for working on that, and sorry for not having the time to comment before
you sent v2.

Cheers,

Philippe.

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Maybe a personal preference, but I would have gone for something a
> little simpler, like the following:
>
> diff --git a/submodule.c b/submodule.c
> index b3bb59f066..4200865174 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1486,7 +1486,7 @@ static int get_next_submodule(struct child_process *cp,
>                          * the submodule is not initialized
>                          */
>                         if (S_ISGITLINK(ce->ce_mode) &&
> -                           !is_empty_dir(ce->name)) {
> +                           !is_empty_dir(repo_worktree_path(spf->r, "%s", ce->name))) {

But then you leak the return value from repo_worktree_path(), no?

>> +test_expect_success 'recursive fetch with uninit submodule' '
>> +	# depends on previous test for setup
>> +
>> +	git -C main submodule deinit -f sub &&
>
> Here you are deiniting the submodule, such that 
> the Git directory will stay in .git/modules/sub. This is not the same thing
> as a submodule that was never initialized ("uninitialized"), for which .git/modules/sub
> will not yet exist. So maybe we could harden the tests by also testing
> for that scenario ? I don't know... maybe the infinite loop only happens
> if .git/modules/sub actually already exists. If so, the test name should be
> "recursive fetch with deinitialized submodule", I think.

Even if the original breakage happens only for deinitialized case,
it would be sensible to test uninitialized one as well, I would
think.

> Thanks for working on that, and sorry for not having the time to comment before
> you sent v2.

Thanks, all.  

It's not like we corral everybody to a single place on a single day
to work on a single thing.  Reviews ov v2 by reviewers who have not
seen v1 is totally expected and very much appreciated.

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kästle]

On 07.12.20 20:43, Junio C Hamano wrote:
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
>> Thanks for working on that, and sorry for not having the time to comment before
>> you sent v2.
> 
> Thanks, all.
> 
> It's not like we corral everybody to a single place on a single day
> to work on a single thing.  Reviews ov v2 by reviewers who have not
> seen v1 is totally expected and very much appreciated.
> 

Thank you very much for all your comments, they're much appreciated.  I 
prefer spending more time on it now for getting it right, than to take 
another revert-rethink-refactor round.
It will take me some time today to work on them.

-- 
kind regards
--peter;

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> I would maybe add more details here, something like the following 
> (we can cite your previous attempt, because it was merged to 'master'):
>
> The first attempt to fix this regression, in 1b7ac4e6d4
> (submodules: fix of regression on fetching of non-init
> subsub-repo, 2020-11-12), by simply reverting a62387b, resulted in
> an infinite loop of submodule fetches in the simpler case of a
> recursive fetch of a superproject with uninitialized submodules,
> and so this commit was reverted in 7091499bc0 (Revert "submodules:
> fix of regression on fetching of non-init subsub-repo",
> 2020-12-02).  To prevent future breakages, also add a regression
> test for this scenario.

Forgot to mention in my other response, but I do find this a very
sensible addition.

Thanks.

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kästle]

On 07.12.20 19:42, Philippe Blain wrote:
> Hi Peter,
> 
>> Le 7 déc. 2020 à 08:46, Peter Kaestle <peter.kaestle@nokia.com> a écrit :
>>
>> A regression has been introduced by a62387b (submodule.c: fetch in
>> submodules git directory instead of in worktree, 2018-11-28).
>>
>> The scenario in which it triggers is when one has a remote repository
>> with a subrepository inside a subrepository like this:
>> superproject/middle_repo/inner_repo
> 
> The correct terminology is "submodule", not "subrepository".
> 
> Also, (minor point) I would just write "when one has a repository",
> as its simpler (the repository by itself is not "remote", it is only "remote"
> in relation the repositories that are cloned from it).

ok.



>> Person A and B have both a clone of it, while Person B is not working
>> with the inner_repo and thus does not have it initialized in his working
>> copy.
>>
>> Now person A introduces a change to the inner_repo and propagates it
>> through the middle_repo and the superproject.
>>
>> Once person A pushed the changes and person B wants to fetch them using
>> "git fetch" on superproject level,
> 
> s/on/at the/

ok.


>> B's git call will return with error
>> saying:
>>
>> Could not access submodule 'inner_repo'
>> Errors during submodule fetch:
>>          middle_repo
>>
>> Expectation is that in this case the inner submodule will be recognized
>> as uninitialized subrepository and skipped by the git fetch command.
> 
> here again, terminology: "as an uninitialized submodule"

ok.


>> This used to work correctly before 'a62387b (submodule.c: fetch in
>> submodules git directory instead of in worktree, 2018-11-28)'.
>>
>> Starting with a62387b the code wants to evaluate "is_empty_dir()" inside
>> .git/modules for a directory only existing in the worktree, delivering
>> then of course wrong return value.
>>
>> This patch ensures is_empty_dir() is getting the correct path of the
>> uninitialized submodule by concatenation of the actual worktree and the
>> name of the uninitialized submodule.
>>
>> Furthermore a regression test case is added, which tests for recursive
>> fetches on a superproject with uninitialized sub repositories.
>>   This
>> issue was leading to an infinite loop when doing a revert of a62387b.
> 
> I would maybe add more details here, something like the following
> (we can cite your previous attempt, because it was merged to 'master'):
> 
> The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:
> fix of regression on fetching of non-init subsub-repo, 2020-11-12), by simply
> reverting a62387b, resulted in
> an infinite loop of submodule fetches in the simpler case of a recursive fetch of a superproject with
> uninitialized submodules, and so this commit was reverted in 7091499bc0 (Revert
> "submodules: fix of regression on fetching of non-init subsub-repo", 2020-12-02).
> To prevent future breakages, also add a regression test for this scenario.

Jip, I like that.


>>
>> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
>> CC: Junio C Hamano <gitster@pobox.com>
>> CC: Philippe Blain <levraiphilippeblain@gmail.com>
>> CC: Ralf Thielow <ralf.thielow@gmail.com>
>> CC: Eric Sunshine <sunshine@sunshineco.com>
>> ---
>> submodule.c                 |   7 ++-
>> t/t5526-fetch-submodules.sh | 104 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 110 insertions(+), 1 deletion(-)
>>
>> diff --git a/submodule.c b/submodule.c
>> index b3bb59f066..b561445329 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,
>> 			strbuf_release(&submodule_prefix);
>> 			return 1;
>> 		} else {
>> +			struct strbuf empty_submodule_path = STRBUF_INIT;
>>
>> 			fetch_task_release(task);
>> 			free(task);
>> @@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,
>> 			 * An empty directory is normal,
>> 			 * the submodule is not initialized
>> 			 */
>> +			strbuf_addf(&empty_submodule_path, "%s/%s/",
>> +							spf->r->worktree,
>> +							ce->name);
>> 			if (S_ISGITLINK(ce->ce_mode) &&
>> -			    !is_empty_dir(ce->name)) {
>> +			    !is_empty_dir(empty_submodule_path.buf)) {
>> 				spf->result = 1;
>> 				strbuf_addf(err,
>> 					    _("Could not access submodule '%s'\n"),
>> 					    ce->name);
>> 			}
>> +			strbuf_release(&empty_submodule_path);
>> 		}
>> 	}
> 
> 
> Maybe a personal preference, but I would have gone for something a little simpler, like the following:
> 
> 
> diff --git a/submodule.c b/submodule.c
> index b3bb59f066..4200865174 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1486,7 +1486,7 @@ static int get_next_submodule(struct child_process *cp,
>                           * the submodule is not initialized
>                           */
>                          if (S_ISGITLINK(ce->ce_mode) &&
> -                           !is_empty_dir(ce->name)) {
> +                           !is_empty_dir(repo_worktree_path(spf->r, "%s", ce->name))) {

I'm not deep enough into the git code to judge which approach is the 
better one.  From my perspective, being a foreigner to the git code, I 
like my proposed code more, as for me it's much easier to understand 
what's happening by having a meaningful variable name and without being 
forced to dig into outer functions first.
Also Junio C Hamano <gitster@pobox.com> is having some concerns, which I 
can't judge:

> But then you leak the return value from repo_worktree_path(), no?

Thus for v3 I'll stick to my proposal and when you'll review it, please 
discuss with each other whether I should go for a v4 using 
repo_worktree_path().

[...]

>> +
>> +test_expect_success 'setup recursive fetch with uninit submodule' '
>> +	# does not depend on any previous test setups
>> +
>> +	git init main &&
>> +	git init sub &&
>> +
>> +	>sub/file &&
>> +	git -C sub add file &&
>> +	git -C sub commit -m "add file" &&
>> +	git -C sub rev-parse HEAD >expect &&
>> +
>> +	git -C main submodule add ../sub &&
>> +	git -C main submodule init &&
>> +	git -C main submodule update --checkout &&
> 
> These two steps are unnecessary as they are implicitly done by 'git submodule add'.
> I think we could reflect real life a little bit more by cloning the superproject, and running
> the 'recursive fetch with uninit submodule' test below in the clone.

Yes, you're right, "...init" and "...update..." can be removed.


>> +	git -C main submodule status >out &&
>> +	sed -e "s/^ //" -e "s/ sub .*$//" out >actual &&
>> +	test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'recursive fetch with uninit submodule' '
>> +	# depends on previous test for setup
>> +
>> +	git -C main submodule deinit -f sub &&
> 
> Here you are deiniting the submodule, such that
> the Git directory will stay in .git/modules/sub. This is not the same thing
> as a submodule that was never initialized ("uninitialized"), for which .git/modules/sub
> will not yet exist. So maybe we could harden the tests by also testing
> for that scenario ? I don't know... maybe the infinite loop only happens
> if .git/modules/sub actually already exists. If so, the test name should be
> "recursive fetch with deinitialized submodule", I think.

I added another test case for v3, which checks for this in case of never 
initialized submodule.  When executing the test, I can see that the 
infinite loop regression only occurs after doing the init followed by a 
deinit.  Thus renaming the test accordingly.

-- 
best regards
--peter;

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Junio C Hamano]

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

> +add_commit_push () {
> +	dir="$1" &&
> +	msg="$2" &&
> +	shift 2 &&
> +	git -C "$dir" add "$@" &&
> +	git -C "$dir" commit -a -m "$msg" &&
> +	git -C "$dir" push
> +}
> +
> +compare_refs_in_dir () {
> +	fail= &&
> +	if test "x$1" = 'x!'
> +	then
> +		fail='!' &&
> +		shift
> +	fi &&
> +	git -C "$1" rev-parse --verify "$2" >expect &&
> +	git -C "$3" rev-parse --verify "$4" >actual &&
> +	eval $fail test_cmp expect actual
> +}



> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
> +	# depends on previous test for setup
> +
> +	git -C B/ fetch &&
> +	compare_refs_in_dir A origin/master B origin/master

Can we do this without relying on the name of the default branch?
Perhaps when outer, middle and inner are prepared, they can be
forced to be on the 'sample' (not 'master' nor 'main') branch, or
something like that?

> +test_expect_success 'setup recursive fetch with uninit submodule' '
> +	# does not depend on any previous test setups
> +
> +	git init main &&
> +	git init sub &&

"super vs sub" would give us a better contrast than "main vs sub",
and it would help reduce mistakes in the mechanical conversion of
"master" to "main" happening in another topic.

> +	# In a regression the following git call will run into infinite recursion.
> +	# To handle that, we connect the grep command to the git call by a pipe
> +	# so that grep can kill the infinite recusion when detected.
> +	# The recursion creates git output like:
> +	# Fetching submodule sub
> +	# Fetching submodule sub/sub              <-- [1]
> +	# Fetching submodule sub/sub/sub
> +	# ...
> +	# [1] grep will trigger here and kill git by exiting and closing its stdin

"trigger here and kill..." -> "stop reading and cause git to
eventually stop and die"

But we probably cannot use 'grep -m1' so it is a moot point.

> +
> +	! git -C main fetch --recurse-submodules 2>&1 |
> +		grep -v -m1 "Fetching submodule sub$" &&

Unfortunately, "grep -m<count>" is not even in POSIX, I would think.

What do we expect to happen in the correct case?

 - A line "Fetching submodule sub" and nothing else is given?  That
   feels a bit brittle (how are we making sure, in the presence of
   "2>&1", that we will not get any other output, like progress?)

 - "sub" is the only thing that appears on lines that begin with
   "Fetching submodule" (i.e. "Fetching submodule $something" where
   $something is not 'sub' is an error), and we allow other garbage
   in the output?  That would be a bit more robust than the above.

As you seem to be comfortable using "sed" below, perhaps use it to
extract the first few lines that say "^Fetching submodule " from the
output and stop, and check that the output has only one such line
about 'sub' and nothing else?

> +	git -C main submodule status >out &&
> +	sed -e "s/^-//" -e "s/ sub$//" out >actual &&
> +	test_cmp expect actual

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Philippe Blain]

Hi Junio,

> Le 7 déc. 2020 à 14:22, Junio C Hamano <gitster@pobox.com> a écrit :
> 
> Peter Kaestle <peter.kaestle@nokia.com> writes:
> 
>> +add_commit_push () {
>> +	dir="$1" &&
>> +	msg="$2" &&
>> +	shift 2 &&
>> +	git -C "$dir" add "$@" &&
>> +	git -C "$dir" commit -a -m "$msg" &&
>> +	git -C "$dir" push
>> +}
>> +
>> +compare_refs_in_dir () {
>> +	fail= &&
>> +	if test "x$1" = 'x!'
>> +	then
>> +		fail='!' &&
>> +		shift
>> +	fi &&
>> +	git -C "$1" rev-parse --verify "$2" >expect &&
>> +	git -C "$3" rev-parse --verify "$4" >actual &&
>> +	eval $fail test_cmp expect actual
>> +}
> 
> 
> 
>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
>> +	# depends on previous test for setup
>> +
>> +	git -C B/ fetch &&
>> +	compare_refs_in_dir A origin/master B origin/master
> 
> Can we do this without relying on the name of the default branch?
> Perhaps when outer, middle and inner are prepared, they can be
> forced to be on the 'sample' (not 'master' nor 'main') branch, or
> something like that?

Or, simpler, we could call "git remote set-head -a' 
in A and B in the setup script, which would make
origin/HEAD in A and B point to the default branch, 
such that the call here could be :

compare_refs_in_dir A origin/HEAD B origin/HEAD

Philippe.

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

>>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
>>> +	# depends on previous test for setup
>>> +
>>> +	git -C B/ fetch &&
>>> +	compare_refs_in_dir A origin/master B origin/master
>> 
>> Can we do this without relying on the name of the default branch?
>> Perhaps when outer, middle and inner are prepared, they can be
>> forced to be on the 'sample' (not 'master' nor 'main') branch, or
>> something like that?
>
> Or, simpler, we could call "git remote set-head -a' 
> in A and B in the setup script, which would make
> origin/HEAD in A and B point to the default branch, 
> such that the call here could be :

The set-up prepares A and B by cloning from elsewhere, no?  Should
we even need a set-head call?

> compare_refs_in_dir A origin/HEAD B origin/HEAD

Yes, using HEAD would be another simple way to avoid having to rely
on the default behaviour.

THanks.

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Philippe Blain <levraiphilippeblain@gmail.com> writes:
>
>>>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
>>>> +	# depends on previous test for setup
>>>> +
>>>> +	git -C B/ fetch &&
>>>> +	compare_refs_in_dir A origin/master B origin/master
>>> 
>>> Can we do this without relying on the name of the default branch?
>>> Perhaps when outer, middle and inner are prepared, they can be
>>> forced to be on the 'sample' (not 'master' nor 'main') branch, or
>>> something like that?
>>
>> Or, simpler, we could call "git remote set-head -a' 
>> in A and B in the setup script, which would make
>> origin/HEAD in A and B point to the default branch, 
>> such that the call here could be :
>
> The set-up prepares A and B by cloning from elsewhere, no?  Should
> we even need a set-head call?

Ah, they are created by cloning an empty repository.  That explains
why.  Thanks.

>> compare_refs_in_dir A origin/HEAD B origin/HEAD
>
> Yes, using HEAD would be another simple way to avoid having to rely
> on the default behaviour.
>
> THanks.

Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kästle]

On 07.12.20 20:22, Junio C Hamano wrote:
> Peter Kaestle <peter.kaestle@nokia.com> writes:
> 
>> +add_commit_push () {
>> +	dir="$1" &&
>> +	msg="$2" &&
>> +	shift 2 &&
>> +	git -C "$dir" add "$@" &&
>> +	git -C "$dir" commit -a -m "$msg" &&
>> +	git -C "$dir" push
>> +}
>> +
>> +compare_refs_in_dir () {
>> +	fail= &&
>> +	if test "x$1" = 'x!'
>> +	then
>> +		fail='!' &&
>> +		shift
>> +	fi &&
>> +	git -C "$1" rev-parse --verify "$2" >expect &&
>> +	git -C "$3" rev-parse --verify "$4" >actual &&
>> +	eval $fail test_cmp expect actual
>> +}
> 
> 
> 
>> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
>> +	# depends on previous test for setup
>> +
>> +	git -C B/ fetch &&
>> +	compare_refs_in_dir A origin/master B origin/master
> 
> Can we do this without relying on the name of the default branch?
> Perhaps when outer, middle and inner are prepared, they can be
> forced to be on the 'sample' (not 'master' nor 'main') branch, or
> something like that?

Using origin/HEAD for compare_refs_in_dir should be fine without 
additional setup, as for the regression the "git -C B/ fetch" will fail 
and return with false (see description of the patch).  This 
compare_refs_in_dir is just for additional checking as you proposed in 
the mail:
https://public-inbox.org/git/xmqqk0uuct94.fsf@gitster.c.googlers.com/
------------8<-------------
> And from B that was an original copy of A with only the top and
> middle layer instantiated, you run "git fetch".  Are you happy as
> long as "git fetch" does not exit with non-zero status?  That is
> hard to believe---it may be a necessary condition for the command to
> exit with zero status, but you have other expectations, like what
> commit the remote tracking branch refs/remotes/origin/HEAD ought to
> be pointing at.  I think we should check that, too.
----------->8-------------


> 
>> +test_expect_success 'setup recursive fetch with uninit submodule' '
>> +	# does not depend on any previous test setups
>> +
>> +	git init main &&
>> +	git init sub &&
> 
> "super vs sub" would give us a better contrast than "main vs sub",
> and it would help reduce mistakes in the mechanical conversion of
> "master" to "main" happening in another topic.
> 

ok.


>> +	# In a regression the following git call will run into infinite recursion.
>> +	# To handle that, we connect the grep command to the git call by a pipe
>> +	# so that grep can kill the infinite recusion when detected.
>> +	# The recursion creates git output like:
>> +	# Fetching submodule sub
>> +	# Fetching submodule sub/sub              <-- [1]
>> +	# Fetching submodule sub/sub/sub
>> +	# ...
>> +	# [1] grep will trigger here and kill git by exiting and closing its stdin
> 
> "trigger here and kill..." -> "stop reading and cause git to
> eventually stop and die"
> 
> But we probably cannot use 'grep -m1' so it is a moot point.
> 
>> +
>> +	! git -C main fetch --recurse-submodules 2>&1 |
>> +		grep -v -m1 "Fetching submodule sub$" &&
> 
> Unfortunately, "grep -m<count>" is not even in POSIX, I would think.
> 
> What do we expect to happen in the correct case?

sigh, we can't use grep -m1.  Too bad, it was such a nice solution.

> 
>   - A line "Fetching submodule sub" and nothing else is given?  That
>     feels a bit brittle (how are we making sure, in the presence of
>     "2>&1", that we will not get any other output, like progress?)
> 
>   - "sub" is the only thing that appears on lines that begin with
>     "Fetching submodule" (i.e. "Fetching submodule $something" where
>     $something is not 'sub' is an error), and we allow other garbage
>     in the output?  That would be a bit more robust than the above.
> 
> As you seem to be comfortable using "sed" below, perhaps use it to
> extract the first few lines that say "^Fetching submodule " from the
> output and stop, and check that the output has only one such line
> about 'sub' and nothing else?

According to [1] posix sed offers equal possibility to quit like grep 
-m1 and I'll adopt:
$> yes posixgrepisnogoodforus | sed "/posix/q"

[1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html


Looking at the other mails I think I processed over all open comments 
and will prepare for v3.  Thanks again.

-- 
kind regards
--peter;

[PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kaestle]

A regression has been introduced by a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28).

The scenario in which it triggers is when one has a repository with a
submodule inside a submodule like this:
superproject/middle_repo/inner_repo

Person A and B have both a clone of it, while Person B is not working
with the inner_repo and thus does not have it initialized in his working
copy.

Now person A introduces a change to the inner_repo and propagates it
through the middle_repo and the superproject.

Once person A pushed the changes and person B wants to fetch them using
"git fetch" at the superproject level, B's git call will return with
error saying:

Could not access submodule 'inner_repo'
Errors during submodule fetch:
         middle_repo

Expectation is that in this case the inner submodule will be recognized
as uninitialized submodule and skipped by the git fetch command.

This used to work correctly before 'a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28)'.

Starting with a62387b the code wants to evaluate "is_empty_dir()" inside
.git/modules for a directory only existing in the worktree, delivering
then of course wrong return value.

This patch ensures is_empty_dir() is getting the correct path of the
uninitialized submodule by concatenation of the actual worktree and the
name of the uninitialized submodule.

Furthermore a regression test case is added, which tests for recursive
fetches on a superproject with uninitialized sub repositories.  This
issue was leading to an infinite loop when doing a revert of a62387b.

The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:
fix of regression on fetching of non-init subsub-repo, 2020-11-12), by
simply reverting a62387b, resulted in an infinite loop of submodule
fetches in the simpler case of a recursive fetch of a superproject with
uninitialized submodules, and so this commit was reverted in 7091499bc0
(Revert "submodules: fix of regression on fetching of non-init
subsub-repo", 2020-12-02).
To prevent future breakages, also add a regression test for this
scenario.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
CC: Junio C Hamano <gitster@pobox.com>
CC: Philippe Blain <levraiphilippeblain@gmail.com>
CC: Ralf Thielow <ralf.thielow@gmail.com>
CC: Philippe Blain <levraiphilippeblain@gmail.com>
---
 submodule.c                 |   7 ++-
 t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 130 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index b3bb59f..b561445 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,
 			strbuf_release(&submodule_prefix);
 			return 1;
 		} else {
+			struct strbuf empty_submodule_path = STRBUF_INIT;
 
 			fetch_task_release(task);
 			free(task);
@@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,
 			 * An empty directory is normal,
 			 * the submodule is not initialized
 			 */
+			strbuf_addf(&empty_submodule_path, "%s/%s/",
+							spf->r->worktree,
+							ce->name);
 			if (S_ISGITLINK(ce->ce_mode) &&
-			    !is_empty_dir(ce->name)) {
+			    !is_empty_dir(empty_submodule_path.buf)) {
 				spf->result = 1;
 				strbuf_addf(err,
 					    _("Could not access submodule '%s'\n"),
 					    ce->name);
 			}
+			strbuf_release(&empty_submodule_path);
 		}
 	}
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index dd8e423..495348a 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -719,4 +719,128 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
 	)
 '
 
+add_commit_push () {
+	dir="$1" &&
+	msg="$2" &&
+	shift 2 &&
+	git -C "$dir" add "$@" &&
+	git -C "$dir" commit -a -m "$msg" &&
+	git -C "$dir" push
+}
+
+compare_refs_in_dir () {
+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail='!' &&
+		shift
+	fi &&
+	git -C "$1" rev-parse --verify "$2" >expect &&
+	git -C "$3" rev-parse --verify "$4" >actual &&
+	eval $fail test_cmp expect actual
+}
+
+
+test_expect_success 'setup nested submodule fetch test' '
+	# does not depend on any previous test setups
+
+	for repo in outer middle inner
+	do
+		git init --bare $repo &&
+		git clone $repo ${repo}_content &&
+		echo "$repo" >"${repo}_content/file" &&
+		add_commit_push ${repo}_content "initial" file ||
+		return 1
+	done &&
+
+	git clone outer A &&
+	git -C A submodule add "$pwd/middle" &&
+	git -C A/middle/ submodule add "$pwd/inner" &&
+	add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
+	add_commit_push A/ "adding middle sub" .gitmodules middle &&
+
+	git clone outer B &&
+	git -C B/ submodule update --init middle &&
+
+	compare_refs_in_dir A HEAD B HEAD &&
+	compare_refs_in_dir A/middle HEAD B/middle HEAD &&
+	test_path_is_file B/file &&
+	test_path_is_file B/middle/file &&
+	test_path_is_missing B/middle/inner/file &&
+
+	echo "change on inner repo of A" >"A/middle/inner/file" &&
+	add_commit_push A/middle/inner "change on inner" file &&
+	add_commit_push A/middle "change on inner" inner &&
+	add_commit_push A "change on inner" middle
+'
+
+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
+	# depends on previous test for setup
+
+	git -C B/ fetch &&
+	compare_refs_in_dir A origin/HEAD B origin/HEAD
+'
+
+fetch_with_recusion_abort () {
+	# In a regression the following git call will run into infinite recursion.
+	# To handle that, we connect the sed command to the git call by a pipe
+	# so that sed can kill the infinite recusion when detected.
+	# The recursion creates git output like:
+	# Fetching submodule sub
+	# Fetching submodule sub/sub              <-- [1]
+	# Fetching submodule sub/sub/sub
+	# ...
+	# [1] sed will stop reading and cause git to eventually stop and die
+
+	git -C "$1" fetch --recurse-submodules 2>&1 |
+		sed "/Fetching submodule $2[^$]/q" >out &&
+	! grep "Fetching submodule $2[^$]" out
+}
+
+test_expect_success 'setup recursive fetch with uninit submodule' '
+	# does not depend on any previous test setups
+
+	# setup a remote superproject to make git fetch work with an uninit submodule
+	git init --bare super_bare &&
+	git clone super_bare super &&
+	git init sub &&
+
+	>sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+	git -C sub rev-parse HEAD >expect &&
+
+	# adding submodule without cloning
+	echo "[submodule \"sub\"]" >super/.gitmodules &&
+	echo "path = sub" >>super/.gitmodules &&
+	echo "url = ../sub" >>super/.gitmodules &&
+	git -C super update-index --add --cacheinfo 160000 $(cat expect) sub &&
+	mkdir super/sub &&
+
+	git -C super submodule status >out &&
+	sed -e "s/^-//" -e "s/ sub.*$//" out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'recursive fetch with uninit submodule' '
+	# depends on previous test for setup
+
+	fetch_with_recusion_abort super sub &&
+	git -C super submodule status >out &&
+	sed -e "s/^-//" -e "s/ sub$//" out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'recursive fetch after deinit a submodule' '
+	# depends on previous test for setup
+
+	git -C super submodule update --init sub &&
+	git -C super submodule deinit -f sub &&
+
+	fetch_with_recusion_abort super sub &&
+	git -C super submodule status >out &&
+	sed -e "s/^-//" -e "s/ sub$//" out >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.6.2

Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kästle]

On 08.12.20 16:42, Peter Kaestle wrote:
> A regression has been introduced by a62387b (submodule.c: fetch in
> submodules git directory instead of in worktree, 2018-11-28).
> 
> The scenario in which it triggers is when one has a repository with a
> submodule inside a submodule like this:
> superproject/middle_repo/inner_repo
> 
> Person A and B have both a clone of it, while Person B is not working
> with the inner_repo and thus does not have it initialized in his working
> copy.
> 
> Now person A introduces a change to the inner_repo and propagates it
> through the middle_repo and the superproject.
> 
> Once person A pushed the changes and person B wants to fetch them using
> "git fetch" at the superproject level, B's git call will return with
> error saying:
> 
> Could not access submodule 'inner_repo'
> Errors during submodule fetch:
>           middle_repo
> 
> Expectation is that in this case the inner submodule will be recognized
> as uninitialized submodule and skipped by the git fetch command.
> 
> This used to work correctly before 'a62387b (submodule.c: fetch in
> submodules git directory instead of in worktree, 2018-11-28)'.
> 
> Starting with a62387b the code wants to evaluate "is_empty_dir()" inside
> .git/modules for a directory only existing in the worktree, delivering
> then of course wrong return value.
> 
> This patch ensures is_empty_dir() is getting the correct path of the
> uninitialized submodule by concatenation of the actual worktree and the
> name of the uninitialized submodule.
> 
> Furthermore a regression test case is added, which tests for recursive
> fetches on a superproject with uninitialized sub repositories.  This
> issue was leading to an infinite loop when doing a revert of a62387b.
> 
> The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:
> fix of regression on fetching of non-init subsub-repo, 2020-11-12), by
> simply reverting a62387b, resulted in an infinite loop of submodule
> fetches in the simpler case of a recursive fetch of a superproject with
> uninitialized submodules, and so this commit was reverted in 7091499bc0
> (Revert "submodules: fix of regression on fetching of non-init
> subsub-repo", 2020-12-02).
> To prevent future breakages, also add a regression test for this
> scenario.
> 
> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
> CC: Junio C Hamano <gitster@pobox.com>
> CC: Philippe Blain <levraiphilippeblain@gmail.com>
> CC: Ralf Thielow <ralf.thielow@gmail.com>
> CC: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>   submodule.c                 |   7 ++-
>   t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 130 insertions(+), 1 deletion(-)
> 
> diff --git a/submodule.c b/submodule.c
> index b3bb59f..b561445 100644
> --- a/submodule.c
> +++ b/submodule.c

[...]


These are my test run outputs:
final_fix [PATCH v3]
==========
# passed all 41 test(s)


recursion_regression [1b7ac4e6d4]
==========
not ok 41 - recursive fetch after deinit a submodule


no_fix [3a0b884c - origin/master]
==========
not ok 38 - fetching a superproject containing an uninitialized sub/sub 
project


-- 
kind regards
--peter;

Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo

[Junio C Hamano]

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

> A regression has been introduced by a62387b (submodule.c: fetch in
> submodules git directory instead of in worktree, 2018-11-28).
>
> The scenario in which it triggers is when one has a repository with a
> submodule inside a submodule like this:
> superproject/middle_repo/inner_repo
>
> Person A and B have both a clone of it, while Person B is not working
> with the inner_repo and thus does not have it initialized in his working
> copy.
>
> Now person A introduces a change to the inner_repo and propagates it
> through the middle_repo and the superproject.
>
> Once person A pushed the changes and person B wants to fetch them using
> "git fetch" at the superproject level, B's git call will return with
> error saying:
>
> Could not access submodule 'inner_repo'
> Errors during submodule fetch:
>          middle_repo
>
> Expectation is that in this case the inner submodule will be recognized
> as uninitialized submodule and skipped by the git fetch command.
>
> This used to work correctly before 'a62387b (submodule.c: fetch in
> submodules git directory instead of in worktree, 2018-11-28)'.
>
> Starting with a62387b the code wants to evaluate "is_empty_dir()" inside
> .git/modules for a directory only existing in the worktree, delivering
> then of course wrong return value.
>
> This patch ensures is_empty_dir() is getting the correct path of the
> uninitialized submodule by concatenation of the actual worktree and the
> name of the uninitialized submodule.
>
> Furthermore a regression test case is added, which tests for recursive
> fetches on a superproject with uninitialized sub repositories.  This
> issue was leading to an infinite loop when doing a revert of a62387b.
>
> The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:
> fix of regression on fetching of non-init subsub-repo, 2020-11-12), by
> simply reverting a62387b, resulted in an infinite loop of submodule
> fetches in the simpler case of a recursive fetch of a superproject with
> uninitialized submodules, and so this commit was reverted in 7091499bc0
> (Revert "submodules: fix of regression on fetching of non-init
> subsub-repo", 2020-12-02).
> To prevent future breakages, also add a regression test for this
> scenario.
>
> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
> CC: Junio C Hamano <gitster@pobox.com>
> CC: Philippe Blain <levraiphilippeblain@gmail.com>
> CC: Ralf Thielow <ralf.thielow@gmail.com>
> CC: Philippe Blain <levraiphilippeblain@gmail.com>
> ---

Thanks, will replace.

Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo

[Philippe Blain]

Hi Peter,

Le mar. 8 déc. 2020, à 10 h 43, Peter Kaestle
<peter.kaestle@nokia.com> a écrit :
>
>  -- 8< --
>
> Furthermore a regression test case is added, which tests for recursive
> fetches on a superproject with uninitialized sub repositories.  This
> issue was leading to an infinite loop when doing a revert of a62387b.

I think this paragraph could be removed as it's saying the same thing as
the one below.

>
> The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:
> fix of regression on fetching of non-init subsub-repo, 2020-11-12), by
> simply reverting a62387b, resulted in an infinite loop of submodule
> fetches in the simpler case of a recursive fetch of a superproject with
> uninitialized submodules, and so this commit was reverted in 7091499bc0
> (Revert "submodules: fix of regression on fetching of non-init
> subsub-repo", 2020-12-02).
> To prevent future breakages, also add a regression test for this
> scenario.
>
> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
> CC: Junio C Hamano <gitster@pobox.com>
> CC: Philippe Blain <levraiphilippeblain@gmail.com>
> CC: Ralf Thielow <ralf.thielow@gmail.com>
> CC: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  submodule.c                 |   7 ++-
>  t/t5526-fetch-submodules.sh | 124 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 130 insertions(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index b3bb59f..b561445 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,
>                         strbuf_release(&submodule_prefix);
>                         return 1;
>                 } else {
> +                       struct strbuf empty_submodule_path = STRBUF_INIT;
>
>                         fetch_task_release(task);
>                         free(task);
> @@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,
>                          * An empty directory is normal,
>                          * the submodule is not initialized
>                          */
> +                       strbuf_addf(&empty_submodule_path, "%s/%s/",
> +                                                       spf->r->worktree,
> +                                                       ce->name);
>                         if (S_ISGITLINK(ce->ce_mode) &&
> -                           !is_empty_dir(ce->name)) {
> +                           !is_empty_dir(empty_submodule_path.buf)) {
>                                 spf->result = 1;
>                                 strbuf_addf(err,
>                                             _("Could not access submodule '%s'\n"),
>                                             ce->name);
>                         }
> +                       strbuf_release(&empty_submodule_path);
>                 }
>         }
>
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index dd8e423..495348a 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -719,4 +719,128 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
>         )
>  '
>
> +add_commit_push () {
> +       dir="$1" &&
> +       msg="$2" &&
> +       shift 2 &&
> +       git -C "$dir" add "$@" &&
> +       git -C "$dir" commit -a -m "$msg" &&
> +       git -C "$dir" push
> +}
> +
> +compare_refs_in_dir () {
> +       fail= &&
> +       if test "x$1" = 'x!'
> +       then
> +               fail='!' &&
> +               shift
> +       fi &&
> +       git -C "$1" rev-parse --verify "$2" >expect &&
> +       git -C "$3" rev-parse --verify "$4" >actual &&
> +       eval $fail test_cmp expect actual
> +}
> +
> +
> +test_expect_success 'setup nested submodule fetch test' '
> +       # does not depend on any previous test setups
> +
> +       for repo in outer middle inner
> +       do
> +               git init --bare $repo &&
> +               git clone $repo ${repo}_content &&
> +               echo "$repo" >"${repo}_content/file" &&
> +               add_commit_push ${repo}_content "initial" file ||
> +               return 1
> +       done &&
> +
> +       git clone outer A &&
> +       git -C A submodule add "$pwd/middle" &&
> +       git -C A/middle/ submodule add "$pwd/inner" &&
> +       add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
> +       add_commit_push A/ "adding middle sub" .gitmodules middle &&
> +
> +       git clone outer B &&
> +       git -C B/ submodule update --init middle &&
> +
> +       compare_refs_in_dir A HEAD B HEAD &&
> +       compare_refs_in_dir A/middle HEAD B/middle HEAD &&
> +       test_path_is_file B/file &&
> +       test_path_is_file B/middle/file &&
> +       test_path_is_missing B/middle/inner/file &&
> +
> +       echo "change on inner repo of A" >"A/middle/inner/file" &&
> +       add_commit_push A/middle/inner "change on inner" file &&
> +       add_commit_push A/middle "change on inner" inner &&
> +       add_commit_push A "change on inner" middle
> +'
> +
> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
> +       # depends on previous test for setup
> +
> +       git -C B/ fetch &&
> +       compare_refs_in_dir A origin/HEAD B origin/HEAD
> +'
> +
> +fetch_with_recusion_abort () {

s/recusion/recursion/

> +       # In a regression the following git call will run into infinite recursion.
> +       # To handle that, we connect the sed command to the git call by a pipe
> +       # so that sed can kill the infinite recusion when detected.

s/recusion/recursion/

> +       # The recursion creates git output like:
> +       # Fetching submodule sub
> +       # Fetching submodule sub/sub              <-- [1]
> +       # Fetching submodule sub/sub/sub
> +       # ...
> +       # [1] sed will stop reading and cause git to eventually stop and die
> +
> +       git -C "$1" fetch --recurse-submodules 2>&1 |
> +               sed "/Fetching submodule $2[^$]/q" >out &&
> +       ! grep "Fetching submodule $2[^$]" out
> +}
> +
> +test_expect_success 'setup recursive fetch with uninit submodule' '
> +       # does not depend on any previous test setups
> +
> +       # setup a remote superproject to make git fetch work with an uninit submodule
> +       git init --bare super_bare &&
> +       git clone super_bare super &&
> +       git init sub &&
> +
> +       >sub/file &&
> +       git -C sub add file &&
> +       git -C sub commit -m "add file" &&
> +       git -C sub rev-parse HEAD >expect &&
> +
> +       # adding submodule without cloning
> +       echo "[submodule \"sub\"]" >super/.gitmodules &&
> +       echo "path = sub" >>super/.gitmodules &&
> +       echo "url = ../sub" >>super/.gitmodules &&
> +       git -C super update-index --add --cacheinfo 160000 $(cat expect) sub &&
> +       mkdir super/sub &&
> +
> +       git -C super submodule status >out &&
> +       sed -e "s/^-//" -e "s/ sub.*$//" out >actual &&
> +       test_cmp expect actual
> +'

I think this is overly complicated, what I was hinting at was adding
the submodule
in the superproject before cloning it, so something along these lines:

test_create_repo super &&
test_commit -C super initial &&
test_create_repo sub &&
test_commit -C sub initial &&
git -C sub rev-parse HEAD >expect &&

git -C super submodule add ../sub &&
git -C super commit -m "add sub" &&

git clone super superclone &&
git -C superclone submodule status >out &&
sed -e "s/^-//" -e "s/ sub.*$//" out >actual &&
test_cmp expect actual

And then running the two tests below in "superclone".


> +
> +test_expect_success 'recursive fetch with uninit submodule' '
> +       # depends on previous test for setup
> +
> +       fetch_with_recusion_abort super sub &&

s/recusion/recursion/

> +       git -C super submodule status >out &&
> +       sed -e "s/^-//" -e "s/ sub$//" out >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'recursive fetch after deinit a submodule' '
> +       # depends on previous test for setup
> +
> +       git -C super submodule update --init sub &&
> +       git -C super submodule deinit -f sub &&
> +
> +       fetch_with_recusion_abort super sub &&

s/recusion/recursion/

> +       git -C super submodule status >out &&
> +       sed -e "s/^-//" -e "s/ sub$//" out >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.6.2
>

Philippe.

Re: [PATCH v3] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kästle]

Hi Philippe,

when sending the patch yesterday, I had the gut feeling to hold it back 
to double check it a day later.  Should have done so, some of your 
findings were really stupid mistakes.


On 09.12.20 00:25, Philippe Blain wrote:
> Le mar. 8 déc. 2020, à 10 h 43, Peter Kaestle
> <peter.kaestle@nokia.com> a écrit :
>>
>>   -- 8< --
>>
>> Furthermore a regression test case is added, which tests for recursive
>> fetches on a superproject with uninitialized sub repositories.  This
>> issue was leading to an infinite loop when doing a revert of a62387b.
> 
> I think this paragraph could be removed as it's saying the same thing as
> the one below.

jip.

[...]

>> +fetch_with_recusion_abort () {
> 
> s/recusion/recursion/
> 

yes.

[...]

>> +test_expect_success 'setup recursive fetch with uninit submodule' '
>> +       # does not depend on any previous test setups
>> +
>> +       # setup a remote superproject to make git fetch work with an uninit submodule
>> +       git init --bare super_bare &&
>> +       git clone super_bare super &&
>> +       git init sub &&
>> +
>> +       >sub/file &&
>> +       git -C sub add file &&
>> +       git -C sub commit -m "add file" &&
>> +       git -C sub rev-parse HEAD >expect &&
>> +
>> +       # adding submodule without cloning
>> +       echo "[submodule \"sub\"]" >super/.gitmodules &&
>> +       echo "path = sub" >>super/.gitmodules &&
>> +       echo "url = ../sub" >>super/.gitmodules &&
>> +       git -C super update-index --add --cacheinfo 160000 $(cat expect) sub &&
>> +       mkdir super/sub &&
>> +
>> +       git -C super submodule status >out &&
>> +       sed -e "s/^-//" -e "s/ sub.*$//" out >actual &&
>> +       test_cmp expect actual
>> +'
> 
> I think this is overly complicated, what I was hinting at was adding
> the submodule
> in the superproject before cloning it, so something along these lines:
> 
> test_create_repo super &&
> test_commit -C super initial &&
> test_create_repo sub &&
> test_commit -C sub initial &&
> git -C sub rev-parse HEAD >expect &&
> 
> git -C super submodule add ../sub &&
> git -C super commit -m "add sub" &&
> 
> git clone super superclone &&
> git -C superclone submodule status >out &&
> sed -e "s/^-//" -e "s/ sub.*$//" out >actual &&
> test_cmp expect actual
> 
> And then running the two tests below in "superclone".

Indeed, looks much simpler, I gave it a try run and it resulted in same 
behavior, will double check the logic of it and then send v4.  Thanks a lot.

[...]


-- 
kind regards
--peter;

[PATCH v4] submodules: fix of regression on fetching of non-init subsub-repo

[Peter Kaestle]

A regression has been introduced by a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28).

The scenario in which it triggers is when one has a repository with a
submodule inside a submodule like this:
superproject/middle_repo/inner_repo

Person A and B have both a clone of it, while Person B is not working
with the inner_repo and thus does not have it initialized in his working
copy.

Now person A introduces a change to the inner_repo and propagates it
through the middle_repo and the superproject.

Once person A pushed the changes and person B wants to fetch them using
"git fetch" at the superproject level, B's git call will return with
error saying:

Could not access submodule 'inner_repo'
Errors during submodule fetch:
         middle_repo

Expectation is that in this case the inner submodule will be recognized
as uninitialized submodule and skipped by the git fetch command.

This used to work correctly before 'a62387b (submodule.c: fetch in
submodules git directory instead of in worktree, 2018-11-28)'.

Starting with a62387b the code wants to evaluate "is_empty_dir()" inside
.git/modules for a directory only existing in the worktree, delivering
then of course wrong return value.

This patch ensures is_empty_dir() is getting the correct path of the
uninitialized submodule by concatenation of the actual worktree and the
name of the uninitialized submodule.

The first attempt to fix this regression, in 1b7ac4e6d4 (submodules:
fix of regression on fetching of non-init subsub-repo, 2020-11-12), by
simply reverting a62387b, resulted in an infinite loop of submodule
fetches in the simpler case of a recursive fetch of a superproject with
uninitialized submodules, and so this commit was reverted in 7091499bc0
(Revert "submodules: fix of regression on fetching of non-init
subsub-repo", 2020-12-02).
To prevent future breakages, also add a regression test for this
scenario.

Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
CC: Junio C Hamano <gitster@pobox.com>
CC: Philippe Blain <levraiphilippeblain@gmail.com>
CC: Ralf Thielow <ralf.thielow@gmail.com>
CC: Eric Sunshine <sunshine@sunshineco.us>
---
 submodule.c                 |   7 ++-
 t/t5526-fetch-submodules.sh | 117 ++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index b3bb59f066..b561445329 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1477,6 +1477,7 @@ static int get_next_submodule(struct child_process *cp,
 			strbuf_release(&submodule_prefix);
 			return 1;
 		} else {
+			struct strbuf empty_submodule_path = STRBUF_INIT;
 
 			fetch_task_release(task);
 			free(task);
@@ -1485,13 +1486,17 @@ static int get_next_submodule(struct child_process *cp,
 			 * An empty directory is normal,
 			 * the submodule is not initialized
 			 */
+			strbuf_addf(&empty_submodule_path, "%s/%s/",
+							spf->r->worktree,
+							ce->name);
 			if (S_ISGITLINK(ce->ce_mode) &&
-			    !is_empty_dir(ce->name)) {
+			    !is_empty_dir(empty_submodule_path.buf)) {
 				spf->result = 1;
 				strbuf_addf(err,
 					    _("Could not access submodule '%s'\n"),
 					    ce->name);
 			}
+			strbuf_release(&empty_submodule_path);
 		}
 	}
 
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index dd8e423d25..c42ece1f04 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -719,4 +719,121 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
 	)
 '
 
+add_commit_push () {
+	dir="$1" &&
+	msg="$2" &&
+	shift 2 &&
+	git -C "$dir" add "$@" &&
+	git -C "$dir" commit -a -m "$msg" &&
+	git -C "$dir" push
+}
+
+compare_refs_in_dir () {
+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail='!' &&
+		shift
+	fi &&
+	git -C "$1" rev-parse --verify "$2" >expect &&
+	git -C "$3" rev-parse --verify "$4" >actual &&
+	eval $fail test_cmp expect actual
+}
+
+
+test_expect_success 'setup nested submodule fetch test' '
+	# does not depend on any previous test setups
+
+	for repo in outer middle inner
+	do
+		git init --bare $repo &&
+		git clone $repo ${repo}_content &&
+		echo "$repo" >"${repo}_content/file" &&
+		add_commit_push ${repo}_content "initial" file ||
+		return 1
+	done &&
+
+	git clone outer A &&
+	git -C A submodule add "$pwd/middle" &&
+	git -C A/middle/ submodule add "$pwd/inner" &&
+	add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
+	add_commit_push A/ "adding middle sub" .gitmodules middle &&
+
+	git clone outer B &&
+	git -C B/ submodule update --init middle &&
+
+	compare_refs_in_dir A HEAD B HEAD &&
+	compare_refs_in_dir A/middle HEAD B/middle HEAD &&
+	test_path_is_file B/file &&
+	test_path_is_file B/middle/file &&
+	test_path_is_missing B/middle/inner/file &&
+
+	echo "change on inner repo of A" >"A/middle/inner/file" &&
+	add_commit_push A/middle/inner "change on inner" file &&
+	add_commit_push A/middle "change on inner" inner &&
+	add_commit_push A "change on inner" middle
+'
+
+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '
+	# depends on previous test for setup
+
+	git -C B/ fetch &&
+	compare_refs_in_dir A origin/HEAD B origin/HEAD
+'
+
+fetch_with_recursion_abort () {
+	# In a regression the following git call will run into infinite recursion.
+	# To handle that, we connect the sed command to the git call by a pipe
+	# so that sed can kill the infinite recursion when detected.
+	# The recursion creates git output like:
+	# Fetching submodule sub
+	# Fetching submodule sub/sub              <-- [1]
+	# Fetching submodule sub/sub/sub
+	# ...
+	# [1] sed will stop reading and cause git to eventually stop and die
+
+	git -C "$1" fetch --recurse-submodules 2>&1 |
+		sed "/Fetching submodule $2[^$]/q" >out &&
+	! grep "Fetching submodule $2[^$]" out
+}
+
+test_expect_success 'setup recursive fetch with uninit submodule' '
+	# does not depend on any previous test setups
+
+	test_create_repo super &&
+	test_commit -C super initial &&
+	test_create_repo sub &&
+	test_commit -C sub initial &&
+	git -C sub rev-parse HEAD >expect &&
+
+	git -C super submodule add ../sub &&
+	git -C super commit -m "add sub" &&
+
+	git clone super superclone &&
+	git -C superclone submodule status >out &&
+	sed -e "s/^-//" -e "s/ sub.*$//" out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'recursive fetch with uninit submodule' '
+	# depends on previous test for setup
+
+	fetch_with_recursion_abort superclone sub &&
+	git -C superclone submodule status >out &&
+	sed -e "s/^-//" -e "s/ sub$//" out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'recursive fetch after deinit a submodule' '
+	# depends on previous test for setup
+
+	git -C superclone submodule update --init sub &&
+	git -C superclone submodule deinit -f sub &&
+
+	fetch_with_recursion_abort superclone sub &&
+	git -C superclone submodule status >out &&
+	sed -e "s/^-//" -e "s/ sub$//" out >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.29.2.457.gf179bca

Re: [PATCH v4] submodules: fix of regression on fetching of non-init subsub-repo

[Philippe Blain]

Le mer. 9 déc. 2020, à 05 h 58, Peter Kaestle
<peter.kaestle@nokia.com> a écrit :
>
> ---8<---
>

Reviewed-by: Philippe Blain <levraiphilippeblain@gmail.com>

Thanks again for working on this.

Philippe.
P.S. I wouldn't call anything you wrote in v3 "stupid mistakes" :) we
all have a lot on our minds
- especially these days!

Re: BUG in fetching non-checked out submodule

[Ralf Thielow]

Hi Philippe,

Am Mi., 2. Dez. 2020 um 18:20 Uhr schrieb Philippe Blain
<levraiphilippeblain@gmail.com>:
>
> Ok, you don't add '--init' but the submodule gets checked out,
> so it looks like you have 'submodule.active' set to a pathspec
> that matches 'sha1collisiondetection'. Did you clone the git repo
> with '--recurse-submodules', which would add '.' as the value of
> 'submodule.active' ? Or maybe you manually configured that value
> in your global gitconfig ?

Sry, I did not mention that I started with the submodule already
initialized and checked out. IIRC I deinitialized the submodule with
'git submodule deinit -f sha1collisiondetection/' and ran 'git fetch'
to see if it happens on the git repo, too, since I encountered the
issue on another repo first.

I can see the same misbehaviour when I run
$ git -c submodule.recurse=false fetch --recurse-submodules
too, so I think it doesn't have necessarily something to do with
the 'submodule.recursive' config, but with 'recurse submodules'
in general.

Ralf

Re: BUG in fetching non-checked out submodule

[Peter Kästle]

Hi Ralf,

On 03.12.20 08:45, Ralf Thielow wrote:
> Am Mi., 2. Dez. 2020 um 18:20 Uhr schrieb Philippe Blain
> <levraiphilippeblain@gmail.com>:
>>
>> Ok, you don't add '--init' but the submodule gets checked out,
>> so it looks like you have 'submodule.active' set to a pathspec
>> that matches 'sha1collisiondetection'. Did you clone the git repo
>> with '--recurse-submodules', which would add '.' as the value of
>> 'submodule.active' ? Or maybe you manually configured that value
>> in your global gitconfig ?
> 
> Sry, I did not mention that I started with the submodule already
> initialized and checked out. IIRC I deinitialized the submodule with
> 'git submodule deinit -f sha1collisiondetection/' and ran 'git fetch'
> to see if it happens on the git repo, too, since I encountered the
> issue on another repo first.
> 
> I can see the same misbehaviour when I run
> $ git -c submodule.recurse=false fetch --recurse-submodules
> too, so I think it doesn't have necessarily something to do with
> the 'submodule.recursive' config, but with 'recurse submodules'
> in general.
This is interesting.  There are several testcases using 
"--recurse-submodules".  Have you tried, whether any of these trigger?

-- 
kind regards
--peter;

Re: BUG in fetching non-checked out submodule

[Ralf Thielow]

Hi Peter,

Am Do., 3. Dez. 2020 um 09:20 Uhr schrieb Peter Kästle
<peter.kaestle@nokia.com>:
> > I can see the same misbehaviour when I run
> > $ git -c submodule.recurse=false fetch --recurse-submodules
> > too, so I think it doesn't have necessarily something to do with
> > the 'submodule.recursive' config, but with 'recurse submodules'
> > in general.
> This is interesting.  There are several testcases using
> "--recurse-submodules".  Have you tried, whether any of these trigger?
>

The test suite passes.

Ralf

Re: BUG in fetching non-checked out submodule

[Peter Kästle]

Hi Ralf,
On 03.12.20 10:38, Ralf Thielow wrote:
> Hi Peter,
> 
> Am Do., 3. Dez. 2020 um 09:20 Uhr schrieb Peter Kästle
> <peter.kaestle@nokia.com>:
>>> I can see the same misbehaviour when I run
>>> $ git -c submodule.recurse=false fetch --recurse-submodules
>>> too, so I think it doesn't have necessarily something to do with
>>> the 'submodule.recursive' config, but with 'recurse submodules'
>>> in general.
>> This is interesting.  There are several testcases using
>> "--recurse-submodules".  Have you tried, whether any of these trigger?
>>
> 
> The test suite passes.

This is what I experienced.  Thus it is important to get your case into 
the test suite.  Could you please send us a command sequence how to 
create your repository structure from scratch, so that a test case can 
be created?  - Of course, if you want, you can also create a test case 
on your own.

-- 
kind regards
--peter;

Re: BUG in fetching non-checked out submodule

[Ralf Thielow]

Hi Peter,

Am Do., 3. Dez. 2020 um 10:43 Uhr schrieb Peter Kästle
<peter.kaestle@nokia.com>:
> >
> > The test suite passes.
>
> This is what I experienced.  Thus it is important to get your case into
> the test suite.  Could you please send us a command sequence how to
> create your repository structure from scratch, so that a test case can
> be created?  - Of course, if you want, you can also create a test case
> on your own.
>

It can be reproduced with the following sequence of commands:

git init sub
cd sub
touch file
git add file
git commit -m "add file"
cd ..
git init main
cd main
git submodule add ../sub
git submodule init
git submodule update --checkout
git submodule deinit -f sub/
git fetch --recurse-submodules

Ralf

Re: BUG in fetching non-checked out submodule

[Peter Kästle]

Hi Ralf,

On 03.12.20 13:30, Ralf Thielow wrote:
> It can be reproduced with the following sequence of commands:
> 
> git init sub
> cd sub
> touch file
> git add file
> git commit -m "add file"
> cd ..
> git init main
> cd main
> git submodule add ../sub
> git submodule init
> git submodule update --checkout
> git submodule deinit -f sub/
> git fetch --recurse-submodules

With git from master state the "git fetch --recurse-submodules" results 
in an infinite recurse call.

I translated your sequence into a bash script, which can then be easily 
converted into a test case for git.  Problematic was the infinite 
recurse loop of the git fetch command, which I solved by grep'ing for 
the second recursion output and abort using -m1.
Could you please confirm, that you see "passed" for the good git 
versions and "failed" for the bad ones?


#!/bin/bash

testcase () {
         rm -Rf main sub &&

         git init main &&
         git init sub &&

         touch sub/file &&
         git -C sub add file &&
         git -C sub commit -m "add file" &&
         git -C sub rev-parse HEAD >expect &&

         git -C main submodule add ../sub &&
         git -C main submodule init &&
         git -C main submodule update --checkout &&

         git -C main submodule deinit -f sub &&
         ! git -C main fetch --recurse-submodules |&
                 grep -v -m1 "Fetching submodule sub$" &&
         git -C main submodule status |
                 sed -e "s/^-//" -e "s/ sub$//" >actual &&
         cmp expect actual
}

if testcase
then
         echo "passed"
else
         echo "failed"
fi


-- 
--peter;

Re: BUG in fetching non-checked out submodule

[Ralf Thielow]

Hi Peter,

Am Do., 3. Dez. 2020 um 16:10 Uhr schrieb Peter Kästle
<peter.kaestle@nokia.com>:
>
> I translated your sequence into a bash script, which can then be easily
> converted into a test case for git.  Problematic was the infinite
> recurse loop of the git fetch command, which I solved by grep'ing for
> the second recursion output and abort using -m1.
> Could you please confirm, that you see "passed" for the good git
> versions and "failed" for the bad ones?
>
>

I can confirm that.  It "failed" on current master version
2.29.2.435.g72ffeb997e
and "passed" on the latest tag 2.29.2.

Thanks,
Ralf