Re: [Monotone-devel] [PATCH] Add a commit message validation hook

[Nathaniel Smith]

On Wed, Jan 25, 2006 at 07:31:34PM -0800, Blake Kaplan wrote:
> I pulled this project right off of the quickies page (and if it
> weren't for my lack of familiarity with the data structures and a
> couple of typos, it would have been really quick).

That's why the quickies are there :-).

> This adds a hook in
> Lua that is called after the user has entered his/her commit message.
> The hook gets the message, a table of the added, deleted, and modified
> files and can veto by returning false and the reason.

This looks great!

Comments:
  -- the passing of added/deleted/modified seems a bit arbitrary; why
     those and not renames, attrs sets, attr deletes?  What if the
     code cares about the difference between added files and dirs?
     1) Is there a particular use case being supported?
     2) Maybe it would make more sense to just pass the text of the
        revision, since lua already has a function to let people parse
        it themselves if they want this info?
  -- Could you add a test or two?  tests/README is a quick-start
     guide.

> I'm not thrilled about the fact that you have to return |true, ""| if
> the message validates, but such is life.

Perhaps we should expose some basic output stuff to lua, like our
warning function.  There are several other places where lua hooks just
call "print", io.write, io.stderr:write, etc., and it would be cleaner
if these use the standard monotone output system.

That way the code could just say
   if (message == "") then
      warn("empty messages are not allowed")
      return false
   end
   return true
or similar.  I guess this might take a bit of tweaking to get the i18n
part quite right, but the current hooks regularly get the i18n part
wrong anyway, so oh well.

> The default hook currently
> disallows empty messages, since that's what monotone currently does,
> but it could probably just as easily return true, "".

That's probably a good default to keep.

If you wanted another thing to work on, there's an old patch to add
support for .mt-template, that never got applied because it was
missing tests (IIRC, check the list archives for details).  I think of
it because it interacts with this one a bit -- I guess you'd want to
add another argument to the hook which was the original text of the
template, so the hook could also require that the template had been
modified?

Cheers,
-- Nathaniel

-- 
"...these, like all words, have single, decontextualized meanings: everyone
knows what each of these words means, everyone knows what constitutes an
instance of each of their referents.  Language is fixed.  Meaning is
certain.  Santa Claus comes down the chimney at midnight on December 24."
  -- The Language War, Robin Lakoff