Re: [PATCH] git-commit: add a prepare-commit-msg hook

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 21, 2008, 12:46 UTC
Message-ID: <alpine.LSU.1.00.0801211231480.5731@racer.site>
In-Reply-To: <47948C9B.7030100@gnu.org>

Hi,

On Mon, 21 Jan 2008, Paolo Bonzini wrote:

> > I have.  But I want to avoid a regression at any cost.  And your patch 
> > just looks to me like it could do that.
> 
> What kind of regression?

See below.

Show 7 quoted lines

> > But it _has_ been already suggested that you could provide arguments 
> > for the existing msg-hook, which would not break anything
> 
> Sure it won't break anything, but it won't work either!  The existing 
> message hook runs after the editing session -- I want the hook to 
> introduce text that is merely a suggestion that the user can delete, or 
> a template that the user needs to customize further.

OMG you're right. But why didn't you say so in the commit message? Something like "This hook complements the commit-msg hook, in that it runs _before_ the editor is launched".

> > since the hook does not get any argument yet, and therefore existing 
> > hooks would be unaffected.
> 
> How does adding a new hook affect existing hooks?

My impression -- even after reading your commit message -- was that it does almost the same as the commit-msg hook, only that it runs _in addition_ to it when doing a non-amend commit.

FWIW this is the first reply by you that uncovers this error of mine.

Show 6 quoted lines

> > Also, the change would be non-intrusive, easy-to-review
> 
> Please.  That's ludicrous.
> 
> My patch is 3 lines of inserted code and 0 modified lines, checking one 
> variable that is set once in builtin-commit.c (edit_message).

Actually, after reading the commit message I was in "this-is-not-necessary" mode, and therefore the diffstat looked too large for me.

That is why I thought that a regression was looming somewhere.

And in reality, your patch should be 2 lines of code.

> The documentation says that it runs whenever the editor runs except for 
> -c, and launch_editor runs after the 3 lines of code I inserted.

Actually, reading your patch again I think it also triggers for "-c", as well as for "[-C|-F|-m] ... -e".

And it does not necessarily start with "an empty log message"; think of "--signoff".

Besides, I think that the name should be more to the point, something like "pre-edit-commit-msg".

Also, I should think that the hook should get some information about the circumstances (possibly as command line arguments), given that it is called in more cases than you said.

So I think the patch is not ready yet, although I finally got the _point_ of having yet-another hook.

> Seeing how biased you are, I don't really know why I bothered answering 
> you.

So I am biased. And you have to convince me. It's not that hard to convince me.

> > take so much time away from the bug-fixing that we want to do right 
> > now in
> 
> That's the first sensible argument that I hear.

Heh.  Then I'm happy that I put it into my mail, too!

Ciao, Dscho