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

[Jeff King <peff@peff.net>]

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

> 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