Re: [PATCH 1/5] diff-merges: implement [no-]hide option and log.diffMergesHide config

[Sergey Organov <sorganov@gmail.com>]

Glen Choo <chooglen@google.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> @@ -49,10 +49,11 @@ ifdef::git-log[]
>>  --diff-merges=m:::
>>  -m:::
>>  	This option makes diff output for merge commits to be shown in
>> -	the default format. `-m` will produce the output only if `-p`
>> -	is given as well. The default format could be changed using
>> +	the default format. The default format could be changed using
>>  	`log.diffMerges` configuration parameter, which default value
>>  	is `separate`.
>> ++
>> +	`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'
>>  +
>
> I found this description difficult to parse, since
>
> a) it wasn't clear that multiple "--diff-merges" would all be respected
> b) I had to read the --diff-merges=hide documentation to understand what
>    this means
>
> Keeping the plain english description would help, something like:
>
>   `-m` only produces the output if `-p` is also given, i.e. it is a
>   shortcut for '--diff-merges=on --diff-merges=hide'.

Thanks, I'll review the documentation if we decide all this stuff is
useful.

>
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 56e2d95e869d..e031021e53b2 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -581,6 +581,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
>>  	}
>>  	if (!strcmp(var, "log.diffmerges"))
>>  		return diff_merges_config(value);
>> +	if (!strcmp(var, "log.diffmergeshide"))
>> +		return diff_merges_hide_config(git_config_bool(var, value));
>>  	if (!strcmp(var, "log.showroot")) {
>>  		default_show_root = git_config_bool(var, value);
>>  		return 0;
>
> Since we have log.diffmergeshide that is different from log.diffmerges,
> it seems like it would be more consistent to have '--diff-merges-hide'
> as a separate flag.

I'd rather drop log.diffmergeshide, as log.diffMerges=hide does the same
thing. I'm just not sure if multiple instances of the same
log.diffMerges config are simple enough to manage through "git config"
interface.

This is independent of --diff-merges-hide suggestion, that, if
implemented, I'd prefer to read as --diff-merges-flags=[no-]hide, to
provide space for future flags, even though I still prefer a syntax
inside existing "--diff-merges" namespace. Something like:

  --diff-merges=<format>[,<flag>[,...]]

<format>: on|off|c|cc|remerge|...
<flag>:   [no-]hide|...

>
>> @@ -69,6 +87,10 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
>>  {
>>  	if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))
>>  		return set_none;
>> +	if (!strcmp(optarg, "hide"))
>> +		return set_hide;
>> +	if (!strcmp(optarg, "no-hide"))
>> +		return set_no_hide;
>>  	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
>>  		return set_first_parent;
>>  	if (!strcmp(optarg, "separate"))
>> @@ -105,7 +127,19 @@ int diff_merges_config(const char *value)
>>  	if (!func)
>>  		return -1;
>>  
>> -	set_to_default = func;
>> +	if (func == set_hide)
>> +		hide = 1;
>> +	else if (func == set_no_hide)
>> +		hide = 0;
>> +	else
>> +		set_to_default = func;
>> +
>> +	return 0;
>> +}
>
> The code is also simpler if we took a separate CLI flag, e.g. we could
> get rid of this special casing of "(func == X)".

I foresee complications elsewhere. Overall complexity won't be that
different either way, I think.

Thanks,
-- 
Sergey Organov