Re: [PATCH 1/3] unpack: replace xwrite() loop with write_in_full()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 4, 2024, 16:43 UTC
- Message-ID
- <xmqqzfveq76w.fsf@gitster.g>
- In-Reply-To
- <xmqqfrx6sbdf.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 22 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
>
>>> - while (input_len) {
>>> - err = xwrite(1, input_buffer + input_offset, input_len);
>>> - if (err <= 0)
>>> - break;
>>> - input_len -= err;
>>> - input_offset += err;
>>> - }
>>> + /* Write the last part of the buffer to stdout */
>>> + write_in_full(1, input_buffer + input_offset, input_len);
>>
>> With this change we stop updating `input_len` and `input_offset`, both
>> of which are global variables. Assuming that tests pass this must be
>> okay right now given that this is the final part of what we are writing.
>> But I wonder whether we shouldn't update those regardless just so that
>> these remain consistent?
>
> It is probably a good hygiene, even though it may not matter at all
> for the correctness in the current code.
>
> Thanks for your sharp eyes.Actually, I changed my mind. As you said, this is flushing the very end of the data in the input_buffer[] and nobody will fill() the input_buffer[] after the call to this function happens.
Show 8 quoted lines
>>> - while (len) {
>>> ...
>>> - len -= ret;
>>> - offset += ret;
>>> - }
>>> + write_in_full(1, buffer + offset, len);
>>
>> Same here.Ditto. We are about to pass the control back to the caller that will exit using the "has_errors" we return from here.
Show 9 quoted lines
>> >> Patrick >> >>> /* All done */ >>> return has_errors; >>> -- >>> 2.44.0-84-gb387623c12 >>> >>>