git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH 0/3] making --first-parent imply -m
@ 2020-07-28 16:36 Jeff King
  2020-07-28 16:36 ` [PATCH 1/3] log: drop "--cc implies -m" logic Jeff King
                   ` (6 more replies)
  0 siblings, 7 replies; 73+ messages in thread
From: Jeff King @ 2020-07-28 16:36 UTC (permalink / raw)
  To: git

For some projects, it's useful to walk the first parent history, looking
at each merge commit as a normal commit introducing all of the changes
no its side branch. E.g.:

  git log --first-parent -m -Sfoo -p

might show you the topic or pull request that introduced code "foo". But
I quite often forget to add "-m", and get confused that it doesn't
return any results.

This series just makes --first-parent imply -m. That doesn't change any
output by itself, but does mean that diff options like "-p", "-S", etc,
behave sensibly.

  [1/3]: log: drop "--cc implies -m" logic
  [2/3]: revision: add "--ignore-merges" option to counteract "-m"
  [3/3]: log: enable "-m" automatically with "--first-parent"

 Documentation/rev-list-options.txt            |  1 +
 builtin/log.c                                 |  7 +-
 revision.c                                    | 10 ++-
 revision.h                                    |  2 +-
 t/t4013-diff-various.sh                       |  1 +
 ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++
 t/t4013/diff.log_-p_--first-parent_master     | 22 ++++++
 7 files changed, 113 insertions(+), 8 deletions(-)
 create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 1/3] log: drop "--cc implies -m" logic
  2020-07-28 16:36 [PATCH 0/3] making --first-parent imply -m Jeff King
@ 2020-07-28 16:36 ` Jeff King
  2020-07-28 16:38 ` [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m" Jeff King
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-28 16:36 UTC (permalink / raw)
  To: git

This was added by 82dee4160c (log: show merge commit when --cc is given,
2015-08-20), which explains why we need it. But that commit failed to
notice that setup_revisions() already does the same thing, since
cd2bdc5309 (Common option parsing for "git log --diff" and friends,
2006-04-14).

Signed-off-by: Jeff King <peff@peff.net>
---
Just a cleanup that would be worth taking regardless of the rest of the
series.

 builtin/log.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index d104d5c688..281d2ae8eb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -731,10 +731,6 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
 	/* Turn --cc/-c into -p --cc/-c when -p was not given */
 	if (!rev->diffopt.output_format && rev->combine_merges)
 		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
-
-	/* Turn -m on when --cc/-c was given */
-	if (rev->combine_merges)
-		rev->ignore_merges = 0;
 }
 
 int cmd_log(int argc, const char **argv, const char *prefix)
-- 
2.28.0.rc2.475.g53c7e1c7f4


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m"
  2020-07-28 16:36 [PATCH 0/3] making --first-parent imply -m Jeff King
  2020-07-28 16:36 ` [PATCH 1/3] log: drop "--cc implies -m" logic Jeff King
@ 2020-07-28 16:38 ` Jeff King
  2020-07-28 17:52   ` Chris Torek
  2020-07-28 21:01   ` Junio C Hamano
  2020-07-28 16:39 ` [PATCH 3/3] log: enable "-m" automatically with "--first-parent" Jeff King
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 73+ messages in thread
From: Jeff King @ 2020-07-28 16:38 UTC (permalink / raw)
  To: git

The "-m" option sets revs->ignore_merges to "0", but there's no way to
undo it. This probably isn't something anybody overly cares about, since
"1" is already the default, but it will serve as an escape hatch when we
flip the default for ignore_merges to "0" in more situations.

We'll also add a few extra niceties:

  - initialize the value to "-1" to indicate "not set", and then resolve
    it to the normal 0/1 bool in setup_revisions(). This lets any tweak
    functions, as well as setup_revisions() itself, avoid clobbering the
    user's preference (which until now they couldn't actually express).

  - since we now have --ignore-merges, let's add the matching
    --no-ignore-merges, which is just a synonym for "-m". In fact, it's
    simpler to just document --no-ignore-merges alongside "-m", and
    leave it implied that its opposite countermands it.

The new test shows that this behaves just the same as the current
behavior without "-m".

Signed-off-by: Jeff King <peff@peff.net>
---
I pulled the option name from the rev_info field name. It might be too
broad (we are not ignoring merges during the traversal, only for the
diff). It could be "--no-diff-merges" or something, but that would
involve flipping the sense of the boolean (but that would just be in the
code, not user-visible, so not that big a deal).

 Documentation/rev-list-options.txt            |  1 +
 builtin/log.c                                 |  4 +-
 revision.c                                    | 10 ++-
 revision.h                                    |  2 +-
 t/t4013-diff-various.sh                       |  1 +
 ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++
 6 files changed, 90 insertions(+), 6 deletions(-)
 create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index b01b2b6773..fbd8fa0381 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -1148,6 +1148,7 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	rename or copy detection have been requested).
 
 -m::
+--no-ignore-merges::
 	This flag makes the merge commits show the full diff like
 	regular commits; for each merge parent, a separate log entry
 	and diff is generated. An exception is that only diff against
diff --git a/builtin/log.c b/builtin/log.c
index 281d2ae8eb..39b3d773a9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -599,8 +599,8 @@ static int show_tree_object(const struct object_id *oid,
 static void show_setup_revisions_tweak(struct rev_info *rev,
 				       struct setup_revision_opt *opt)
 {
-	if (rev->ignore_merges) {
-		/* There was no "-m" on the command line */
+	if (rev->ignore_merges < 0) {
+		/* There was no "-m" variant on the command line */
 		rev->ignore_merges = 0;
 		if (!rev->first_parent_only && !rev->combine_merges) {
 			/* No "--first-parent", "-c", or "--cc" */
diff --git a/revision.c b/revision.c
index 6aa7f4f567..a36c4eaf26 100644
--- a/revision.c
+++ b/revision.c
@@ -1795,7 +1795,7 @@ void repo_init_revisions(struct repository *r,
 
 	revs->repo = r;
 	revs->abbrev = DEFAULT_ABBREV;
-	revs->ignore_merges = 1;
+	revs->ignore_merges = -1;
 	revs->simplify_history = 1;
 	revs->pruning.repo = r;
 	revs->pruning.flags.recursive = 1;
@@ -2323,8 +2323,10 @@ 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")) {
+	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--no-ignore-merges")) {
 		revs->ignore_merges = 0;
+	} else if (!strcmp(arg, "--ignore-merges")) {
+		revs->ignore_merges = 1;
 	} else if (!strcmp(arg, "-c")) {
 		revs->diff = 1;
 		revs->dense_combined_merges = 0;
@@ -2834,8 +2836,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			copy_pathspec(&revs->diffopt.pathspec,
 				      &revs->prune_data);
 	}
-	if (revs->combine_merges)
+	if (revs->combine_merges && revs->ignore_merges < 0)
 		revs->ignore_merges = 0;
+	if (revs->ignore_merges < 0)
+		revs->ignore_merges = 1;
 	if (revs->combined_all_paths && !revs->combine_merges)
 		die("--combined-all-paths makes no sense without -c or --cc");
 
diff --git a/revision.h b/revision.h
index f412ae85eb..5258024743 100644
--- a/revision.h
+++ b/revision.h
@@ -190,11 +190,11 @@ struct rev_info {
 			show_root_diff:1,
 			no_commit_id:1,
 			verbose_header:1,
-			ignore_merges:1,
 			combine_merges:1,
 			combined_all_paths:1,
 			dense_combined_merges:1,
 			always_show_header:1;
+	int             ignore_merges:2;
 
 	/* Format info */
 	int		show_notes;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 43267d6024..8f9181316f 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -297,6 +297,7 @@ log --root --patch-with-stat --summary master
 log --root -c --patch-with-stat --summary master
 # improved by Timo's patch
 log --root --cc --patch-with-stat --summary master
+log --ignore-merges -p --first-parent master
 log -p --first-parent master
 log -m -p --first-parent master
 log -m -p master
diff --git a/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
new file mode 100644
index 0000000000..fa0cdc8a23
--- /dev/null
+++ b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
@@ -0,0 +1,78 @@
+$ git log --ignore-merges -p --first-parent master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side' into master
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+diff --git a/dir/sub b/dir/sub
+index 8422d40..cead32e 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -2,3 +2,5 @@ A
+ B
+ C
+ D
++E
++F
+diff --git a/file1 b/file1
+new file mode 100644
+index 0000000..b1e6722
+--- /dev/null
++++ b/file1
+@@ -0,0 +1,3 @@
++A
++B
++C
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+diff --git a/dir/sub b/dir/sub
+index 35d242b..8422d40 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++C
++D
+diff --git a/file0 b/file0
+index 01e79c3..b414108 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++4
++5
++6
+diff --git a/file2 b/file2
+deleted file mode 100644
+index 01e79c3..0000000
+--- a/file2
++++ /dev/null
+@@ -1,3 +0,0 @@
+-1
+-2
+-3
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.28.0.rc2.475.g53c7e1c7f4


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH 3/3] log: enable "-m" automatically with "--first-parent"
  2020-07-28 16:36 [PATCH 0/3] making --first-parent imply -m Jeff King
  2020-07-28 16:36 ` [PATCH 1/3] log: drop "--cc implies -m" logic Jeff King
  2020-07-28 16:38 ` [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m" Jeff King
@ 2020-07-28 16:39 ` Jeff King
  2020-07-28 16:47 ` [PATCH 0/3] making --first-parent imply -m Jeff King
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-28 16:39 UTC (permalink / raw)
  To: git

When using "--first-parent" to consider history as a single line of
commits, git-log still defaults to treating merges specially, even
though they could be considered as single commits in the linearized
history (that just introduce all of the changes from the second and
higher parents).

Let's instead have "--first-parent" imply "-m", which makes something
like:

  git log --first-parent -p

do what you'd expect. Likewise:

  git log --first-parent -Sfoo

will find "foo" in merge commits.

No new test is needed; we'll tweak the output of the existing
"--first-parent -p" test, which now matches the "-m --first-parent -p"
test. The unchanged existing test for "--ignore-merges" confirms that
the user can get the old behavior if they want.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c                             |  3 +++
 t/t4013/diff.log_-p_--first-parent_master | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index 39b3d773a9..83b147c23a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -731,6 +731,9 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
 	/* Turn --cc/-c into -p --cc/-c when -p was not given */
 	if (!rev->diffopt.output_format && rev->combine_merges)
 		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
+
+	if (rev->first_parent_only && rev->ignore_merges < 0)
+		rev->ignore_merges = 0;
 }
 
 int cmd_log(int argc, const char **argv, const char *prefix)
diff --git a/t/t4013/diff.log_-p_--first-parent_master b/t/t4013/diff.log_-p_--first-parent_master
index c6a5876d80..fe044399f0 100644
--- a/t/t4013/diff.log_-p_--first-parent_master
+++ b/t/t4013/diff.log_-p_--first-parent_master
@@ -6,6 +6,28 @@ Date:   Mon Jun 26 00:04:00 2006 +0000
 
     Merge branch 'side' into master
 
+diff --git a/dir/sub b/dir/sub
+index cead32e..992913c 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -4,3 +4,5 @@ C
+ D
+ E
+ F
++1
++2
+diff --git a/file0 b/file0
+index b414108..10a8a9f 100644
+--- a/file0
++++ b/file0
+@@ -4,3 +4,6 @@
+ 4
+ 5
+ 6
++A
++B
++C
+
 commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:02:00 2006 +0000
-- 
2.28.0.rc2.475.g53c7e1c7f4

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 0/3] making --first-parent imply -m
  2020-07-28 16:36 [PATCH 0/3] making --first-parent imply -m Jeff King
                   ` (2 preceding siblings ...)
  2020-07-28 16:39 ` [PATCH 3/3] log: enable "-m" automatically with "--first-parent" Jeff King
@ 2020-07-28 16:47 ` Jeff King
  2020-07-28 16:48 ` Junio C Hamano
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-28 16:47 UTC (permalink / raw)
  To: git

On Tue, Jul 28, 2020 at 12:36:18PM -0400, Jeff King wrote:

> For some projects, it's useful to walk the first parent history, looking
> at each merge commit as a normal commit introducing all of the changes
> no its side branch. E.g.:
> 
>   git log --first-parent -m -Sfoo -p

One thing I should have mentioned, since the subject may be misleading:
this is only for git-log, not for plumbing like rev-list (which anyway
would need the option in diff-tree). The commits themselves are clear
that this is the case, but the cover letter was not. :)

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 0/3] making --first-parent imply -m
  2020-07-28 16:36 [PATCH 0/3] making --first-parent imply -m Jeff King
                   ` (3 preceding siblings ...)
  2020-07-28 16:47 ` [PATCH 0/3] making --first-parent imply -m Jeff King
@ 2020-07-28 16:48 ` Junio C Hamano
  2020-07-28 16:53   ` Jeff King
  2020-07-28 16:55 ` Taylor Blau
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
  6 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-07-28 16:48 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> For some projects, it's useful to walk the first parent history, looking
> at each merge commit as a normal commit introducing all of the changes
> no its side branch. E.g.:
>
>   git log --first-parent -m -Sfoo -p
>
> might show you the topic or pull request that introduced code "foo". But
> I quite often forget to add "-m", and get confused that it doesn't
> return any results.

Yes.  I agree that --first-parent should imply -m when combined with
diff options like -p, --raw, etc.  I am not sure if -m should kick
in without any diff options, though.  Doesn't it have side effects?

Thanks.


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 0/3] making --first-parent imply -m
  2020-07-28 16:48 ` Junio C Hamano
@ 2020-07-28 16:53   ` Jeff King
  0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-28 16:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Jul 28, 2020 at 09:48:20AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > For some projects, it's useful to walk the first parent history, looking
> > at each merge commit as a normal commit introducing all of the changes
> > no its side branch. E.g.:
> >
> >   git log --first-parent -m -Sfoo -p
> >
> > might show you the topic or pull request that introduced code "foo". But
> > I quite often forget to add "-m", and get confused that it doesn't
> > return any results.
> 
> Yes.  I agree that --first-parent should imply -m when combined with
> diff options like -p, --raw, etc.  I am not sure if -m should kick
> in without any diff options, though.  Doesn't it have side effects?

I couldn't find any. It impacts log_tree_diff() if we see a commit with
two parents, but --first-parent will already override that.

I poked around in the code and couldn't find any other cases that would
be impacted, nor do any tests fail. Definitely if you can find a case
where it matters outside of the diff, I'd be interested to see it. :)

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 0/3] making --first-parent imply -m
  2020-07-28 16:36 [PATCH 0/3] making --first-parent imply -m Jeff King
                   ` (4 preceding siblings ...)
  2020-07-28 16:48 ` Junio C Hamano
@ 2020-07-28 16:55 ` Taylor Blau
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
  6 siblings, 0 replies; 73+ messages in thread
From: Taylor Blau @ 2020-07-28 16:55 UTC (permalink / raw)
  To: Jeff King; +Cc: git

On Tue, Jul 28, 2020 at 12:36:17PM -0400, Jeff King wrote:
> For some projects, it's useful to walk the first parent history, looking
> at each merge commit as a normal commit introducing all of the changes
> no its side branch. E.g.:
>
>   git log --first-parent -m -Sfoo -p
>
> might show you the topic or pull request that introduced code "foo". But
> I quite often forget to add "-m", and get confused that it doesn't
> return any results.
>
> This series just makes --first-parent imply -m. That doesn't change any
> output by itself, but does mean that diff options like "-p", "-S", etc,
> behave sensibly.
>
>   [1/3]: log: drop "--cc implies -m" logic
>   [2/3]: revision: add "--ignore-merges" option to counteract "-m"
>   [3/3]: log: enable "-m" automatically with "--first-parent"
>
>  Documentation/rev-list-options.txt            |  1 +
>  builtin/log.c                                 |  7 +-
>  revision.c                                    | 10 ++-
>  revision.h                                    |  2 +-
>  t/t4013-diff-various.sh                       |  1 +
>  ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++
>  t/t4013/diff.log_-p_--first-parent_master     | 22 ++++++
>  7 files changed, 113 insertions(+), 8 deletions(-)
>  create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
>
> -Peff

Very handy to have. The first patch looks good and could be taken
independently. The other two patches look fine to me, too, and it's
behavior that at least I would want.

So, the whole series can have my:

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m"
  2020-07-28 16:38 ` [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m" Jeff King
@ 2020-07-28 17:52   ` Chris Torek
  2020-07-29 18:42     ` Jeff King
  2020-07-28 21:01   ` Junio C Hamano
  1 sibling, 1 reply; 73+ messages in thread
From: Chris Torek @ 2020-07-28 17:52 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List

First, not that anyone should particularly care: I heartily
approve. :-)

On Tue, Jul 28, 2020 at 9:40 AM Jeff King <peff@peff.net> wrote:
> I pulled the option name from the rev_info field name. It might be too
> broad (we are not ignoring merges during the traversal, only for the
> diff). It could be "--no-diff-merges" or something, but that would
> involve flipping the sense of the boolean (but that would just be in the
> code, not user-visible, so not that big a deal).

Perhaps a bit bikesheddy, but I would suggest some clarifying
notes in the documentation.  The fact that `git log` *follows*
merges by default is pretty obvious, but the fact that it doesn't
*diff them at all* by default appears to be surprising to most Git
newcomers.  (It was to me, so many years ago, and I see this all
the time on stackoverflow.)

  Note that --ignore-merges / --no-ignore-merges affect
  only diff generation, not commit traversal.  Note
  also that by default, "git log -p" does not generate
  diffs for merges except when using --first-parent.
  See also the -c and --cc options and note that the
  default for "git show" is "--cc".

(The "except when using --first-parent" is of course new here.)

This probably needs a bit of tweaking, but the big idea is
to communicate and emphasize three things:

 1. "git log -p" and "git show" behave differently by default.
    It's a surprise, so we should call it out separately.

 2. The default for "git log -p" is no diff at all for merge
    commits, so see -c / --cc when you're looking at -m.

 3. The default for "git show" is "--cc".  (This note doesn't
    really belong here; perhaps it should be separately
    listed under the -c and --cc options?)

Chris

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m"
  2020-07-28 16:38 ` [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m" Jeff King
  2020-07-28 17:52   ` Chris Torek
@ 2020-07-28 21:01   ` Junio C Hamano
  2020-07-29 18:22     ` Jeff King
  1 sibling, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-07-28 21:01 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> The "-m" option sets revs->ignore_merges to "0", but there's no way to
> undo it. This probably isn't something anybody overly cares about, since
> "1" is already the default, but it will serve as an escape hatch when we
> flip the default for ignore_merges to "0" in more situations.
>
> We'll also add a few extra niceties:
>
>   - initialize the value to "-1" to indicate "not set", and then resolve
>     it to the normal 0/1 bool in setup_revisions(). This lets any tweak
>     functions, as well as setup_revisions() itself, avoid clobbering the
>     user's preference (which until now they couldn't actually express).
>
>   - since we now have --ignore-merges, let's add the matching
>     --no-ignore-merges, which is just a synonym for "-m". In fact, it's
>     simpler to just document --no-ignore-merges alongside "-m", and
>     leave it implied that its opposite countermands it.
>
> The new test shows that this behaves just the same as the current
> behavior without "-m".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I pulled the option name from the rev_info field name. It might be too
> broad (we are not ignoring merges during the traversal, only for the
> diff). It could be "--no-diff-merges" or something, but that would
> involve flipping the sense of the boolean (but that would just be in the
> code, not user-visible, so not that big a deal).
>
>  Documentation/rev-list-options.txt            |  1 +
>  builtin/log.c                                 |  4 +-
>  revision.c                                    | 10 ++-
>  revision.h                                    |  2 +-
>  t/t4013-diff-various.sh                       |  1 +
>  ...g_--ignore-merges_-p_--first-parent_master | 78 +++++++++++++++++++
>  6 files changed, 90 insertions(+), 6 deletions(-)
>  create mode 100644 t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index b01b2b6773..fbd8fa0381 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -1148,6 +1148,7 @@ options may be given. See linkgit:git-diff-files[1] for more options.
>  	rename or copy detection have been requested).
>  
>  -m::
> +--no-ignore-merges::

This invites a natural "does --ignore-merges exist, and if so what
does it do?"  Why not to have "--[no-]ignore-merges" as a separate
entry immediately after the existing "-m" instead?

>  	This flag makes the merge commits show the full diff like
>  	regular commits; for each merge parent, a separate log entry
>  	and diff is generated. An exception is that only diff against

That is,

	--[no-]ignore-merges::

		`--no-ignore-merges` is a synonym to `-m`.
		`--ignore-merges` countermands an earlier `-m` that
		is either explicitly or implicitly given.

or something along the line, perhaps?

> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 43267d6024..8f9181316f 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -297,6 +297,7 @@ log --root --patch-with-stat --summary master
>  log --root -c --patch-with-stat --summary master
>  # improved by Timo's patch
>  log --root --cc --patch-with-stat --summary master
> +log --ignore-merges -p --first-parent master

This explicitly says "I do not want to see diff for merges" ...

>  log -p --first-parent master
>  log -m -p --first-parent master
>  log -m -p master
> diff --git a/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
> new file mode 100644
> index 0000000000..fa0cdc8a23
> --- /dev/null
> +++ b/t/t4013/diff.log_--ignore-merges_-p_--first-parent_master
> @@ -0,0 +1,78 @@
> +$ git log --ignore-merges -p --first-parent master
> +commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
> +Merge: 9a6d494 c7a2ab9
> +Author: A U Thor <author@example.com>
> +Date:   Mon Jun 26 00:04:00 2006 +0000
> +
> +    Merge branch 'side' into master

... and that is honored here?  Good.


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m"
  2020-07-28 21:01   ` Junio C Hamano
@ 2020-07-29 18:22     ` Jeff King
  2020-07-29 18:30       ` Jeff King
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-07-29 18:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Jul 28, 2020 at 02:01:27PM -0700, Junio C Hamano wrote:

> >  -m::
> > +--no-ignore-merges::
> 
> This invites a natural "does --ignore-merges exist, and if so what
> does it do?"  Why not to have "--[no-]ignore-merges" as a separate
> entry immediately after the existing "-m" instead?

I was hoping it would all be implied and I could dodge those questions.
But it seems not. :)

After thinking on it more, I flipped it to:

  -m::
  --diff-merges::
     [existing text...]

and then I don't think we need to have another block for
--no-diff-merges.

I'll likewise add a statement that "-m" is implied by "--first-parent"
and can be counteracted with the "--no" form, which I think should spell
out all the implications of the series.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m"
  2020-07-29 18:22     ` Jeff King
@ 2020-07-29 18:30       ` Jeff King
  2020-07-29 18:53         ` Junio C Hamano
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-07-29 18:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Wed, Jul 29, 2020 at 02:22:07PM -0400, Jeff King wrote:

> After thinking on it more, I flipped it to:
> 
>   -m::
>   --diff-merges::
>      [existing text...]
> 
> and then I don't think we need to have another block for
> --no-diff-merges.
> 
> I'll likewise add a statement that "-m" is implied by "--first-parent"
> and can be counteracted with the "--no" form, which I think should spell
> out all the implications of the series.

Hmm, I take that back. I tried adding this:

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 0785a0cfe9..41c859e63f 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -1154,7 +1154,9 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	and diff is generated. An exception is that only diff against
 	the first parent is shown when `--first-parent` option is given;
 	in that case, the output represents the changes the merge
-	brought _into_ the then-current branch.
+	brought _into_ the then-current branch. Note that
+	`--first-parent` implies `-m` if no combined-diff option is
+	enabled; you can use `--no-diff-merges` to override that.
 
 -r::
 	Show recursive diffs.

but then I'm left wondering: why would you ever want to override it? I
added the option as an escape hatch in case anybody really needed the
old behavior. But I have trouble imagining a scenario where that is the
case, and I wonder if it is even worth advertising.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m"
  2020-07-28 17:52   ` Chris Torek
@ 2020-07-29 18:42     ` Jeff King
  0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-29 18:42 UTC (permalink / raw)
  To: Chris Torek; +Cc: Git List

On Tue, Jul 28, 2020 at 10:52:14AM -0700, Chris Torek wrote:

> On Tue, Jul 28, 2020 at 9:40 AM Jeff King <peff@peff.net> wrote:
> > I pulled the option name from the rev_info field name. It might be too
> > broad (we are not ignoring merges during the traversal, only for the
> > diff). It could be "--no-diff-merges" or something, but that would
> > involve flipping the sense of the boolean (but that would just be in the
> > code, not user-visible, so not that big a deal).
> 
> Perhaps a bit bikesheddy, but I would suggest some clarifying
> notes in the documentation.  The fact that `git log` *follows*
> merges by default is pretty obvious, but the fact that it doesn't
> *diff them at all* by default appears to be surprising to most Git
> newcomers.  (It was to me, so many years ago, and I see this all
> the time on stackoverflow.)

Yeah, I agree this is a point of confusion. My hope is that the code
change makes some of the confusion go away without people having to read
the documentation.

But I think "git log -p" (or "git log -Sfoo") still remains as a point
of confusion. Possibly we should be using "--cc" by default there, too.
It's obviously more expensive, but I wonder how much that really matters
in practice. I'd rather not tie it in to this series, though, which I
think is more unambiguously helpful. :)

>   Note that --ignore-merges / --no-ignore-merges affect
>   only diff generation, not commit traversal.

Yeah, that would definitely reduce confusion, but I'm switching it to
--diff-merges (and flipping the boolean nature), which I think is even
better.

>   Note
>   also that by default, "git log -p" does not generate
>   diffs for merges except when using --first-parent.
>   See also the -c and --cc options and note that the
>   default for "git show" is "--cc".

I think this would be useful to have in the manpage, but not under this
option. It looks like we can expand on this under the "Diff Formatting"
section. That's still kind of buried, but is the first place that makes
any sense to me. I'll see if I can do a patch on top of my series.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m"
  2020-07-29 18:30       ` Jeff King
@ 2020-07-29 18:53         ` Junio C Hamano
  0 siblings, 0 replies; 73+ messages in thread
From: Junio C Hamano @ 2020-07-29 18:53 UTC (permalink / raw)
  To: Jeff King; +Cc: git

Jeff King <peff@peff.net> writes:

> On Wed, Jul 29, 2020 at 02:22:07PM -0400, Jeff King wrote:
>
>> After thinking on it more, I flipped it to:
>> 
>>   -m::
>>   --diff-merges::
>>      [existing text...]
>> 
>> and then I don't think we need to have another block for
>> --no-diff-merges.
>> 
>> I'll likewise add a statement that "-m" is implied by "--first-parent"
>> and can be counteracted with the "--no" form, which I think should spell
>> out all the implications of the series.
>
> Hmm, I take that back. I tried adding this:
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 0785a0cfe9..41c859e63f 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -1154,7 +1154,9 @@ options may be given. See linkgit:git-diff-files[1] for more options.
>  	and diff is generated. An exception is that only diff against
>  	the first parent is shown when `--first-parent` option is given;
>  	in that case, the output represents the changes the merge
> -	brought _into_ the then-current branch.
> +	brought _into_ the then-current branch. Note that
> +	`--first-parent` implies `-m` if no combined-diff option is
> +	enabled; you can use `--no-diff-merges` to override that.
>  
>  -r::
>  	Show recursive diffs.
>
> but then I'm left wondering: why would you ever want to override it? I
> added the option as an escape hatch in case anybody really needed the
> old behavior.

Old behaviour meaning "git log --first-parent -p master..next" would
mostly list commits without patches unless there is a single-parent
commit thrown in the first-parent chain?  I would imagine it would
be one way to locate and view reverts, cherry-picks and "oops I
botched the merge previously" fix-ups on 'next'.

I could add "--no-merges" to that command line but then the output
loses all the clues on when topics are merged in what order and show
only those single-parent commits, so it is not exactly the same
thing.  It loses a lot, and the current behaviour strikes a very
good balance for the use case.

So, if we make "-m" the default in some cases (like "--first-parent"),
we do need a way to toggle it off.

Thanks.


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH v2 0/7] making log --first-parent imply -m
  2020-07-28 16:36 [PATCH 0/3] making --first-parent imply -m Jeff King
                   ` (5 preceding siblings ...)
  2020-07-28 16:55 ` Taylor Blau
@ 2020-07-29 20:10 ` Jeff King
  2020-07-29 20:10   ` [PATCH v2 1/7] log: drop "--cc implies -m" logic Jeff King
                     ` (8 more replies)
  6 siblings, 9 replies; 73+ messages in thread
From: Jeff King @ 2020-07-29 20:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Chris Torek

On Tue, Jul 28, 2020 at 12:36:18PM -0400, Jeff King wrote:

> This series just makes --first-parent imply -m. That doesn't change any
> output by itself, but does mean that diff options like "-p", "-S", etc,
> behave sensibly.

Here's a re-roll taking into account the discussion so far:

  - the escape hatch option name is flipped to "--no-diff-merges" (with
    "--diff-merges" matching "-m")

  - the documentation now discusses the change as well as the existing
    handling of merges; this involved a few extra cleanups. Try
    "doc-diff" for a better view of the actual rendered changes.

    I do think longer term we'd be better off to stop having this maze
    of ifdef'd inclusions, and just have gitdiff(7) which covers all of
    the possibilities in human-readable text (so yes, you might see a
    mention of diff-files while you're looking for info on git-log, but
    that would also broaden your mind about how the different commands
    work). But that's clearly outside the scope of this series. I think
    what's here is a strict improvement.

Patches:

  [1/7]: log: drop "--cc implies -m" logic
  [2/7]: revision: add "--no-diff-merges" option to counteract "-m"
  [3/7]: log: enable "-m" automatically with "--first-parent"
  [4/7]: doc/git-log: move "Diff Formatting" from rev-list-options
  [5/7]: doc/git-log: drop "-r" diff option
  [6/7]: doc/git-log: move "-t" into diff-options list
  [7/7]: doc/git-log: clarify handling of merge commit diffs

 Documentation/diff-options.txt                |  5 ++
 Documentation/git-log.txt                     | 43 +++++++++-
 Documentation/rev-list-options.txt            | 45 -----------
 builtin/log.c                                 |  7 +-
 revision.c                                    | 10 ++-
 revision.h                                    |  2 +-
 t/t4013-diff-various.sh                       |  1 +
 ..._--no-diff-merges_-p_--first-parent_master | 78 +++++++++++++++++++
 t/t4013/diff.log_-p_--first-parent_master     | 22 ++++++
 9 files changed, 158 insertions(+), 55 deletions(-)
 create mode 100644 t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master

Range diff from v1:

1:  518deab41f = 1:  4277d82c0c log: drop "--cc implies -m" logic
2:  836553f54e ! 2:  78750f9054 revision: add "--ignore-merges" option to counteract "-m"
    @@ Metadata
     Author: Jeff King <peff@peff.net>
     
      ## Commit message ##
    -    revision: add "--ignore-merges" option to counteract "-m"
    +    revision: add "--no-diff-merges" option to counteract "-m"
     
         The "-m" option sets revs->ignore_merges to "0", but there's no way to
         undo it. This probably isn't something anybody overly cares about, since
    @@ Commit message
             functions, as well as setup_revisions() itself, avoid clobbering the
             user's preference (which until now they couldn't actually express).
     
    -      - since we now have --ignore-merges, let's add the matching
    -        --no-ignore-merges, which is just a synonym for "-m". In fact, it's
    -        simpler to just document --no-ignore-merges alongside "-m", and
    -        leave it implied that its opposite countermands it.
    +      - since we now have --no-diff-merges, let's add the matching
    +        --diff-merges, which is just a synonym for "-m". Then we don't even
    +        need to document --no-diff-merges separately; it countermands the
    +        long form of "-m" in the usual way.
     
         The new test shows that this behaves just the same as the current
         behavior without "-m".
    @@ Documentation/rev-list-options.txt: options may be given. See linkgit:git-diff-f
      	rename or copy detection have been requested).
      
      -m::
    -+--no-ignore-merges::
    ++--diff-merges::
      	This flag makes the merge commits show the full diff like
      	regular commits; for each merge parent, a separate log entry
      	and diff is generated. An exception is that only diff against
    @@ revision.c: static int handle_revision_opt(struct rev_info *revs, int argc, cons
      		revs->diffopt.flags.recursive = 1;
      		revs->diffopt.flags.tree_in_recursive = 1;
     -	} else if (!strcmp(arg, "-m")) {
    -+	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--no-ignore-merges")) {
    ++	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
      		revs->ignore_merges = 0;
    -+	} else if (!strcmp(arg, "--ignore-merges")) {
    ++	} else if (!strcmp(arg, "--no-diff-merges")) {
     +		revs->ignore_merges = 1;
      	} else if (!strcmp(arg, "-c")) {
      		revs->diff = 1;
    @@ t/t4013-diff-various.sh: log --root --patch-with-stat --summary master
      log --root -c --patch-with-stat --summary master
      # improved by Timo's patch
      log --root --cc --patch-with-stat --summary master
    -+log --ignore-merges -p --first-parent master
    ++log --no-diff-merges -p --first-parent master
      log -p --first-parent master
      log -m -p --first-parent master
      log -m -p master
     
    - ## t/t4013/diff.log_--ignore-merges_-p_--first-parent_master (new) ##
    + ## t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master (new) ##
     @@
    -+$ git log --ignore-merges -p --first-parent master
    ++$ git log --no-diff-merges -p --first-parent master
     +commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
     +Merge: 9a6d494 c7a2ab9
     +Author: A U Thor <author@example.com>
3:  3381fefeeb ! 3:  512b230003 log: enable "-m" automatically with "--first-parent"
    @@ Commit message
     
         No new test is needed; we'll tweak the output of the existing
         "--first-parent -p" test, which now matches the "-m --first-parent -p"
    -    test. The unchanged existing test for "--ignore-merges" confirms that
    +    test. The unchanged existing test for "--no-diff-merges" confirms that
         the user can get the old behavior if they want.
     
      ## builtin/log.c ##
-:  ---------- > 4:  2310dd62a6 doc/git-log: move "Diff Formatting" from rev-list-options
-:  ---------- > 5:  7a9a6b2d94 doc/git-log: drop "-r" diff option
-:  ---------- > 6:  e369e0ac50 doc/git-log: move "-t" into diff-options list
-:  ---------- > 7:  c1e769448c doc/git-log: clarify handling of merge commit diffs

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH v2 1/7] log: drop "--cc implies -m" logic
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
@ 2020-07-29 20:10   ` Jeff King
  2020-07-29 20:10   ` [PATCH v2 2/7] revision: add "--no-diff-merges" option to counteract "-m" Jeff King
                     ` (7 subsequent siblings)
  8 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-29 20:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Chris Torek

This was added by 82dee4160c (log: show merge commit when --cc is given,
2015-08-20), which explains why we need it. But that commit failed to
notice that setup_revisions() already does the same thing, since
cd2bdc5309 (Common option parsing for "git log --diff" and friends,
2006-04-14).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index d104d5c688..281d2ae8eb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -731,10 +731,6 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
 	/* Turn --cc/-c into -p --cc/-c when -p was not given */
 	if (!rev->diffopt.output_format && rev->combine_merges)
 		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
-
-	/* Turn -m on when --cc/-c was given */
-	if (rev->combine_merges)
-		rev->ignore_merges = 0;
 }
 
 int cmd_log(int argc, const char **argv, const char *prefix)
-- 
2.28.0.465.gd2839157e3


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH v2 2/7] revision: add "--no-diff-merges" option to counteract "-m"
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
  2020-07-29 20:10   ` [PATCH v2 1/7] log: drop "--cc implies -m" logic Jeff King
@ 2020-07-29 20:10   ` Jeff King
  2020-07-29 20:10   ` [PATCH v2 3/7] log: enable "-m" automatically with "--first-parent" Jeff King
                     ` (6 subsequent siblings)
  8 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-29 20:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Chris Torek

The "-m" option sets revs->ignore_merges to "0", but there's no way to
undo it. This probably isn't something anybody overly cares about, since
"1" is already the default, but it will serve as an escape hatch when we
flip the default for ignore_merges to "0" in more situations.

We'll also add a few extra niceties:

  - initialize the value to "-1" to indicate "not set", and then resolve
    it to the normal 0/1 bool in setup_revisions(). This lets any tweak
    functions, as well as setup_revisions() itself, avoid clobbering the
    user's preference (which until now they couldn't actually express).

  - since we now have --no-diff-merges, let's add the matching
    --diff-merges, which is just a synonym for "-m". Then we don't even
    need to document --no-diff-merges separately; it countermands the
    long form of "-m" in the usual way.

The new test shows that this behaves just the same as the current
behavior without "-m".

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/rev-list-options.txt            |  1 +
 builtin/log.c                                 |  4 +-
 revision.c                                    | 10 ++-
 revision.h                                    |  2 +-
 t/t4013-diff-various.sh                       |  1 +
 ..._--no-diff-merges_-p_--first-parent_master | 78 +++++++++++++++++++
 6 files changed, 90 insertions(+), 6 deletions(-)
 create mode 100644 t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index b01b2b6773..0785a0cfe9 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -1148,6 +1148,7 @@ options may be given. See linkgit:git-diff-files[1] for more options.
 	rename or copy detection have been requested).
 
 -m::
+--diff-merges::
 	This flag makes the merge commits show the full diff like
 	regular commits; for each merge parent, a separate log entry
 	and diff is generated. An exception is that only diff against
diff --git a/builtin/log.c b/builtin/log.c
index 281d2ae8eb..39b3d773a9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -599,8 +599,8 @@ static int show_tree_object(const struct object_id *oid,
 static void show_setup_revisions_tweak(struct rev_info *rev,
 				       struct setup_revision_opt *opt)
 {
-	if (rev->ignore_merges) {
-		/* There was no "-m" on the command line */
+	if (rev->ignore_merges < 0) {
+		/* There was no "-m" variant on the command line */
 		rev->ignore_merges = 0;
 		if (!rev->first_parent_only && !rev->combine_merges) {
 			/* No "--first-parent", "-c", or "--cc" */
diff --git a/revision.c b/revision.c
index 6aa7f4f567..669bc85669 100644
--- a/revision.c
+++ b/revision.c
@@ -1795,7 +1795,7 @@ void repo_init_revisions(struct repository *r,
 
 	revs->repo = r;
 	revs->abbrev = DEFAULT_ABBREV;
-	revs->ignore_merges = 1;
+	revs->ignore_merges = -1;
 	revs->simplify_history = 1;
 	revs->pruning.repo = r;
 	revs->pruning.flags.recursive = 1;
@@ -2323,8 +2323,10 @@ 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")) {
+	} else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
 		revs->ignore_merges = 0;
+	} else if (!strcmp(arg, "--no-diff-merges")) {
+		revs->ignore_merges = 1;
 	} else if (!strcmp(arg, "-c")) {
 		revs->diff = 1;
 		revs->dense_combined_merges = 0;
@@ -2834,8 +2836,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			copy_pathspec(&revs->diffopt.pathspec,
 				      &revs->prune_data);
 	}
-	if (revs->combine_merges)
+	if (revs->combine_merges && revs->ignore_merges < 0)
 		revs->ignore_merges = 0;
+	if (revs->ignore_merges < 0)
+		revs->ignore_merges = 1;
 	if (revs->combined_all_paths && !revs->combine_merges)
 		die("--combined-all-paths makes no sense without -c or --cc");
 
diff --git a/revision.h b/revision.h
index f412ae85eb..5258024743 100644
--- a/revision.h
+++ b/revision.h
@@ -190,11 +190,11 @@ struct rev_info {
 			show_root_diff:1,
 			no_commit_id:1,
 			verbose_header:1,
-			ignore_merges:1,
 			combine_merges:1,
 			combined_all_paths:1,
 			dense_combined_merges:1,
 			always_show_header:1;
+	int             ignore_merges:2;
 
 	/* Format info */
 	int		show_notes;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 43267d6024..40e222c945 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -297,6 +297,7 @@ log --root --patch-with-stat --summary master
 log --root -c --patch-with-stat --summary master
 # improved by Timo's patch
 log --root --cc --patch-with-stat --summary master
+log --no-diff-merges -p --first-parent master
 log -p --first-parent master
 log -m -p --first-parent master
 log -m -p master
diff --git a/t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master b/t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master
new file mode 100644
index 0000000000..94bf1850b2
--- /dev/null
+++ b/t/t4013/diff.log_--no-diff-merges_-p_--first-parent_master
@@ -0,0 +1,78 @@
+$ git log --no-diff-merges -p --first-parent master
+commit 59d314ad6f356dd08601a4cd5e530381da3e3c64
+Merge: 9a6d494 c7a2ab9
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:04:00 2006 +0000
+
+    Merge branch 'side' into master
+
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+
+diff --git a/dir/sub b/dir/sub
+index 8422d40..cead32e 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -2,3 +2,5 @@ A
+ B
+ C
+ D
++E
++F
+diff --git a/file1 b/file1
+new file mode 100644
+index 0000000..b1e6722
+--- /dev/null
++++ b/file1
+@@ -0,0 +1,3 @@
++A
++B
++C
+
+commit 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:01:00 2006 +0000
+
+    Second
+    
+    This is the second commit.
+
+diff --git a/dir/sub b/dir/sub
+index 35d242b..8422d40 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++C
++D
+diff --git a/file0 b/file0
+index 01e79c3..b414108 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++4
++5
++6
+diff --git a/file2 b/file2
+deleted file mode 100644
+index 01e79c3..0000000
+--- a/file2
++++ /dev/null
+@@ -1,3 +0,0 @@
+-1
+-2
+-3
+
+commit 444ac553ac7612cc88969031b02b3767fb8a353a
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:00:00 2006 +0000
+
+    Initial
+$
-- 
2.28.0.465.gd2839157e3


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH v2 3/7] log: enable "-m" automatically with "--first-parent"
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
  2020-07-29 20:10   ` [PATCH v2 1/7] log: drop "--cc implies -m" logic Jeff King
  2020-07-29 20:10   ` [PATCH v2 2/7] revision: add "--no-diff-merges" option to counteract "-m" Jeff King
@ 2020-07-29 20:10   ` Jeff King
  2020-07-29 20:28     ` Junio C Hamano
  2020-07-29 20:11   ` [PATCH v2 4/7] doc/git-log: move "Diff Formatting" from rev-list-options Jeff King
                     ` (5 subsequent siblings)
  8 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-07-29 20:10 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Chris Torek

When using "--first-parent" to consider history as a single line of
commits, git-log still defaults to treating merges specially, even
though they could be considered as single commits in the linearized
history (that just introduce all of the changes from the second and
higher parents).

Let's instead have "--first-parent" imply "-m", which makes something
like:

  git log --first-parent -p

do what you'd expect. Likewise:

  git log --first-parent -Sfoo

will find "foo" in merge commits.

No new test is needed; we'll tweak the output of the existing
"--first-parent -p" test, which now matches the "-m --first-parent -p"
test. The unchanged existing test for "--no-diff-merges" confirms that
the user can get the old behavior if they want.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/log.c                             |  3 +++
 t/t4013/diff.log_-p_--first-parent_master | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index 39b3d773a9..83b147c23a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -731,6 +731,9 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
 	/* Turn --cc/-c into -p --cc/-c when -p was not given */
 	if (!rev->diffopt.output_format && rev->combine_merges)
 		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
+
+	if (rev->first_parent_only && rev->ignore_merges < 0)
+		rev->ignore_merges = 0;
 }
 
 int cmd_log(int argc, const char **argv, const char *prefix)
diff --git a/t/t4013/diff.log_-p_--first-parent_master b/t/t4013/diff.log_-p_--first-parent_master
index c6a5876d80..fe044399f0 100644
--- a/t/t4013/diff.log_-p_--first-parent_master
+++ b/t/t4013/diff.log_-p_--first-parent_master
@@ -6,6 +6,28 @@ Date:   Mon Jun 26 00:04:00 2006 +0000
 
     Merge branch 'side' into master
 
+diff --git a/dir/sub b/dir/sub
+index cead32e..992913c 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -4,3 +4,5 @@ C
+ D
+ E
+ F
++1
++2
+diff --git a/file0 b/file0
+index b414108..10a8a9f 100644
+--- a/file0
++++ b/file0
+@@ -4,3 +4,6 @@
+ 4
+ 5
+ 6
++A
++B
++C
+
 commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
 Author: A U Thor <author@example.com>
 Date:   Mon Jun 26 00:02:00 2006 +0000
-- 
2.28.0.465.gd2839157e3


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH v2 4/7] doc/git-log: move "Diff Formatting" from rev-list-options
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
                     ` (2 preceding siblings ...)
  2020-07-29 20:10   ` [PATCH v2 3/7] log: enable "-m" automatically with "--first-parent" Jeff King
@ 2020-07-29 20:11   ` Jeff King
  2020-07-29 20:56     ` Junio C Hamano
  2020-07-29 20:11   ` [PATCH v2 5/7] doc/git-log: drop "-r" diff option Jeff King
                     ` (4 subsequent siblings)
  8 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-07-29 20:11 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Chris Torek

Our rev-list-options.txt include has a "Diff Formatting" section, but it
is ifndef'd out for all manpages except git-log. And a few bits of the
text are rather out of date.

We say "some of these options are specific to git-rev-list". That's
obviously silly since we (even before this patch) show the content only
for git-log. But moreover, it's not true; each of the listed options is
meaningful for other diff commands.

We also say "...however other diff options may be given. See git-diff-files
for more options." But there's no need to do so; git-log already has a
"Common Diff Options" section which includes diff-options.txt.

So let's move these options over to git-log and put them with the other
diff options, giving a single "diff" section for the git-log
documentation. We'll call it "Diff Formatting" but use the all-caps
top-level header to match its sibling sections. And we'll rewrite the
section intro to remove the useless bits and give a more generic
overview of the section which can be later extended.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-log.txt          | 42 +++++++++++++++++++++++++--
 Documentation/rev-list-options.txt | 46 ------------------------------
 2 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 20e6d21a74..fb3998d8e0 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -111,8 +111,46 @@ include::rev-list-options.txt[]
 
 include::pretty-formats.txt[]
 
-COMMON DIFF OPTIONS
--------------------
+DIFF FORMATTING
+---------------
+
+By default, `git log` does not generate any diff output. The options
+below can be used to show the changes made by each commit.
+
+-c::
+	With this option, diff output for a merge commit
+	shows the differences from each of the parents to the merge result
+	simultaneously instead of showing pairwise diff between a parent
+	and the result one at a time. Furthermore, it lists only files
+	which were modified from all parents.
+
+--cc::
+	This flag implies the `-c` option and further compresses the
+	patch output by omitting uninteresting hunks whose contents in
+	the parents have only two variants and the merge result picks
+	one of them without modification.
+
+--combined-all-paths::
+	This flag causes combined diffs (used for merge commits) to
+	list the name of the file from all parents.  It thus only has
+	effect when -c or --cc are specified, and is likely only
+	useful if filename changes are detected (i.e. when either
+	rename or copy detection have been requested).
+
+-m::
+--diff-merges::
+	This flag makes the merge commits show the full diff like
+	regular commits; for each merge parent, a separate log entry
+	and diff is generated. An exception is that only diff against
+	the first parent is shown when `--first-parent` option is given;
+	in that case, the output represents the changes the merge
+	brought _into_ the then-current branch.
+
+-r::
+	Show recursive diffs.
+
+-t::
+	Show the tree objects in the diff output. This implies `-r`.
 
 :git-log: 1
 include::diff-options.txt[]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 0785a0cfe9..398178d72a 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -1117,49 +1117,3 @@ ifdef::git-rev-list[]
 	by a tab.
 endif::git-rev-list[]
 endif::git-shortlog[]
-
-ifndef::git-shortlog[]
-ifndef::git-rev-list[]
-Diff Formatting
-~~~~~~~~~~~~~~~
-
-Listed below are options that control the formatting of diff output.
-Some of them are specific to linkgit:git-rev-list[1], however other diff
-options may be given. See linkgit:git-diff-files[1] for more options.
-
--c::
-	With this option, diff output for a merge commit
-	shows the differences from each of the parents to the merge result
-	simultaneously instead of showing pairwise diff between a parent
-	and the result one at a time. Furthermore, it lists only files
-	which were modified from all parents.
-
---cc::
-	This flag implies the `-c` option and further compresses the
-	patch output by omitting uninteresting hunks whose contents in
-	the parents have only two variants and the merge result picks
-	one of them without modification.
-
---combined-all-paths::
-	This flag causes combined diffs (used for merge commits) to
-	list the name of the file from all parents.  It thus only has
-	effect when -c or --cc are specified, and is likely only
-	useful if filename changes are detected (i.e. when either
-	rename or copy detection have been requested).
-
--m::
---diff-merges::
-	This flag makes the merge commits show the full diff like
-	regular commits; for each merge parent, a separate log entry
-	and diff is generated. An exception is that only diff against
-	the first parent is shown when `--first-parent` option is given;
-	in that case, the output represents the changes the merge
-	brought _into_ the then-current branch.
-
--r::
-	Show recursive diffs.
-
--t::
-	Show the tree objects in the diff output. This implies `-r`.
-endif::git-rev-list[]
-endif::git-shortlog[]
-- 
2.28.0.465.gd2839157e3


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH v2 5/7] doc/git-log: drop "-r" diff option
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
                     ` (3 preceding siblings ...)
  2020-07-29 20:11   ` [PATCH v2 4/7] doc/git-log: move "Diff Formatting" from rev-list-options Jeff King
@ 2020-07-29 20:11   ` Jeff King
  2020-07-29 20:12   ` [PATCH v2 6/7] doc/git-log: move "-t" into diff-options list Jeff King
                     ` (3 subsequent siblings)
  8 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-29 20:11 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Chris Torek

This has been the default since 170c04383b (Porcelain level "log" family
should recurse when diffing., 2007-08-27). There's not even a way to
turn it off, so you'd never even want "-r" to override that.

It's not the default for plumbing like diff-tree, of course, but the
option is documented separately there.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-log.txt | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index fb3998d8e0..2cbe636b2b 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -146,11 +146,8 @@ below can be used to show the changes made by each commit.
 	in that case, the output represents the changes the merge
 	brought _into_ the then-current branch.
 
--r::
-	Show recursive diffs.
-
 -t::
-	Show the tree objects in the diff output. This implies `-r`.
+	Show the tree objects in the diff output.
 
 :git-log: 1
 include::diff-options.txt[]
-- 
2.28.0.465.gd2839157e3


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH v2 6/7] doc/git-log: move "-t" into diff-options list
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
                     ` (4 preceding siblings ...)
  2020-07-29 20:11   ` [PATCH v2 5/7] doc/git-log: drop "-r" diff option Jeff King
@ 2020-07-29 20:12   ` Jeff King
  2020-07-29 21:10     ` Junio C Hamano
  2020-07-29 20:12   ` [PATCH v2 7/7] doc/git-log: clarify handling of merge commit diffs Jeff King
                     ` (2 subsequent siblings)
  8 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-07-29 20:12 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Chris Torek

The "-t" option is infrequently used; it doesn't deserve a spot near the
top of the options list. Let's push it down into the diff-options
include, near the definition of --raw.

We'll protect it with a git-log ifdef, since it doesn't make any sense
for non-tree diff commands. Note that this means it also shows up in
git-show, but that's a good thing; it applies equally well there.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm open to suggestions of a better spot to put it. The diff options
list is a rather unwieldy mess.

 Documentation/diff-options.txt | 5 +++++
 Documentation/git-log.txt      | 3 ---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7987d72b02..b7af973d9c 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -73,6 +73,11 @@ ifndef::git-format-patch[]
 	Synonym for `-p --raw`.
 endif::git-format-patch[]
 
+ifdef::git-log[]
+-t::
+	Show the tree objects in the diff output.
+endif::git-log[]
+
 --indent-heuristic::
 	Enable the heuristic that shifts diff hunk boundaries to make patches
 	easier to read. This is the default.
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 2cbe636b2b..0a4c99e5f8 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -146,9 +146,6 @@ below can be used to show the changes made by each commit.
 	in that case, the output represents the changes the merge
 	brought _into_ the then-current branch.
 
--t::
-	Show the tree objects in the diff output.
-
 :git-log: 1
 include::diff-options.txt[]
 
-- 
2.28.0.465.gd2839157e3


^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH v2 7/7] doc/git-log: clarify handling of merge commit diffs
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
                     ` (5 preceding siblings ...)
  2020-07-29 20:12   ` [PATCH v2 6/7] doc/git-log: move "-t" into diff-options list Jeff King
@ 2020-07-29 20:12   ` Jeff King
  2020-07-29 21:03   ` [PATCH v2 0/7] making log --first-parent imply -m Chris Torek
  2020-07-29 21:41   ` Sergey Organov
  8 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-29 20:12 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Chris Torek

It can be surprising that git-log doesn't show any diff for merge
commits by default. Arguably "--cc" would be a reasonable default, but
it's very expensive (which is why we turn it on for "git show" but not
for "git log"). Let's at least document the current behavior, including
the recent "--first-parent implies -m" case

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-log.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 0a4c99e5f8..9ccba65469 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -117,6 +117,13 @@ DIFF FORMATTING
 By default, `git log` does not generate any diff output. The options
 below can be used to show the changes made by each commit.
 
+Note that unless one of `-c`, `--cc`, or `-m` is given, merge commits
+will never show a diff, even if a diff format like `--patch` is
+selected, nor will they match search options like `-S`. The exception is
+when `--first-parent` is in use, in which merges are treated like normal
+single-parent commits (this can be overridden by providing a
+combined-diff option or with `--no-diff-merges`).
+
 -c::
 	With this option, diff output for a merge commit
 	shows the differences from each of the parents to the merge result
-- 
2.28.0.465.gd2839157e3

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 3/7] log: enable "-m" automatically with "--first-parent"
  2020-07-29 20:10   ` [PATCH v2 3/7] log: enable "-m" automatically with "--first-parent" Jeff King
@ 2020-07-29 20:28     ` Junio C Hamano
  0 siblings, 0 replies; 73+ messages in thread
From: Junio C Hamano @ 2020-07-29 20:28 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Chris Torek

Jeff King <peff@peff.net> writes:

> diff --git a/builtin/log.c b/builtin/log.c
> index 39b3d773a9..83b147c23a 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -731,6 +731,9 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
>  	/* Turn --cc/-c into -p --cc/-c when -p was not given */
>  	if (!rev->diffopt.output_format && rev->combine_merges)
>  		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
> +
> +	if (rev->first_parent_only && rev->ignore_merges < 0)
> +		rev->ignore_merges = 0;

Nice; thanks to the previous "initialize to -1 and override only
when not touched", this becomes quite straight-forward.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 4/7] doc/git-log: move "Diff Formatting" from rev-list-options
  2020-07-29 20:56     ` Junio C Hamano
@ 2020-07-29 21:02       ` Jeff King
  0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-29 21:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Chris Torek

On Wed, Jul 29, 2020 at 01:56:59PM -0700, Junio C Hamano wrote:

> > So let's move these options over to git-log and put them with the other
> > diff options, giving a single "diff" section for the git-log
> > documentation. We'll call it "Diff Formatting" but use the all-caps
> > top-level header to match its sibling sections. And we'll rewrite the
> > section intro to remove the useless bits and give a more generic
> > overview of the section which can be later extended.
> 
> Makes sense.  I first was afraid of regressing "git show"
> documentation because the conditional inclusion was
> 
>     > -
>     > -ifndef::git-shortlog[]
>     > -ifndef::git-rev-list[]
>     > -Diff Formatting
>     > -~~~~~~~~~~~~~~~
>     > -
> 
> but it seems that Documentation/git-show.txt does not even include
> this section being moved in the first place.
> 
> We might move these to a new file and include it from both git-log.txt
> and git-show.txt but that can be left outside the topioc.

Yeah. As I said earlier, the maze of includes is a bit of a mess. I did
wonder whether git-show might need some mention of merge-handling, too,
but it explicitly mentions that "--cc" is the default, and it contains
the whole "combined diff" section by including diff-generate-patch.txt.

So I think it's OK as-is, though I wouldn't object if anybody wanted to
look at reorganizing all of the diff documentation. :)

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
                     ` (6 preceding siblings ...)
  2020-07-29 20:12   ` [PATCH v2 7/7] doc/git-log: clarify handling of merge commit diffs Jeff King
@ 2020-07-29 21:03   ` Chris Torek
  2020-07-29 21:41   ` Sergey Organov
  8 siblings, 0 replies; 73+ messages in thread
From: Chris Torek @ 2020-07-29 21:03 UTC (permalink / raw)
  To: Jeff King; +Cc: Git List, Junio C Hamano

On Wed, Jul 29, 2020 at 1:10 PM Jeff King <peff@peff.net> wrote:
> Here's a re-roll taking into account the discussion so far:
>
>   - the escape hatch option name is flipped to "--no-diff-merges" (with
>     "--diff-merges" matching "-m")
>
>   - the documentation now discusses the change as well as the existing
>     handling of merges; this involved a few extra cleanups. Try
>     "doc-diff" for a better view of the actual rendered changes.
>
>     I do think longer term we'd be better off to stop having this maze
>     of ifdef'd inclusions, and just have gitdiff(7) which covers all of
>     the possibilities in human-readable text (so yes, you might see a
>     mention of diff-files while you're looking for info on git-log, but
>     that would also broaden your mind about how the different commands
>     work). But that's clearly outside the scope of this series. I think
>     what's here is a strict improvement.

I agree.  The new series looks good.

Chris

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 6/7] doc/git-log: move "-t" into diff-options list
  2020-07-29 20:12   ` [PATCH v2 6/7] doc/git-log: move "-t" into diff-options list Jeff King
@ 2020-07-29 21:10     ` Junio C Hamano
  2020-07-29 21:55       ` Jeff King
  0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-07-29 21:10 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Chris Torek

Jeff King <peff@peff.net> writes:

> The "-t" option is infrequently used; it doesn't deserve a spot near the
> top of the options list. Let's push it down into the diff-options
> include, near the definition of --raw.

True.  It does not have any effect unless --raw is in use (i.e.  it
does have an effect to "git show --patch-with-raw" or anything that
(also) shows "--raw" output), and it makes sense to have "-t" near
"--raw".

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
                     ` (7 preceding siblings ...)
  2020-07-29 21:03   ` [PATCH v2 0/7] making log --first-parent imply -m Chris Torek
@ 2020-07-29 21:41   ` Sergey Organov
  2020-07-31 23:08     ` Jeff King
  8 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-07-29 21:41 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Chris Torek

Jeff King <peff@peff.net> writes:

> On Tue, Jul 28, 2020 at 12:36:18PM -0400, Jeff King wrote:
>
>> This series just makes --first-parent imply -m. That doesn't change any
>> output by itself, but does mean that diff options like "-p", "-S", etc,
>> behave sensibly.

This is definitely step in the right direction, thanks a lot for working
on it!

>
> Here's a re-roll taking into account the discussion so far:
>
>   - the escape hatch option name is flipped to "--no-diff-merges" (with
>     "--diff-merges" matching "-m")

Rather than being just a synonym for -m, is there a chance for
"--diff-merges" implementation to be turned to output diff to the first
parent only, no matter if --first-parent is active or not?

Alternatively, may it have a parameter such as "-m parent-number" of
"git cherry-pick" being set to "1" by default?

This -m output of diffs to all the parents is in fact primary source of
confusion for me, even over all these mind-blowing inter-dependencies
between --first-parent, --cc, -c, -m, -p and what not. Who ever needs
these (potentially huge) diffs against other parents, anyway?

Introduction of this new option is a great opportunity for improvement
that would be a pity to miss.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 6/7] doc/git-log: move "-t" into diff-options list
  2020-07-29 21:10     ` Junio C Hamano
@ 2020-07-29 21:55       ` Jeff King
  0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-07-29 21:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Chris Torek

On Wed, Jul 29, 2020 at 02:10:31PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The "-t" option is infrequently used; it doesn't deserve a spot near the
> > top of the options list. Let's push it down into the diff-options
> > include, near the definition of --raw.
> 
> True.  It does not have any effect unless --raw is in use (i.e.  it
> does have an effect to "git show --patch-with-raw" or anything that
> (also) shows "--raw" output), and it makes sense to have "-t" near
> "--raw".

It affects --name-status, etc, too. But yeah, I would think the primary
use is with --raw.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-07-29 21:41   ` Sergey Organov
@ 2020-07-31 23:08     ` Jeff King
  2020-08-02 12:59       ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-07-31 23:08 UTC (permalink / raw)
  To: Sergey Organov; +Cc: git, Junio C Hamano, Chris Torek

On Thu, Jul 30, 2020 at 12:41:39AM +0300, Sergey Organov wrote:

> > Here's a re-roll taking into account the discussion so far:
> >
> >   - the escape hatch option name is flipped to "--no-diff-merges" (with
> >     "--diff-merges" matching "-m")
> 
> Rather than being just a synonym for -m, is there a chance for
> "--diff-merges" implementation to be turned to output diff to the first
> parent only, no matter if --first-parent is active or not?
>
> Alternatively, may it have a parameter such as "-m parent-number" of
> "git cherry-pick" being set to "1" by default?

Yes, I agree that would be a useful feature, but I don't think it needs
to be part of this series. It could be implemented as --diff-merges=1 to
show only the one against the first parent, or as its own option. But we
can add that on top.

> This -m output of diffs to all the parents is in fact primary source of
> confusion for me, even over all these mind-blowing inter-dependencies
> between --first-parent, --cc, -c, -m, -p and what not. Who ever needs
> these (potentially huge) diffs against other parents, anyway?

I've used "-m" second-parent diffs occasionally for hunting down
mismerges, etc, but I agree that most of the time you just want to see
the diff against the first parent.

> Introduction of this new option is a great opportunity for improvement
> that would be a pity to miss.

Adding an optional value to the flag is something we can do later. We
would miss the opportunity for "--diff-merges" to default to
"--diff-merges=1", but I'm not sure I'd want to do that anyway. Having
it be consistent with "-m" seems less confusing to me, and it is already
too late to change that.

If we want an option that defaults to "1", we can give it a new name.
The only thing that is lost now is that --diff-merges would already be
taken. :) But I think I'd probably call such an option "--diff-parents"
or something like that anyway.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-07-31 23:08     ` Jeff King
@ 2020-08-02 12:59       ` Sergey Organov
  2020-08-02 17:35         ` Junio C Hamano
  0 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-02 12:59 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Chris Torek

Jeff King <peff@peff.net> writes:

> On Thu, Jul 30, 2020 at 12:41:39AM +0300, Sergey Organov wrote:
>
>> > Here's a re-roll taking into account the discussion so far:
>> >
>> >   - the escape hatch option name is flipped to "--no-diff-merges" (with
>> >     "--diff-merges" matching "-m")
>> 
>> Rather than being just a synonym for -m, is there a chance for
>> "--diff-merges" implementation to be turned to output diff to the first
>> parent only, no matter if --first-parent is active or not?
>>
>> Alternatively, may it have a parameter such as "-m parent-number" of
>> "git cherry-pick" being set to "1" by default?
>
> Yes, I agree that would be a useful feature, but I don't think it needs
> to be part of this series. It could be implemented as --diff-merges=1 to
> show only the one against the first parent, or as its own option. But we
> can add that on top.
>
>> This -m output of diffs to all the parents is in fact primary source of
>> confusion for me, even over all these mind-blowing inter-dependencies
>> between --first-parent, --cc, -c, -m, -p and what not. Who ever needs
>> these (potentially huge) diffs against other parents, anyway?
>
> I've used "-m" second-parent diffs occasionally for hunting down
> mismerges, etc, but I agree that most of the time you just want to see
> the diff against the first parent.
>
>> Introduction of this new option is a great opportunity for improvement
>> that would be a pity to miss.
>
> Adding an optional value to the flag is something we can do later. We
> would miss the opportunity for "--diff-merges" to default to
> "--diff-merges=1", but I'm not sure I'd want to do that anyway. Having
> it be consistent with "-m" seems less confusing to me, and it is already
> too late to change that.
>
> If we want an option that defaults to "1", we can give it a new name.
> The only thing that is lost now is that --diff-merges would already be
> taken. :) But I think I'd probably call such an option "--diff-parents"
> or something like that anyway.

Yeah, I see your point, thanks for considering!

What I in fact have in mind is something like:

  --diff-merges[=<parent-number>|c|cc|all]

to have a single point of definition of the needed format of merges
representation.

Then comes the question of the default. "all" is what'd make it behave
as "-m" behaves now. If it's too late for --diff-merges to have
different default, could the default possibly be a configuration
option rather than yet another command-line option?

-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-02 12:59       ` Sergey Organov
@ 2020-08-02 17:35         ` Junio C Hamano
  2020-08-03 15:47           ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-02 17:35 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Jeff King, git, Chris Torek

Sergey Organov <sorganov@gmail.com> writes:

> What I in fact have in mind is something like:
>
>   --diff-merges[=<parent-number>|c|cc|all]
>
> to have a single point of definition of the needed format of merges
> representation.

A command line option that takes _optional_ argument is evil; it
hurts teachability (e.g. "why does this option always require
"--opt=val" and refuses '--opt val'?").  Making the optional value
configurable is even more so (e.g. "teacher said to use '--opt', but
it does not behave the way she taught---ah, I have this config").

> Then comes the question of the default. "all" is what'd make it behave
> as "-m" behaves now. If it's too late for --diff-merges to have
> different default, could the default possibly be a configuration
> option rather than yet another command-line option?

Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is
different from --diff-merges, and -m and --[no-]diff-merges can be
defined in terms of that, at which point we can gradually deprecate
them if we wanted to, no?


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-02 17:35         ` Junio C Hamano
@ 2020-08-03 15:47           ` Sergey Organov
  2020-08-03 16:35             ` Junio C Hamano
  2020-08-03 18:08             ` Jeff King
  0 siblings, 2 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-03 15:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Chris Torek

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> What I in fact have in mind is something like:
>>
>>   --diff-merges[=<parent-number>|c|cc|all]
>>
>> to have a single point of definition of the needed format of merges
>> representation.
>
> A command line option that takes _optional_ argument is evil; it
> hurts teachability (e.g. "why does this option always require
> "--opt=val" and refuses '--opt val'?").

Yeah, I sympathize.

> Making the optional value configurable is even more so (e.g. "teacher
> said to use '--opt', but it does not behave the way she taught---ah, I
> have this config").

OK, let's drop the suggestion then.

>
>> Then comes the question of the default. "all" is what'd make it behave
>> as "-m" behaves now. If it's too late for --diff-merges to have
>> different default, could the default possibly be a configuration
>> option rather than yet another command-line option?
>
> Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is
> different from --diff-merges, and -m and --[no-]diff-merges can be
> defined in terms of that, at which point we can gradually deprecate
> them if we wanted to, no?

Sounds great, I only hoped we can do it right now, with this new
--diff-merges option, maybe as a pre-requisite to the patches in
question, but Jeff said it's too late, dunno why.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 15:47           ` Sergey Organov
@ 2020-08-03 16:35             ` Junio C Hamano
  2020-08-03 16:41               ` Sergey Organov
  2020-08-03 18:08             ` Jeff King
  1 sibling, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-03 16:35 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Jeff King, git, Chris Torek

Sergey Organov <sorganov@gmail.com> writes:

> Sounds great, I only hoped we can do it right now, with this new
> --diff-merges option, maybe as a pre-requisite to the patches in
> question, but Jeff said it's too late, dunno why.

A follow-up patch or two to remove the "--diff-merges" option and
add the "--diff-parents=(none|<number>|c|cc|all)" option on top of
the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a
possibility.

But is it worth the delay?  I dunno.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 16:35             ` Junio C Hamano
@ 2020-08-03 16:41               ` Sergey Organov
  2020-08-03 17:21                 ` Junio C Hamano
  0 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-03 16:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Chris Torek

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Sounds great, I only hoped we can do it right now, with this new
>> --diff-merges option, maybe as a pre-requisite to the patches in
>> question, but Jeff said it's too late, dunno why.
>
> A follow-up patch or two to remove the "--diff-merges" option and
> add the "--diff-parents=(none|<number>|c|cc|all)" option on top of
> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a
> possibility.
>
> But is it worth the delay?  I dunno.

I don't think it's worth the delay, provided yet another new
--diff-parents is to be implemented rather that using --diff-merges for
that.

-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 16:41               ` Sergey Organov
@ 2020-08-03 17:21                 ` Junio C Hamano
  2020-08-03 20:25                   ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-03 17:21 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Jeff King, git, Chris Torek

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Sounds great, I only hoped we can do it right now, with this new
>>> --diff-merges option, maybe as a pre-requisite to the patches in
>>> question, but Jeff said it's too late, dunno why.
>>
>> A follow-up patch or two to remove the "--diff-merges" option and
>> add the "--diff-parents=(none|<number>|c|cc|all)" option on top of
>> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a
>> possibility.
>>
>> But is it worth the delay?  I dunno.
>
> I don't think it's worth the delay, provided yet another new
> --diff-parents is to be implemented rather that using --diff-merges for
> that.

I was responding to your "it's too late, dunno why", as you seemed
not to want to waste an option "--diff-merges" that will become
unused when "--diff-parents" come and also wanted it to happen right
now.  If you no longer want to see it happen right now, that's OK by
me.  




^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 15:47           ` Sergey Organov
  2020-08-03 16:35             ` Junio C Hamano
@ 2020-08-03 18:08             ` Jeff King
  2020-08-03 20:00               ` Sergey Organov
  2020-08-04 17:50               ` Sergey Organov
  1 sibling, 2 replies; 73+ messages in thread
From: Jeff King @ 2020-08-03 18:08 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Mon, Aug 03, 2020 at 06:47:20PM +0300, Sergey Organov wrote:

> > A command line option that takes _optional_ argument is evil; it
> > hurts teachability (e.g. "why does this option always require
> > "--opt=val" and refuses '--opt val'?").
> 
> Yeah, I sympathize.

Sorry, the optional argument was my suggestion. I don't think they're
that evil, but I agree they require extra knowledge for the user. I
don't mind avoiding them when possible (and it's definitely possible
here).

> > Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is
> > different from --diff-merges, and -m and --[no-]diff-merges can be
> > defined in terms of that, at which point we can gradually deprecate
> > them if we wanted to, no?
> 
> Sounds great, I only hoped we can do it right now, with this new
> --diff-merges option, maybe as a pre-requisite to the patches in
> question, but Jeff said it's too late, dunno why.

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.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 18:08             ` Jeff King
@ 2020-08-03 20:00               ` Sergey Organov
  2020-08-03 20:55                 ` Jeff King
  2020-08-04 17:50               ` Sergey Organov
  1 sibling, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-03 20:00 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Mon, Aug 03, 2020 at 06:47:20PM +0300, Sergey Organov wrote:
>
>> > A command line option that takes _optional_ argument is evil; it
>> > hurts teachability (e.g. "why does this option always require
>> > "--opt=val" and refuses '--opt val'?").
>> 
>> Yeah, I sympathize.
>
> Sorry, the optional argument was my suggestion. I don't think they're
> that evil, but I agree they require extra knowledge for the user. I
> don't mind avoiding them when possible (and it's definitely possible
> here).
>
>> > Introduce --diff-parent=(none|<parent-number>|c|cc|all) that is
>> > different from --diff-merges, and -m and --[no-]diff-merges can be
>> > defined in terms of that, at which point we can gradually deprecate
>> > them if we wanted to, no?
>> 
>> Sounds great, I only hoped we can do it right now, with this new
>> --diff-merges option, maybe as a pre-requisite to the patches in
>> question, but Jeff said it's too late, dunno why.
>
> 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).

I thought not of changing semantics of -m. Suppose we introduce

  --diff-merges=(none|<parent-number>|c|cc|all)

before your patch(es). Then your patch would read: "making --first-parent
imply --diff-merges=1" and it'd miss that --[no-]diff-merges part, no?

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

Can't we do yours on top instead? I'd expect it'd then be even simpler.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 17:21                 ` Junio C Hamano
@ 2020-08-03 20:25                   ` Sergey Organov
  2020-08-03 20:58                     ` Jeff King
  0 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-03 20:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Chris Torek

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sergey Organov <sorganov@gmail.com> writes:
>>>
>>>> Sounds great, I only hoped we can do it right now, with this new
>>>> --diff-merges option, maybe as a pre-requisite to the patches in
>>>> question, but Jeff said it's too late, dunno why.
>>>
>>> A follow-up patch or two to remove the "--diff-merges" option and
>>> add the "--diff-parents=(none|<number>|c|cc|all)" option on top of
>>> the jk/log-fp-implies-m topic BEFORE it graduates to 'master' is a
>>> possibility.
>>>
>>> But is it worth the delay?  I dunno.
>>
>> I don't think it's worth the delay, provided yet another new
>> --diff-parents is to be implemented rather that using --diff-merges for
>> that.
>
> I was responding to your "it's too late, dunno why", as you seemed
> not to want to waste an option "--diff-merges" that will become
> unused when "--diff-parents" come and also wanted it to happen right
> now.  If you no longer want to see it happen right now, that's OK by
> me.

Eh, no, as I see it, I suggested to have

  --diff-merges=(none|<number>|c|cc|all)

right now rather than introduce yet another option (--diff-parents)
later, as well as to make --first-parent imply --diff-merges=1 rather
than "-m" (the latter in turn being synonym for --diff-merges=all), and
I thought that's what was rejected by Jeff on the ground that it's too
late, but as he clarified in his recent response it was not.

I mean, why introduce --[no-]diff-merges in the first place, if we do
agree --xxx=(none|...) is where we'd like to end up? I thought the
answer was "it's too late", but in fact it was an answer to changing
semantics of -m that I don't think I suggest.

As a side-note, my secret hope is for pure "git log -p" to give me diff
against first parent for all the commits, no matter how many parents
they happen to have. This desire still sounds like a job for
configuration option though, rather than, or in addition to,
--diff-merges or --diff-parents? We well can later introduce a
config to assume --diff-merges=<config> when no explicit
--diff-merges=<value> is specified, right?

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 20:00               ` Sergey Organov
@ 2020-08-03 20:55                 ` Jeff King
  2020-08-03 21:18                   ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-08-03 20:55 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Mon, Aug 03, 2020 at 11:00:11PM +0300, Sergey Organov wrote:

> > 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).
> 
> I thought not of changing semantics of -m. Suppose we introduce
> 
>   --diff-merges=(none|<parent-number>|c|cc|all)
> 
> before your patch(es). Then your patch would read: "making --first-parent
> imply --diff-merges=1" and it'd miss that --[no-]diff-merges part, no?

Sure, that would be OK with me. You'd have --diff-merges=none to get
the current behavior, and probably make --no-diff-merges an alias for
that.

> > 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.
> 
> Can't we do yours on top instead? I'd expect it'd then be even simpler.

Mine is in 'next', so there is no rebuilding it on top of anything else
without a revert. But I don't see any particular reason to do that
versus just changing the behavior on top. What's in 'next' is generally
not rewound, but the behaviors are not cemented with respect to
backwards compatibility.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 20:25                   ` Sergey Organov
@ 2020-08-03 20:58                     ` Jeff King
  2020-08-03 21:16                       ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-08-03 20:58 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Mon, Aug 03, 2020 at 11:25:05PM +0300, Sergey Organov wrote:

> I mean, why introduce --[no-]diff-merges in the first place, if we do
> agree --xxx=(none|...) is where we'd like to end up? I thought the
> answer was "it's too late", but in fact it was an answer to changing
> semantics of -m that I don't think I suggest.

Spelling out the between the lines of my answer a bit more, it was
really: I am happy enough with the topic as-is and do not want to rework
it again. But if _you_ want to do so, I have no problem with it. :)

> As a side-note, my secret hope is for pure "git log -p" to give me diff
> against first parent for all the commits, no matter how many parents
> they happen to have. This desire still sounds like a job for
> configuration option though, rather than, or in addition to,
> --diff-merges or --diff-parents? We well can later introduce a
> config to assume --diff-merges=<config> when no explicit
> --diff-merges=<value> is specified, right?

Yes, I think a config there would be reasonable as long as:

  - we have the command-line options to counteract it when necessary
    (i.e., --diff-parents or your advanced --diff-merges exists, too)

  - it only impacts porcelain "git log", and not plumbing like
    diff-tree.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 20:58                     ` Jeff King
@ 2020-08-03 21:16                       ` Sergey Organov
  0 siblings, 0 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-03 21:16 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Mon, Aug 03, 2020 at 11:25:05PM +0300, Sergey Organov wrote:
>
>> I mean, why introduce --[no-]diff-merges in the first place, if we do
>> agree --xxx=(none|...) is where we'd like to end up? I thought the
>> answer was "it's too late", but in fact it was an answer to changing
>> semantics of -m that I don't think I suggest.
>
> Spelling out the between the lines of my answer a bit more, it was
> really: I am happy enough with the topic as-is and do not want to rework
> it again. But if _you_ want to do so, I have no problem with it. :)

OK, I see!

>
>> As a side-note, my secret hope is for pure "git log -p" to give me diff
>> against first parent for all the commits, no matter how many parents
>> they happen to have. This desire still sounds like a job for
>> configuration option though, rather than, or in addition to,
>> --diff-merges or --diff-parents? We well can later introduce a
>> config to assume --diff-merges=<config> when no explicit
>> --diff-merges=<value> is specified, right?
>
> Yes, I think a config there would be reasonable as long as:
>
>   - we have the command-line options to counteract it when necessary
>     (i.e., --diff-parents or your advanced --diff-merges exists, too)
>
>   - it only impacts porcelain "git log", and not plumbing like
>     diff-tree.

Yeah, these two are totally agreed upon.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 20:55                 ` Jeff King
@ 2020-08-03 21:18                   ` Sergey Organov
  0 siblings, 0 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-03 21:18 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Mon, Aug 03, 2020 at 11:00:11PM +0300, Sergey Organov wrote:
>
>> > 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).
>> 
>> I thought not of changing semantics of -m. Suppose we introduce
>> 
>>   --diff-merges=(none|<parent-number>|c|cc|all)
>> 
>> before your patch(es). Then your patch would read: "making --first-parent
>> imply --diff-merges=1" and it'd miss that --[no-]diff-merges part, no?
>
> Sure, that would be OK with me. You'd have --diff-merges=none to get
> the current behavior, and probably make --no-diff-merges an alias for
> that.

Yes, keeping --no-diff-merges as an alias might make sense, especially
if it's on top of yours.

>
>> > 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.
>> 
>> Can't we do yours on top instead? I'd expect it'd then be even simpler.
>
> Mine is in 'next', so there is no rebuilding it on top of anything else
> without a revert. But I don't see any particular reason to do that
> versus just changing the behavior on top. What's in 'next' is generally
> not rewound, but the behaviors are not cemented with respect to
> backwards compatibility.

Ah, now I see, thanks!

-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-03 18:08             ` Jeff King
  2020-08-03 20:00               ` Sergey Organov
@ 2020-08-04 17:50               ` Sergey Organov
  2020-08-04 19:42                 ` Junio C Hamano
  2020-08-04 19:58                 ` Jeff King
  1 sibling, 2 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 17:50 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

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


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 17:50               ` Sergey Organov
@ 2020-08-04 19:42                 ` Junio C Hamano
  2020-08-04 20:00                   ` Jeff King
  2020-08-04 19:58                 ` Jeff King
  1 sibling, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-04 19:42 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Jeff King, git, Chris Torek

Sergey Organov <sorganov@gmail.com> writes:

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

The overall direction is probably OK, but I wouldn't call it minimal.

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

tinyurl.com/gitCal

> As for me, I'm not sure 'combined-all-paths' should be included and if
> it should, is it a good enough name.

As a user, I do not think I can guess, from the option name, what
that option is trying to do.

As a minimum patch, I think it is OK to have just 'all' and 'none'
(not even c or cc, let alone the one with ultra-long name whose
effect is mystery) before we let the result graduate to 'master'.
Others can be added on top, as the primary focus of Peff's series is
to make sure "-m" can be countermanded, for which being able to say
"no" is sufficient, and the primary reason why we are further
futzing with the series with this addition is to leave the door open
for later additions of different "modes" in which how
"--diff-merges" option can operate (iow, Peff's was merely on/off,
but you are making sure others such as <num> can be added over
time).

Thanks.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 17:50               ` Sergey Organov
  2020-08-04 19:42                 ` Junio C Hamano
@ 2020-08-04 19:58                 ` Jeff King
  2020-08-04 20:56                   ` Sergey Organov
                                     ` (2 more replies)
  1 sibling, 3 replies; 73+ messages in thread
From: Jeff King @ 2020-08-04 19:58 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:

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

Thanks, I like the direction here. Definitely it would need
documentation, but also tests (probably in t4013 alongside the ones my
series added; in fact you'd probably need to adjust my tests for the
non-optional argument).

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

Should this be revs->ignore_merges = 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;

I think Junio's suggestion to push these out to a separate patch is a
good one.

It's unfortunate that we have to duplicate all of the various options
that get set (from the "--cc", etc, blocks). But I think the boilerplate
for pushing it into a helper would make it even harder to read.

> +		} else {
> +			die("--diff-merges: unknown value '%s'.", optarg);
> +		}

A few nits:

  - we usually don't have a period at the end of our error messages

  - this should probably be marked for translation, i.e.,
    die(_("translated message"), optarg)

  - I think other similar messages are more like:

      unknown value for --diff-merges: %s

> +		return argcount;
>  	} else if (!strcmp(arg, "--no-diff-merges")) {
>  		revs->ignore_merges = 1;

I thought at first that the --no- form would be handled by
parse_long_opt() via the parseopt code, but it is not using parseopt at
all. :) So it is correct to keep this.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 19:42                 ` Junio C Hamano
@ 2020-08-04 20:00                   ` Jeff King
  2020-08-04 20:55                     ` Sergey Organov
  2020-08-04 21:21                     ` Sergey Organov
  0 siblings, 2 replies; 73+ messages in thread
From: Jeff King @ 2020-08-04 20:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Sergey Organov, git, Chris Torek

On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote:

> As a minimum patch, I think it is OK to have just 'all' and 'none'
> (not even c or cc, let alone the one with ultra-long name whose
> effect is mystery) before we let the result graduate to 'master'.
> Others can be added on top, as the primary focus of Peff's series is
> to make sure "-m" can be countermanded, for which being able to say
> "no" is sufficient, and the primary reason why we are further
> futzing with the series with this addition is to leave the door open
> for later additions of different "modes" in which how
> "--diff-merges" option can operate (iow, Peff's was merely on/off,
> but you are making sure others such as <num> can be added over
> time).

I like that suggestion very much. It solves the "optional arguments are
evil" problem without having to worry about the other bits.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 20:00                   ` Jeff King
@ 2020-08-04 20:55                     ` Sergey Organov
  2020-08-04 21:22                       ` Jeff King
  2020-08-04 21:21                     ` Sergey Organov
  1 sibling, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 20:55 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote:
>
>> As a minimum patch, I think it is OK to have just 'all' and 'none'
>> (not even c or cc, let alone the one with ultra-long name whose
>> effect is mystery) before we let the result graduate to 'master'.
>> Others can be added on top, as the primary focus of Peff's series is
>> to make sure "-m" can be countermanded, for which being able to say
>> "no" is sufficient, and the primary reason why we are further
>> futzing with the series with this addition is to leave the door open
>> for later additions of different "modes" in which how
>> "--diff-merges" option can operate (iow, Peff's was merely on/off,
>> but you are making sure others such as <num> can be added over
>> time).
>
> I like that suggestion very much. It solves the "optional arguments are
> evil" problem without having to worry about the other bits.

So do I,  will do in a few minutes.

I only don't like --diff-merges=none (even though it sounds great for
--diff-parents=none) and used --diff-merges=off instead. It's not a
strong feeling though, and I'm fine with whatever we decide.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 19:58                 ` Jeff King
@ 2020-08-04 20:56                   ` Sergey Organov
  2020-08-04 21:25                     ` Jeff King
  2020-08-04 21:27                     ` Junio C Hamano
  2020-08-04 21:58                   ` Sergey Organov
  2020-08-05 15:37                   ` [PATCH v2 0/7] making log --first-parent imply -m Sergey Organov
  2 siblings, 2 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 20:56 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:
>
>> 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.
>
> Thanks, I like the direction here. Definitely it would need
> documentation, but also tests (probably in t4013 alongside the ones my
> series added; in fact you'd probably need to adjust my tests for the
> non-optional argument).
>
>> 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;
>
> Should this be revs->ignore_merges = 0?

It's 4 lines above, as it's in fact common for all the cases but the
first one.

-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 20:00                   ` Jeff King
  2020-08-04 20:55                     ` Sergey Organov
@ 2020-08-04 21:21                     ` Sergey Organov
  2020-08-04 21:26                       ` Jeff King
  1 sibling, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 21:21 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

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

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2020 at 12:42:45PM -0700, Junio C Hamano wrote:
>
>> As a minimum patch, I think it is OK to have just 'all' and 'none'
>> (not even c or cc, let alone the one with ultra-long name whose
>> effect is mystery) before we let the result graduate to 'master'.
>> Others can be added on top, as the primary focus of Peff's series is
>> to make sure "-m" can be countermanded, for which being able to say
>> "no" is sufficient, and the primary reason why we are further
>> futzing with the series with this addition is to leave the door open
>> for later additions of different "modes" in which how
>> "--diff-merges" option can operate (iow, Peff's was merely on/off,
>> but you are making sure others such as <num> can be added over
>> time).
>
> I like that suggestion very much. It solves the "optional arguments are
> evil" problem without having to worry about the other bits.

Here is the patch reduced to absolute minimum, both functionally and
textually. I removed even 'all', as it has its own subtleties that need
further discussion, so the patch only introduces --diff-merges=off.

If it looks OK, I'll do documentation and tests parts.

Thanks,
-- 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: 1417 bytes --]

From 34cba8c49e2c40fbc228b9904491633939792b6d 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 v2] revision: change "--diff-merges" option to require
 parameter

--diff-merges=off is the only accepted form for now, a synonym for
--no-diff-merges.

This patch is a preparation for adding more values, as well as supporting
--diff-merges=<parent>, where <parent> is single parent number to output diff
against.

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

diff --git a/revision.c b/revision.c
index 669bc856694f..88a0bfdb4a4c 100644
--- a/revision.c
+++ b/revision.c
@@ -2323,8 +2323,15 @@ 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 (!strcmp(arg, "-m")) {
 		revs->ignore_merges = 0;
+	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
+		if (!strcmp(optarg, "off")) {
+			revs->ignore_merges = 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, "-c")) {
-- 
2.20.1


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 20:55                     ` Sergey Organov
@ 2020-08-04 21:22                       ` Jeff King
  2020-08-04 21:55                         ` Junio C Hamano
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-08-04 21:22 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote:

> I only don't like --diff-merges=none (even though it sounds great for
> --diff-parents=none) and used --diff-merges=off instead. It's not a
> strong feeling though, and I'm fine with whatever we decide.

I think that is fine. I took "none" to be "diff against none of the
parents", which is the opposite of "all". But "off" conveys that, too.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 20:56                   ` Sergey Organov
@ 2020-08-04 21:25                     ` Jeff King
  2020-08-04 21:41                       ` Sergey Organov
  2020-08-04 21:27                     ` Junio C Hamano
  1 sibling, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-08-04 21:25 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote:

> >> 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;
> >
> > Should this be revs->ignore_merges = 0?
> 
> It's 4 lines above, as it's in fact common for all the cases but the
> first one.

Ah, I missed that. That raises more questions, though. ;)

For "-m" we do not need to set revs->diff; why do we need to do so
here?

For "--cc", we do not need to set revs->ignore_merges. Why do we need to
do so here? We do need it set eventually, but I think setup_revisions()
later handles that, and wants ignore_merges untouched to decide whether
the user asked for it or not.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 21:21                     ` Sergey Organov
@ 2020-08-04 21:26                       ` Jeff King
  0 siblings, 0 replies; 73+ messages in thread
From: Jeff King @ 2020-08-04 21:26 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Wed, Aug 05, 2020 at 12:21:03AM +0300, Sergey Organov wrote:

> Here is the patch reduced to absolute minimum, both functionally and
> textually. I removed even 'all', as it has its own subtleties that need
> further discussion, so the patch only introduces --diff-merges=off.

Yeah, I agree this is the minimum (though I suspect the documentation
may be easier with "all" or similar to explain "-m" in terms of
--diff-merges).

> If it looks OK, I'll do documentation and tests parts.

My nits about the die() message remain, but other than that it looks OK
to me.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 20:56                   ` Sergey Organov
  2020-08-04 21:25                     ` Jeff King
@ 2020-08-04 21:27                     ` Junio C Hamano
  2020-08-04 21:44                       ` Sergey Organov
  1 sibling, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-04 21:27 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Jeff King, git, Chris Torek

Sergey Organov <sorganov@gmail.com> writes:

> Jeff King <peff@peff.net> writes:
>
>>> +	} 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;
>>
>> Should this be revs->ignore_merges = 0?
>
> It's 4 lines above, as it's in fact common for all the cases but the
> first one.

I may be mistaken, but I thought Peff was asking about turning
revs->diff off.  I somehow thought that the equivalence planned for
the short term is:

            (new)               (peff's)         (master)
	diff-merges=none == --no-diff-merges == ! -m
	diff-merges=all  == --diff-merges    == -m

and future extension would add equivalents to --cc and -c, in
addition to <parentNum>, which is not something we can do with the
current UI and machinery.

So, shouldn't the body of that else if clause for "all" be a no-op?
i.e.

		} else if (!strcmp(optarg, "all")) {
			; /* nothing */
		}



^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 21:25                     ` Jeff King
@ 2020-08-04 21:41                       ` Sergey Organov
  2020-08-04 22:07                         ` Jeff King
  0 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 21:41 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote:
>
>> >> 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;
>> >
>> > Should this be revs->ignore_merges = 0?
>> 
>> It's 4 lines above, as it's in fact common for all the cases but the
>> first one.
>
> Ah, I missed that. That raises more questions, though. ;)
>
> For "-m" we do not need to set revs->diff; why do we need to do so
> here?

Good question!

I believe --diff-merges=all should reset revs->diff back to 0 to
entirely undo all the effects of '-c' and '--cc', provided those
appeared before on the command-line, that '-m' fails to do.

Admittedly, I didn't yet check what is the outcome of, say,
"git log -c -m". Is it = "-c", ="-m", or what?

Also, this makes 'all' not the entire synonym for '-m', and that's why I
removed 'all' from the second version of the patch ;-)

>
> For "--cc", we do not need to set revs->ignore_merges. Why do we need to
> do so here? We do need it set eventually, but I think setup_revisions()
> later handles that, and wants ignore_merges untouched to decide whether
> the user asked for it or not.

OK, I'll take further look at this for further changes, -- thanks for
telling!

My general approach though is that every of mutually exclusive options
should better explicitly set all the involved variables appropriately.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 21:27                     ` Junio C Hamano
@ 2020-08-04 21:44                       ` Sergey Organov
  0 siblings, 0 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 21:44 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Chris Torek

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>>>> +	} 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;
>>>
>>> Should this be revs->ignore_merges = 0?
>>
>> It's 4 lines above, as it's in fact common for all the cases but the
>> first one.
>
> I may be mistaken, but I thought Peff was asking about turning
> revs->diff off.

No, but this one was in his follow-up that I already answered a few
minutes ago.

> I somehow thought that the equivalence planned for
> the short term is:
>
>             (new)               (peff's)         (master)
> 	diff-merges=none == --no-diff-merges == ! -m
> 	diff-merges=all  == --diff-merges    == -m

The second one is somewhat problematic, so I excluded it for now (see
aforementioned answer for more discussion).

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 21:22                       ` Jeff King
@ 2020-08-04 21:55                         ` Junio C Hamano
  2020-08-04 22:06                           ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-04 21:55 UTC (permalink / raw)
  To: Jeff King; +Cc: Sergey Organov, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote:
>
>> I only don't like --diff-merges=none (even though it sounds great for
>> --diff-parents=none) and used --diff-merges=off instead. It's not a
>> strong feeling though, and I'm fine with whatever we decide.
>
> I think that is fine. I took "none" to be "diff against none of the
> parents", which is the opposite of "all". But "off" conveys that, too.

For now, "off" is OK, but then we'll regret when "all" comes,
because "off" would not exactly sit opposite to "all".  On the other
hand, "none" would: Compare with all parents?  Compare with none of
the parents?

Thanks.


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 19:58                 ` Jeff King
  2020-08-04 20:56                   ` Sergey Organov
@ 2020-08-04 21:58                   ` Sergey Organov
  2020-08-04 22:08                     ` Jeff King
  2020-08-05 15:37                   ` [PATCH v2 0/7] making log --first-parent imply -m Sergey Organov
  2 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 21:58 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:

[...]

>
>> +		} else {
>> +			die("--diff-merges: unknown value '%s'.", optarg);
>> +		}
>
> A few nits:

I missed this the first time, sorry!

>
>   - we usually don't have a period at the end of our error messages

Oops, I got the dot from

  die("--unpacked=<packfile> no longer supported.");

in the same file. Will fix.

>   - this should probably be marked for translation, i.e.,
>     die(_("translated message"), optarg)

OK, will do.

>
>   - I think other similar messages are more like:
>
>       unknown value for --diff-merges: %s

Thanks, I'll change wording to this one.

-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 21:55                         ` Junio C Hamano
@ 2020-08-04 22:06                           ` Sergey Organov
  2020-08-04 22:14                             ` Jeff King
  0 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 22:06 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Chris Torek

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

> Jeff King <peff@peff.net> writes:
>
>> On Tue, Aug 04, 2020 at 11:55:19PM +0300, Sergey Organov wrote:
>>
>>> I only don't like --diff-merges=none (even though it sounds great for
>>> --diff-parents=none) and used --diff-merges=off instead. It's not a
>>> strong feeling though, and I'm fine with whatever we decide.
>>
>> I think that is fine. I took "none" to be "diff against none of the
>> parents", which is the opposite of "all". But "off" conveys that, too.
>
> For now, "off" is OK, but then we'll regret when "all" comes,
> because "off" would not exactly sit opposite to "all".

IMHO, "off" does not need to be opposite for "all", as it suppresses
diff output altogether. I read --diff-merge=off as "turn /off/ diff
output for merge commits".

Besides, "all", that I don't like either, is among "c" and "cc", all 3
being different versions of diffs against all the parents, no?

> On the other hand, "none" would: Compare with all parents? Compare
> with none of the parents?

I think "none" would have been appropriate for --diff-parents indeed,
but not for --diff-merges.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 21:41                       ` Sergey Organov
@ 2020-08-04 22:07                         ` Jeff King
  2020-08-04 22:15                           ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-08-04 22:07 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote:

> >> It's 4 lines above, as it's in fact common for all the cases but the
> >> first one.
> >
> > Ah, I missed that. That raises more questions, though. ;)
> >
> > For "-m" we do not need to set revs->diff; why do we need to do so
> > here?
> 
> Good question!
> 
> I believe --diff-merges=all should reset revs->diff back to 0 to
> entirely undo all the effects of '-c' and '--cc', provided those
> appeared before on the command-line, that '-m' fails to do.

Making it counteract "--cc" makes sense, but revs->diff is used for much
more than that. So "--cc" sets revs->diff to 1, but so do many other
unrelated options (e.g., "--full-diff" for one).

I think to do it right you'd need to have this part of the code just set
a single enum variable, like:

  ...
  else if (!strcmp(arg, "--cc")) {
          revs->diff_merges = DIFF_MERGES_DENSE_COMBINED;
  } else if (!strcmp(arg, "-m")) {
          revs->diff_merges = DIFF_MERGES_ALL_PARENTS;
  }
  ...etc...

and then later resolve that into the set of flags we need:

  switch (revs->diff_merges) {
  case DIFF_MERGES_DENSE_COMBINED:
	revs->diff = 1;
	revs->dense_combined_merges = 1;
	revs->combine_merges = 1;
	revs->ignore_merges = 0;
	break;
  case DIFF_MERGES_ALL_PARENTS:
	revs->ignore_merges = 0;
	break;
  ...etc...
  }

it may even be that we can get rid of the separate combine_merges and
dense_combined_merges flag and just have callers look at
rev.diff_merges, which would simplify the code even further. But that
cleanup could also come later on top.

> Admittedly, I didn't yet check what is the outcome of, say,
> "git log -c -m". Is it = "-c", ="-m", or what?

Having looked at the code, I'm pretty sure it behaves like "-c". IMHO
that is a bug and the two should be mutually exclusive (i.e., override
each other). Changing that would not be strictly backwards, but I think
it may be OK under the notion that the current behavior is nonsense.

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 21:58                   ` Sergey Organov
@ 2020-08-04 22:08                     ` Jeff King
  2020-08-04 22:26                       ` [PATCH] revision: fix die() message for "--unpacked=" Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-08-04 22:08 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Wed, Aug 05, 2020 at 12:58:03AM +0300, Sergey Organov wrote:

> >   - we usually don't have a period at the end of our error messages
> 
> Oops, I got the dot from
> 
>   die("--unpacked=<packfile> no longer supported.");
> 
> in the same file. Will fix.

Yeah, there are a few that have snuck in. I wouldn't be opposed to a
patch to fix that one. :)

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 22:06                           ` Sergey Organov
@ 2020-08-04 22:14                             ` Jeff King
  2020-08-04 22:49                               ` Junio C Hamano
  2020-08-04 22:53                               ` Sergey Organov
  0 siblings, 2 replies; 73+ messages in thread
From: Jeff King @ 2020-08-04 22:14 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Junio C Hamano, git, Chris Torek

On Wed, Aug 05, 2020 at 01:06:51AM +0300, Sergey Organov wrote:

> > For now, "off" is OK, but then we'll regret when "all" comes,
> > because "off" would not exactly sit opposite to "all".
> 
> IMHO, "off" does not need to be opposite for "all", as it suppresses
> diff output altogether. I read --diff-merge=off as "turn /off/ diff
> output for merge commits".
> 
> Besides, "all", that I don't like either, is among "c" and "cc", all 3
> being different versions of diffs against all the parents, no?

I think "all-parents" is much more descriptive than "all" (which might
make you think "all merges", but it has nothing to do with that). It
would make more sense if we later add the building to say "diff against
parent 1" or "diff against parents 1 and 3".

You might also consider whether "combined" is actually mutually
exclusive with parent selection. We have focused on which parents you'd
want to "-m" against. But in the most general case, you could ask for a
combined-diff between parents 1 and 3 of an octopus merge.

That's just coming from the angle of "what is the most general and
orthogonal set of features". I think the vast majority of what anyone
would want to do would be covered by doing a diff against only a single
parent, and then it would almost always be the first parent. And
certainly you'd need to add a bunch of code to the combined diff
machinery to make it support arbitrary sets of parents. So this probably
isn't that interesting a direction to go, at least for now. I'm just
raising the issue now because we'll be locked into the semantics of this
option, which may not be able to express the full set of what's possible
(so we'd be stuck adding another option later).

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 22:07                         ` Jeff King
@ 2020-08-04 22:15                           ` Sergey Organov
  0 siblings, 0 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 22:15 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote:
>
>> >> It's 4 lines above, as it's in fact common for all the cases but the
>> >> first one.
>> >
>> > Ah, I missed that. That raises more questions, though. ;)
>> >
>> > For "-m" we do not need to set revs->diff; why do we need to do so
>> > here?
>> 
>> Good question!
>> 
>> I believe --diff-merges=all should reset revs->diff back to 0 to
>> entirely undo all the effects of '-c' and '--cc', provided those
>> appeared before on the command-line, that '-m' fails to do.
>
> Making it counteract "--cc" makes sense, but revs->diff is used for much
> more than that. So "--cc" sets revs->diff to 1, but so do many other
> unrelated options (e.g., "--full-diff" for one).
>
> I think to do it right you'd need to have this part of the code just set
> a single enum variable, like:
>
>   ...
>   else if (!strcmp(arg, "--cc")) {
>           revs->diff_merges = DIFF_MERGES_DENSE_COMBINED;
>   } else if (!strcmp(arg, "-m")) {
>           revs->diff_merges = DIFF_MERGES_ALL_PARENTS;
>   }
>   ...etc...
>
> and then later resolve that into the set of flags we need:
>
>   switch (revs->diff_merges) {
>   case DIFF_MERGES_DENSE_COMBINED:
> 	revs->diff = 1;
> 	revs->dense_combined_merges = 1;
> 	revs->combine_merges = 1;
> 	revs->ignore_merges = 0;
> 	break;
>   case DIFF_MERGES_ALL_PARENTS:
> 	revs->ignore_merges = 0;
> 	break;
>   ...etc...
>   }
>
> it may even be that we can get rid of the separate combine_merges and
> dense_combined_merges flag and just have callers look at
> rev.diff_merges, which would simplify the code even further. But that
> cleanup could also come later on top.

Sounds like a plan! I like it.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* [PATCH] revision: fix die() message for "--unpacked="
  2020-08-04 22:08                     ` Jeff King
@ 2020-08-04 22:26                       ` Sergey Organov
  2020-08-05  0:03                         ` Junio C Hamano
  0 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 22:26 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git


Get rid of the trailing dot and mark for translation.

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

diff --git a/revision.c b/revision.c
index 669bc856694f..d08cb5c0e9cd 100644
--- a/revision.c
+++ b/revision.c
@@ -2315,7 +2315,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--unpacked")) {
 		revs->unpacked = 1;
 	} else if (starts_with(arg, "--unpacked=")) {
-		die("--unpacked=<packfile> no longer supported.");
+		die(_("--unpacked=<packfile> no longer supported"));
 	} else if (!strcmp(arg, "-r")) {
 		revs->diff = 1;
 		revs->diffopt.flags.recursive = 1;
-- 
2.20.1

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 22:14                             ` Jeff King
@ 2020-08-04 22:49                               ` Junio C Hamano
  2020-08-07  8:26                                 ` Jeff King
  2020-08-04 22:53                               ` Sergey Organov
  1 sibling, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-04 22:49 UTC (permalink / raw)
  To: Jeff King; +Cc: Sergey Organov, git, Chris Torek

Jeff King <peff@peff.net> writes:

> You might also consider whether "combined" is actually mutually
> exclusive with parent selection. We have focused on which parents you'd
> want to "-m" against. But in the most general case, you could ask for a
> combined-diff between parents 1 and 3 of an octopus merge.

Yeah, we want to specify a possibly empty set of integers
(1..<num-parents>) to combine the diff; if it is empty set, we won't
see any diff.  If it is full set, we'd get the current c/cc behavior.
Anything in between we cannot currently express.  Fun ;-)

> That's just coming from the angle of "what is the most general and
> orthogonal set of features". I think the vast majority of what anyone
> would want to do would be covered by doing a diff against only a single
> parent, and then it would almost always be the first parent. And
> certainly you'd need to add a bunch of code to the combined diff
> machinery to make it support arbitrary sets of parents. So this probably
> isn't that interesting a direction to go, at least for now. 

Yeah, it is mostly for fun--I do not see an immediate practical use
case, either.

> I'm just
> raising the issue now because we'll be locked into the semantics of this
> option, which may not be able to express the full set of what's possible
> (so we'd be stuck adding another option later).

Yeah, but a good thing is that we won't have to worry about this
until much later, as long as we would just be introducing "diff
against no parents" and nothing else (or together with "diff against
all parents", which would make it easier to explain "-m").


^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 22:14                             ` Jeff King
  2020-08-04 22:49                               ` Junio C Hamano
@ 2020-08-04 22:53                               ` Sergey Organov
  1 sibling, 0 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-04 22:53 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Wed, Aug 05, 2020 at 01:06:51AM +0300, Sergey Organov wrote:
>
>> > For now, "off" is OK, but then we'll regret when "all" comes,
>> > because "off" would not exactly sit opposite to "all".
>> 
>> IMHO, "off" does not need to be opposite for "all", as it suppresses
>> diff output altogether. I read --diff-merge=off as "turn /off/ diff
>> output for merge commits".
>> 
>> Besides, "all", that I don't like either, is among "c" and "cc", all 3
>> being different versions of diffs against all the parents, no?
>
> I think "all-parents" is much more descriptive than "all" (which might
> make you think "all merges", but it has nothing to do with that). It
> would make more sense if we later add the building to say "diff against
> parent 1" or "diff against parents 1 and 3".
>
> You might also consider whether "combined" is actually mutually
> exclusive with parent selection. We have focused on which parents you'd
> want to "-m" against. But in the most general case, you could ask for a
> combined-diff between parents 1 and 3 of an octopus merge.
>
> That's just coming from the angle of "what is the most general and
> orthogonal set of features". I think the vast majority of what anyone
> would want to do would be covered by doing a diff against only a single
> parent, and then it would almost always be the first parent. And
> certainly you'd need to add a bunch of code to the combined diff
> machinery to make it support arbitrary sets of parents. So this probably
> isn't that interesting a direction to go, at least for now. I'm just
> raising the issue now because we'll be locked into the semantics of this
> option, which may not be able to express the full set of what's possible
> (so we'd be stuck adding another option later).

Makes sense, and I got an idea.

--diff-merges=<parent> will still give diff against one specific parent.

In case of combined/separate diffs, it will produce diffs against all
the parents that, if happens to be needed, could later be refined by a
new --diff-parents option that defaults to 'all'.

Then, for example,

--diff-merges=1

would finally be just a short-cut for

--diff-merges=separate --diff-parents=1

while

--diff-merges=separate --diff-parents=all

would be the same as

--diff-merges=separate (what we called "all" so far)

and then we may have

--diff-merges=condensed-combined --diff-parents=1-3,8

for the bravest ;-)

This would lead us, currently to:

--diff-merges=(off,separate,combined,condensed-combined,<parent>)

and leave us ability to implement advanced parents selection, in an
unlikely case it's needed, in a separate new option.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH] revision: fix die() message for "--unpacked="
  2020-08-04 22:26                       ` [PATCH] revision: fix die() message for "--unpacked=" Sergey Organov
@ 2020-08-05  0:03                         ` Junio C Hamano
  0 siblings, 0 replies; 73+ messages in thread
From: Junio C Hamano @ 2020-08-05  0:03 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Jeff King, git

Sergey Organov <sorganov@gmail.com> writes:

> Get rid of the trailing dot and mark for translation.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  revision.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/revision.c b/revision.c
> index 669bc856694f..d08cb5c0e9cd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2315,7 +2315,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if (!strcmp(arg, "--unpacked")) {
>  		revs->unpacked = 1;
>  	} else if (starts_with(arg, "--unpacked=")) {
> -		die("--unpacked=<packfile> no longer supported.");
> +		die(_("--unpacked=<packfile> no longer supported"));
>  	} else if (!strcmp(arg, "-r")) {
>  		revs->diff = 1;
>  		revs->diffopt.flags.recursive = 1;

Makes sense.  Will queue.  thanks.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 19:58                 ` Jeff King
  2020-08-04 20:56                   ` Sergey Organov
  2020-08-04 21:58                   ` Sergey Organov
@ 2020-08-05 15:37                   ` Sergey Organov
  2020-08-05 16:05                     ` Junio C Hamano
  2 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-05 15:37 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2020 at 08:50:16PM +0300, Sergey Organov wrote:
>
>> 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.
>
> Thanks, I like the direction here. Definitely it would need
> documentation, but also tests (probably in t4013 alongside the ones my
> series added; in fact you'd probably need to adjust my tests for the
> non-optional argument).

I turned to tests, and found that I have a doubt about the test
you've added:

git log --no-diff-merges -p --first-parent master

In modified tests, I'd like to move --no-diff-merges to the end, for the
test to be less restrictive:

git log -p --first-parent --no-diff-merges master

It should change nothing for now, but it will allow us in the future to
get rid of mutual dependencies between in -m and --first-parent in favor
of --first-parent to imply --diff-merges=1. We then will need to
override the latter by subsequent --no-diff-merges:

git log -p --first-parent [--diff-merges=1: implied] --no-diff-merges master

In this case your original test:

git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master

would fail, as implied --diff-merges=1 then wins.

Then I'm going to add a copy:

git log -p --first-parent --diff-merges=off master

to check that this form works as well.

What do you think?

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-05 15:37                   ` [PATCH v2 0/7] making log --first-parent imply -m Sergey Organov
@ 2020-08-05 16:05                     ` Junio C Hamano
  2020-08-05 17:55                       ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-05 16:05 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Jeff King, git, Chris Torek

Sergey Organov <sorganov@gmail.com> writes:

> Jeff King <peff@peff.net> writes:
> ...
> In this case your original test:
>
> git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master
>
> would fail, as implied --diff-merges=1 then wins.

IMHO, I think this is an absolutely wrong thing to do.  At least to
me (and I suspect it would be to many users), what "--first-parent
implies 'when showing a diff, compare only with the first parent'"
means is that it should do so unless told to do otherwise.

    git log --no-diff-merges -p --first-parent

explicitly tells the command that the user does not want to see
patches for merge commits.  I do not see any reason why
"--first-parent", which merely *implies* a specific diff generation
preference for merges, countermand it.  IOW the implication is
conditional.

It is like saying

    git log --first-parent

should show patches because it *implies* comparing only with the
first parent, but you can see why it is wrong.  It is because that
implication is conditional---it kicks in only when the command is
told to compare with any parent (i.e. "-p").  

I.e. the implication is "compare only with the first parent if told
to compare, and if not told what to compare with explicitly".

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-05 16:05                     ` Junio C Hamano
@ 2020-08-05 17:55                       ` Sergey Organov
  2020-08-05 19:24                         ` Junio C Hamano
  0 siblings, 1 reply; 73+ messages in thread
From: Sergey Organov @ 2020-08-05 17:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Chris Torek

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>> ...
>> In this case your original test:
>>
>> git log --no-diff-merges -p --first-parent [--diff-merges=1: implied] master
>>
>> would fail, as implied --diff-merges=1 then wins.
>
> IMHO, I think this is an absolutely wrong thing to do.  At least to
> me (and I suspect it would be to many users), what "--first-parent
> implies 'when showing a diff, compare only with the first parent'"
> means is that it should do so unless told to do otherwise.
>
>     git log --no-diff-merges -p --first-parent
>
> explicitly tells the command that the user does not want to see
> patches for merge commits.  I do not see any reason why
> "--first-parent", which merely *implies* a specific diff generation
> preference for merges, countermand it.  IOW the implication is
> conditional.
>
> It is like saying
>
>     git log --first-parent
>
> should show patches because it *implies* comparing only with the
> first parent, but you can see why it is wrong.  It is because that
> implication is conditional---it kicks in only when the command is
> told to compare with any parent (i.e. "-p").  
>
> I.e. the implication is "compare only with the first parent if told
> to compare, and if not told what to compare with explicitly".

I believe my approach is more straightforward and is free from
interpretations.

To make my point let's get back to the subject (for a moment :-)).

To me "--first-parent implies -m" is simple and unambiguous:

(git log [*] --first-parent [*]) == (git log [*] --first-parent -m [*])

No further explanations are needed.

The consequence is that an option that is supposed to override -m must
follow -m, and thus --first-parent, not precede it, period.

Yes, we can invent the rule that implied options don't participate in
overriding of explicit options, or that explicit option always overrides
all the implicit, or some such, but I see absolutely no reason to
complicate the model.

Thanks,
-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-05 17:55                       ` Sergey Organov
@ 2020-08-05 19:24                         ` Junio C Hamano
  2020-08-05 20:27                           ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Junio C Hamano @ 2020-08-05 19:24 UTC (permalink / raw)
  To: Sergey Organov; +Cc: Jeff King, git, Chris Torek

Sergey Organov <sorganov@gmail.com> writes:

> Yes, we can invent the rule that implied options don't ...

"invent"?  It is nothing new, isn't it?  IIRC, Peff's "first-parent
implies 'm' but can be countermanded with --no-diff-merges" defines
"implication" exactly that way.  I do not think that is a recent
invention but it is just following the patterns set by other options
that has conditional implications.

IOW,

	$ git log --no-diff-merges --first-parent -p next
	$ git log --first-parent -p --no-diff-merges next

should both mean the same thing.  The user said no patch is wanted
for merge commits with --no-diff-merges and --first-parent does not
affect it.



^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-05 19:24                         ` Junio C Hamano
@ 2020-08-05 20:27                           ` Sergey Organov
  0 siblings, 0 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-05 20:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Chris Torek

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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Yes, we can invent the rule that implied options don't ...
>
> "invent"?  It is nothing new, isn't it?  IIRC, Peff's "first-parent
> implies 'm' but can be countermanded with --no-diff-merges" defines
> "implication" exactly that way.  I do not think that is a recent
> invention but it is just following the patterns set by other options
> that has conditional implications.
>
> IOW,
>
> 	$ git log --no-diff-merges --first-parent -p next
> 	$ git log --first-parent -p --no-diff-merges next
>
> should both mean the same thing.  The user said no patch is wanted
> for merge commits with --no-diff-merges and --first-parent does not
> affect it.

I disagree, but I drop the issue for now for the goal of making sensible
progress with --diff-merges. I'll do the patches without this
modifications of the tests.

Thanks,
-- Sergey.

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-04 22:49                               ` Junio C Hamano
@ 2020-08-07  8:26                                 ` Jeff King
  2020-08-07  9:25                                   ` Sergey Organov
  0 siblings, 1 reply; 73+ messages in thread
From: Jeff King @ 2020-08-07  8:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Sergey Organov, git, Chris Torek

On Tue, Aug 04, 2020 at 03:49:17PM -0700, Junio C Hamano wrote:

> > I'm just
> > raising the issue now because we'll be locked into the semantics of this
> > option, which may not be able to express the full set of what's possible
> > (so we'd be stuck adding another option later).
> 
> Yeah, but a good thing is that we won't have to worry about this
> until much later, as long as we would just be introducing "diff
> against no parents" and nothing else (or together with "diff against
> all parents", which would make it easier to explain "-m").

Agreed. My only question is whether the possibility of later having
those other options might influence how we name the two options we add
now. I think it's clear to all of us in this thread how those two easy
options should behave, but if the intent is to eventually allow these to
be mutually exclusive:

  - no diff
  - combined
  - dense combined
  - individual diff against each parent

but orthogonal to the selection of the parent-set (none, all, or
selected ones) then e.g. "all" makes less sense for "individual diff
against each parent". I don't have a good succinct name suggestion,
though.

TBH, I would be happy enough with any of the suggestions in the thread,
so I am really just finishing the thought here, and not trying to derail
progress. :)

-Peff

^ permalink raw reply	[flat|nested] 73+ messages in thread

* Re: [PATCH v2 0/7] making log --first-parent imply -m
  2020-08-07  8:26                                 ` Jeff King
@ 2020-08-07  9:25                                   ` Sergey Organov
  0 siblings, 0 replies; 73+ messages in thread
From: Sergey Organov @ 2020-08-07  9:25 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git, Chris Torek

Jeff King <peff@peff.net> writes:

> On Tue, Aug 04, 2020 at 03:49:17PM -0700, Junio C Hamano wrote:
>
>> > I'm just
>> > raising the issue now because we'll be locked into the semantics of this
>> > option, which may not be able to express the full set of what's possible
>> > (so we'd be stuck adding another option later).
>> 
>> Yeah, but a good thing is that we won't have to worry about this
>> until much later, as long as we would just be introducing "diff
>> against no parents" and nothing else (or together with "diff against
>> all parents", which would make it easier to explain "-m").
>
> Agreed. My only question is whether the possibility of later having
> those other options might influence how we name the two options we add
> now. I think it's clear to all of us in this thread how those two easy
> options should behave, but if the intent is to eventually allow these to
> be mutually exclusive:
>
>   - no diff
>   - combined
>   - dense combined
>   - individual diff against each parent
>
> but orthogonal to the selection of the parent-set (none, all, or
> selected ones) then e.g. "all" makes less sense for "individual diff
> against each parent". I don't have a good succinct name suggestion,
> though.

I have "split" and "separate" in mind, the latter likely shortened to
"sep".

Overall:

--diff-merges=(off,none|comb|dense,dense-comb,comb-dense|sep,split)

-- Sergey

^ permalink raw reply	[flat|nested] 73+ messages in thread

end of thread, back to index

Thread overview: 73+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 16:36 [PATCH 0/3] making --first-parent imply -m Jeff King
2020-07-28 16:36 ` [PATCH 1/3] log: drop "--cc implies -m" logic Jeff King
2020-07-28 16:38 ` [PATCH 2/3] revision: add "--ignore-merges" option to counteract "-m" Jeff King
2020-07-28 17:52   ` Chris Torek
2020-07-29 18:42     ` Jeff King
2020-07-28 21:01   ` Junio C Hamano
2020-07-29 18:22     ` Jeff King
2020-07-29 18:30       ` Jeff King
2020-07-29 18:53         ` Junio C Hamano
2020-07-28 16:39 ` [PATCH 3/3] log: enable "-m" automatically with "--first-parent" Jeff King
2020-07-28 16:47 ` [PATCH 0/3] making --first-parent imply -m Jeff King
2020-07-28 16:48 ` Junio C Hamano
2020-07-28 16:53   ` Jeff King
2020-07-28 16:55 ` Taylor Blau
2020-07-29 20:10 ` [PATCH v2 0/7] making log " Jeff King
2020-07-29 20:10   ` [PATCH v2 1/7] log: drop "--cc implies -m" logic Jeff King
2020-07-29 20:10   ` [PATCH v2 2/7] revision: add "--no-diff-merges" option to counteract "-m" Jeff King
2020-07-29 20:10   ` [PATCH v2 3/7] log: enable "-m" automatically with "--first-parent" Jeff King
2020-07-29 20:28     ` Junio C Hamano
2020-07-29 20:11   ` [PATCH v2 4/7] doc/git-log: move "Diff Formatting" from rev-list-options Jeff King
2020-07-29 20:56     ` Junio C Hamano
2020-07-29 21:02       ` Jeff King
2020-07-29 20:11   ` [PATCH v2 5/7] doc/git-log: drop "-r" diff option Jeff King
2020-07-29 20:12   ` [PATCH v2 6/7] doc/git-log: move "-t" into diff-options list Jeff King
2020-07-29 21:10     ` Junio C Hamano
2020-07-29 21:55       ` Jeff King
2020-07-29 20:12   ` [PATCH v2 7/7] doc/git-log: clarify handling of merge commit diffs Jeff King
2020-07-29 21:03   ` [PATCH v2 0/7] making log --first-parent imply -m Chris Torek
2020-07-29 21:41   ` Sergey Organov
2020-07-31 23:08     ` Jeff King
2020-08-02 12:59       ` Sergey Organov
2020-08-02 17:35         ` Junio C Hamano
2020-08-03 15:47           ` Sergey Organov
2020-08-03 16:35             ` Junio C Hamano
2020-08-03 16:41               ` Sergey Organov
2020-08-03 17:21                 ` Junio C Hamano
2020-08-03 20:25                   ` Sergey Organov
2020-08-03 20:58                     ` Jeff King
2020-08-03 21:16                       ` Sergey Organov
2020-08-03 18:08             ` Jeff King
2020-08-03 20:00               ` Sergey Organov
2020-08-03 20:55                 ` Jeff King
2020-08-03 21:18                   ` Sergey Organov
2020-08-04 17:50               ` Sergey Organov
2020-08-04 19:42                 ` Junio C Hamano
2020-08-04 20:00                   ` Jeff King
2020-08-04 20:55                     ` Sergey Organov
2020-08-04 21:22                       ` Jeff King
2020-08-04 21:55                         ` Junio C Hamano
2020-08-04 22:06                           ` Sergey Organov
2020-08-04 22:14                             ` Jeff King
2020-08-04 22:49                               ` Junio C Hamano
2020-08-07  8:26                                 ` Jeff King
2020-08-07  9:25                                   ` Sergey Organov
2020-08-04 22:53                               ` Sergey Organov
2020-08-04 21:21                     ` Sergey Organov
2020-08-04 21:26                       ` Jeff King
2020-08-04 19:58                 ` Jeff King
2020-08-04 20:56                   ` Sergey Organov
2020-08-04 21:25                     ` Jeff King
2020-08-04 21:41                       ` Sergey Organov
2020-08-04 22:07                         ` Jeff King
2020-08-04 22:15                           ` Sergey Organov
2020-08-04 21:27                     ` Junio C Hamano
2020-08-04 21:44                       ` Sergey Organov
2020-08-04 21:58                   ` Sergey Organov
2020-08-04 22:08                     ` Jeff King
2020-08-04 22:26                       ` [PATCH] revision: fix die() message for "--unpacked=" Sergey Organov
2020-08-05  0:03                         ` Junio C Hamano
2020-08-05 15:37                   ` [PATCH v2 0/7] making log --first-parent imply -m Sergey Organov
2020-08-05 16:05                     ` Junio C Hamano
2020-08-05 17:55                       ` Sergey Organov
2020-08-05 19:24                         ` Junio C Hamano
2020-08-05 20:27                           ` Sergey Organov

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git