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

From: Glen Choo <chooglen@google.com>
Date: Dec 8, 2022, 00:06 UTC
Message-ID: <kl6lfsdqep25.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20221127093721.31012-2-sorganov@gmail.com>

Sergey Organov <sorganov@gmail.com> writes:

Show 12 quoted lines

> @@ -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'.

Show 13 quoted lines

> 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.

Show 25 quoted lines

> @@ -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)".