[PATCH V2 0/7] fix hunk editing with 'commit -p -m'

This patch fixes the fact that hunk editing with 'commit -p -m' does not work:
GIT_EDITOR is set to ':' to indicate to hooks that no editor will be launched,
which result in the 'hunk edit' option not launching the editor (and selecting
the whole hunk).

The fix consists in deferring the GIT_EDITOR override to the hook subprocess,
like it's already done for GIT_INDEX_FILE:
- rename 'run_hook' to 'run_hook_le' and change the first parameter to the environment to set
- add a 'run_hook_ve' variant that take a va_list
- add a new 'run_commit_hook' helper (to set both GIT_EDITOR and GIT_INDEX_FILE)
- the old 'run_hook' functionality is available as 'run_hook_with_custom_index'
  (and marked as deprecated in the last optional patch of this series)

N.B.: the merge builtin 'prepare-commit-msg' hook handling has also been updated
to be consistent; i.e. GIT_EDITOR will not be set to ':' if the '--edit' option
is used.

Benoit Pierre (7):
  merge hook tests: fix missing '&&' in test
  merge hook tests: use 'test_must_fail' instead of '!'
  test patch hunk editing with "commit -p -m"
  commit: fix patch hunk editing with "commit -p -m"
  merge: fix GIT_EDITOR override for commit hook
  merge hook tests: fix and update tests
  run-command: mark run_hook_with_custom_index as deprecated

 builtin/checkout.c                 |  8 +++----
 builtin/clone.c                    |  4 ++--
 builtin/commit.c                   | 35 ++++++++++++++++++++++++------
 builtin/gc.c                       |  2 +-
 builtin/merge.c                    |  6 +++---
 commit.h                           |  3 +++
 run-command.c                      | 44 +++++++++++++++++++++++++++-----------
 run-command.h                      |  7 +++++-
 t/t7505-prepare-commit-msg-hook.sh | 33 ++++++++++++++++++++--------
 t/t7513-commit_-p_-m_hunk_edit.sh  | 34 +++++++++++++++++++++++++++++
 10 files changed, 137 insertions(+), 39 deletions(-)
 create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh

-- 
1.9.0

Signed-off-by: Benoit Pierre <redacted>
---
 t/t7505-prepare-commit-msg-hook.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 3573751..1c95652 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh

@@ -174,7 +174,7 @@ test_expect_success 'with failing hook (merge)' '
 	git add file &&
 	rm -f "$HOOK" &&
 	git commit -m other &&
-	write_script "$HOOK" <<-EOF
+	write_script "$HOOK" <<-EOF &&
 	exit 1
 	EOF
 	git checkout - &&

-- 
1.9.0

Add (failing) test: with commit changing the environment to let hooks
now that no editor will be used (by setting GIT_EDITOR to ":"), the
"edit hunk" functionality does not work (no editor is launched and the
whole hunk is committed).

Signed-off-by: Benoit Pierre <redacted>
---
 t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh

diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
new file mode 100755
index 0000000..994939a
--- /dev/null
+++ b/t/t7513-commit_-p_-m_hunk_edit.sh

@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='hunk edit with "commit -p -m"'
+. ./test-lib.sh
+
+if ! test_have_prereq PERL
+then
+	skip_all="skipping '$test_description' tests, perl not available"
+	test_done
+fi
+
+test_expect_success 'setup (initial)' '
+	echo line1 >file &&
+	git add file &&
+	git commit -m commit1 &&
+	echo line3 >>file &&
+	cat >expect <<-\EOF
+	diff --git a/file b/file
+	index a29bdeb..c0d0fb4 100644
+	--- a/file
+	+++ b/file
+	@@ -1 +1,2 @@
+	 line1
+	+line2
+	EOF
+'
+
+test_expect_failure 'edit hunk "commit -p -m message"' '
+	echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
+	git diff HEAD^ HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
1.9.0

Signed-off-by: Benoit Pierre <redacted>
---
 t/t7505-prepare-commit-msg-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 1c95652..5531abb 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh

@@ -154,7 +154,7 @@ test_expect_success 'with failing hook' '
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
-	! GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit -c $head
+	test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit -c $head
 
 '
 

@@ -163,7 +163,7 @@ test_expect_success 'with failing hook (--no-verify)' '
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&
-	! GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify -c $head
+	test_must_fail env GIT_EDITOR="\"\$FAKE_EDITOR\"" git commit --no-verify -c $head
 
 '
 

-- 
1.9.0

Don't set GIT_EDITOR to ":" when calling prepare-commit-msg hook if the
editor is going to be called (e.g. with "merge -e").

Signed-off-by: Benoit Pierre <redacted>
---
 builtin/merge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 67f312d..b11a528 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -821,7 +821,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
-	if (run_commit_hook(1, get_index_file(), "prepare-commit-msg",
+	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path("MERGE_MSG"), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {

-- 
1.9.0

- update 'no editor' hook test and add 'editor' hook test
- make sure the tree is reset to a clean state after running a test
  (using test_when_finished) so later tests are not impacted

Signed-off-by: Benoit Pierre <redacted>
---
 t/t7505-prepare-commit-msg-hook.sh | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 5531abb..03dce09 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh

@@ -134,14 +134,26 @@ test_expect_success 'with hook (-c)' '
 
 test_expect_success 'with hook (merge)' '
 
-	head=`git rev-parse HEAD` &&
-	git checkout -b other HEAD@{1} &&
-	echo "more" >> file &&
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m other &&
+	git checkout - &&
+	git merge --no-ff other &&
+	test "`git log -1 --pretty=format:%s`" = "merge (no editor)"
+'
+
+test_expect_success 'with hook and editor (merge)' '
+
+	test_when_finished "git checkout -f master" &&
+	git checkout -B other HEAD@{1} &&
+	echo "more" >>file &&
 	git add file &&
 	git commit -m other &&
 	git checkout - &&
-	git merge other &&
-	test "`git log -1 --pretty=format:%s`" = merge
+	env GIT_EDITOR="\"\$FAKE_EDITOR\"" git merge --no-ff -e other &&
+	test "`git log -1 --pretty=format:%s`" = "merge"
 '
 
 cat > "$HOOK" <<'EOF'

@@ -151,6 +163,7 @@ EOF
 
 test_expect_success 'with failing hook' '
 
+	test_when_finished "git checkout -f master" &&
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&

@@ -160,6 +173,7 @@ test_expect_success 'with failing hook' '
 
 test_expect_success 'with failing hook (--no-verify)' '
 
+	test_when_finished "git checkout -f master" &&
 	head=`git rev-parse HEAD` &&
 	echo "more" >> file &&
 	git add file &&

@@ -169,6 +183,7 @@ test_expect_success 'with failing hook (--no-verify)' '
 
 test_expect_success 'with failing hook (merge)' '
 
+	test_when_finished "git checkout -f master" &&
 	git checkout -B other HEAD@{1} &&
 	echo "more" >> file &&
 	git add file &&

@@ -178,7 +193,7 @@ test_expect_success 'with failing hook (merge)' '
 	exit 1
 	EOF
 	git checkout - &&
-	test_must_fail git merge other
+	test_must_fail git merge --no-ff other
 
 '
 

-- 
1.9.0

Don't change git environment: move the GIT_EDITOR=":" override to the
hook command subprocess, like it's already done for GIT_INDEX_FILE.

Signed-off-by: Benoit Pierre <redacted>
---
 builtin/checkout.c                |  8 +++----
 builtin/clone.c                   |  4 ++--
 builtin/commit.c                  | 35 ++++++++++++++++++++++++-------
 builtin/gc.c                      |  2 +-
 builtin/merge.c                   |  6 +++---
 commit.h                          |  3 +++
 run-command.c                     | 44 ++++++++++++++++++++++++++++-----------
 run-command.h                     |  6 +++++-
 t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-
 9 files changed, 79 insertions(+), 31 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..da423b2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -53,10 +53,10 @@ struct checkout_opts {
 static int post_checkout_hook(struct commit *old, struct commit *new,
 			      int changed)
 {
-	return run_hook(NULL, "post-checkout",
-			sha1_to_hex(old ? old->object.sha1 : null_sha1),
-			sha1_to_hex(new ? new->object.sha1 : null_sha1),
-			changed ? "1" : "0", NULL);
+    return run_hook_le(NULL, "post-checkout",
+		       sha1_to_hex(old ? old->object.sha1 : null_sha1),
+		       sha1_to_hex(new ? new->object.sha1 : null_sha1),
+		       changed ? "1" : "0", NULL);
 	/* "new" can be NULL when checking out from the index before
 	   a commit exists. */
 

diff --git a/builtin/clone.c b/builtin/clone.c
index 43e772c..9b3c04d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -660,8 +660,8 @@ static int checkout(void)
 	    commit_locked_index(lock_file))
 		die(_("unable to write new index file"));
 
-	err |= run_hook(NULL, "post-checkout", sha1_to_hex(null_sha1),
-			sha1_to_hex(sha1), "1", NULL);
+	err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
+			   sha1_to_hex(sha1), "1", NULL);
 
 	if (!err && option_recursive)
 		err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);

diff --git a/builtin/commit.c b/builtin/commit.c
index 3767478..baf1fc0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -612,7 +612,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
 		return 0;
 
 	if (squash_message) {

@@ -866,8 +866,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_hook(index_file, "prepare-commit-msg",
-		     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+	if (run_commit_hook(use_editor, index_file, "prepare-commit-msg",
+			    git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
 		return 0;
 
 	if (use_editor) {

@@ -883,7 +883,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+	    run_commit_hook(use_editor, index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
 		return 0;
 	}
 

@@ -1067,8 +1067,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_editor = 0;
 	if (0 <= edit_flag)
 		use_editor = edit_flag;
-	if (!use_editor)
-		setenv("GIT_EDITOR", ":", 1);
 
 	/* Sanity check options */
 	if (amend && !current_head)

@@ -1445,6 +1443,29 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 	return finish_command(&proc);
 }
 
+int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
+{
+	const char *hook_env[3] =  { NULL };
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook_env[0] = index;
+
+	/*
+	 * Let the hook know that no editor will be launched.
+	 */
+	if (!editor_is_used)
+		hook_env[1] = "GIT_EDITOR=:";
+
+	va_start(args, name);
+	ret = run_hook_ve(hook_env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	static struct wt_status s;

@@ -1669,7 +1690,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		     "not exceeded, and then \"git reset HEAD\" to recover."));
 
 	rerere(0);
-	run_hook(get_index_file(), "post-commit", NULL);
+	run_commit_hook(use_editor, get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		struct notes_rewrite_cfg *cfg;
 		cfg = init_copy_notes_for_rewrite("amend");

diff --git a/builtin/gc.c b/builtin/gc.c
index c19545d..7fa717a 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c

@@ -179,7 +179,7 @@ static int need_to_gc(void)
 	else if (!too_many_loose_objects())
 		return 0;
 
-	if (run_hook(NULL, "pre-auto-gc", NULL))
+	if (run_hook_le(NULL, "pre-auto-gc", NULL))
 		return 0;
 	return 1;
 }

diff --git a/builtin/merge.c b/builtin/merge.c
index e576a7f..67f312d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -421,7 +421,7 @@ static void finish(struct commit *head_commit,
 	}
 
 	/* Run a post-merge hook */
-	run_hook(NULL, "post-merge", squash ? "1" : "0", NULL);
+	run_hook_le(NULL, "post-merge", squash ? "1" : "0", NULL);
 
 	strbuf_release(&reflog_message);
 }

@@ -821,8 +821,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
-	if (run_hook(get_index_file(), "prepare-commit-msg",
-		     git_path("MERGE_MSG"), "merge", NULL, NULL))
+	if (run_commit_hook(1, get_index_file(), "prepare-commit-msg",
+			    git_path("MERGE_MSG"), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))

diff --git a/commit.h b/commit.h
index 16d9c43..8d97a5c 100644
--- a/commit.h
+++ b/commit.h

@@ -304,4 +304,7 @@ extern void check_commit_signature(const struct commit* commit, struct signature
 
 int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
 
+LAST_ARG_MUST_BE_NULL
+extern int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...);
+
 #endif /* COMMIT_H */

diff --git a/run-command.c b/run-command.c
index 3914d9c..75abc47 100644
--- a/run-command.c
+++ b/run-command.c

@@ -760,13 +760,11 @@ char *find_hook(const char *name)
 	return path;
 }
 
-int run_hook(const char *index_file, const char *name, ...)
+int run_hook_ve(const char *const *env, const char *name, va_list args)
 {
 	struct child_process hook;
 	struct argv_array argv = ARGV_ARRAY_INIT;
-	const char *p, *env[2];
-	char index[PATH_MAX];
-	va_list args;
+	const char *p;
 	int ret;
 
 	p = find_hook(name);

@@ -775,23 +773,45 @@ int run_hook(const char *index_file, const char *name, ...)
 
 	argv_array_push(&argv, p);
 
-	va_start(args, name);
 	while ((p = va_arg(args, const char *)))
 		argv_array_push(&argv, p);
-	va_end(args);
 
 	memset(&hook, 0, sizeof(hook));
 	hook.argv = argv.argv;
+	hook.env = env;
 	hook.no_stdin = 1;
 	hook.stdout_to_stderr = 1;
-	if (index_file) {
-		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		env[0] = index;
-		env[1] = NULL;
-		hook.env = env;
-	}
 
 	ret = run_command(&hook);
 	argv_array_clear(&argv);
 	return ret;
 }
+
+int run_hook_le(const char *const *env, const char *name, ...)
+{
+	va_list args;
+	int ret;
+
+	va_start(args, name);
+	ret = run_hook_ve(env, name, args);
+	va_end(args);
+
+	return ret;
+}
+
+int run_hook_with_custom_index(const char *index_file, const char *name, ...)
+{
+	const char *hook_env[3] =  { NULL };
+	char index[PATH_MAX];
+	va_list args;
+	int ret;
+
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	hook_env[0] = index;
+
+	va_start(args, name);
+	ret = run_hook_ve(hook_env, name, args);
+	va_end(args);
+
+	return ret;
+}

diff --git a/run-command.h b/run-command.h
index 6b985af..88460f9 100644
--- a/run-command.h
+++ b/run-command.h

@@ -47,7 +47,11 @@ int run_command(struct child_process *);
 
 extern char *find_hook(const char *name);
 LAST_ARG_MUST_BE_NULL
-extern int run_hook(const char *index_file, const char *name, ...);
+extern int run_hook_le(const char *const *env, const char *name, ...);
+extern int run_hook_ve(const char *const *env, const char *name, va_list args);
+
+LAST_ARG_MUST_BE_NULL
+extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
 #define RUN_GIT_CMD	     2	/*If this is to be git sub-command */

diff --git a/t/t7513-commit_-p_-m_hunk_edit.sh b/t/t7513-commit_-p_-m_hunk_edit.sh
index 994939a..1f05d32 100755
--- a/t/t7513-commit_-p_-m_hunk_edit.sh
+++ b/t/t7513-commit_-p_-m_hunk_edit.sh

@@ -25,7 +25,7 @@ test_expect_success 'setup (initial)' '
 	EOF
 '
 
-test_expect_failure 'edit hunk "commit -p -m message"' '
+test_expect_success 'edit hunk "commit -p -m message"' '
 	echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
 	git diff HEAD^ HEAD >actual &&
 	test_cmp expect actual

-- 
1.9.0

---
 run-command.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/run-command.h b/run-command.h
index 88460f9..3653bfa 100644
--- a/run-command.h
+++ b/run-command.h

@@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);
 extern int run_hook_ve(const char *const *env, const char *name, va_list args);
 
 LAST_ARG_MUST_BE_NULL
+__attribute__((deprecated))
 extern int run_hook_with_custom_index(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1

-- 
1.9.0

On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:

Don't change git environment: move the GIT_EDITOR=":" override to the
hook command subprocess, like it's already done for GIT_INDEX_FILE.

Signed-off-by: Benoit Pierre <redacted>
---
 builtin/checkout.c                |  8 +++----
 builtin/clone.c                   |  4 ++--
 builtin/commit.c                  | 35 ++++++++++++++++++++++++-------
 builtin/gc.c                      |  2 +-
 builtin/merge.c                   |  6 +++---
 commit.h                          |  3 +++
 run-command.c                     | 44 ++++++++++++++++++++++++++++-----------
 run-command.h                     |  6 +++++-
 t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-
 9 files changed, 79 insertions(+), 31 deletions(-)

This is a lot of change, and in some ways I think it is making things
better overall. However, the simplest fix for this is basically to move
the setting of GIT_EDITOR down to after we prepare the index.

Jun Hao (cc'd) has been preparing a series for this based on the
Bloomberg git hackday a few weeks ago, but it hasn't been sent to the
list yet.

Commits are here:

  https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing

if you care to look. I'm not sure which solution is technically
superior, but it's worth considering both.

I regret not encouraging Jun to post to the list sooner, as we might
have avoided some duplicated effort. But that's a sunk cost, and we
should pick up whichever is the best for the project.

-Peff

On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre [off-list ref] wrote:

Add (failing) test: with commit changing the environment to let hooks
now that no editor will be used (by setting GIT_EDITOR to ":"), the
"edit hunk" functionality does not work (no editor is launched and the
whole hunk is committed).

Signed-off-by: Benoit Pierre <redacted>
---
 t/t7513-commit_-p_-m_hunk_edit.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100755 t/t7513-commit_-p_-m_hunk_edit.sh

Is it possible to give this file a name less unusual and more
consistent with other test scripts? Perhaps choose a more generic name
which may allow other similar tests to be added to the file in the
future (if needed)?

In the previous review, the suggestion was that creation of 'expect'
should be moved to the test below where it is actually used rather
than into the 'setup' phase above.

+'
+
+test_expect_failure 'edit hunk "commit -p -m message"' '
+       echo e | env GIT_EDITOR="sed s/+line3\$/+line2/ -i" git commit -p -m commit2 file &&
+       git diff HEAD^ HEAD >actual &&
+       test_cmp expect actual
+'
+
+test_done
--
1.9.0

On Mon, Mar 10, 2014 at 2:49 PM, Benoit Pierre [off-list ref] wrote:

- update 'no editor' hook test and add 'editor' hook test
- make sure the tree is reset to a clean state after running a test
  (using test_when_finished) so later tests are not impacted

As conceptually distinct changes, it might make sense to split these
into two patches, one a purely cleanup patch adding
test_when_finished, and the second introducing your new test. Doing so
would ease the review process and make the intended changes more
obvious.

mincro nit.
From: "Benoit Pierre" <redacted>

Add (failing) test: with commit changing the environment to let hooks
now that no editor will be used (by setting GIT_EDITOR to ":"), the

s/now/know/

Jeff King [off-list ref] writes:

On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:

quoted

Don't change git environment: move the GIT_EDITOR=":" override to the
hook command subprocess, like it's already done for GIT_INDEX_FILE.

Signed-off-by: Benoit Pierre <redacted>
---
 builtin/checkout.c                |  8 +++----
 builtin/clone.c                   |  4 ++--
 builtin/commit.c                  | 35 ++++++++++++++++++++++++-------
 builtin/gc.c                      |  2 +-
 builtin/merge.c                   |  6 +++---
 commit.h                          |  3 +++
 run-command.c                     | 44 ++++++++++++++++++++++++++++-----------
 run-command.h                     |  6 +++++-
 t/t7513-commit_-p_-m_hunk_edit.sh |  2 +-
 9 files changed, 79 insertions(+), 31 deletions(-)

This is a lot of change, and in some ways I think it is making things
better overall. However, the simplest fix for this is basically to move
the setting of GIT_EDITOR down to after we prepare the index.

Jun Hao (cc'd) has been preparing a series for this based on the
Bloomberg git hackday a few weeks ago, but it hasn't been sent to the
list yet.

Commits are here:

  https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing

if you care to look. I'm not sure which solution is technically
superior, but it's worth considering both.

I regret not encouraging Jun to post to the list sooner, as we might
have avoided some duplicated effort. But that's a sunk cost, and we
should pick up whichever is the best for the project.

-Peff

I like the idea that the environment setting should be done in one
place. Just not sure run_hook is the right place tho. If user doesn't have
any hook setup, will this kick in? 
One more question, will this work for dry run? Or dry run doesn't matter
in this case?

Thanks - Jun

On Mon, Mar 10, 2014 at 07:49:37PM +0100, Benoit Pierre wrote:

It doesn't appear that we use the deprecated attribute anywhere else in
the code.  Wouldn't it just be better to change the places that use this
and then remove the function altogether?  I imagine your current patch
might introduce a number of warnings that some people would rather
avoid.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

On Tue, Mar 11, 2014 at 2:00 AM, brian m. carlson
[off-list ref] wrote:

On Mon, Mar 10, 2014 at 07:49:37PM +0100, Benoit Pierre wrote:

quoted

---
 run-command.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/run-command.h b/run-command.h
index 88460f9..3653bfa 100644
--- a/run-command.h
+++ b/run-command.h

@@ -51,6 +51,7 @@ extern int run_hook_le(const char *const *env, const char *name, ...);
 extern int run_hook_ve(const char *const *env, const char *name, va_list args);

 LAST_ARG_MUST_BE_NULL
+__attribute__((deprecated))

It doesn't appear that we use the deprecated attribute anywhere else in
the code.  Wouldn't it just be better to change the places that use this
and then remove the function altogether?  I imagine your current patch
might introduce a number of warnings that some people would rather
avoid.

This last patch is optional. There are no callers to
run_hook_with_custom_index, so no warnings for now. See discussion on
first draft as to why I added it:

http://article.gmane.org/gmane.comp.version-control.git/243561

-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?

On Mon, Mar 10, 2014 at 9:07 PM, Jeff King [off-list ref] wrote:

On Mon, Mar 10, 2014 at 07:49:34PM +0100, Benoit Pierre wrote:

quoted

Don't change git environment: move the GIT_EDITOR=":" override to the
hook command subprocess, like it's already done for GIT_INDEX_FILE.
[...]

This is a lot of change, and in some ways I think it is making things
better overall. However, the simplest fix for this is basically to move
the setting of GIT_EDITOR down to after we prepare the index.

Jun Hao (cc'd) has been preparing a series for this based on the
Bloomberg git hackday a few weeks ago, but it hasn't been sent to the
list yet.

Commits are here:

  https://github.com/bloomberg/git/compare/commit-patch-allow-hunk-editing

if you care to look. I'm not sure which solution is technically
superior, but it's worth considering both.

I regret not encouraging Jun to post to the list sooner, as we might
have avoided some duplicated effort. But that's a sunk cost, and we
should pick up whichever is the best for the project.

Replying here instead of to Jun Hao message (I'm not subscribed to the
mailing list, so I did not received it):

Jun Hao wrote:

I like the idea that the environment setting should be done in one
place. Just not sure run_hook is the right place tho. If user doesn't have
any hook setup, will this kick in?
One more question, will this work for dry run? Or dry run doesn't matter
in this case?

According to the original commit, the change to GIT_EDITOR is only
here for hooks:

commit 406400ce4f69e79b544dd3539a71b85d99331820
Author: Paolo Bonzini [off-list ref]
Date:   Tue Feb 5 11:01:45 2008 +0100

    git-commit: set GIT_EDITOR=: if editor will not be launched

    This is a preparatory patch that provides a simple way for the future
    prepare-commit-msg hook to discover if the editor will be launched.

    Signed-off-by: Junio C Hamano [off-list ref]

So there is really no reason to set it earlier, and not just in the
hook subprocess environment.

Regarding dry run: the bug is present, and my patch fix it too (but is
missing a test for this).

As to which patch is better: it's really not for me to decide! It's a
question for the maintainer(s), Jun Hao patch is sure much smaller and
simpler, mine is more involved and I believe cleaner in the long term:
there is no risk of another part of the commit process to be impacted
by the change of environment. Also note that my patch changes the
merge builtin too: GIT_EDITOR will not be overriden if an editor will
be launched (when used with --edit).

-- 
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?

On Tue, Mar 11, 2014 at 06:56:02PM +0100, Benoit Pierre wrote:

According to the original commit, the change to GIT_EDITOR is only
here for hooks:

commit 406400ce4f69e79b544dd3539a71b85d99331820
Author: Paolo Bonzini [off-list ref]
Date:   Tue Feb 5 11:01:45 2008 +0100

    git-commit: set GIT_EDITOR=: if editor will not be launched

    This is a preparatory patch that provides a simple way for the future
    prepare-commit-msg hook to discover if the editor will be launched.

    Signed-off-by: Junio C Hamano [off-list ref]

So there is really no reason to set it earlier, and not just in the
hook subprocess environment.

Ah, you're right. I was thinking that our invocation of launch_editor
also respected it. It does, but we never get there at all because
use_editor is set to 0. So yeah, it really is just needed for the hook.

Your patch, even though it is a bigger change, keeps the setting to the
minimal area, which is cleaner and more maintainable in the long run.

-Peff

Jeff King <peff <at> peff.net> writes:

Ah, you're right. I was thinking that our invocation of launch_editor
also respected it. It does, but we never get there at all because
use_editor is set to 0. So yeah, it really is just needed for the hook.

Your patch, even though it is a bigger change, keeps the setting to the
minimal area, which is cleaner and more maintainable in the long run.

-Peff

Oh, didn't realize GIT_EDITOR change is only for hooks. Good catch. I agree 
Benoit's patch is better for the long term.

Thanks - Jun

Hi,

A quick note for the future:

Benoit Pierre wrote:

This patch fixes the fact that hunk editing with 'commit -p -m' does not work:
GIT_EDITOR is set to ':' to indicate to hooks that no editor will be launched,
which result in the 'hunk edit' option not launching the editor (and selecting
the whole hunk).

This information should have gone in the relevant patch's commit
message itself.  That way, people don't have to hunt down the relevant
mailing list thread to understand the patch.

Generally a cover letter should say as little as possible (mostly
"here is what patch you might want to look at first, and here is an
overview of why the patches are in this particular order").

Thanks for a nice fix.  Perhaps we'll see more in the future, hence
this note. :)  And if you have ideas for where an explanation of this
could go in the documentation (somewhere in
Documentation/SubmittingPatches?), that would be welcome too.

Thanks,
Jonathan