fix: roll back staged rename when a step fails

Wrap the staging and commit in Page.rename so a failure after the rename
has been staged (e.g. during backlink rewriting) resets the repository to
HEAD instead of leaving dangling uncommitted changes.

Author: redimp

otterwiki/gitstorage.py

@@ -422,6 +422,18 @@ def commit(self, filenames, message="", author=("", ""), no_add=False):
         if repo_manager:
             repo_manager.auto_push_if_enabled()
 
+    def reset(self, revision="HEAD"):
+        """Discard staged and working-tree changes, restoring the repository
+        to `revision` (HEAD by default). Used to roll back a partially
+        applied, uncommitted operation so the repository is never left with
+        dangling changes.
+        """
+        self._validate_revision(revision)
+        try:
+            self.repo.git.reset("--hard", revision)
+        except git.exc.GitCommandError as e:
+            raise StorageError("Reset to {} failed: {}.".format(revision, e))
+
     def revert(self, revision, message="", author=("", "")):
         self._validate_revision(revision)
         actor = git.Actor(author[0], author[1])

otterwiki/wiki.py

@@ -1008,42 +1008,51 @@ def rename(self, new_pagename, message, author, update_backlinks):
         # (e.g. the target already exists), and we must not rewrite backlinks
         # in other pages before the rename itself has succeeded - otherwise a
         # failure would leave those rewrites staged but uncommitted.
-        changed_files = []
-        if (len(files) + len(directories)) > 0:
-            # rename attachment directory
-            new_attachment_directoryname = get_attachment_directoryname(
-                new_filename
-            )
-            storage.rename(
-                self.attachment_directoryname,
-                new_attachment_directoryname,
-                author=author,
-                message=message,
-                no_commit=True,
-            )
-            changed_files.append(new_attachment_directoryname)
-        # rename page
-        if self.exists:
-            storage.rename(
-                self.filename,
-                new_filename,
-                message=message,
-                author=author,
-                no_commit=True,
+        #
+        # Everything up to and including the commit is staged in the working
+        # tree; if any step raises we roll the repository back to HEAD so a
+        # partially applied rename never leaves dangling changes behind.
+        try:
+            changed_files = []
+            if (len(files) + len(directories)) > 0:
+                # rename attachment directory
+                new_attachment_directoryname = get_attachment_directoryname(
+                    new_filename
+                )
+                storage.rename(
+                    self.attachment_directoryname,
+                    new_attachment_directoryname,
+                    author=author,
+                    message=message,
+                    no_commit=True,
+                )
+                changed_files.append(new_attachment_directoryname)
+            # rename page
+            if self.exists:
+                storage.rename(
+                    self.filename,
+                    new_filename,
+                    message=message,
+                    author=author,
+                    no_commit=True,
+                )
+                changed_files.append(new_filename)
+
+            # Now that the rename has succeeded, rewrite backlinks in other
+            # pages.
+            pages_updated = (
+                rename_backlinks(self.filename, new_pagename)
+                if update_backlinks
+                else {}
             )
-            changed_files.append(new_filename)
-
-        # Now that the rename has succeeded, rewrite backlinks in other pages.
-        pages_updated = (
-            rename_backlinks(self.filename, new_pagename)
-            if update_backlinks
-            else {}
-        )
-        changed_files += list(pages_updated.keys())
-
-        # Commit the rename together with the rewritten backlinks, so the
-        # repository is never left with uncommitted changes.
-        storage.commit(changed_files, message, author, no_add=True)
+            changed_files += list(pages_updated.keys())
+
+            # Commit the rename together with the rewritten backlinks, so the
+            # repository is never left with uncommitted changes.
+            storage.commit(changed_files, message, author, no_add=True)
+        except Exception:
+            storage.reset()
+            raise
 
         # notify plugins of backlink pages updated
         for pagepath, content in pages_updated.items():

tests/test_rename_with_updated_links.py

@@ -111,6 +111,54 @@ def test_rename_leaves_no_uncommitted_changes(test_client):
     )
 
 
+def test_rename_rolls_back_when_backlink_rewrite_fails(
+    test_client, monkeypatch
+):
+    """If any step fails after the rename has been staged, the repository
+    must be rolled back to its previous state - neither the staged rename nor
+    the partially rewritten backlinks may be left in the working tree."""
+    storage = test_client.application.storage
+    save_shortcut(
+        test_client, "RollbackTarget", "# RollbackTarget\n", "created target"
+    )
+    save_shortcut(
+        test_client,
+        "RollbackLinker",
+        "# RollbackLinker\n\n[a link](/RollbackTarget)\n",
+        "created linker",
+    )
+    head_before = _git_show(storage, "rollbacktarget.md")
+
+    def boom(*args, **kwargs):
+        raise RuntimeError("simulated backlink rewrite failure")
+
+    monkeypatch.setattr("otterwiki.wiki.rename_backlinks", boom)
+
+    # the rename must fail gracefully (handle_rename catches and toasts)
+    rv = test_client.post(
+        "/RollbackTarget/rename",
+        data={
+            "new_pagename": "RollbackRenamed",
+            "message": "",
+            "update_backlinks": "1",
+        },
+        follow_redirects=True,
+    )
+    assert rv.status_code == 200
+
+    # the repository must be clean ...
+    status = _git_status(storage)
+    assert status == "", (
+        "repository left dirty after failed rename; uncommitted changes:\n"
+        + status
+    )
+    # ... the original page must still exist under its old name ...
+    assert storage.exists("rollbacktarget.md")
+    assert not storage.exists("rollbackrenamed.md")
+    # ... and the backlink must be untouched.
+    assert _git_show(storage, "rollbacktarget.md") == head_before
+
+
 def test_rename_updates_percent_encoded_markdown_link(test_client):
     """Markdown links to a page with a space in its name are commonly
     written percent-encoded, e.g. [a link](/Target%20Page). Renaming