Glen Choo <chooglen@xxxxxxxxxx> writes:
> Sergey Organov <sorganov@xxxxxxxxx> 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
