Re: [PATCH] fixup! propagate errno from failing read

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2021, 18:30 UTC
Message-ID: <xmqqo89u61ww.fsf@gitster.g>
In-Reply-To: <CAFQ2z_Ni1bvj0Skgp_3p9htQfjn_M=3uF06pyZm_hkXgT_L61g@mail.gmail.com>

Han-Wen Nienhuys <hanwen@google.com> writes:

Show 21 quoted lines

>> I think the convention to assign errno to myerr in this codepath
>> originates in a0731250 (refs: explicitly return failure_errno from
>> parse_loose_ref_contents, 2021-07-20), and it forgot the part of the
>> code being fixed with this patch.  The commit being fixed is already
>> is in 'next' as part of the hn/refs-errno-cleanup topic.
>>
>> Usually, a flaw in a topic that is already in 'next' is corrected by
>> a follow-up patch, but then they won't say "fixup!" (none of our
>> bugfix patches do).  But a post-release is a special time, as we
>> will soon be rewinding 'next', restarting it from the latest release
>> and we have a choice to tentatively eject a topic, fix it up or
>> even replace it, before merging the corrected topic to 'next'.
>>
>> Do you mean that you want me to squash this change into that commit
>> before the topic graduates to 'master' during the new development
>> cycle?  If so please be a bit more explicit next time.  Using the
>> title of the commit after "fixup!" would be a good starting point.
>
> The problem fixed here affects anyone who uses git-repo (ie. does
> Android development) and runs "git-branch -m", which is a large group
> of people, so I think it should not be allowed to get into a release.

OK. The problem already is in 'next' and we want to make sure it won't graduate to 'master' for the next release as-is. I agree with that ;-)

> So it could be squashed into commit a0731250, or put on top of next as
> a separate commit (probably with 'fixup!' removed).

I'd try the former first and will fall back on the latter, then.

> Note that, even though commit a0731250 originates from a branch called
> "hn/XXX" and has me as Author, the BUG() call causing the crash was
> actually introduced by AEvar when he reworked the series.

Yup, I see his Sob after yours and it is quite understandable if a new bug was introduced by his changes. It also would be understandable if his change was only to add a call to BUG() in order to assert that the original patch used myerr consistently, and it uncovered a bug in the original version he took from you.

I do not care too much about how exactly the bug was introduced and uncovered---it matters more that the end result has one fewer bug thanks to the team effort.