From: Heiko Voigt <hvoigt@hvoigt.net>
To: Stefan Beller <sbeller@google.com>
Cc: Junio C Hamano <gitster@pobox.com>,
Jonathan Tan <jonathantanmy@google.com>,
"git@vger.kernel.org" <git@vger.kernel.org>,
Jonathan Nieder <jrnieder@gmail.com>
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 [thread overview]
Message-ID: <20170816163554.GA4683@book.hvoigt.net> (raw)
In-Reply-To: <CAGZ79kZAkdiKHUweQK6U4dqAakfzReDMfiHKDmzsKGuAH3BgyA@mail.gmail.com>
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
prev parent reply other threads:[~2017-08-16 16:36 UTC|newest]
Thread overview: 20+ 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 publicly 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=20170816163554.GA4683@book.hvoigt.net \
--to=hvoigt@hvoigt.net \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=jrnieder@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).