hook: allow hooks to disable stdout_to_stderr

The last batch of hooks converted to the hook.[ch] API introduced
a regression because pick_next_hook() always sets stdout_to_stderr
for its child processes.

Pre-push is the only hook API user which requires stdout_to_stderr
to be 0, so it can be argued that pre-push needs fixing, however
this will likely break many pre-push hooks, so it's better to allow
it to be 0, i.e. to match the previous behavior.

To prevent such regressions in the future, extend the hook tests to
verify hooks write to the expected stdout vs stderr streams and
maintain backward compatibility with the hooks output assumptions.

The tests are independent of the actual hook implementations: I've
tested they work the same before and after the hook.[ch] conversion
and will continue to work after we eventually introduce parallel
hook execution and config-based hooks.

Fixes: 3e2836a742d8 ("transport: convert pre-push to hook API")
Reported-by: Chris Darroch <chrisd@apache.org>
Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/hook.c b/hook.c
index 6b8ddfe7b603841b819493c37e4b6119eb1be00b..47feff9cd72bb43efc9626eb79084ec4266b8b79 100644 (file)
--- a/hook.c
+++ b/hook.c
@@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,
                cp->in = -1;
        }
 
-       cp->stdout_to_stderr = 1;
+       cp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;
        cp->trace2_hook_name = hook_cb->hook_name;
        cp->dir = hook_cb->options->dir;
 

diff --git a/hook.h b/hook.h
index ae502178b9bfad6dcaed21cfb6bb7457dfe5bd07..2488db71339d834c7e57a52188118a5f665fafd2 100644 (file)
--- a/hook.h
+++ b/hook.h
@@ -39,6 +39,11 @@ struct run_hooks_opt
         */
        unsigned int ungroup:1;
 
+       /**
+        * Send the hook's stdout to stderr.
+        */
+       unsigned int stdout_to_stderr:1;
+
        /**
         * Path to file which should be piped to stdin for each hook.
         */
@@ -93,6 +98,7 @@ struct run_hooks_opt
 #define RUN_HOOKS_OPT_INIT { \
        .env = STRVEC_INIT, \
        .args = STRVEC_INIT, \
+       .stdout_to_stderr = 1, \
 }
 
 struct hook_cb_data {

diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 4feaf0d7bef71fc558e640f5fae37ae1fb95c95c..0e4f93fb3147cade6af23410a091b2af7fa3a60f 100755 (executable)
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -184,4 +184,131 @@ test_expect_success 'stdin to hooks' '
        test_cmp expect actual
 '
 
+check_stdout_separate_from_stderr () {
+       for hook in "$@"
+       do
+               test_grep ! "Hook $hook stdout" stderr.actual &&
+               test_grep ! "Hook $hook stderr" stdout.actual &&
+               test_grep "Hook $hook stderr" stderr.actual &&
+               test_grep "Hook $hook stdout" stdout.actual || return 1
+       done
+}
+
+check_stdout_merged_to_stderr () {
+       test_grep ! "Hook .* stdout" stdout.actual &&
+       test_grep ! "Hook .* stderr" stdout.actual &&
+       for hook in "$@"
+       do
+               test_grep "Hook $hook stdout" stderr.actual &&
+               test_grep "Hook $hook stderr" stderr.actual || return 1
+       done
+}
+
+test_expect_success 'client pre-push hook expects separate stdout and stderr' '
+       test_when_finished "rm -f stdout.actual stderr.actual" &&
+       git init --bare remote &&
+       git remote add origin remote &&
+       test_commit A &&
+
+       hook=pre-push &&
+       test_hook $hook <<-EOF &&
+       echo >&1 Hook $hook stdout
+       echo >&2 Hook $hook stderr
+       EOF
+
+       git push origin HEAD:main >stdout.actual 2>stderr.actual &&
+       check_stdout_separate_from_stderr pre-push
+'
+
+test_expect_success 'client hooks expect stdout redirected to stderr' '
+       test_when_finished "rm -f stdout.actual stderr.actual" &&
+       for hook in pre-commit post-commit post-checkout pre-merge-commit \
+               prepare-commit-msg commit-msg post-merge post-rewrite reference-transaction \
+               applypatch-msg pre-applypatch post-applypatch pre-rebase post-index-change
+       do
+               test_hook $hook <<-EOF || return 1
+               echo >&1 Hook $hook stdout
+               echo >&2 Hook $hook stderr
+               EOF
+       done &&
+
+       git checkout -B main &&
+       git checkout -b branch-a &&
+       test_commit commit-on-branch-a &&
+
+       # Trigger pre-commit, prepare-commit-msg, commit-msg, post-commit, reference-transaction
+       git commit --allow-empty -m "Test" >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr pre-commit prepare-commit-msg commit-msg post-commit reference-transaction &&
+
+       # Trigger post-checkout, reference-transaction
+       git checkout -b new-branch main >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr post-checkout reference-transaction &&
+
+       # Trigger pre-merge-commit, post-merge, reference-transaction
+       test_commit new-branch-commit &&
+       git merge --no-ff branch-a >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr pre-merge-commit post-merge reference-transaction &&
+
+       # Trigger post-rewrite, reference-transaction
+       git commit --amend --allow-empty --no-edit >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr post-rewrite reference-transaction &&
+
+       # Trigger applypatch-msg, pre-applypatch, post-applypatch
+       git checkout -b branch-b main &&
+       test_commit branch-b &&
+       git format-patch -1 --stdout >patch &&
+       git checkout -b branch-c main &&
+       git am patch >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr applypatch-msg pre-applypatch post-applypatch &&
+
+       # Trigger pre-rebase
+       git checkout -b branch-d main &&
+       test_commit branch-d &&
+       git checkout main &&
+       test_commit diverge-main &&
+       git checkout branch-d &&
+       git rebase main >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr pre-rebase &&
+
+       # Trigger post-index-change
+       oid=$(git hash-object -w --stdin </dev/null) &&
+       git update-index --add --cacheinfo 100644 $oid new-file >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr post-index-change
+'
+
+test_expect_success 'server hooks expect stdout redirected to stderr' '
+       test_when_finished "rm -f stdout.actual stderr.actual" &&
+       git init --bare remote-server &&
+       git remote add origin-server remote-server &&
+
+       for hook in pre-receive update post-receive post-update
+       do
+               write_script remote-server/hooks/$hook <<-EOF || return 1
+               echo >&1 Hook $hook stdout
+               echo >&2 Hook $hook stderr
+               EOF
+       done &&
+
+       # Trigger pre-receive update post-receive post-update
+       git push origin-server HEAD:new-branch >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr pre-receive update post-receive post-update
+'
+
+test_expect_success 'server push-to-checkout hook expects stdout redirected to stderr' '
+       test_when_finished "rm -f stdout.actual stderr.actual" &&
+       git init server &&
+       git -C server checkout -b main &&
+       test_config -C server receive.denyCurrentBranch updateInstead &&
+       git remote add origin-server-2 server &&
+
+       write_script server/.git/hooks/push-to-checkout <<-EOF &&
+       echo >&1 Hook push-to-checkout stdout
+       echo >&2 Hook push-to-checkout stderr
+       EOF
+
+       # Trigger push-to-checkout
+       git push origin-server-2 HEAD:main >stdout.actual 2>stderr.actual &&
+       check_stdout_merged_to_stderr push-to-checkout
+'
+
 test_done

diff --git a/transport.c b/transport.c
index 6d0f02be5d7c00ce47580f6e79921c913e3fbecb..e876cc9189d33ffe025cde09fb62c3fb1728a38d 100644 (file)
--- a/transport.c
+++ b/transport.c
@@ -1373,6 +1373,12 @@ static int run_pre_push_hook(struct transport *transport,
        opt.feed_pipe = pre_push_hook_feed_stdin;
        opt.feed_pipe_cb_data = &data;
 
+       /*
+        * pre-push hooks expect stdout & stderr to be separate, so don't merge
+        * them to keep backwards compatibility with existing hooks.
+        */
+       opt.stdout_to_stderr = 0;
+
        ret = run_hooks_opt(the_repository, "pre-push", &opt);
 
        strbuf_release(&data.buf);