mailing list mirror (one of many)
 help / Atom feed
From: Heiko Voigt <>
To: Stefan Beller <>
Cc: Junio C Hamano <>,
	Jonathan Tan <>,
	"" <>,
	Jonathan Nieder <>
Subject: Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand
Date: Wed, 16 Aug 2017 18:35:54 +0200
Message-ID: <> (raw)
In-Reply-To: <>


was about to write that we are maybe overly cautious here. Because the
current way a submodule ends up in the list to be pushed is through:


that itself collects all changed submodules when submodule_needs_pushing() is
true. In there we have this:

    if (!submodule_has_commits(path, commits))
         * NOTE: We do consider it safe to return "no" here. The
         * correct answer would be "We do not know" instead of
         * "No push needed", but it is quite hard to change
         * the submodule pointer without having the submodule
         * around. If a user did however change the submodules
         * without having the submodule around, this indicates
         * an expert who knows what they are doing or a
         * maintainer integrating work from other people. In
         * both cases it should be safe to skip this check.
        return 0;

So if the check, whether a submodule has commits, fails for any reason it will
not end up in the list to be pushed.

As a side note: inside submodule_has_commits() there is an add_submodule_odb()
followed by a process to really make sure that the commits are in the

So IMO at this point we can be sure that the *database* exists and this extra
check could be dropped if we said that a caller to push_submodule() should make
sure that the submodule exists. The current ones are doing it already (if I did
not miss anything).

On Tue, Aug 15, 2017 at 06:05:25PM -0700, Stefan Beller wrote:
> On Tue, Aug 15, 2017 at 5:11 PM, Junio C Hamano <> wrote:
> > Stefan Beller <> writes:
> >
> >>> Is "is it populated" a good thing to check here, though?  IIRC,
> >>> add-submodule-odb allows you to add the object database of an
> >>> inactivated submodule, so this seems to change the behaviour.  I do
> >>> not know if the behaviour change is a good thing (i.e. bugfix) or
> >>> not (i.e. regression) offhand, though.
> >>
> >> Good point, we should be able to push non-populated, even inactive(?)
> >> submodules. For that we strictly need add_submodule_odb here
> >> (or the repo object of the submodule, eventually).
> >>
> >> So let's retract this patch for now.
> >
> > Not so fast.
> Ok, I took another look at the code.
> While we may desire that un-populated submodules can be pushed
> (due to checking out another revision where the submodule
> doesn't exist, before pushing), this is not supported currently, because
> the call to run the push in the submodule assumes there is a
> "<path>/.git" on which the child process can operate.
> So for now we HAVE to have the submodule populated.

That is a good point though. In the current form of push_submodule() we need to
have a populated submodule. So IMO to check whether the submodule is actually
*populated* instead of adding the odb is correct and a possible bug fix.

Cheers Heiko

      parent reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-12 23:45 [PATCH] submodule: use cheaper check for submodule pushes Stefan Beller
2017-07-13  0:01 ` Jonathan Nieder
2017-07-13  0:09   ` Stefan Beller
2017-07-13  0:53   ` Junio C Hamano
2017-07-13  5:14     ` Stefan Beller
2017-07-13 18:37       ` Junio C Hamano
2017-07-13 19:39         ` Stefan Beller
2017-07-13 20:48           ` Jonathan Nieder
2017-07-13 20:54             ` Stefan Beller
2017-08-15 22:43               ` [PATCH] push: do not add submodule odb as an alternate when recursing on demand Stefan Beller
2017-08-15 23:10                 ` Jonathan Nieder
2017-08-15 23:14                 ` Jonathan Nieder
2017-08-15 23:27                   ` Stefan Beller
2017-08-15 23:23                 ` Junio C Hamano
2017-08-15 23:31                   ` Stefan Beller
2017-08-16  0:11                     ` Junio C Hamano
2017-08-16  1:05                       ` Stefan Beller
2017-08-16  2:08                         ` Jonathan Nieder
2017-08-16  5:52                           ` Stefan Beller
2017-08-16 16:35                         ` Heiko Voigt [this message]

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:

  List information:

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

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror
	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:

 note: .onion URLs require Tor:
       or Tor2web:

AGPL code for this site: git clone public-inbox