Fix commit-graph v1 regression in v2.55.0#954
Merged
dscho merged 6 commits intoJul 16, 2026
Conversation
Add a topology where the correct merge base (M2) has a lower committer date than its ancestor (M1) due to clock skew. With a v1 commit graph (topological levels only, no corrected commit dates), paint_down_to_common() falls back to commit-date ordering. In that mode, M1 pops before M2, acquires both paint sides, and the !FIND_ALL early exit fires -- returning the wrong merge base. Mark the test as test_expect_failure to document the bug; the next commit will fix it. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When paint_down_to_common() falls back to commit-date ordering (for v1 commit graphs without corrected commit dates), the !FIND_ALL early exit incorrectly fires. The exit assumes the queue is generation- ordered, so the first RESULT commit found must be the shallowest. With date ordering this is not guaranteed: a closer merge base with a lower committer date (clock skew) may still be in the queue behind deeper commits. Add a gen_ordered flag that is cleared when the date fallback fires, and require it for the early exit. Update the test from the previous commit to test_expect_success. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
dscho
changed the title
paint_down_to_common() regression in v2.55.0
Member
Author
I love it when my PR Checks fail for reasons that are totally outside my control...</s> |
derrickstolee
approved these changes
Jul 9, 2026
gitgitgadget
Bot
force-pushed
the
kk/commit-reach-find-all-fix
branch
from
44992e5 to
ae68032
Compare
Member
Author
|
Oops. By resolving the merge conflicts via 44992e5, I accidentally updated the branch in |
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Some users reported issues of repeated messages: fatal: recursion detected in die handler This wasn't happening every time, but we eventually captured a GIT_TRACE2_PERF log file with this issue and revealed an interesting internal detail, failing with this message: unable to format message: %4d-%02d-%02dT%02d:%02d:%02d.%06ldZ This specific format string tracks to tr2_tbuf_utc_datetime_extended() in trace2/tr2_tbuf.c. This logic began as tr2_tbuf_utc_time() in ee4512e (trace2: create new combined trace facility, 2019-02-22) but was later split in bad229a (trace2: clarify UTC datetime formatting, 2019-04-15). This use of xsnprintf() is writing a very specific datetime format into a 32-character buffer. The format requires that the input data will not overflow the format digits or the buffer will not hold the result. Since we are using xsnprintf() here, those failures turn into die() events. This method and its siblings, tr2_tbuf_local_time() and tr2_tbuf_utc_datetime(), are used in the tracing library. The extended form is used only for the 'event' format, which these users were using via a config setting for use in client-side telemetry. The non-extended form is used to help generate the 'SID' that defines the process in the traces. Not only are these inappropriate times for a failure, but the extended method is called specifially during the 'atexit' event, which was triggering this problem in a loop as the 'atexit' event would be retriggered by the die(). I could not determine the exact cause of why these errors started occuring in a bunch. My best guess is that these users are dogfooding an early operating system version that is more likely to fail in the gettimeofday() function and thus leaves the structures uninitialized and potentially violating the expected values. However, for full defense-in-depth I made several modifications: 1. Both 'tv' and 'tm' structs are initialized with zero values, allowing an erroring gettimeofday() or gmtime_r() method to leave them zero-valued. A zero-valued date is better than a die() here. 2. Replace the use of xsnprintf() with snprintf() to avoid the possibility of calling die() here. Instead, check the response to see if there was a failure. On failure, put a blank value into the buffer instead of possibly allowing a value that would not format correctly for a trace2 consumer. This value should be seen as obviously wrong and therefore signals a problem. As the core issue in this code seems to require a system method returning an error, no test accompanies this change. This change removes all uses of xsnprintf() from the trace2/ directory. There are two uses of xstrdup() that could be considered for removal, but they only die() on out-of-memory errors instead of formatting issues. I chose to leave those in place for now. Signed-off-by: Derrick Stolee <stolee@gmail.com>
Users were noticing some cases of infinite loops with the error message: ``` fatal: recursion detected in die handler ``` It's only happening for repos with a post-command hook, but it's not deterministic. I'm not sure what is triggering the problem, but the post-command hook is definitely able to recurse with its existing logic around the `run_post_hook` variable. Move this earlier to avoid a potential double-call. It's a fixup to the introduction of the method, which may cause conflicts with later adjustments to this method still in the branch thicket (but maybe those should be squashed, too). See #955 for the version on 2.54.0.
Fix a commit-graph v1 regression in v2.55.0: The regression was introduced by git/git@93e5b1680e (merged as git/git@6390da42c7) and first shipped in v2.55.0, which is why the base here is vfs-2.55.0. That commit added a fast-path early exit in paint_down_to_common() predicated on the priority queue being generation-ordered. The predicate holds under the generation-then-date ordering, but the same function falls back to plain commit-date ordering when !min_generation && !corrected_commit_dates_enabled(r), which is true whenever the loaded commit-graph lacks a GDAT chunk (i.e. a v1 graph). Under that fallback the early exit is unsound: with clock skew, a closer merge base can sit behind deeper commits in the queue, and git merge-base (without --all) can silently return a non-best answer. The tip commit git/git@ae68032a8d0 threads a gen_ordered flag through and gates the early exit on it. The reason microsoft/git in particular needs to carry this promptly, rather than wait the usual weeks for upstream graduation, is Scalar. set_recommended_config() pins commitGraph.generationVersion=1 (scalar.c:191), and that pin is applied on every scalar clone, scalar register, and scalar reconfigure. Stolee introduced it in 18580f0 as a deliberate but temporary compatibility measure for existing enlistments with v1 graphs on disk, with the accompanying Documentation/scalar.adoc note calling out that v2 is preferred and the pin is expected to change once the upgrade story solidifies. Every Scalar-managed enlistment on microsoft/git 2.55.x therefore lands on exactly the broken code path, and Scalar's target audience (Office monorepos and external adopters) is precisely the large-monorepo, many-active-branches cohort Kristofer's original optimization was aimed at in the first place. Sadly, even though this regression first appeared in v2.55.0, the topic branch is unfortunately based on an older commit where a context line changed, causing merge conflicts. Resolve them in the same way as 0444c74 (Merge branch 'kk/commit-reach-find-all-fix' into next, 2026-07-10). Signed-off-by: Johannes Schindelin <johasc@microsoft.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Junio has marked Kristofer's
paint_down_to_common()fix fornext(lore), but the topic has not yet graduated tomaster. This PR is a backport ontovfs-2.55.0, queued while upstream finishes cooking.The regression was introduced by git/git@93e5b1680e (merged as git/git@6390da42c7) and first shipped in v2.55.0, which is why the base here is
vfs-2.55.0. That commit added a fast-path early exit inpaint_down_to_common()predicated on the priority queue being generation-ordered. The predicate holds under the generation-then-date ordering, but the same function falls back to plain commit-date ordering when!min_generation && !corrected_commit_dates_enabled(r), which is true whenever the loaded commit-graph lacks a GDAT chunk (i.e. a v1 graph). Under that fallback the early exit is unsound: with clock skew, a closer merge base can sit behind deeper commits in the queue, andgit merge-base(without--all) can silently return a non-best answer. The tip commit git/git@ae68032a8d0 threads agen_orderedflag through and gates the early exit on it.The reason microsoft/git in particular needs to carry this promptly, rather than wait the usual weeks for upstream graduation, is Scalar.
set_recommended_config()pinscommitGraph.generationVersion=1(scalar.c:191), and that pin is applied on everyscalar clone,scalar register, andscalar reconfigure. Stolee introduced it in 18580f020d1 as a deliberate but temporary compatibility measure for existing enlistments with v1 graphs on disk, with the accompanyingDocumentation/scalar.adocnote calling out that v2 is preferred and the pin is expected to change once the upgrade story solidifies. Every Scalar-managed enlistment on microsoft/git 2.55.x therefore lands on exactly the broken code path, and Scalar's target audience (Office monorepos and external adopters) is precisely the large-monorepo, many-active-branches cohort Kristofer's original optimization was aimed at in the first place.