Re: [PATCH] commit: make default of "cleanup" option configurable

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Ralf Thielow wrote:

> The default of the "cleanup" option in "git commit"
> is not configurable. Users who don't want to use the
> default have to pass this option on every commit since
> there's no way to configure it.

Could you give an example?  I'm trying to get a sense of whether these
habitual --cleanup= passers would use --cleanup=verbatim or
--cleanup=strip.

I'm a bit worried that a setting like 'commit.cleanup = strip' would
be likely to break scripts.  Yes, scripts using "git commit" instead
of commit-tree while caring about detailed semantics are asking for
trouble, but I'm worried about importers, for example, which are
sometimes written by new git users.  Some scripts might assume that
"git commit" preserves the entire change description from their source
data, even when some lines happen to start by listing the ways the
author is #1.

I don't think that necessarily rules out this change, but:

 1. We need more of an explanation of the purpose than "this lets
    people type less".  What workflow does setting this option fit
    into?

 2. I would prefer to see a little thought about whether it's possible
    to avoid silently impacting scripts.  E.g., depending on the
    answer to (1), maybe supporting "verbatim" but not "strip" would be
    ok?  Or alternatively, maybe a search of public code repositories
    would reveal that new git users tend to write their importers in a
    way that this doesn't break.

As a side effect, the information gathered to answer (1) and (2) could
have the potential to make the user-level documentation more useful,
by adding context to the description of the configuration item.

[...]
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -103,7 +103,7 @@ static enum {
>  	CLEANUP_NONE,
>  	CLEANUP_ALL
>  } cleanup_mode;
> -static char *cleanup_arg;
> +const static char *cleanup_arg;

Style nit: storage class comes first, then the type.  Otherwise the
typename "const char *" is split up.

Hope that helps,
Jonathan