Re: [PATCH v2 0/7] making log --first-parent imply -m

[Sergey Organov <sorganov@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

Jeff King <peff@peff.net> writes:

> It's too late for "-m" to change semantics (we could do a long
> deprecation, but I don't see much point in doing so). But --diff-merges
> is definitely still changeable until we release v2.29. My resistance was
> mostly that I didn't want to complicate my series by adding new
> elements. But we could do something on top.

Attached is rather minimal incompatible change to --diff-merges that'd
allow extensions in the future, to get out of urge for the discussed
changes. I'm going to follow-up with actual improvements and I'm aware
it lacks documentation changes.

What do you think, is it OK to have something like this before v2.29?
And, by the way, what's approximate timeline to v2.29?

As for me, I'm not sure 'combined-all-paths' should be included and if
it should, is it a good enough name. In addition, do we need more
descriptive (additional) names for "c" and "cc" modes?

-- Sergey


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-revision-change-diff-merges-option-to-require-parame.patch --]
[-- Type: text/x-patch, Size: 2282 bytes --]

From b75e410797d4576e266d056ece16acf07e4b0116 Mon Sep 17 00:00:00 2001
From: Sergey Organov <sorganov@gmail.com>
Date: Tue, 4 Aug 2020 16:48:27 +0300
Subject: [PATCH RFC] revision: change "--diff-merges" option to require
 parameter

Make --diff-merges require parameter having one of the following values:

  off, all, c, cc, combined-all-paths

that are equivalents of passing the following separate options, respectively:

  --no-diff-merges, -m, -c, -cc, --combined-all-paths

that, except --no-diff-merges, could be deprecated later.

This gives us single option that entirely defines how merge commits are to be
represented.

This patch is a preparation for supporting --diff-merges=<parent>, where
<parent> is parent number to provide diff against, that adds new essential
functionality and therefore is a separate change.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 revision.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 669bc856694f..dcdff59bc36a 100644
--- a/revision.c
+++ b/revision.c
@@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->diff = 1;
 		revs->diffopt.flags.recursive = 1;
 		revs->diffopt.flags.tree_in_recursive = 1;
-	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
+	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
 		revs->ignore_merges = 0;
+		if (!strcmp(optarg, "off")) {
+			revs->ignore_merges = 1;
+		} else if (!strcmp(optarg, "all")) {
+			revs->diff = 0;
+		} else if (!strcmp(optarg, "c")) {
+			revs->diff = 1;
+			revs->dense_combined_merges = 0;
+			revs->combine_merges = 1;
+		} else if (!strcmp(optarg, "cc")) {
+			revs->diff = 1;
+			revs->dense_combined_merges = 1;
+			revs->combine_merges = 1;
+		} else if (!strcmp(optarg, "combined-all-paths")) {
+			revs->diff = 1;
+			revs->combined_all_paths = 1;
+		} else {
+			die("--diff-merges: unknown value '%s'.", optarg);
+		}
+		return argcount;
 	} else if (!strcmp(arg, "--no-diff-merges")) {
 		revs->ignore_merges = 1;
+	} else if (!strcmp(arg, "-m")) {
+		revs->ignore_merges = 0;
 	} else if (!strcmp(arg, "-c")) {
 		revs->diff = 1;
 		revs->dense_combined_merges = 0;
-- 
2.20.1