| Message ID | 20210520214703.27323-11-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f5bfcc823ba242a46e20fb6f71c9fbf7ebb222fe |
| Headers | show |
| Series | [v3,01/10] t4013: test that "-m" alone has no effect in "git log" | expand |
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
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
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 &&
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(-)