Re: [PATCH v4 1/1] cat-file: add mailmap subcommand to --batch-command

> Siddharth Asthana <siddharthasthana31@gmail.com> writes:
> 
>> diff --git a/Documentation/git-cat-file.adoc b/Documentation/git-cat-file.adoc
>> index c139f55a16..0f499c9d1b 100644
>> --- a/Documentation/git-cat-file.adoc
>> +++ b/Documentation/git-cat-file.adoc
>> @@ -174,6 +174,18 @@ flush::
>>   	since the beginning or since the last flush was issued. When `--buffer`
>>   	is used, no output will come until a `flush` is issued. When `--buffer`
>>   	is not used, commands are flushed each time without issuing `flush`.
>> +
>> +`mailmap (<bool>)`::
>> +	Enable or disable mailmap for subsequent commands. The `<bool>`
>> +	argument accepts the same boolean values as linkgit:git-config[1].
>> +	Possible effects are:
>> ++
>> +`true`;;
>> +	Mailmap data is loaded on first use and kept in memory until the
>> +	process exits. Passing `true` again does not reload the data.
>> +`false`;;
>> +	Mailmap replacements are disabled for subsequent commands, but data
>> +	already loaded stays in memory.
>>   --
>>   +
> 
> While the above may not be telling any lies, I think the focus of
> the explanation is placed on a wrong thing.  What primarily matters
> to end-users is that toggling 'mailmap' to true makes the mailmap
> applied to identities before they appear in the output and false
> makes the identities output without modification.  The fact that you

> read the mailmap data only once and keep it around even when
> toggling the feature off, just in case the feature gets turned on
> again, is an implementation detail that is of much lessor interest
> to end users, no?
> 
> Perhaps delete everything from "Possible effects are" and replace it
> with a brief explanation, e.g.,
> 
> `mailmap (<bool>)`::
> 	Enable or disable mailmap for subsequent commands. The `<bool>`
> 	argument accepts the same boolean values as linkgit:git-config[1].
> 	The mailmap data is read upon the first use and only once,
> 	even after the `mailmap` command is given multiple times to
> 	toggle it off and then on back again.
> 
> would be sufficient (I would omit "even after ..." part, if I were
> writing it).  What is more important than the optimization aspect of

> this implementation detail to end users is that we do not re-read,
> so if you update the file while you are running "cat-file --batch",
> even giving 'mailmap off; mailmap on' would not cause it to re-read
> the updated data, and "upon the first use and only once" should be
> sufficient to understand that.
> 
> The implementation looks great.  I do not recall how good the tests
> were but this round does not change anything there since the
> previous iteration.