Re: [PATCH 0/3] push: detect local refspec errors early

From: Jeff King <peff@peff.net>
Date: Mar 5, 2014, 20:56 UTC
Message-ID: <20140305205644.GA13149@sigill.intra.peff.net>
In-Reply-To: <xmqqsiqwfjyd.fsf@gitster.dls.corp.google.com>

On Wed, Mar 05, 2014 at 12:51:06PM -0800, Junio C Hamano wrote:

Show 18 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > We can't fully process the refspecs until we have talked to the other
> > side, because they may involve matching refs from the remote; I don't
> > think git even really looks at them until after we've connected.
> >
> > But I think there are some obvious cases, like a bogus left-hand side
> > (i.e., what you have here) that cannot ever succeed, no matter what the
> > other side has. We could sanity check the refspecs before doing anything
> > else.
> 
> The user's wallclock time is more important than machine cycles,
> checking things we could check before having the user do things is a
> good principle to follow.
> 
> I wish that the solution did not have to involve doing the same
> computation twice, but I do not think there is a clean way around
> that in this codepath.

Yeah, there are two inefficiencies here:

  1. We parse the refspecs twice. In theory we could parse them once,
     feed the result to check_push_refspecs, then again to
     match_push_refspecs. That wouldn't be too hard a refactor.

  2. We match the "src" side to local refs twice. Getting rid of this
     would involve splitting match_push_refs into two (analyzing the
     "src" half and the "dst" half), somehow storing the intermediate
     the two calls, and only contacting the remote after the first step
     is done. This is probably trickier.

I'd be happy if somebody wanted to do those cleanups on top, but I don't personally have an interest in spending time on them. The amount of duplicated computation we're talking about here is not very much (and the number of refspecs tends to be small, anyway).

-Peff