From: Junio C Hamano Date: Tue, 04 Feb 2020 20:50:39 GMT Subject: Re: [PATCH v2 4/4] git-p4: add p4 submit hooks Message-ID: In-Reply-To: <3d34dd042e47fce7bc49a63f676378172e15775e.1580507895.git.gitgitgadget@gmail.com> "Ben Keene via GitGitGadget" writes: > From: Ben Keene > > The git command "commit" supports a number of hooks that support > changing the behavior of the commit command. The git-p4.py program only > has one existing hook, "p4-p4-pre-submit". This command occurs early > in the process. There are no hooks in the process flow for modifying > the P4 changelist text programmatically. OK. It is unclear to me how the line between 3/4 and 4/4 was decided. It makes sense that 2/4 was done as adding the mechanism to run hooks. A natural progression after your 1/4 and 2/4 would have been to add the "--[no-]verify" mechanism to disable any verification hook in 3/n, possibly rewrite the implementation of existing hooks using the mechanism you added with 2/4 and 3/n in 4/n, and then either (1) add and document all new hooks in 5/n, or (2) add and document one new hook per one step in 5/n, 6/n, 7/n and 8/n (you are adding one in your 3/4 and then three in your 4/4 here). As a reviewer I do not have strong preference either way (i.e. 4 final steps to add 4 hooks, or 1 final step to add 4 hooks), but 3/4 and 4/4 you have here look harder to reason than necessary, at least to me. Thanks.