hook: introduce extensions.hookStdoutToStderr

All hooks already redirect stdout to stderr with the exception of
pre-push which has a known user who depends on the separate stdout
versus stderr outputs (the git-lfs project).

The pre-push behavior was a surprise which we found out about after
causing a regression for git-lfs. Notably, it might not be the only
exception (it's the one we know about). There might be more.

This presents a challenge because stdout_to_stderr is required for
hook parallelization, so run-command can buffer and de-interleave
the hook outputs using ungroup=0, when hook.jobs > 1.

Introduce an extension to enforce consistency: all hooks merge stdout
into stderr and can be safely parallelized. This provides a clean
separation and avoids breaking existing stdout vs stderr behavior.

When this extension is disabled, the `hook.jobs` config has no
effect for pre-push, to prevent garbled (interleaved) parallel
output, so it runs sequentially like before.

Alternatives I've considered to this extension include:
1. Allowing pre-push to run in parallel with interleaved output.
2. Always running pre-push sequentially (no parallel jobs for it).
3. Making users (only git-lfs? maybe more?) fix their hooks to read
   stderr not stdout.

Out of all these alternatives, I think this extension is the most
reasonable compromise, to not break existing users, allow pre-push
parallel jobs for those who need it (with correct outputs) and also
future-proofing in case there are any more exceptions to be added.

Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Author: 10ne1

Documentation/config/extensions.adoc

@@ -102,6 +102,18 @@ The extension can be enabled automatically for new repositories by setting
 `init.defaultSubmodulePathConfig` to `true`, for example by running
 `git config --global init.defaultSubmodulePathConfig true`.
 
+hookStdoutToStderr:::
+	If enabled, the stdout of all hooks is redirected to stderr. This
+	enforces consistency, since by default most hooks already behave
+	this way, with pre-push being the only known exception.
++
+This is useful for parallel hook execution (see the `hook.jobs` config in
+linkgit:git-config[1]), as it allows the output of multiple hooks running
+in parallel to be grouped (de-interleaved) correctly.
++
+Defaults to disabled. When disabled, `hook.jobs` has no effect for pre-push
+hooks, which will always be run sequentially.
+
 worktreeConfig:::
 	If enabled, then worktrees will load config settings from the
 	`$GIT_DIR/config.worktree` file in addition to the

Documentation/config/hook.adoc

@@ -62,3 +62,6 @@ hook.jobs::
 +
 This setting has no effect unless all configured hooks for the event have
 `hook.<name>.parallel` set to `true`.
++
+This has no effect for hooks requiring separate output streams (like `pre-push`)
+unless `extensions.hookStdoutToStderr` is enabled.

repository.c

@@ -283,6 +283,7 @@ int repo_init(struct repository *repo,
 	repo->repository_format_relative_worktrees = format.relative_worktrees;
 	repo->repository_format_precious_objects = format.precious_objects;
 	repo->repository_format_submodule_path_cfg = format.submodule_path_cfg;
+	repo->repository_format_hook_stdout_to_stderr = format.hook_stdout_to_stderr;
 
 	/* take ownership of format.partial_clone */
 	repo->repository_format_partial_clone = format.partial_clone;

repository.h

@@ -173,6 +173,7 @@ struct repository {
 	int repository_format_relative_worktrees;
 	int repository_format_precious_objects;
 	int repository_format_submodule_path_cfg;
+	int repository_format_hook_stdout_to_stderr;
 
 	/* Indicate if a repository has a different 'commondir' from 'gitdir' */
 	unsigned different_commondir:1;

setup.c

@@ -688,6 +688,9 @@ static enum extension_result handle_extension(const char *var,
 	} else if (!strcmp(ext, "submodulepathconfig")) {
 		data->submodule_path_cfg = git_config_bool(var, value);
 		return EXTENSION_OK;
+	} else if (!strcmp(ext, "hookstdouttostderr")) {
+		data->hook_stdout_to_stderr = git_config_bool(var, value);
+		return EXTENSION_OK;
 	}
 	return EXTENSION_UNKNOWN;
 }
@@ -1951,6 +1954,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				repo_fmt.relative_worktrees;
 			the_repository->repository_format_submodule_path_cfg =
 				repo_fmt.submodule_path_cfg;
+			the_repository->repository_format_hook_stdout_to_stderr =
+				repo_fmt.hook_stdout_to_stderr;
 			/* take ownership of repo_fmt.partial_clone */
 			the_repository->repository_format_partial_clone =
 				repo_fmt.partial_clone;
@@ -2053,6 +2058,8 @@ void check_repository_format(struct repository_format *fmt)
 		fmt->submodule_path_cfg;
 	the_repository->repository_format_relative_worktrees =
 		fmt->relative_worktrees;
+	the_repository->repository_format_hook_stdout_to_stderr =
+		fmt->hook_stdout_to_stderr;
 	the_repository->repository_format_partial_clone =
 		xstrdup_or_null(fmt->partial_clone);
 	clear_repository_format(&repo_fmt);

setup.h

@@ -168,6 +168,7 @@ struct repository_format {
 	int worktree_config;
 	int relative_worktrees;
 	int submodule_path_cfg;
+	int hook_stdout_to_stderr;
 	int is_bare;
 	int hash_algo;
 	int compat_hash_algo;

t/t1800-hook.sh

@@ -532,6 +532,43 @@ test_expect_success 'client hooks: pre-push expects separate stdout and stderr'
 	check_stdout_separate_from_stderr pre-push
 '
 
+test_expect_success 'client hooks: extension makes pre-push merge stdout to stderr' '
+	test_when_finished "rm -rf remote2 stdout.actual stderr.actual" &&
+	git init --bare remote2 &&
+	git remote add origin2 remote2 &&
+	test_commit B &&
+	git config set core.repositoryformatversion 1 &&
+	test_config extensions.hookStdoutToStderr true &&
+	setup_hooks pre-push &&
+	git push origin2 HEAD:main >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-push
+'
+
+test_expect_success 'client hooks: pre-push defaults to serial execution' '
+	test_when_finished "rm -rf remote-serial repo-serial" &&
+	git init --bare remote-serial &&
+	git init repo-serial &&
+	git -C repo-serial remote add origin ../remote-serial &&
+	test_commit -C repo-serial A &&
+
+	# Setup 2 pre-push hooks; no parallel=true so they must run serially.
+	# Use sentinel/detector pattern: hook-1 (sentinel, configured) runs first
+	# because configured hooks precede traditional hooks in list order; hook-2
+	# (detector) runs second and checks whether hook-1 has finished.
+	git -C repo-serial config hook.hook-1.event pre-push &&
+	git -C repo-serial config hook.hook-1.command \
+	    "touch sentinel.started; sleep 2; touch sentinel.done" &&
+	git -C repo-serial config hook.hook-2.event pre-push &&
+	git -C repo-serial config hook.hook-2.command \
+	    "$(sentinel_detector sentinel hook.order)" &&
+
+	git -C repo-serial config hook.jobs 2 &&
+
+	git -C repo-serial push origin HEAD >out 2>err &&
+	echo serial >expect &&
+	test_cmp expect repo-serial/hook.order
+'
+
 test_expect_success 'client hooks: commit hooks expect stdout redirected to stderr' '
 	hooks="pre-commit prepare-commit-msg \
 		commit-msg post-commit \

transport.c

@@ -1388,11 +1388,8 @@ static int run_pre_push_hook(struct transport *transport,
 	opt.feed_pipe_cb_data_alloc = pre_push_hook_data_alloc;
 	opt.feed_pipe_cb_data_free = pre_push_hook_data_free;
 
-	/*
-	 * 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;
+	/* merge stdout to stderr only when extensions.hookStdoutToStderr is enabled */
+	opt.stdout_to_stderr = the_repository->repository_format_hook_stdout_to_stderr;
 
 	ret = run_hooks_opt(the_repository, "pre-push", &opt);