diff mbox

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

Message ID 20210410171657.20159-1-sorganov@gmail.com (mailing list archive)
State New
Headers show

Commit Message

Sergey Organov April 10, 2021, 5:16 p.m. UTC
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:

Comments

Junio C Hamano April 11, 2021, 4:13 p.m. UTC | #1
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.
Sergey Organov April 11, 2021, 6:04 p.m. UTC | #2
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
Junio C Hamano April 11, 2021, 7:02 p.m. UTC | #3
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.
Sergey Organov April 11, 2021, 8:38 p.m. UTC | #4
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
Sergey Organov April 11, 2021, 9:58 p.m. UTC | #5
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
diff mbox

Patch

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
 '