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

[Jonathan Nieder <jrnieder@gmail.com>]

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