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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Dec 22, 2007, 19:18 UTC
Message-ID: <alpine.LFD.0.9999.0712221107240.21557@woody.linux-foundation.org>
In-Reply-To: <20071222184624.GA4167@steel.home>

On Sat, 22 Dec 2007, Alex Riesen wrote:

>
> +	if (!no_edit) {

This is unrelated to the rest of the patch, but I do think double negations are horrible, so I thoink we should probably make the "no_edit" flag change meaning, and call it "run_editor" or something.

That said, the thing that I'm actually reacting to is:

Show 18 quoted lines

> +		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_ALL)
> +			fprintf(fp,
> +				"# (Comment lines starting with '#' will not be included)\n");
> +		if (only_include_assumed)
> +			fprintf(fp, "# %s\n", only_include_assumed);

The thing that still worries me about this set is that if you have "cleanup_mode == CLEANUP_NONE" or "cleanup_mode == CLEANUP_SPACE", we're now adding all these messages, and we depend on the user knowing that *he* has to remove them.

So I wonder if we should perhaps:

 (a) Only add these messages at all when we do *not* do CLEANUP_ALL

     In other words, change the

		if (!no_edit) {

     line to a

		if (cleanup_mode == CLEANUP_ALL && !no_edit) {

     instead, or

 (b) Add a a new line to replace he "will not be included" message, ie

		if (cleanup_mode != CLEANUP_ALL)
			fprintf(p,
				"# Please remove these comment lines manually,  "
				"they will not be automatically removed\n");

     or something similar.

I personally would prefer (a) - since anybody who then explicitly uses --cleanup={space|none} would presumably already know what he is doing.

But this is not a huge deal. Regardless, the patch looks ok, so you can add a "Acked-by:" from me.

		Linus