[PATCH 0/5] diff-merges: more features

[PATCH 0/5] diff-merges: more features

[Sergey Organov]

1. --diff-merges=[no]-hide

The set of diff-merges options happened to be incomplete, and failed
to implement exact semantics of -m option that hides output of diffs
for merge commits unless -p option is active as well.

The new "hide" option fixes this issue, so that now

  --diff-merges=on --diff-merges=hide

combo is the exact synonym for -m.

The log.diffMerges configuration also accepts "hide" and "no-hide"
values, and governs the default value for the hide bit. The
configuration behaves as if "--diff-merges=[no-]hide" is inserted
first in the command-line.

2. log.diffMerges-m-imply-p

Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'
options do. Simply fixing this inconsistency by unconditional
modification of '-m' semantics appeared to be a bad idea, as it broke
some legacy scripts/aliases. This patch rather provides configuration
variable to tweak '-m' behavior accordingly.

3. log.diffMergesForce

Force specific log format for -c, --cc, and --remerge-diff options
instead of their respective formats. The override is useful when some
external tool hard-codes diff for merges format option.

4. Support list of values for --diff-merges

This allows for shorter --diff-merges=on,hide forms.

5. Issue warning for lone '-m'.

Lone '-m' is in use by scripts/aliases that aim at enabling diff
output for merge commits, but only if '-p' is then specified as well.

As '-m' may now be configured to imply '-p' (using
'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,
and suggest to instead use '--diff-merges=on,hide' that does not
depend on user configuration.

This is expected to give a provision for enabling
log.diffMerges-m-imply-p by default in the future.

Sergey Organov (5):
  diff-merges: implement [no-]hide option and log.diffMergesHide config
  diff-merges: implement log.diffMerges-m-imply-p config
  diff-merges: implement log.diffMergesForce config
  diff-merges: support list of values for --diff-merges
  diff-merges: issue warning on lone '-m' option

 Documentation/config/log.txt                  |  20 ++++
 Documentation/diff-options.txt                |  20 +++-
 builtin/log.c                                 |   6 +
 diff-merges.c                                 | 108 +++++++++++++++---
 diff-merges.h                                 |   6 +
 t/t4013-diff-various.sh                       |  89 ++++++++++++++-
 ...ges=first-parent_--diff-merges=hide_master |  34 ++++++
 t/t9902-completion.sh                         |   9 ++
 8 files changed, 272 insertions(+), 20 deletions(-)
 create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master

-- 
2.37.3.526.g5f84746cb16b

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

[Sergey Organov]

The set of diff-merges options happened to be incomplete, and failed
to implement exact semantics of -m option that hides output of diffs
for merge commits unless -p option is active as well.

The new "hide" option fixes this issue, so that now

  --diff-merges=on --diff-merges=hide

combo is the exact synonym for -m.

The log.diffMerges configuration also accepts "hide" and "no-hide"
values, and governs the default value for the hide bit. The
configuration behaves as if "--diff-merges=[no-]hide" is inserted
first in the command-line.

New log.diffMergesHide boolean configuration is implemented as well,
for the case when log.diffMerges is already in use for specifying the
format.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/config/log.txt                  |  6 +++
 Documentation/diff-options.txt                | 13 +++--
 builtin/log.c                                 |  2 +
 diff-merges.c                                 | 40 ++++++++++++--
 diff-merges.h                                 |  2 +
 t/t4013-diff-various.sh                       | 54 ++++++++++++++++++-
 ...ges=first-parent_--diff-merges=hide_master | 34 ++++++++++++
 t/t9902-completion.sh                         |  3 ++
 8 files changed, 147 insertions(+), 7 deletions(-)
 create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index cbe34d759221..9c6be643bcf6 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -33,6 +33,12 @@ log.diffMerges::
 	Set diff format to be used when `--diff-merges=on` is
 	specified, see `--diff-merges` in linkgit:git-log[1] for
 	details. Defaults to `separate`.
++
+When used with 'hide' or 'no-hide', sets the default hiding of
+diffs for merge commits when `-p` option is not used.
+
+log.diffMergesHide::
+	`true` implies `--diff-merges=hide` option.
 
 log.follow::
 	If `true`, `git log` will act as if the `--follow` option was used when
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 3674ac48e92c..351d51215717 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -34,7 +34,7 @@ endif::git-diff[]
 endif::git-format-patch[]
 
 ifdef::git-log[]
---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::
+--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r|[no-]hide)::
 --no-diff-merges::
 	Specify diff format to be used for merge commits. Default is
 	{diff-merges-default} unless `--first-parent` is in use, in which case
@@ -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'
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
@@ -72,6 +73,12 @@ ifdef::git-log[]
 	with conflict markers and such.  A diff is then shown between
 	that temporary tree and the actual merge commit.
 +
+--diff-merges=hide:::
+--diff-merges=no-hide:::
+	Hide (do not hide) the diff for merge commits unless `-p` options is given
+	as well. The default `no-hide` could be changed using `log.diffMerges`
+	configuration parameter.
++
 The output emitted when this option is used is subject to change, and
 so is its interaction with other options (unless explicitly
 documented).
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;
diff --git a/diff-merges.c b/diff-merges.c
index 85cbefa5afd7..55fe5b70c102 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -7,6 +7,7 @@ static void set_separate(struct rev_info *revs);
 
 static diff_merges_setup_func_t set_to_default = set_separate;
 static int suppress_m_parsing;
+static int hide = 0;
 
 static void suppress(struct rev_info *revs)
 {
@@ -20,10 +21,15 @@ static void suppress(struct rev_info *revs)
 	revs->remerge_diff = 0;
 }
 
+static void set_need_diff(struct rev_info *revs)
+{
+	revs->merges_need_diff = !hide;
+}
+
 static void common_setup(struct rev_info *revs)
 {
 	suppress(revs);
-	revs->merges_need_diff = 1;
+	set_need_diff(revs);
 }
 
 static void set_none(struct rev_info *revs)
@@ -31,6 +37,18 @@ static void set_none(struct rev_info *revs)
 	suppress(revs);
 }
 
+static void set_hide(struct rev_info *revs)
+{
+	hide = 1;
+	set_need_diff(revs);
+}
+
+static void set_no_hide(struct rev_info *revs)
+{
+	hide = 0;
+	set_need_diff(revs);
+}
+
 static void set_separate(struct rev_info *revs)
 {
 	common_setup(revs);
@@ -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;
+}
+
+int diff_merges_hide_config(int on)
+{
+	hide = on;
 	return 0;
 }
 
@@ -122,7 +156,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
 		set_to_default(revs);
-		revs->merges_need_diff = 0;
+		set_hide(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/diff-merges.h b/diff-merges.h
index 19639689bb05..e86e5381693b 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -11,6 +11,8 @@ struct rev_info;
 
 int diff_merges_config(const char *value);
 
+int diff_merges_hide_config(int hide);
+
 void diff_merges_suppress_m_parsing(void);
 
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index dfcf3a0aaae3..4fc8ba2fc59c 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -334,6 +334,7 @@ log --first-parent --diff-merges=off -p master
 log -p --first-parent master
 log -p --diff-merges=first-parent master
 log --diff-merges=first-parent master
+log --diff-merges=first-parent --diff-merges=hide master
 log -m -p --first-parent master
 log -m -p master
 log --cc -m -p master
@@ -460,7 +461,18 @@ EOF
 test_expect_success 'log -m matches pure log' '
 	git log master >result &&
 	process_diffs result >expected &&
-	git log -m >result &&
+	git log -m master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'log --diff-merges=on matches -m only with --diff-merges=hide' '
+	git log -m master >result &&
+	process_diffs result >expected &&
+	git log --diff-merges=on master >result &&
+	process_diffs result >actual &&
+	! test_cmp expected actual &&
+	git log --diff-merges=on --diff-merges=hide master >result &&
 	process_diffs result >actual &&
 	test_cmp expected actual
 '
@@ -496,6 +508,46 @@ test_expect_success 'git config log.diffMerges first-parent vs -m' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git config log.diffMerges hide: has effect' '
+	git log --diff-merges=on master >result &&
+	process_diffs result >no-hide &&
+	test_config log.diffMerges hide &&
+	git log --diff-merges=on master >result &&
+	process_diffs result >hide &&
+	! test_cmp no-hide hide
+'
+
+test_expect_success 'git config log.diffMerges no-hide: is the default' '
+	git log --diff-merges=on master >result &&
+	process_diffs result >default &&
+	test_config log.diffMerges no-hide &&
+	git log --diff-merges=on master >result &&
+	process_diffs result >no-hide &&
+	test_cmp default no-hide
+'
+
+# As "-m" is synonym for "--diff-merges=hide --diff-merges=on", the
+# "log.diffMerges=hide" configuration should have no effect on "-m"
+test_expect_success 'git config log.diffMerges hide: has no effect on -m' '
+	git log -m master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges hide &&
+	git log -m master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
+# As "--cc" implies "-p", the "log.diffMerges=hide" configuration
+# should have no effect on "--cc"
+test_expect_success 'git config log.diffMerges hide: has no effect on --cc' '
+	git log --cc master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges hide &&
+	git log --cc master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 # -m in "git diff-index" means "match missing", that differs
 # from its meaning in "git diff". Let's check it in diff-index.
 # The line in the output for removed file should disappear when
diff --git a/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master b/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master
new file mode 100644
index 000000000000..596caec64298
--- /dev/null
+++ b/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master
@@ -0,0 +1,34 @@
+$ git log --diff-merges=first-parent --diff-merges=hide master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 43de868b8005..fc6b0722216a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2497,6 +2497,7 @@ test_expect_success 'git config - variable name' '
 	log.date Z
 	log.decorate Z
 	log.diffMerges Z
+	log.diffMergesHide Z
 	EOF
 '
 
@@ -2525,6 +2526,7 @@ test_expect_success 'git -c - variable name' '
 	log.date=Z
 	log.decorate=Z
 	log.diffMerges=Z
+	log.diffMergesHide=Z
 	EOF
 '
 
@@ -2547,6 +2549,7 @@ test_expect_success 'git clone --config= - variable name' '
 	log.date=Z
 	log.decorate=Z
 	log.diffMerges=Z
+	log.diffMergesHide=Z
 	EOF
 '
 
-- 
2.37.3.526.g5f84746cb16b

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

[Glen Choo]

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

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

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

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

[Sergey Organov]

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

[PATCH 2/5] diff-merges: implement log.diffMerges-m-imply-p config

[Sergey Organov]

Historically, `-m` doesn't imply `-p` whereas similar `-c` and `--cc`
options do. Simply fixing this inconsistency by unconditional
modification of `-m` semantics appeared to be a bad idea, as it broke
some legacy scripts.

Implement "log.diffMerges-m-imply-p" boolean configuration variable
that allows user to enable implication of `-p` by `-m`.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/config/log.txt   | 3 +++
 Documentation/diff-options.txt | 6 ++++--
 builtin/log.c                  | 2 ++
 diff-merges.c                  | 8 ++++++++
 diff-merges.h                  | 2 ++
 t/t4013-diff-various.sh        | 9 +++++++++
 t/t9902-completion.sh          | 3 +++
 7 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 9c6be643bcf6..265a57312e58 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -40,6 +40,9 @@ diffs for merge commits when `-p` option is not used.
 log.diffMergesHide::
 	`true` implies `--diff-merges=hide` option.
 
+log.diffMerges-m-imply-p::
+	`true` enables implication of `-p` by `-m`.
+
 log.follow::
 	If `true`, `git log` will act as if the `--follow` option was used when
 	a single <path> is given.  This has the same limitations as `--follow`,
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 351d51215717..e64066af8d96 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -48,12 +48,14 @@ ifdef::git-log[]
 --diff-merges=on:::
 --diff-merges=m:::
 -m:::
-	This option makes diff output for merge commits to be shown in
+	These options make diff output for merge commits to be shown in
 	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'
+	`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'.
+	In addition it implies `-p` when `log.diffMerges-m-imply-p` is
+	active.
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
diff --git a/builtin/log.c b/builtin/log.c
index e031021e53b2..332b5e478cc5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -583,6 +583,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return diff_merges_config(value);
 	if (!strcmp(var, "log.diffmergeshide"))
 		return diff_merges_hide_config(git_config_bool(var, value));
+	if (!strcmp(var, "log.diffmerges-m-imply-p"))
+		return diff_merges_m_imply_p_config(git_config_bool(var, value));
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);
 		return 0;
diff --git a/diff-merges.c b/diff-merges.c
index 55fe5b70c102..1fbf476d378e 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -8,6 +8,7 @@ static void set_separate(struct rev_info *revs);
 static diff_merges_setup_func_t set_to_default = set_separate;
 static int suppress_m_parsing;
 static int hide = 0;
+static int m_imply_p = 0;
 
 static void suppress(struct rev_info *revs)
 {
@@ -143,6 +144,12 @@ int diff_merges_hide_config(int on)
 	return 0;
 }
 
+int diff_merges_m_imply_p_config(int on)
+{
+	m_imply_p = on;
+	return 0;
+}
+
 void diff_merges_suppress_m_parsing(void)
 {
 	suppress_m_parsing = 1;
@@ -157,6 +164,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
 		set_to_default(revs);
 		set_hide(revs);
+		revs->merges_imply_patch = m_imply_p;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/diff-merges.h b/diff-merges.h
index e86e5381693b..9f0b3901fe82 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -13,6 +13,8 @@ int diff_merges_config(const char *value);
 
 int diff_merges_hide_config(int hide);
 
+int diff_merges_m_imply_p_config(int on);
+
 void diff_merges_suppress_m_parsing(void);
 
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 4fc8ba2fc59c..1789dd6063c5 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -548,6 +548,15 @@ test_expect_success 'git config log.diffMerges hide: has no effect on --cc' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '
+	git log -m -p master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges-m-imply-p true &&
+	git log -m master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 # -m in "git diff-index" means "match missing", that differs
 # from its meaning in "git diff". Let's check it in diff-index.
 # The line in the output for removed file should disappear when
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index fc6b0722216a..26a7e4ff877c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2498,6 +2498,7 @@ test_expect_success 'git config - variable name' '
 	log.decorate Z
 	log.diffMerges Z
 	log.diffMergesHide Z
+	log.diffMerges-m-imply-p Z
 	EOF
 '
 
@@ -2527,6 +2528,7 @@ test_expect_success 'git -c - variable name' '
 	log.decorate=Z
 	log.diffMerges=Z
 	log.diffMergesHide=Z
+	log.diffMerges-m-imply-p=Z
 	EOF
 '
 
@@ -2550,6 +2552,7 @@ test_expect_success 'git clone --config= - variable name' '
 	log.decorate=Z
 	log.diffMerges=Z
 	log.diffMergesHide=Z
+	log.diffMerges-m-imply-p=Z
 	EOF
 '
 
-- 
2.37.3.526.g5f84746cb16b

[PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Sergey Organov]

Force specified log format for -c, --cc, and --remerge-diff options
instead of their respective formats. The override is useful when some
external tool hard-codes diff for merges format option.

Using any of the above options twice or more will get back the
original meaning of the option no matter what configuration says.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/config/log.txt | 11 +++++++++++
 builtin/log.c                |  2 ++
 diff-merges.c                | 32 ++++++++++++++++++++++++++------
 diff-merges.h                |  2 ++
 t/t4013-diff-various.sh      | 18 ++++++++++++++++++
 t/t9902-completion.sh        |  3 +++
 6 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 265a57312e58..7452c7fad638 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -43,6 +43,17 @@ log.diffMergesHide::
 log.diffMerges-m-imply-p::
 	`true` enables implication of `-p` by `-m`.
 
+log.diffMergesForce::
+	Use specified log format for -c, --cc, and --remerge-diff
+	options instead of their respective formats when the option
+	appears on the command line one time. See `--diff-merges` in
+	linkgit:git-log[1] for allowed values. Using 'off' or 'none'
+	disables the override (default).
++
+The override is useful when external tool hard-codes one of the above
+options. Using any of these options two (or more) times will get back
+the original meaning of the options.
+
 log.follow::
 	If `true`, `git log` will act as if the `--follow` option was used when
 	a single <path> is given.  This has the same limitations as `--follow`,
diff --git a/builtin/log.c b/builtin/log.c
index 332b5e478cc5..1e8d0a2545a9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -585,6 +585,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return diff_merges_hide_config(git_config_bool(var, value));
 	if (!strcmp(var, "log.diffmerges-m-imply-p"))
 		return diff_merges_m_imply_p_config(git_config_bool(var, value));
+	if (!strcmp(var, "log.diffmergesforce"))
+		return diff_merges_force_config(value);
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);
 		return 0;
diff --git a/diff-merges.c b/diff-merges.c
index 1fbf476d378e..cedd7652bf42 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -6,6 +6,7 @@ typedef void (*diff_merges_setup_func_t)(struct rev_info *);
 static void set_separate(struct rev_info *revs);
 
 static diff_merges_setup_func_t set_to_default = set_separate;
+static diff_merges_setup_func_t force_func = NULL;
 static int suppress_m_parsing;
 static int hide = 0;
 static int m_imply_p = 0;
@@ -150,6 +151,21 @@ int diff_merges_m_imply_p_config(int on)
 	return 0;
 }
 
+int diff_merges_force_config(const char *value)
+{
+	diff_merges_setup_func_t func = func_by_opt(value);
+
+	if (!func)
+		return -1;
+
+	if (func == set_none)
+		force_func = NULL;
+	else if (func != set_hide && func != set_no_hide)
+		force_func = func;
+
+	return 0;
+}
+
 void diff_merges_suppress_m_parsing(void)
 {
 	suppress_m_parsing = 1;
@@ -160,20 +176,18 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 	int argcount = 1;
 	const char *optarg;
 	const char *arg = argv[0];
+	diff_merges_setup_func_t set_func = NULL;
 
 	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
 		set_to_default(revs);
 		set_hide(revs);
 		revs->merges_imply_patch = m_imply_p;
 	} else if (!strcmp(arg, "-c")) {
-		set_combined(revs);
-		revs->merges_imply_patch = 1;
+		set_func = set_combined;
 	} else if (!strcmp(arg, "--cc")) {
-		set_dense_combined(revs);
-		revs->merges_imply_patch = 1;
+		set_func = set_dense_combined;
 	} else if (!strcmp(arg, "--remerge-diff")) {
-		set_remerge_diff(revs);
-		revs->merges_imply_patch = 1;
+		set_func = set_remerge_diff;
 	} else if (!strcmp(arg, "--no-diff-merges")) {
 		set_none(revs);
 	} else if (!strcmp(arg, "--combined-all-paths")) {
@@ -183,6 +197,12 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 	} else
 		return 0;
 
+	if (set_func != NULL) {
+		(force_func ? force_func : set_func)(revs);
+		force_func = NULL;
+		revs->merges_imply_patch = 1;
+	}
+
 	revs->explicit_diff_merges = 1;
 	return argcount;
 }
diff --git a/diff-merges.h b/diff-merges.h
index 9f0b3901fe82..6ef0cc87bb2a 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -15,6 +15,8 @@ int diff_merges_hide_config(int hide);
 
 int diff_merges_m_imply_p_config(int on);
 
+int diff_merges_force_config(const char *value);
+
 void diff_merges_suppress_m_parsing(void);
 
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 1789dd6063c5..8a90d2dac360 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git config log.diffMergesForce has proper effect' '
+	git log -m -p master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMergesForce on &&
+	git log --cc master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git config log.diffMergesForce override by duplicate' '
+	git log --cc master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMergesForce on &&
+	git log --cc --cc master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 # -m in "git diff-index" means "match missing", that differs
 # from its meaning in "git diff". Let's check it in diff-index.
 # The line in the output for removed file should disappear when
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 26a7e4ff877c..411e08b2fa1b 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2498,6 +2498,7 @@ test_expect_success 'git config - variable name' '
 	log.decorate Z
 	log.diffMerges Z
 	log.diffMergesHide Z
+	log.diffMergesForce Z
 	log.diffMerges-m-imply-p Z
 	EOF
 '
@@ -2528,6 +2529,7 @@ test_expect_success 'git -c - variable name' '
 	log.decorate=Z
 	log.diffMerges=Z
 	log.diffMergesHide=Z
+	log.diffMergesForce=Z
 	log.diffMerges-m-imply-p=Z
 	EOF
 '
@@ -2552,6 +2554,7 @@ test_expect_success 'git clone --config= - variable name' '
 	log.decorate=Z
 	log.diffMerges=Z
 	log.diffMergesHide=Z
+	log.diffMergesForce=Z
 	log.diffMerges-m-imply-p=Z
 	EOF
 '
-- 
2.37.3.526.g5f84746cb16b

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> +	if (set_func != NULL) {

Please write it like so:

	if (set_func) {

I am not reviewing any new feature topic during -rc period (yet),
but this triggered CI failure at the tip of 'seen', so.

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> +	if (set_func != NULL) {
>
> Please write it like so:
>
> 	if (set_func) {

OK, will do.

>
> I am not reviewing any new feature topic during -rc period (yet),
> but this triggered CI failure at the tip of 'seen', so.

Thanks! Do we now have tool for auto-check for these issues? I still use
one from Linux kernel, and it didn't object to this form.

-- Sergey Organov

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> +	if (set_func != NULL) {
>>
>> Please write it like so:
>>
>> 	if (set_func) {
>
> OK, will do.
>
>>
>> I am not reviewing any new feature topic during -rc period (yet),
>> but this triggered CI failure at the tip of 'seen', so.
>
> Thanks! Do we now have tool for auto-check for these issues? I still use
> one from Linux kernel, and it didn't object to this form.

I noticed it when I pushed to GitHub, which ran the CI ;-)

If you have your own fork at GitHub of https://github.com/git/git/, I
think preparing a pull request against it triggers the CI.

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Ævar Arnfjörð Bjarmason]

On Wed, Nov 30 2022, Junio C Hamano wrote:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sergey Organov <sorganov@gmail.com> writes:
>>>
>>>> +	if (set_func != NULL) {
>>>
>>> Please write it like so:
>>>
>>> 	if (set_func) {
>>
>> OK, will do.
>>
>>>
>>> I am not reviewing any new feature topic during -rc period (yet),
>>> but this triggered CI failure at the tip of 'seen', so.
>>
>> Thanks! Do we now have tool for auto-check for these issues? I still use
>> one from Linux kernel, and it didn't object to this form.
>
> I noticed it when I pushed to GitHub, which ran the CI ;-)
>
> If you have your own fork at GitHub of https://github.com/git/git/, I
> think preparing a pull request against it triggers the CI.

...in this case though there's no reason to wait for the glacially slow
GitHub CI. You just need spatch installed and:

	make coccicheck

Sergey: If you've hacked on the (I'm assuming linux) kernel you likely
have that installed already, they use it too (being the canonical and I
believe original use-case for it).

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Sergey Organov]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, Nov 30 2022, Junio C Hamano wrote:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> Sergey Organov <sorganov@gmail.com> writes:
>>>>
>>>>> +	if (set_func != NULL) {
>>>>
>>>> Please write it like so:
>>>>
>>>> 	if (set_func) {
>>>
>>> OK, will do.
>>>
>>>>
>>>> I am not reviewing any new feature topic during -rc period (yet),
>>>> but this triggered CI failure at the tip of 'seen', so.
>>>
>>> Thanks! Do we now have tool for auto-check for these issues? I still use
>>> one from Linux kernel, and it didn't object to this form.
>>
>> I noticed it when I pushed to GitHub, which ran the CI ;-)
>>
>> If you have your own fork at GitHub of https://github.com/git/git/, I
>> think preparing a pull request against it triggers the CI.
>
> ...in this case though there's no reason to wait for the glacially slow
> GitHub CI. You just need spatch installed and:
>
> 	make coccicheck
>
> Sergey: If you've hacked on the (I'm assuming linux) kernel you likely
> have that installed already, they use it too (being the canonical and I
> believe original use-case for it).

Thanks a lot! I'll definitely give it a try even though I didn't use it
while hacking on the Linux kernel.

-- Sergey Organov

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> If you have your own fork at GitHub of https://github.com/git/git/, I
>> think preparing a pull request against it triggers the CI.
>
> ...in this case though there's no reason to wait for the glacially slow
> GitHub CI. You just need spatch installed and:
>
> 	make coccicheck

And you have to wait for the glacially slow coccicheck run locally,
though ;-).

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Jeff King]

On Mon, Nov 28, 2022 at 05:44:29PM +0300, Sergey Organov wrote:

> >> +	if (set_func != NULL) {
> >
> > Please write it like so:
> >
> > 	if (set_func) {
>[...]
> Thanks! Do we now have tool for auto-check for these issues? I still use
> one from Linux kernel, and it didn't object to this form.

Running "make coccicheck" will find this, but of course you need to have
coccinelle installed. Note that if it finds anything, "make" won't
report an error. You have to check for non-empty files in
contrib/coccinelle/*.patch.

-Peff

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Sergey Organov]

Jeff King <peff@peff.net> writes:

> On Mon, Nov 28, 2022 at 05:44:29PM +0300, Sergey Organov wrote:
>
>> >> +	if (set_func != NULL) {
>> >
>> > Please write it like so:
>> >
>> > 	if (set_func) {
>>[...]
>> Thanks! Do we now have tool for auto-check for these issues? I still use
>> one from Linux kernel, and it didn't object to this form.
>
> Running "make coccicheck" will find this, but of course you need to have
> coccinelle installed. Note that if it finds anything, "make" won't
> report an error. You have to check for non-empty files in
> contrib/coccinelle/*.patch.

Thank you! I'll give it a try.

-- Sergey Organov

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Elijah Newren]

On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Force specified log format for -c, --cc, and --remerge-diff options
> instead of their respective formats. The override is useful when some
> external tool hard-codes diff for merges format option.
>
> Using any of the above options twice or more will get back the
> original meaning of the option no matter what configuration says.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  Documentation/config/log.txt | 11 +++++++++++
>  builtin/log.c                |  2 ++
>  diff-merges.c                | 32 ++++++++++++++++++++++++++------
>  diff-merges.h                |  2 ++
>  t/t4013-diff-various.sh      | 18 ++++++++++++++++++
>  t/t9902-completion.sh        |  3 +++
>  6 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
> index 265a57312e58..7452c7fad638 100644
> --- a/Documentation/config/log.txt
> +++ b/Documentation/config/log.txt
> @@ -43,6 +43,17 @@ log.diffMergesHide::
>  log.diffMerges-m-imply-p::
>         `true` enables implication of `-p` by `-m`.
>
> +log.diffMergesForce::
> +       Use specified log format for -c, --cc, and --remerge-diff
> +       options instead of their respective formats when the option
> +       appears on the command line one time. See `--diff-merges` in
> +       linkgit:git-log[1] for allowed values. Using 'off' or 'none'
> +       disables the override (default).
> ++
> +The override is useful when external tool hard-codes one of the above
> +options. Using any of these options two (or more) times will get back
> +the original meaning of the options.

I didn't quite understand your intent here from this explanation.
When you pointed out to Junio that you wanted to override magit's
hard-coded `git log --cc` and turn it into `git log -m -p`, then it
suddenly made more sense.  And the two or more times I guess is your
escape hatch to allow users to say "I *really* do want this other
format, so `git log --cc --cc` will get it for me.".

Maybe something like:

Override -c, --cc, --remerge-diff options and use the specified
diff-generation scheme for merges instead.  However, this config
setting can in turn be overridden by specifying an alternate option
multiple times (e.g. `git log --cc --cc`).  Overriding the
diff-generation scheme for merges can be useful when an external tool
has a hard-coded command line it calls such as `git log --cc`.  See
`--diff-merges` in linkgit:git-log[1] for allowed values.  Using 'off'
or 'none' disables the override (default).

However:
  * This feels like we're trying to workaround bugs or inflexibility
in other tools with code in Git.  This feels like a slippery slope
issue and/or fixing the wrong tool.
  * Why is this just for -c, --cc, and --remerge-diff, and not for
also overriding -m?  It seems odd that one would be left out,
especially since tools are more likely to have hard-coded it than
--remerge-diff, given that -m has been around for a long time and
--remerge-diff is new.

> +
>  log.follow::
>         If `true`, `git log` will act as if the `--follow` option was used when
>         a single <path> is given.  This has the same limitations as `--follow`,
[...]
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 1789dd6063c5..8a90d2dac360 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '
>         test_cmp expected actual
>  '
>
> +test_expect_success 'git config log.diffMergesForce has proper effect' '
> +       git log -m -p master >result &&
> +       process_diffs result >expected &&
> +       test_config log.diffMergesForce on &&

I think the default for `on` is bad; it made sense at the time, but I
think we have a better option now.  Perhaps we switch to it, perhaps
we don't, but if there's _any_ chance at all we change the default for
"on" (which I think there definitely is), then you should really use
the option that matches the actual mode you are using rather than a
synonym for it; doing so future-proofs this testcase.

> +       git log --cc master >result &&
> +       process_diffs result >actual &&
> +       test_cmp expected actual
> +'
> +
> +test_expect_success 'git config log.diffMergesForce override by duplicate' '
> +       git log --cc master >result &&
> +       process_diffs result >expected &&
> +       test_config log.diffMergesForce on &&

Matters less here, but just in case "--cc" were to become the default,
it'd be nice to explicitly use something else like separate here.

> +       git log --cc --cc master >result &&
> +       process_diffs result >actual &&
> +       test_cmp expected actual
> +'

Re: [PATCH 3/5] diff-merges: implement log.diffMergesForce config

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Force specified log format for -c, --cc, and --remerge-diff options
>> instead of their respective formats. The override is useful when some
>> external tool hard-codes diff for merges format option.
>>
>> Using any of the above options twice or more will get back the
>> original meaning of the option no matter what configuration says.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  Documentation/config/log.txt | 11 +++++++++++
>>  builtin/log.c                |  2 ++
>>  diff-merges.c                | 32 ++++++++++++++++++++++++++------
>>  diff-merges.h                |  2 ++
>>  t/t4013-diff-various.sh      | 18 ++++++++++++++++++
>>  t/t9902-completion.sh        |  3 +++
>>  6 files changed, 62 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
>> index 265a57312e58..7452c7fad638 100644
>> --- a/Documentation/config/log.txt
>> +++ b/Documentation/config/log.txt
>> @@ -43,6 +43,17 @@ log.diffMergesHide::
>>  log.diffMerges-m-imply-p::
>>         `true` enables implication of `-p` by `-m`.
>>
>> +log.diffMergesForce::
>> +       Use specified log format for -c, --cc, and --remerge-diff
>> +       options instead of their respective formats when the option
>> +       appears on the command line one time. See `--diff-merges` in
>> +       linkgit:git-log[1] for allowed values. Using 'off' or 'none'
>> +       disables the override (default).
>> ++
>> +The override is useful when external tool hard-codes one of the above
>> +options. Using any of these options two (or more) times will get back
>> +the original meaning of the options.
>
> I didn't quite understand your intent here from this explanation.
> When you pointed out to Junio that you wanted to override magit's
> hard-coded `git log --cc` and turn it into `git log -m -p`, then it
> suddenly made more sense.  And the two or more times I guess is your
> escape hatch to allow users to say "I *really* do want this other
> format, so `git log --cc --cc` will get it for me.".
>
> Maybe something like:
>
> Override -c, --cc, --remerge-diff options and use the specified
> diff-generation scheme for merges instead.  However, this config
> setting can in turn be overridden by specifying an alternate option
> multiple times (e.g. `git log --cc --cc`).  Overriding the
> diff-generation scheme for merges can be useful when an external tool
> has a hard-coded command line it calls such as `git log --cc`.  See
> `--diff-merges` in linkgit:git-log[1] for allowed values.  Using 'off'
> or 'none' disables the override (default).

Thanks for suggestion, I'll take this into consideration should we agree
to actually let this feature in.

>
> However:
>   * This feels like we're trying to workaround bugs or inflexibility
> in other tools with code in Git.  This feels like a slippery slope
> issue and/or fixing the wrong tool.

Yep, that's why I said in my another answer to Junio that I won't insist
on it if you guys object, even though it does look useful for me.

>   * Why is this just for -c, --cc, and --remerge-diff, and not for
> also overriding -m?  It seems odd that one would be left out,
> especially since tools are more likely to have hard-coded it than
> --remerge-diff, given that -m has been around for a long time and
> --remerge-diff is new.

'-m' is rather the first one that got an override support, see
'log.diffMerges'.

[As for --remerge-diff, as a side note, I'd call it something like --rd
for short, as we have --diff-merges=remerge anyway. And then I'll think
about adding --pd  (pure-diff) or --fpd (first-parent-diff) ;-)]

>
>> +
>>  log.follow::
>>         If `true`, `git log` will act as if the `--follow` option was used when
>>         a single <path> is given.  This has the same limitations as `--follow`,
> [...]
>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
>> index 1789dd6063c5..8a90d2dac360 100755
>> --- a/t/t4013-diff-various.sh
>> +++ b/t/t4013-diff-various.sh
>> @@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '
>>         test_cmp expected actual
>>  '
>>
>> +test_expect_success 'git config log.diffMergesForce has proper effect' '
>> +       git log -m -p master >result &&
>> +       process_diffs result >expected &&
>> +       test_config log.diffMergesForce on &&
>
> I think the default for `on` is bad; it made sense at the time, but I
> think we have a better option now.

We probably disagree about what a better option actually is, but the
point is valid anyway.

> Perhaps we switch to it, perhaps we don't, but if there's _any_ chance
> at all we change the default for "on" (which I think there definitely
> is), then you should really use the option that matches the actual
> mode you are using rather than a synonym for it; doing so
> future-proofs this testcase.

Yep, agreed. Thanks for the catch!

>
>> +       git log --cc master >result &&
>> +       process_diffs result >actual &&
>> +       test_cmp expected actual
>> +'
>> +
>> +test_expect_success 'git config log.diffMergesForce override by duplicate' '
>> +       git log --cc master >result &&
>> +       process_diffs result >expected &&
>> +       test_config log.diffMergesForce on &&
>
> Matters less here, but just in case "--cc" were to become the default,
> it'd be nice to explicitly use something else like separate here.

Yes, thanks!

>
>> +       git log --cc --cc master >result &&
>> +       process_diffs result >actual &&
>> +       test_cmp expected actual
>> +'

-- Sergey Organov

[PATCH 4/5] diff-merges: support list of values for --diff-merges

[Sergey Organov]

Support comma-separated list of options in --diff-merges=. This allows
for shorter:

  --diff-merges=on,hide

instead of:

  --diff-merges=on --diff-merges=hide

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/diff-options.txt |  5 +++--
 diff-merges.c                  | 22 ++++++++++++++++++----
 t/t4013-diff-various.sh        |  8 ++++++++
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e64066af8d96..46c98c87e24f 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -38,7 +38,8 @@ ifdef::git-log[]
 --no-diff-merges::
 	Specify diff format to be used for merge commits. Default is
 	{diff-merges-default} unless `--first-parent` is in use, in which case
-	`first-parent` is the default.
+	`first-parent` is the default. Comma-separated list of
+	supported values is accepted as well.
 +
 --diff-merges=(off|none):::
 --no-diff-merges:::
@@ -53,7 +54,7 @@ ifdef::git-log[]
 	`log.diffMerges` configuration parameter, which default value
 	is `separate`.
 +
-	`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'.
+	`-m` is a shortcut for '--diff-merges=on,hide'.
 	In addition it implies `-p` when `log.diffMerges-m-imply-p` is
 	active.
 +
diff --git a/diff-merges.c b/diff-merges.c
index cedd7652bf42..ddf9a411c49c 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -1,6 +1,7 @@
 #include "diff-merges.h"
 
 #include "revision.h"
+#include "strbuf.h"
 
 typedef void (*diff_merges_setup_func_t)(struct rev_info *);
 static void set_separate(struct rev_info *revs);
@@ -110,12 +111,25 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 
 static void set_diff_merges(struct rev_info *revs, const char *optarg)
 {
-	diff_merges_setup_func_t func = func_by_opt(optarg);
+	char const delim = ',';
+	struct strbuf **opts = strbuf_split_str(optarg, delim, -1);
+	struct strbuf **p;
 
-	if (!func)
-		die(_("invalid value for '%s': '%s'"), "--diff-merges", optarg);
+	for (p = opts; *p; p++) {
+		diff_merges_setup_func_t func;
+		char *opt = (*p)->buf;
+		int len = (*p)->len;
 
-	func(revs);
+		if (opt[len - 1] == delim)
+			opt[len - 1] = '\0';
+		func = func_by_opt(opt);
+		if (!func) {
+			strbuf_list_free(opts);
+			die(_("invalid value for '%s': '%s'"), "--diff-merges", opt);
+		}
+		func(revs);
+	}
+	strbuf_list_free(opts);
 }
 
 /*
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 8a90d2dac360..2c68d06074ed 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -485,6 +485,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
 	test_cmp expected actual
 '
 
+test_expect_success 'log --diff-merges=<V1>,<V2>' '
+	git log --diff-merges=1,hide master >result &&
+	process_diffs result >expected &&
+	git log --diff-merges=1 --diff-merges=hide master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'deny wrong log.diffMerges config' '
 	test_config log.diffMerges wrong-value &&
 	test_expect_code 128 git log
-- 
2.37.3.526.g5f84746cb16b

[PATCH 5/5] diff-merges: issue warning on lone '-m' option

[Sergey Organov]

Lone '-m' is in use by scripts/aliases that aim at enabling diff
output for merge commits, but only if '-p' is then specified as well.

As '-m' may now be configured to imply '-p', using
'log.diffMerges-m-imply-p', issue warning and suggest to instead use

  --diff-merges=on,hide

that does not depend on user configuration.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/diff-merges.c b/diff-merges.c
index ddf9a411c49c..b3b3c9e44ba8 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -11,6 +11,7 @@ static diff_merges_setup_func_t force_func = NULL;
 static int suppress_m_parsing;
 static int hide = 0;
 static int m_imply_p = 0;
+static int got_m = 0;
 
 static void suppress(struct rev_info *revs)
 {
@@ -196,6 +197,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 		set_to_default(revs);
 		set_hide(revs);
 		revs->merges_imply_patch = m_imply_p;
+		got_m = 1;
 	} else if (!strcmp(arg, "-c")) {
 		set_func = set_combined;
 	} else if (!strcmp(arg, "--cc")) {
@@ -259,5 +261,7 @@ void diff_merges_setup_revs(struct rev_info *revs)
 	if (revs->merges_imply_patch || revs->merges_need_diff) {
 		if (!revs->diffopt.output_format)
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
-	}
+	} else if (got_m)
+		warning(_("legacy use of lone '-m' detected: please use '--diff-merges=on,hide' instead, as '-m' may imply '-p'"));
+
 }
-- 
2.37.3.526.g5f84746cb16b

Re: [PATCH 0/5] diff-merges: more features

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> 2. log.diffMerges-m-imply-p
>
> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'
> options do. Simply fixing this inconsistency by unconditional
> modification of '-m' semantics appeared to be a bad idea, as it broke
> some legacy scripts/aliases. This patch rather provides configuration
> variable to tweak '-m' behavior accordingly.

I do not know how this can be a good idea.  For those users who set
the configuration variables, those scripts and aliases get broken
anyway, don't they?  IOW, I am not sure how this is better than the
"modification of '-m'" that is "a bad idea".  I would understand why
it may be a safer and more sensible solution, if the proposed
approach were to find an unused letter $X and to introduce "-$X"
that is the same as "-m" but implies "-p", though.

> 3. log.diffMergesForce
>
> Force specific log format for -c, --cc, and --remerge-diff options
> instead of their respective formats. The override is useful when some
> external tool hard-codes diff for merges format option.

Not convinced it is a good idea for the same reason as above (not
convinced it is a bad idea, either, though).

> 4. Support list of values for --diff-merges
>
> This allows for shorter --diff-merges=on,hide forms.

Good, probably.

Re: [PATCH 0/5] diff-merges: more features

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> 2. log.diffMerges-m-imply-p
>>
>> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'
>> options do. Simply fixing this inconsistency by unconditional
>> modification of '-m' semantics appeared to be a bad idea, as it broke
>> some legacy scripts/aliases. This patch rather provides configuration
>> variable to tweak '-m' behavior accordingly.
>
> I do not know how this can be a good idea.  For those users who set
> the configuration variables, those scripts and aliases get broken
> anyway, don't they?  IOW, I am not sure how this is better than the
> "modification of '-m'" that is "a bad idea".

The history behind this is that before the patch #1 of these series
there was no way to get exact semantics of current '-m' option using new
--diff-merges option, so there was no sensible way to "fix" these
scripts/aliases. That was in fact the primary objection for the new '-m'
semantics.

Now, when one stomps on such script/alias (by explicitly enabling the
new configuration option), they can fix it by replacing '-m' with
'--diff-merges=on,hide', that, along with the last patch of these series
that produces warning when lone '-m' is detected, looks to me as a way
to eventually get rid of the legacy and surprising '-m' semantics.

> I would understand why it may be a safer and more sensible solution,
> if the proposed approach were to find an unused letter $X and to
> introduce "-$X" that is the same as "-m" but implies "-p", though.

I've in fact considered this as well, and '-d' was free for such use
last time I've checked. It was very tempting to just add '-d' that
always shows diff to first parent for everything, be it merge or not,
and call it a day, but it won't fix '-m', which inconsistent behavior
still surprises people, and besides started the whole --diff-merges
business in the first place.

>
>> 3. log.diffMergesForce
>>
>> Force specific log format for -c, --cc, and --remerge-diff options
>> instead of their respective formats. The override is useful when some
>> external tool hard-codes diff for merges format option.
>
> Not convinced it is a good idea for the same reason as above (not
> convinced it is a bad idea, either, though).

Here the intention was entirely different though. I'm using magit and it
does hard-code --cc in one place that I'd like to be able to override.
If I got this problem, I figured chances are high somebody else will get
it as well, and that's the rationale.

Then I figure everybody here has own favorite format for merge commits,
and this toy gives you exactly this: whatever option is used (once), you
get what you prefer instead, or use an option second time to enforce it.

For stable scripting, we for rather long time now have --diff-merges=c,
and --diff-merges=cc, as well as slightly more recent
--diff-merges=remerge, that are not affected by the configuration in
question.

That said, it looks useful to me, but I won't insist on the feature
should you guys object.

>
>> 4. Support list of values for --diff-merges
>>
>> This allows for shorter --diff-merges=on,hide forms.
>
> Good, probably.

Was useless before 'hide' though, as the rest of options just override
each other entirely.

Thanks,
-- Sergey Organov

Re: [PATCH 0/5] diff-merges: more features

[Elijah Newren]

On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> 1. --diff-merges=[no]-hide

This seems problematic to me.  Currently, all the options to
diff-merges are exclusive of each other; the user is picking one of
them to determine how to format diffs for merges.  Now you are
introducing the ability to combine various options, leading users to
think that perhaps they can run with all three of
`--diff-merges=combined-dense --diff-merges=remerge
--diff-merges=separate` or other nonsensical combinations.  Shouldn't
this [no-]hide stuff be a separate flag rather than reusing
--diff-merges?

> The set of diff-merges options happened to be incomplete, and failed
> to implement exact semantics of -m option that hides output of diffs
> for merge commits unless -p option is active as well.
>
> The new "hide" option fixes this issue, so that now
>
>   --diff-merges=on --diff-merges=hide
>
> combo is the exact synonym for -m.

Why is completeness important here?  Perhaps I should state this
another way: when would users ever want to use this new "hide" option?
 I got through your cover letter not knowing the answer to this, but
was hoping it'd at least be covered in one of your commit messages or
documentation changes.  Maybe it was there, but I somehow missed it.

Is the only goal some sense of developer completeness for these
options, or are these end-user-facing options of utility to actual end
users?  I'm hoping the latter, but if so, can that be documented and
explained somewhere?  I'm pretty sure this is explained somewhere in
an old mailing list discussion, but where?

> The log.diffMerges configuration also accepts "hide" and "no-hide"
> values, and governs the default value for the hide bit. The
> configuration behaves as if "--diff-merges=[no-]hide" is inserted
> first in the command-line.
>
> 2. log.diffMerges-m-imply-p
>
> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'
> options do. Simply fixing this inconsistency by unconditional
> modification of '-m' semantics appeared to be a bad idea, as it broke
> some legacy scripts/aliases. This patch rather provides configuration
> variable to tweak '-m' behavior accordingly.

> 3. log.diffMergesForce
>
> Force specific log format for -c, --cc, and --remerge-diff options
> instead of their respective formats. The override is useful when some
> external tool hard-codes diff for merges format option.

Why just these three options and not -m (or --diff-merges=separate)?

Also, I read this and didn't quite fully grasp the intent; your
explanation in response to Junio seemed much more enlightening.
Perhaps the wording/explanation could be cleaned up a bit?  I'll
comment more on that specific patch...

> 4. Support list of values for --diff-merges
>
> This allows for shorter --diff-merges=on,hide forms.

And thus making users think they can pass
--diff-merges=combined-dense,remerge,separate and suspecting that
it'll do something useful?  Seems like this is reinforcing a mistake
to me.


> 5. Issue warning for lone '-m'.
>
> Lone '-m' is in use by scripts/aliases that aim at enabling diff
> output for merge commits, but only if '-p' is then specified as well.
>
> As '-m' may now be configured to imply '-p' (using
> 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,
> and suggest to instead use '--diff-merges=on,hide' that does not
> depend on user configuration.
>
> This is expected to give a provision for enabling
> log.diffMerges-m-imply-p by default in the future.
>
> Sergey Organov (5):
>   diff-merges: implement [no-]hide option and log.diffMergesHide config
>   diff-merges: implement log.diffMerges-m-imply-p config
>   diff-merges: implement log.diffMergesForce config
>   diff-merges: support list of values for --diff-merges
>   diff-merges: issue warning on lone '-m' option
>
>  Documentation/config/log.txt                  |  20 ++++
>  Documentation/diff-options.txt                |  20 +++-
>  builtin/log.c                                 |   6 +
>  diff-merges.c                                 | 108 +++++++++++++++---
>  diff-merges.h                                 |   6 +
>  t/t4013-diff-various.sh                       |  89 ++++++++++++++-
>  ...ges=first-parent_--diff-merges=hide_master |  34 ++++++
>  t/t9902-completion.sh                         |   9 ++
>  8 files changed, 272 insertions(+), 20 deletions(-)
>  create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master
>
> --
> 2.37.3.526.g5f84746cb16b
>

Re: [PATCH 0/5] diff-merges: more features

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> 1. --diff-merges=[no]-hide
>
> This seems problematic to me.  Currently, all the options to
> diff-merges are exclusive of each other; the user is picking one of
> them to determine how to format diffs for merges.  Now you are
> introducing the ability to combine various options, leading users to
> think that perhaps they can run with all three of
> `--diff-merges=combined-dense --diff-merges=remerge
> --diff-merges=separate` or other nonsensical combinations.  Shouldn't
> this [no-]hide stuff be a separate flag rather than reusing
> --diff-merges?

Yes, it's a precedent indeed, but I don't see any actual problem here.
Unlike git silently dropping changes on rebase, this can cause no
damage. I think I can emphasize that we now have "formats" and "flags"
in the documentation, where "formats" are mutually exclusive (the latest
specified wins), while "flags" are cumulative.

>
>> The set of diff-merges options happened to be incomplete, and failed
>> to implement exact semantics of -m option that hides output of diffs
>> for merge commits unless -p option is active as well.
>>
>> The new "hide" option fixes this issue, so that now
>>
>>   --diff-merges=on --diff-merges=hide
>>
>> combo is the exact synonym for -m.
>
> Why is completeness important here?  Perhaps I should state this
> another way: when would users ever want to use this new "hide" option?
>  I got through your cover letter not knowing the answer to this, but
> was hoping it'd at least be covered in one of your commit messages or
> documentation changes.  Maybe it was there, but I somehow missed it.
>
> Is the only goal some sense of developer completeness for these
> options, or are these end-user-facing options of utility to actual end
> users?  I'm hoping the latter, but if so, can that be documented and
> explained somewhere?  I'm pretty sure this is explained somewhere in
> an old mailing list discussion, but where?

Completeness is essential as I want '--diff-merges' to provide all the
needed capabilities, and one of them was actually missing, that is there
in the '-m' semantics, exactly as I said in the descriptions.

>
>> The log.diffMerges configuration also accepts "hide" and "no-hide"
>> values, and governs the default value for the hide bit. The
>> configuration behaves as if "--diff-merges=[no-]hide" is inserted
>> first in the command-line.
>>
>> 2. log.diffMerges-m-imply-p
>>
>> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'
>> options do. Simply fixing this inconsistency by unconditional
>> modification of '-m' semantics appeared to be a bad idea, as it broke
>> some legacy scripts/aliases. This patch rather provides configuration
>> variable to tweak '-m' behavior accordingly.
>
>> 3. log.diffMergesForce
>>
>> Force specific log format for -c, --cc, and --remerge-diff options
>> instead of their respective formats. The override is useful when some
>> external tool hard-codes diff for merges format option.
>
> Why just these three options and not -m (or --diff-merges=separate)?

As I said in my answer to your other mail, '-m' is already configurable,
so it is not needed to be included.

None of --diff-merges= options are affected by diffMergesForce, only 3
specific options from the documentation.

>
> Also, I read this and didn't quite fully grasp the intent; your
> explanation in response to Junio seemed much more enlightening.
> Perhaps the wording/explanation could be cleaned up a bit?  I'll
> comment more on that specific patch...

Yeah, thanks, I got your suggestion you put in another mail.

>
>> 4. Support list of values for --diff-merges
>>
>> This allows for shorter --diff-merges=on,hide forms.
>
> And thus making users think they can pass
> --diff-merges=combined-dense,remerge,separate and suspecting that
> it'll do something useful?  Seems like this is reinforcing a mistake
> to me.

Yes, they can. For now it's useful only for 'hide', but we might add
more flags in the future. It's also harmless, so I don't see it as a
serious issue.

Thanks,
-- Sergey Organov

Re: [PATCH 0/5] diff-merges: more features

[Elijah Newren]

On Wed, Nov 30, 2022 at 5:16 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> 1. --diff-merges=[no]-hide
> >
> > This seems problematic to me.  Currently, all the options to
> > diff-merges are exclusive of each other; the user is picking one of
> > them to determine how to format diffs for merges.  Now you are
> > introducing the ability to combine various options, leading users to
> > think that perhaps they can run with all three of
> > `--diff-merges=combined-dense --diff-merges=remerge
> > --diff-merges=separate` or other nonsensical combinations.  Shouldn't
> > this [no-]hide stuff be a separate flag rather than reusing
> > --diff-merges?
>
> Yes, it's a precedent indeed, but I don't see any actual problem here.
> Unlike git silently dropping changes on rebase, this can cause no
> damage.

Sure, read-only options for querying things won't cause future damage.
That doesn't mean that the UI for commands like diff/log/grep/etc are
unimportant, though, and certainly doesn't excuse intentionally
creating bad UI for them.

> I think I can emphasize that we now have "formats" and "flags"
> in the documentation, where "formats" are mutually exclusive (the latest
> specified wins), while "flags" are cumulative.

Why not just give it a different flag name, so that "formats" and
"flags" are clearly separated without even needing a lengthy
explanation?  That'd be much simpler to understand and explain.

> >> The set of diff-merges options happened to be incomplete, and failed
> >> to implement exact semantics of -m option that hides output of diffs
> >> for merge commits unless -p option is active as well.
> >>
> >> The new "hide" option fixes this issue, so that now
> >>
> >>   --diff-merges=on --diff-merges=hide
> >>
> >> combo is the exact synonym for -m.
> >
> > Why is completeness important here?  Perhaps I should state this
> > another way: when would users ever want to use this new "hide" option?
> >  I got through your cover letter not knowing the answer to this, but
> > was hoping it'd at least be covered in one of your commit messages or
> > documentation changes.  Maybe it was there, but I somehow missed it.
> >
> > Is the only goal some sense of developer completeness for these
> > options, or are these end-user-facing options of utility to actual end
> > users?  I'm hoping the latter, but if so, can that be documented and
> > explained somewhere?  I'm pretty sure this is explained somewhere in
> > an old mailing list discussion, but where?
>
> Completeness is essential as I want '--diff-merges' to provide all the
> needed capabilities, and one of them was actually missing, that is there
> in the '-m' semantics, exactly as I said in the descriptions.

I ask you why a user would want to use this option, and you simply
assert that it's a "needed capabilit[y]"?  Could you explain *why*
it's needed or helpful for users instead of just repeating the
assertion that it is needed?

If I can't figure out why it's needed or useful for users despite
having read your cover letter, commit messages, underlying source code
and documentation, and this full thread, then there may well be
something wrong with me...but it seems likely that many users will
also have difficulty figuring out why this option is useful.

Re: [PATCH 0/5] diff-merges: more features

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Wed, Nov 30, 2022 at 5:16 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>> > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote:
>> >>
>> >> 1. --diff-merges=[no]-hide
>> >
>> > This seems problematic to me.  Currently, all the options to
>> > diff-merges are exclusive of each other; the user is picking one of
>> > them to determine how to format diffs for merges.  Now you are
>> > introducing the ability to combine various options, leading users to
>> > think that perhaps they can run with all three of
>> > `--diff-merges=combined-dense --diff-merges=remerge
>> > --diff-merges=separate` or other nonsensical combinations.  Shouldn't
>> > this [no-]hide stuff be a separate flag rather than reusing
>> > --diff-merges?
>>
>> Yes, it's a precedent indeed, but I don't see any actual problem here.
>> Unlike git silently dropping changes on rebase, this can cause no
>> damage.
>
> Sure, read-only options for querying things won't cause future damage.
> That doesn't mean that the UI for commands like diff/log/grep/etc are
> unimportant, though, and certainly doesn't excuse intentionally
> creating bad UI for them.

I just don't see it as a bad UI, sorry.

>
>> I think I can emphasize that we now have "formats" and "flags"
>> in the documentation, where "formats" are mutually exclusive (the latest
>> specified wins), while "flags" are cumulative.
>
> Why not just give it a different flag name, so that "formats" and
> "flags" are clearly separated without even needing a lengthy
> explanation?  That'd be much simpler to understand and explain.

What I did is the best I was able to think of. The --diff-merges
introduces namespace for everything related to formats of output of
diffs for merge commits, and 'hide' fits there perfectly. User doesn't
need to look elsewhere to figure entire set of capabilities.

I honestly don't see how to improve the UI by introducing yet another
option, especially provided the letter has its own drawbacks.

>
>> >> The set of diff-merges options happened to be incomplete, and failed
>> >> to implement exact semantics of -m option that hides output of diffs
>> >> for merge commits unless -p option is active as well.
>> >>
>> >> The new "hide" option fixes this issue, so that now
>> >>
>> >>   --diff-merges=on --diff-merges=hide
>> >>
>> >> combo is the exact synonym for -m.
>> >
>> > Why is completeness important here?  Perhaps I should state this
>> > another way: when would users ever want to use this new "hide" option?
>> >  I got through your cover letter not knowing the answer to this, but
>> > was hoping it'd at least be covered in one of your commit messages or
>> > documentation changes.  Maybe it was there, but I somehow missed it.
>> >
>> > Is the only goal some sense of developer completeness for these
>> > options, or are these end-user-facing options of utility to actual end
>> > users?  I'm hoping the latter, but if so, can that be documented and
>> > explained somewhere?  I'm pretty sure this is explained somewhere in
>> > an old mailing list discussion, but where?
>>
>> Completeness is essential as I want '--diff-merges' to provide all the
>> needed capabilities, and one of them was actually missing, that is there
>> in the '-m' semantics, exactly as I said in the descriptions.
>
> I ask you why a user would want to use this option, and you simply
> assert that it's a "needed capabilit[y]"?  Could you explain *why*
> it's needed or helpful for users instead of just repeating the
> assertion that it is needed?

I'm trying to explain that the use-case(s) is(are) at least the same as
for existing '-m' option, and '-m' is used in practice, so it must be
useful, right? Who am I to judge? So I don't.

For me one of the goals is to let people replace '-m' in scripts/aliases
with '--diff-merges=on,hide' and eventually let '-m' behave better as a
short user option, similar to '-c/--cc/--remrege-diff'. And then it
might happen that 'hide' is actually useful elsewhere (see below for a
try), as it often happens once particular functionality is properly
factored out of given use-case.

> If I can't figure out why it's needed or useful for users despite
> having read your cover letter, commit messages, underlying source code
> and documentation, and this full thread, then there may well be
> something wrong with me...but it seems likely that many users will
> also have difficulty figuring out why this option is useful.

Then they are still free not to use it, and I doubt they will try to
find why it's useful in the commit messages anyway, so do we need to put
something into the documentation then? What do you suggest in addition
to the functional explanation of the 'hide' that is already there?

That said, what comes to mind, as a use-case, I figure you might try to
define an alias for 'diff log' that will use 'remerge' format by default
once diffs are requested using '-p':

$ git config alias.lr 'log --diff-merges=remerge,hide'

and check if this 'git lr' is useful.

Thanks,
-- Sergey Organov

Re: [PATCH 0/5] diff-merges: more features

[Glen Choo]

We covered this series in Review Club, thanks for coming Sergey! For
those who are interested, the notes are here:

  https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?usp=sharing

and reviewers will send feedback to the list separately anyway. I mostly
had comments on the design, so I'll leave most of my comments here.

Commenting on the cover letter as a whole, on first read, it wasn't
obvious to me what this series was trying to achieve because the CL
presents the 5 patches individually instead of a cohesive story. From my
understanding, the story is as follows: we want '-m' (enable
diff-merges) to also imply '-p', but we can't just change the default
behavior, so we do the following instead:

- Add a config option that gives the behavior that we want (2/5).
- Deprecate '-m' by giving '--diff-merges=on;hide' as a synonym and
  encouraging users to use that instead (1,4,5/5).

Patch 3/5 is completely separate. There was some resistance to it during
Review Club, but if we still want this, it might be worth splitting off
into its own series so that we can keep the discussions separate.

During the discussion, it also appeared that this "modification of '-m'
semantics" refers to a patch that changed the default but got reverted
due to breaking legacy scripts. It would be extremely useful to include
a link to that previous patch and the discussion around its revert,
especially given the discussion about whether users actually need
'-diff-merges=hide' ([1] and elsewhere).

[1] https://lore.kernel.org/git/CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com/

Sergey Organov <sorganov@gmail.com> writes:

> 1. --diff-merges=[no]-hide
>
> The set of diff-merges options happened to be incomplete, and failed
> to implement exact semantics of -m option that hides output of diffs
> for merge commits unless -p option is active as well.
>
> The new "hide" option fixes this issue, so that now
>
>   --diff-merges=on --diff-merges=hide
>
> combo is the exact synonym for -m.
>
> The log.diffMerges configuration also accepts "hide" and "no-hide"
> values, and governs the default value for the hide bit. The
> configuration behaves as if "--diff-merges=[no-]hide" is inserted
> first in the command-line.

I had the same concerns as Elijah, which is that this behavior is
probably clearer as a separate flag (like "--hide-diff-merges"), which
is more consistent with how '--diff-options' is used today, which means
that:

a) it is easier to explain to users
b) the implementation is simpler (I'll comment on Patch 1 code
   separately)
c) it makes Patch 4 obsolete

But I'm not convinced that we actually want this behavior at all. I
don't see why a user would use a flag that says "do nothing unless
other flags are given". I don't find the 'alias use case' compelling,
because the user still has to choose whether to pass '-p', so at that
point they could just add a different alias.

I haven't dug through the code/ML to figure out whether '-m' requiring
'-p' was an intentional feature or not, but if you could find the old
thread where you changed the default (and it got reverted), that would
help the discussion a lot :)

> 2. log.diffMerges-m-imply-p
>
> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'
> options do. Simply fixing this inconsistency by unconditional
> modification of '-m' semantics appeared to be a bad idea, as it broke
> some legacy scripts/aliases. This patch rather provides configuration
> variable to tweak '-m' behavior accordingly.

I thought that Junio's suggestion to implement a flag that acts like
'-m' with '-p' [2] was quite a good one (maybe '-M' or
'--diff-merges=show'), since I think that very few users would actually
set this config, but the ones that would actually use it can just
replace '-m' with '-M'.

[2] https://lore.kernel.org/git/xmqqfse37c0n.fsf@gitster.g/

> 3. log.diffMergesForce
>
> Force specific log format for -c, --cc, and --remerge-diff options
> instead of their respective formats. The override is useful when some
> external tool hard-codes diff for merges format option.

This might be better off as its own series, since the change isn't
related to '-m', but I'm worried about the precedent that this sets.
To my knowledge, CLI options always overwrite config, but this is the
opposite. I would prefer not to do this, especially if the use case is
to work around an external tool (since it is arguably the tool that is
broken).

> 5. Issue warning for lone '-m'.
>
> Lone '-m' is in use by scripts/aliases that aim at enabling diff
> output for merge commits, but only if '-p' is then specified as well.
>
> As '-m' may now be configured to imply '-p' (using
> 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,
> and suggest to instead use '--diff-merges=on,hide' that does not
> depend on user configuration.
>
> This is expected to give a provision for enabling
> log.diffMerges-m-imply-p by default in the future.

Since '-m' without '-p' is a mistake in most cases, I wonder if we
should just emit this warning today (maybe via the advise() API). Even
if we don't keep '--diff-options=hide', deprecating lone '-m' and giving
a warning seems good to keep.

Re: [PATCH 0/5] diff-merges: more features

[Sergey Organov]

Glen Choo <chooglen@google.com> writes:

> We covered this series in Review Club, thanks for coming Sergey! For
> those who are interested, the notes are here:

Thank you all guys for the review and for valuable discussion!

[...]

> During the discussion, it also appeared that this "modification of '-m'
> semantics" refers to a patch that changed the default but got reverted
> due to breaking legacy scripts. It would be extremely useful to include
> a link to that previous patch and the discussion around its revert,
> especially given the discussion about whether users actually need
> '-diff-merges=hide' ([1] and elsewhere).

The last time '-m' issue appeared on the list, it all started here:

https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/

In particular, the final patch and its revert is deeper down this tread:

https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t

and

https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/

Where do you prefer these references to be put, in the cover letter, in
the commit message, or in both places?

-- Sergey Organov

Re: [PATCH 0/5] diff-merges: more features

[Glen Choo]

Sergey Organov <sorganov@gmail.com> writes:

> The last time '-m' issue appeared on the list, it all started here:
>
> https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/
>
> In particular, the final patch and its revert is deeper down this tread:
>
> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t
>
> and
>
> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/

Thanks, these provide extremely helpful context :) In particular:

- Junio describes this "do nothing unless -p" is given behavior as an
  accident [1].
- Jonathan Nieder notes that this change accidentally broke scripts
  where "-m" probably wasn't doing anything useful, but we wanted to
  avoid breaking the scripts for backwards compatibility anyway [2].

I got the sense that the closest thing to an intentional use case of
"-m" is for users who thought that "-m" would affect path limiting [3],
although it doesn't actually do that. So what I've reads so far suggests
that "do nothing unless -p" (aka --diff-merges=hide) is not actually
useful, and we should just drop it.

We could keep the warning for "-m" without "-p" (Patch 5), and recommend
"--diff-merges=(on|m)".

[1] https://lore.kernel.org/git/xmqqwnsl93m3.fsf@gitster.g/
[2] https://lore.kernel.org/git/YQtYEftByY8cNMml@google.com/
[3] https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/
>
> Where do you prefer these references to be put, in the cover letter, in
> the commit message, or in both places?
>
> -- Sergey Organov

Re: [PATCH 0/5] diff-merges: more features

[Sergey Organov]

Glen Choo <chooglen@google.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> The last time '-m' issue appeared on the list, it all started here:
>>
>> https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/
>>
>> In particular, the final patch and its revert is deeper down this tread:
>>
>> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t
>>
>> and
>>
>> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/
>
> Thanks, these provide extremely helpful context :) In particular:
>
> - Junio describes this "do nothing unless -p" is given behavior as an
>   accident [1].

I rather read it as Junio saying that "-m does not imply -p" is
historical accident, and yes, it is, provided '-c/--cc' were fixed at
some point, and '-m' was not, so in fact I figure Junio meant: "-m
differs from -c/--cc in not implying '-p'" is historical accident. And
then he suggests to leave it as is, with which I disagree.

In addition, in all the discussions, I believe Junio at least once said
he does see valid usages for current hush '-m':

<quote>
But "stash list" example shows that "log --first-parent -m" without
"-p" in a script has a valid reason, and a change that hurts those
who correctly used a command and an option in a way they were
intended to do _is_ problematic.
</quote>

here:

https://lore.kernel.org/git/xmqqy29chim6.fsf@gitster.g/

> - Jonathan Nieder notes that this change accidentally broke scripts
>   where "-m" probably wasn't doing anything useful, but we wanted to
>   avoid breaking the scripts for backwards compatibility anyway [2].
>
> I got the sense that the closest thing to an intentional use case of
> "-m" is for users who thought that "-m" would affect path limiting [3],
> although it doesn't actually do that.

I don't think so. Dunno why you got such feeling. It's rather that for
some time "--first-parent -m" was the only way to produce *most* useful
form of "-m" format: show diff with respect to the *first* parent only,
whereas without "--first-parent" "-m" produced diff output for *every*
parent in turn(!) giving extremely confusing result. Please notice how
--first-parent appears in most of those discussions.

Overall, the (simplified) history of '-m' goes like this, as far as I
can tell:

0. Original '-m', documented only for 'diff-tree'. The diffs were
   produced to all the parents that was probably very logical for
   plumbing 'diff-tree', as it reflects symmetric nature of merge
   commits in the DAG, that is the core of git data model.

   However, while "git log -p" does not produce patches for merge
   commits (apparently to get rid of often large output), '-m' in fact
   enforces the output for merge commits, in the format produced by
   'diff-tree -m', i.e., diffs to all the parents in turn.

   [The latter was probably the first mistake, it should have rather
   produced the diff with respect to the first parent that is more
   suitable for "git log" being porcelain, to show changes introduced by
   the commit to the mainline, exactly as for non-merge commits]

1. '-c' is introduced, then '--cc' is introduced, with semantics similar
   to '-m' with respect to '-p', but different kinds of output.

   At this point we have consistent behavior of '-m', '-c', and '--cc'
   with respect to '-p', none of which produce any output unless '-p' is
   specified as well.

2. '-c' and '--cc' are changed to imply '-p'[0]. '-m' is left alone,
   supposedly forgotten as being undocumented for "git log" and of
   limited use, due to its large and surprising output.

   [I think that was the second mistake, forgetting to change '-m'
   accordingly]

3. '-m' is changed to produce diff with respect to the first parent only
   when '--first-parent' is specified [1]. '-m' finally starts to
   (sometimes) give useful output, and starts to be actually used,
   but only together with '--first-parent' most of times.

   BTW, this is the first time '-m' has been documented as part of "git
   log": "This patch properly documents the -m switch, which has so far
   been mentioned only as a fairly special diff-tree flag."

   [I think there are more mistakes here: not changing '-m' to imply
   '-p' at this point, and not producing single-parent diff even without
   '--first-parent']

4. "--first-parent" is suggested to imply "-m"(!) to let "--first-parent
   -p" to produce diff for merge commits[2]. That in turn needed an
   option that will negate implied "-m", and that's where
   --[no-]diff-merges was suggested.

   Please notice that if '-m' implied '-p' (as it should) at this point,
   there should be little needed for these patches, as just saying "git
   log --first-parent -m" would produce required result. So, mistakes
   above caused a need to fix them.

5. At this point, in the referenced discussion I suggested
   '--diff-merges=on|off' instead of '--[no-]diff-merges', to allow for
   further extensions.

6. '--diff-merges=' option actually born to provide some missing
   functionality and to get rid of inconsistencies[3].

7. '-m' format becomes configurable using new "log.diffMerges"
   configuration[4] so that we can make it conveniently useful even
   without "--first-parent". This was immediately implemented in
   generalized manner to allow to configure '-m' to produce not only
   single-parent diff, but any supported format.

8. As a reply to yet another request on the mailing list "why '-m'
   produces no output", I tried "-m imply -p" patch series[5], which
   were accepted, but then the last patch only(!) from the series, that
   actually introduced required behavior, has been reverted.

   This left me feel I got some unfinished job to do.

9. These patch series, trying "-m imply -p" again, now more carefully.

> So what I've reads so far suggests that "do nothing unless -p" (aka
> --diff-merges=hide) is not actually useful, and we should just drop
> it.

Again, I've tried exactly this before, and that was first accepted, and
then reverted, that's why --diff-merges=hide has been introduced in
these series, to address the issues raised during revert request.

References:

[0]: Showing merges easier with "git log":

https://lore.kernel.org/git/1440110591-12941-1-git-send-email-gitster@pobox.com/

[1]: git log -p -m: Document, honor --first-parent

https://lore.kernel.org/git/20100210011149.GR9553@machine.or.cz/

[2]: making --first-parent imply -m:

https://lore.kernel.org/git/20200728163617.GA2649887@coredump.intra.peff.net/

[3]: git-log: implement new --diff-merge options:

https://lore.kernel.org/git/20201221152000.13134-1-sorganov@gmail.com/

[4]: git log: configurable default format for merge diffs

https://lore.kernel.org/git/20210413114118.25693-1-sorganov@gmail.com/

[5]: diff-merges: let -m imply -p

https://lore.kernel.org/git/20210520214703.27323-1-sorganov@gmail.com/

Thanks,
-- Sergey Organov

Re: [PATCH 0/5] diff-merges: more features

[Glen Choo]

Sergey Organov <sorganov@gmail.com> writes:

> Where do you prefer these references to be put, in the cover letter, in
> the commit message, or in both places?

Wherever it might be relevant as motivation behind the change, so
probably both, but especially the cover letter :)

>
> -- Sergey Organov

Re: [PATCH 0/5] diff-merges: more features

[Sergey Organov]

Glen Choo <chooglen@google.com> writes:

> We covered this series in Review Club, thanks for coming Sergey! For
> those who are interested, the notes are here:
>
>   https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?usp=sharing
>
> and reviewers will send feedback to the list separately anyway. I mostly
> had comments on the design, so I'll leave most of my comments here.
>
> Commenting on the cover letter as a whole, on first read, it wasn't
> obvious to me what this series was trying to achieve because the CL
> presents the 5 patches individually instead of a cohesive story. From my
> understanding, the story is as follows: we want '-m' (enable
> diff-merges) to also imply '-p', but we can't just change the default
> behavior, so we do the following instead:
>
> - Add a config option that gives the behavior that we want (2/5).
> - Deprecate '-m' by giving '--diff-merges=on;hide' as a synonym and
>   encouraging users to use that instead (1,4,5/5).

Well, very close, but not exactly. I'd rather say:

1. Provide exact semantics of '-m' trough --diff-merges UI by
   introducing 'hide' option, after which '--diff-merges=on,hide' and
   '-m' have exactly the same behavior.

2. Add a config option that gives the new behavior we want to '-m': to
   rather be a synonym for '--diff-merges=on[,hide] -p". (Please notice
   that 'hide' is not needed here as it's immediately canceled by '-p'.)

So, essentially, after (1) is there, the config option turns '-m'
meaning from default '--diff-merges=on,hide' to desired
'--diff-merges=on -p'.

Please also notice that at this point we may instead decide to just switch
'-m' meaning to new semantics, either without config at all, or with
config that'd rather restore previous semantics. In fact, the primary
reason why previously such patch has been reverted was absence of (1),
and so with (2) maybe I was just overly cautious.

> Patch 3/5 is completely separate. There was some resistance to it during
> Review Club, but if we still want this, it might be worth splitting off
> into its own series so that we can keep the discussions separate.

OK, I'll cut it off for now.

>
> During the discussion, it also appeared that this "modification of '-m'
> semantics" refers to a patch that changed the default but got reverted
> due to breaking legacy scripts. It would be extremely useful to include
> a link to that previous patch and the discussion around its revert,
> especially given the discussion about whether users actually need
> '-diff-merges=hide' ([1] and elsewhere).

Yep, please see references I've sent in my previous answer to this
message.

>
> [1] https://lore.kernel.org/git/CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com/
>
> Sergey Organov <sorganov@gmail.com> writes:
>
>> 1. --diff-merges=[no]-hide
>>
>> The set of diff-merges options happened to be incomplete, and failed
>> to implement exact semantics of -m option that hides output of diffs
>> for merge commits unless -p option is active as well.
>>
>> The new "hide" option fixes this issue, so that now
>>
>>   --diff-merges=on --diff-merges=hide
>>
>> combo is the exact synonym for -m.
>>
>> The log.diffMerges configuration also accepts "hide" and "no-hide"
>> values, and governs the default value for the hide bit. The
>> configuration behaves as if "--diff-merges=[no-]hide" is inserted
>> first in the command-line.
>
> I had the same concerns as Elijah, which is that this behavior is
> probably clearer as a separate flag (like "--hide-diff-merges"), which
> is more consistent with how '--diff-options' is used today, which means
> that:
>
> a) it is easier to explain to users
> b) the implementation is simpler (I'll comment on Patch 1 code
>    separately)
> c) it makes Patch 4 obsolete

I'd postpone implementation/design discussion till we get to agreement
of the need for this option in the first place.

> But I'm not convinced that we actually want this behavior at all. I
> don't see why a user would use a flag that says "do nothing unless
> other flags are given". don't find the 'alias use case' compelling,
> because the user still has to choose whether to pass '-p', so at that
> point they could just add a different alias.

If one travels back the history, they will find that originally all -m,
-c, and --cc were behaving exactly this way: "do nothing, unless diffs
are actually requested", i.e. they specified only diff format to be used
once requested, and did not request the output themselves. I prefer to
stay on the safe side, and assume that such behavior is still useful,
even though -c/--cc turned to imply '-p' eventually, as doing the same
to '-m' caused so much desire and resistance simultaneously.

OTOH, --diff-merges=<format> does two things: specifies the format and
requests the output for merge commits. It could have been design
mistake, even though it has been discussed at the time of introduction,
but now the only way to get another behavior is to turn off one of the
actions they do: turn off requesting the actual output for merge
commits, and that's what proposed 'hide' means.

> I haven't dug through the code/ML to figure out whether '-m' requiring
> '-p' was an intentional feature or not, but if you could find the old
> thread where you changed the default (and it got reverted), that would
> help the discussion a lot :)

Yep, I gave the links in my first answer to this message.

>
>> 2. log.diffMerges-m-imply-p
>>
>> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'
>> options do. Simply fixing this inconsistency by unconditional
>> modification of '-m' semantics appeared to be a bad idea, as it broke
>> some legacy scripts/aliases. This patch rather provides configuration
>> variable to tweak '-m' behavior accordingly.
>
> I thought that Junio's suggestion to implement a flag that acts like
> '-m' with '-p' [2] was quite a good one (maybe '-M' or
> '--diff-merges=show'), since I think that very few users would actually
> set this config, but the ones that would actually use it can just
> replace '-m' with '-M'.

Introducing new short convenience option(s) is out of scope of these
series, and suggested --diff-merges=show, as I see it, is essentially
"--diff-merges=on -p" that I find hard to explain inside the
'--diff-merges=' context which name suggests it affects merge commits
only.

That said, saying: "we have slightly broken '-m' that we refrain to fix,
so let's introduce '-M' instead of fixing '-m'" does not sound very
convincing either.

>
> [2] https://lore.kernel.org/git/xmqqfse37c0n.fsf@gitster.g/
>
>> 3. log.diffMergesForce
>>
>> Force specific log format for -c, --cc, and --remerge-diff options
>> instead of their respective formats. The override is useful when some
>> external tool hard-codes diff for merges format option.
>
> This might be better off as its own series, since the change isn't
> related to '-m',

Yep, I'm sorry to mix it into the series, -- the only excuse is that it
looked very relevant to me when I did it :)

> but I'm worried about the precedent that this sets.
> To my knowledge, CLI options always overwrite config, but this is the
> opposite. I would prefer not to do this, especially if the use case is
> to work around an external tool (since it is arguably the tool that is
> broken).

The tool was only an initial motivation for the patch. From following a
few discussions on the list I got feeling that every person has their
own preference for --diff-merges format, and rarely wants to see
anything else. This config essentially gives them a way to say "please
use my preferred format everywhere, unless I explicitly say otherwise",
in a centralized manner.

>
>> 5. Issue warning for lone '-m'.
>>
>> Lone '-m' is in use by scripts/aliases that aim at enabling diff
>> output for merge commits, but only if '-p' is then specified as well.
>>
>> As '-m' may now be configured to imply '-p' (using
>> 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,
>> and suggest to instead use '--diff-merges=on,hide' that does not
>> depend on user configuration.
>>
>> This is expected to give a provision for enabling
>> log.diffMerges-m-imply-p by default in the future.
>
> Since '-m' without '-p' is a mistake in most cases, I wonder if we
> should just emit this warning today (maybe via the advise() API). Even
> if we don't keep '--diff-options=hide', deprecating lone '-m' and giving
> a warning seems good to keep.

I'd tend to rather not.

Actually, as far as I'm aware, the only actual use that has been
detected was "--fist-parent -m", and that use case was exactly what you
guys don't find useful: specify default format for merge commits. In
this particular case it is the diff to the first parent, and dates back
to the days before --diff-merges, when using history traversal option
--first-parent was the only way to get diffs with respect to the first
parent for merges.

Maybe we should instead flag the "--first-parent -m" as a warning, as
producing a warning for lone "-m" without these patches is effectively
getting users out of using lone "-m" instead of fixing the latter.

I rather see the bright future as using "-m" all the time, as it's now
extremely configurable.

Thanks,
-- 
Sergey Organov

[PATCH v1 0/5] diff-merges: more features to fix '-m'

[Sergey Organov]

These patch series is yet another attempt to improve '-m' behavior by
first improving --diff-merges to be able to provide current '-m' semantics,
and then introducing a configuration variable to tweak '-m' behavior.

Below is some historic context of the issue, followed by more thorough
description of the patch series.

The last attempt to fix '-m' option for "git log" to imply '-p', to
make it consistent with similar options (-c/--cc), was called by the
request on the mailing list, here:

https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/

However, the suggested (and accepted at first) patch series:

https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t

appeared to have two problems:

* --diff-merges options are incomplete and have no way to provide
  exact existing '-m' semantics.

* implying '-p' by '-m' by default broke some legacy uses of
  "git log --firt-parent -m".

Due to this, the last patch of those patch series has been later
reverted:

https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/

effectively leaving the issue unresolved.

These patch series in turn consists of the following patches:

1. --diff-merges=[no]-hide

The set of diff-merges options happened to be incomplete, and failed
to implement exact semantics of -m option that hides output of diffs
for merge commits unless -p option is active as well.

The new "hide" option fixes this issue, so that now

  --diff-merges=on --diff-merges=hide

combo is the exact synonym for -m.

The log.diffMerges configuration also accepts "hide" and "no-hide"
values, and governs the default value for the hide bit. The
configuration behaves as if "--diff-merges=[no-]hide" is inserted
first in the command-line.

A new log.diffMergesHide configuration is introduced as well, to aid
in avoiding of multiple log.diffMerges configuration entries when both
format and hide flag are to be configured.

2. log.diffMerges-m-imply-p

Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc'
options do. Simply fixing this inconsistency by unconditional
modification of '-m' semantics appeared to be a bad idea, as it broke
some legacy scripts/aliases. This patch rather provides configuration
variable to tweak '-m' behavior accordingly.

3. Support list of values for --diff-merges

This allows for shorter --diff-merges=on,hide forms.

4. Issue warning for lone '-m'.

Lone '-m' is in use by scripts/aliases that aim at enabling diff
output for merge commits, but only if '-p' is then specified as well.

As '-m' may now be configured to imply '-p' (using
'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified,
and suggest to instead use '--diff-merges=on,hide' that does not
depend on user configuration.

This is expected to give a provision for enabling
log.diffMerges-m-imply-p by default in the future.

5. Improve documentation for --diff-merges.

Updates in v1:

  * Added background context to the patch series foreword.

  * Documentation improvements added in a separate commit.

  * Remove commit unrelated to '-m' behavior.

  * Fix style: (p != NULL) -> (p).

  * Fix documentation placement of 'hide' option.

  * Fix log.diffMergesForce tests to avoid dependency on current
    defaults.

Sergey Organov (5):
  diff-merges: implement [no-]hide option and log.diffMergesHide config
  diff-merges: implement log.diffMerges-m-imply-p config
  diff-merges: support list of values for --diff-merges
  diff-merges: issue warning on lone '-m' option
  diff-merges: improve --diff-merges documentation

 Documentation/config/log.txt                  |   9 ++
 Documentation/diff-options.txt                | 104 +++++++++++-------
 builtin/log.c                                 |   4 +
 diff-merges.c                                 |  76 +++++++++++--
 diff-merges.h                                 |   4 +
 t/t4013-diff-various.sh                       |  71 +++++++++++-
 ...ges=first-parent_--diff-merges=hide_master |  34 ++++++
 t/t9902-completion.sh                         |   6 +
 8 files changed, 257 insertions(+), 51 deletions(-)
 create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master

Interdiff against v0:
diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 7452c7fad638..265a57312e58 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -43,17 +43,6 @@ log.diffMergesHide::
 log.diffMerges-m-imply-p::
 	`true` enables implication of `-p` by `-m`.
 
-log.diffMergesForce::
-	Use specified log format for -c, --cc, and --remerge-diff
-	options instead of their respective formats when the option
-	appears on the command line one time. See `--diff-merges` in
-	linkgit:git-log[1] for allowed values. Using 'off' or 'none'
-	disables the override (default).
-+
-The override is useful when external tool hard-codes one of the above
-options. Using any of these options two (or more) times will get back
-the original meaning of the options.
-
 log.follow::
 	If `true`, `git log` will act as if the `--follow` option was used when
 	a single <path> is given.  This has the same limitations as `--follow`,
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 46c98c87e24f..a3fbdb85a8b8 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -34,76 +34,86 @@ endif::git-diff[]
 endif::git-format-patch[]
 
 ifdef::git-log[]
---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r|[no-]hide)::
+-m::
+	Show diffs for merge commits in the default format.
+	Shortcut for '--diff-merges=on,hide' unless
+	`log.diffMerges-m-imply-p` configuration is active, in which
+	case it's a shortcut for '--diff-merges=on -p'.
+
+-c::
+	Shortcut for '--diff-merges=combined -p'.
+
+--cc::
+	Shortcut for '--diff-merges=dense-combined -p'.
+
+--remerge-diff::
+	Shortcut for '--diff-merges=remerge -p'.
+
 --no-diff-merges::
-	Specify diff format to be used for merge commits. Default is
+	Synonym for '--diff-merges=off'.
+
+--diff-merges=(<format>|<flag>)[,(<format>|<flag>),...]::
+	Specify diff format and flags to be used for merge commits. Default is
 	{diff-merges-default} unless `--first-parent` is in use, in which case
-	`first-parent` is the default. Comma-separated list of
-	supported values is accepted as well.
+	`first-parent` is the default.
 +
---diff-merges=(off|none):::
---no-diff-merges:::
+The last format specified has precedence, whereas flags are
+cumulative. Comma-separated list is handy to provide flag(s) along
+with format, e.g.: `--diff-merges=first-parent,hide` is handy form of
+`--diff-merges=first-parent --diff-merges=hide`.
++
+The following formats are supported:
++
+--
+off, none::
 	Disable output of diffs for merge commits. Useful to override
 	implied value.
 +
---diff-merges=on:::
---diff-merges=m:::
--m:::
-	These options make diff output for merge commits to be shown in
-	the default format. The default format could be changed using
+on, m::
+	Make diff output for merge commits to be shown in 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,hide'.
-	In addition it implies `-p` when `log.diffMerges-m-imply-p` is
-	active.
-+
---diff-merges=first-parent:::
---diff-merges=1:::
-	This option makes merge commits show the full diff with
-	respect to the first parent only.
-+
---diff-merges=separate:::
-	This makes merge commits show the full diff with respect to
-	each of the parents. Separate log entry and diff is generated
-	for each parent.
-+
---diff-merges=remerge:::
---diff-merges=r:::
---remerge-diff:::
-	With this option, two-parent merge commits are remerged to
-	create a temporary tree object -- potentially containing files
-	with conflict markers and such.  A diff is then shown between
-	that temporary tree and the actual merge commit.
-+
---diff-merges=hide:::
---diff-merges=no-hide:::
-	Hide (do not hide) the diff for merge commits unless `-p` options is given
-	as well. The default `no-hide` could be changed using `log.diffMerges`
-	configuration parameter.
+first-parent, 1::
+	Show full diff with respect to first parent. This is the same
+	format as `--patch` produces for non-merge commits.
++
+separate::
+	Show full diff with respect to each of parents.
+	Separate log entry and diff is generated for each parent.
++
+remerge, r::
+	Remerge two-parent merge commits to create a temporary tree
+	object--potentially containing files with conflict markers
+	and such.  A diff is then shown between that temporary tree
+	and the actual merge commit.
 +
 The output emitted when this option is used is subject to change, and
 so is its interaction with other options (unless explicitly
 documented).
 +
---diff-merges=combined:::
---diff-merges=c:::
--c:::
-	With this option, diff output for a merge commit shows the
-	differences from each of the parents to the merge result
-	simultaneously instead of showing pairwise diff between a
-	parent and the result one at a time. Furthermore, it lists
-	only files which were modified from all parents. `-c` implies
-	`-p`.
+combined, c::
+	Show differences from each of the parents to the merge
+	result simultaneously instead of showing pairwise diff between
+	a parent and the result one at a time. Furthermore, it lists
+	only files which were modified from all parents.
 +
---diff-merges=dense-combined:::
---diff-merges=cc:::
---cc:::
-	With this option the output produced by
-	`--diff-merges=combined` is further compressed by omitting
-	uninteresting hunks whose contents in the parents have only
-	two variants and the merge result picks one of them without
-	modification.  `--cc` implies `-p`.
+dense-combined, cc::
+	Further compress output produced by `--diff-merges=combined`
+	by omitting uninteresting hunks whose contents in the parents
+	have only two variants and the merge result picks one of them
+	without modification.
+--
++
+The following flags are supported:
++
+--
+[no-]hide::
+	Hide diff for merge commits unless `-p` options is given as
+	well. The default `no-hide` could be changed using
+	`log.diffMerges` configuration parameter.
+--
 
 --combined-all-paths::
 	This flag causes combined diffs (used for merge commits) to
diff --git a/builtin/log.c b/builtin/log.c
index 1e8d0a2545a9..332b5e478cc5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -585,8 +585,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return diff_merges_hide_config(git_config_bool(var, value));
 	if (!strcmp(var, "log.diffmerges-m-imply-p"))
 		return diff_merges_m_imply_p_config(git_config_bool(var, value));
-	if (!strcmp(var, "log.diffmergesforce"))
-		return diff_merges_force_config(value);
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);
 		return 0;
diff --git a/diff-merges.c b/diff-merges.c
index b3b3c9e44ba8..1f4e43e16940 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -7,7 +7,6 @@ typedef void (*diff_merges_setup_func_t)(struct rev_info *);
 static void set_separate(struct rev_info *revs);
 
 static diff_merges_setup_func_t set_to_default = set_separate;
-static diff_merges_setup_func_t force_func = NULL;
 static int suppress_m_parsing;
 static int hide = 0;
 static int m_imply_p = 0;
@@ -166,21 +165,6 @@ int diff_merges_m_imply_p_config(int on)
 	return 0;
 }
 
-int diff_merges_force_config(const char *value)
-{
-	diff_merges_setup_func_t func = func_by_opt(value);
-
-	if (!func)
-		return -1;
-
-	if (func == set_none)
-		force_func = NULL;
-	else if (func != set_hide && func != set_no_hide)
-		force_func = func;
-
-	return 0;
-}
-
 void diff_merges_suppress_m_parsing(void)
 {
 	suppress_m_parsing = 1;
@@ -191,7 +175,6 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 	int argcount = 1;
 	const char *optarg;
 	const char *arg = argv[0];
-	diff_merges_setup_func_t set_func = NULL;
 
 	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
 		set_to_default(revs);
@@ -199,11 +182,14 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 		revs->merges_imply_patch = m_imply_p;
 		got_m = 1;
 	} else if (!strcmp(arg, "-c")) {
-		set_func = set_combined;
+		set_combined(revs);
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--cc")) {
-		set_func = set_dense_combined;
+		set_dense_combined(revs);
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--remerge-diff")) {
-		set_func = set_remerge_diff;
+		set_remerge_diff(revs);
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--no-diff-merges")) {
 		set_none(revs);
 	} else if (!strcmp(arg, "--combined-all-paths")) {
@@ -213,12 +199,6 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 	} else
 		return 0;
 
-	if (set_func != NULL) {
-		(force_func ? force_func : set_func)(revs);
-		force_func = NULL;
-		revs->merges_imply_patch = 1;
-	}
-
 	revs->explicit_diff_merges = 1;
 	return argcount;
 }
diff --git a/diff-merges.h b/diff-merges.h
index 6ef0cc87bb2a..9f0b3901fe82 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -15,8 +15,6 @@ int diff_merges_hide_config(int hide);
 
 int diff_merges_m_imply_p_config(int on);
 
-int diff_merges_force_config(const char *value);
-
 void diff_merges_suppress_m_parsing(void);
 
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 2c68d06074ed..a07513e67fd6 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -565,24 +565,6 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '
 	test_cmp expected actual
 '
 
-test_expect_success 'git config log.diffMergesForce has proper effect' '
-	git log -m -p master >result &&
-	process_diffs result >expected &&
-	test_config log.diffMergesForce on &&
-	git log --cc master >result &&
-	process_diffs result >actual &&
-	test_cmp expected actual
-'
-
-test_expect_success 'git config log.diffMergesForce override by duplicate' '
-	git log --cc master >result &&
-	process_diffs result >expected &&
-	test_config log.diffMergesForce on &&
-	git log --cc --cc master >result &&
-	process_diffs result >actual &&
-	test_cmp expected actual
-'
-
 # -m in "git diff-index" means "match missing", that differs
 # from its meaning in "git diff". Let's check it in diff-index.
 # The line in the output for removed file should disappear when
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 411e08b2fa1b..26a7e4ff877c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2498,7 +2498,6 @@ test_expect_success 'git config - variable name' '
 	log.decorate Z
 	log.diffMerges Z
 	log.diffMergesHide Z
-	log.diffMergesForce Z
 	log.diffMerges-m-imply-p Z
 	EOF
 '
@@ -2529,7 +2528,6 @@ test_expect_success 'git -c - variable name' '
 	log.decorate=Z
 	log.diffMerges=Z
 	log.diffMergesHide=Z
-	log.diffMergesForce=Z
 	log.diffMerges-m-imply-p=Z
 	EOF
 '
@@ -2554,7 +2552,6 @@ test_expect_success 'git clone --config= - variable name' '
 	log.decorate=Z
 	log.diffMerges=Z
 	log.diffMergesHide=Z
-	log.diffMergesForce=Z
 	log.diffMerges-m-imply-p=Z
 	EOF
 '
-- 
2.25.1

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

[Sergey Organov]

The set of diff-merges options happened to be incomplete, and failed
to implement exact semantics of -m option that hides output of diffs
for merge commits unless -p option is active as well.

The new "hide" option fixes this issue, so that now

  --diff-merges=on --diff-merges=hide

combo is the exact synonym for -m.

The log.diffMerges configuration also accepts "hide" and "no-hide"
values, and governs the default value for the hide bit. The
configuration behaves as if "--diff-merges=[no-]hide" is inserted
first in the command-line.

New log.diffMergesHide boolean configuration is implemented as well,
for the case when log.diffMerges is already in use for specifying the
format.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/config/log.txt                  |  6 +++
 Documentation/diff-options.txt                | 13 +++--
 builtin/log.c                                 |  2 +
 diff-merges.c                                 | 40 ++++++++++++--
 diff-merges.h                                 |  2 +
 t/t4013-diff-various.sh                       | 54 ++++++++++++++++++-
 ...ges=first-parent_--diff-merges=hide_master | 34 ++++++++++++
 t/t9902-completion.sh                         |  3 ++
 8 files changed, 147 insertions(+), 7 deletions(-)
 create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index cbe34d759221..9c6be643bcf6 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -33,6 +33,12 @@ log.diffMerges::
 	Set diff format to be used when `--diff-merges=on` is
 	specified, see `--diff-merges` in linkgit:git-log[1] for
 	details. Defaults to `separate`.
++
+When used with 'hide' or 'no-hide', sets the default hiding of
+diffs for merge commits when `-p` option is not used.
+
+log.diffMergesHide::
+	`true` implies `--diff-merges=hide` option.
 
 log.follow::
 	If `true`, `git log` will act as if the `--follow` option was used when
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 3674ac48e92c..b062bdab04d9 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -34,7 +34,7 @@ endif::git-diff[]
 endif::git-format-patch[]
 
 ifdef::git-log[]
---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r)::
+--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r|[no-]hide)::
 --no-diff-merges::
 	Specify diff format to be used for merge commits. Default is
 	{diff-merges-default} unless `--first-parent` is in use, in which case
@@ -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'
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
@@ -94,6 +95,12 @@ documented).
 	uninteresting hunks whose contents in the parents have only
 	two variants and the merge result picks one of them without
 	modification.  `--cc` implies `-p`.
++
+--diff-merges=hide:::
+--diff-merges=no-hide:::
+	Hide (do not hide) the diff for merge commits unless `-p` options is given
+	as well. The default `no-hide` could be changed using `log.diffMerges`
+	configuration parameter.
 
 --combined-all-paths::
 	This flag causes combined diffs (used for merge commits) to
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;
diff --git a/diff-merges.c b/diff-merges.c
index 85cbefa5afd7..55fe5b70c102 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -7,6 +7,7 @@ static void set_separate(struct rev_info *revs);
 
 static diff_merges_setup_func_t set_to_default = set_separate;
 static int suppress_m_parsing;
+static int hide = 0;
 
 static void suppress(struct rev_info *revs)
 {
@@ -20,10 +21,15 @@ static void suppress(struct rev_info *revs)
 	revs->remerge_diff = 0;
 }
 
+static void set_need_diff(struct rev_info *revs)
+{
+	revs->merges_need_diff = !hide;
+}
+
 static void common_setup(struct rev_info *revs)
 {
 	suppress(revs);
-	revs->merges_need_diff = 1;
+	set_need_diff(revs);
 }
 
 static void set_none(struct rev_info *revs)
@@ -31,6 +37,18 @@ static void set_none(struct rev_info *revs)
 	suppress(revs);
 }
 
+static void set_hide(struct rev_info *revs)
+{
+	hide = 1;
+	set_need_diff(revs);
+}
+
+static void set_no_hide(struct rev_info *revs)
+{
+	hide = 0;
+	set_need_diff(revs);
+}
+
 static void set_separate(struct rev_info *revs)
 {
 	common_setup(revs);
@@ -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;
+}
+
+int diff_merges_hide_config(int on)
+{
+	hide = on;
 	return 0;
 }
 
@@ -122,7 +156,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
 		set_to_default(revs);
-		revs->merges_need_diff = 0;
+		set_hide(revs);
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/diff-merges.h b/diff-merges.h
index 19639689bb05..e86e5381693b 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -11,6 +11,8 @@ struct rev_info;
 
 int diff_merges_config(const char *value);
 
+int diff_merges_hide_config(int hide);
+
 void diff_merges_suppress_m_parsing(void);
 
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index dfcf3a0aaae3..4fc8ba2fc59c 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -334,6 +334,7 @@ log --first-parent --diff-merges=off -p master
 log -p --first-parent master
 log -p --diff-merges=first-parent master
 log --diff-merges=first-parent master
+log --diff-merges=first-parent --diff-merges=hide master
 log -m -p --first-parent master
 log -m -p master
 log --cc -m -p master
@@ -460,7 +461,18 @@ EOF
 test_expect_success 'log -m matches pure log' '
 	git log master >result &&
 	process_diffs result >expected &&
-	git log -m >result &&
+	git log -m master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'log --diff-merges=on matches -m only with --diff-merges=hide' '
+	git log -m master >result &&
+	process_diffs result >expected &&
+	git log --diff-merges=on master >result &&
+	process_diffs result >actual &&
+	! test_cmp expected actual &&
+	git log --diff-merges=on --diff-merges=hide master >result &&
 	process_diffs result >actual &&
 	test_cmp expected actual
 '
@@ -496,6 +508,46 @@ test_expect_success 'git config log.diffMerges first-parent vs -m' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git config log.diffMerges hide: has effect' '
+	git log --diff-merges=on master >result &&
+	process_diffs result >no-hide &&
+	test_config log.diffMerges hide &&
+	git log --diff-merges=on master >result &&
+	process_diffs result >hide &&
+	! test_cmp no-hide hide
+'
+
+test_expect_success 'git config log.diffMerges no-hide: is the default' '
+	git log --diff-merges=on master >result &&
+	process_diffs result >default &&
+	test_config log.diffMerges no-hide &&
+	git log --diff-merges=on master >result &&
+	process_diffs result >no-hide &&
+	test_cmp default no-hide
+'
+
+# As "-m" is synonym for "--diff-merges=hide --diff-merges=on", the
+# "log.diffMerges=hide" configuration should have no effect on "-m"
+test_expect_success 'git config log.diffMerges hide: has no effect on -m' '
+	git log -m master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges hide &&
+	git log -m master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
+# As "--cc" implies "-p", the "log.diffMerges=hide" configuration
+# should have no effect on "--cc"
+test_expect_success 'git config log.diffMerges hide: has no effect on --cc' '
+	git log --cc master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges hide &&
+	git log --cc master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 # -m in "git diff-index" means "match missing", that differs
 # from its meaning in "git diff". Let's check it in diff-index.
 # The line in the output for removed file should disappear when
diff --git a/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master b/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master
new file mode 100644
index 000000000000..596caec64298
--- /dev/null
+++ b/t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master
@@ -0,0 +1,34 @@
+$ git log --diff-merges=first-parent --diff-merges=hide master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side'
+
+commit c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:03:00 2006 +0000
+
+    Side
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 43de868b8005..fc6b0722216a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2497,6 +2497,7 @@ test_expect_success 'git config - variable name' '
 	log.date Z
 	log.decorate Z
 	log.diffMerges Z
+	log.diffMergesHide Z
 	EOF
 '
 
@@ -2525,6 +2526,7 @@ test_expect_success 'git -c - variable name' '
 	log.date=Z
 	log.decorate=Z
 	log.diffMerges=Z
+	log.diffMergesHide=Z
 	EOF
 '
 
@@ -2547,6 +2549,7 @@ test_expect_success 'git clone --config= - variable name' '
 	log.date=Z
 	log.decorate=Z
 	log.diffMerges=Z
+	log.diffMergesHide=Z
 	EOF
 '
 
-- 
2.25.1

[PATCH v1 2/5] diff-merges: implement log.diffMerges-m-imply-p config

[Sergey Organov]

Historically, `-m` doesn't imply `-p` whereas similar `-c` and `--cc`
options do. Simply fixing this inconsistency by unconditional
modification of `-m` semantics appeared to be a bad idea, as it broke
some legacy scripts.

Implement "log.diffMerges-m-imply-p" boolean configuration variable
that allows user to enable implication of `-p` by `-m`.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/config/log.txt   | 3 +++
 Documentation/diff-options.txt | 6 ++++--
 builtin/log.c                  | 2 ++
 diff-merges.c                  | 8 ++++++++
 diff-merges.h                  | 2 ++
 t/t4013-diff-various.sh        | 9 +++++++++
 t/t9902-completion.sh          | 3 +++
 7 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 9c6be643bcf6..265a57312e58 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -40,6 +40,9 @@ diffs for merge commits when `-p` option is not used.
 log.diffMergesHide::
 	`true` implies `--diff-merges=hide` option.
 
+log.diffMerges-m-imply-p::
+	`true` enables implication of `-p` by `-m`.
+
 log.follow::
 	If `true`, `git log` will act as if the `--follow` option was used when
 	a single <path> is given.  This has the same limitations as `--follow`,
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index b062bdab04d9..fe15693492a2 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -48,12 +48,14 @@ ifdef::git-log[]
 --diff-merges=on:::
 --diff-merges=m:::
 -m:::
-	This option makes diff output for merge commits to be shown in
+	These options make diff output for merge commits to be shown in
 	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'
+	`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'.
+	In addition it implies `-p` when `log.diffMerges-m-imply-p` is
+	active.
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
diff --git a/builtin/log.c b/builtin/log.c
index e031021e53b2..332b5e478cc5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -583,6 +583,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		return diff_merges_config(value);
 	if (!strcmp(var, "log.diffmergeshide"))
 		return diff_merges_hide_config(git_config_bool(var, value));
+	if (!strcmp(var, "log.diffmerges-m-imply-p"))
+		return diff_merges_m_imply_p_config(git_config_bool(var, value));
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);
 		return 0;
diff --git a/diff-merges.c b/diff-merges.c
index 55fe5b70c102..1fbf476d378e 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -8,6 +8,7 @@ static void set_separate(struct rev_info *revs);
 static diff_merges_setup_func_t set_to_default = set_separate;
 static int suppress_m_parsing;
 static int hide = 0;
+static int m_imply_p = 0;
 
 static void suppress(struct rev_info *revs)
 {
@@ -143,6 +144,12 @@ int diff_merges_hide_config(int on)
 	return 0;
 }
 
+int diff_merges_m_imply_p_config(int on)
+{
+	m_imply_p = on;
+	return 0;
+}
+
 void diff_merges_suppress_m_parsing(void)
 {
 	suppress_m_parsing = 1;
@@ -157,6 +164,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
 		set_to_default(revs);
 		set_hide(revs);
+		revs->merges_imply_patch = m_imply_p;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/diff-merges.h b/diff-merges.h
index e86e5381693b..9f0b3901fe82 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -13,6 +13,8 @@ int diff_merges_config(const char *value);
 
 int diff_merges_hide_config(int hide);
 
+int diff_merges_m_imply_p_config(int on);
+
 void diff_merges_suppress_m_parsing(void);
 
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 4fc8ba2fc59c..1789dd6063c5 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -548,6 +548,15 @@ test_expect_success 'git config log.diffMerges hide: has no effect on --cc' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' '
+	git log -m -p master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges-m-imply-p true &&
+	git log -m master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 # -m in "git diff-index" means "match missing", that differs
 # from its meaning in "git diff". Let's check it in diff-index.
 # The line in the output for removed file should disappear when
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index fc6b0722216a..26a7e4ff877c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2498,6 +2498,7 @@ test_expect_success 'git config - variable name' '
 	log.decorate Z
 	log.diffMerges Z
 	log.diffMergesHide Z
+	log.diffMerges-m-imply-p Z
 	EOF
 '
 
@@ -2527,6 +2528,7 @@ test_expect_success 'git -c - variable name' '
 	log.decorate=Z
 	log.diffMerges=Z
 	log.diffMergesHide=Z
+	log.diffMerges-m-imply-p=Z
 	EOF
 '
 
@@ -2550,6 +2552,7 @@ test_expect_success 'git clone --config= - variable name' '
 	log.decorate=Z
 	log.diffMerges=Z
 	log.diffMergesHide=Z
+	log.diffMerges-m-imply-p=Z
 	EOF
 '
 
-- 
2.25.1

[PATCH v1 3/5] diff-merges: support list of values for --diff-merges

[Sergey Organov]

Support comma-separated list of options in --diff-merges=. This allows
for shorter:

  --diff-merges=on,hide

instead of:

  --diff-merges=on --diff-merges=hide

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/diff-options.txt |  5 +++--
 diff-merges.c                  | 22 ++++++++++++++++++----
 t/t4013-diff-various.sh        |  8 ++++++++
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index fe15693492a2..977f9135b0d6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -38,7 +38,8 @@ ifdef::git-log[]
 --no-diff-merges::
 	Specify diff format to be used for merge commits. Default is
 	{diff-merges-default} unless `--first-parent` is in use, in which case
-	`first-parent` is the default.
+	`first-parent` is the default. Comma-separated list of
+	supported values is accepted as well.
 +
 --diff-merges=(off|none):::
 --no-diff-merges:::
@@ -53,7 +54,7 @@ ifdef::git-log[]
 	`log.diffMerges` configuration parameter, which default value
 	is `separate`.
 +
-	`-m` is a shortcut for '--diff-merges=on --diff-merges=hide'.
+	`-m` is a shortcut for '--diff-merges=on,hide'.
 	In addition it implies `-p` when `log.diffMerges-m-imply-p` is
 	active.
 +
diff --git a/diff-merges.c b/diff-merges.c
index 1fbf476d378e..bb2797ff8cc5 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -1,6 +1,7 @@
 #include "diff-merges.h"
 
 #include "revision.h"
+#include "strbuf.h"
 
 typedef void (*diff_merges_setup_func_t)(struct rev_info *);
 static void set_separate(struct rev_info *revs);
@@ -109,12 +110,25 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 
 static void set_diff_merges(struct rev_info *revs, const char *optarg)
 {
-	diff_merges_setup_func_t func = func_by_opt(optarg);
+	char const delim = ',';
+	struct strbuf **opts = strbuf_split_str(optarg, delim, -1);
+	struct strbuf **p;
 
-	if (!func)
-		die(_("invalid value for '%s': '%s'"), "--diff-merges", optarg);
+	for (p = opts; *p; p++) {
+		diff_merges_setup_func_t func;
+		char *opt = (*p)->buf;
+		int len = (*p)->len;
 
-	func(revs);
+		if (opt[len - 1] == delim)
+			opt[len - 1] = '\0';
+		func = func_by_opt(opt);
+		if (!func) {
+			strbuf_list_free(opts);
+			die(_("invalid value for '%s': '%s'"), "--diff-merges", opt);
+		}
+		func(revs);
+	}
+	strbuf_list_free(opts);
 }
 
 /*
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 1789dd6063c5..a07513e67fd6 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -485,6 +485,14 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
 	test_cmp expected actual
 '
 
+test_expect_success 'log --diff-merges=<V1>,<V2>' '
+	git log --diff-merges=1,hide master >result &&
+	process_diffs result >expected &&
+	git log --diff-merges=1 --diff-merges=hide master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'deny wrong log.diffMerges config' '
 	test_config log.diffMerges wrong-value &&
 	test_expect_code 128 git log
-- 
2.25.1

[PATCH v1 4/5] diff-merges: issue warning on lone '-m' option

[Sergey Organov]

Lone '-m' is in use by scripts/aliases that aim at enabling diff
output for merge commits, but only if '-p' is then specified as well.

As '-m' may now be configured to imply '-p', using
'log.diffMerges-m-imply-p', issue warning and suggest to instead use

  --diff-merges=on,hide

that does not depend on user configuration.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/diff-merges.c b/diff-merges.c
index bb2797ff8cc5..1f4e43e16940 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -10,6 +10,7 @@ static diff_merges_setup_func_t set_to_default = set_separate;
 static int suppress_m_parsing;
 static int hide = 0;
 static int m_imply_p = 0;
+static int got_m = 0;
 
 static void suppress(struct rev_info *revs)
 {
@@ -179,6 +180,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 		set_to_default(revs);
 		set_hide(revs);
 		revs->merges_imply_patch = m_imply_p;
+		got_m = 1;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
@@ -239,5 +241,7 @@ void diff_merges_setup_revs(struct rev_info *revs)
 	if (revs->merges_imply_patch || revs->merges_need_diff) {
 		if (!revs->diffopt.output_format)
 			revs->diffopt.output_format = DIFF_FORMAT_PATCH;
-	}
+	} else if (got_m)
+		warning(_("legacy use of lone '-m' detected: please use '--diff-merges=on,hide' instead, as '-m' may imply '-p'"));
+
 }
-- 
2.25.1

[PATCH v1 5/5] diff-merges: improve --diff-merges documentation

[Sergey Organov]

As we've now added a flag to --diff-merges, and so there are now flags
and formats, document them as such.

Another improvement is that now there is no extremely long line
containg all the --diff-merges options.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/diff-options.txt | 114 ++++++++++++++++++---------------
 1 file changed, 62 insertions(+), 52 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 977f9135b0d6..a3fbdb85a8b8 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -34,76 +34,86 @@ endif::git-diff[]
 endif::git-format-patch[]
 
 ifdef::git-log[]
---diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc|remerge|r|[no-]hide)::
+-m::
+	Show diffs for merge commits in the default format.
+	Shortcut for '--diff-merges=on,hide' unless
+	`log.diffMerges-m-imply-p` configuration is active, in which
+	case it's a shortcut for '--diff-merges=on -p'.
+
+-c::
+	Shortcut for '--diff-merges=combined -p'.
+
+--cc::
+	Shortcut for '--diff-merges=dense-combined -p'.
+
+--remerge-diff::
+	Shortcut for '--diff-merges=remerge -p'.
+
 --no-diff-merges::
-	Specify diff format to be used for merge commits. Default is
+	Synonym for '--diff-merges=off'.
+
+--diff-merges=(<format>|<flag>)[,(<format>|<flag>),...]::
+	Specify diff format and flags to be used for merge commits. Default is
 	{diff-merges-default} unless `--first-parent` is in use, in which case
-	`first-parent` is the default. Comma-separated list of
-	supported values is accepted as well.
+	`first-parent` is the default.
 +
---diff-merges=(off|none):::
---no-diff-merges:::
+The last format specified has precedence, whereas flags are
+cumulative. Comma-separated list is handy to provide flag(s) along
+with format, e.g.: `--diff-merges=first-parent,hide` is handy form of
+`--diff-merges=first-parent --diff-merges=hide`.
++
+The following formats are supported:
++
+--
+off, none::
 	Disable output of diffs for merge commits. Useful to override
 	implied value.
 +
---diff-merges=on:::
---diff-merges=m:::
--m:::
-	These options make diff output for merge commits to be shown in
-	the default format. The default format could be changed using
+on, m::
+	Make diff output for merge commits to be shown in 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,hide'.
-	In addition it implies `-p` when `log.diffMerges-m-imply-p` is
-	active.
+first-parent, 1::
+	Show full diff with respect to first parent. This is the same
+	format as `--patch` produces for non-merge commits.
 +
---diff-merges=first-parent:::
---diff-merges=1:::
-	This option makes merge commits show the full diff with
-	respect to the first parent only.
+separate::
+	Show full diff with respect to each of parents.
+	Separate log entry and diff is generated for each parent.
 +
---diff-merges=separate:::
-	This makes merge commits show the full diff with respect to
-	each of the parents. Separate log entry and diff is generated
-	for each parent.
-+
---diff-merges=remerge:::
---diff-merges=r:::
---remerge-diff:::
-	With this option, two-parent merge commits are remerged to
-	create a temporary tree object -- potentially containing files
-	with conflict markers and such.  A diff is then shown between
-	that temporary tree and the actual merge commit.
+remerge, r::
+	Remerge two-parent merge commits to create a temporary tree
+	object--potentially containing files with conflict markers
+	and such.  A diff is then shown between that temporary tree
+	and the actual merge commit.
 +
 The output emitted when this option is used is subject to change, and
 so is its interaction with other options (unless explicitly
 documented).
 +
---diff-merges=combined:::
---diff-merges=c:::
--c:::
-	With this option, diff output for a merge commit shows the
-	differences from each of the parents to the merge result
-	simultaneously instead of showing pairwise diff between a
-	parent and the result one at a time. Furthermore, it lists
-	only files which were modified from all parents. `-c` implies
-	`-p`.
+combined, c::
+	Show differences from each of the parents to the merge
+	result simultaneously instead of showing pairwise diff between
+	a parent and the result one at a time. Furthermore, it lists
+	only files which were modified from all parents.
 +
---diff-merges=dense-combined:::
---diff-merges=cc:::
---cc:::
-	With this option the output produced by
-	`--diff-merges=combined` is further compressed by omitting
-	uninteresting hunks whose contents in the parents have only
-	two variants and the merge result picks one of them without
-	modification.  `--cc` implies `-p`.
+dense-combined, cc::
+	Further compress output produced by `--diff-merges=combined`
+	by omitting uninteresting hunks whose contents in the parents
+	have only two variants and the merge result picks one of them
+	without modification.
+--
 +
---diff-merges=hide:::
---diff-merges=no-hide:::
-	Hide (do not hide) the diff for merge commits unless `-p` options is given
-	as well. The default `no-hide` could be changed using `log.diffMerges`
-	configuration parameter.
+The following flags are supported:
++
+--
+[no-]hide::
+	Hide diff for merge commits unless `-p` options is given as
+	well. The default `no-hide` could be changed using
+	`log.diffMerges` configuration parameter.
+--
 
 --combined-all-paths::
 	This flag causes combined diffs (used for merge commits) to
-- 
2.25.1

Re: [PATCH v1 0/5] diff-merges: more features to fix '-m'

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> The last attempt to fix '-m' option for "git log" to imply '-p', to
> make it consistent with similar options (-c/--cc), was called by the
> request on the mailing list, here:

In retrospect, this old attempt may probably shouldn't have been
done, as there wasn't really a compelling need to change the
behaviour of "-m".  The "combined diff" options were "if specified,
give output" from day one, unlike "-m" which was "modify the
behaviour of '-p' if given" for a long time.  Changing any
established behaviour risks breaking the exising users and the
upside must outweigh the risk.  There wasn't overwhelming upside
back then to risk, and of course it backfired, ...

> However, the suggested (and accepted at first) patch series:
>
> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t
>
> appeared to have two problems:
>
> * --diff-merges options are incomplete and have no way to provide
>   exact existing '-m' semantics.
>
> * implying '-p' by '-m' by default broke some legacy uses of
>   "git log --firt-parent -m".

... like so.  Without learning from the experience, we may repeat
doing the same thing over and over and expecting different outcome,
but it would not give us a very good end-user experience if it
breaks them every time we try doing that.

> Due to this, the last patch of those patch series has been later
> reverted:
>
> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/
>
> effectively leaving the issue unresolved.

Fairly accurate description.

These patches do look like a good approach to solve the first point
among the "two problems" in the previous round. Thanks for working
on it.

IIRC, the previous round (why is this round marked as v1, by the
way?) was reviewed by some folks, so lets wait to hear from them
how this round does better.

Unfortunately, I do not think of any "solution" that would avoid
breaking folks, if its end goal is to flip the default, either by
hardcoding or with a configuration variable.  IOW, the other one
among the "two problems" in the previous round sounds unsolvable.
We should question if it was really an "issue" worth "resolving",
though.

Re: [PATCH v1 0/5] diff-merges: more features to fix '-m'

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> The last attempt to fix '-m' option for "git log" to imply '-p', to
>> make it consistent with similar options (-c/--cc), was called by the
>> request on the mailing list, here:
>
> In retrospect, this old attempt may probably shouldn't have been
> done, as there wasn't really a compelling need to change the
> behaviour of "-m".  The "combined diff" options were "if specified,
> give output" from day one, unlike "-m" which was "modify the
> behaviour of '-p' if given" for a long time.

No, I think your memory played trick on you, at least for --cc:

https://lore.kernel.org/git/1440110591-12941-3-git-send-email-gitster@pobox.com/

Please also see the history of -m, -c, --cc that I've tried to reproduce
from the repo and mailing list, here:

https://lore.kernel.org/git/87ilijt2c3.fsf@osv.gnss.ru/

Thanks,
-- Sergey Organov

Re: [PATCH v1 0/5] diff-merges: more features to fix '-m'

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> > Sergey Organov <sorganov@gmail.com> writes:
> > * implying '-p' by '-m' by default broke some legacy uses of
> >   "git log --firt-parent -m".
> 
> ... like so.  Without learning from the experience, we may repeat
> doing the same thing over and over and expecting different outcome,
> but it would not give us a very good end-user experience if it
> breaks them every time we try doing that.

The experience tells that this form was likely the only useful form of
using -m, and then it has been obsoleted for more than two years
already, since --first-parent started to imply -m:

https://lore.kernel.org/git/20200728163617.GA2649887@coredump.intra.peff.net/

[...]

> IIRC, the previous round (why is this round marked as v1, by the
> way?)

Well, everybody knows indexing always starts at 0, right? ;-)

Now, "git format-patch" has the --reroll-count=N, and as that was the
first re-roll, I've used "1" for that. Is this my mistake?

> was reviewed by some folks, so lets wait to hear from them
> how this round does better.

Yes, correct.

>
> Unfortunately, I do not think of any "solution" that would avoid
> breaking folks, if its end goal is to flip the default, either by
> hardcoding or with a configuration variable.  IOW, the other one
> among the "two problems" in the previous round sounds unsolvable.
> We should question if it was really an "issue" worth "resolving",
> though.

Dunno, but similar issue with --cc apparently was worth resolving when
you've changed its behavior back in 2015:

https://lore.kernel.org/git/1440110591-12941-3-git-send-email-gitster@pobox.com/

The '-m' not behaving as people expect is an issue that I'm trying to
solve.

Thanks,
-- Sergey Organov