Re: [PATCH v3] Advertise the ability to abort a commit

From: Petr Baudis <pasky@suse.cz>
Date: Jul 31, 2008, 10:55 UTC
Message-ID: <20080731105539.GM32184@machine.or.cz>
In-Reply-To: <20080731073609.GA8049@sigill.intra.peff.net>

On Thu, Jul 31, 2008 at 03:36:09AM -0400, Jeff King wrote:

Show 33 quoted lines

>  builtin-commit.c  |   19 ++++++++++++-------
>  t/t7502-commit.sh |   11 +++++------
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin-commit.c b/builtin-commit.c
> index f7c053a..b783e6e 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -554,14 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
>  
>  		fprintf(fp,
>  			"\n"
> -			"# Please enter the commit message for your changes.\n"
> -			"# To abort the commit, use an empty commit message.\n"
> -			"# (Comment lines starting with '#' will ");
> +			"# Please enter the commit message for your changes.");
>  		if (cleanup_mode == CLEANUP_ALL)
> -			fprintf(fp, "not be included)\n");
> +			fprintf(fp,
> +				" Lines starting\n"
> +				"# with '#' will be ignored, and an empty"
> +				" message aborts the commit.\n");
>  		else /* CLEANUP_SPACE, that is. */
> -			fprintf(fp, "be kept.\n"
> -				"# You can remove them yourself if you want to)\n");
> +			fprintf(fp,
> +				" Lines starting\n"
> +				"# with '#' will be kept; you may remove them"
> +				" yourself if you want to.\n"
> +				"# An empty message aborts the commit.\n");
>  		if (only_include_assumed)
>  			fprintf(fp, "# %s\n", only_include_assumed);
>

This is rather funny-looking; you print _one_ fragment of the common string by a common fprintf, but then repeat _second_ fragment of the still-common string in a per-case fprintf. Can't we at least split this on the line boundary, if not do something loosely like this?

		fprintf(fp,
			"\n"
			"# Please enter the commit message for your "
			"changes. Lines starting\n"
			"# with a '#' will be %s "
			"and an empty message aborts the commit\n",
			cleanup_mode == CLEANUP_ALL ? "ignored,"
			/* CLEANUP_SPACE */ : "kept (you may remove them "
				"yourself if you want to)\n#");

-- 
				Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name.  -- Ken Thompson and Dennis M. Ritchie