Repository: VirtusLab/git-machete
Pull request #1168 (2024-02-07 18:45:48)

Make fork-point also take into account common ancestors (and not only reflogs) in more cases


Patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index e6be018..ab6ae38 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,5 +1,9 @@
 # Release notes
 
+## New in git-machete 3.23.2
+
+- fixed: make fork-point also take into account common ancestors (and not only reflogs) in more cases
+
 ## New in git-machete 3.23.1
 
 - fixed: if a PR has a pre-v3.23.0 `Based on PR #...` header, then it's removed by `git machete github retarget-pr` in favor of the new extended PR chain
diff --git a/docs/man/git-machete.1 b/docs/man/git-machete.1
index a7ed6fc..24f6a6e 100644
--- a/docs/man/git-machete.1
+++ b/docs/man/git-machete.1
@@ -29,7 +29,7 @@ level margin: \\n[rst2man-indent\\n[rst2man-indent-level]]
 ..
 .TH "GIT-MACHETE" "1" "Feb 08, 2024" "" "git-machete"
 .SH NAME
-git-machete \- git-machete 3.23.1
+git-machete \- git-machete 3.23.2
 .sp
 git machete is a robust tool that \fBsimplifies your git workflows\fP\&.
 .sp
diff --git a/git_machete/__init__.py b/git_machete/__init__.py
index 9571cb3..77882e2 100644
--- a/git_machete/__init__.py
+++ b/git_machete/__init__.py
@@ -1,1 +1,1 @@
-__version__ = '3.23.1'
+__version__ = '3.23.2'
diff --git a/git_machete/client.py b/git_machete/client.py
index 7dee476..ce0086c 100644
--- a/git_machete/client.py
+++ b/git_machete/client.py
@@ -1296,30 +1296,30 @@ class MacheteClient:
         upstream_hash = self.__git.get_commit_hash_by_revision(upstream) if upstream else None
 
         if use_overrides:
-            overridden_fp_hash = self.__get_overridden_fork_point(branch)
-            if overridden_fp_hash:
+            overridden_fork_point = self.__get_overridden_fork_point(branch)
+            if overridden_fork_point:
                 if upstream and upstream_hash and \
                         self.__git.is_ancestor_or_equal(upstream.full_name(), branch.full_name()) and \
-                        not self.__git.is_ancestor_or_equal(upstream.full_name(), overridden_fp_hash):
+                        not self.__git.is_ancestor_or_equal(upstream.full_name(), overridden_fork_point):
                     # We need to handle the case when branch is a descendant of upstream,
                     # but the fork point of branch is overridden to a commit that is NOT a descendant of upstream.
-                    # In this case it's more reasonable to assume that upstream (and not overridden_fp_hash) is the fork point.
+                    # In this case it's more reasonable to assume that upstream (and not overridden_fork_point) is the fork point.
                     debug(
-                        f"{branch} is descendant of its upstream {upstream}, but overridden fork point commit {overridden_fp_hash} "
+                        f"{branch} is descendant of its upstream {upstream}, but overridden fork point commit {overridden_fork_point} "
                         f"is NOT a descendant of {upstream}; falling back to {upstream} as fork point")
                     return upstream_hash, []
                 elif upstream and \
-                        self.__git.is_ancestor_or_equal(overridden_fp_hash, upstream.full_name()):
+                        self.__git.is_ancestor_or_equal(overridden_fork_point, upstream.full_name()):
                     common_ancestor = self.__git.get_merge_base(upstream.full_name(), branch.full_name())
-                    # We are sure that a common ancestor exists - `overridden_fp_hash` is an ancestor of both `branch` and `upstream`.
+                    # We are sure that a common ancestor exists - `overridden_fork_point` is an ancestor of both `branch` and `upstream`.
                     assert common_ancestor is not None
                     return common_ancestor, []
                 else:
-                    debug(f"fork point of {branch} is overridden to {overridden_fp_hash}; skipping inference")
-                    return overridden_fp_hash, []
+                    debug(f"fork point of {branch} is overridden to {overridden_fork_point}; skipping inference")
+                    return overridden_fork_point, []
 
         try:
-            fp_hash, containing_branch_pairs = next(self.__match_log_to_filtered_reflogs(branch))
+            computed_fork_point, containing_branch_pairs = next(self.__match_log_to_filtered_reflogs(branch))
         except StopIteration:
             if upstream and upstream_hash:
                 if self.__git.is_ancestor_or_equal(upstream.full_name(), branch.full_name()):
@@ -1337,13 +1337,13 @@ class MacheteClient:
             raise MacheteException(f"Fork point not found for branch <b>{branch}</b>; "
                                    f"use `git machete fork-point {branch} --override-to...`")
         else:
-            debug(f"commit {fp_hash} is the most recent point in history of {branch} to occur on "
+            debug(f"commit {computed_fork_point} is the most recent point in history of {branch} to occur on "
                   "filtered reflog of any other branch or its remote counterpart "
                   f"(specifically: {' and '.join(map(utils.get_second, containing_branch_pairs))})")
 
             if upstream and upstream_hash and \
                     self.__git.is_ancestor_or_equal(upstream.full_name(), branch.full_name()) and \
-                    not self.__git.is_ancestor_or_equal(upstream.full_name(), fp_hash):
+                    not self.__git.is_ancestor_or_equal(upstream.full_name(), computed_fork_point):
                 # That happens very rarely in practice (typically current head
                 # of any branch, including upstream, should occur on the reflog
                 # of this branch, thus is_ancestor(upstream, branch) should imply
@@ -1351,23 +1351,35 @@ class MacheteClient:
                 # case reflog of upstream is incomplete for whatever reason.
                 debug(
                     f"{upstream} is an ancestor of its upstream {branch}, "
-                    f"but the inferred fork point commit {fp_hash} is NOT a descendant of {upstream}; "
+                    f"but the inferred fork point commit {computed_fork_point} is NOT a descendant of {upstream}; "
                     f"falling back to {upstream} as fork point")
                 return upstream_hash, []
             elif upstream and \
                     not self.__git.is_ancestor_or_equal(upstream.full_name(), branch.full_name()) and \
-                    self.__git.is_ancestor_or_equal(fp_hash, upstream.full_name()):
+                    self.__git.is_ancestor_or_equal(computed_fork_point, upstream.full_name()):
 
-                # We are sure that a common ancestor exists - `fp_hash` is an ancestor of both `branch` and `upstream`.
+                # We are sure that a common ancestor exists - `computed_fork_point` is an ancestor of both `branch` and `upstream`.
                 common_ancestor_hash = self.__git.get_merge_base(upstream.full_name(), branch.full_name())
                 assert common_ancestor_hash is not None
                 debug(
                     f"{upstream} is NOT an ancestor of its upstream {branch}, "
-                    f"but the inferred fork point commit {fp_hash} is an ancestor of {upstream}; "
+                    f"but the inferred fork point commit {computed_fork_point} is an ancestor of {upstream}; "
                     f"falling back to the common ancestor of {branch} and {upstream} (commit {common_ancestor_hash}) as fork point")
                 return common_ancestor_hash, []
             else:
-                return fp_hash, containing_branch_pairs
+                improved_fork_point = computed_fork_point
+                improved_containing_branch_pairs = containing_branch_pairs
+                for candidate_branch, original_matched_branch in containing_branch_pairs:
+                    merge_base = self.__git.get_merge_base(original_matched_branch, branch)
+                    debug(f"improving fork point {improved_fork_point} "
+                          f"by checking for merge_base({original_matched_branch}, {branch}) = {merge_base}")
+                    if merge_base:
+                        if self.__git.is_ancestor(improved_fork_point, merge_base):
+                            debug(f"improving fork point {improved_fork_point} to {merge_base}")
+                            improved_fork_point = merge_base
+                            improved_containing_branch_pairs = [BranchPair(candidate_branch, original_matched_branch)]
+                debug(f"effective fork point of {branch} is {improved_fork_point}")
+                return improved_fork_point, improved_containing_branch_pairs
 
     def fork_point(self, branch: LocalBranchShortName, use_overrides: bool) -> FullCommitHash:
         hash, containing_branch_pairs = self.__fork_point_and_containing_branch_pairs(branch, use_overrides)
@@ -1545,7 +1557,7 @@ class MacheteClient:
         # This can't be done with `git reset` since it doesn't allow for a custom reflog message.
         # Even worse, reset's reflog message would be filtered out in our fork point algorithm,
         # so the squashed commit would not even be considered to "belong"
-        # (in the FP sense) to the current branch's history.
+        # (in the fork-point sense) to the current branch's history.
         self.__git.update_head_ref_to_new_hash_with_reflog_subject(
             squashed_hash, f"squash: {earliest_commit.subject}")
 
@@ -1805,7 +1817,7 @@ class MacheteClient:
         self.__git.set_config_attr(to_key, to_hash)
 
         # Let's still set the now-deprecated `whileDescendantOf` key to maintain compatibility with older git-machete clients
-        # (esp. IntelliJ plugin) that still require that key for an override to apply.
+        # that still require that key for an override to apply.
         while_descendant_of_key = git_config_keys.override_fork_point_while_descendant_of(branch)
         self.__git.set_config_attr(while_descendant_of_key, to_hash)
 
diff --git a/git_machete/git_operations.py b/git_machete/git_operations.py
index 328f3ce..98829ed 100644
--- a/git_machete/git_operations.py
+++ b/git_machete/git_operations.py
@@ -747,17 +747,20 @@ class GitContext:
 
     # Note: the 'git rev-parse --verify' validation is not performed in case for either of earlier/later
     # if the corresponding prefix is empty AND the revision is a 40 hex digit hash.
-    def is_ancestor_or_equal(
-            self,
-            earlier_revision: AnyRevision,
-            later_revision: AnyRevision,
-    ) -> bool:
+    def is_ancestor_or_equal(self, earlier_revision: AnyRevision, later_revision: AnyRevision) -> bool:
         earlier_hash = self.get_commit_hash_by_revision(earlier_revision)
         later_hash = self.get_commit_hash_by_revision(later_revision)
         if not earlier_hash or not later_hash:
             return False
         return self.__get_merge_base_for_commit_hashes(earlier_hash, later_hash) == earlier_hash
 
+    def is_ancestor(self, earlier_revision: AnyRevision, later_revision: AnyRevision) -> bool:
+        earlier_hash = self.get_commit_hash_by_revision(earlier_revision)
+        later_hash = self.get_commit_hash_by_revision(later_revision)
+        if not earlier_hash or not later_hash or earlier_hash == later_hash:
+            return False
+        return self.is_ancestor_or_equal(earlier_hash, later_hash)
+
     def get_merge_base(
             self,
             earlier_revision: AnyRevision,


Test patch:
diff --git a/tests/base_test.py b/tests/base_test.py
index f050326..be103ff 100644
--- a/tests/base_test.py
+++ b/tests/base_test.py
@@ -17,8 +17,6 @@ class BaseTest:
             .new_repo(self.repo_sandbox.remote_path, bare=True)
             .new_repo(self.repo_sandbox.local_path, bare=False)
             .add_remote("origin", self.repo_sandbox.remote_path)
-            .set_git_config_key("user.email", "tester@test.com")
-            .set_git_config_key("user.name", "Tester Test")
         )
         self.expected_mock_methods: Set[str] = set()
 
@@ -62,6 +60,9 @@ class GitRepositorySandbox:
         os.chdir(directory)
         bare_opt = '--bare' if bare else ''
         self.execute(f'git init --quiet "{directory}" {bare_opt}')
+        if not bare:
+            self.set_git_config_key("user.email", "tester@test.com")
+            self.set_git_config_key("user.name", "Tester Test")
         if not switch_dir_to_new_repo:
             os.chdir(previous_dir)
         return self
@@ -118,6 +119,10 @@ class GitRepositorySandbox:
         self.execute("git pull")
         return self
 
+    def fetch(self) -> "GitRepositorySandbox":
+        self.execute("git fetch")
+        return self
+
     def merge(self, branch_name: str) -> "GitRepositorySandbox":
         self.execute(f'git merge {branch_name}')
         return self
diff --git a/tests/completion_e2e/test_completion_e2e.py b/tests/completion_e2e/test_completion_e2e.py
index 50bf347..90ece35 100644
--- a/tests/completion_e2e/test_completion_e2e.py
+++ b/tests/completion_e2e/test_completion_e2e.py
@@ -132,8 +132,6 @@ class TestCompletionEndToEnd:
             .new_repo(cls.repo_sandbox.remote_path, bare=True)
             .new_repo(cls.repo_sandbox.local_path, bare=False)
             .add_remote("origin", cls.repo_sandbox.remote_path)
-            .set_git_config_key("user.email", "tester@test.com")
-            .set_git_config_key("user.name", "Tester Test")
             .new_branch("master")
             .commit()
             .new_branch("develop")
diff --git a/tests/test_fork_point.py b/tests/test_fork_point.py
index 4b5bcfc..c7e1d42 100644
--- a/tests/test_fork_point.py
+++ b/tests/test_fork_point.py
@@ -245,16 +245,11 @@ class TestForkPoint(BaseTest):
         )
 
     def test_fork_point_covering_reflog_of_remote_branch(self) -> None:
-        origin_1_remote_path = mkdtemp()
-        origin_2_remote_path = mkdtemp()
-        self.repo_sandbox.new_repo(origin_1_remote_path, bare=True, switch_dir_to_new_repo=False)
-        self.repo_sandbox.new_repo(origin_2_remote_path, bare=True, switch_dir_to_new_repo=False)
-
         with fixed_author_and_committer_date_in_past():
             (
                 self.repo_sandbox
                 .new_branch("master").commit().push()
-                .new_branch("develop").commit().push()
+                .new_branch("develop").commit().push()  # commit dcd2db5
                 .chdir(self.repo_sandbox.remote_path)
                 .execute("git update-ref refs/heads/master develop")
                 .chdir(self.repo_sandbox.local_path)
@@ -266,9 +261,59 @@ class TestForkPoint(BaseTest):
             "dcd2db55125a1b67b367565e890a604639949a51\n"
         )
 
+        # Let's remove reflogs for local branches.
+        # Fork point should be inferred based on reflog of origin/master.
         self.repo_sandbox.remove_directory(".git/logs/refs/heads/")
-
         assert_success(
             ["fork-point"],
             "dcd2db55125a1b67b367565e890a604639949a51\n"
         )
+
+    def test_fork_point_reachable_from_master_but_not_on_its_reflog(self) -> None:
+        other_local_path = mkdtemp()
+        self.repo_sandbox.new_repo(other_local_path, bare=False, switch_dir_to_new_repo=False)
+
+        with fixed_author_and_committer_date_in_past():
+            (
+                self.repo_sandbox
+                .new_branch("master").commit().push()
+            )
+            first_master_commit = self.repo_sandbox.get_current_commit_hash()
+            (
+                self.repo_sandbox
+                .chdir(other_local_path)
+                .add_remote("origin", self.repo_sandbox.remote_path)
+                .fetch()
+                .check_out("master").commit().push()
+            )
+            second_master_commit = self.repo_sandbox.get_current_commit_hash()
+            (
+                self.repo_sandbox
+                .new_branch("feature").commit().push()
+                .check_out("master").commit().push()
+            )
+            (
+                self.repo_sandbox.chdir(self.repo_sandbox.local_path)
+                .check_out("master").pull()
+                .check_out("feature")
+            )
+
+        assert_success(
+            ["fork-point"],
+            second_master_commit + "\n"
+        )
+
+        # This is an unlikely scenario, devised to cover the case
+        # when there's no merge base between `feature` branch and a branch containing its un-improved fork point.
+        with fixed_author_and_committer_date_in_past():
+            (
+                self.repo_sandbox
+                .new_orphan_branch("develop").commit()
+                .check_out("master").reset_to("develop")
+                .check_out("feature")
+            )
+
+        assert_success(
+            ["fork-point"],
+            first_master_commit + "\n"
+        )
diff --git a/tests/test_github_checkout_prs.py b/tests/test_github_checkout_prs.py
index a0395e5..2ea7408 100644
--- a/tests/test_github_checkout_prs.py
+++ b/tests/test_github_checkout_prs.py
@@ -252,8 +252,6 @@ class TestGitHubCheckoutPRs(BaseTest):
             self.repo_sandbox
             .new_repo(other_local_path, bare=False)
             .add_remote("origin", GitRepositorySandbox.second_remote_path)
-            .set_git_config_key("user.email", "tester@test.com")
-            .set_git_config_key("user.name", "Tester Test")
             .new_branch('main')
             .commit('initial commit')
             .push()
@@ -335,8 +333,6 @@ class TestGitHubCheckoutPRs(BaseTest):
         (
             self.repo_sandbox.new_repo(local_path, bare=False)
             .add_remote("origin", GitRepositorySandbox.second_remote_path)
-            .set_git_config_key("user.email", "tester@test.com")
-            .set_git_config_key("user.name", "Tester Test")
             .new_branch('feature')
             .commit('initial commit')
             .push()