Re: [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed

Charvi Mendiratta [off-list ref] writes:

Okay, I looked into the write_message(...) and agree that it does not return
a positive value and only returns non-zero for error case and zero for
success. So, for this patch maybe we can ignore checking '< 0' here and
later add another patch to make this function follow the convention of
"negative is error".

Please don't.  There is a higher cognitive cost to readers when you write

	if (write_message(...)) {

The reader is forced to look at its implementation to see if it
returns positive in a non-error situation.

If you write it like so from the beginning

	if (write_message(...) < 0) {

the reader can trust that the code follows "negative is an error"
convention.  One fewer thing readers have to worry about.

Hi Junio,

On Fri, 15 Jan 2021 at 22:52, Junio C Hamano [off-list ref] wrote:

Charvi Mendiratta [off-list ref] writes:

quoted

Okay, I looked into the write_message(...) and agree that it does not return
a positive value and only returns non-zero for error case and zero for
success. So, for this patch maybe we can ignore checking '< 0' here and
later add another patch to make this function follow the convention of
"negative is error".

Please don't.  There is a higher cognitive cost to readers when you write

        if (write_message(...)) {

The reader is forced to look at its implementation to see if it
returns positive in a non-error situation.

If you write it like so from the beginning

        if (write_message(...) < 0) {

the reader can trust that the code follows "negative is an error"
convention.  One fewer thing readers have to worry about.

I agree, earlier I was confused as there were many instances of
write_message() without '< 0' in sequencer.c but considering the
above I completely agree to follow the convention.

Thanks and Regards,
Charvi