> I wondered what would happen when it does not succeed. It looks like the > whole diff process just dies. > > The batch fetch is purely an optimization, because we'd eventually fetch > the individual objects on demand. If the batch one fails, should we > continue with the operation? That leaves any error-handling for the > overall operation to the "real" code. And it would also mean that this > bug became an annoying error message, On the one hand, if the batch fetch fails, then the individual prefetching would likely fail as well. But on the other hand, as you said below, we sometimes extraneously fetch objects, so making the batch fetch non-fatal might be a good idea too. > But certainly your fix is the right thing to do regardless. > > Tangential to your fix, but I also noticed while poking at this that > we're pretty aggressive about fetching objects, even if they won't be > needed. I know we touched on this briefly when discussing the original > patch, and the logic can get pretty complicated, so we punted. But there > are a few cases that I think might have a good cost/benefit ratio: > > 1. a --raw diff without renames doesn't need the blobs (and even with > renames, it only needs added/deleted entries). I imagine that being > able to "git log --raw" on a full history without pulling in a > bunch of blobs might be worthwhile (and a fairly common operation). > > 2. Files that exceed bigFileThreshold or are marked as binary via > gitattributes will generally be skipped during the file-level diff, > without even loading them. These are the minority of files, but > they also have an outsized cost to fetching them (and in fact may > be the very thing people are trying to avoid with a blob filter). > > Again, not anything to hold up your patch, but just cataloging some > future work for this area. Good points. Thanks.
