New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix in_merge_bases_many() with commit-graphs #739
Fix in_merge_bases_many() with commit-graphs #739
Conversation
Way back in f9b8908 (commit.c: use generation numbers for in_merge_bases(), 2018-05-01), a heuristic was used to short-circuit the in_merge_bases() walk. This works just fine as long as the caller is checking only two commits, but when there are multiple, there is a possibility that this heuristic is _very wrong_. Some code moves since then has changed this method to repo_in_merge_bases_many() inside commit-reach.c. The heuristic computes the minimum generation number of the "reference" list, then compares this number to the generation number of the "commit". In a recent topic, a test was added that used in_merge_bases_many() to test if a commit was reachable from a number of commits pulled from a reflog. However, this highlighted the problem: if any of the reference commits have a smaller generation number than the given commit, then the walk is skipped _even if there exist some with higher generation number_. This heuristic is wrong! It must check the MAXIMUM generation number of the reference commits, not the MINIMUM. This highlights a testing gap. t6600-test-reach.sh covers many methods in commit-reach.c, including in_merge_bases() and get_merge_bases_many(), but since these methods either restrict to two input commits or actually look for the full list of merge bases, they don't check this heuristic! Add a possible input to "test-tool reach" that tests in_merge_bases_many() and add tests to t6600-test-reach.sh that cover this heuristic. This includes cases for the reference commits having generation above and below the generation of the input commit, but also having maximum generation below the generation of the input commit. The fix itself is to swap min_generation with a max_generation in repo_in_merge_bases_many(). Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de> Reported-by: Srinidhi Kaushik <shrinidhi.kaushik@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
/submit |
Submitted as pull.739.git.1601650736489.gitgitgadget@gmail.com To fetch this version into
To fetch this version to local tag
|
On the Git mailing list, Junio C Hamano wrote (reply to this):
|
On the Git mailing list, Junio C Hamano wrote (reply to this):
|
On the Git mailing list, Derrick Stolee wrote (reply to this):
|
User |
This branch is now known as |
This patch series was integrated into seen via git@68ef15f. |
On the Git mailing list, Srinidhi Kaushik wrote (reply to this):
|
User |
On the Git mailing list, Junio C Hamano wrote (reply to this):
|
On the Git mailing list, Derrick Stolee wrote (reply to this):
|
This patch series was integrated into seen via git@e1d6635. |
This patch series was integrated into next via git@b736873. |
This patch series was integrated into seen via git@732b9ad. |
This patch series was integrated into seen via git@c01b041. |
This patch series was integrated into next via git@c01b041. |
This patch series was integrated into master via git@c01b041. |
Closed via c01b041. |
Johannes alerted me to the difficulties Srinidhi was having with
in_merge_bases_many()
and commit-graphs. Sorry that I hadn't seen that thread and the issues therein.After working with Johannes to investigate what was happening, we found a 2-year-old bug in the generation number checks!
Thanks,
-Stolee
cc: Derrick Stolee stolee@gmail.com
cc: Srinidhi Kaushik shrinidhi.kaushik@gmail.com