Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
Jonathan Nieder 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
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
Re: [PATCH] merge: make merge state available to prepare-commit-msg hook
Ryan Biesemeyer 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=" > 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/ -- 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
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
On 2014-01-08, at 20:06Z, Matthieu Moy wrote: > Ryan Biesemeyer 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 > 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
Ryan Biesemeyer 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 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/ -- 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 a1f898fdf560e719d447a544569d5d1457307855 Mon Sep 17 00:00:00 2001 From: Ryan Biesemeyer 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 --- 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
From 9b431e5206652cf62ebb09dad4607989976e7748 Mon Sep 17 00:00:00 2001 From: Ryan Biesemeyer 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 --- 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
[PATCH] merge: make merge state available to prepare-commit-msg hook
From a1f898fdf560e719d447a544569d5d1457307855 Mon Sep 17 00:00:00 2001 From: Ryan Biesemeyer Date: Wed, 8 Jan 2014 04:22:12 + 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 signature.asc Description: Message signed with OpenPGP using GPGMail