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

On Saturday 11 July 2009, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

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, [off-list ref]
www.herland.net