From 9a821e85799adddd79d899012331f5f59871c428 Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Tue, 24 Oct 2023 17:10:51 +0200
Subject: [PATCH 01/10] First draft of plackback hook logic

---
 perfact/zodbsync/zodbsync.py | 145 +++++++++++++++++++++--------------
 1 file changed, 86 insertions(+), 59 deletions(-)

diff --git a/perfact/zodbsync/zodbsync.py b/perfact/zodbsync/zodbsync.py
index 9e36c90..d94f4a6 100644
--- a/perfact/zodbsync/zodbsync.py
+++ b/perfact/zodbsync/zodbsync.py
@@ -661,7 +661,7 @@ def _playback_fixorder(self, path):
         del self.fs_data[path]
 
     def playback_paths(self, paths, recurse=True, override=False,
-                       skip_errors=False, dryrun=False):
+                       skip_errors=False, dryrun=False, skip_postproc=False):
         self.recurse = recurse
         self.override = override
         self.skip_errors = skip_errors
@@ -689,67 +689,94 @@ def playback_paths(self, paths, recurse=True, override=False,
         self.num_obj_current = 0
         self.num_obj_total = len(paths)
 
-        note = 'zodbsync'
-        if len(paths) == 1:
-            note += ': ' + paths[0]
-        txn_mgr = self.start_transaction(note=note)
-
-        # Stack of paths that are to be played back (reversed alphabetical
-        # order, we pop elements from the top)
-        self.playback_todo = []
-        todo = self.playback_todo  # local alias
-
-        # Stack of paths for which we need to fix the order after everything
-        # below that path has been handled.
-        self.playback_fixorder = []
-        fixorder = self.playback_fixorder  # local alias
-
-        # Cached read metadata for paths that are in fixorder so we
-        # don't need to read it again from disk.
-        self.fs_data = {}
-
-        # Reverse order, ensure that paths end in '/' so startswith can be used
-        # reliably and remove elements that are to be deleted in that reverse
-        # order so properties of their parents can take their place
-        for path in reversed(paths):
-            if not os.path.isdir(self.fs_path(path)):
-                # Call it immediately, it will read None from the FS and remove
-                # the object
-                self._playback_path(path)
+        playback_hook = self.config.get('playback_hook', None)
+        if playback_hook and os.path.isfile(playback_hook):
+            proc = subprocess.Popen(
+                playback_hook, stdin=subprocess.PIPE,
+                stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+                universal_newlines=True)
+            out, _ = proc.communicate(json.dumps({'paths': paths}))
+            returncode = proc.returncode
+            if returncode:
+                raise AssertionError(
+                    "Error calling playback hook, returncode "
+                    "{}, [[{}]] on {}".format(
+                        returncode, playback_hook, out
+                    )
+                )
+            phases = json.loads(out)
+        else:
+            phases = [{'name': 'playback', 'paths': paths}]
+
+        for ix, phase in enumerate(phases):
+            note = 'zodbsync'
+            if phase.get('name'):
+                note += '/' + phase['name']
             else:
-                todo.append(path)
+                note += '/' + str(ix)
+            if len(phase['paths']) == 1:
+                note += ': ' + phase['paths'][0]
+            txn_mgr = self.start_transaction(note=note)
+
+            # Stack of paths that are to be played back (reversed
+            # alphabetical order, we pop elements from the top)
+            self.playback_todo = []
+            todo = self.playback_todo  # local alias
+
+            # Stack of paths for which we need to fix the order after
+            # everything below that path has been handled.
+            self.playback_fixorder = []
+            fixorder = self.playback_fixorder  # local alias
+
+            # Cached read metadata for paths that are in fixorder so we
+            # don't need to read it again from disk.
+            self.fs_data = {}
+
+            # Reverse order, ensure that paths end in '/' so startswith can be
+            # used reliably and remove elements that are to be deleted in that
+            # reverse order so properties of their parents can take their
+            # place
+            for path in reversed(phase['paths']):
+                if not os.path.isdir(self.fs_path(path)):
+                    # Call it immediately, it will read None from the FS and
+                    # remove the object
+                    self._playback_path(path)
+                else:
+                    todo.append(path)
 
-        # Iterate until both stacks are empty. Whenever the topmost element in
-        # todo is no longer a subelement of the topmost element of fixorder, we
-        # handle the fixorder element, otherwise we handle the todo element.
-        try:
-            while todo or fixorder:
-                if fixorder:
-                    # Handle next object on which to fix order unless there are
-                    # still subpaths to be handled
-                    path = fixorder[-1]
-                    if not (todo and todo[-1].startswith(path)):
-                        self._playback_fixorder(fixorder.pop())
-                        continue
-
-                path = todo.pop()
-                self._playback_path(path)
-        except Exception:
-            self.logger.exception('Error with path: ' + path)
-            txn_mgr.abort()
-            raise
+            # Iterate until both stacks are empty. Whenever the topmost element
+            # in todo is no longer a subelement of the topmost element of
+            # fixorder, we handle the fixorder element, otherwise we handle the
+            # todo element.
+            try:
+                while todo or fixorder:
+                    if fixorder:
+                        # Handle next object on which to fix order unless
+                        # there are still subpaths to be handled
+                        path = fixorder[-1]
+                        if not (todo and todo[-1].startswith(path)):
+                            self._playback_fixorder(fixorder.pop())
+                            continue
+
+                    path = todo.pop()
+                    self._playback_path(path)
+            except Exception:
+                self.logger.exception('Error with path: ' + path)
+                txn_mgr.abort()
+                raise
 
-        if dryrun:
-            self.logger.info('Dry-run. Rolling back')
-            txn_mgr.abort()
-        else:
-            txn_mgr.commit()
-            postproc = self.config.get('run_after_playback', None)
-            if postproc and os.path.isfile(postproc):
-                self.logger.info('Calling postprocessing script ' + postproc)
-                proc = subprocess.Popen(postproc, stdin=subprocess.PIPE,
-                                        universal_newlines=True)
-                proc.communicate(json.dumps({'paths': paths}))
+            if dryrun:
+                self.logger.info('Dry-run. Rolling back')
+                txn_mgr.abort()
+            else:
+                txn_mgr.commit()
+
+        postproc = self.config.get('run_after_playback', None)
+        if not skip_postproc and postproc and os.path.isfile(postproc):
+            self.logger.info('Calling postprocessing script ' + postproc)
+            proc = subprocess.Popen(postproc, stdin=subprocess.PIPE,
+                                    universal_newlines=True)
+            proc.communicate(json.dumps({'paths': paths}))
 
     def recent_changes(self, since_secs=None, txnid=None, limit=50,
                        search_limit=100):

From eff6d55e2081d42491621d37cf549a9a5e29a0cd Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Wed, 25 Oct 2023 09:30:15 +0200
Subject: [PATCH 02/10] First running test

---
 perfact/zodbsync/tests/test_sync.py | 48 ++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/perfact/zodbsync/tests/test_sync.py b/perfact/zodbsync/tests/test_sync.py
index 1081db4..c03f160 100644
--- a/perfact/zodbsync/tests/test_sync.py
+++ b/perfact/zodbsync/tests/test_sync.py
@@ -304,7 +304,7 @@ def prepare_pick(self, name='TestFolder', msg='Second commit'):
         initialized repository. Returns the commit ID.
         '''
         # Add a folder, commit it
-        self.add_folder('TestFolder', 'Second commit')
+        self.add_folder(name, msg)
         commit = self.get_head_id()
 
         # Reset the commit
@@ -1541,3 +1541,49 @@ def test_playback_postprocess(self):
         with open(self.config.path, 'w') as f:
             f.write(orig_config)
         del self.runner
+
+    def test_playback_hook(self):
+        """
+        Add configuration option for a playback hook script and check that
+        only the paths returned are played back
+        """
+        self.add_folder('NewFolder', 'First Folder')
+        commit = self.prepare_pick('NewFolder2', 'Second Folder')
+
+        playback_cmd = "{}/playback_cmd".format(self.zeo.path)
+        cmd_script = '\n'.join([
+            "#!/bin/bash"
+            "cat > {}"
+        ]).format('{}.out'.format(playback_cmd))
+        with open(playback_cmd, 'w') as f:
+            f.write(cmd_script)
+        os.chmod(cmd_script, 0o700)
+
+        fname = "{}/playback_hook".format(self.zeo.path)
+        playback_dict = [{
+            "paths": ["/NewFolder"],
+            "cmd": playback_cmd
+        }]
+
+        script = '\n'.join([
+            "#!/bin/bash",
+            "echo '{}'".format(json.dumps(playback_dict)),
+        ])
+        with open(fname, 'w') as f:
+            f.write(script)
+        os.chmod(fname, 0o700)
+        with open(self.config.path) as f:
+            orig_config = f.read()
+        with open(self.config.path, 'a') as f:
+            f.write('\nplayback_hook = "{}"\n'.format(fname))
+
+        # Avoid error regarding reusing runner with changed config
+        del self.runner
+        self.run('pick', commit)
+
+        assert 'NewFolder' in self.app.objectIds()
+        assert 'NewFolder2' not in self.app.objectIds()
+
+        with open(self.config.path, 'w') as f:
+            f.write(orig_config)
+        del self.runner

From eb70b1d48f60bd5365c0c5997802067e6d3a1355 Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Wed, 25 Oct 2023 15:55:20 +0200
Subject: [PATCH 03/10] Put command execution to gitexec decorator

---
 perfact/zodbsync/subcommand.py      |  78 +++++++++++---
 perfact/zodbsync/tests/test_sync.py |   5 +-
 perfact/zodbsync/zodbsync.py        | 157 ++++++++++++----------------
 3 files changed, 133 insertions(+), 107 deletions(-)

diff --git a/perfact/zodbsync/subcommand.py b/perfact/zodbsync/subcommand.py
index 9f1f53a..5b92310 100644
--- a/perfact/zodbsync/subcommand.py
+++ b/perfact/zodbsync/subcommand.py
@@ -5,6 +5,7 @@
 import os
 
 import filelock
+import json
 
 from .helpers import Namespace
 
@@ -150,6 +151,69 @@ def gitexec(func):
         - play back changed objects (diff between old and new HEAD)
         - unstash
         """
+        def _playback_paths(self, paths):
+            paths = self.sync.prepare_paths(paths)
+            dryrun = self.args.dry_run
+
+            playback_hook = self.config.get('playback_hook', None)
+            if playback_hook and os.path.isfile(playback_hook):
+                proc = subprocess.Popen(
+                    playback_hook, stdin=subprocess.PIPE,
+                    stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+                    universal_newlines=True)
+                out, _ = proc.communicate(json.dumps({'paths': paths}))
+                returncode = proc.returncode
+                if returncode:
+                    raise AssertionError(
+                        "Error calling playback hook, returncode "
+                        "{}, [[{}]] on {}".format(
+                            returncode, playback_hook, out
+                        )
+                    )
+                phases = json.loads(out)
+            else:
+                phases = [{'name': 'playback', 'paths': paths}]
+                if self.config.get('run_after_playback', None):
+                    phases[0]['cmd'] = self.config['run_after_playback']
+
+            for ix, phase in enumerate(phases):
+                phase_name = phase.get('name') or str(ix)
+                phase_cmd = phase.get('cmd')
+
+                self.sync.playback_paths(
+                    paths=phase['paths'],
+                    recurse=False,
+                    override=True,
+                    skip_errors=self.args.skip_errors,
+                    dryrun=dryrun,
+                )
+
+                if not dryrun and phase_cmd and os.path.isfile(phase_cmd):
+                    self.logger.info(
+                        'Calling command for Phase %s', phase_name
+                    )
+                    proc = subprocess.Popen(
+                        phase_cmd, stdin=subprocess.PIPE,
+                        stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+                        universal_newlines=True)
+                    out, _ = proc.communicate(json.dumps({'paths': paths}))
+                    returncode = proc.returncode
+
+                    if returncode:
+                        self.logger.error(
+                            "Error during phase command %s, %s",
+                            returncode, out
+                        )
+                        if sys.stdin.isatty():
+                            print("Enter 'y' to continue, other to rollback")
+                            res = input()
+                            if res == 'y':
+                                continue
+
+                        raise AssertionError(
+                            "Unrecoverable error in phase command"
+                        )
+
         @SubCommand.with_lock
         def wrapper(self, *args, **kwargs):
             # Check for unstaged changes
@@ -173,20 +237,10 @@ def wrapper(self, *args, **kwargs):
                 conflicts = files & set(self.unstaged_changes)
                 assert not conflicts, "Change in unstaged files, aborting"
 
-                # Strip site name from the start
-                files = [fname[len(self.sync.site):] for fname in files]
-                # Strip filename to get the object path
-                dirs = [fname.rsplit('/', 1)[0] for fname in files]
                 # Make unique and sort
-                paths = sorted(set(dirs))
+                paths = sorted(set(files))
 
-                self.sync.playback_paths(
-                    paths=paths,
-                    recurse=False,
-                    override=True,
-                    skip_errors=self.args.skip_errors,
-                    dryrun=self.args.dry_run,
-                )
+                _playback_paths(self, paths)
 
                 if self.args.dry_run:
                     self.abort()
diff --git a/perfact/zodbsync/tests/test_sync.py b/perfact/zodbsync/tests/test_sync.py
index c03f160..440b456 100644
--- a/perfact/zodbsync/tests/test_sync.py
+++ b/perfact/zodbsync/tests/test_sync.py
@@ -1552,12 +1552,12 @@ def test_playback_hook(self):
 
         playback_cmd = "{}/playback_cmd".format(self.zeo.path)
         cmd_script = '\n'.join([
-            "#!/bin/bash"
+            "#!/bin/bash",
             "cat > {}"
         ]).format('{}.out'.format(playback_cmd))
         with open(playback_cmd, 'w') as f:
             f.write(cmd_script)
-        os.chmod(cmd_script, 0o700)
+        os.chmod(playback_cmd, 0o700)
 
         fname = "{}/playback_hook".format(self.zeo.path)
         playback_dict = [{
@@ -1583,6 +1583,7 @@ def test_playback_hook(self):
 
         assert 'NewFolder' in self.app.objectIds()
         assert 'NewFolder2' not in self.app.objectIds()
+        assert os.path.isfile('{}.out'.format(playback_cmd))
 
         with open(self.config.path, 'w') as f:
             f.write(orig_config)
diff --git a/perfact/zodbsync/zodbsync.py b/perfact/zodbsync/zodbsync.py
index d94f4a6..090aa34 100644
--- a/perfact/zodbsync/zodbsync.py
+++ b/perfact/zodbsync/zodbsync.py
@@ -5,8 +5,6 @@
 import shutil
 import time  # for periodic output
 import sys
-import json
-import subprocess
 
 # for using an explicit transaction manager
 import transaction
@@ -660,11 +658,7 @@ def _playback_fixorder(self, path):
         object_handlers[fs_data['type']].fix_order(obj, fs_data)
         del self.fs_data[path]
 
-    def playback_paths(self, paths, recurse=True, override=False,
-                       skip_errors=False, dryrun=False, skip_postproc=False):
-        self.recurse = recurse
-        self.override = override
-        self.skip_errors = skip_errors
+    def prepare_paths(self, paths):
         # normalize paths - cut off filenames and the site name
         paths = {
             path.rsplit('/', 1)[0] if (
@@ -679,9 +673,17 @@ def playback_paths(self, paths, recurse=True, override=False,
         })
 
         if not len(paths):
-            return
+            return []
 
         paths = [path.rstrip('/') + '/' for path in paths]
+        return paths
+
+    def playback_paths(self, paths, recurse=True, override=False,
+                       skip_errors=False, dryrun=False):
+        self.recurse = recurse
+        self.override = override
+        self.skip_errors = skip_errors
+        paths = self.prepare_paths(paths)
 
         if recurse:
             paths = remove_redundant_paths(paths)
@@ -689,94 +691,63 @@ def playback_paths(self, paths, recurse=True, override=False,
         self.num_obj_current = 0
         self.num_obj_total = len(paths)
 
-        playback_hook = self.config.get('playback_hook', None)
-        if playback_hook and os.path.isfile(playback_hook):
-            proc = subprocess.Popen(
-                playback_hook, stdin=subprocess.PIPE,
-                stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-                universal_newlines=True)
-            out, _ = proc.communicate(json.dumps({'paths': paths}))
-            returncode = proc.returncode
-            if returncode:
-                raise AssertionError(
-                    "Error calling playback hook, returncode "
-                    "{}, [[{}]] on {}".format(
-                        returncode, playback_hook, out
-                    )
-                )
-            phases = json.loads(out)
-        else:
-            phases = [{'name': 'playback', 'paths': paths}]
-
-        for ix, phase in enumerate(phases):
-            note = 'zodbsync'
-            if phase.get('name'):
-                note += '/' + phase['name']
+        note = 'zodbsync/'
+        if len('paths') == 1:
+            note += ': ' + paths[0]
+        txn_mgr = self.start_transaction(note=note)
+
+        # Stack of paths that are to be played back (reversed
+        # alphabetical order, we pop elements from the top)
+        self.playback_todo = []
+        todo = self.playback_todo  # local alias
+
+        # Stack of paths for which we need to fix the order after
+        # everything below that path has been handled.
+        self.playback_fixorder = []
+        fixorder = self.playback_fixorder  # local alias
+
+        # Cached read metadata for paths that are in fixorder so we
+        # don't need to read it again from disk.
+        self.fs_data = {}
+
+        # Reverse order, ensure that paths end in '/' so startswith can be
+        # used reliably and remove elements that are to be deleted in that
+        # reverse order so properties of their parents can take their
+        # place
+        for path in reversed(paths):
+            if not os.path.isdir(self.fs_path(path)):
+                # Call it immediately, it will read None from the FS and
+                # remove the object
+                self._playback_path(path)
             else:
-                note += '/' + str(ix)
-            if len(phase['paths']) == 1:
-                note += ': ' + phase['paths'][0]
-            txn_mgr = self.start_transaction(note=note)
-
-            # Stack of paths that are to be played back (reversed
-            # alphabetical order, we pop elements from the top)
-            self.playback_todo = []
-            todo = self.playback_todo  # local alias
-
-            # Stack of paths for which we need to fix the order after
-            # everything below that path has been handled.
-            self.playback_fixorder = []
-            fixorder = self.playback_fixorder  # local alias
-
-            # Cached read metadata for paths that are in fixorder so we
-            # don't need to read it again from disk.
-            self.fs_data = {}
-
-            # Reverse order, ensure that paths end in '/' so startswith can be
-            # used reliably and remove elements that are to be deleted in that
-            # reverse order so properties of their parents can take their
-            # place
-            for path in reversed(phase['paths']):
-                if not os.path.isdir(self.fs_path(path)):
-                    # Call it immediately, it will read None from the FS and
-                    # remove the object
-                    self._playback_path(path)
-                else:
-                    todo.append(path)
+                todo.append(path)
 
-            # Iterate until both stacks are empty. Whenever the topmost element
-            # in todo is no longer a subelement of the topmost element of
-            # fixorder, we handle the fixorder element, otherwise we handle the
-            # todo element.
-            try:
-                while todo or fixorder:
-                    if fixorder:
-                        # Handle next object on which to fix order unless
-                        # there are still subpaths to be handled
-                        path = fixorder[-1]
-                        if not (todo and todo[-1].startswith(path)):
-                            self._playback_fixorder(fixorder.pop())
-                            continue
-
-                    path = todo.pop()
-                    self._playback_path(path)
-            except Exception:
-                self.logger.exception('Error with path: ' + path)
-                txn_mgr.abort()
-                raise
+        # Iterate until both stacks are empty. Whenever the topmost element
+        # in todo is no longer a subelement of the topmost element of
+        # fixorder, we handle the fixorder element, otherwise we handle the
+        # todo element.
+        try:
+            while todo or fixorder:
+                if fixorder:
+                    # Handle next object on which to fix order unless
+                    # there are still subpaths to be handled
+                    path = fixorder[-1]
+                    if not (todo and todo[-1].startswith(path)):
+                        self._playback_fixorder(fixorder.pop())
+                        continue
+
+                path = todo.pop()
+                self._playback_path(path)
+        except Exception:
+            self.logger.exception('Error with path: ' + path)
+            txn_mgr.abort()
+            raise
 
-            if dryrun:
-                self.logger.info('Dry-run. Rolling back')
-                txn_mgr.abort()
-            else:
-                txn_mgr.commit()
-
-        postproc = self.config.get('run_after_playback', None)
-        if not skip_postproc and postproc and os.path.isfile(postproc):
-            self.logger.info('Calling postprocessing script ' + postproc)
-            proc = subprocess.Popen(postproc, stdin=subprocess.PIPE,
-                                    universal_newlines=True)
-            proc.communicate(json.dumps({'paths': paths}))
+        if dryrun:
+            self.logger.info('Dry-run. Rolling back')
+            txn_mgr.abort()
+        else:
+            txn_mgr.commit()
 
     def recent_changes(self, since_secs=None, txnid=None, limit=50,
                        search_limit=100):

From 79e9aae1376b7a5bf88579f642b9ecc8477e265b Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Wed, 25 Oct 2023 17:22:34 +0200
Subject: [PATCH 04/10] Add rollback on failure

---
 perfact/zodbsync/subcommand.py      | 20 ++++++++--
 perfact/zodbsync/tests/test_sync.py | 59 ++++++++++++++++++++++++++++-
 2 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/perfact/zodbsync/subcommand.py b/perfact/zodbsync/subcommand.py
index 5b92310..ca32bbd 100644
--- a/perfact/zodbsync/subcommand.py
+++ b/perfact/zodbsync/subcommand.py
@@ -196,7 +196,9 @@ def _playback_paths(self, paths):
                         phase_cmd, stdin=subprocess.PIPE,
                         stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
                         universal_newlines=True)
-                    out, _ = proc.communicate(json.dumps({'paths': paths}))
+                    out, _ = proc.communicate(json.dumps(
+                        {'paths': phase['paths']}
+                    ))
                     returncode = proc.returncode
 
                     if returncode:
@@ -220,6 +222,7 @@ def wrapper(self, *args, **kwargs):
             self.check_repo()
 
             try:
+                self.paths = []
                 func(self, *args, **kwargs)
 
                 # Fail and roll back for any of the markers of an interrupted
@@ -238,9 +241,9 @@ def wrapper(self, *args, **kwargs):
                 assert not conflicts, "Change in unstaged files, aborting"
 
                 # Make unique and sort
-                paths = sorted(set(files))
+                self.paths = sorted(set(files))
 
-                _playback_paths(self, paths)
+                _playback_paths(self, self.paths)
 
                 if self.args.dry_run:
                     self.abort()
@@ -277,6 +280,17 @@ def wrapper(self, *args, **kwargs):
                         self.logger.exception("Unable to show diff")
 
                 self.abort()
+                # if we are not in dryrun we can't be sure we havent already
+                # committed some stuff to the data-fs so playback all paths
+                # abort
+                if not self.args.dry_run and self.paths:
+                    self.sync.playback_paths(
+                        paths=self.paths,
+                        recurse=False,
+                        override=True,
+                        skip_errors=True,
+                        dryrun=False,
+                    )
                 raise
 
         return wrapper
diff --git a/perfact/zodbsync/tests/test_sync.py b/perfact/zodbsync/tests/test_sync.py
index 440b456..682d092 100644
--- a/perfact/zodbsync/tests/test_sync.py
+++ b/perfact/zodbsync/tests/test_sync.py
@@ -1548,7 +1548,10 @@ def test_playback_hook(self):
         only the paths returned are played back
         """
         self.add_folder('NewFolder', 'First Folder')
-        commit = self.prepare_pick('NewFolder2', 'Second Folder')
+        self.add_folder('NewFolder2', 'Second Folder')
+        commit = self.get_head_id()
+        # Reset the commit
+        self.gitrun('reset', '--hard', 'HEAD~2')
 
         playback_cmd = "{}/playback_cmd".format(self.zeo.path)
         cmd_script = '\n'.join([
@@ -1579,7 +1582,7 @@ def test_playback_hook(self):
 
         # Avoid error regarding reusing runner with changed config
         del self.runner
-        self.run('pick', commit)
+        self.run('pick', 'HEAD..{}'.format(commit))
 
         assert 'NewFolder' in self.app.objectIds()
         assert 'NewFolder2' not in self.app.objectIds()
@@ -1588,3 +1591,55 @@ def test_playback_hook(self):
         with open(self.config.path, 'w') as f:
             f.write(orig_config)
         del self.runner
+
+    def test_playback_hook_failed(self):
+        """
+        Add configuration option for a playback hook script and check that
+        only the paths returned are played back
+        """
+        self.add_folder('NewFolder', 'First Folder')
+        self.add_folder('NewFolder2', 'Second Folder')
+        commit = self.get_head_id()
+        # Reset the commit
+        self.gitrun('reset', '--hard', 'HEAD~2')
+
+        playback_cmd = "{}/playback_cmd".format(self.zeo.path)
+        cmd_script = '\n'.join([
+            "#!/bin/bash",
+            "exit 42"
+        ])
+        with open(playback_cmd, 'w') as f:
+            f.write(cmd_script)
+        os.chmod(playback_cmd, 0o700)
+
+        fname = "{}/playback_hook".format(self.zeo.path)
+        playback_dict = [{
+            "paths": ["/NewFolder"],
+            "cmd": playback_cmd
+            }, {
+            "paths": ["/NewFolder2"],
+            },
+        ]
+        script = '\n'.join([
+            "#!/bin/bash",
+            "echo '{}'".format(json.dumps(playback_dict)),
+        ])
+        with open(fname, 'w') as f:
+            f.write(script)
+        os.chmod(fname, 0o700)
+        with open(self.config.path) as f:
+            orig_config = f.read()
+        with open(self.config.path, 'a') as f:
+            f.write('\nplayback_hook = "{}"\n'.format(fname))
+
+        # Avoid error regarding reusing runner with changed config
+        del self.runner
+        with pytest.raises(AssertionError):
+            self.run('pick', 'HEAD..{}'.format(commit))
+
+        assert 'NewFolder' not in self.app.objectIds()
+        assert 'NewFolder2' not in self.app.objectIds()
+
+        with open(self.config.path, 'w') as f:
+            f.write(orig_config)
+        del self.runner

From 5051274afb410ae6d38b69d76b36f369a57cf118 Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Wed, 25 Oct 2023 17:27:29 +0200
Subject: [PATCH 05/10] Restore old formatting in zodbsync.py

---
 perfact/zodbsync/zodbsync.py | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/perfact/zodbsync/zodbsync.py b/perfact/zodbsync/zodbsync.py
index 090aa34..613e048 100644
--- a/perfact/zodbsync/zodbsync.py
+++ b/perfact/zodbsync/zodbsync.py
@@ -691,18 +691,18 @@ def playback_paths(self, paths, recurse=True, override=False,
         self.num_obj_current = 0
         self.num_obj_total = len(paths)
 
-        note = 'zodbsync/'
-        if len('paths') == 1:
+        note = 'zodbsync'
+        if len(paths) == 1:
             note += ': ' + paths[0]
         txn_mgr = self.start_transaction(note=note)
 
-        # Stack of paths that are to be played back (reversed
-        # alphabetical order, we pop elements from the top)
+        # Stack of paths that are to be played back (reversed alphabetical
+        # order, we pop elements from the top)
         self.playback_todo = []
         todo = self.playback_todo  # local alias
 
-        # Stack of paths for which we need to fix the order after
-        # everything below that path has been handled.
+        # Stack of paths for which we need to fix the order after everything
+        # below that path has been handled.
         self.playback_fixorder = []
         fixorder = self.playback_fixorder  # local alias
 
@@ -710,27 +710,25 @@ def playback_paths(self, paths, recurse=True, override=False,
         # don't need to read it again from disk.
         self.fs_data = {}
 
-        # Reverse order, ensure that paths end in '/' so startswith can be
-        # used reliably and remove elements that are to be deleted in that
-        # reverse order so properties of their parents can take their
-        # place
+        # Reverse order, ensure that paths end in '/' so startswith can be used
+        # reliably and remove elements that are to be deleted in that reverse
+        # order so properties of their parents can take their place
         for path in reversed(paths):
             if not os.path.isdir(self.fs_path(path)):
-                # Call it immediately, it will read None from the FS and
-                # remove the object
+                # Call it immediately, it will read None from the FS and remove
+                # the object
                 self._playback_path(path)
             else:
                 todo.append(path)
 
-        # Iterate until both stacks are empty. Whenever the topmost element
-        # in todo is no longer a subelement of the topmost element of
-        # fixorder, we handle the fixorder element, otherwise we handle the
-        # todo element.
+        # Iterate until both stacks are empty. Whenever the topmost element in
+        # todo is no longer a subelement of the topmost element of fixorder, we
+        # handle the fixorder element, otherwise we handle the todo element.
         try:
             while todo or fixorder:
                 if fixorder:
-                    # Handle next object on which to fix order unless
-                    # there are still subpaths to be handled
+                    # Handle next object on which to fix order unless there are
+                    # still subpaths to be handled
                     path = fixorder[-1]
                     if not (todo and todo[-1].startswith(path)):
                         self._playback_fixorder(fixorder.pop())

From 9042f9bb8006528d14b89c40aa8e1eda08f6d52d Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Fri, 27 Oct 2023 10:45:38 +0200
Subject: [PATCH 06/10] Also show output of phase script when not failing

---
 perfact/zodbsync/subcommand.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/perfact/zodbsync/subcommand.py b/perfact/zodbsync/subcommand.py
index ca32bbd..c86fcce 100644
--- a/perfact/zodbsync/subcommand.py
+++ b/perfact/zodbsync/subcommand.py
@@ -215,6 +215,8 @@ def _playback_paths(self, paths):
                         raise AssertionError(
                             "Unrecoverable error in phase command"
                         )
+                    else:
+                        self.logger.info(out)
 
         @SubCommand.with_lock
         def wrapper(self, *args, **kwargs):

From f28008d77aad435c6537105a3420c0480985f2a5 Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Fri, 12 Jan 2024 12:16:58 +0100
Subject: [PATCH 07/10] Adjust formatting

---
 perfact/zodbsync/subcommand.py      | 2 +-
 perfact/zodbsync/tests/test_sync.py | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/perfact/zodbsync/subcommand.py b/perfact/zodbsync/subcommand.py
index c86fcce..7014771 100644
--- a/perfact/zodbsync/subcommand.py
+++ b/perfact/zodbsync/subcommand.py
@@ -190,7 +190,7 @@ def _playback_paths(self, paths):
 
                 if not dryrun and phase_cmd and os.path.isfile(phase_cmd):
                     self.logger.info(
-                        'Calling command for Phase %s', phase_name
+                        'Calling phase %s, command: %s', phase_name, phase_cmd
                     )
                     proc = subprocess.Popen(
                         phase_cmd, stdin=subprocess.PIPE,
diff --git a/perfact/zodbsync/tests/test_sync.py b/perfact/zodbsync/tests/test_sync.py
index 682d092..9f556d2 100644
--- a/perfact/zodbsync/tests/test_sync.py
+++ b/perfact/zodbsync/tests/test_sync.py
@@ -1594,8 +1594,8 @@ def test_playback_hook(self):
 
     def test_playback_hook_failed(self):
         """
-        Add configuration option for a playback hook script and check that
-        only the paths returned are played back
+        Add configuration option for a playback hook script with a
+        failing cmd and check that all changes are rolled back
         """
         self.add_folder('NewFolder', 'First Folder')
         self.add_folder('NewFolder2', 'Second Folder')

From 48253e7e6940facf417340dc4d2deb82d1d59bb4 Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Mon, 29 Jan 2024 14:37:20 +0100
Subject: [PATCH 08/10] Add entry in changelog, readme and config.py

---
 CHANGELOG |  1 +
 README.md | 17 +++++++++++++++++
 config.py |  4 ++++
 3 files changed, 22 insertions(+)

diff --git a/CHANGELOG b/CHANGELOG
index 2b059b9..5d670cb 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 unreleased
   * Update README
   * Explicitly name git branch in tests
+  * Add configuration option playback_hook
 
 22.2.5
   * Omit title attributes if they are callable.
diff --git a/README.md b/README.md
index 90c7072..2a960c7 100644
--- a/README.md
+++ b/README.md
@@ -84,6 +84,23 @@ Path to a script that is executed after a successful (non-recursive) playback,
 including indirect calls from `reset` or `pick`. If the script exists, it is
 called and fed the list of changed objects in a JSON format.
 
+### `playback_hook`
+Path to script which is called to define the phases of playback to be
+executed, Recieves a json dictionary in the form of `{"paths": [...]}`
+and should output a json dictionary in the form of
+
+```json
+[
+  {
+    "paths": [...],
+    "cmd": [...]
+  },
+  {
+    "paths": [...],
+  }
+]
+```
+
 ## Usage
 
 The executable `zodbsync` provides several subcommands
diff --git a/config.py b/config.py
index f2534f6..4f348e5 100644
--- a/config.py
+++ b/config.py
@@ -31,5 +31,9 @@
 #codechange_mail = "zope-devel@example.de"
 #codechange_sender = "no-reply-zodbsync-changes@example.de"
 
+# Path to script which is called to define the phases of playback to be
+# executed.
+# playback_hook = '/usr/share/perfact/zope4-tools/zodbsync-playback-hook'
+
 # Path to script that is called for postprocessing after a playback if it exists
 # run_after_playback = '/usr/share/perfact/zope4-tools/zodbsync-postproc'

From 080dbc24fe4833fc2bd4d8091070dc83f7174c52 Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Mon, 29 Jan 2024 15:14:53 +0100
Subject: [PATCH 09/10] Refactoring of gitexec after review

---
 perfact/zodbsync/subcommand.py | 137 +++++++++++++++++----------------
 1 file changed, 70 insertions(+), 67 deletions(-)

diff --git a/perfact/zodbsync/subcommand.py b/perfact/zodbsync/subcommand.py
index 7014771..1f2e1d5 100644
--- a/perfact/zodbsync/subcommand.py
+++ b/perfact/zodbsync/subcommand.py
@@ -140,6 +140,75 @@ def check_repo(self):
         # The commit to compare to with regards to changed files
         self.orig_commit = self.branches[self.orig_branch]
 
+    def _playback_paths(self, paths):
+        paths = self.sync.prepare_paths(paths)
+        dryrun = self.args.dry_run
+
+        playback_hook = self.config.get('playback_hook', None)
+        if playback_hook and os.path.isfile(playback_hook):
+            proc = subprocess.Popen(
+                playback_hook, stdin=subprocess.PIPE,
+                stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+                universal_newlines=True)
+            out, _ = proc.communicate(json.dumps({'paths': paths}))
+            returncode = proc.returncode
+            if returncode:
+                raise AssertionError(
+                    "Error calling playback hook, returncode "
+                    "{}, [[{}]] on {}".format(
+                        returncode, playback_hook, out
+                    )
+                )
+            phases = json.loads(out)
+        else:
+            phases = [{'name': 'playback', 'paths': paths}]
+            if self.config.get('run_after_playback', None):
+                phases[-1]['cmd'] = self.config['run_after_playback']
+
+        for ix, phase in enumerate(phases):
+            phase_name = phase.get('name') or str(ix)
+            phase_cmd = phase.get('cmd')
+
+            self.sync.playback_paths(
+                paths=phase['paths'],
+                recurse=False,
+                override=True,
+                skip_errors=self.args.skip_errors,
+                dryrun=dryrun,
+            )
+
+            if dryrun or not (phase_cmd and os.path.isfile(phase_cmd)):
+                continue
+
+            self.logger.info(
+                'Calling phase %s, command: %s', phase_name, phase_cmd
+            )
+            proc = subprocess.Popen(
+                phase_cmd, stdin=subprocess.PIPE,
+                stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+                universal_newlines=True)
+            out, _ = proc.communicate(json.dumps(
+                {'paths': phase['paths']}
+            ))
+            returncode = proc.returncode
+
+            if returncode:
+                self.logger.error(
+                    "Error during phase command %s, %s",
+                    returncode, out
+                )
+                if sys.stdin.isatty():
+                    print("Enter 'y' to continue, other to rollback")
+                    res = input()
+                    if res == 'y':
+                        continue
+
+                raise AssertionError(
+                    "Unrecoverable error in phase command"
+                )
+            else:
+                self.logger.info(out)
+
     @staticmethod
     def gitexec(func):
         """
@@ -151,72 +220,6 @@ def gitexec(func):
         - play back changed objects (diff between old and new HEAD)
         - unstash
         """
-        def _playback_paths(self, paths):
-            paths = self.sync.prepare_paths(paths)
-            dryrun = self.args.dry_run
-
-            playback_hook = self.config.get('playback_hook', None)
-            if playback_hook and os.path.isfile(playback_hook):
-                proc = subprocess.Popen(
-                    playback_hook, stdin=subprocess.PIPE,
-                    stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-                    universal_newlines=True)
-                out, _ = proc.communicate(json.dumps({'paths': paths}))
-                returncode = proc.returncode
-                if returncode:
-                    raise AssertionError(
-                        "Error calling playback hook, returncode "
-                        "{}, [[{}]] on {}".format(
-                            returncode, playback_hook, out
-                        )
-                    )
-                phases = json.loads(out)
-            else:
-                phases = [{'name': 'playback', 'paths': paths}]
-                if self.config.get('run_after_playback', None):
-                    phases[0]['cmd'] = self.config['run_after_playback']
-
-            for ix, phase in enumerate(phases):
-                phase_name = phase.get('name') or str(ix)
-                phase_cmd = phase.get('cmd')
-
-                self.sync.playback_paths(
-                    paths=phase['paths'],
-                    recurse=False,
-                    override=True,
-                    skip_errors=self.args.skip_errors,
-                    dryrun=dryrun,
-                )
-
-                if not dryrun and phase_cmd and os.path.isfile(phase_cmd):
-                    self.logger.info(
-                        'Calling phase %s, command: %s', phase_name, phase_cmd
-                    )
-                    proc = subprocess.Popen(
-                        phase_cmd, stdin=subprocess.PIPE,
-                        stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-                        universal_newlines=True)
-                    out, _ = proc.communicate(json.dumps(
-                        {'paths': phase['paths']}
-                    ))
-                    returncode = proc.returncode
-
-                    if returncode:
-                        self.logger.error(
-                            "Error during phase command %s, %s",
-                            returncode, out
-                        )
-                        if sys.stdin.isatty():
-                            print("Enter 'y' to continue, other to rollback")
-                            res = input()
-                            if res == 'y':
-                                continue
-
-                        raise AssertionError(
-                            "Unrecoverable error in phase command"
-                        )
-                    else:
-                        self.logger.info(out)
 
         @SubCommand.with_lock
         def wrapper(self, *args, **kwargs):
@@ -245,7 +248,7 @@ def wrapper(self, *args, **kwargs):
                 # Make unique and sort
                 self.paths = sorted(set(files))
 
-                _playback_paths(self, self.paths)
+                self._playback_paths(self.paths)
 
                 if self.args.dry_run:
                     self.abort()

From e41ea0cf72ad2cda924149b5049ae5686b385df7 Mon Sep 17 00:00:00 2001
From: Alexander Rolfes <alexander.rolfes@perfact.de>
Date: Mon, 29 Jan 2024 15:23:12 +0100
Subject: [PATCH 10/10] Update workflow to use latest  major version of actions

---
 .github/workflows/check.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml
index 1d19c6a..5a7a6c5 100644
--- a/.github/workflows/check.yml
+++ b/.github/workflows/check.yml
@@ -13,9 +13,9 @@ jobs:
         python-version: ['3.8', '3.9', '3.10']
 
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
     - name: Set up Python ${{ matrix.python-version }}
-      uses: actions/setup-python@v4
+      uses: actions/setup-python@v5
       with:
         python-version: ${{ matrix.python-version }}
     - name: Install dependencies