BUG: submodule code prints '(null)'

BUG: submodule code prints '(null)'

[Duy Nguyen]

I do not know how to reproduce this (and didn't bother to look deeply
into it after I found it was not a trivial fix) but one of my "git
fetch" showed

warning: Submodule in commit be2db96a6c506464525f588da59cade0cedddb5e
at path: '(null)' collides with a submodule named the same. Skipping
it.

I think it's reported that some libc implementation will not be able
to gracefully handle NULL strings like glibc and may crash instead of
printing '(null)' here. I'll leave it to submodule people to fix this
:)
-- 
Duy

Re: BUG: submodule code prints '(null)'

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 1242 bytes --]

On Tuesday 05 June 2018 09:01 PM, Duy Nguyen wrote:
> I'll leave it to submodule people to fix this :)
> 

I'm Ccing the only one I know to gain attention.


-- 
Sivaraam

QUOTE:

“The three principal virtues of a programmer are Laziness, Impatience,
and Hubris.”

	- Camel book

Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: BUG: submodule code prints '(null)'

[Duy Nguyen]

On Tue, Jun 05, 2018 at 05:31:41PM +0200, Duy Nguyen wrote:
> I do not know how to reproduce this (and didn't bother to look deeply
> into it after I found it was not a trivial fix) but one of my "git
> fetch" showed
> 
> warning: Submodule in commit be2db96a6c506464525f588da59cade0cedddb5e
> at path: '(null)' collides with a submodule named the same. Skipping
> it.

The problem is default_name_or_path() can return NULL when a submodule
is not populated. The fix could simply be printing path instead of
name (because we are talking about path in the commit message), like
below.

But I don't really understand c68f837576 (implement fetching of moved
submodules - 2017-10-16), the commit that made this change, and not
sure if we should be reporting name here or path. Heiko?

diff --git a/submodule.c b/submodule.c
index 939d6870ec..61c2177755 100644
--- a/submodule.c
+++ b/submodule.c
@@ -745,7 +745,7 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,
 				warning("Submodule in commit %s at path: "
 					"'%s' collides with a submodule named "
 					"the same. Skipping it.",
-					oid_to_hex(commit_oid), name);
+					oid_to_hex(commit_oid), p->two->path);
 				name = NULL;
 			}
 		}



> 
> I think it's reported that some libc implementation will not be able
> to gracefully handle NULL strings like glibc and may crash instead of
> printing '(null)' here. I'll leave it to submodule people to fix this
> :)
> -- 
> Duy

Re: BUG: submodule code prints '(null)'

[Stefan Beller]

On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Tue, Jun 05, 2018 at 05:31:41PM +0200, Duy Nguyen wrote:
> > I do not know how to reproduce this (and didn't bother to look deeply
> > into it after I found it was not a trivial fix) but one of my "git
> > fetch" showed
> >
> > warning: Submodule in commit be2db96a6c506464525f588da59cade0cedddb5e
> > at path: '(null)' collides with a submodule named the same. Skipping
> > it.
>
> The problem is default_name_or_path() can return NULL when a submodule
> is not populated. The fix could simply be printing path instead of
> name (because we are talking about path in the commit message), like
> below.
>
> But I don't really understand c68f837576 (implement fetching of moved
> submodules - 2017-10-16), the commit that made this change, and not
> sure if we should be reporting name here or path. Heiko?
>
> diff --git a/submodule.c b/submodule.c
> index 939d6870ec..61c2177755 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -745,7 +745,7 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,

[Not in the context of this patch, but in the code right before the
context starts:]

            name = default_name_or_path(p->two->path);
            /* make sure name does not collide with existing one */
            submodule = submodule_from_name(the_repository, commit_oid, name);
            if (submodule) {

Currently I see 4 callers of default_name_or_path and the other 3 except this
one have checks against NULL in place, which is good.
However I think we have to guard this even more, and have to check
for !name before we call submodule_from_name.

It is technically ok to call submodule_from_name with a NULL name,
but it is semantically broken, see the comment in config_from that
is called from submodule_from_name:

    /*
     * If any parameter except the cache is a NULL pointer just
     * return the first submodule. Can be used to check whether
     * there are any submodules parsed.
     */
    if (!treeish_name || !key) {
        ...


>                                 warning("Submodule in commit %s at path: "
>                                         "'%s' collides with a submodule named "
>                                         "the same. Skipping it.",
> -                                       oid_to_hex(commit_oid), name);
> +                                       oid_to_hex(commit_oid), p->two->path);

This is correct for the error message, both in terms of not crashing as well
as correctness, we really need to report a *path* here and no the name_or_path,
which  default_name_or_path gives.

Re: BUG: submodule code prints '(null)'

[Stefan Beller]

On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Tue, Jun 05, 2018 at 05:31:41PM +0200, Duy Nguyen wrote:
> > I do not know how to reproduce this (and didn't bother to look deeply
> > into it after I found it was not a trivial fix) but one of my "git
> > fetch" showed
> >
> > warning: Submodule in commit be2db96a6c506464525f588da59cade0cedddb5e
> > at path: '(null)' collides with a submodule named the same. Skipping
> > it.
>
> The problem is default_name_or_path() can return NULL when a submodule
> is not populated. The fix could simply be printing path instead of
> name (because we are talking about path in the commit message), like
> below.
>
> But I don't really understand c68f837576 (implement fetching of moved
> submodules - 2017-10-16), the commit that made this change, and not
> sure if we should be reporting name here or path. Heiko?

That change is quite interesting as I did not understand it at first
sight as well.
See https://public-inbox.org/git/20171016135827.GC12756@book.hvoigt.net/
and the follow ups, specifically
https://public-inbox.org/git/20171019181109.27792-2-sbeller@google.com/
that tries to clean up the code, but was ultimately dropped.

Re: BUG: submodule code prints '(null)'

[Heiko Voigt]

On Mon, Jun 11, 2018 at 03:56:16PM -0700, Stefan Beller wrote:
> On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Tue, Jun 05, 2018 at 05:31:41PM +0200, Duy Nguyen wrote:
> > > I do not know how to reproduce this (and didn't bother to look deeply
> > > into it after I found it was not a trivial fix) but one of my "git
> > > fetch" showed
> > >
> > > warning: Submodule in commit be2db96a6c506464525f588da59cade0cedddb5e
> > > at path: '(null)' collides with a submodule named the same. Skipping
> > > it.
> >
> > The problem is default_name_or_path() can return NULL when a submodule
> > is not populated. The fix could simply be printing path instead of
> > name (because we are talking about path in the commit message), like
> > below.
> >
> > But I don't really understand c68f837576 (implement fetching of moved
> > submodules - 2017-10-16), the commit that made this change, and not
> > sure if we should be reporting name here or path. Heiko?
> >
> > diff --git a/submodule.c b/submodule.c
> > index 939d6870ec..61c2177755 100644
> > --- a/submodule.c
> > +++ b/submodule.c
> > @@ -745,7 +745,7 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,
> 
> [Not in the context of this patch, but in the code right before the
> context starts:]
> 
>             name = default_name_or_path(p->two->path);
>             /* make sure name does not collide with existing one */
>             submodule = submodule_from_name(the_repository, commit_oid, name);
>             if (submodule) {
> 
> Currently I see 4 callers of default_name_or_path and the other 3 except this
> one have checks against NULL in place, which is good.
> However I think we have to guard this even more, and have to check
> for !name before we call submodule_from_name.
> 
> It is technically ok to call submodule_from_name with a NULL name,
> but it is semantically broken, see the comment in config_from that
> is called from submodule_from_name:
> 
>     /*
>      * If any parameter except the cache is a NULL pointer just
>      * return the first submodule. Can be used to check whether
>      * there are any submodules parsed.
>      */
>     if (!treeish_name || !key) {
>         ...
> 
> 
> >                                 warning("Submodule in commit %s at path: "
> >                                         "'%s' collides with a submodule named "
> >                                         "the same. Skipping it.",
> > -                                       oid_to_hex(commit_oid), name);
> > +                                       oid_to_hex(commit_oid), p->two->path);
> 
> This is correct for the error message, both in terms of not crashing as well
> as correctness, we really need to report a *path* here and no the name_or_path,
> which  default_name_or_path gives.

Sorry for the late reply. I agree with Stefan, this change is correct,
since we are talking about the path in the warning message anyway. It
seems to me that this resulted from the name being the same as the path
in this location here.

I think we should report both here, if we can, the path and the name.

We are skipping the submodule if we can not get a name later:

                if (!name)     
                        continue;

I also agree, that it does not make sense to call submodule_from_name
with a NULL name. I think we should simply skip calling it in case the
name is NULL and then let the code later handle it.

E.g.: 

...
                        /* make sure name does not collide with existing one */
		        if (name)	
                            submodule = submodule_from_name(the_repository, commit_oid, name);
                        if (submodule) {
                                warning("Submodule in commit %s at path: "
...

Would you want to update your patch? Or should I put one on top?

Cheers Heiko

Re: BUG: submodule code prints '(null)'

[Duy Nguyen]

On Thu, Jun 14, 2018 at 5:15 PM Heiko Voigt <hvoigt@hvoigt.net> wrote:
> ...
> Would you want to update your patch? Or should I put one on top?

I think it's better that you make a proper patch. You can provide
explanation and all. I am more like a bug reporter :)
-- 
Duy

[PATCH] submodule: fix NULL correctness in renamed broken submodules

[Stefan Beller]

When fetching with recursing into submodules, the fetch logic inspects
the superproject which submodules actually need to be fetched. This is
tricky for submodules that were renamed in the fetched range of commits.
This was implemented in c68f8375760 (implement fetching of moved
submodules, 2017-10-16), and this patch fixes a mistake in the logic
there.

When the warning is printed, the `name` might be NULL as
default_name_or_path can return NULL, so fix the warning to use the path
as obtained from the diff machinery, as that is not NULL.

While at it, make sure we only attempt to load the submodule if a git
directory of the submodule is found as default_name_or_path will return
NULL in case the git directory cannot be found. Note that passing NULL
to submodule_from_name is just a semantic error, as submodule_from_name
accepts NULL as a value, but then the return value is not the submodule
that was asked for, but some arbitrary other submodule. (Cf. 'config_from'
in submodule-config.c: "If any parameter except the cache is a NULL
pointer just return the first submodule. Can be used to check whether
there are any submodules parsed.")

Reported-by: Duy Nguyen <pclouds@gmail.com>
Helped-by: Duy Nguyen <pclouds@gmail.com>
Helped-by: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 939d6870ecd..0998ea23458 100644
--- a/submodule.c
+++ b/submodule.c
@@ -740,12 +740,14 @@ static void collect_changed_submodules_cb(struct diff_queue_struct *q,
 		else {
 			name = default_name_or_path(p->two->path);
 			/* make sure name does not collide with existing one */
-			submodule = submodule_from_name(the_repository, commit_oid, name);
+			if (name)
+				submodule = submodule_from_name(the_repository,
+								commit_oid, name);
 			if (submodule) {
 				warning("Submodule in commit %s at path: "
 					"'%s' collides with a submodule named "
 					"the same. Skipping it.",
-					oid_to_hex(commit_oid), name);
+					oid_to_hex(commit_oid), p->two->path);
 				name = NULL;
 			}
 		}
-- 
2.18.0.rc1.244.gcf134e6275-goog

[PATCH] t5526: test recursive submodules when fetching moved submodules

[Stefan Beller]

The topic merged in 0c7ecb7c311 (Merge branch 'sb/submodule-move-nested',
2018-05-08) provided support for moving nested submodules.

Remove the NEEDSWORK comment and implement the nested submodules test as
the comment hinted at.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

I found this when digging around for the previous patch.

Thanks,
Stefan

 t/t5526-fetch-submodules.sh | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 9cc4b569c05..359e03ff836 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -574,11 +574,7 @@ test_expect_success "fetch new commits when submodule got renamed" '
 	git clone . downstream_rename &&
 	(
 		cd downstream_rename &&
-		git submodule update --init &&
-# NEEDSWORK: we omitted --recursive for the submodule update here since
-# that does not work. See test 7001 for mv "moving nested submodules"
-# for details. Once that is fixed we should add the --recursive option
-# here.
+		git submodule update --init --recursive &&
 		git checkout -b rename &&
 		git mv submodule submodule_renamed &&
 		(
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH] t5526: test recursive submodules when fetching moved submodules

[Heiko Voigt]

On Thu, Jun 14, 2018 at 10:37:30AM -0700, Stefan Beller wrote:
> The topic merged in 0c7ecb7c311 (Merge branch 'sb/submodule-move-nested',
> 2018-05-08) provided support for moving nested submodules.
> 
> Remove the NEEDSWORK comment and implement the nested submodules test as
> the comment hinted at.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Looks good to me.

Cheers Heiko

Re: [PATCH] submodule: fix NULL correctness in renamed broken submodules

[Heiko Voigt]

Hi,

On Thu, Jun 14, 2018 at 10:31:07AM -0700, Stefan Beller wrote:
> When fetching with recursing into submodules, the fetch logic inspects
> the superproject which submodules actually need to be fetched. This is
> tricky for submodules that were renamed in the fetched range of commits.
> This was implemented in c68f8375760 (implement fetching of moved
> submodules, 2017-10-16), and this patch fixes a mistake in the logic
> there.
> 
> When the warning is printed, the `name` might be NULL as
> default_name_or_path can return NULL, so fix the warning to use the path
> as obtained from the diff machinery, as that is not NULL.
> 
> While at it, make sure we only attempt to load the submodule if a git
> directory of the submodule is found as default_name_or_path will return
> NULL in case the git directory cannot be found. Note that passing NULL
> to submodule_from_name is just a semantic error, as submodule_from_name
> accepts NULL as a value, but then the return value is not the submodule
> that was asked for, but some arbitrary other submodule. (Cf. 'config_from'
> in submodule-config.c: "If any parameter except the cache is a NULL
> pointer just return the first submodule. Can be used to check whether
> there are any submodules parsed.")
> 
> Reported-by: Duy Nguyen <pclouds@gmail.com>
> Helped-by: Duy Nguyen <pclouds@gmail.com>
> Helped-by: Heiko Voigt <hvoigt@hvoigt.net>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Looks good to me.

Cheers Heiko

Re: [PATCH] submodule: fix NULL correctness in renamed broken submodules

[Kaartic Sivaraam]

[-- Attachment #1.1: Type: text/plain, Size: 2207 bytes --]

On Thursday 14 June 2018 11:01 PM, Stefan Beller wrote:
> While at it, make sure we only attempt to load the submodule if a git
> directory of the submodule is found as default_name_or_path will return
> NULL in case the git directory cannot be found.

I found this a little hard to read. Maybe it could be sentence could be
shrinked a little. Possibly,

    While at it, make sure we only attempt to load the submodule if
    a git directory of the submodule is found.

I guess the other part of the sentence doesn't make much sense in the
log message. Maybe it could be an in-code comment. Speaking of in-code
comment, the following would also fit there, wouldn't it?

> Note that passing NULL
> to submodule_from_name is just a semantic error, as submodule_from_name
> accepts NULL as a value, but then the return value is not the submodule
> that was asked for, but some arbitrary other submodule. (Cf. 'config_from'
> in submodule-config.c: "If any parameter except the cache is a NULL
> pointer just return the first submodule. Can be used to check whether
> there are any submodules parsed.")
> 


-- 
Sivaraam

QUOTE:

“The three principal virtues of a programmer are Laziness, Impatience,
and Hubris.”

	- Camel book

Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]