Re: [PATCH 1/1] Add commit log message spell checking feature.

From: Jeff King <peff@peff.net>
Date: Mar 3, 2010, 14:45 UTC
Message-ID: <20100303144517.GA21385@coredump.intra.peff.net>
In-Reply-To: <alpine.LNX.2.00.1003032018330.21953@vqena.qenxr.bet.am>

On Wed, Mar 03, 2010 at 07:50:34PM +1300, Steven Drake wrote:

Show 10 quoted lines

> On Sun, 28 Feb 2010, Jeff King wrote:
> > On Sun, Feb 28, 2010 at 01:03:00PM +1300, Steven Drake wrote:
> > 
> > > Add 'git commit --spell' to run a spell checker on commit log message.
> > > The `commit.spell` configuration variable can be used to enable the spell
> > > checker by default and can be turned off by '--no-spell'.
> > 
> > Isn't this exactly the sort of thing the commit-msg hook is for?
> 
> Accept then there would be no way of having '--spell'/'--no-spell' (Yet!).

You would have to spell it:

  NO_SPELL=1 git commit

which admittedly isn't as nice. But I'm not sure why you would want --no-spell. I guess for rebases and such where you aren't writing the message directly? But in that case, shouldn't it be passing --no-verify already (and if we go with your patch, should --no-verify perhaps imply --no-spell)?

> Plus as I have just found out all hooks are run with stdin as '/dev/null' so
> there is no way of running an interactive command like 'ispell' from a hook!

Yes, you have to do "ispell </dev/tty". Though both that and your original suffer from somebody running "git gui" or similar in a terminal that the user is no longer looking at. I see you check for isatty(), but I don't know if that is enough for git gui (or any of the other graphical commit helpers).

> > Though personally I would probably just invoke interactive spell-checking
> > from the editor.
> 
> I would probably forget to.

Sure, but that is a problem with --spell, too. And you have already solved it here, with commit.spell configuration. So why does that technique not apply to configuring your editor?

I'm not 100% against your patch. I'm just concerned that it is adding code and complexity for a feature that nobody will use, because everybody else is already doing the same thing through their editor, which is cleaner (e.g., we don't have to worry about handling --no-spell for scripts).

-Peff