git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] Allow selection of different cleanup modes for commit messages

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 22, 2007, 07:59 UTC
Message-ID
<7v3atv17o9.fsf@gitster.siamese.dyndns.org>
In-Reply-To
<20071221230851.GA3260@steel.home>
Alex Riesen <raa.lkml@gmail.com> writes:
Show 9 quoted lines
> Junio C Hamano, Fri, Dec 21, 2007 22:43:49 +0100:
>> Alex Riesen <raa.lkml@gmail.com> writes:
>> 
>> > The patch is on top of my previos patches. Junio, if you wish, I can
>> > squash and resend.
>> 
>> That would be a sane thing to do.
>
> Done

Thanks. I am afraid I have to ask you to either refute my misunderstanding or a second round.

Show 9 quoted lines
> +--cleanup=<mode>::
> +	This option sets how the commit message is cleaned up.
> +	The  '<mode>' can be one of 'verbatim', 'whitespace', 'strip',
> +	and 'default'. The 'default' mode will strip leading and
> +	trailing empty lines and #commentary from the commit message
> +	only if the message is to be edited. Otherwise only whitespace
> +	removed. The 'verbatim' mode wont change message at all,
> +	'whitespace' removes just leading/trailing whitespace lines
> +	and 'strip' removes both whitespace and commentary.
(minor) s/wont/does not/
Show 21 quoted lines
> @@ -394,20 +410,24 @@ static int prepare_log_message(const char *index_file, const char *prefix)
>  		return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
>  	}
>  
> +	if (!no_edit) {
> +		if (in_merge)
> +			fprintf(fp,
> +				"#\n"
> +				"# It looks like you may be committing a MERGE.\n"
> +				"# If this is not correct, please remove the file\n"
> +				"#	%s\n"
> +				"# and try again.\n"
> +				"#\n",
> +				git_path("MERGE_HEAD"));
> +		if (cleanup_mode != CLEANUP_NONE)
> +			fprintf(fp,
> +				"\n"
> +				"# Please enter the commit message for your changes.\n");
> +		if (cleanup_mode == CLEANUP_DEFAULT)
> +			fprintf(fp,
> +				"# (Comment lines starting with '#' will not be included)\n");

Can't cleanup_mode be CLEANUP_ALL at this point, and if so shouldn't this insn be given as well?

In addition, if:
 - we are talking with editor, and
 - the mode is verbatim, and
 - we added any "#" line ourselves (e.g. in_merge adds the insn
   shown above, or perhaps we added "git status" output),

then perhaps we would want to say that "#" lines need to be stripped by the user; otherwise it will be left in the commit.

Show 12 quoted lines
> @@ -431,10 +451,13 @@ static int message_is_empty(struct strbuf *sb, int start)
>  	const char *nl;
>  	int eol, i;
>  
> +	if (cleanup_mode == CLEANUP_NONE && sb->len)
> +		return 0;
> +
>  	/* See if the template is just a prefix of the message. */
>  	strbuf_init(&tmpl, 0);
>  	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
> -		stripspace(&tmpl, 1);
> +		stripspace(&tmpl, !no_edit);

We at this point know that some stripping would happen. The template needs to be cleaned the same way as the commit message was stripped. Is checking no_edit the right thing to do? What if cleanup_mode was CLEANUP_ALL and there is no editing going on?

Show 8 quoted lines
> @@ -813,7 +846,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	if (p != NULL)
>  		strbuf_setlen(&sb, p - sb.buf + 1);
>  
> -	stripspace(&sb, 1);
> +	if (cleanup_mode != CLEANUP_NONE)
> +		stripspace(&sb, cleanup_mode == CLEANUP_DEFAULT ?
> +			   !no_edit: cleanup_mode == CLEANUP_ALL);
When writing such a long ? : expression, laying it out like:
	condition
        ? if-true
        : if-false

would be easier to read. You tilt your head sideways 90 degrees, just like you read a smiley ;-), and you will see the parse tree.

But I suspect, assuming that the two issues around CLEANUP_DEFAULT vs CLEANUP_ALL I mentioned above are indeed bugs, it may be cleaner to:

  - parse the options; you get one of NONE/DEFAULT/SPACE/ALL
  - convert DEFAULT to SPACE or ALL if editor is in effect.
  - do not worry about no_editor or DEFAULT in the rest of the
    code when calling stripspace().  The only values you will
    see are NONE, SPACE or ALL.
Show 10 quoted lines
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index 21ac785..6219378 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -89,4 +89,44 @@ test_expect_success 'verbose' '
>  
>  '
>  
> +test_expect_success 'verbatim commit messages' '
> +

These tests check too many things at once, and it would be very hard to diagnose when breakage does happen. Please split them perhaps into three separate tests like this:

Show 5 quoted lines
> +	echo >>negative &&
> +	{ echo;echo "# text";echo; } >expect &&
> +	git commit --cleanup=verbatim -t expect -a &&
> +	git cat-file -p HEAD |sed -e "1,/^\$/d" |head -n 3 >actual &&
> +	diff -u expect actual &&

This is one test --- making sure the commit body match the unedited template verbatim, except the status part to be shown in the editor. But we are not using editor in this test and I thought the point of this --cleanup=verbatim exercise was that the "status" part is not even have to be added to the commit log message when editor is not in effect... Isn't the "head -n 3" just sweeping a bug under the rug?

> +	echo >>negative &&
> +	git commit --cleanup=verbatim -F expect -a &&
> +	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> +	diff -u expect actual &&
And this is another test.
> +	echo >>negative &&
> +	git commit --cleanup=verbatim -m "$(cat expect)" -a &&
> +	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
> +	diff -u expect actual
And this is yet another.
> +'

Also, the tests do not check "-F file -e", "-m msg -e" etc. We would want to make sure the right insn are shown to the user in the editor, wouldn't we?

Previous: Alex RiesenNext: Alex Riesen
Message 18 of 23 in “git-commit: add --verbatim to allow unstripped commit messages”
  1. git-commit: add --verbatim to allow unstripped commit messagesAlex Riesen, Dec 20, 2007
  2. Linus TorvaldsDec 20, 2007
  3. Alex RiesenDec 20, 2007
  4. git-commit: add --verbatim to allow unstripped commit messagesAlex Riesen, Dec 20, 2007
  5. Only filter "#" comments from commits if the editor is usedAlex Riesen, Dec 20, 2007
  6. Junio C HamanoDec 20, 2007
  7. Alex RiesenDec 20, 2007
  8. Junio C HamanoDec 20, 2007
  9. Fix thinko in checking for commit message emptinessAlex Riesen, Dec 20, 2007
  10. Linus TorvaldsDec 20, 2007
  11. Björn SteinbrinkDec 21, 2007
  12. Linus TorvaldsDec 21, 2007
  13. Junio C HamanoDec 20, 2007
  14. Linus TorvaldsDec 21, 2007
  15. Allow selection of different cleanup modes for commit messagesAlex Riesen, Dec 21, 2007
  16. Junio C HamanoDec 21, 2007
  17. Allow selection of different cleanup modes for commit messagesAlex Riesen, Dec 21, 2007
  18. Junio C HamanoDec 22, 2007
  19. Allow selection of different cleanup modes for commit messagesAlex Riesen, Dec 22, 2007
  20. Linus TorvaldsDec 22, 2007
  21. Junio C HamanoDec 22, 2007
  22. Junio C HamanoDec 23, 2007
  23. Junio C HamanoDec 23, 2007

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.