Hi,

On Mon, 23 Jul 2007, Steven Grimm wrote:

quoted text
> Johannes Schindelin wrote:
> > Ah, I missed that.  But IIRC your patch does not wrap that logic behind test
> > ! -z "$templatefile", right?  So this is my modified suggestion:
> > 
> > test ! -z "$templatefile" && {
> > 	grep -vie '^Signed-off-by:' < "$GIT_DIR"/COMMIT_MSG" > "$GIT_DIR"/tmp1
> > 	grep -ve '^#' < "$templatefile" > "$GIT_DIR"/tmp1
> > 	trap 'rm "$GIT_DIR"/tmp[12]' 0
> > 	cmp "$GIT_DIR"/tmp[12] &&
> > 	die "Unchanged message; will not commit"
> > }
> >   
> 
> So you are suggesting I do this in addition to the existing git-commit
> stripping of Signed-off-by: lines? I can certainly do that, but I didn't want
> to make two passes over the commit message doing exactly the same stripping.
> 
> Hmm, maybe I should outline my understanding of the current (unpatched)
> behavior and what I want it to do. Currently:
> 
> * Strip off all comment lines (happens when COMMIT_MSG is created)
> * Strip off all Signed-off-by: lines
> * Trim whitespace
> * If the result has no content (`wc -l` == 0), abort.
> 
> With the patch, my intent was:
> 
> * Strip off all comment lines
> * Strip off all Signed-off-by: lines
> * Trim whitespace
> * If the result has no content (! -s file), abort.
> * If a template file was specified:
>   * Strip off all comment and Signed-off-by: lines from the template
>   * Trim whitespace from the template
>   * If the resulting trimmed template is the same as the trimmed commit
> message, abort.
> 
> So I guess before getting to the specifics of the code, I'll ask: does the
> above make sense as a design? I wanted to preserve the existing behavior in
> the absence of a template.

Yes, I think that makes sense.

quoted text
> Since the existing code is already stripping Signed-off-by: lines to test for
> a zero-length commit message, I figured I should reuse that work. However,
> it's no big deal to do it twice if people feel that results in more readable
> code -- certainly no human will ever notice the time it takes to re-grep the
> commit message.

Okay, but I was really confused by the big if thing.

I'd make sure that COMMIT_BAREMSG is removed with a trap, and really try 
to just enhance the

	if cnt=`grep -v -i '^Signed-off-by' "$GIT_DIR"/COMMIT_MSG |
	        git stripspace |
	        wc -l` &&
	   test 0 -lt $cnt

so that it reads something like

	if test -s "$GIT_DIR"/COMMIT_BAREMSG &&
		test -z "$templatefile" ||
		(grep -ve '^#' < "$templatefile" |
		 git stripspace |
		 git diff --quiet - "$GIT_DIR"/COMMIT_BAREMSG)
	then
		...

(Totally untested, of course.) Hmm?

Ciao,
Dscho

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html