Repository: VirtusLab/git-machete
Pull request #1209 (2024-03-25 10:53:03)

Allow setting per-branch update=merge option
This PR adds support for a `update=merge` annotation. This makes it so that `git machete traverse` or `git machete update` will switch to merging that branch if it has that annotation, without requiring the use of the `-M` option for all the branches.

The use case behind this is the following. As stated in the `git-machete` docs, rebase is usually the best option to maintain a clean history with branches, thus it makes sense to use it as a default. However, when a branch is already being reviewed in PRs, rebasing it can cause Github to lose track of what was reviewed and what changed after the reviews, so it makes sense to merge instead of rebase in those cases. With this PR this can be done automatically. This is something very useful for my workflow and I wanted to send a PR in case it is interesting to include in the upstream. I will probably work in the future on adding the annotation automatically based on the PR state, if you think it's interesting I can send that as a PR too (although maybe that PR is a little bit more opinionated and less useful to the general userbase).

### Notes 

I think I did not miss any place where merges can be optionally done, I searched for the usage of `opt_merge` too just in case. I added a test case to check that this was working, based on the test that uses the `-M` option. I had to remove the restriction on passing `--no-edit-merge` without `-M`, as now it does make sense to pass that option even if merge is not the default behavior.



codecov-commenter: ## [Codecov](https://app.codecov.io/gh/VirtusLab/git-machete/pull/1209?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 98.62%. Comparing base [(`3ed83f1`)](https://app.codecov.io/gh/VirtusLab/git-machete/commit/3ed83f1541bfa2bc6e42c094c1bcdafa84a1feee?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab) to head [(`4908978`)](https://app.codecov.io/gh/VirtusLab/git-machete/pull/1209?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab).


<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff            @@
##           develop    #1209   +/-   ##
========================================
  Coverage    98.62%   98.62%           
========================================
  Files           12       12           
  Lines         3416     3424    +8     
  Branches       773      773           
========================================
+ Hits          3369     3377    +8     
  Misses          31       31           
  Partials        16       16           
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/VirtusLab/git-machete/pull/1209?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=VirtusLab).

PawelLipski: Wow nice 😯 still, you know our stance on merges in general... they just don't work well with stacked branches 😅 but anyway, the options like `-M` are there for the specific cases/workflows where it makes sense.

One thing that I'd reconsider in this PR... I'm worried that calling this qualifier `merge=yes` would be confusing. The semantics of the existing `{push,rebase,slide-out}=no` qualifiers is that the given operation is **not** performed when running `git machete traverse`... which esp. matters in case of `traverse --yes`.

Along these lines... one could then ask for `merge=no` qualifier, which would make `traverse` skip the merges for the specific branch when `--merge` mode is selected.

Now, if we consider hypothetical `...=yes` qualifiers (which don't exist so far)... I'd say it'd be more logical for them to mean sth like _perform rebase/push/slide-out/merge even if this operation is disabled via some other mechanism_ 🤔

But! how about renaming this qualifier to sth like `update=merge`? There's `git machete update` command, coming from the very earliest days of git-machete... which does the syncing of a branch to its parent via rebase or merge. Using a `update=merge` qualifier would logically mean that we let `traverse` perform update via merge, rather than the default rebase 🤔  
PawelLipski: (edited the description to fix the typo)
PawelLipski: >  I will probably work in the future on adding the annotation automatically based on the PR state, if you think it's interesting I can send that as a PR too (although maybe that PR is a little bit more opinionated and less useful to the general userbase).

Wow I'm afraid indeed that this would be too opinionated to place directly in core git-machete 😅 this might however be added to the repo as a sample standalone script (git hook? see [hook_samples directory](https://github.com/VirtusLab/git-machete/tree/master/hook_samples))
gjulianm: > But! how about **renaming this qualifier to sth like `update=merge`**?

It does make much more sense than `merge=yes` indeed. Changed and updated the PR :) 
gjulianm: Fixed your comments :) I bumped to v3.25.0 as it's a behavior change, not sure if changes in annotations are worth of a minor bump though, if you want a patch increase let me know.

Patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 9440503..4c9f3cf 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,5 +1,9 @@
 # Release notes
 
+## New in git-machete 3.25.0
+
+- added: qualifier `update=merge` allows selecting merge strategy per branch (contributed by @gjulianm)
+
 ## New in git-machete 3.24.3
 
 - added: Scoop package for Windows (suggested by @ppasieka)
diff --git a/docs/man/git-machete.1 b/docs/man/git-machete.1
index 3c3cac3..29c3692 100644
--- a/docs/man/git-machete.1
+++ b/docs/man/git-machete.1
@@ -27,9 +27,9 @@ level margin: \\n[rst2man-indent\\n[rst2man-indent-level]]
 .\" new: \\n[rst2man-indent\\n[rst2man-indent-level]]
 .in \\n[rst2man-indent\\n[rst2man-indent-level]]u
 ..
-.TH "GIT-MACHETE" "1" "Mar 11, 2024" "" "git-machete"
+.TH "GIT-MACHETE" "1" "Mar 26, 2024" "" "git-machete"
 .SH NAME
-git-machete \- git-machete 3.24.3
+git-machete \- git-machete 3.25.0
 .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 479ba18..07824c0 100644
--- a/git_machete/__init__.py
+++ b/git_machete/__init__.py
@@ -1,1 +1,1 @@
-__version__ = '3.24.3'
+__version__ = '3.25.0'
diff --git a/git_machete/annotation.py b/git_machete/annotation.py
index ccbb536..de32369 100644
--- a/git_machete/annotation.py
+++ b/git_machete/annotation.py
@@ -9,39 +9,52 @@ class Qualifiers:
         self.__push_text: str = ''
         self.__rebase_text: str = ''
         self.__slide_out_text: str = ''
+        self.__merge_text: str = ""
         self.rebase: bool = True
         self.push: bool = True
         self.slide_out: bool = True
+        self.merge: bool = False
 
-        def match_pattern(text: str) -> str:
-            return f'.*\\b{text}=no\\b.*'
+        def match_pattern(pattern: str) -> str:
+            return f".*\\b{pattern}\\b.*"
 
-        def sub_pattern(text: str) -> str:
-            return f'[ ]?{text}=no[ ]?'
+        def sub_pattern(pattern: str) -> str:
+            return f"[ ]?{pattern}[ ]?"
 
-        rebase_match = re.match(match_pattern('rebase'), annotation)
+        rebase_anno = 'rebase=no'
+        rebase_match = re.match(match_pattern(rebase_anno), annotation)
         if rebase_match:
             self.rebase = False
-            self.__rebase_text = 'rebase=no'
-            self.__annotation_without_qualifiers = re.sub(sub_pattern('rebase'), ' ', self.__annotation_without_qualifiers)
+            self.__rebase_text = rebase_anno
+            self.__annotation_without_qualifiers = re.sub(sub_pattern(rebase_anno), ' ', self.__annotation_without_qualifiers)
 
-        push_match = re.match(match_pattern('push'), annotation)
+        push_anno = 'push=no'
+        push_match = re.match(match_pattern(push_anno), annotation)
         if push_match:
             self.push = False
-            self.__push_text = 'push=no'
-            self.__annotation_without_qualifiers = re.sub(sub_pattern('push'), ' ', self.__annotation_without_qualifiers)
+            self.__push_text = push_anno
+            self.__annotation_without_qualifiers = re.sub(sub_pattern(push_anno), ' ', self.__annotation_without_qualifiers)
 
-        slide_out_match = re.match(match_pattern('slide-out'), annotation)
+        slide_out_anno = 'slide-out=no'
+        slide_out_match = re.match(match_pattern(slide_out_anno), annotation)
         if slide_out_match:
             self.slide_out = False
-            self.__slide_out_text = 'slide-out=no'
-            self.__annotation_without_qualifiers = re.sub(sub_pattern('slide-out'), ' ', self.__annotation_without_qualifiers)
+            self.__slide_out_text = slide_out_anno
+            self.__annotation_without_qualifiers = re.sub(sub_pattern(slide_out_anno), ' ', self.__annotation_without_qualifiers)
+
+        merge_anno = 'update=merge'
+        merge_match = re.match(match_pattern(merge_anno), annotation)
+        if merge_match:
+            self.merge = True
+            self.__merge_text = merge_anno
+            self.__annotation_without_qualifiers = re.sub(sub_pattern(merge_anno), ' ', self.__annotation_without_qualifiers)
 
     def get_annotation_text_without_qualifiers(self) -> str:
         return self.__annotation_without_qualifiers.strip()
 
     def get_qualifiers_text(self) -> str:
-        return f'{self.__rebase_text} {self.__push_text} {self.__slide_out_text}'.replace('  ', ' ').strip()
+        text = f'{self.__rebase_text} {self.__push_text} {self.__slide_out_text} {self.__merge_text}'
+        return re.sub(' +', ' ', text).strip()
 
 
 class Annotation:
diff --git a/git_machete/client.py b/git_machete/client.py
index 4aa6415..ea407e9 100644
--- a/git_machete/client.py
+++ b/git_machete/client.py
@@ -346,7 +346,8 @@ class MacheteClient:
             self, *, opt_merge: bool, opt_no_edit_merge: bool,
             opt_no_interactive_rebase: bool, opt_fork_point: Optional[AnyRevision]) -> None:
         current_branch = self.__git.get_current_branch()
-        if opt_merge:
+        use_merge = opt_merge or (current_branch in self.annotations and self.annotations[current_branch].qualifiers.merge)
+        if use_merge:
             with_branch = self.up(
                 current_branch,
                 prompt_if_inferred_msg=("Branch <b>%s</b> not found in the tree of branch dependencies. "
@@ -577,7 +578,8 @@ class MacheteClient:
         self.__git.checkout(new_upstream)
         for new_downstream in new_downstreams:
             self.__git.checkout(new_downstream)
-            if opt_merge:
+            use_merge = opt_merge or (new_downstream in self.annotations and self.annotations[new_downstream].qualifiers.merge)
+            if use_merge:
                 print(f"Merging {bold(new_upstream)} into {bold(new_downstream)}...")
                 self.__git.merge(new_upstream, new_downstream, opt_no_edit_merge)
             else:
@@ -735,6 +737,8 @@ class MacheteClient:
             else:
                 needs_remote_sync = False
 
+            use_merge = opt_merge or (branch in self.annotations and self.annotations[branch].qualifiers.merge)
+
             if needs_slide_out:
                 # Avoid unnecessary fork point check if we already know that the
                 # branch qualifies for slide out;
@@ -745,7 +749,7 @@ class MacheteClient:
                 # branch qualifies for resetting to remote counterpart;
                 # neither rebase nor merge will be suggested in such case anyway.
                 needs_parent_sync = False
-            elif opt_merge:
+            elif use_merge:
                 needs_parent_sync = bool(
                     upstream and not self.__git.is_ancestor_or_equal(upstream.full_name(), branch.full_name()))
             else:  # using rebase
@@ -809,14 +813,14 @@ class MacheteClient:
                 any_action_suggested = True
                 self.__print_new_line(False)
                 assert upstream is not None
-                if opt_merge:
+                if use_merge:
                     ans = self.ask_if(f"Merge {bold(upstream)} into {bold(branch)}?" + get_pretty_choices('y', 'N', 'q', 'yq'),
                                       f"Merging {bold(upstream)} into {bold(branch)}...", opt_yes=opt_yes)
                 else:
                     ans = self.ask_if(f"Rebase {bold(branch)} onto {bold(upstream)}?" + get_pretty_choices('y', 'N', 'q', 'yq'),
                                       f"Rebasing {bold(branch)} onto {bold(upstream)}...", opt_yes=opt_yes)
                 if ans in ('y', 'yes', 'yq'):
-                    if opt_merge:
+                    if use_merge:
                         self.__git.merge(upstream, branch, opt_no_edit_merge)
                         # It's clearly possible that merge can be in progress
                         # after 'git merge' returned non-zero exit code;
diff --git a/git_machete/options.py b/git_machete/options.py
index 496c8b4..df9dd3b 100644
--- a/git_machete/options.py
+++ b/git_machete/options.py
@@ -48,10 +48,6 @@ class CommandLineOptions:
         if self.opt_as_root and self.opt_as_first_child:
             raise MacheteException(
                 "Option `-R/--as-root` cannot be specified together with `-f/--as-first-child`.")
-        if self.opt_no_edit_merge and not self.opt_merge:
-            raise MacheteException(
-                "Option `--no-edit-merge` only makes sense when using merge and "
-                "must be specified together with `-M/--merge`.")
         if self.opt_no_interactive_rebase and self.opt_merge:
             raise MacheteException(
                 "Option `--no-interactive-rebase` only makes sense when using "


Test patch:
diff --git a/tests/test_traverse.py b/tests/test_traverse.py
index f0f5c9b..e7db79d 100644
--- a/tests/test_traverse.py
+++ b/tests/test_traverse.py
@@ -746,6 +746,50 @@ class TestTraverse(BaseTest):
             """
         )
 
+    def test_traverse_with_merge_annotation(self, mocker: MockerFixture) -> None:
+        (
+            self.repo_sandbox.remove_remote()
+            .new_branch("develop")
+            .commit()
+            .new_branch("mars")
+            .commit()
+            .new_branch("snickers")
+            .commit()
+            .check_out("mars")
+            .commit()
+            .check_out("develop")
+            .commit()
+        )
+
+        body: str = """
+            develop
+                mars update=merge
+                    snickers
+            """
+        rewrite_branch_layout_file(body)
+
+        self.patch_symbol(mocker, "builtins.input", mock_input_returning("q"))
+        launch_command("traverse")
+        self.patch_symbol(mocker, "builtins.input", mock_input_returning("n", "q"))
+        launch_command("traverse")
+        self.patch_symbol(mocker, "builtins.input", mock_input_returning("yq"))
+        assert_success(
+            ["traverse", "--start-from=root", "--no-edit-merge"],
+            """
+            Checking out the root branch (develop)
+
+            Checking out mars
+
+              develop
+              |
+              x-mars *  update=merge
+                |
+                x-snickers
+
+            Merge develop into mars? (y, N, q, yq)
+            """,
+        )
+
     def test_traverse_qualifiers_no_push(self) -> None:
         self.setup_standard_tree()
 
diff --git a/tests/test_update.py b/tests/test_update.py
index 785ef55..4075656 100644
--- a/tests/test_update.py
+++ b/tests/test_update.py
@@ -278,12 +278,6 @@ class TestUpdate(BaseTest):
         self.repo_sandbox.write_to_file(".git/hooks/machete-pre-rebase", "#!/bin/sh\nexit 1")
         assert_failure(["update", "-n"], "The machete-pre-rebase hook refused to rebase. Error code: 1")
 
-    def test_update_no_edit_merge_but_without_merge(self) -> None:
-        assert_failure(
-            ['update', '--no-edit-merge'],
-            "Option --no-edit-merge only makes sense when using merge and must be specified together with -M/--merge."
-        )
-
     def test_update_no_interactive_rebase_with_merge(self) -> None:
         assert_failure(
             ['update', '--no-interactive-rebase', '--merge'],