[PATCH] merge: make merge state available to prepare-commit-msg hook

[PATCH] merge: make merge state available to prepare-commit-msg hook

[Ryan Biesemeyer]

[-- Attachment #1: Type: text/plain, Size: 867 bytes --]

From a1f898fdf560e719d447a544569d5d1457307855 Mon Sep 17 00:00:00 2001
From: Ryan Biesemeyer <ryan@yaauie.com>
Date: Wed, 8 Jan 2014 04:22:12 +0000
Subject: [PATCH 0/2] merge make merge state available to prepare-commit-msg hook

Since prepare-commit-msg hook is given 'merge' as an argument when a merge is
taking place, it was surprising that the merge state (MERGE_HEAD, etc.) was not
present for the hook's execution.

Make sure that the merge state is written before the prepare-commit-msg hook is
called.

Ryan Biesemeyer (2):
  merge: make prepare_to_commit responsible for write_merge_state
  merge: drop unused arg from abort_commit method signature

 builtin/merge.c                    | 13 +++++++------
 t/t7505-prepare-commit-msg-hook.sh | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)

-- 
1.8.5



[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] merge: make merge state available to prepare-commit-msg hook

[Ryan Biesemeyer]

[-- Attachment #1: Type: text/plain, Size: 2534 bytes --]

From 9b431e5206652cf62ebb09dad4607989976e7748 Mon Sep 17 00:00:00 2001
From: Ryan Biesemeyer <ryan@yaauie.com>
Date: Wed, 8 Jan 2014 00:46:41 +0000
Subject: [PATCH 1/2] merge: make prepare_to_commit responsible for
 write_merge_state

When merging, make the prepare-commit-msg hook have access to the merge
state in order to make decisions about the commit message it is preparing.

Since `abort_commit` is *only* called after `write_merge_state`, and a
successful commit *always* calls `drop_save` to clear the saved state, this
change effectively ensures that the merge state is also available to the
prepare-commit-msg hook and while the message is being edited.

Signed-off-by: Ryan Biesemeyer <ryan@yaauie.com>
---
 builtin/merge.c                    |  3 ++-
 t/t7505-prepare-commit-msg-hook.sh | 22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 4941a6c..b7bfc9c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -802,7 +802,6 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
 		error("%s", err_msg);
 	fprintf(stderr,
 		_("Not committing merge; use 'git commit' to complete the merge.\n"));
-	write_merge_state(remoteheads);
 	exit(1);
 }
 
@@ -816,6 +815,8 @@ N_("Please enter a commit message to explain why this merge is necessary,\n"
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+
+	write_merge_state(remoteheads);
 	strbuf_addbuf(&msg, &merge_msg);
 	strbuf_addch(&msg, '\n');
 	if (0 < option_edit)
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 3573751..89cdfe8 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -181,5 +181,27 @@ test_expect_success 'with failing hook (merge)' '
 	test_must_fail git merge other
 
 '
+git merge --abort # cleanup, since the merge failed.
+
+test_expect_success 'should have MERGE_HEAD (merge)' '
+
+	git checkout -B other HEAD@{1} &&
+	echo "more" >> file &&
+	git add file &&
+	rm -f "$HOOK" &&
+	git commit -m other &&
+	git checkout - &&
+	write_script "$HOOK" <<-EOF
+	if [ -s "$(git rev-parse --git-dir)/MERGE_HEAD" ]; then
+		exit 0
+	else
+		exit 1
+	fi
+	EOF
+	git merge other &&
+	test "`git log -1 --pretty=format:%s`" = "Merge branch '"'"'other'"'"'" &&
+	test ! -s "$(git rev-parse --git-dir)/MERGE_HEAD"
+
+# '
 
 test_done
-- 
1.8.5


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] merge: make merge state available to prepare-commit-msg hook

[Ryan Biesemeyer]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

From a1f898fdf560e719d447a544569d5d1457307855 Mon Sep 17 00:00:00 2001
From: Ryan Biesemeyer <ryan@yaauie.com>
Date: Wed, 8 Jan 2014 00:47:41 +0000
Subject: [PATCH 2/2] merge: drop unused arg from abort_commit method signature

Since abort_commit is no longer responsible for writing merge state, remove
the unused argument that was originally needed solely for writing merge state.

Signed-off-by: Ryan Biesemeyer <ryan@yaauie.com>
---
 builtin/merge.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index b7bfc9c..c3108cf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -795,8 +795,7 @@ static void read_merge_msg(struct strbuf *msg)
 		die_errno(_("Could not read from '%s'"), filename);
 }
 
-static void write_merge_state(struct commit_list *);
-static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
+static void abort_commit(const char *err_msg)
 {
 	if (err_msg)
 		error("%s", err_msg);
@@ -812,6 +811,7 @@ N_("Please enter a commit message to explain why this merge is necessary,\n"
    "Lines starting with '%c' will be ignored, and an empty message aborts\n"
    "the commit.\n");
 
+static void write_merge_state(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
@@ -824,15 +824,15 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	write_merge_msg(&msg);
 	if (run_hook(get_index_file(), "prepare-commit-msg",
 		     git_path("MERGE_MSG"), "merge", NULL, NULL))
-		abort_commit(remoteheads, NULL);
+		abort_commit(NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
-			abort_commit(remoteheads, NULL);
+			abort_commit(NULL);
 	}
 	read_merge_msg(&msg);
 	stripspace(&msg, 0 < option_edit);
 	if (!msg.len)
-		abort_commit(remoteheads, _("Empty commit message."));
+		abort_commit(_("Empty commit message."));
 	strbuf_release(&merge_msg);
 	strbuf_addbuf(&merge_msg, &msg);
 	strbuf_release(&msg);
-- 
1.8.5



[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] merge: make merge state available to prepare-commit-msg hook

[Matthieu Moy]

Ryan Biesemeyer <ryan@yaauie.com> writes:

> index 3573751..89cdfe8 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -181,5 +181,27 @@ test_expect_success 'with failing hook (merge)' '
>  	test_must_fail git merge other
>  
>  '
> +git merge --abort # cleanup, since the merge failed.

Please, avoid having code outside a test_expect_* (see t/README, " - Put
all code inside test_expect_success and other assertions.").

> +test_expect_success 'should have MERGE_HEAD (merge)' '
> +
> +	git checkout -B other HEAD@{1} &&
> +	echo "more" >> file &&
> +	git add file &&
> +	rm -f "$HOOK" &&
> +	git commit -m other &&
> +	git checkout - &&
> +	write_script "$HOOK" <<-EOF
> +	if [ -s "$(git rev-parse --git-dir)/MERGE_HEAD" ]; then
> +		exit 0
> +	else
> +		exit 1
> +	fi
> +	EOF

I think you lack one && for the write_script line.

There's another instance in the same file (probably where you got it
from), you should add this to your patch series:

>From c3d754a2a16d98b31d43a7e45973821ae8adc043 Mon Sep 17 00:00:00 2001
From: Matthieu Moy <Matthieu.Moy@imag.fr>
Date: Wed, 8 Jan 2014 21:03:27 +0100
Subject: [PATCH] t7505: add missing &&

---
 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.8.5.rc3.4.g8bd3721

(a quick "git grep write_script" seems to show a lot of other instances,
but no time to dig this now sorry)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] merge: make merge state available to prepare-commit-msg hook

[Ryan Biesemeyer]

[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]


On 2014-01-08, at 20:06Z, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

> Ryan Biesemeyer <ryan@yaauie.com> writes:
> 
>> index 3573751..89cdfe8 100755
>> --- a/t/t7505-prepare-commit-msg-hook.sh
>> +++ b/t/t7505-prepare-commit-msg-hook.sh
>> @@ -181,5 +181,27 @@ test_expect_success 'with failing hook (merge)' '
>> 	test_must_fail git merge other
>> 
>> '
>> +git merge --abort # cleanup, since the merge failed.
> 
> Please, avoid having code outside a test_expect_* (see t/README, " - Put
> all code inside test_expect_success and other assertions.").

In this case it was not immediately clear to me how to add cleanup to an existing
test that dirtied the state of the test repository by leaving behind an in-progress
merge. I see `test_cleanup` defined in the test lib & related functions, but see no
examples of its use in the test suites. Could you advise? 

> 
>> +test_expect_success 'should have MERGE_HEAD (merge)' '
>> +
>> +	git checkout -B other HEAD@{1} &&
>> +	echo "more" >> file &&
>> +	git add file &&
>> +	rm -f "$HOOK" &&
>> +	git commit -m other &&
>> +	git checkout - &&
>> +	write_script "$HOOK" <<-EOF
>> +	if [ -s "$(git rev-parse --git-dir)/MERGE_HEAD" ]; then
>> +		exit 0
>> +	else
>> +		exit 1
>> +	fi
>> +	EOF
> 
> I think you lack one && for the write_script line.

Good catch.

> There's another instance in the same file (probably where you got it
> from), you should add this to your patch series:

I'm new to the mailing-list patch submission process; how would I go about adding it?
Submit the cover-letter & patches again? Squash your commit into the relevant one of
mine?

> From c3d754a2a16d98b31d43a7e45973821ae8adc043 Mon Sep 17 00:00:00 2001
> From: Matthieu Moy <Matthieu.Moy@imag.fr>
> Date: Wed, 8 Jan 2014 21:03:27 +0100
> Subject: [PATCH] t7505: add missing &&
> 
> ---
> 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.8.5.rc3.4.g8bd3721
> 
> (a quick "git grep write_script" seems to show a lot of other instances,
> but no time to dig this now sorry)
> 
> -- 
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Thanks for the review.

--
Ryan Biesemeyer (@yaauie)


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] merge: make merge state available to prepare-commit-msg hook

[Jonathan Nieder]

Hi,

Ryan Biesemeyer wrote:

> In this case it was not immediately clear to me how to add cleanup to an existing
> test that dirtied the state of the test repository by leaving behind an in-progress
> merge. I see `test_cleanup` defined in the test lib & related functions, but see no
> examples of its use in the test suites. Could you advise? 

test_when_finished pushes a command to be run unconditionally
when the current test assertion finishes.

Thanks,
Jonathan

Re: [PATCH] merge: make merge state available to prepare-commit-msg hook

[Matthieu Moy]

Ryan Biesemeyer <ryan@yaauie.com> writes:

> In this case it was not immediately clear to me how to add cleanup to an existing
> test that dirtied the state of the test repository by leaving behind an in-progress
> merge.

Jonathan's answer is an option. Another one is

test_expect_success 'cleanup' '
	git reset ...
'

So if the cleanup goes wrong, one can notice.

> I'm new to the mailing-list patch submission process; how would I go
> about adding it?

You can apply my patch with "git am" in your tree (or at worse, do it by
hand and steal authorship, I don't mind for a 2 characters patch ;-) ),
fix your patch to add the missing &&, and then resend with stg like "git
send-email -v2 --in-reply-to=<old-msg-id>"

> Submit the cover-letter & patches again?

Definitely submit patches again. Usually, the cover letter for a resend
emphasizes on changes compared to previous version.

> Squash your commit into the relevant one of mine?

Preferably not, as my fix is unrelated from yours (mine can come before,
as a cleanup).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] merge: make merge state available to prepare-commit-msg hook

[Jonathan Nieder]

Matthieu Moy wrote:

> Jonathan's answer is an option. Another one is
[...]
> So if the cleanup goes wrong, one can notice.

test_when_finished also makes the test fail if the cleanup failed.

Another common strategy is

	test_expect_success 'my exciting test' '
		# this test will rely on these files being absent
		rm -f a b c etc &&

		... rest of the test goes here ...
	'

which can be a handy way for an especially picky test to protect
itself (for example with 'git clean -fdx') regardless of the state
other test assertions create for it.

This particular example (merge --abort) seems like a good use for
test_when_finished because it is about a specific test having made a
mess and needing to clean up after itself to restore sanity.

Hoping that clarifies,
Jonathan

[PATCH v2 0/4] merge: make merge state available to prepare-commit-msg hook

[Ryan Biesemeyer]

Ensure merge state is available to the prepare-commit-msg hook.

v2 patchset incorporates early feedback from the list

Matthieu Moy (1):
  t7505: add missing &&

Ryan Biesemeyer (3):
  t7505: ensure cleanup after hook blocks merge
  merge: make prepare_to_commit responsible for write_merge_state
  merge: drop unused arg from abort_commit method signature

 builtin/merge.c                    | 13 +++++++------
 t/t7505-prepare-commit-msg-hook.sh | 30 ++++++++++++++++++++++++++----
 2 files changed, 33 insertions(+), 10 deletions(-)

-- 
1.8.5

[PATCH v2 1/4] t7505: add missing &&

[Ryan Biesemeyer]

From: Matthieu Moy <Matthieu.Moy@imag.fr>

Signed-off-by: Ryan Biesemeyer <ryan@yaauie.com>
---
 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.8.5

[PATCH v2 2/4] t7505: ensure cleanup after hook blocks merge

[Ryan Biesemeyer]

Signed-off-by: Ryan Biesemeyer <ryan@yaauie.com>
---
 t/t7505-prepare-commit-msg-hook.sh | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 1c95652..697ecc0 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -168,18 +168,19 @@ test_expect_success 'with failing hook (--no-verify)' '
 '
 
 test_expect_success 'with failing hook (merge)' '
-
-	git checkout -B other HEAD@{1} &&
-	echo "more" >> file &&
-	git add file &&
-	rm -f "$HOOK" &&
-	git commit -m other &&
-	write_script "$HOOK" <<-EOF &&
-	exit 1
-	EOF
-	git checkout - &&
-	test_must_fail git merge other
-
+  test_when_finished "git merge --abort" &&
+  (
+		git checkout -B other HEAD@{1} &&
+		echo "more" >> file &&
+		git add file &&
+		rm -f "$HOOK" &&
+		git commit -m other &&
+		write_script "$HOOK" <<-EOF &&
+		exit 1
+		EOF
+		git checkout - &&
+		test_must_fail git merge other
+  )
 '
 
 test_done
-- 
1.8.5

[PATCH v2 3/4] merge: make prepare_to_commit responsible for write_merge_state

[Ryan Biesemeyer]

When merging, make the prepare-commit-msg hook have access to the merge
state in order to make decisions about the commit message it is preparing.

Since `abort_commit` is *only* called after `write_merge_state`, and a
successful commit *always* calls `drop_save` to clear the saved state, this
change effectively ensures that the merge state is also available to the
prepare-commit-msg hook and while the message is being edited.

Signed-off-by: Ryan Biesemeyer <ryan@yaauie.com>
---
 builtin/merge.c                    |  3 ++-
 t/t7505-prepare-commit-msg-hook.sh | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 4941a6c..b7bfc9c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -802,7 +802,6 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
 		error("%s", err_msg);
 	fprintf(stderr,
 		_("Not committing merge; use 'git commit' to complete the merge.\n"));
-	write_merge_state(remoteheads);
 	exit(1);
 }
 
@@ -816,6 +815,8 @@ N_("Please enter a commit message to explain why this merge is necessary,\n"
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
+
+	write_merge_state(remoteheads);
 	strbuf_addbuf(&msg, &merge_msg);
 	strbuf_addch(&msg, '\n');
 	if (0 < option_edit)
diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
index 697ecc0..7ccf870 100755
--- a/t/t7505-prepare-commit-msg-hook.sh
+++ b/t/t7505-prepare-commit-msg-hook.sh
@@ -183,4 +183,25 @@ test_expect_success 'with failing hook (merge)' '
   )
 '
 
+test_expect_success 'should have MERGE_HEAD (merge)' '
+
+	git checkout -B other HEAD@{1} &&
+	echo "more" >> file &&
+	git add file &&
+	rm -f "$HOOK" &&
+	git commit -m other &&
+	git checkout - &&
+	write_script "$HOOK" <<-EOF &&
+	if [ -s "$(git rev-parse --git-dir)/MERGE_HEAD" ]; then
+		exit 0
+	else
+		exit 1
+	fi
+	EOF
+	git merge other &&
+	test "`git log -1 --pretty=format:%s`" = "Merge branch '"'"'other'"'"'" &&
+	test ! -s "$(git rev-parse --git-dir)/MERGE_HEAD"
+
+'
+
 test_done
-- 
1.8.5

[PATCH v2 4/4] merge: drop unused arg from abort_commit method signature

[Ryan Biesemeyer]

Since abort_commit is no longer responsible for writing merge state, remove
the unused argument that was originally needed solely for writing merge state.

Signed-off-by: Ryan Biesemeyer <ryan@yaauie.com>
---
 builtin/merge.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index b7bfc9c..c3108cf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -795,8 +795,7 @@ static void read_merge_msg(struct strbuf *msg)
 		die_errno(_("Could not read from '%s'"), filename);
 }
 
-static void write_merge_state(struct commit_list *);
-static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
+static void abort_commit(const char *err_msg)
 {
 	if (err_msg)
 		error("%s", err_msg);
@@ -812,6 +811,7 @@ N_("Please enter a commit message to explain why this merge is necessary,\n"
    "Lines starting with '%c' will be ignored, and an empty message aborts\n"
    "the commit.\n");
 
+static void write_merge_state(struct commit_list *);
 static void prepare_to_commit(struct commit_list *remoteheads)
 {
 	struct strbuf msg = STRBUF_INIT;
@@ -824,15 +824,15 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	write_merge_msg(&msg);
 	if (run_hook(get_index_file(), "prepare-commit-msg",
 		     git_path("MERGE_MSG"), "merge", NULL, NULL))
-		abort_commit(remoteheads, NULL);
+		abort_commit(NULL);
 	if (0 < option_edit) {
 		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
-			abort_commit(remoteheads, NULL);
+			abort_commit(NULL);
 	}
 	read_merge_msg(&msg);
 	stripspace(&msg, 0 < option_edit);
 	if (!msg.len)
-		abort_commit(remoteheads, _("Empty commit message."));
+		abort_commit(_("Empty commit message."));
 	strbuf_release(&merge_msg);
 	strbuf_addbuf(&merge_msg, &msg);
 	strbuf_release(&msg);
-- 
1.8.5

Re: [PATCH v2 2/4] t7505: ensure cleanup after hook blocks merge

[Matthieu Moy]

Ryan Biesemeyer <ryan@yaauie.com> writes:

> +  test_when_finished "git merge --abort" &&
> +  (
> +		git checkout -B other HEAD@{1} &&

Weird indentation (space/tab mix).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] merge: make merge state available to prepare-commit-msg hook

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Matthieu Moy wrote:
>
>> Jonathan's answer is an option. Another one is
> [...]
>> So if the cleanup goes wrong, one can notice.
>
> test_when_finished also makes the test fail if the cleanup failed.

Yes, I was mentionning it as opposed to "throwing the code at the
toplevel of the shell", not as opposed to test_when_finished.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 2/4] t7505: ensure cleanup after hook blocks merge

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Ryan Biesemeyer <ryan@yaauie.com> writes:
>
>> +  test_when_finished "git merge --abort" &&
>> +  (
>> +		git checkout -B other HEAD@{1} &&
>
> Weird indentation (space/tab mix).

Also I do not quite see why the body has to be in a subshell.

Re: [PATCH v2 3/4] merge: make prepare_to_commit responsible for write_merge_state

[Junio C Hamano]

Ryan Biesemeyer <ryan@yaauie.com> writes:

> When merging, make the prepare-commit-msg hook have access to the merge
> state in order to make decisions about the commit message it is preparing.

What information is currently not available, and if available how
would that help the hook to formulate a better message?

	I am not asking you to answer the question to me in an
	e-mail response. The above is an example of a natural
	question a reader of the above statement would have and
	would wish the log message already answered when the reader
	read it.

> Since `abort_commit` is *only* called after `write_merge_state`, and a
> successful commit *always* calls `drop_save` to clear the saved state, this
> change effectively ensures that the merge state is also available to the
> prepare-commit-msg hook and while the message is being edited.
>
> Signed-off-by: Ryan Biesemeyer <ryan@yaauie.com>
> ---

OK.  The most important part is that this makes sure that these
intermediate state files never remains after the normal codepath
finishes what it does.

You seem to be only interested in prepare-commit-msg hook, but
because of your calling write_merge_state() early, these state files
will persist while we call finish() and they are also visible while
the post-merge hook is run.  While I think it may be a good thing
that the post-merge hook too can view that information, the log
message makes it sound as if it is an unintended side effect of the
change.

>  builtin/merge.c                    |  3 ++-
>  t/t7505-prepare-commit-msg-hook.sh | 21 +++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 4941a6c..b7bfc9c 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -802,7 +802,6 @@ static void abort_commit(struct commit_list *remoteheads, const char *err_msg)
>  		error("%s", err_msg);
>  	fprintf(stderr,
>  		_("Not committing merge; use 'git commit' to complete the merge.\n"));
> -	write_merge_state(remoteheads);
>  	exit(1);
>  }
>  
> @@ -816,6 +815,8 @@ N_("Please enter a commit message to explain why this merge is necessary,\n"
>  static void prepare_to_commit(struct commit_list *remoteheads)
>  {
>  	struct strbuf msg = STRBUF_INIT;
> +
> +	write_merge_state(remoteheads);
>  	strbuf_addbuf(&msg, &merge_msg);
>  	strbuf_addch(&msg, '\n');
>  	if (0 < option_edit)
> diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh
> index 697ecc0..7ccf870 100755
> --- a/t/t7505-prepare-commit-msg-hook.sh
> +++ b/t/t7505-prepare-commit-msg-hook.sh
> @@ -183,4 +183,25 @@ test_expect_success 'with failing hook (merge)' '
>    )
>  '
>  
> +test_expect_success 'should have MERGE_HEAD (merge)' '
> +
> +	git checkout -B other HEAD@{1} &&
> +	echo "more" >> file &&

Style: no SP between the redirection operator and its target, i.e.

	echo more >>file &&

> +	git add file &&
> +	rm -f "$HOOK" &&
> +	git commit -m other &&
> +	git checkout - &&
> +	write_script "$HOOK" <<-EOF &&
> +	if [ -s "$(git rev-parse --git-dir)/MERGE_HEAD" ]; then

Style: no [], and no semicolon to join two lines of control
statement into one, i.e.

	if test -s ...
	then

> +		exit 0
> +	else
> +		exit 1
> +	fi
> +	EOF
> +	git merge other &&
> +	test "`git log -1 --pretty=format:%s`" = "Merge branch '"'"'other'"'"'" &&

Style:

    - After "sh t7505-*.sh v -i" fails for whatever reason, being
      able to inspect the trash directory to see what actually was
      produced is much easier way to debug than having to re-run the
      command with "sh -x" to peek into what the "test" statement is
      getting.

    - $(...) is much easier to read than `...`, but you do not have
      to use either if you follow the above.

i.e.

	git log -1 --format=%s >actual &&
        echo "Merge branch '\''other'\''" >expect &&
        test_cmp expect actual &&

> +	test ! -s "$(git rev-parse --git-dir)/MERGE_HEAD"
> +
> +'
> +
>  test_done

Re: [PATCH v2 3/4] merge: make prepare_to_commit responsible for write_merge_state

[Junio C Hamano]

Ryan Biesemeyer <ryan@yaauie.com> writes:

> +	write_script "$HOOK" <<-EOF &&
> +	if [ -s "$(git rev-parse --git-dir)/MERGE_HEAD" ]; then
> +		exit 0
> +	else
> +		exit 1
> +	fi
> +	EOF

The script can be a one-liner

	write_scirpt "$HOOK" <<-\EOF &&
        test -s "$(git rev-parse --git-dir)/MERGE_HEAD"
	EOF

can't it?  I also do not think you want to have the rev-parse run
while writing the script (rather, you would want it run inside the
script, no?)

> +	git merge other &&
> +	test "`git log -1 --pretty=format:%s`" = "Merge branch '"'"'other'"'"'" &&
> +	test ! -s "$(git rev-parse --git-dir)/MERGE_HEAD"
> +
> +'
> +
>  test_done