feat: improve merge behavior (#70)

When merging, we use the `sync(prompt)` commit as parent to prevent
inaccurate conflicts due to prior changes.

Author: mtth

docs/git-draft.adoc

@@ -74,10 +74,6 @@ git draft [options] --show-templates [--json | [--edit] TEMPLATE]
   Lists available templates.
   With an template name argument, displays the corresponding template's contents or, if the `--edit` option is set, opens an interactive editor.
 
--t TIMEOUT::
---timeout=TIMEOUT::
-  Action timeout.
-
 --version::
   Show version and exit.
 

src/git_draft/__main__.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 from collections.abc import Sequence
+import enum
 import importlib.metadata
 import logging
 import optparse
@@ -11,7 +12,7 @@
 
 from .bots import load_bot
 from .common import PROGRAM, Config, UnreachableError, ensure_state_home
-from .drafter import Accept, Drafter
+from .drafter import Drafter, DraftMergeStrategy
 from .editor import open_editor
 from .git import Repo
 from .prompt import Template, TemplatedPrompt, find_template, templates_table
@@ -95,15 +96,28 @@ def callback(
         action="store_const",
         const=0,
     )
-    parser.add_option(
-        "--timeout",
-        dest="timeout",
-        help="generation timeout",
-    )
 
     return parser
 
 
+class Accept(enum.Enum):
+    """Valid change accept mode"""
+
+    MANUAL = 0
+    MERGE = enum.auto()
+    MERGE_THEIRS = enum.auto()
+    FINALIZE = enum.auto()
+
+    def merge_strategy(self) -> DraftMergeStrategy | None:
+        match self.value:
+            case Accept.MANUAL:
+                return None
+            case Accept.MERGE:
+                return "ignore-all-space"
+            case _:
+                return "theirs"
+
+
 class ToolPrinter(ToolVisitor):
     """Visitor implementation which prints invocations to stdout"""
 
@@ -199,17 +213,17 @@ def main() -> None:  # noqa: PLR0912 PLR0915
             drafter.generate_draft(
                 prompt,
                 bot,
-                accept=accept,
-                bot_name=opts.bot,
                 prompt_transform=open_editor if editable else None,
+                merge_strategy=accept.merge_strategy(),
                 tool_visitors=[ToolPrinter()],
             )
             match accept:
                 case Accept.MANUAL:
                     print("Generated draft.")
-                case Accept.MERGE:
+                case Accept.MERGE | Accept.MERGE_THEIRS:
                     print("Merged draft.")
                 case Accept.FINALIZE:
+                    drafter.finalize_folio()
                     print("Finalized draft.")
                 case _:
                     raise UnreachableError()

src/git_draft/bots/common.py

@@ -14,7 +14,7 @@ class Goal:
     """Bot request"""
 
     prompt: str
-    timeout: float | None
+    # TODO: Add timeout.
 
 
 @dataclasses.dataclass

src/git_draft/drafter.py

@@ -5,13 +5,13 @@
 from collections.abc import Callable, Sequence
 import dataclasses
 from datetime import datetime, timedelta
-import enum
 import json
 import logging
 from pathlib import PurePosixPath
 import re
 import textwrap
 import time
+from typing import Literal
 
 from .bots import Action, Bot, Goal
 from .common import JSONObject, qualified_class_name
@@ -24,14 +24,6 @@
 _logger = logging.getLogger(__name__)
 
 
-class Accept(enum.Enum):
-    """Valid change accept mode"""
-
-    MANUAL = 0
-    MERGE = enum.auto()
-    FINALIZE = enum.auto()
-
-
 @dataclasses.dataclass(frozen=True)
 class Draft:
     """Generated changes"""
@@ -81,6 +73,18 @@ def _active_folio(repo: Repo) -> Folio | None:
     return Folio(int(match[1]))
 
 
+#: Select ort strategies.
+DraftMergeStrategy = Literal[
+    "ours",
+    "theirs",
+    "ignore-space-change",
+    "ignore-all-space",
+    "ignore-space-at-eol",
+    "ignore-cr-at-eol",
+    "find-renames",
+]
+
+
 class Drafter:
     """Draft state orchestrator"""
 
@@ -94,19 +98,14 @@ def create(cls, repo: Repo, store: Store) -> Drafter:
             cursor.executescript(sql("create-tables"))
         return cls(store, repo)
 
-    def generate_draft(  # noqa: PLR0913
+    def generate_draft(
         self,
         prompt: str | TemplatedPrompt,
         bot: Bot,
-        accept: Accept = Accept.MANUAL,
-        bot_name: str | None = None,
+        merge_strategy: DraftMergeStrategy | None = None,
         prompt_transform: Callable[[str], str] | None = None,
-        timeout: float | None = None,
         tool_visitors: Sequence[ToolVisitor] | None = None,
     ) -> Draft:
-        if timeout is not None:
-            raise NotImplementedError()  # TODO: Implement
-
         # Handle prompt templating and editing. We do this first in case this
         # fails, to avoid creating unnecessary branches.
         toolbox, dirty = RepoToolbox.for_working_dir(self._repo)
@@ -134,7 +133,7 @@ def generate_draft(  # noqa: PLR0913
         operation_recorder = _OperationRecorder()
         change = self._generate_change(
             bot,
-            Goal(prompt_contents, timeout),
+            Goal(prompt_contents),
             toolbox.with_visitors(
                 [operation_recorder, *list(tool_visitors or [])],
             ),
@@ -159,7 +158,6 @@ def generate_draft(  # noqa: PLR0913
                 {
                     "commit_sha": commit_sha,
                     "prompt_id": prompt_id,
-                    "bot_name": bot_name,
                     "bot_class": qualified_class_name(bot.__class__),
                     "walltime_seconds": change.walltime.total_seconds(),
                     "request_count": change.action.request_count,
@@ -181,18 +179,21 @@ def generate_draft(  # noqa: PLR0913
             )
         _logger.info("Created new change in folio %s.", folio.id)
 
-        if accept.value >= Accept.MERGE.value:
+        if merge_strategy:
+            if parent_commit_rev != "HEAD":
+                # If there was a sync(prompt) commit, we move forward to it.
+                # This will avoid conflicts with changes that happened earlier.
+                self._repo.git("reset", "--soft", parent_commit_rev)
             self._sync_head("merge")
             self._repo.git(
                 "merge",
                 "--no-ff",
-                "-Xtheirs",
+                "-X",
+                merge_strategy,
                 "-m",
                 "draft! merge",
                 commit_sha,
             )
-        if accept.value >= Accept.FINALIZE.value:
-            self.finalize_folio()
 
         return Draft(
             folio=folio,

src/git_draft/git.py

@@ -50,9 +50,10 @@ def sync(
         stdout, stderr = popen.communicate(input=stdin)
         code = popen.returncode
         if expect_codes and code not in expect_codes:
-            raise GitError(
-                f"Git command failed with code {code}\n{stderr}\n{stdout}"
-            )
+            message = f"Git command failed with code exit {code}: {stderr}"
+            if stdout:
+                message += f"\n{stdout}"
+            raise GitError(message)
         return cls(code, stdout.rstrip(), stderr.rstrip())
 
 

src/git_draft/queries/add-action.sql

@@ -1,15 +1,13 @@
 insert into actions (
     commit_sha,
     prompt_id,
-    bot_name,
     bot_class,
     walltime_seconds,
     request_count,
     token_count)
   values (
     :commit_sha,
     :prompt_id,
-    :bot_name,
     :bot_class,
     :walltime_seconds,
     :request_count,

src/git_draft/queries/create-tables.sql

@@ -23,7 +23,6 @@ create table if not exists actions (
   commit_sha text primary key,
   created_at timestamp default current_timestamp,
   prompt_id integer not null,
-  bot_name text,
   bot_class text not null,
   walltime_seconds real not null,
   request_count int,

tests/git_draft/drafter_test.py

@@ -6,7 +6,7 @@
 
 from git_draft.bots import Action, Bot, Goal, Toolbox
 import git_draft.drafter as sut
-from git_draft.git import SHA, Repo
+from git_draft.git import SHA, GitError, Repo
 from git_draft.store import Store
 
 from .conftest import RepoFS
@@ -71,28 +71,61 @@ def test_generate_empty_draft(self) -> None:
         assert len(self._commits()) == 1
         assert len(self._commits("@{u}")) == 2
 
-    def test_generate_draft_accept_merge(self) -> None:
+    def test_generate_draft_merge(self) -> None:
         self._fs.write("p1", "a")
+
         self._drafter.generate_draft(
-            "hello",
-            _SimpleBot({"p2": "b"}),
-            accept=sut.Accept.MERGE,
+            "hello", _SimpleBot({"p2": "b"}), merge_strategy="ignore-all-space"
         )
-        assert len(self._commits()) == 5  # init, sync, prompt, sync, merge
+        # No sync(merge) commit since no changes happened between.
+        assert len(self._commits()) == 4  # init, sync(prompt), prompt, merge
         assert self._fs.read("p1") == "a"
         assert self._fs.read("p2") == "b"
 
-    def test_generate_draft_accept_finalize(self) -> None:
+    def test_generate_draft_merge_no_conflict(self) -> None:
         self._fs.write("p1", "a")
+
+        def update(_goal: Goal) -> str:
+            self._fs.write("p2", "b")
+            return "A"
+
         self._drafter.generate_draft(
             "hello",
-            _SimpleBot({"p1": "A", "p2": "b"}),
-            accept=sut.Accept.FINALIZE,
+            _SimpleBot({"p1": update}),
+            merge_strategy="ignore-all-space",
         )
-        assert len(self._commits()) == 1  # init
+        assert len(self._commits()) == 5  # init, sync, prompt, sync, merge
         assert self._fs.read("p1") == "A"
         assert self._fs.read("p2") == "b"
 
+    def test_generate_draft_merge_theirs(self) -> None:
+        self._fs.write("p1", "a")
+
+        def update(_goal: Goal) -> str:
+            self._fs.write("p1", "b")
+            return "A"
+
+        self._drafter.generate_draft(
+            "hello", _SimpleBot({"p1": update}), merge_strategy="theirs"
+        )
+        # sync(merge) commit here since p1 was updated separately.
+        assert len(self._commits()) == 5  # init, sync, prompt, sync, merge
+        assert self._fs.read("p1") == "A"
+
+    def test_generate_draft_merge_conflict(self) -> None:
+        self._fs.write("p1", "a")
+
+        def update(_goal: Goal) -> str:
+            self._fs.write("p1", "b")
+            return "A"
+
+        with pytest.raises(GitError):
+            self._drafter.generate_draft(
+                "hello",
+                _SimpleBot({"p1": update}),
+                merge_strategy="ignore-all-space",
+            )
+
     def test_generate_outside_branch(self) -> None:
         self._repo.git("checkout", "--detach")
         with pytest.raises(RuntimeError):
@@ -104,18 +137,16 @@ def test_generate_empty_prompt(self) -> None:
 
     def test_generate_reuse_branch(self) -> None:
         bot = _SimpleBot({"prompt": lambda goal: goal.prompt})
-        self._drafter.generate_draft("prompt1", bot, sut.Accept.MERGE)
-        self._drafter.generate_draft("prompt2", bot, sut.Accept.MERGE)
+        self._drafter.generate_draft("prompt1", bot, "theirs")
+        self._drafter.generate_draft("prompt2", bot, "theirs")
         assert self._fs.read("prompt") == "prompt2"
 
     def test_delete_unknown_file(self) -> None:
         self._drafter.generate_draft("hello", _SimpleBot({"p1": None}))
 
     def test_finalize_keeps_changes(self) -> None:
         self._fs.write("p1.txt", "a1")
-        self._drafter.generate_draft(
-            "hello", _SimpleBot.prompt(), sut.Accept.MERGE
-        )
+        self._drafter.generate_draft("hello", _SimpleBot.prompt(), "theirs")
         self._fs.write("p1.txt", "a2")
         self._drafter.finalize_folio()
         assert self._fs.read("p1.txt") == "a2"