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

From: Ralf Thielow <ralf.thielow@gmail.com>
Date: Jan 9, 2013, 08:16 UTC
Message-ID: <CAN0XMO+t2gu9UKJFVXAxt91-hUUhMqqmMoop88KYp0vo3x6c_g@mail.gmail.com>
In-Reply-To: <20130109072952.GC6503@elie.Belkin>

Hi,

2013/1/9 Jonathan Nieder <jrnieder@gmail.com>:

Show 13 quoted lines

> 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.
>

It's actually my own usecase :). The bugtracker I'm using is able to create relationships between issues and related commits. It expects that a part of the commit message contains the issue number in format "#<issueId>". So I need to use a cleanup mode different from "default" to keep the commentary. The mode I'd use is "whitespace", "verbatim" is also possible.

Show 9 quoted lines

> 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.
>

When a user uses a script/importer which expects that the "default" option is used without setting it explicitly, and then the user changes the default, isn't it the users fault if that would break things?

Show 17 quoted lines

> 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.
>

I'll add a sentence of my bugtracker example to it. I think many developers are using such a tool, so it'd makes sense.

Show 13 quoted lines

> [...]
>> --- 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.
>

Thanks. I'll send a new version of the patch including changes of your and Junios comments.

Ralf