git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Heiko Voigt <hvoigt@hvoigt.net>
To: Stefan Beller <sbeller@google.com>
Cc: Duy Nguyen <pclouds@gmail.com>, git <git@vger.kernel.org>,
	Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Subject: Re: BUG: submodule code prints '(null)'
Date: Thu, 14 Jun 2018 17:15:21 +0200
Message-ID: <20180614151521.GA2436@book.hvoigt.net> (raw)
In-Reply-To: <CAGZ79kZk=OGPJdsDEHtgmPUrO7P2rOLSV40aJawdLs5e0=Kduw@mail.gmail.com>

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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-05 15:31 Duy Nguyen
2018-06-06 18:32 ` Kaartic Sivaraam
2018-06-09 11:04 ` Duy Nguyen
2018-06-11 22:56   ` Stefan Beller
2018-06-14 15:15     ` Heiko Voigt [this message]
2018-06-14 15:44       ` Duy Nguyen
2018-06-14 17:31         ` [PATCH] submodule: fix NULL correctness in renamed broken submodules Stefan Beller
2018-06-14 17:37           ` [PATCH] t5526: test recursive submodules when fetching moved submodules Stefan Beller
2018-06-14 20:23             ` Heiko Voigt
2018-06-14 20:24           ` [PATCH] submodule: fix NULL correctness in renamed broken submodules Heiko Voigt
2018-06-17 14:02           ` Kaartic Sivaraam
2018-06-12 17:35   ` BUG: submodule code prints '(null)' Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180614151521.GA2436@book.hvoigt.net \
    --to=hvoigt@hvoigt.net \
    --cc=git@vger.kernel.org \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox