From: Marius Storm-Olsen <marius@trolltech.com>
Date: Thu, 05 Feb 2009 19:33:56 GMT
Subject: Re: [PATCH v4 1/4] Add log.mailmap as configurational option for mailmap location
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:
> 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