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

2 messages, 2 authors, 2021-08-18 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2021-08-17 22:38:44

"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
From: Han-Wen Nienhuys <redacted>

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

Thanks.

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

From: Han-Wen Nienhuys <hidden>
Date: 2021-08-18 09:00:51

On Wed, Aug 18, 2021 at 12:38 AM Junio C Hamano [off-list ref] wrote:
"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:
quoted
From: Han-Wen Nienhuys <redacted>

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 <redacted>
---
    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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help