Re: [PATCH v4 1/4] Add log.mailmap as configurational option for mailmap location

From: Marius Storm-Olsen <marius@trolltech.com>
Date: Feb 5, 2009, 19:33 UTC
Message-ID: <498B3F24.6080305@trolltech.com>
In-Reply-To: <7vljskeq21.fsf@gitster.siamese.dyndns.org>

Junio C Hamano said the following on 05.02.2009 18:44:

Show 40 quoted lines

> Marius Storm-Olsen <marius@trolltech.com> writes:
> 
>> diff --git a/config.c b/config.c
>> index 790405a..9ebcbbe 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -565,6 +565,13 @@ static int git_default_branch_config(const char *var, const char *value)
>>  	return 0;
>>  }
>>  
>> +static int git_default_log_config(const char *var, const char *value)
>> +{
>> +	if (!strcmp(var, "log.mailmap"))
>> +		return git_config_string(&git_log_mailmap, var, value);
>> +	return 0;
>> +}
>> +
>>  int git_default_config(const char *var, const char *value, void *dummy)
>>  {
>>  	if (!prefixcmp(var, "core."))
>> @@ -579,6 +586,9 @@ int git_default_config(const char *var, const char *value, void *dummy)
>>  	if (!prefixcmp(var, "branch."))
>>  		return git_default_branch_config(var, value);
>>  
>> +	if (!prefixcmp(var, "log."))
>> +		return git_default_log_config(var, value);
>> +
> 
> The placement of this looked *really* wrong, as mailmap is not
> *that* important to most of the commands.
> 
> Initially I wondered if this should be better done inside existing 
> git_log_config().  I suspect that the reason you didn't do so is
> because you would want to use this also in blame, which is not part
> of the log family, and does not use git_log_config() (nor it
> should).
> 
> Which probably means that the code can stay here (it is just two
> strcmp and assignment to a pointer variable), but also suggests
> that log.mailmap is perhaps misnamed.

Correct, that was my reasoning behind it. Since shortlog is the only place in the documentation where mailmap is *directly* mentioned, it feels slightly tied to log. But, since blame and pretty.c also reference it, I needed the configuration option to be read as default.

Given that in total shortlog, blame, log, diff-tree, rev-list, show and whatchanged use it (the latter 5 through the pretty option), I'm tempted to say that it justifies its own option (mailmap.file?); but it would still have to be handled by git_default_config(). Renaming it would give it stronger reason to *be there* though.

I'm fine either way, really. Though, I think if we rename the option, it also justifies pulling the mailmap documentation out of git-shortlog.txt, into its own file, and link to it from shortlog, and the other commands which use it (git-blame.txt and pretty-format.txt)

I'll happily do the job, if "yay", or leave it as is if "nay". Either way, feel free to rename log.mailmap to something else.

-- .marius