diff mbox series

[2/3] revision: add "--ignore-merges" option to counteract "-m"

Message ID 20200728163853.GB2650252@coredump.intra.peff.net
State New, archived
Headers show
Series making --first-parent imply -m | expand

Commit Message

Jeff King July 28, 2020, 4:38 p.m. UTC
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

Comments

Chris Torek July 28, 2020, 5:52 p.m. UTC | #1
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
Junio C Hamano July 28, 2020, 9:01 p.m. UTC | #2
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.
Jeff King July 29, 2020, 6:22 p.m. UTC | #3
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
Jeff King July 29, 2020, 6:30 p.m. UTC | #4
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
Jeff King July 29, 2020, 6:42 p.m. UTC | #5
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
Junio C Hamano July 29, 2020, 6:53 p.m. UTC | #6
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.
diff mbox series

Patch

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
+$