On 24/04/2019 23:46, brian m. carlson wrote:
On Wed, Apr 24, 2019 at 10:51:56AM +0100, Phillip Wood wrote:
On 24/04/2019 01:49, brian m. carlson wrote:
Add support for multiple post-rewrite hooks, both for "git commit
--amend" and "git rebase".
Additionally add support for multiple prepare-commit-msg hooks.
Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx>
---
builtin/am.c | 28 ++++++---
Having read the patch subject I was surprised to see this touching
bulitin/am.c
I can rewrite the subject. Unfortunately, the same hook for rebase goes
through two wildly different modules, so in order to completely convert
the post-rewrite hook, I have to update both at the same time.
It makes sense to convert both in the same commit, it's just that when I
see "sequencer" I think of the subset of rebase (-k/-i/-m/-r/-x) that
uses sequencer.c
+static void run_interactive_rewrite_hook(void)
+{
+ struct string_list *hooks;
+ struct string_list_item *p;
+ struct child_process child;
+
+ hooks = find_hooks("post-rewrite");
+ if (!hooks)
+ return;
+
+ for_each_string_list_item(p, hooks) {
+ child_process_init(&child);
+
+ child.in = open(rebase_path_rewritten_list(),
+ O_RDONLY);
+ child.stdout_to_stderr = 1;
+ child.trace2_hook_name = "post-rewrite";
+ argv_array_push(&child.args, p->string);
+ argv_array_push(&child.args, "rebase");
+ if (run_command(&child))
+ break;
+ }
+ free_hooks(hooks);
}
If you're adding a function to do this it would be nice to use it from
am.c as well rather than duplicating essentially the same code. Is there
any way to use a helper to run all the hooks, rather than introducing a
similar loop everywhere where we call a hook?
It's becoming clear to me that a helper is probably going to be cleaner,
so I'll add one in for v2.
void commit_post_rewrite(struct repository *r,
@@ -1326,6 +1362,7 @@ static int try_to_commit(struct repository *r,
char *amend_author = NULL;
const char *hook_commit = NULL;
enum commit_msg_cleanup_mode cleanup;
+ struct string_list *hooks;
int res = 0;
if (parse_head(r, ¤t_head))
@@ -1369,7 +1406,10 @@ static int try_to_commit(struct repository *r,
goto out;
}
- if (find_hook("prepare-commit-msg")) {
+ hooks = find_hooks("prepare-commit-msg");
+ if (hooks) {
+ free_hooks(hooks);
I think you forgot to update run_prepare_commit_msg_hook(), it should
probably be passed this list now. It might be outside the scope of this
series but unifying this with builtin/commit.c
run_prepare_commit_msg_hook calls run_hook_le, which looks up that value
itself. It's unfortunate that we have to do it twice, but we need to
know whether we need to re-read the commit msg or not. I can explain
this further in the commit message.
Thanks for explaining that, it would be nice to have that in the commit
message (I probably should have read the previous two patches to see
what run_hook_le() was doing).
Best Wishes
Phillip
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index ba8bd1b514..5b83f037b5 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -3,6 +3,7 @@
test_description='prepare-commit-msg hook'
. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-hooks.sh"
test_expect_success 'set up commits for rebasing' '
test_commit root &&
@@ -317,4 +318,12 @@ test_expect_success C_LOCALE_OUTPUT 'with failing hook (cherry-pick)' '
test $(grep -c prepare-commit-msg actual) = 1
'
+commit_command () {
+ echo "$1" >>file &&
+ git add file &&
+ git commit -m "$1"
+}
+
+test_multiple_hooks prepare-commit-msg commit_command
It's not clear to me that this is testing the sequencer
You're right. I need to adopt a different approach here.
