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

From: Junio C Hamano <gitster@pobox.com>
To: Han-Wen Nienhuys <hanwen@google.com>
Cc: "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org, "Jonathan Tan" <jonathantanmy@google.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Han-Wen Nienhuys" <hanwenn@gmail.com>
Subject: Re: [PATCH] fixup! propagate errno from failing read
Date: Wed, 18 Aug 2021 11:30:39 -0700	[thread overview]
Message-ID: <xmqqo89u61ww.fsf@gitster.g> (raw)
In-Reply-To: <CAFQ2z_Ni1bvj0Skgp_3p9htQfjn_M=3uF06pyZm_hkXgT_L61g@mail.gmail.com> (Han-Wen Nienhuys's message of "Wed, 18 Aug 2021 11:00:36 +0200")

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

>> 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.