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

[Johan Herland <johan@herland.net>]

On Saturday 11 July 2009, Junio C Hamano wrote:
> 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