Re: [PATCH v3 10/10] diff-merges: let "-m" imply "-p"

Re: [PATCH v3 10/10] diff-merges: let "-m" imply "-p"

[Joshua Nelson]

Jonathan Nieder wrote:

> What happens if someone wants to build an older version of Rust?

For what it's worth, almost no one builds old versions of rust from source
except for distros, and distros wouldn't ever set download-ci-llvm = true. So
this shouldn't affect anyone in practice now that we've removed `-m` on master.

Joshua

Why doesn't `git log -m` imply `-p`?

[Alex Henrie]

I read the following in `man git-log` today:

--diff-merges=separate, --diff-merges=m, -m
    This makes merge commits show the full diff with respect to each of
    the parents. Separate log entry and diff is generated for each
    parent.  -m doesn't produce any output without -p.

--diff-merges=combined, --diff-merges=c, -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.  -c implies -p.

--diff-merges=dense-combined, --diff-merges=cc, --cc
    With this option the output produced by --diff-merges=combined is
    further compressed by omitting uninteresting hunks whose contents
    in the parents have only two variants and the merge result picks one
    of them without modification.  --cc implies -p.

Why do -c and -cc imply -p, but -m does not? I tried to use both `git
log -c` and `git log -m` today and was confused when the latter didn't
produce any output. Could we change this behavior in a future version
of Git?

-Alex

[PATCH v3 00/10] diff-merges: let -m imply -p

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p, on
one side, and -m that did not imply -p, on the other side.

Only the last patch is the actual functional change. The rest of
patches are additional tests and refactorings that are not expected to
alter existing behaviors.

After these patches

  git log -m

produces diffs without need to provide -p as well, that improves both
consistency and usability. It gets even more useful if one sets
"log.diffMerges" configuration variable to "first-parent" to force -m
produce usual diff with respect to first parent only.

These series, however, don't change behavior when specific diff format
is explicitly provided on the command-line, so that commands like

  git log -m --raw
  git log -m --stat

are not affected, nor do they change commands where specific diff
format is active by default, such as:

  git diff-tree -m

It's also worth to be noticed that exact historical semantics of -m is
still provided by --diff-merges=separate.

Updates in v3:

  * Add test for "diff-tree -m" and mention it in descriptions

  * Fix one more typo in descriptions

Updates in v2:

  * Fix style and typos in descriptions

Updates in v1:

  * Stop parsing distinct diff-index options beforehand, as it could
    cause unexpected behaviors. Implement different strategy to avoid
    clash of diff-index "-m" and diff-merges "-m".

  * Added tests for "git log -m --raw" and "git log -m".

Sergey Organov (10):
  t4013: test that "-m" alone has no effect in "git log"
  t4013: test "git log -m --raw"
  t4013: test "git log -m --stat"
  t4013: test "git diff-tree -m"
  t4013: test "git diff-index -m"
  diff-merges: move specific diff-index "-m" handling to diff-index
  git-svn: stop passing "-m" to "git rev-list"
  stash list: stop passing "-m" to "git log"
  diff-merges: rename "combined_imply_patch" to "merges_imply_patch"
  diff-merges: let "-m" imply "-p"

 Documentation/diff-options.txt    |  8 ++--
 builtin/diff-index.c              |  9 +++++
 builtin/stash.c                   |  2 +-
 diff-merges.c                     | 36 +++++++++--------
 diff-merges.h                     |  2 +
 perl/Git/SVN.pm                   |  2 +-
 revision.h                        |  2 +-
 t/t3903-stash.sh                  |  2 +-
 t/t4013-diff-various.sh           | 24 +++++++++++
 t/t4013/diff.diff-tree_-m_master  | 11 ++++++
 t/t4013/diff.log_-m_--raw_master  | 61 ++++++++++++++++++++++++++++
 t/t4013/diff.log_-m_--stat_master | 66 +++++++++++++++++++++++++++++++
 12 files changed, 200 insertions(+), 25 deletions(-)
 create mode 100644 t/t4013/diff.diff-tree_-m_master
 create mode 100644 t/t4013/diff.log_-m_--raw_master
 create mode 100644 t/t4013/diff.log_-m_--stat_master

Interdiff against v2:
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 15ca3c75bff7..7fadc985cccd 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -293,6 +293,7 @@ diff-tree --stat initial mode
 diff-tree --summary initial mode
 
 diff-tree master
+diff-tree -m master
 diff-tree -p master
 diff-tree -p -m master
 diff-tree -c master
diff --git a/t/t4013/diff.diff-tree_-m_master b/t/t4013/diff.diff-tree_-m_master
new file mode 100644
index 000000000000..6d0a2207fb30
--- /dev/null
+++ b/t/t4013/diff.diff-tree_-m_master
@@ -0,0 +1,11 @@
+$ git diff-tree -m master
+59d314ad6f356dd08601a4cd5e530381da3e3c64
+:040000 040000 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 0564e026437809817a64fff393079714b6dd4628 M	dir
+:100644 100644 b414108e81e5091fe0974a1858b4d0d22b107f70 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+59d314ad6f356dd08601a4cd5e530381da3e3c64
+:040000 040000 f977ed46ae6873c1c30ab878e15a4accedc3618b 0564e026437809817a64fff393079714b6dd4628 M	dir
+:100644 100644 f4615da674c09df322d6ba8d6b21ecfb1b1ba510 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M	file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A	file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D	file2
+:100644 000000 7289e35bff32727c08dda207511bec138fdb9ea5 0000000000000000000000000000000000000000 D	file3
+$
-- 
2.25.1

[PATCH v3 10/10] diff-merges: let "-m" imply "-p"

[Sergey Organov]

Fix long standing inconsistency between -c/--cc that do imply -p on
one side, and -m that did not imply -p on the other side.

Change corresponding test accordingly, as "log -m" output should now
match one from "log -m -p", rather than from just "log".

Change documentation accordingly.

NOTES:

After this patch

  git log -m

produces diffs without need to provide -p as well, that improves both
consistency and usability. It gets even more useful if one sets
"log.diffMerges" configuration variable to "first-parent" to force -m
produce usual diff with respect to first parent only.

This patch, however, does not change behavior when specific diff
format is explicitly provided on the command-line, so that commands
like

  git log -m --raw
  git log -m --stat

are not affected, nor does it change commands where specific diff
format is active by default, such as:

  git diff-tree -m

It's also worth to be noticed that exact historical semantics of -m is
still provided by --diff-merges=separate.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/diff-options.txt | 8 ++++----
 diff-merges.c                  | 1 +
 t/t4013-diff-various.sh        | 4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 530d1159141f..32e6dee5ac3b 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -49,10 +49,9 @@ ifdef::git-log[]
 --diff-merges=m:::
 -m:::
 	This option makes diff output for merge commits to be shown in
-	the default format. `-m` will produce the output only if `-p`
-	is given as well. The default format could be changed using
+	the default format. The default format could be changed using
 	`log.diffMerges` configuration parameter, which default value
-	is `separate`.
+	is `separate`. `-m` implies `-p`.
 +
 --diff-merges=first-parent:::
 --diff-merges=1:::
@@ -62,7 +61,8 @@ ifdef::git-log[]
 --diff-merges=separate:::
 	This makes merge commits show the full diff with respect to
 	each of the parents. Separate log entry and diff is generated
-	for each parent.
+	for each parent. This is the format that `-m` produced
+	historically.
 +
 --diff-merges=combined:::
 --diff-merges=c:::
diff --git a/diff-merges.c b/diff-merges.c
index d897fd8a2933..0dfcaa1b11b0 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -107,6 +107,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!strcmp(arg, "-m")) {
 		set_to_default(revs);
+		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index e561a8e48521..7fadc985cccd 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -455,8 +455,8 @@ diff-tree --stat --compact-summary initial mode
 diff-tree -R --stat --compact-summary initial mode
 EOF
 
-test_expect_success 'log -m matches pure log' '
-	git log master >result &&
+test_expect_success 'log -m matches log -m -p' '
+	git log -m -p master >result &&
 	process_diffs result >expected &&
 	git log -m >result &&
 	process_diffs result >actual &&
-- 
2.25.1

Re: [PATCH v3 10/10] diff-merges: let "-m" imply "-p"

[Jonathan Nieder]

Hi Sergey,

Sergey Organov wrote:

> Fix long standing inconsistency between -c/--cc that do imply -p on
> one side, and -m that did not imply -p on the other side.

As I mentioned before, I quite like this change for interactive use.

But:

[...]
> It's also worth to be noticed that exact historical semantics of -m is
> still provided by --diff-merges=separate.

Is that true?  When I try it locally, -m shows no diff by default,
whereas --diff-merges=separate shows a diff for merges.

Anyway, the reason I write is that this ended up tripping up some
scripts, namely Rust's bootstrap.py
(https://github.com/rust-lang/rust/commit/df004df3a79b70b4af2b8c267457a5be76bb0d85):

	git log --author=bors --format=%H -n1 \
		-m --first-parent \
		-- src/llvm-project

It's not clear what that *meant* the -m to do --- perhaps they intended
it as an abbreviation for --merges.  That was fixed in Rust by
https://github.com/rust-lang/rust/pull/87513; in the current code at
https://github.com/rust-lang/rust/blob/master/src/bootstrap/bootstrap.py
Rust now uses

	git log --author=bors --format=%H -n1 \
		--no-patch --first-parent \
		-- [etc]

There's also an open pull request at
https://github.com/rust-lang/rust/pull/87532 to simplify it further, to

	git rev-list --author=bors@rust-lang.org -n1 \
		--merges --first-parent HEAD \
		-- [etc]

In any event, the code using -m was pretty clearly a typo, and people
trying to build current Rust won't be affected since it's fixed
already, so this might not be too worrisome.

What happens if someone wants to build an older version of Rust?
bootstrap.py is "symlinked" from x.py at the toplevel of a Rust
distribution; the README explains

	## Installing from Source

	The Rust build system uses a Python script called `x.py` to build the compiler,
	which manages the bootstrapping process. It lives in the root of the project.

	The `x.py` command can be run directly on most systems in the following format:

	```sh
	./x.py <subcommand> [flags]
	```

so this tool is fundamental to everything.  The relevant code using 'git
log -m' is used in the

	if self.downloading_llvm() and stage0:

case to find out how recently llvm changed, in order to check that we
have downloaded that recent of a version of llvm.  It has a
not-too-complicated workaround: if you build LLVM from source using
the src/llvm-project submodule then this logic does not get triggered.

In other words, I don't think this issue will be _too_ problematic for
people working with the Rust project.  Hudson or Taylor (cc-ed) may be
able to correct me if that's wrong.

Still, it does feel a bit like we've pulled the rug from underneath
script authors.  "git log --format=%H" is generally a pretty stable
tool, and here we've changed it when passing -m from not printing
diffs to printing diffs.  What do you think we should do?

Some possibilities:

 a. Revert 'diff-merges: let "-m" imply "-p"'.  This buys us time to
    make a more targeted change, make the change more gradually in a
    future release, or just stop encouraging use of "-m" in docs.

 b. Make "-m" imply "-p", except in some more 'script-ish'
    circumstances (e.g. when using log --format with a format string)

 c. Go ahead with the change and advertise it in release notes.

Searching for other examples using
https://codesearch.debian.net/search?q=%5Cbgit%5Cb.*%5Cblog%5Cb.*-m%5Cb&literal=0,
I find that almost all uses of "git log -m" also include "-p", so (c)
is kind of tempting.  What do you think?

Thanks,
Jonathan

Re: [PATCH v3 10/10] diff-merges: let "-m" imply "-p"

[Sergey Organov]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi Sergey,
>
> Sergey Organov wrote:
>
>> Fix long standing inconsistency between -c/--cc that do imply -p on
>> one side, and -m that did not imply -p on the other side.
>
> As I mentioned before, I quite like this change for interactive use.
>
> But:
>
> [...]
>> It's also worth to be noticed that exact historical semantics of -m is
>> still provided by --diff-merges=separate.
>
> Is that true?  When I try it locally, -m shows no diff by default,
> whereas --diff-merges=separate shows a diff for merges.

You are right, it's not true that --diff-merges=separate behaves exactly
like -m did before this commit.

Actually, I think this notice was meant to be referring to the

"It gets even more useful if one sets "log.diffMerges" configuration
 variable to "first-parent" to force -m produce usual diff with respect
 to first parent only."

part of the message when I wrote it, but it ends up being confusing and
thus wrong, sorry.

Thanks,
-- Sergey Organov