Re: [PATCH 1/1] Add support for an optional .nextmsg file to git-commit-script

[Junio C Hamano <junkio@cox.net>]

>>>>> "JS" == Jon Seymour <jon.seymour@gmail.com> writes:

JS> This change allows a commit message to be prepared prior to git-commit-script being
JS> invoked. During git commit the user is then given an opportunity to edit the
JS> existing message, if any, and must confirm the commit by deleting a sentinel
JS> line from the top of the edit buffer.

JS>  #!/bin/sh
JS> +SENTINEL="*** delete this line to confirm the commit ***"
JS> ..
JS> +egrep -v "^#|$SENTINEL" < .editmsg | git-stripspace > .cmitmsg

A handful comments.

(1) Although it happens to work in this case, I do not like
    scripts that use grep without quoting metacharacters in a
    string.  You have two egreps that phrases the above slightly
    differently.  Make up your mind ;-) and pick one.

(2) What happens if I later want to make a commit with this commit
    log?

    [PATCH] Reword the sentinel line.

    Instead of saying "*** delete this line to confirm the commit ***",
    just say "# If you want to abort the commit, make this file empty.".

(3) The original implementation knows that the commit is to be
    aborted when resulting .cmitmsg is empty.  You may want to
    tell the user in the comment in the commit template about
    it, instead of using the sentinel line.

(4) Instead of teaching git-commit-script about the .nextmsg
    file, it may be cleaner to split it into two parts:

    - one that prepares the .cmitmsg file for editing;

    - the other that invokes the editor on prepared a .cmitmsg
      file, checks if the message file is edited and asks for
      confirmation to abort if the file is unchanged, and does the
      actual commit.

   The new git-commit-script would prepare its .cmitmsg the way it
   does now and only call the latter half.  Your re-commit script
   can do the first half differently from what git-commit-script
   does, and call the same latter half.

P.S. I see you are using "git format-patch"; I'd suggest you
hand edit " 1/1" out of [PATCH 1/1] subject line for a patch
like this that is not part of a series.  I did not special case
1/1 to do it myself because I often have a handful independent
patches and end up removing 1/7 2/7 ... etc. anyway.