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

Hi Sergey,

Sergey Organov wrote:

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

But:

[...]

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