Re: BUG: submodule code prints '(null)'
On Thu, Jun 14, 2018 at 5:15 PM Heiko Voigt 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
Re: BUG: submodule code prints '(null)'
On Mon, Jun 11, 2018 at 03:56:16PM -0700, Stefan Beller wrote:
> On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen 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)'
On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen 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.gc12...@book.hvoigt.net/ and the follow ups, specifically https://public-inbox.org/git/20171019181109.27792-2-sbel...@google.com/ that tries to clean up the code, but was ultimately dropped.
Re: BUG: submodule code prints '(null)'
On Sat, Jun 9, 2018 at 4:04 AM Duy Nguyen 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)'
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)'
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! signature.asc Description: OpenPGP digital signature
