Re: [PATCH 3/2] batch check whether submodule needs pushing into one call

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Sep 16, 2016, 12:31 UTC
Message-ID: <20160916123155.GA40725@book.hvoigt.net>
In-Reply-To: <20160916094019.GB1488@book.hvoigt.net>

On Fri, Sep 16, 2016 at 11:40:19AM +0200, Heiko Voigt wrote:

> > By the way, with the two new patches, 'pu' seems to start failing
> > some tests, e.g. 5533 5404 5405.
> 
> Ah ok I did only test on master, will look into those.

Ok I had a look into these and the reason t5533 fails is because on pu --recurse-submodules is enabled by default and I missed the case when overwriting a ref. In that case we get the sha1 from the remote side as old. So we could catch that and fall back to all revisions there, but...

... tl;dr: The solution to use the old revisions from the remote side was too simple and does not make matters better but actually worse for some typical usecases. Its only in the last patch.

... that lead me to further thinking about the previous solution (using the locally cached remote refs) which might actually be a good default for the non-fastforward cases like creating new ref or overwriting a ref.

My current patch would handle the --mirror case nicer, since it gets a lot of old revs to reduce the revisions to check. For the typical one branch push it would most likely be worse. Except when the user is updating (fast-forwarding) the remote ref we would scan all revs of a ref until the root because we do not get enough valid revs that already exist on the remote.

The most exact solution would be to use all actual remote refs available (not sure if we have them at this point in the process?) another solution would be to still append the --remotes=<remotename> option as a fallback to reduce the revisions.

What do others think? Will leave this for a little bit further thinking. Its just the last patch ("use actual start hashes for submodule push check instead of local refs") that needs to go back to the drawing board.

Cheers Heiko