Re: [PATCHv2] push: change submodule default to check

Jeff King [off-list ref] writes:

This seems to be dropped from the list, probably due to no "To:"
header in the original, which led to "no", "To-header" "on" and
"input <" on YOUR recipient list, so I am quoting it in full without
trimming.

On Wed, Aug 24, 2016 at 10:30:17AM -0700, Stefan Beller wrote:

quoted

When working with submodules, it is easy to forget to push the submodules.
The setting 'check', which checks if any existing submodule is present on
at least one remote of the submodule remotes, is designed to prevent this
mistake.

Flipping the default to check for submodules is safer than the current
default of ignoring submodules while pushing.

It is safer, and that's good. But it's also slower, because it requires
an extra traversal of all of the pushed commits. And now people will
have to pay the price even if they are not using submodules at all.

For instance, try this from a checkout of linux.git:

  for i in no check; do
	rm -rf dst.git
	git init --bare dst.git
	echo "==> Pushing with submodules=$i"
	time git push --recurse-submodules=$i dst.git HEAD
  done

The second case takes 30-40 seconds longer. This is a full push of
history, so it's an extreme case[1], but it's still rather unfortunate.

Can we tie this default to some sign that submodules are actually in
use? I don't think the presence of .gitmodules is perfect (because you
might be in a bare repo, for example, and have just fetched some other
history you are relaying), but it may be a good compromise.  I'm
envisioning something like "--recurse-submodules=auto-check" which
auto-detects common situations (e.g., presence of .gitmodules or
.git/modules checkouts) and enables "check", and then setting the
default to that in the long run.

-Peff

[1] Actually, there is another much worse case lurking there. Try:

      git push --recurse-submodules=check --mirror dst.git

    from the kernel. I didn't let it finish, but I'd estimate it would
    take on the order of 5 hours. The problem is that push feeds each
    updated ref tip to find_unpushed_submodules(), so we end up walking
    over the same history over and over.

    I think it should feed all of the "before" and "after" ref tips it
    proposes to update to a _single_ revision traversal.

That sounds massively ... broken.  So before even thinking about
flipping it to default, this needs to be fixed first.

    I also notice that it uses "--remote=...", which is weird, because
    push knows exactly what it proposes to update, which may be ahead of
    where our refs/remotes/* cache is. Not to mention that we may be
    pushing to a remote for which we do not keep tracking refs at all!

    So I'd actually suspect that with your patch, a bare URL like:

      git push https://github.com/peff/linux.git

    would do the full 40-second walk, even if I was only pushing up one
    or two objects.