Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
Jonathan Nieder jrnie...@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
From 9b431e5206652cf62ebb09dad4607989976e7748 Mon Sep 17 00:00:00 2001
From: Ryan Biesemeyer r...@yaauie.com
Date: Wed, 8 Jan 2014 00:46:41 +
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 r...@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
signature.asc
Description: Message signed with OpenPGP using GPGMail
Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
From a1f898fdf560e719d447a544569d5d1457307855 Mon Sep 17 00:00:00 2001
From: Ryan Biesemeyer r...@yaauie.com
Date: Wed, 8 Jan 2014 00:47:41 +
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 r...@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
signature.asc
Description: Message signed with OpenPGP using GPGMail
Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
On 2014-01-08, at 20:06Z, Matthieu Moy matthieu@grenoble-inp.fr wrote:
Ryan Biesemeyer r...@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@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)
signature.asc
Description: Message signed with OpenPGP using GPGMail
Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
