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

[Laurent Goubet <laurent.goubet@xxxxxxx>]

On 04/11/2014 10:41, Christian Halstrick wrote:

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.

Will do asap.

 

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.

Now that you mention it... I do change the behavior slightly. I've decided to check for the existence of a hook only when running it (hook present on not, I try to run it, so I prepare its arguments beforehand). The argument for the "commit-msg" hook is the path to a file containing the proposed commit message (.git/COMMIT_EDITMSG), so that the hook can change it. To run this hook, I thus serialize the message, run the hook if it is present, then re-read the commit message file. I initially did that to minimize I/O operations involving the hooks... but I didn't realize I did generate more I/O that way for that particular hook (especially in the case where it is not present...). I'll change my implementation.

Thanks for the input!

Laurent

begin:vcard
fn:Laurent Goubet
n:Goubet;Laurent
org:<a href="http://www.obeo.fr";>Obeo</a>
email;internet:laurent.goubet@xxxxxxx
url:http://www.obeo.fr
version:2.1
end:vcard