Prevent potential segfault in scalar reconfigure --all
#647
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This microsoft/git-specific change went through a few refactors to get into this state, but now it's sub-optimal: it "succeeds" if setting the config -or- toggling maintenance succeeds. The logic used to be that it set a failure in either of these modes, but the variable was changed to be a success. Merge these if statements together. This should simplify the logic for now. The resulting squashed commit could also be upstreamed. Signed-off-by: Derrick Stolee <stolee@gmail.com>
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. In my case, this is due to one of my repositories having a detached HEAD, which requires get_oid_hex() to parse that the HEAD reference is valid. Another way to cause a failure is to use the "includeIf.onbranch" config key, which will lead to a BUG() statement. My first inclination was to try to refactor cmd_reconfigure() to execute 'git for-each-repo' instead of this loop. In addition to the difficulty of executing 'scalar reconfigure' within 'git for-each-repo', it would be difficult to perform the clean-up logic for non-existent repos if we relied on that child process. Instead, I chose to move the temporary repo to be within the loop and reinstate the_repository to its old value after we are done performing logic on the current array item. Add tests to t9210-scalar.sh to test 'scalar reconfigure --all' with multiple registered repos. There are two different ways that the old use of the_repository could trigger bugs. These issues are being solved independently to be more careful about the_repository being uninitialized, but the change in this patch around the use of the_repository is still a good safety precaution. Co-authored-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
…-vfs-2.45.0 TODO: Replace the right-hand side of this merge with the upstream version of this change. Note that this was an easy merge with the left-hand side's fixup! commit squashing two if blocks together. Signed-off-by: Derrick Stolee <stolee@gmail.com>
dscho
force-pushed
the
scalar-reconfigure-vfs-2.45.0
branch
from
68b0d4c
to
633892e
Compare
dscho
approved these changes
May 14, 2024
dscho
added a commit
that referenced
this pull request
May 14, 2024
dscho
added a commit
that referenced
this pull request
May 14, 2024
dscho
added a commit
that referenced
this pull request
May 14, 2024
dscho
added a commit
that referenced
this pull request
May 14, 2024
dscho
added a commit
that referenced
this pull request
Jun 3, 2024
dscho
added a commit
that referenced
this pull request
Jun 3, 2024
dscho
added a commit
that referenced
this pull request
Jun 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See gitgitgadget#1724 for the upstream fix. This includes an extra detail around the context of that change that will require some reaction in microsoft/git. Please see the
fixup!and merge commit messages for details.This merges in the upstream commit that is going into
mastersoon.