git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: "Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: "Orgad Shaneh" <orgads@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Orgad Shaneh" <orgads@gmail.com>,
	"Orgad Shaneh" <orgads@gmail.com>
Subject: [PATCH v5 2/2] hooks: allow input from stdin for commit-related hooks
Date: Wed, 09 Dec 2020 20:06:48 +0000
Message-ID: <25db4da3cd5fc7e81141078261086c392541c5d1.1607544408.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.790.v5.git.1607544408.gitgitgadget@gmail.com>

From: Orgad Shaneh <orgads@gmail.com>

Let hooks receive user input if applicable.

Closing stdin originates in f5bbc3225 (Port git commit to C, 2007).
Looks like the original shell implementation did have stdin open.

Due to stdin being closed, hooks that require user input have to either
read the input directly from the console (which can't work when running
from GUI applications), or popup a GUI dialog (which is inconvenient
when running from the terminal).

This allows for example prompting the user to choose an issue in
prepare-commit-msg, and add "Fixes #123" to the commit message.

Another possible use-case is running sanity test on pre-commit, and
having a prompt like "This and that issue were found in your changes.
Are you sure you want to commit? [Y/N]".

It's important to note that the hook author should be aware that stdin
is not always applicable. For example, when running from IDE. This can
be checked by isatty on stdin. The hooks should handle cases of closed
input, and possibly fall-back to GUI input, or have sane defaults with
a message to the user on this case.

Allow stdin only for commit-related hooks. Some of the other hooks pass
their own input to the hook, so don't change them.

Note: If pre-commit reads from stdin, and git commit is executed with
-F - (read message from stdin), stdin cannot be passed to the hook,
since it will consume it before reaching the point where it is read for
the commit message.

Signed-off-by: Orgad Shaneh <orgads@gmail.com>
---
 builtin/commit.c                              | 14 ++++--
 builtin/merge.c                               | 12 +++--
 sequencer.c                                   |  6 +--
 ...3-pre-commit-and-pre-merge-commit-hooks.sh | 46 ++++++++++++++++++-
 t/t7504-commit-msg-hook.sh                    | 15 ++++++
 t/t7505-prepare-commit-msg-hook.sh            | 14 ++++++
 6 files changed, 94 insertions(+), 13 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 70a7842e224..074a57937f1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -695,11 +695,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	int clean_message_contents = (cleanup_mode != COMMIT_MSG_CLEANUP_NONE);
 	int old_display_comment_prefix;
 	int merge_contains_scissors = 0;
+	int message_from_stdin = logfile && !strcmp(logfile, "-");
+	const unsigned hook_flags = message_from_stdin ? 0 : RUN_HOOK_ALLOW_STDIN;
 
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_commit_hook(use_editor, index_file, 0, "pre-commit", NULL))
+	if (!no_verify &&
+	    run_commit_hook(use_editor, index_file, hook_flags, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {
@@ -724,7 +727,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (have_option_m && !fixup_message) {
 		strbuf_addbuf(&sb, &message);
 		hook_arg1 = "message";
-	} else if (logfile && !strcmp(logfile, "-")) {
+	} else if (message_from_stdin) {
 		if (isatty(0))
 			fprintf(stderr, _("(reading log message from standard input)\n"));
 		if (strbuf_read(&sb, 0, 0) < 0)
@@ -998,7 +1001,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_commit_hook(use_editor, index_file, 0, "prepare-commit-msg",
+	if (run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, "prepare-commit-msg",
 			    git_path_commit_editmsg(), hook_arg1, hook_arg2, NULL))
 		return 0;
 
@@ -1015,7 +1018,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_commit_hook(use_editor, index_file, 0, "commit-msg", git_path_commit_editmsg(), NULL)) {
+	    run_commit_hook(use_editor, index_file, RUN_HOOK_ALLOW_STDIN, "commit-msg",
+			    git_path_commit_editmsg(), NULL)) {
 		return 0;
 	}
 
@@ -1701,7 +1705,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	repo_rerere(the_repository, 0);
 	run_auto_maintenance(quiet);
-	run_commit_hook(use_editor, get_index_file(), 0, "post-commit", NULL);
+	run_commit_hook(use_editor, get_index_file(), RUN_HOOK_ALLOW_STDIN, "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		commit_post_rewrite(the_repository, current_head, &oid);
 	}
diff --git a/builtin/merge.c b/builtin/merge.c
index 26e6ae15993..d6faca59258 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -836,8 +836,11 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	struct strbuf msg = STRBUF_INIT;
 	const char *index_file = get_index_file();
 
-	if (!no_verify && run_commit_hook(0 < option_edit, index_file, 0, "pre-merge-commit", NULL))
+	if (!no_verify &&
+	    run_commit_hook(0 < option_edit, index_file, RUN_HOOK_ALLOW_STDIN,
+			    "pre-merge-commit", NULL)) {
 		abort_commit(remoteheads, NULL);
+	}
 	/*
 	 * Re-read the index as pre-merge-commit hook could have updated it,
 	 * and write it out as a tree.  We must do this before we invoke
@@ -864,8 +867,9 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 		append_signoff(&msg, ignore_non_trailer(msg.buf, msg.len), 0);
 	write_merge_heads(remoteheads);
 	write_file_buf(git_path_merge_msg(the_repository), msg.buf, msg.len);
-	if (run_commit_hook(0 < option_edit, get_index_file(), 0, "prepare-commit-msg",
-			    git_path_merge_msg(the_repository), "merge", NULL))
+	if (run_commit_hook(0 < option_edit, get_index_file(), RUN_HOOK_ALLOW_STDIN,
+			    "prepare-commit-msg", git_path_merge_msg(the_repository),
+			    "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))
@@ -873,7 +877,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	}
 
 	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
-					  0, "commit-msg",
+					  RUN_HOOK_ALLOW_STDIN, "commit-msg",
 					  git_path_merge_msg(the_repository), NULL))
 		abort_commit(remoteheads, NULL);
 
diff --git a/sequencer.c b/sequencer.c
index 5f48d32e2fa..5190879695a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1203,8 +1203,8 @@ static int run_prepare_commit_msg_hook(struct repository *r,
 	} else {
 		arg1 = "message";
 	}
-	if (run_commit_hook(0, r->index_file, 0, "prepare-commit-msg", name,
-			    arg1, arg2, NULL))
+	if (run_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN,
+			    "prepare-commit-msg", name, arg1, arg2, NULL))
 		ret = error(_("'prepare-commit-msg' hook failed"));
 
 	return ret;
@@ -1528,7 +1528,7 @@ static int try_to_commit(struct repository *r,
 		goto out;
 	}
 
-	run_commit_hook(0, r->index_file, 0, "post-commit", NULL);
+	run_commit_hook(0, r->index_file, RUN_HOOK_ALLOW_STDIN, "post-commit", NULL);
 	if (flags & AMEND_MSG)
 		commit_post_rewrite(r, current_head, oid);
 
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index b3485450a20..a243b7efa19 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks'
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 PRECOMMIT="$HOOKDIR/pre-commit"
 PREMERGE="$HOOKDIR/pre-merge-commit"
+POSTCOMMIT="$HOOKDIR/post-commit"
 
 # Prepare sample scripts that write their $0 to actual_hooks
 test_expect_success 'sample script setup' '
@@ -28,11 +29,14 @@ test_expect_success 'sample script setup' '
 	echo $0 >>actual_hooks
 	test $GIT_PREFIX = "success/"
 	EOF
-	write_script "$HOOKDIR/check-author.sample" <<-\EOF
+	write_script "$HOOKDIR/check-author.sample" <<-\EOF &&
 	echo $0 >>actual_hooks
 	test "$GIT_AUTHOR_NAME" = "New Author" &&
 	test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com"
 	EOF
+	write_script "$HOOKDIR/user-input.sample" <<-\EOF
+	! read -r line || echo "$line" >hook_input
+	EOF
 '
 
 test_expect_success 'root commit' '
@@ -278,4 +282,44 @@ test_expect_success 'check the author in hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+test_expect_success 'with user input' '
+	test_when_finished "rm -f \"$PRECOMMIT\" user_input hook_input" &&
+	cp "$HOOKDIR/user-input.sample" "$PRECOMMIT" &&
+	echo "user input" >user_input &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m "more" <user_input &&
+	test_cmp user_input hook_input
+'
+
+test_expect_success 'with user input combined with -F -' '
+	test_when_finished "rm -f \"$PRECOMMIT\" user_input hook_input" &&
+	cp "$HOOKDIR/user-input.sample" "$PRECOMMIT" &&
+	echo "user input" >user_input &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -F - <user_input &&
+	! test_path_is_file hook_input
+'
+
+test_expect_success 'post-commit with user input' '
+	test_when_finished "rm -f \"$POSTCOMMIT\" user_input hook_input" &&
+	cp "$HOOKDIR/user-input.sample" "$POSTCOMMIT" &&
+	echo "user input" >user_input &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m "more" <user_input &&
+	test_cmp user_input hook_input
+'
+
+test_expect_success 'with user input (merge)' '
+	test_when_finished "rm -f \"$PREMERGE\" user_input hook_input" &&
+	cp "$HOOKDIR/user-input.sample" "$PREMERGE" &&
+	echo "user input" >user_input &&
+	git checkout side &&
+	git merge -m "merge master" master <user_input &&
+	git checkout master &&
+	test_cmp user_input hook_input
+'
+
 test_done
diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh
index 31b9c6a2c1d..aa76eb7e1f9 100755
--- a/t/t7504-commit-msg-hook.sh
+++ b/t/t7504-commit-msg-hook.sh
@@ -294,5 +294,20 @@ test_expect_success 'hook is called for reword during `rebase -i`' '
 
 '
 
+# now a hook that accepts input and writes it as the commit message
+cat >"$HOOK" <<'EOF'
+#!/bin/sh
+! read -r line || echo "$line" >"$1"
+EOF
+chmod +x "$HOOK"
+
+test_expect_success 'hook with user input' '
+
+	echo "additional" >>file &&
+	git add file &&
+	echo "user input" | git commit -m "additional" &&
+	commit_msg_is "user input"
+
+'
 
 test_done
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 94f85cdf831..aa9c9375e63 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -91,6 +91,11 @@ else
 fi
 test "$GIT_EDITOR" = : && source="$source (no editor)"
 
+if read -r line
+then
+	source="$source $line"
+fi
+
 if test $rebasing = 1
 then
 	echo "$source $(get_last_cmd)" >"$1"
@@ -113,6 +118,15 @@ test_expect_success 'with hook (-m)' '
 
 '
 
+test_expect_success 'with hook (-m and input)' '
+
+	echo "more" >>file &&
+	git add file &&
+	echo "user input" | git commit -m "more" &&
+	test "$(git log -1 --pretty=format:%s)" = "message (no editor) user input"
+
+'
+
 test_expect_success 'with hook (-m editor)' '
 
 	echo "more" >> file &&
-- 
gitgitgadget

  parent reply	other threads:[~2020-12-09 20:09 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-17 15:02 [PATCH] hooks: allow input from stdin Orgad Shaneh via GitGitGadget
2020-11-17 19:59 ` Junio C Hamano
2020-11-19 15:50 ` [PATCH v2] " Orgad Shaneh via GitGitGadget
2020-11-19 15:56   ` [PATCH v3] hooks: allow input from stdin for commit-related hooks Orgad Shaneh via GitGitGadget
2020-11-19 19:16     ` Junio C Hamano
2020-11-19 20:41       ` Orgad Shaneh
2020-11-19 20:56     ` [PATCH v4 0/2] " Orgad Shaneh via GitGitGadget
2020-11-19 20:56       ` [PATCH v4 1/2] " Orgad Shaneh via GitGitGadget
2020-11-19 21:23         ` Eric Sunshine
2020-11-19 21:32           ` Junio C Hamano
2020-11-20  5:23           ` Orgad Shaneh
2020-11-20  6:38             ` Eric Sunshine
2020-11-20  6:48               ` Eric Sunshine
2020-11-20  7:16                 ` Orgad Shaneh
2020-11-20 18:13               ` Junio C Hamano
2020-11-20 10:59             ` Ævar Arnfjörð Bjarmason
2020-11-20 12:34               ` Orgad Shaneh
2020-11-19 20:56       ` [PATCH v4 2/2] commit: fix stdin conflict between message and hook Orgad Shaneh via GitGitGadget
2020-12-09 20:06       ` [PATCH v5 0/2] hooks: allow input from stdin for commit-related hooks Orgad Shaneh via GitGitGadget
2020-12-09 20:06         ` [PATCH v5 1/2] hooks: lay foundations for passing stdin to hooks Orgad Shaneh via GitGitGadget
2020-12-09 20:06         ` Orgad Shaneh via GitGitGadget [this message]
2020-12-09 22:37           ` [PATCH v5 2/2] hooks: allow input from stdin for commit-related hooks Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=25db4da3cd5fc7e81141078261086c392541c5d1.1607544408.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=orgads@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git