On Tue, Feb 27, 2018 at 06:47:04AM -0500, Derrick Stolee wrote: > Peff made an excellent point about the nested if statements. This > goes back to Christian's original recommendation. > > -- >8 -- > > During abbreviation checks, we navigate to the position within a > pack-index that an OID would be inserted and check surrounding OIDs > for the maximum matching prefix. This position may be beyond the > last position, because the given OID is lexicographically larger > than every OID in the pack. Then nth_packed_object_oid() does not > initialize "oid". > > Use the return value of nth_packed_object_oid() to prevent these > errors. > > Reported-by: Christian Couder <christian.couder@xxxxxxxxx> > Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> Thanks, this looks good to me. Semi-related, I wondered also at the weird asymmetry in the if-else, which is: if ... else if ... if ... but the comment directly above says: "we consider a maximum of three objects nearby". I think it's actually two, because you can only trigger one of the first two conditionals. Is that right? Let's imagine we're looking for object 1234abcd. If we didn't find a match, then we might have: 1234abcc 1234abce <-- first points here in which case we need to check both first-1 and first. And we do. If we do have a match, then we might see: 1234abcc 1234abcd <-- first points here 1234abce and we must check first-1 and first+1, but _not_ first. So I think the code is right, but the comment is wrong. -Peff
