[PATCH 0/9] git log: configurable default format for merge diffs

[PATCH 0/9] git log: configurable default format for merge diffs

[Sergey Organov]

These patches introduce capability to configure the default format of
output of diffs for merge commits by means of new log.diffMerges
configuration variable. The default format is then used by -m,
--diff-merges=m, and new --diff-merges=def options.

In particular,

  git config log.diffMerges first-parent

will change -m option format from "separate" to "first-parent" that
will in turn cause, say,

  git show -m <merge_commit>

to output diff to the first parent only, instead of appending
typically large and surprising diff to the second parent at the end of
the output.

Sergey Organov (9):
  diff-merges: introduce --diff-merges=def
  diff-merges: refactor set_diff_merges()
  diff-merges: introduce log.diffMerges config variable
  diff-merges: adapt -m to enable default diff format
  t4013: add test for --diff-merges=def
  t4013: add tests for log.diffMerges config
  t9902: fix completion tests for log.d* to match log.diffMerges
  doc/diff-options: document new --diff-merges features
  doc/config: document log.diffMerges

 Documentation/config/log.txt   |  5 +++
 Documentation/diff-options.txt | 15 ++++++---
 builtin/log.c                  |  2 ++
 diff-merges.c                  | 58 ++++++++++++++++++++++++----------
 diff-merges.h                  |  2 ++
 t/t4013-diff-various.sh        | 34 ++++++++++++++++++++
 t/t9902-completion.sh          |  3 ++
 7 files changed, 98 insertions(+), 21 deletions(-)

-- 
2.25.1

[PATCH 1/9] diff-merges: introduce --diff-merges=def

[Sergey Organov]

Introduce the notion of default diff format for merges, and the option
"def" to select it. The default is "separate" and can't yet be
changed, so effectively "dev" is just a synonym for "separate" for
now.

This is in preparation for introducing log.diffMerges configuration
option that will let --diff-merges=def to be configured to any
supported format.

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

diff --git a/diff-merges.c b/diff-merges.c
index 146bb50316a6..0887a07cfc67 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -2,6 +2,8 @@
 
 #include "revision.h"
 
+typedef void (*diff_merges_setup_func_t)(struct rev_info *);
+
 static void suppress(struct rev_info *revs)
 {
 	revs->separate_merges = 0;
@@ -19,6 +21,8 @@ static void set_separate(struct rev_info *revs)
 	revs->separate_merges = 1;
 }
 
+static diff_merges_setup_func_t set_to_default = set_separate;
+
 static void set_first_parent(struct rev_info *revs)
 {
 	set_separate(revs);
@@ -66,6 +70,8 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
 		set_combined(revs);
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		set_dense_combined(revs);
+	else if (!strcmp(optarg, "def"))
+		set_to_default(revs);
 	else
 		die(_("unknown value for --diff-merges: %s"), optarg);
 
-- 
2.25.1

Re: [PATCH 1/9] diff-merges: introduce --diff-merges=def

[Philip Oakley]

Hi,

On 07/04/2021 23:56, Sergey Organov wrote:
> Introduce the notion of default diff format for merges, and the option
> "def" to select it. The default is "separate" and can't yet be
"def" feels a bit too short and sounds similar to "define" - why not
spell out in full?
> changed, so effectively "dev" is just a synonym for "separate" for
did you mean "def"?  i.e. s/dev/def/   (..spell out in full ;-)

--
Philip
> now.
>
> This is in preparation for introducing log.diffMerges configuration
> option that will let --diff-merges=def to be configured to any
> supported format.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  diff-merges.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/diff-merges.c b/diff-merges.c
> index 146bb50316a6..0887a07cfc67 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -2,6 +2,8 @@
>  
>  #include "revision.h"
>  
> +typedef void (*diff_merges_setup_func_t)(struct rev_info *);
> +
>  static void suppress(struct rev_info *revs)
>  {
>  	revs->separate_merges = 0;
> @@ -19,6 +21,8 @@ static void set_separate(struct rev_info *revs)
>  	revs->separate_merges = 1;
>  }
>  
> +static diff_merges_setup_func_t set_to_default = set_separate;
> +
>  static void set_first_parent(struct rev_info *revs)
>  {
>  	set_separate(revs);
> @@ -66,6 +70,8 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
>  		set_combined(revs);
>  	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
>  		set_dense_combined(revs);
> +	else if (!strcmp(optarg, "def"))
> +		set_to_default(revs);
>  	else
>  		die(_("unknown value for --diff-merges: %s"), optarg);
>  

Re: [PATCH 1/9] diff-merges: introduce --diff-merges=def

[Sergey Organov]

Hi,

Philip Oakley <philipoakley@iee.email> writes:
> Hi,
>
> On 07/04/2021 23:56, Sergey Organov wrote:
>> Introduce the notion of default diff format for merges, and the option
>> "def" to select it. The default is "separate" and can't yet be
> "def" feels a bit too short and sounds similar to "define" - why not
> spell out in full?

Dunno, it just happened. No sound reason. Will change to "default" for
the next re-roll.

>> changed, so effectively "dev" is just a synonym for "separate" for
> did you mean "def"?  i.e. s/dev/def/   (..spell out in full ;-)

Yep, thanks!

-- Sergey

Re: [PATCH 1/9] diff-merges: introduce --diff-merges=def

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Hi,
>
> Philip Oakley <philipoakley@iee.email> writes:
>> Hi,
>>
>> On 07/04/2021 23:56, Sergey Organov wrote:
>>> Introduce the notion of default diff format for merges, and the option
>>> "def" to select it. The default is "separate" and can't yet be
>> "def" feels a bit too short and sounds similar to "define" - why not
>> spell out in full?
>
> Dunno, it just happened. No sound reason. Will change to "default" for
> the next re-roll.

I do not immediately see the point of writing --diff-merges=default
on the command line in the first place.  If what it calls for is the
default, wouldn't it be easier to just leave it out?

But if we have to have it as one of the choice, please do not invent
such an abbreviation, especially without taking the fully-spelled
form.

Thanks.

Re: [PATCH 1/9] diff-merges: introduce --diff-merges=def

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Hi,
>>
>> Philip Oakley <philipoakley@iee.email> writes:
>>> Hi,
>>>
>>> On 07/04/2021 23:56, Sergey Organov wrote:
>>>> Introduce the notion of default diff format for merges, and the option
>>>> "def" to select it. The default is "separate" and can't yet be
>>> "def" feels a bit too short and sounds similar to "define" - why not
>>> spell out in full?
>>
>> Dunno, it just happened. No sound reason. Will change to "default" for
>> the next re-roll.
>
> I do not immediately see the point of writing --diff-merges=default
> on the command line in the first place.  If what it calls for is the
> default, wouldn't it be easier to just leave it out?

It does enable output of diffs for merge commits, so it's not the same
as leaving it out. The "default" is the exact format it will use for the
output.

Or do you mean using bare "--diff-merges", without "=value"? It is
considered bad practice, right?

>
> But if we have to have it as one of the choice, please do not invent
> such an abbreviation, especially without taking the fully-spelled
> form.

I think we have to, see above, and yes, I'll turn it to the full form.

Thanks,

-- Sergey Organov

[PATCH 2/9] diff-merges: refactor set_diff_merges()

[Sergey Organov]

Split set_diff_merges() into separate parsing and execution functions,
the former to be reused later in the series for parsing of
configuration values.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index 0887a07cfc67..93ede09fb36f 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -3,6 +3,9 @@
 #include "revision.h"
 
 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 void suppress(struct rev_info *revs)
 {
@@ -21,8 +24,6 @@ static void set_separate(struct rev_info *revs)
 	revs->separate_merges = 1;
 }
 
-static diff_merges_setup_func_t set_to_default = set_separate;
-
 static void set_first_parent(struct rev_info *revs)
 {
 	set_separate(revs);
@@ -54,29 +55,35 @@ static void set_dense_combined(struct rev_info *revs)
 	revs->dense_combined_merges = 1;
 }
 
-static void set_diff_merges(struct rev_info *revs, const char *optarg)
+static diff_merges_setup_func_t func_by_opt(const char *optarg)
 {
-	if (!strcmp(optarg, "off") || !strcmp(optarg, "none")) {
-		suppress(revs);
-		/* Return early to leave revs->merges_need_diff unset */
-		return;
-	}
-
+	if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))
+		return suppress;
 	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
-		set_first_parent(revs);
+		return set_first_parent;
 	else if (!strcmp(optarg, "m") || !strcmp(optarg, "separate"))
-		set_separate(revs);
+		return set_separate;
 	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
-		set_combined(revs);
+		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
-		set_dense_combined(revs);
+		return set_dense_combined;
 	else if (!strcmp(optarg, "def"))
-		set_to_default(revs);
-	else
+		return set_to_default;
+	return NULL;
+}
+
+static void set_diff_merges(struct rev_info *revs, const char *optarg)
+{
+	diff_merges_setup_func_t func = func_by_opt(optarg);
+
+	if (!func)
 		die(_("unknown value for --diff-merges: %s"), optarg);
 
-	/* The flag is cleared by set_xxx() functions, so don't move this up */
-	revs->merges_need_diff = 1;
+	func(revs);
+
+	/* NOTE: the merges_need_diff flag is cleared by func() call */
+	if (func != suppress)
+		revs->merges_need_diff = 1;
 }
 
 /*
-- 
2.25.1

[PATCH 3/9] diff-merges: introduce log.diffMerges config variable

[Sergey Organov]

New log.diffMerges configuration variable sets the format that
--diff-merges=def will be using. The default is "separate".

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 builtin/log.c |  2 ++
 diff-merges.c | 11 +++++++++++
 diff-merges.h |  2 ++
 3 files changed, 15 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index 8acd285dafd8..6102893fccb9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -481,6 +481,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 			decoration_style = 0; /* maybe warn? */
 		return 0;
 	}
+	if (!strcmp(var, "log.diffmerges"))
+		return diff_merges_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 93ede09fb36f..ca4d94a9039d 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -90,6 +90,17 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
  * Public functions. They are in the order they are called.
  */
 
+int diff_merges_config(const char *value)
+{
+	diff_merges_setup_func_t func = func_by_opt(value);
+
+	if (!func)
+		return -1;
+
+	set_to_default = func;
+	return 0;
+}
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 {
 	int argcount = 1;
diff --git a/diff-merges.h b/diff-merges.h
index 659467c99a4f..09d9a6c9a4fb 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -9,6 +9,8 @@
 
 struct rev_info;
 
+int diff_merges_config(const char *value);
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
 
 void diff_merges_suppress(struct rev_info *revs);
-- 
2.25.1

Re: [PATCH 3/9] diff-merges: introduce log.diffMerges config variable

[SZEDER Gábor]

On Thu, Apr 08, 2021 at 01:56:02AM +0300, Sergey Organov wrote:
> New log.diffMerges configuration variable sets the format that
> --diff-merges=def will be using. The default is "separate".
> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  builtin/log.c |  2 ++
>  diff-merges.c | 11 +++++++++++
>  diff-merges.h |  2 ++
>  3 files changed, 15 insertions(+)

Please don't forget to document this new configuration variable.

Re: [PATCH 3/9] diff-merges: introduce log.diffMerges config variable

[SZEDER Gábor]

On Thu, Apr 08, 2021 at 11:37:36PM +0200, SZEDER Gábor wrote:
> On Thu, Apr 08, 2021 at 01:56:02AM +0300, Sergey Organov wrote:
> > New log.diffMerges configuration variable sets the format that
> > --diff-merges=def will be using. The default is "separate".
> > 
> > Signed-off-by: Sergey Organov <sorganov@gmail.com>
> > ---
> >  builtin/log.c |  2 ++
> >  diff-merges.c | 11 +++++++++++
> >  diff-merges.h |  2 ++
> >  3 files changed, 15 insertions(+)
> 
> Please don't forget to document this new configuration variable.

Oh, just noticed that you do document it in the last patch of the
series, and, similarly, you add new options early in this patch series
and add the corresponding documentation in the second to last patch.
Please squash in those documentation updates into the corresponding
patches.

Re: [PATCH 3/9] diff-merges: introduce log.diffMerges config variable

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Thu, Apr 08, 2021 at 11:37:36PM +0200, SZEDER Gábor wrote:
>> On Thu, Apr 08, 2021 at 01:56:02AM +0300, Sergey Organov wrote:
>> > New log.diffMerges configuration variable sets the format that
>> > --diff-merges=def will be using. The default is "separate".
>> > 
>> > Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> > ---
>> >  builtin/log.c |  2 ++
>> >  diff-merges.c | 11 +++++++++++
>> >  diff-merges.h |  2 ++
>> >  3 files changed, 15 insertions(+)
>> 
>> Please don't forget to document this new configuration variable.
>
> Oh, just noticed that you do document it in the last patch of the
> series, and, similarly, you add new options early in this patch series
> and add the corresponding documentation in the second to last patch.
> Please squash in those documentation updates into the corresponding
> patches.

Since any new topic that adds new configuration variable or update
the description of an existing one would interact badly with the
last step of Emily's es/config-hooks topic, b58f84c4 (docs: unify
githooks and git-hook manpages, 2021-03-10), where the description
of all the individual options are moved to a newly created file, and
it is not practical to take all the new topics that touch the
documentation for the configuration variables hostage to the topic
that seems to be dormant for quite a while, I'll discard the last
step from es/config-hooks topic for now.  We really should get it
moving soon (or discard and reboot it later---it is getting in the
way for other topics to keep it in my tree, either way).

Thanks.

Re: [PATCH 3/9] diff-merges: introduce log.diffMerges config variable

[Sergey Organov]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Thu, Apr 08, 2021 at 11:37:36PM +0200, SZEDER Gábor wrote:
>> On Thu, Apr 08, 2021 at 01:56:02AM +0300, Sergey Organov wrote:
>> > New log.diffMerges configuration variable sets the format that
>> > --diff-merges=def will be using. The default is "separate".
>> > 
>> > Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> > ---
>> >  builtin/log.c |  2 ++
>> >  diff-merges.c | 11 +++++++++++
>> >  diff-merges.h |  2 ++
>> >  3 files changed, 15 insertions(+)
>> 
>> Please don't forget to document this new configuration variable.
>
> Oh, just noticed that you do document it in the last patch of the
> series, and, similarly, you add new options early in this patch series
> and add the corresponding documentation in the second to last patch.
> Please squash in those documentation updates into the corresponding
> patches.

Sorry, I fail to see how to do it that way, as documentation has mutual
references between --diff-merges=def and log.diffMerges, that are
introduced in different commits.

That said, after squashing tests into corresponding code commits, that
has been already requested, the documentation updates will be closer to
the code commits. Is it OK with you then to leave documentation changes
as separate commits?

Also, Junio, please clarify what you prefer as maintainer, fine-grained
commits, or squash everything even remotely relevant into a single one?

I honestly fail to see where the preferred margin is, and start to get a
feeling that I'd better squash everything together.

Thanks,

-- Sergey Organov

[PATCH 4/9] diff-merges: adapt -m to enable default diff format

[Sergey Organov]

Let -m option (and --diff-merges=m) enable the default format instead
of "separate", to be able to tune it with log.diffMerges option.

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

diff --git a/diff-merges.c b/diff-merges.c
index ca4d94a9039d..f68e4376fd63 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -34,10 +34,10 @@ static void set_m(struct rev_info *revs)
 {
 	/*
 	 * To "diff-index", "-m" means "match missing", and to the "log"
-	 * family of commands, it means "show full diff for merges". Set
+	 * family of commands, it means "show default diff for merges". Set
 	 * both fields appropriately.
 	 */
-	set_separate(revs);
+	set_to_default(revs);
 	revs->match_missing = 1;
 }
 
@@ -61,13 +61,13 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 		return suppress;
 	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
 		return set_first_parent;
-	else if (!strcmp(optarg, "m") || !strcmp(optarg, "separate"))
+	else if (!strcmp(optarg, "separate"))
 		return set_separate;
 	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
 		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		return set_dense_combined;
-	else if (!strcmp(optarg, "def"))
+	else if (!strcmp(optarg, "m") || !strcmp(optarg, "def"))
 		return set_to_default;
 	return NULL;
 }
-- 
2.25.1

[PATCH 5/9] t4013: add test for --diff-merges=def

[Sergey Organov]

This new option by default should match --diff-merges=separate, so
test this.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 6cca8b84a6bf..275a6790896d 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+test_expect_success 'log --diff-merges=def matches --diff-merges=separate' '
+	git log -p --diff-merges=separate master >result &&
+	process_diffs result >expected &&
+	git log -p --diff-merges=def master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
-- 
2.25.1

[PATCH 6/9] t4013: add tests for log.diffMerges config

[Sergey Organov]

Test that wrong values are denied.

Test that the value of log.diffMerges properly affects both
--diff-merges=def and -m.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t4013-diff-various.sh | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 275a6790896d..ee4afca06ced 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -460,6 +460,32 @@ test_expect_success 'log --diff-merges=def matches --diff-merges=separate' '
 	test_cmp expected actual
 '
 
+test_expect_success 'deny wrong log.diffMerges config' '
+	git config log.diffMerges wrong-value &&
+	test_expect_code 128 git log &&
+	git config --unset log.diffMerges
+'
+
+test_expect_success 'git config log.diffMerges first-parent' '
+	git log -p --diff-merges=first-parent master >result &&
+	process_diffs result >expected &&
+	git config log.diffMerges first-parent &&
+	git log -p --diff-merges=def master >result &&
+	process_diffs result >actual &&
+	git config --unset log.diffMerges &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git config log.diffMerges first-parent vs -m' '
+	git log -p --diff-merges=first-parent master >result &&
+	process_diffs result >expected &&
+	git config log.diffMerges first-parent &&
+	git log -p -m master >result &&
+	process_diffs result >actual &&
+	git config --unset log.diffMerges &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
-- 
2.25.1

Re: [PATCH 6/9] t4013: add tests for log.diffMerges config

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 08 2021, Sergey Organov wrote:

> Test that wrong values are denied.
>
> Test that the value of log.diffMerges properly affects both
> --diff-merges=def and -m.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  t/t4013-diff-various.sh | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 275a6790896d..ee4afca06ced 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -460,6 +460,32 @@ test_expect_success 'log --diff-merges=def matches --diff-merges=separate' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'deny wrong log.diffMerges config' '
> +	git config log.diffMerges wrong-value &&
> +	test_expect_code 128 git log &&
> +	git config --unset log.diffMerges

Don't use "git config", but "test_config" at the start, then you don't
need the --unset at the end, it'll happen automatically. Ditto for the
following tests.

> +'
> +
> +test_expect_success 'git config log.diffMerges first-parent' '
> +	git log -p --diff-merges=first-parent master >result &&
> +	process_diffs result >expected &&
> +	git config log.diffMerges first-parent &&
> +	git log -p --diff-merges=def master >result &&
> +	process_diffs result >actual &&
> +	git config --unset log.diffMerges &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'git config log.diffMerges first-parent vs -m' '
> +	git log -p --diff-merges=first-parent master >result &&
> +	process_diffs result >expected &&
> +	git config log.diffMerges first-parent &&
> +	git log -p -m master >result &&
> +	process_diffs result >actual &&
> +	git config --unset log.diffMerges &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'log -S requires an argument' '
>  	test_must_fail git log -S
>  '

Re: [PATCH 6/9] t4013: add tests for log.diffMerges config

[Junio C Hamano]

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

>> +test_expect_success 'deny wrong log.diffMerges config' '
>> +	git config log.diffMerges wrong-value &&
>> +	test_expect_code 128 git log &&
>> +	git config --unset log.diffMerges
>
> Don't use "git config", but "test_config" at the start, then you don't
> need the --unset at the end, it'll happen automatically. Ditto for the
> following tests.

More importantly, test_config arranges the unset to happen even if
a step in the middle (e.g. test_expect_code in the above example)
fails.  In the posted version, the control would not reach the
"git config --unset" and leaves the configuration behind.

And that is the biggest reason why the above should use test_config.

Thanks for a good suggestion.

Re: [PATCH 6/9] t4013: add tests for log.diffMerges config

[Sergey Organov]

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

> On Thu, Apr 08 2021, Sergey Organov wrote:

[...]

>> +test_expect_success 'deny wrong log.diffMerges config' '
>> +	git config log.diffMerges wrong-value &&
>> +	test_expect_code 128 git log &&
>> +	git config --unset log.diffMerges
>
> Don't use "git config", but "test_config" at the start, then you don't
> need the --unset at the end, it'll happen automatically. Ditto for the
> following tests.

[...]

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> +test_expect_success 'deny wrong log.diffMerges config' '
>>> +	git config log.diffMerges wrong-value &&
>>> +	test_expect_code 128 git log &&
>>> +	git config --unset log.diffMerges
>>
>> Don't use "git config", but "test_config" at the start, then you don't
>> need the --unset at the end, it'll happen automatically. Ditto for the
>> following tests.
>
> More importantly, test_config arranges the unset to happen even if
> a step in the middle (e.g. test_expect_code in the above example)
> fails.  In the posted version, the control would not reach the
> "git config --unset" and leaves the configuration behind.
>
> And that is the biggest reason why the above should use test_config.

Yeah, thanks for pointing, – will fix for the next re-roll.

-- Sergey Organov

[PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges

[Sergey Organov]

There were 3 completion tests failures due to introduction of
log.diffMerges configuration variable that affected the result of
completion of log.d. Fixed them accordingly.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t9902-completion.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 04ce884ef5ac..4d732d6d4f81 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
 	test_completion "git config log.d" <<-\EOF
 	log.date Z
 	log.decorate Z
+	log.diffMerges Z
 	EOF
 '
 
@@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
 	test_completion "git -c log.d" <<-\EOF
 	log.date=Z
 	log.decorate=Z
+	log.diffMerges=Z
 	EOF
 '
 
@@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
 	test_completion "git clone --config=log.d" <<-\EOF
 	log.date=Z
 	log.decorate=Z
+	log.diffMerges=Z
 	EOF
 '
 
-- 
2.25.1

Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 08 2021, Sergey Organov wrote:

> There were 3 completion tests failures due to introduction of
> log.diffMerges configuration variable that affected the result of
> completion of log.d. Fixed them accordingly.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  t/t9902-completion.sh | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 04ce884ef5ac..4d732d6d4f81 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
>  	test_completion "git config log.d" <<-\EOF
>  	log.date Z
>  	log.decorate Z
> +	log.diffMerges Z
>  	EOF
>  '
>  
> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
>  	test_completion "git -c log.d" <<-\EOF
>  	log.date=Z
>  	log.decorate=Z
> +	log.diffMerges=Z
>  	EOF
>  '
>  
> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
>  	test_completion "git clone --config=log.d" <<-\EOF
>  	log.date=Z
>  	log.decorate=Z
> +	log.diffMerges=Z
>  	EOF
>  '

Commits should be made in such a way as to not break the build/tests
partway through a series, which it seems is happening until this fixup.

Having read this far most of what you have in this 9 patch series
could/should be squashed into something much smaller, e.g. tests being
added for code added in previous steps, let's add the tests along with
the code since this isn't such a large change.

Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges

[Sergey Organov]

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

> On Thu, Apr 08 2021, Sergey Organov wrote:
>
>> There were 3 completion tests failures due to introduction of
>> log.diffMerges configuration variable that affected the result of
>> completion of log.d. Fixed them accordingly.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  t/t9902-completion.sh | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 04ce884ef5ac..4d732d6d4f81 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
>>  	test_completion "git config log.d" <<-\EOF
>>  	log.date Z
>>  	log.decorate Z
>> +	log.diffMerges Z
>>  	EOF
>>  '
>>  
>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
>>  	test_completion "git -c log.d" <<-\EOF
>>  	log.date=Z
>>  	log.decorate=Z
>> +	log.diffMerges=Z
>>  	EOF
>>  '
>>  
>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
>>  	test_completion "git clone --config=log.d" <<-\EOF
>>  	log.date=Z
>>  	log.decorate=Z
>> +	log.diffMerges=Z
>>  	EOF
>>  '
>
> Commits should be made in such a way as to not break the build/tests
> partway through a series, which it seems is happening until this
> fixup.

Yep.

Could these tests be somehow written in a more robust manner, to be
protected against future additions of configuration variables that are
unrelated to the features being tested? If so, I'd prefer to fix them as
a prerequisite to the series rather than adding fixes to unrelated 
existing tests into my patches.

> Having read this far most of what you have in this 9 patch series
> could/should be squashed into something much smaller, e.g. tests being
> added for code added in previous steps, let's add the tests along with
> the code since this isn't such a large change.

In general, I try to make commits as small as possible, but if you
prefer tests to be included with the code in the same commit, – that's
fine with me too.

Will meld new tests into code commits for the next re-roll.

Thanks!

-- Sergey Organov

Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 08 2021, Sergey Organov wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Thu, Apr 08 2021, Sergey Organov wrote:
>>
>>> There were 3 completion tests failures due to introduction of
>>> log.diffMerges configuration variable that affected the result of
>>> completion of log.d. Fixed them accordingly.
>>>
>>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>>> ---
>>>  t/t9902-completion.sh | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>>> index 04ce884ef5ac..4d732d6d4f81 100755
>>> --- a/t/t9902-completion.sh
>>> +++ b/t/t9902-completion.sh
>>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
>>>  	test_completion "git config log.d" <<-\EOF
>>>  	log.date Z
>>>  	log.decorate Z
>>> +	log.diffMerges Z
>>>  	EOF
>>>  '
>>>  
>>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
>>>  	test_completion "git -c log.d" <<-\EOF
>>>  	log.date=Z
>>>  	log.decorate=Z
>>> +	log.diffMerges=Z
>>>  	EOF
>>>  '
>>>  
>>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
>>>  	test_completion "git clone --config=log.d" <<-\EOF
>>>  	log.date=Z
>>>  	log.decorate=Z
>>> +	log.diffMerges=Z
>>>  	EOF
>>>  '
>>
>> Commits should be made in such a way as to not break the build/tests
>> partway through a series, which it seems is happening until this
>> fixup.
>
> Yep.
>
> Could these tests be somehow written in a more robust manner, to be
> protected against future additions of configuration variables that are
> unrelated to the features being tested? If so, I'd prefer to fix them as
> a prerequisite to the series rather than adding fixes to unrelated 
> existing tests into my patches.

Hrm? I mean if you have a commit fixing up failing tests in an earlier
commit then that change should in one way or the other be made as part
of that earlier change.

Yes we can skip the tests or something in the meantime, which we do
sometimes as part of some really large changes, but these can just be
squashed, no?

>> Having read this far most of what you have in this 9 patch series
>> could/should be squashed into something much smaller, e.g. tests being
>> added for code added in previous steps, let's add the tests along with
>> the code since this isn't such a large change.
>
> In general, I try to make commits as small as possible, but if you
> prefer tests to be included with the code in the same commit, – that's
> fine with me too.
>
> Will meld new tests into code commits for the next re-roll.

I'm probably the last person to give advice on this list about not
overly splitting up ones commits :)

Having said that, some sage advice:

It's really helpful to split commits into discrete understandable pieces
when it aids in reviewing/understanding the code.

But something like say your 8/9 is IMNSHO a step to far, you're just
adding a feature earlier and then docs for it later. That doesn't help
to review or understand the change, now you just need to look in two
places for what's one logical change.

Ditto for e.g. the 5/9 here. That's just a test for a feature added
earlier. So let's add it to the commit where we add that feature.

There *are* cases where it helps to split up these changes, but they're
things like adding tests for existing behavior before changing
something, as an aid to demonstrate what the behavior was before &
after.

In those cases it's a lot better to split the commits, because nobody
wants to waste time discerning what's a test for existing v.s. new
behavior.

Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges

[Sergey Organov]

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

> On Thu, Apr 08 2021, Sergey Organov wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> On Thu, Apr 08 2021, Sergey Organov wrote:
>>>
>>>> There were 3 completion tests failures due to introduction of
>>>> log.diffMerges configuration variable that affected the result of
>>>> completion of log.d. Fixed them accordingly.
>>>>
>>>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>>>> ---
>>>>  t/t9902-completion.sh | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>>>> index 04ce884ef5ac..4d732d6d4f81 100755
>>>> --- a/t/t9902-completion.sh
>>>> +++ b/t/t9902-completion.sh
>>>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
>>>>  	test_completion "git config log.d" <<-\EOF
>>>>  	log.date Z
>>>>  	log.decorate Z
>>>> +	log.diffMerges Z
>>>>  	EOF
>>>>  '
>>>>  
>>>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
>>>>  	test_completion "git -c log.d" <<-\EOF
>>>>  	log.date=Z
>>>>  	log.decorate=Z
>>>> +	log.diffMerges=Z
>>>>  	EOF
>>>>  '
>>>>  
>>>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
>>>>  	test_completion "git clone --config=log.d" <<-\EOF
>>>>  	log.date=Z
>>>>  	log.decorate=Z
>>>> +	log.diffMerges=Z
>>>>  	EOF
>>>>  '
>>>
>>> Commits should be made in such a way as to not break the build/tests
>>> partway through a series, which it seems is happening until this
>>> fixup.
>>
>> Yep.
>>
>> Could these tests be somehow written in a more robust manner, to be
>> protected against future additions of configuration variables that are
>> unrelated to the features being tested? If so, I'd prefer to fix them as
>> a prerequisite to the series rather than adding fixes to unrelated 
>> existing tests into my patches.
>
> Hrm? I mean if you have a commit fixing up failing tests in an earlier
> commit then that change should in one way or the other be made as part
> of that earlier change.
>
> Yes we can skip the tests or something in the meantime, which we do
> sometimes as part of some really large changes, but these can just be
> squashed, no?

I mean I don't want this change at all.

I didn't change completion mechanism, so completion tests should not
suddenly fail because of my changes. I did entirely unrelated change and
noticed the breakage only by accident, as tests even don't fail unless
you *install* git, not only make it. So, for example, just "make test"
doesn't fail, while "make install; make test" will.

It looks like something is wrong here, a bug or misfeature, or even two,
and if it's fixed before these series, I won't need this in my series at
all. Besides, that's yet another reason *not* to squash this change into
an otherwise unrelated commit.

-- Sergey Organov

Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges

[SZEDER Gábor]

On Thu, Apr 08, 2021 at 11:26:38PM +0300, Sergey Organov wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > On Thu, Apr 08 2021, Sergey Organov wrote:
> >
> >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >>
> >>> On Thu, Apr 08 2021, Sergey Organov wrote:
> >>>
> >>>> There were 3 completion tests failures due to introduction of
> >>>> log.diffMerges configuration variable that affected the result of
> >>>> completion of log.d. Fixed them accordingly.
> >>>>
> >>>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> >>>> ---
> >>>>  t/t9902-completion.sh | 3 +++
> >>>>  1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> >>>> index 04ce884ef5ac..4d732d6d4f81 100755
> >>>> --- a/t/t9902-completion.sh
> >>>> +++ b/t/t9902-completion.sh
> >>>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
> >>>>  	test_completion "git config log.d" <<-\EOF
> >>>>  	log.date Z
> >>>>  	log.decorate Z
> >>>> +	log.diffMerges Z
> >>>>  	EOF
> >>>>  '
> >>>>  
> >>>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
> >>>>  	test_completion "git -c log.d" <<-\EOF
> >>>>  	log.date=Z
> >>>>  	log.decorate=Z
> >>>> +	log.diffMerges=Z
> >>>>  	EOF
> >>>>  '
> >>>>  
> >>>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
> >>>>  	test_completion "git clone --config=log.d" <<-\EOF
> >>>>  	log.date=Z
> >>>>  	log.decorate=Z
> >>>> +	log.diffMerges=Z
> >>>>  	EOF
> >>>>  '
> >>>
> >>> Commits should be made in such a way as to not break the build/tests
> >>> partway through a series, which it seems is happening until this
> >>> fixup.

Well, actually no: it _starts_ to break with this patch, because
'log.diffMerges' is not documented yet, and it will pass again with
the last patch in the series that adds that missing piece of
documentation.

> >> Yep.
> >>
> >> Could these tests be somehow written in a more robust manner, to be
> >> protected against future additions of configuration variables that are
> >> unrelated to the features being tested? If so, I'd prefer to fix them as
> >> a prerequisite to the series rather than adding fixes to unrelated 
> >> existing tests into my patches.
> >
> > Hrm? I mean if you have a commit fixing up failing tests in an earlier
> > commit then that change should in one way or the other be made as part
> > of that earlier change.
> >
> > Yes we can skip the tests or something in the meantime, which we do
> > sometimes as part of some really large changes, but these can just be
> > squashed, no?
> 
> I mean I don't want this change at all.

You'll definitely need this change, though.

> I didn't change completion mechanism, so completion tests should not
> suddenly fail because of my changes.

We auto-generate the list of supported configuration variables from
the documentation and use that list in our Bash completion script to
list possible configuration variables for 'git config <TAB>' and 'git
-c <TAB>'.  And we want to make sure that this feature works as
intended, so we have a couple of tests that try to complete real
config variable sections and names.  You are just unlucky to introduce
a new configuraton variable that happenes to start with the same
prefix that is used in some of those tests.

> I did entirely unrelated change and
> noticed the breakage only by accident, as tests even don't fail unless
> you *install* git, not only make it. So, for example, just "make test"
> doesn't fail, while "make install; make test" will.

It might be related to a bug in the build process that doesn't update
that auto-generated list of supported configuration variables after
e.g. 'Documentation/config/log.txt' was modified; see a proposed fix
at:

  https://public-inbox.org/git/20210408212915.3060286-1-szeder.dev@gmail.com/

> It looks like something is wrong here, a bug or misfeature, or even two,
> and if it's fixed before these series, I won't need this in my series at
> all. Besides, that's yet another reason *not* to squash this change into
> an otherwise unrelated commit.

The introduction of the new configuration variable, its documentation
and this test update should all go into a single patch.  The whole
test suite must pass for every single commit.

Re: [PATCH 7/9] t9902: fix completion tests for log.d* to match log.diffMerges

[Sergey Organov]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Thu, Apr 08, 2021 at 11:26:38PM +0300, Sergey Organov wrote:

[...]

>
>> It looks like something is wrong here, a bug or misfeature, or even two,
>> and if it's fixed before these series, I won't need this in my series at
>> all. Besides, that's yet another reason *not* to squash this change into
>> an otherwise unrelated commit.
>
> The introduction of the new configuration variable, its documentation
> and this test update should all go into a single patch.  The whole
> test suite must pass for every single commit.

OK, fine, thanks for clarification!

-- Sergey Organov

[PATCH 8/9] doc/diff-options: document new --diff-merges features

[Sergey Organov]

Document changes in -m and --diff-merges=m semantics, as well as new
--diff-merges=def option.

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

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index aa2b5c11f20b..09b07231b5a4 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|first-parent|1|separate|m|combined|c|dense-combined|cc)::
+--diff-merges=(off|none|def|first-parent|1|separate|m|combined|c|dense-combined|cc)::
 --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
@@ -45,17 +45,24 @@ ifdef::git-log[]
 	Disable output of diffs for merge commits. Useful to override
 	implied value.
 +
+--diff-merges=def:::
+--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
+	`log.diffMerges` configuration parameter, which default value
+	is `separate`.
++
 --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:::
---diff-merges=m:::
--m:::
 	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. `-m` doesn't produce any output without `-p`.
+	for each parent.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
-- 
2.25.1

[PATCH 9/9] doc/config: document log.diffMerges

[Sergey Organov]

Added documentation for the new log.diffMerges configuration option.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/config/log.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 208d5fdcaa68..456eb07800cb 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -24,6 +24,11 @@ log.excludeDecoration::
 	the config option can be overridden by the `--decorate-refs`
 	option.
 
+log.diffMerges::
+	Set default diff format to be used for merge commits. See
+	`--diff-merges` in linkgit:git-log[1] for details.
+	Defaults to `separate`.
+
 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`,
-- 
2.25.1

[PATCH v1 0/5] git log: configurable default format for merge diffs

[Sergey Organov]

These patches introduce capability to configure the default format of
output of diffs for merge commits by means of new log.diffMerges
configuration variable. The default format is then used by -m,
--diff-merges=m, and new --diff-merges=default options.

In particular,

  git config log.diffMerges first-parent

will change -m option format from "separate" to "first-parent" that
will in turn cause, say,

  git show -m <merge_commit>

to output diff to the first parent only, instead of appending
typically large and surprising diff to the second parent at the end of
the output.

Updates in v1:

  * Renamed abbreviated value "def" to full "default"

  * Fixed tests to use "test_config" instead of "git config"

  * Meld all "git config" changes into single commit that includes
    code, documentation, and tests, as they are mutually
    interdependent.

Sergey Organov (5):
  diff-merges: introduce --diff-merges=default
  diff-merges: refactor set_diff_merges()
  diff-merges: adapt -m to enable default diff format
  diff-merges: introduce log.diffMerges config variable
  doc/diff-options: document new --diff-merges features

 Documentation/config/log.txt   |  5 +++
 Documentation/diff-options.txt | 15 ++++++---
 builtin/log.c                  |  2 ++
 diff-merges.c                  | 58 ++++++++++++++++++++++++----------
 diff-merges.h                  |  2 ++
 t/t4013-diff-various.sh        | 31 ++++++++++++++++++
 t/t9902-completion.sh          |  3 ++
 7 files changed, 95 insertions(+), 21 deletions(-)

Interdiff against v0:
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 09b07231b5a4..31e2bacf5252 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|def|first-parent|1|separate|m|combined|c|dense-combined|cc)::
+--diff-merges=(off|none|default|first-parent|1|separate|m|combined|c|dense-combined|cc)::
 --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
@@ -45,7 +45,7 @@ ifdef::git-log[]
 	Disable output of diffs for merge commits. Useful to override
 	implied value.
 +
---diff-merges=def:::
+--diff-merges=default:::
 --diff-merges=m:::
 -m:::
 	This option makes diff output for merge commits to be shown in
diff --git a/diff-merges.c b/diff-merges.c
index f68e4376fd63..75630fb8e6b8 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -67,7 +67,7 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		return set_dense_combined;
-	else if (!strcmp(optarg, "m") || !strcmp(optarg, "def"))
+	else if (!strcmp(optarg, "m") || !strcmp(optarg, "default"))
 		return set_to_default;
 	return NULL;
 }
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index ee4afca06ced..87cab7867135 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -452,37 +452,34 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
-test_expect_success 'log --diff-merges=def matches --diff-merges=separate' '
+test_expect_success 'log --diff-merges=default matches --diff-merges=separate' '
 	git log -p --diff-merges=separate master >result &&
 	process_diffs result >expected &&
-	git log -p --diff-merges=def master >result &&
+	git log -p --diff-merges=default master >result &&
 	process_diffs result >actual &&
 	test_cmp expected actual
 '
 
 test_expect_success 'deny wrong log.diffMerges config' '
-	git config log.diffMerges wrong-value &&
-	test_expect_code 128 git log &&
-	git config --unset log.diffMerges
+	test_config log.diffMerges wrong-value &&
+	test_expect_code 128 git log
 '
 
 test_expect_success 'git config log.diffMerges first-parent' '
 	git log -p --diff-merges=first-parent master >result &&
 	process_diffs result >expected &&
-	git config log.diffMerges first-parent &&
-	git log -p --diff-merges=def master >result &&
+	test_config log.diffMerges first-parent &&
+	git log -p --diff-merges=default master >result &&
 	process_diffs result >actual &&
-	git config --unset log.diffMerges &&
 	test_cmp expected actual
 '
 
 test_expect_success 'git config log.diffMerges first-parent vs -m' '
 	git log -p --diff-merges=first-parent master >result &&
 	process_diffs result >expected &&
-	git config log.diffMerges first-parent &&
+	test_config log.diffMerges first-parent &&
 	git log -p -m master >result &&
 	process_diffs result >actual &&
-	git config --unset log.diffMerges &&
 	test_cmp expected actual
 '
 
-- 
2.25.1

[PATCH v1 1/5] diff-merges: introduce --diff-merges=default

[Sergey Organov]

Introduce the notion of default diff format for merges, and the option
"default" to select it. The default format is "separate" and can't yet
be changed, so effectively "default" is just a synonym for "separate"
for now. Add corresponding test to t4013.

This is in preparation for introducing log.diffMerges configuration
option that will let --diff-merges=default to be configured to any
supported format.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c           | 7 +++++++
 t/t4013-diff-various.sh | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/diff-merges.c b/diff-merges.c
index 146bb50316a6..7690580d7464 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -2,6 +2,11 @@
 
 #include "revision.h"
 
+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 void suppress(struct rev_info *revs)
 {
 	revs->separate_merges = 0;
@@ -66,6 +71,8 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
 		set_combined(revs);
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		set_dense_combined(revs);
+	else if (!strcmp(optarg, "default"))
+		set_to_default(revs);
 	else
 		die(_("unknown value for --diff-merges: %s"), optarg);
 
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 6cca8b84a6bf..8acb5b866900 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+test_expect_success 'log --diff-merges=default matches --diff-merges=separate' '
+	git log -p --diff-merges=separate master >result &&
+	process_diffs result >expected &&
+	git log -p --diff-merges=default master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
-- 
2.25.1

[PATCH v1 2/5] diff-merges: refactor set_diff_merges()

[Sergey Organov]

Split set_diff_merges() into separate parsing and execution functions,
the former to be reused for parsing of configuration values later in
the patch series.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index 7690580d7464..9918b6ac55e4 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -55,29 +55,35 @@ static void set_dense_combined(struct rev_info *revs)
 	revs->dense_combined_merges = 1;
 }
 
-static void set_diff_merges(struct rev_info *revs, const char *optarg)
+static diff_merges_setup_func_t func_by_opt(const char *optarg)
 {
-	if (!strcmp(optarg, "off") || !strcmp(optarg, "none")) {
-		suppress(revs);
-		/* Return early to leave revs->merges_need_diff unset */
-		return;
-	}
-
+	if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))
+		return suppress;
 	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
-		set_first_parent(revs);
+		return set_first_parent;
 	else if (!strcmp(optarg, "m") || !strcmp(optarg, "separate"))
-		set_separate(revs);
+		return set_separate;
 	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
-		set_combined(revs);
+		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
-		set_dense_combined(revs);
+		return set_dense_combined;
 	else if (!strcmp(optarg, "default"))
-		set_to_default(revs);
-	else
+		return set_to_default;
+	return NULL;
+}
+
+static void set_diff_merges(struct rev_info *revs, const char *optarg)
+{
+	diff_merges_setup_func_t func = func_by_opt(optarg);
+
+	if (!func)
 		die(_("unknown value for --diff-merges: %s"), optarg);
 
-	/* The flag is cleared by set_xxx() functions, so don't move this up */
-	revs->merges_need_diff = 1;
+	func(revs);
+
+	/* NOTE: the merges_need_diff flag is cleared by func() call */
+	if (func != suppress)
+		revs->merges_need_diff = 1;
 }
 
 /*
-- 
2.25.1

[PATCH v1 3/5] diff-merges: adapt -m to enable default diff format

[Sergey Organov]

Let -m option (and --diff-merges=m) enable the default format instead
of "separate", to be able to tune it with log.diffMerges option.

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

diff --git a/diff-merges.c b/diff-merges.c
index 9918b6ac55e4..a02f39828336 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -34,10 +34,10 @@ static void set_m(struct rev_info *revs)
 {
 	/*
 	 * To "diff-index", "-m" means "match missing", and to the "log"
-	 * family of commands, it means "show full diff for merges". Set
+	 * family of commands, it means "show default diff for merges". Set
 	 * both fields appropriately.
 	 */
-	set_separate(revs);
+	set_to_default(revs);
 	revs->match_missing = 1;
 }
 
@@ -61,13 +61,13 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 		return suppress;
 	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
 		return set_first_parent;
-	else if (!strcmp(optarg, "m") || !strcmp(optarg, "separate"))
+	else if (!strcmp(optarg, "separate"))
 		return set_separate;
 	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
 		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		return set_dense_combined;
-	else if (!strcmp(optarg, "default"))
+	else if (!strcmp(optarg, "m") || !strcmp(optarg, "default"))
 		return set_to_default;
 	return NULL;
 }
-- 
2.25.1

[PATCH v1 4/5] diff-merges: introduce log.diffMerges config variable

[Sergey Organov]

New log.diffMerges configuration variable sets the format that
--diff-merges=default will be using. The default is "separate".

t4013: add the following tests for log.diffMerges config:

* Test that wrong values are denied.

* Test that the value of log.diffMerges properly affects both
--diff-merges=default and -m.

t9902: fix completion tests for log.d* to match log.diffMerges.

Added documentation for log.diffMerges.

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

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 208d5fdcaa68..456eb07800cb 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -24,6 +24,11 @@ log.excludeDecoration::
 	the config option can be overridden by the `--decorate-refs`
 	option.
 
+log.diffMerges::
+	Set default diff format to be used for merge commits. See
+	`--diff-merges` in linkgit:git-log[1] for details.
+	Defaults to `separate`.
+
 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 8acd285dafd8..6102893fccb9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -481,6 +481,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 			decoration_style = 0; /* maybe warn? */
 		return 0;
 	}
+	if (!strcmp(var, "log.diffmerges"))
+		return diff_merges_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 a02f39828336..75630fb8e6b8 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -90,6 +90,17 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
  * Public functions. They are in the order they are called.
  */
 
+int diff_merges_config(const char *value)
+{
+	diff_merges_setup_func_t func = func_by_opt(value);
+
+	if (!func)
+		return -1;
+
+	set_to_default = func;
+	return 0;
+}
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 {
 	int argcount = 1;
diff --git a/diff-merges.h b/diff-merges.h
index 659467c99a4f..09d9a6c9a4fb 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -9,6 +9,8 @@
 
 struct rev_info;
 
+int diff_merges_config(const char *value);
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
 
 void diff_merges_suppress(struct rev_info *revs);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 8acb5b866900..87cab7867135 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -460,6 +460,29 @@ test_expect_success 'log --diff-merges=default matches --diff-merges=separate' '
 	test_cmp expected actual
 '
 
+test_expect_success 'deny wrong log.diffMerges config' '
+	test_config log.diffMerges wrong-value &&
+	test_expect_code 128 git log
+'
+
+test_expect_success 'git config log.diffMerges first-parent' '
+	git log -p --diff-merges=first-parent master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges first-parent &&
+	git log -p --diff-merges=default master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git config log.diffMerges first-parent vs -m' '
+	git log -p --diff-merges=first-parent master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges first-parent &&
+	git log -p -m master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 04ce884ef5ac..4d732d6d4f81 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
 	test_completion "git config log.d" <<-\EOF
 	log.date Z
 	log.decorate Z
+	log.diffMerges Z
 	EOF
 '
 
@@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
 	test_completion "git -c log.d" <<-\EOF
 	log.date=Z
 	log.decorate=Z
+	log.diffMerges=Z
 	EOF
 '
 
@@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
 	test_completion "git clone --config=log.d" <<-\EOF
 	log.date=Z
 	log.decorate=Z
+	log.diffMerges=Z
 	EOF
 '
 
-- 
2.25.1

[PATCH v1 5/5] doc/diff-options: document new --diff-merges features

[Sergey Organov]

Document changes in -m and --diff-merges=m semantics, as well as new
--diff-merges=default option.

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

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index aa2b5c11f20b..31e2bacf5252 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|first-parent|1|separate|m|combined|c|dense-combined|cc)::
+--diff-merges=(off|none|default|first-parent|1|separate|m|combined|c|dense-combined|cc)::
 --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
@@ -45,17 +45,24 @@ ifdef::git-log[]
 	Disable output of diffs for merge commits. Useful to override
 	implied value.
 +
+--diff-merges=default:::
+--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
+	`log.diffMerges` configuration parameter, which default value
+	is `separate`.
++
 --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:::
---diff-merges=m:::
--m:::
 	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. `-m` doesn't produce any output without `-p`.
+	for each parent.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
-- 
2.25.1

Re: [PATCH v1 0/5] git log: configurable default format for merge diffs

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> These patches introduce capability to configure the default format of
> output of diffs for merge commits by means of new log.diffMerges
> configuration variable. The default format is then used by -m,
> --diff-merges=m, and new --diff-merges=default options.
>
> In particular,
>
>   git config log.diffMerges first-parent
>
> will change -m option format from "separate" to "first-parent" that
> will in turn cause, say,
>
>   git show -m <merge_commit>
>
> to output diff to the first parent only, instead of appending
> typically large and surprising diff to the second parent at the end of
> the output.

I think that it is a good goal to free a short-and-sweet "-m" from
getting tied forever to the current "two-tree diff for each of the
parent" (aka "separate"), and a configuration to change what the
"-m" option means would be a good approach to do so.  It would help
the interactive use by human end-users, which is the point of having
short-and-sweet options.  Existing scripts may depend on the current
behaviour, so the configuration cannot be introduced right away, but
over time they can be migrated to use the longer and more explicit
option "--diff-merges=separate".

But I do not see much point in adding the "--diff-merges=default".
Who is the target audience?  Certainly not scripts that want to
avoid depending on the 'default' that can be different and easily
vary per user.

Or is the plan to deprecate and remove the short-and-sweet "-m"
option and standardize on "--diff-merges=<style>"?  If so, such a
design makes sense from pureness and completeness standpoint, but I
am not sure if that is a good design for practical use.

Re: [PATCH v1 0/5] git log: configurable default format for merge diffs

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> These patches introduce capability to configure the default format of
>> output of diffs for merge commits by means of new log.diffMerges
>> configuration variable. The default format is then used by -m,
>> --diff-merges=m, and new --diff-merges=default options.
>>
>> In particular,
>>
>>   git config log.diffMerges first-parent
>>
>> will change -m option format from "separate" to "first-parent" that
>> will in turn cause, say,
>>
>>   git show -m <merge_commit>
>>
>> to output diff to the first parent only, instead of appending
>> typically large and surprising diff to the second parent at the end of
>> the output.
>
> I think that it is a good goal to free a short-and-sweet "-m" from
> getting tied forever to the current "two-tree diff for each of the
> parent" (aka "separate"), and a configuration to change what the
> "-m" option means would be a good approach to do so.  It would help
> the interactive use by human end-users, which is the point of having
> short-and-sweet options.  Existing scripts may depend on the current
> behaviour, so the configuration cannot be introduced right away, but
> over time they can be migrated to use the longer and more explicit
> option "--diff-merges=separate".

Yep, that's exactly the plan I have in mind.

To tell the truth, I hope there are no scripts that use "git log -m -p",
or "git show -m", but I do want to be on the safe side with it anyway,
and then sometime in the future maybe we will be safe to change
configuration default.

>
> But I do not see much point in adding the "--diff-merges=default".
> Who is the target audience?  Certainly not scripts that want to
> avoid depending on the 'default' that can be different and easily
> vary per user.

There are 2 reasons to have "default":

1. --diff-merges=default and -m are not exact synonyms: unlike -m,
--diff-merges=default (similar to other --diff-merges options)
immediately enables diff output for merges, without -p, thus, for
example, allowing to output diffs for merge commits only.

The exact synonyms are rather --diff-merges=m and --diff-merges=default,
and then we get to the next reason:

2. We have descriptive long name for every other option, and it'd be an
exception if we'd have none for --diff-merges=m. In fact, it's
--diff-merges=m that could have been removed, but it'd break resemblance
with --cc and -c that both do have their --diff-merges=cc and
--diff-merges=c counterparts.

Overall, having "default" has both functional and consistency merits.

> Or is the plan to deprecate and remove the short-and-sweet "-m"
> option and standardize on "--diff-merges=<style>"?  If so, such a
> design makes sense from pureness and completeness standpoint, but I
> am not sure if that is a good design for practical use.

No, what I have in mind is resurrection of -m as more useful option, not
removing it.

Thanks,
-- Sergey Organov

Re: [PATCH v1 0/5] git log: configurable default format for merge diffs

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> 2. We have descriptive long name for every other option, and it'd be an
> exception if we'd have none for --diff-merges=m. In fact, it's
> --diff-merges=m that could have been removed, but it'd break resemblance
> with --cc and -c that both do have their --diff-merges=cc and
> --diff-merges=c counterparts.

Hmph, a devil's advocate in me suspects that it may just be arguing
why user-configurable 'default' is a bad idea, though.

Re: [PATCH v1 0/5] git log: configurable default format for merge diffs

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> 2. We have descriptive long name for every other option, and it'd be an
>> exception if we'd have none for --diff-merges=m. In fact, it's
>> --diff-merges=m that could have been removed, but it'd break resemblance
>> with --cc and -c that both do have their --diff-merges=cc and
>> --diff-merges=c counterparts.
>
> Hmph, a devil's advocate in me suspects that it may just be arguing
> why user-configurable 'default' is a bad idea, though.

What feels bad about it? Is there something inherently wrong with an
ability to configure a default and then request that default to be
applied, using command-line option?

-- Sergey Organov

Re: [PATCH v1 0/5] git log: configurable default format for merge diffs

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> 2. We have descriptive long name for every other option, and it'd be an
>> exception if we'd have none for --diff-merges=m. In fact, it's
>> --diff-merges=m that could have been removed, but it'd break resemblance
>> with --cc and -c that both do have their --diff-merges=cc and
>> --diff-merges=c counterparts.
>
> Hmph, a devil's advocate in me suspects that it may just be arguing
> why user-configurable 'default' is a bad idea, though.

After you've said this I figured the option might have been simply
called --diff-merges=on. Recall that we already have --diff-merges=off.

Makes more sense than --diff-merges=default?

-- Sergey Organov

[PATCH v2 0/5] git log: configurable default format for merge diffs

[Sergey Organov]

These patches introduce capability to configure the default format of
output of diffs for merge commits by means of new log.diffMerges
configuration variable. The default format could be requested by the
new value "on" for --diff-merges option (--diff-merges=on).

Then -m and --diff-merges=m are also changed to use the default
format, in a backward compatible manner, as visible behavior doesn't
change unless user customizes log.diffMerges configuration.

In particular,

  git config log.diffMerges first-parent

will change -m option format from "separate" to "first-parent" that
will in turn cause, say,

  git show -m <merge_commit>

to output diff to the first parent only, instead of appending
typically large and surprising diff to the second parent at the end of
the output.

Updates in v2:

  * Renamed --diff-merges=default to --diff-merges=on. Junio didn't
    like the "default" here, and I agree. Dunno why I've even called
    it "default" in the first place.

Updates in v1:

  * Renamed abbreviated value "def" to full "default"

  * Fixed tests to use "test_config" instead of "git config"

  * Meld all "git config" changes into single commit that includes
    code, documentation, and tests, as they are mutually
    interdependent.

Signed-off-by: Sergey Organov <sorganov@gmail.com>

Sergey Organov (5):
  diff-merges: introduce --diff-merges=on
  diff-merges: refactor set_diff_merges()
  diff-merges: adapt -m to enable default diff format
  diff-merges: introduce log.diffMerges config variable
  doc/diff-options: document new --diff-merges features

 Documentation/config/log.txt   |  5 +++
 Documentation/diff-options.txt | 15 ++++++---
 builtin/log.c                  |  2 ++
 diff-merges.c                  | 58 ++++++++++++++++++++++++----------
 diff-merges.h                  |  2 ++
 t/t4013-diff-various.sh        | 31 ++++++++++++++++++
 t/t9902-completion.sh          |  3 ++
 7 files changed, 95 insertions(+), 21 deletions(-)

Interdiff against v1:
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 31e2bacf5252..6d968b9012dc 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|default|first-parent|1|separate|m|combined|c|dense-combined|cc)::
+--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc)::
 --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
@@ -45,7 +45,7 @@ ifdef::git-log[]
 	Disable output of diffs for merge commits. Useful to override
 	implied value.
 +
---diff-merges=default:::
+--diff-merges=on:::
 --diff-merges=m:::
 -m:::
 	This option makes diff output for merge commits to be shown in
diff --git a/diff-merges.c b/diff-merges.c
index 75630fb8e6b8..f3a9daed7e05 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -67,7 +67,7 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		return set_dense_combined;
-	else if (!strcmp(optarg, "m") || !strcmp(optarg, "default"))
+	else if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
 		return set_to_default;
 	return NULL;
 }
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 87cab7867135..87def81699bf 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -452,10 +452,10 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
-test_expect_success 'log --diff-merges=default matches --diff-merges=separate' '
+test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
 	git log -p --diff-merges=separate master >result &&
 	process_diffs result >expected &&
-	git log -p --diff-merges=default master >result &&
+	git log -p --diff-merges=on master >result &&
 	process_diffs result >actual &&
 	test_cmp expected actual
 '
@@ -469,7 +469,7 @@ test_expect_success 'git config log.diffMerges first-parent' '
 	git log -p --diff-merges=first-parent master >result &&
 	process_diffs result >expected &&
 	test_config log.diffMerges first-parent &&
-	git log -p --diff-merges=default master >result &&
+	git log -p --diff-merges=on master >result &&
 	process_diffs result >actual &&
 	test_cmp expected actual
 '
-- 
2.25.1

[PATCH v2 1/5] diff-merges: introduce --diff-merges=on

[Sergey Organov]

Introduce the notion of default diff format for merges, and the option
"on" to select it. The default format is "separate" and can't yet
be changed, so effectively "on" is just a synonym for "separate"
for now. Add corresponding test to t4013.

This is in preparation for introducing log.diffMerges configuration
option that will let --diff-merges=on to be configured to any
supported format.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c           | 7 +++++++
 t/t4013-diff-various.sh | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/diff-merges.c b/diff-merges.c
index 146bb50316a6..ff227368bd46 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -2,6 +2,11 @@
 
 #include "revision.h"
 
+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 void suppress(struct rev_info *revs)
 {
 	revs->separate_merges = 0;
@@ -66,6 +71,8 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
 		set_combined(revs);
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		set_dense_combined(revs);
+	else if (!strcmp(optarg, "on"))
+		set_to_default(revs);
 	else
 		die(_("unknown value for --diff-merges: %s"), optarg);
 
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 6cca8b84a6bf..26a7b4d19d4d 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
+test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
+	git log -p --diff-merges=separate master >result &&
+	process_diffs result >expected &&
+	git log -p --diff-merges=on master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
-- 
2.25.1

Re: [PATCH v2 1/5] diff-merges: introduce --diff-merges=on

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Introduce the notion of default diff format for merges, and the option
> "on" to select it. The default format is "separate" and can't yet
> be changed, so effectively "on" is just a synonym for "separate"
> for now. Add corresponding test to t4013.
>
> This is in preparation for introducing log.diffMerges configuration
> option that will let --diff-merges=on to be configured to any
> supported format.

"on"---that's short-and-sweet and really nice, compared to the "default"
in the previous iteration.

Clever.

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  diff-merges.c           | 7 +++++++
>  t/t4013-diff-various.sh | 8 ++++++++
>  2 files changed, 15 insertions(+)
>
> diff --git a/diff-merges.c b/diff-merges.c
> index 146bb50316a6..ff227368bd46 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -2,6 +2,11 @@
>  
>  #include "revision.h"
>  
> +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 void suppress(struct rev_info *revs)
>  {
>  	revs->separate_merges = 0;
> @@ -66,6 +71,8 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
>  		set_combined(revs);
>  	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
>  		set_dense_combined(revs);
> +	else if (!strcmp(optarg, "on"))
> +		set_to_default(revs);
>  	else
>  		die(_("unknown value for --diff-merges: %s"), optarg);
>  
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 6cca8b84a6bf..26a7b4d19d4d 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -452,6 +452,14 @@ diff-tree --stat --compact-summary initial mode
>  diff-tree -R --stat --compact-summary initial mode
>  EOF
>  
> +test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
> +	git log -p --diff-merges=separate master >result &&
> +	process_diffs result >expected &&
> +	git log -p --diff-merges=on master >result &&
> +	process_diffs result >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'log -S requires an argument' '
>  	test_must_fail git log -S
>  '

[PATCH v2 2/5] diff-merges: refactor set_diff_merges()

[Sergey Organov]

Split set_diff_merges() into separate parsing and execution functions,
the former to be reused for parsing of configuration values later in
the patch series.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index ff227368bd46..66c8ba0cc6a0 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -55,29 +55,35 @@ static void set_dense_combined(struct rev_info *revs)
 	revs->dense_combined_merges = 1;
 }
 
-static void set_diff_merges(struct rev_info *revs, const char *optarg)
+static diff_merges_setup_func_t func_by_opt(const char *optarg)
 {
-	if (!strcmp(optarg, "off") || !strcmp(optarg, "none")) {
-		suppress(revs);
-		/* Return early to leave revs->merges_need_diff unset */
-		return;
-	}
-
+	if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))
+		return suppress;
 	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
-		set_first_parent(revs);
+		return set_first_parent;
 	else if (!strcmp(optarg, "m") || !strcmp(optarg, "separate"))
-		set_separate(revs);
+		return set_separate;
 	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
-		set_combined(revs);
+		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
-		set_dense_combined(revs);
+		return set_dense_combined;
 	else if (!strcmp(optarg, "on"))
-		set_to_default(revs);
-	else
+		return set_to_default;
+	return NULL;
+}
+
+static void set_diff_merges(struct rev_info *revs, const char *optarg)
+{
+	diff_merges_setup_func_t func = func_by_opt(optarg);
+
+	if (!func)
 		die(_("unknown value for --diff-merges: %s"), optarg);
 
-	/* The flag is cleared by set_xxx() functions, so don't move this up */
-	revs->merges_need_diff = 1;
+	func(revs);
+
+	/* NOTE: the merges_need_diff flag is cleared by func() call */
+	if (func != suppress)
+		revs->merges_need_diff = 1;
 }
 
 /*
-- 
2.25.1

[PATCH v2 3/5] diff-merges: adapt -m to enable default diff format

[Sergey Organov]

Let -m option (and --diff-merges=m) enable the default format instead
of "separate", to be able to tune it with log.diffMerges option.

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

diff --git a/diff-merges.c b/diff-merges.c
index 66c8ba0cc6a0..9d19225b3ec9 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -34,10 +34,10 @@ static void set_m(struct rev_info *revs)
 {
 	/*
 	 * To "diff-index", "-m" means "match missing", and to the "log"
-	 * family of commands, it means "show full diff for merges". Set
+	 * family of commands, it means "show default diff for merges". Set
 	 * both fields appropriately.
 	 */
-	set_separate(revs);
+	set_to_default(revs);
 	revs->match_missing = 1;
 }
 
@@ -61,13 +61,13 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 		return suppress;
 	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
 		return set_first_parent;
-	else if (!strcmp(optarg, "m") || !strcmp(optarg, "separate"))
+	else if (!strcmp(optarg, "separate"))
 		return set_separate;
 	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
 		return set_combined;
 	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		return set_dense_combined;
-	else if (!strcmp(optarg, "on"))
+	else if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
 		return set_to_default;
 	return NULL;
 }
-- 
2.25.1

[PATCH v2 4/5] diff-merges: introduce log.diffMerges config variable

[Sergey Organov]

New log.diffMerges configuration variable sets the format that
--diff-merges=on will be using. The default is "separate".

t4013: add the following tests for log.diffMerges config:

* Test that wrong values are denied.

* Test that the value of log.diffMerges properly affects both
--diff-merges=on and -m.

t9902: fix completion tests for log.d* to match log.diffMerges.

Added documentation for log.diffMerges.

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

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 208d5fdcaa68..456eb07800cb 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -24,6 +24,11 @@ log.excludeDecoration::
 	the config option can be overridden by the `--decorate-refs`
 	option.
 
+log.diffMerges::
+	Set default diff format to be used for merge commits. See
+	`--diff-merges` in linkgit:git-log[1] for details.
+	Defaults to `separate`.
+
 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 8acd285dafd8..6102893fccb9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -481,6 +481,8 @@ static int git_log_config(const char *var, const char *value, void *cb)
 			decoration_style = 0; /* maybe warn? */
 		return 0;
 	}
+	if (!strcmp(var, "log.diffmerges"))
+		return diff_merges_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 9d19225b3ec9..f3a9daed7e05 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -90,6 +90,17 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
  * Public functions. They are in the order they are called.
  */
 
+int diff_merges_config(const char *value)
+{
+	diff_merges_setup_func_t func = func_by_opt(value);
+
+	if (!func)
+		return -1;
+
+	set_to_default = func;
+	return 0;
+}
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 {
 	int argcount = 1;
diff --git a/diff-merges.h b/diff-merges.h
index 659467c99a4f..09d9a6c9a4fb 100644
--- a/diff-merges.h
+++ b/diff-merges.h
@@ -9,6 +9,8 @@
 
 struct rev_info;
 
+int diff_merges_config(const char *value);
+
 int diff_merges_parse_opts(struct rev_info *revs, const char **argv);
 
 void diff_merges_suppress(struct rev_info *revs);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 26a7b4d19d4d..87def81699bf 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -460,6 +460,29 @@ test_expect_success 'log --diff-merges=on matches --diff-merges=separate' '
 	test_cmp expected actual
 '
 
+test_expect_success 'deny wrong log.diffMerges config' '
+	test_config log.diffMerges wrong-value &&
+	test_expect_code 128 git log
+'
+
+test_expect_success 'git config log.diffMerges first-parent' '
+	git log -p --diff-merges=first-parent master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges first-parent &&
+	git log -p --diff-merges=on master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git config log.diffMerges first-parent vs -m' '
+	git log -p --diff-merges=first-parent master >result &&
+	process_diffs result >expected &&
+	test_config log.diffMerges first-parent &&
+	git log -p -m master >result &&
+	process_diffs result >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'log -S requires an argument' '
 	test_must_fail git log -S
 '
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 04ce884ef5ac..4d732d6d4f81 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
 	test_completion "git config log.d" <<-\EOF
 	log.date Z
 	log.decorate Z
+	log.diffMerges Z
 	EOF
 '
 
@@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
 	test_completion "git -c log.d" <<-\EOF
 	log.date=Z
 	log.decorate=Z
+	log.diffMerges=Z
 	EOF
 '
 
@@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
 	test_completion "git clone --config=log.d" <<-\EOF
 	log.date=Z
 	log.decorate=Z
+	log.diffMerges=Z
 	EOF
 '
 
-- 
2.25.1

Re: [PATCH v2 4/5] diff-merges: introduce log.diffMerges config variable

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 04ce884ef5ac..4d732d6d4f81 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
>  	test_completion "git config log.d" <<-\EOF
>  	log.date Z
>  	log.decorate Z
> +	log.diffMerges Z
>  	EOF
>  '
>  
> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
>  	test_completion "git -c log.d" <<-\EOF
>  	log.date=Z
>  	log.decorate=Z
> +	log.diffMerges=Z
>  	EOF
>  '
>  
> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
>  	test_completion "git clone --config=log.d" <<-\EOF
>  	log.date=Z
>  	log.decorate=Z
> +	log.diffMerges=Z
>  	EOF
>  '

$ sh ./t9902-completion.sh -i -v

ends like the attached.  Is there a prerequisite patch I am missing,
or something?


ok 195 - git config - section

expecting success of 9902.196 'git config - variable name':
        test_completion "git config log.d" <<-\EOF
        log.date Z
        log.decorate Z
        log.diffMerges Z
        EOF

--- expected    2021-04-15 20:20:09.652861741 +0000
+++ out_sorted  2021-04-15 20:20:09.660862400 +0000
@@ -1,3 +1,2 @@
 log.date
 log.decorate
-log.diffMerges
not ok 196 - git config - variable name
#
#               test_completion "git config log.d" <<-\EOF
#               log.date Z
#               log.decorate Z
#               log.diffMerges Z
#               EOF
#
:

Re: [PATCH v2 4/5] diff-merges: introduce log.diffMerges config variable

[Sergey Organov]

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 04ce884ef5ac..4d732d6d4f81 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -2306,6 +2306,7 @@ test_expect_success 'git config - variable name' '
>>  	test_completion "git config log.d" <<-\EOF
>>  	log.date Z
>>  	log.decorate Z
>> +	log.diffMerges Z
>>  	EOF
>>  '
>>  
>> @@ -2327,6 +2328,7 @@ test_expect_success 'git -c - variable name' '
>>  	test_completion "git -c log.d" <<-\EOF
>>  	log.date=Z
>>  	log.decorate=Z
>> +	log.diffMerges=Z
>>  	EOF
>>  '
>>  
>> @@ -2348,6 +2350,7 @@ test_expect_success 'git clone --config= - variable name' '
>>  	test_completion "git clone --config=log.d" <<-\EOF
>>  	log.date=Z
>>  	log.decorate=Z
>> +	log.diffMerges=Z
>>  	EOF
>>  '
>
> $ sh ./t9902-completion.sh -i -v
>
> ends like the attached.  Is there a prerequisite patch I am missing,
> or something?

To me these completion tests don't work as expected unless I "make
install; make test" resulting Git version rather than simply "make
tests".

I have been told this by SZEDER Gábor <szeder.dev@gmail.com> earlier in
the discussion of these patch series:

<quote>
It might be related to a bug in the build process that doesn't update
that auto-generated list of supported configuration variables after
e.g. 'Documentation/config/log.txt' was modified; see a proposed fix
at:

  https://public-inbox.org/git/20210408212915.3060286-1-szeder.dev@gmail.com/

</quote>

Looks like that's it?

For reference, I've checked all the commits in the series with this
script:

#!/usr/bin/env bash

while read -r rev; do
    git checkout "$rev"
    make clean > /dev/null 2>&1
    commit=$(git log --oneline --abbrev=6 -n1 $rev)
    echo "Make $commit"
    if ! make prefix=$HOME/git -j8 all >/dev/null 2>&1; then
       >&2 echo "Make for commit $rev failed"
       exit 1
    fi
    echo "Install $commit"
    if ! make prefix=$HOME/git install>/dev/null 2>&1; then
       >&2 echo "Install for commit $rev failed"
       exit 2
    fi
    echo "Test $commit"
    if ! make test; then
        >&2 echo "Test for commit $rev failed"
        exit 3
    fi
done < <(git rev-list --reverse "$1")

-- Sergey Organov

[PATCH v2 5/5] doc/diff-options: document new --diff-merges features

[Sergey Organov]

Document changes in -m and --diff-merges=m semantics, as well as new
--diff-merges=on option.

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

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index aa2b5c11f20b..6d968b9012dc 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|first-parent|1|separate|m|combined|c|dense-combined|cc)::
+--diff-merges=(off|none|on|first-parent|1|separate|m|combined|c|dense-combined|cc)::
 --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
@@ -45,17 +45,24 @@ ifdef::git-log[]
 	Disable output of diffs for merge commits. Useful to override
 	implied value.
 +
+--diff-merges=on:::
+--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
+	`log.diffMerges` configuration parameter, which default value
+	is `separate`.
++
 --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:::
---diff-merges=m:::
--m:::
 	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. `-m` doesn't produce any output without `-p`.
+	for each parent.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
-- 
2.25.1