Revert "Merge branch 'ar/run-command-hook'"

This reverts commit f406b89,
reversing changes made to 1627809.

It seems to have caused a few regressions, two of the three known
ones we have proposed solutions for.  Let's give ourselves a bit
more room to maneuver during the pre-release freeze period and
restart once the 2.53 ships.

Author: gitster

Documentation/RelNotes/2.53.0.adoc

@@ -93,9 +93,6 @@ Performance, Internal Implementation, Development Support etc.
  * Prepare test suite for Git for Windows that supports symbolic
    links.
 
- * Use hook API to replace ad-hoc invocation of hook scripts with the
-   run_command() API.
-
  * Import newer version of "clar", unit testing framework.
    (merge 84071a6dea ps/clar-integers later to maint).
 

builtin/hook.c

@@ -43,12 +43,6 @@ static int run(int argc, const char **argv, const char *prefix,
 	if (!argc)
 		goto usage;
 
-	/*
-	 * All current "hook run" use-cases require ungrouped child output.
-	 * If this changes, a hook run argument can be added to toggle it.
-	 */
-	opt.ungroup = 1;
-
 	/*
 	 * Having a -- for "run" when providing <hook-args> is
 	 * mandatory.

builtin/receive-pack.c

@@ -749,7 +749,7 @@ static int check_cert_push_options(const struct string_list *push_options)
 	return retval;
 }
 
-static void prepare_push_cert_sha1(struct run_hooks_opt *opt)
+static void prepare_push_cert_sha1(struct child_process *proc)
 {
 	static int already_done;
 
@@ -775,23 +775,23 @@ static void prepare_push_cert_sha1(struct run_hooks_opt *opt)
 		nonce_status = check_nonce(sigcheck.payload);
 	}
 	if (!is_null_oid(&push_cert_oid)) {
-		strvec_pushf(&opt->env, "GIT_PUSH_CERT=%s",
+		strvec_pushf(&proc->env, "GIT_PUSH_CERT=%s",
 			     oid_to_hex(&push_cert_oid));
-		strvec_pushf(&opt->env, "GIT_PUSH_CERT_SIGNER=%s",
+		strvec_pushf(&proc->env, "GIT_PUSH_CERT_SIGNER=%s",
 			     sigcheck.signer ? sigcheck.signer : "");
-		strvec_pushf(&opt->env, "GIT_PUSH_CERT_KEY=%s",
+		strvec_pushf(&proc->env, "GIT_PUSH_CERT_KEY=%s",
 			     sigcheck.key ? sigcheck.key : "");
-		strvec_pushf(&opt->env, "GIT_PUSH_CERT_STATUS=%c",
+		strvec_pushf(&proc->env, "GIT_PUSH_CERT_STATUS=%c",
 			     sigcheck.result);
 		if (push_cert_nonce) {
-			strvec_pushf(&opt->env,
+			strvec_pushf(&proc->env,
 				     "GIT_PUSH_CERT_NONCE=%s",
 				     push_cert_nonce);
-			strvec_pushf(&opt->env,
+			strvec_pushf(&proc->env,
 				     "GIT_PUSH_CERT_NONCE_STATUS=%s",
 				     nonce_status);
 			if (nonce_status == NONCE_SLOP)
-				strvec_pushf(&opt->env,
+				strvec_pushf(&proc->env,
 					     "GIT_PUSH_CERT_NONCE_SLOP=%ld",
 					     nonce_stamp_slop);
 		}
@@ -803,140 +803,167 @@ struct receive_hook_feed_state {
 	struct ref_push_report *report;
 	int skip_broken;
 	struct strbuf buf;
+	const struct string_list *push_options;
 };
 
-static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb UNUSED, void *pp_task_cb)
+typedef int (*feed_fn)(void *, const char **, size_t *);
+static int run_and_feed_hook(const char *hook_name, feed_fn feed,
+			     struct receive_hook_feed_state *feed_state)
 {
-	struct receive_hook_feed_state *state = pp_task_cb;
-	struct command *cmd = state->cmd;
-	unsigned int lines_batch_size = 500;
-
-	strbuf_reset(&state->buf);
-
-	/* batch lines to avoid going through run-command's poll loop for each line */
-	for (unsigned int i = 0; i < lines_batch_size; i++) {
-		while (cmd &&
-		       state->skip_broken && (cmd->error_string || cmd->did_not_exist))
-			cmd = cmd->next;
+	struct child_process proc = CHILD_PROCESS_INIT;
+	struct async muxer;
+	int code;
+	const char *hook_path = find_hook(the_repository, hook_name);
 
-		if (!cmd)
-			break;  /* no more commands left */
+	if (!hook_path)
+		return 0;
 
-		if (!state->report)
-			state->report = cmd->report;
+	strvec_push(&proc.args, hook_path);
+	proc.in = -1;
+	proc.stdout_to_stderr = 1;
+	proc.trace2_hook_name = hook_name;
+
+	if (feed_state->push_options) {
+		size_t i;
+		for (i = 0; i < feed_state->push_options->nr; i++)
+			strvec_pushf(&proc.env,
+				     "GIT_PUSH_OPTION_%"PRIuMAX"=%s",
+				     (uintmax_t)i,
+				     feed_state->push_options->items[i].string);
+		strvec_pushf(&proc.env, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"",
+			     (uintmax_t)feed_state->push_options->nr);
+	} else
+		strvec_pushf(&proc.env, "GIT_PUSH_OPTION_COUNT");
 
-		if (state->report) {
-			struct object_id *old_oid;
-			struct object_id *new_oid;
-			const char *ref_name;
+	if (tmp_objdir)
+		strvec_pushv(&proc.env, tmp_objdir_env(tmp_objdir));
 
-			old_oid = state->report->old_oid ? state->report->old_oid : &cmd->old_oid;
-			new_oid = state->report->new_oid ? state->report->new_oid : &cmd->new_oid;
-			ref_name = state->report->ref_name ? state->report->ref_name : cmd->ref_name;
+	if (use_sideband) {
+		memset(&muxer, 0, sizeof(muxer));
+		muxer.proc = copy_to_sideband;
+		muxer.in = -1;
+		code = start_async(&muxer);
+		if (code)
+			return code;
+		proc.err = muxer.in;
+	}
 
-			strbuf_addf(&state->buf, "%s %s %s\n",
-				    oid_to_hex(old_oid), oid_to_hex(new_oid),
-				    ref_name);
+	prepare_push_cert_sha1(&proc);
 
-			state->report = state->report->next;
-			if (!state->report)
-				cmd = cmd->next;
-		} else {
-			strbuf_addf(&state->buf, "%s %s %s\n",
-				    oid_to_hex(&cmd->old_oid), oid_to_hex(&cmd->new_oid),
-				    cmd->ref_name);
-			cmd = cmd->next;
-		}
+	code = start_command(&proc);
+	if (code) {
+		if (use_sideband)
+			finish_async(&muxer);
+		return code;
 	}
 
-	state->cmd = cmd;
+	sigchain_push(SIGPIPE, SIG_IGN);
 
-	if (state->buf.len > 0) {
-		int ret = write_in_full(hook_stdin_fd, state->buf.buf, state->buf.len);
-		if (ret < 0) {
-			if (errno == EPIPE)
-				return 1; /* child closed pipe */
-			return ret;
-		}
+	while (1) {
+		const char *buf;
+		size_t n;
+		if (feed(feed_state, &buf, &n))
+			break;
+		if (write_in_full(proc.in, buf, n) < 0)
+			break;
 	}
+	close(proc.in);
+	if (use_sideband)
+		finish_async(&muxer);
 
-	return state->cmd ? 0 : 1;  /* 0 = more to come, 1 = EOF */
+	sigchain_pop(SIGPIPE);
+
+	return finish_command(&proc);
 }
 
-static void hook_output_to_sideband(struct strbuf *output, void *cb_data UNUSED)
+static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 {
-	if (!output)
-		BUG("output must be non-NULL");
+	struct receive_hook_feed_state *state = state_;
+	struct command *cmd = state->cmd;
 
-	/* buffer might be empty for keepalives */
-	if (output->len)
-		send_sideband(1, 2, output->buf, output->len, use_sideband);
+	while (cmd &&
+	       state->skip_broken && (cmd->error_string || cmd->did_not_exist))
+		cmd = cmd->next;
+	if (!cmd)
+		return -1; /* EOF */
+	if (!bufp)
+		return 0; /* OK, can feed something. */
+	strbuf_reset(&state->buf);
+	if (!state->report)
+		state->report = cmd->report;
+	if (state->report) {
+		struct object_id *old_oid;
+		struct object_id *new_oid;
+		const char *ref_name;
+
+		old_oid = state->report->old_oid ? state->report->old_oid : &cmd->old_oid;
+		new_oid = state->report->new_oid ? state->report->new_oid : &cmd->new_oid;
+		ref_name = state->report->ref_name ? state->report->ref_name : cmd->ref_name;
+		strbuf_addf(&state->buf, "%s %s %s\n",
+			    oid_to_hex(old_oid), oid_to_hex(new_oid),
+			    ref_name);
+		state->report = state->report->next;
+		if (!state->report)
+			state->cmd = cmd->next;
+	} else {
+		strbuf_addf(&state->buf, "%s %s %s\n",
+			    oid_to_hex(&cmd->old_oid), oid_to_hex(&cmd->new_oid),
+			    cmd->ref_name);
+		state->cmd = cmd->next;
+	}
+	if (bufp) {
+		*bufp = state->buf.buf;
+		*sizep = state->buf.len;
+	}
+	return 0;
 }
 
 static int run_receive_hook(struct command *commands,
 			    const char *hook_name,
 			    int skip_broken,
 			    const struct string_list *push_options)
 {
-	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
-	struct command *iter = commands;
-	struct receive_hook_feed_state feed_state;
-	int ret;
-
-	/* if there are no valid commands, don't invoke the hook at all. */
-	while (iter && skip_broken && (iter->error_string || iter->did_not_exist))
-		iter = iter->next;
-	if (!iter)
-		return 0;
-
-	if (push_options) {
-		for (int i = 0; i < push_options->nr; i++)
-			strvec_pushf(&opt.env, "GIT_PUSH_OPTION_%d=%s", i,
-				     push_options->items[i].string);
-		strvec_pushf(&opt.env, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"",
-					     (uintmax_t)push_options->nr);
-	} else {
-		strvec_push(&opt.env, "GIT_PUSH_OPTION_COUNT");
-	}
-
-	if (tmp_objdir)
-		strvec_pushv(&opt.env, tmp_objdir_env(tmp_objdir));
-
-	prepare_push_cert_sha1(&opt);
-
-	/* set up sideband printer */
-	if (use_sideband)
-		opt.consume_output = hook_output_to_sideband;
-
-	/* set up stdin callback */
-	feed_state.cmd = commands;
-	feed_state.skip_broken = skip_broken;
-	feed_state.report = NULL;
-	strbuf_init(&feed_state.buf, 0);
-	opt.feed_pipe_cb_data = &feed_state;
-	opt.feed_pipe = feed_receive_hook_cb;
-
-	ret = run_hooks_opt(the_repository, hook_name, &opt);
-
-	strbuf_release(&feed_state.buf);
+	struct receive_hook_feed_state state;
+	int status;
 
-	return ret;
+	strbuf_init(&state.buf, 0);
+	state.cmd = commands;
+	state.skip_broken = skip_broken;
+	state.report = NULL;
+	if (feed_receive_hook(&state, NULL, NULL))
+		return 0;
+	state.cmd = commands;
+	state.push_options = push_options;
+	status = run_and_feed_hook(hook_name, feed_receive_hook, &state);
+	strbuf_release(&state.buf);
+	return status;
 }
 
 static int run_update_hook(struct command *cmd)
 {
-	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
+	struct child_process proc = CHILD_PROCESS_INIT;
+	int code;
+	const char *hook_path = find_hook(the_repository, "update");
 
-	strvec_pushl(&opt.args,
-		     cmd->ref_name,
-		     oid_to_hex(&cmd->old_oid),
-		     oid_to_hex(&cmd->new_oid),
-		     NULL);
+	if (!hook_path)
+		return 0;
 
-	if (use_sideband)
-		opt.consume_output = hook_output_to_sideband;
+	strvec_push(&proc.args, hook_path);
+	strvec_push(&proc.args, cmd->ref_name);
+	strvec_push(&proc.args, oid_to_hex(&cmd->old_oid));
+	strvec_push(&proc.args, oid_to_hex(&cmd->new_oid));
+
+	proc.no_stdin = 1;
+	proc.stdout_to_stderr = 1;
+	proc.err = use_sideband ? -1 : 0;
+	proc.trace2_hook_name = "update";
 
-	return run_hooks_opt(the_repository, "update", &opt);
+	code = start_command(&proc);
+	if (code)
+		return code;
+	if (use_sideband)
+		copy_to_sideband(proc.err, -1, NULL);
+	return finish_command(&proc);
 }
 
 static struct command *find_command_by_refname(struct command *list,
@@ -1613,20 +1640,33 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 static void run_update_post_hook(struct command *commands)
 {
 	struct command *cmd;
-	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
+	struct child_process proc = CHILD_PROCESS_INIT;
+	const char *hook;
+
+	hook = find_hook(the_repository, "post-update");
+	if (!hook)
+		return;
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (cmd->error_string || cmd->did_not_exist)
 			continue;
-		strvec_push(&opt.args, cmd->ref_name);
+		if (!proc.args.nr)
+			strvec_push(&proc.args, hook);
+		strvec_push(&proc.args, cmd->ref_name);
 	}
-	if (!opt.args.nr)
+	if (!proc.args.nr)
 		return;
 
-	if (use_sideband)
-		opt.consume_output = hook_output_to_sideband;
+	proc.no_stdin = 1;
+	proc.stdout_to_stderr = 1;
+	proc.err = use_sideband ? -1 : 0;
+	proc.trace2_hook_name = "post-update";
 
-	run_hooks_opt(the_repository, "post-update", &opt);
+	if (!start_command(&proc)) {
+		if (use_sideband)
+			copy_to_sideband(proc.err, -1, NULL);
+		finish_command(&proc);
+	}
 }
 
 static void check_aliased_update_internal(struct command *cmd,

commit.c

@@ -1978,9 +1978,6 @@ int run_commit_hook(int editor_is_used, const char *index_file,
 		strvec_push(&opt.args, arg);
 	va_end(args);
 
-	/* All commit hook use-cases require ungrouping child output. */
-	opt.ungroup = 1;
-
 	opt.invoked_hook = invoked_hook;
 	return run_hooks_opt(the_repository, name, &opt);
 }