Re: [PATCH v5] quickfetch(): Prevent overflow of the rev-list command line

From: Johan Herland <johan@herland.net>
Date: Jul 11, 2009, 10:58 UTC
Message-ID: <200907111258.25118.johan@herland.net>
In-Reply-To: <7vy6qvn1ya.fsf@alter.siamese.dyndns.org>

On Saturday 11 July 2009, Junio C Hamano wrote:

Show 17 quoted lines

> Johan Herland <johan@herland.net> writes:
> > quickfetch() calls rev-list to check whether the objects we are about
> > to fetch are already present in the repo (if so, we can skip the object
> > fetch). However, when there are many (~1000) refs to be fetched, the
> > rev-list command line grows larger than the maximum command line size
> > on some systems (32K in Windows). This causes rev-list to fail, making
> > quickfetch() return non-zero, which unnecessarily triggers the
> > transport machinery. This somehow causes fetch to fail with an exit
> > code.
> >
> > By using the --stdin option to rev-list (and feeding the object list to
> > its standard input), we prevent the overflow of the rev-list command
> > line, which causes quickfetch(), and subsequently the overall fetch, to
> > succeed.
>
> I feel uneasy with that "somehow" at the end of the first paragraph, but
> nevertheless this is the right thing to do.

Yes, it feels wrong that transport_fetch_refs() returns error when there are no objects to be fetched. After all, the quickfetch() routine is only meant to be an optimization (to skip the object fetching when not needed). Only if quickfetch() returned a false positive (indicating that all objects are present when they're really not) would I expect it to have adverse effects on the result of the fetch. But even then, as you indicate, fixing quickfetch() itself is the right thing to do, and looking into transport_fetch_refs() is a separate issue.

> Since it is a very isolated
> change, I'd queue this directly on 'master' and see if anybody notices a
> breakage, as it would be relatively pain-free to revert if it turns out
> to be necessary.

Thanks.

Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net