On 5/2/24 2:46 AM, Patrick Steinhardt wrote:
On Tue, Apr 30, 2024 at 04:58:52PM +0000, Derrick Stolee via GitGitGadget wrote:
From: Derrick Stolee <stolee@xxxxxxxxx>
During the latest v2.45.0 update, 'scalar reconfigure --all' started to
segfault on my machine. Breaking it down via the debugger, it was
faulting on a NULL reference to the_hash_algo, which is a macro pointing
to the_repository->hash_algo.
This NULL reference appears to be due to the way the loop is abusing the
the_repository pointer, pointing it to a local repository struct after
discovering that the current directory is a valid Git repository. This
repo-swapping bit was in the original implementation from 4582676075
(scalar: teach 'reconfigure' to optionally handle all registered
enlistments, 2021-12-03), but only recently started segfaulting while
trying to parse the HEAD reference. This also only happens on the
_second_ repository in the list, so does not reproduce if there is only
one registered repo.
Interesting. This also has some overlap with my patch series that aims
to drop the default-SHA1 fallback that we have in place for
`the_repository` [1].
Thanks for this pointer. It indeed will help.
Add a test to t9210-scalar.sh to test 'scalar reconfigure --all' with
multiple registered repos, as a precaution. Unfortunately, I was unable
to reproduce the segfault using this test, so there is some coverage
left to be desired. What exactly causes my setup to hit this bug but not
this test structure? Unclear.
One way to trigger _a_ BUG is to use an "includeIf.onbranch:foobar.path"
config. This will cause Git to try and look up the "HEAD" reference, but
because we have a partially-configured repository, only, that will crash
with:
BUG: refs.c:2123: reference backend is unknown
This is a good extra find. After your explanation for the second
test, I'm confident that I was hitting the detached HEAD case on
my machine.
I will shamelessly steal your tests in my v2.
This issue should be fixed by my patch series in [1] because we start to
use `get_oid_hex_any()` to parse detached HEADs.
Anyway, your fix is indeed effective because with `repo_init()` we now
properly configure the repository.
I appreciate that your series will fix this in a ref-focused way. I think
this change could prevent other bad interactions with the_repository in
the future.
Thanks,
-Stolee
