| Message ID | cover.1564695892.git.steadmon@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pre-merge-commit hook | expand |
[Dropped cc-list the first time around. Apologies to those who receive this twice...] On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote: > > This series adds a new pre-merge-commit hook, similar in usage to > pre-commit. It also improves hook testing in t7503, by verifying that > the correct hooks are run or bypassed as expected. I really like those test improvements. Now it should be harder to mess up a future refactoring and run the wrong hook. These hooks are "very related" so I think this is important. I've messed with the test a bit and offer these potential improvements for your consideration. I was lazy and just built this on top of your series -- if you agree to some or all of these, you'll probably need to squash them into a few individual patches. The first four are perhaps more or less a matter of opinion, although I do think that patch 2/5 is based on an opinion shared by others. ;-) Patch 5/5 or something like it seems pretty important to me to make sure that of these two "similar"/"related" hooks with some "backwards-compatibility-and-code-copying-history" around them, we'd better pick the right one when they're both available. Feel free to pick and squash as you see fit. (I don't think it makes sense to have these go in as-are. They really are meant for squashing.) Martin Martin Ågren (5): t7503: use "&&" in "test_when_finished" rather than ";" t7503: avoid touch when mtime doesn't matter t7503: simplify file-juggling t7503: don't create "actual_hooks" for later appending t7503: test failing merge with both hooks available ...3-pre-commit-and-pre-merge-commit-hooks.sh | 84 +++++++++++-------- 1 file changed, 50 insertions(+), 34 deletions(-)
On 2019.08.02 11:56, Martin Ågren wrote: > [Dropped cc-list the first time around. Apologies to those who receive > this twice...] > > On Fri, 2 Aug 2019 at 00:20, Josh Steadmon <steadmon@google.com> wrote: > > > > This series adds a new pre-merge-commit hook, similar in usage to > > pre-commit. It also improves hook testing in t7503, by verifying that > > the correct hooks are run or bypassed as expected. > > I really like those test improvements. Now it should be harder to mess > up a future refactoring and run the wrong hook. These hooks are "very > related" so I think this is important. > > I've messed with the test a bit and offer these potential improvements > for your consideration. I was lazy and just built this on top of your > series -- if you agree to some or all of these, you'll probably need to > squash them into a few individual patches. > > The first four are perhaps more or less a matter of opinion, although I > do think that patch 2/5 is based on an opinion shared by others. ;-) > Patch 5/5 or something like it seems pretty important to me to make > sure that of these two "similar"/"related" hooks with some > "backwards-compatibility-and-code-copying-history" around them, we'd > better pick the right one when they're both available. > > Feel free to pick and squash as you see fit. (I don't think it makes > sense to have these go in as-are. They really are meant for squashing.) > > Martin > > Martin Ågren (5): > t7503: use "&&" in "test_when_finished" rather than ";" > t7503: avoid touch when mtime doesn't matter > t7503: simplify file-juggling > t7503: don't create "actual_hooks" for later appending > t7503: test failing merge with both hooks available > > ...3-pre-commit-and-pre-merge-commit-hooks.sh | 84 +++++++++++-------- > 1 file changed, 50 insertions(+), 34 deletions(-) > > -- > 2.23.0.rc0.30.g51cf315870 These all look good to me, thank you for the suggestions! I'll include them in V4.
This series adds a new pre-merge-commit hook, similar in usage to pre-commit. It also improves hook testing in t7503, by verifying that the correct hooks are run or bypassed as expected. The original series was done by Michael J Gruber <git@grubix.eu>. I have addressed the outstanding review comments, and noted my changes in the commit messages in "[js: ...]" blocks. Changes since V2: * Renamed the hook from "pre-merge" to "pre-merge-commit". * Added a new patch (1/4) to improve t7503 by verifying that the expected hooks are (or are not) run. * Squashed test changes (from V2's patch 4/4) into patch 3/4. Modified the tests to follow the example set in patch 1/4. * Reworded commit messages to match with the current state of certain flags, which changed in between V1 and V2 of this series. Josh Steadmon (1): t7503: verify proper hook execution Michael J Gruber (3): merge: do no-verify like commit git-merge: honor pre-merge-commit hook merge: --no-verify to bypass pre-merge-commit hook Documentation/git-merge.txt | 2 +- Documentation/githooks.txt | 7 + Documentation/merge-options.txt | 4 + builtin/merge.c | 18 +- ...3-pre-commit-and-pre-merge-commit-hooks.sh | 237 ++++++++++++++++++ t/t7503-pre-commit-hook.sh | 139 ---------- templates/hooks--pre-merge-commit.sample | 13 + 7 files changed, 277 insertions(+), 143 deletions(-) create mode 100755 t/t7503-pre-commit-and-pre-merge-commit-hooks.sh delete mode 100755 t/t7503-pre-commit-hook.sh create mode 100755 templates/hooks--pre-merge-commit.sample Range-diff against v2: 1: e8b3fd8a5b < -: ---------- git-merge: Honor pre-merge hook -: ---------- > 1: f0476b2b1e t7503: verify proper hook execution 2: 36406a85be ! 2: 89ddbf410f merge: do no-verify like commit @@ Commit message "git merge --help" to be more clear that the hook return code is respected by default. - [js: reworded commit message, and moved documentation changes from patch - 3/4 to this commit.] + [js: * reworded commit message + * squashed documentation changes from original series' patch 3/4 + ] Signed-off-by: Michael J Gruber <git@grubix.eu> @@ Documentation/git-merge.txt: SYNOPSIS + [--no-verify] [-s <strategy>] [-X <strategy-option>] [-S[<keyid>]] [--[no-]allow-unrelated-histories] [--[no-]rerere-autoupdate] [-m <msg>] [-F <file>] [<commit>...] - 'git merge' --abort + 'git merge' (--continue | --abort | --quit) ## Documentation/merge-options.txt ## @@ Documentation/merge-options.txt: option can be used to override --squash. 3: 2440ad35e4 < -: ---------- merge: --no-verify to bypass pre-merge hook 4: 69dc3696e7 < -: ---------- t7503: add tests for pre-merge-hook -: ---------- > 3: 61b989ff16 git-merge: honor pre-merge-commit hook -: ---------- > 4: 45828c56fc merge: --no-verify to bypass pre-merge-commit hook