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@xxxxxxxxxx> 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.
