On Wed, Oct 31, 2018 at 01:23:54PM +0900, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > The bug comes from commit f6371f9210 (sha1_file: add > > read_loose_object() function, 2017-01-13), which > > reimplemented some of the existing loose object functions. > > So it's worth checking if this bug was inherited from any of > > those. The answers seems to be no. The two obvious > > candidates are both OK: > > > > 1. unpack_sha1_rest(); this doesn't need to loop on > > Z_BUF_ERROR at all, since it allocates the expected > > output buffer in advance (which we can't do since we're > > explicitly streaming here) > > > > 2. check_object_signature(); the streaming path relies on > > the istream interface, which uses read_istream_loose() > > for this case. That function uses a similar "is our > > output buffer full" check with Z_BUF_ERROR (which is > > where I stole it from for this patch!) > > See 692f0bc7 to find who did the fix you stole from, and for what > kind of breakage the original fix was made. Heh. I did not dig into it, but actually thought "I'll bet Junio had to get this right when he wrote the streaming code. No wonder he spotted my mistake so quickly!". > By the way, a very similar loop for pack_non_delta istream iterates > while total_read is smaller than sz, but it does not have the same > check upon BUF_ERROR to see if we've read everything. Indeed. Did you find that one by inspection, or did you peek at: https://public-inbox.org/git/20130325202114.GD16019@xxxxxxxxxxxxxxxxxxxxx/ ? :) -Peff
