Re: [PATCH] push: do not add submodule odb as an alternate when recursing on demand

[Heiko Voigt <hvoigt@hvoigt.net>]

Hi,

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:

    find_unpushed_submodules()

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
submodule.

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 <gitster@pobox.com> wrote:
> > Stefan Beller <sbeller@google.com> 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