On 10/5/2021 4:36 PM, Jeff King wrote: > When we're enumerating all objects in the object database, it doesn't > make sense to respect refs/replace. The point of this option is to > enumerate all of the objects in the database at a low level. By > definition we'd already show the replacement object's contents (under > its real oid), and showing those contents under another oid is almost > certainly working against what the user is trying to do. ... > So it has worked reliably this way for over 7 years, and we should make > sure it continues to do so. That could also be an argument that > --batch-all-objects should not change behavior (which this patch is > doing), but I really consider the current behavior to be an unintended > bug. It's a side effect of how the code is implemented (feeding the oids > back into oid_object_info() rather than looking at what we found while > reading the loose and packed object storage). I'm with you on thinking this is a bug. > The implementation is straight-forward: we just disable the global > read_replace_refs flag when we're in --batch-all-objects mode. It would > perhaps be a little cleaner to change the flag we pass to > oid_object_info_extended(), but that's not enough. We also read objects > via read_object_file() and stream_blob_to_fd(). The former could switch > to its _extended() form, but the streaming code has no mechanism for > disabling replace refs. Setting the global flag works, and as a bonus, > it's impossible to have any "oops, we're sometimes replacing the object > and sometimes not" bugs in the output (like the ones caused by > 98e2092b50 above). ... > Reading between the lines, you might guess that I also introduced a > "whoops, our size/content do not match" bug while trying the other > approach. ;) I understand this perspective. Turning the feature off is the only way to be sure. Patch makes sense. I just wanted to :+1: your reasoning. Thanks, -Stolee
