| Message ID | 20250225063351.GI1293961@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | some zlib inflating bug fixes | expand |
Jeff King <peff@peff.net> writes: > Inflating a loose object is considered successful only if we got > Z_STREAM_END and there were no more bytes. We check both of those > conditions and return success, but then have to check them a second time > to decide which error message to produce. > > I.e., we do something like this: > > if (!error_1 && !error_2) > ...return success... > > if (error_1) > ...handle error1... > else if (error_2) > ...handle error2... > ...common error handling... > > This repetition was the source of a small bug fixed in an earlier commit > (our Z_STREAM_END check was not the same in the two conditionals). > > Instead we can chain them all into a single if/else cascade, which > avoids repeating ourselves: > > if (error_1) > ...handle error1... > else if (error_2) > ...handle error2.... > else > ...return success... > ...common error handling... > > Signed-off-by: Jeff King <peff@peff.net> > --- > object-file.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Of course the resulting code is so much cleaner and more obvious. Thanks for cleaning it up. > diff --git a/object-file.c b/object-file.c > index 8cf87caef5..b7928fb74e 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -1436,15 +1436,15 @@ static void *unpack_loose_rest(git_zstream *stream, > obj_read_lock(); > } > } > - if (status == Z_STREAM_END && !stream->avail_in) { > - return buf; > - } > > if (status != Z_STREAM_END) > error(_("corrupt loose object '%s'"), oid_to_hex(oid)); > else if (stream->avail_in) > error(_("garbage at end of loose object '%s'"), > oid_to_hex(oid)); > + else > + return buf; > + > free(buf); > return NULL; > }
diff --git a/object-file.c b/object-file.c index 8cf87caef5..b7928fb74e 100644 --- a/object-file.c +++ b/object-file.c @@ -1436,15 +1436,15 @@ static void *unpack_loose_rest(git_zstream *stream, obj_read_lock(); } } - if (status == Z_STREAM_END && !stream->avail_in) { - return buf; - } if (status != Z_STREAM_END) error(_("corrupt loose object '%s'"), oid_to_hex(oid)); else if (stream->avail_in) error(_("garbage at end of loose object '%s'"), oid_to_hex(oid)); + else + return buf; + free(buf); return NULL; }
Inflating a loose object is considered successful only if we got Z_STREAM_END and there were no more bytes. We check both of those conditions and return success, but then have to check them a second time to decide which error message to produce. I.e., we do something like this: if (!error_1 && !error_2) ...return success... if (error_1) ...handle error1... else if (error_2) ...handle error2... ...common error handling... This repetition was the source of a small bug fixed in an earlier commit (our Z_STREAM_END check was not the same in the two conditionals). Instead we can chain them all into a single if/else cascade, which avoids repeating ourselves: if (error_1) ...handle error1... else if (error_2) ...handle error2.... else ...return success... ...common error handling... Signed-off-by: Jeff King <peff@peff.net> --- object-file.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)