threads / bug / 48657

BUG: submodule code prints '(null)'

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

## tl;dr

12 messages between Jun 5, 2018 and Jun 17, 2018.

replies: 11people: 4as markdown or json

Duy Nguyen· Jun 5, 2018, 15:31 UTC · lore

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
Kaartic Sivaraam· Jun 6, 2018, 18:32 UTC · re: Duy Nguyen · lore

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

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!
Duy Nguyen· Jun 9, 2018, 11:04 UTC · re: Duy Nguyen · lore

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

On Tue, Jun 05, 2018 at 05:31:41PM +0200, Duy Nguyen wrote:
Show 7 quoted lines
> 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
Stefan Beller· Jun 11, 2018, 22:56 UTC · re: Duy Nguyen · lore

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

On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen <pclouds@gmail.com> wrote:
Show 24 quoted lines
>
> 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) {
        ...
Show 5 quoted lines
>                                 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.

Heiko Voigt· Jun 14, 2018, 15:15 UTC · re: Stefan Beller · lore

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

On Mon, Jun 11, 2018 at 03:56:16PM -0700, Stefan Beller wrote:
Show 61 quoted lines
> 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
Duy Nguyen· Jun 14, 2018, 15:44 UTC · re: Heiko Voigt · lore

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

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
Stefan Beller· Jun 14, 2018, 17:31 UTC · re: Duy Nguyen · lore

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

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
Stefan Beller· Jun 14, 2018, 17:37 UTC · re: Stefan Beller · lore

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

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
Heiko Voigt· Jun 14, 2018, 20:23 UTC · re: Stefan Beller · lore

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

On Thu, Jun 14, 2018 at 10:37:30AM -0700, Stefan Beller wrote:
Show 8 quoted lines
> 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
Heiko Voigt· Jun 14, 2018, 20:24 UTC · re: Stefan Beller · lore

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

Hi,
On Thu, Jun 14, 2018 at 10:31:07AM -0700, Stefan Beller wrote:
Show 26 quoted lines
> 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
Kaartic Sivaraam· Jun 17, 2018, 14:02 UTC · re: Stefan Beller · lore

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

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?

Show 8 quoted lines
> 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!
Stefan Beller· Jun 12, 2018, 17:35 UTC · re: Duy Nguyen · lore

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

On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen <pclouds@gmail.com> wrote:
Show 18 quoted lines
>
> 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.

← back to recent threads