Re: [PATCH] fixup! propagate errno from failing read - Han-Wen Nienhuys

From: Han-Wen Nienhuys <hanwen@google.com>
To: Junio C Hamano <gitster@pobox.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:00:36 +0200	[thread overview]
Message-ID: <CAFQ2z_Ni1bvj0Skgp_3p9htQfjn_M=3uF06pyZm_hkXgT_L61g@mail.gmail.com> (raw)
In-Reply-To: <xmqq4kbn7l3l.fsf@gitster.g>

On Wed, Aug 18, 2021 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Han-Wen Nienhuys <hanwen@google.com>
> >
> > This fixes a crash triggered by the BUG() statement.
> >
> > This can occur with symlinked .git/refs. To check availability,
> > refs_verify_refname_available will run refs_read_raw_ref() on each prefix,
> > leading to a read() from .git/refs (which is a directory).
> >
> > When handling the symlink case, it is probably more robust to re-issue the
> > lstat() as a normal stat(), in which case, we would fall back to the directory
> > case.
> >
> > For now, propagating errno from strbuf_read() is simpler and avoids the crash.
> >
> > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> > ---
> >     fixup! propagate errno from failing read
>
> Hmph, I do not see a commit with "propagate errno from failing read"
> in its title anywhere in 'seen'.

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

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

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.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado