Re: [jgit-dev] git hooks for [e|j]git

[Christian Halstrick <christian.halstrick@xxxxxxxxx>]

Hi

On Mon, Nov 3, 2014 at 11:16 AM, Laurent Goubet <laurent.goubet@xxxxxxx> wrote:

I have a few question though... First, the four reviews for the hooks themselves are dependent on one-another, in a line... would you rather I make these commits independent, having one "base" review (the API) then the four commits depending on it each introducing its own hook? (This will make the independant "hook" commits conflicting with each other though, so I'll have to rebase each one manually after one is accepted and submitted in the code base).

I would prefer if we have independent commits. It's only easier with a chain if you expect that the whole chain of dependent commits will be accepted in one shot. But it starts to be a nightmare if everybody agreed on the last change in your queue and wants to get it in but one change in the middle is updated often and requires all following changes to be rebased often (e.g. with every new patchset). But of course if you have good reasons for a dependency (e.g. pre-commit hook adds something post-commit hook wants to reuse then go ahead with dependencies. I would try to bring things needed by multiple hooks into the very first commit which brings the infrastructure for commits. 

 

Second... the "commit-msg" hook support makes three unit test case because I trim the commit message when re-reading it after the hook has done its job... and the failing unit cases use a commit message with a trailing LF. Should I remove the trailing white space from the unit test, or would you rather we accept these white spaces? (cgit trims the commit message.)

I guess that the old tests which are failing now don't use any hooks. Means your hook support changes semantic in repos which don't use hooks. That sounds strange. May we had a bug before and you fixed it now (means: let jgit behave like cgit). Ideal would be a fix of that in a separate change. But if your fix requires the coding you did for hook support than I personally would say: just fix the test cases also and mention it commit message what you say.