Skip to content

bootstrap.py: change git log option to indicate desired behavior#87513

Merged
bors merged 1 commit intorust-lang:masterfrom
hudson-ayers:bootstrap-py-fix
Jul 28, 2021
Merged

bootstrap.py: change git log option to indicate desired behavior#87513
bors merged 1 commit intorust-lang:masterfrom
hudson-ayers:bootstrap-py-fix

Conversation

@hudson-ayers
Copy link
Contributor

@hudson-ayers hudson-ayers commented Jul 27, 2021 •
edited
Loading

When determining which LLVM artifacts to download, bootstrap.py calls: git log --author=bors --format=%H -n1 -m --first-parent -- src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version. However, the -m option has no effect, per the git log help:

-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 log.diffMerges
configuration parameter, which default value is separate.

Accordingly, this commit removes use of the -m option in favor of --diff-merges=off --no-patch, since no diff information is needed, and in fact the presence of a diff breaks the command. Tested using git 2.32, this does not change the output of the command.

The motivation for this change is that some patched versions of git change the behavior of the -m flag to imply -p, rather than to do nothing unless -p is passed. These patched versions of git lead to this script not working. Google's corp-provided git is one such example.

@rust-highfive
Copy link
Contributor

rust-highfive commented Jul 27, 2021

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2021
@hudson-ayers
bootstrap.py: remove unused git log option
1259742 
When determining which LLVM artifacts to download, bootstrap.py
calls: `git log --author=bors --format=%H -n1 -m --first-parent --
src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version`.
However, the `-m` option has no effect, per the `git log` help:

> -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 log.diffMerges
> configuration parameter, which default value is separate.

Accordingly, this commit removes use of the -m option in favor of
`--no-patch`, to make clear that this command should never output
diff information, as the SHA-1 hash is the only desired output.
Tested using git 2.32, this does not change the
output of the command.

The motivation for this change is that some patched versions of git
change the behavior of the `-m` flag to imply `-p`, rather than to do
nothing unless `-p` is passed. These patched versions of git lead to
this script not working. Google's corp-provided git is one such example.
@hudson-ayers
Copy link
Contributor Author

hudson-ayers commented Jul 27, 2021

Updated to use --no-patch instead of --diff-merges=off based on https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/.2E.2Fx.2Epy.20build.20failures/near/247342368

@jyn514
Copy link
Member

jyn514 commented Jul 27, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 27, 2021

📌 Commit 1259742 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2021
@jyn514 jyn514 added A-contributor-roadblock Area: Makes things more difficult for new or seasoned contributors to Rust T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 27, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 27, 2021
@GuillaumeGomez
Rollup merge of rust-lang#87513 - hudson-ayers:bootstrap-py-fix, r=jy…
dcd8e8e 
…n514

bootstrap.py: change `git log` option to indicate desired behavior

When determining which LLVM artifacts to download, bootstrap.py calls: `git log --author=bors --format=%H -n1 -m --first-parent --
src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version`. However, the `-m` option has no effect, per the `git log` help:

> -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 log.diffMerges
> configuration parameter, which default value is separate.

Accordingly, this commit removes use of the -m option in favor of ~~`--diff-merges=off`~~ `--no-patch`, since no diff information is needed, and in fact the presence of a diff breaks the command. Tested using git 2.32, this does not change the output of the command.

The motivation for this change is that some patched versions of git change the behavior of the `-m` flag to imply `-p`, rather than to do nothing unless `-p` is passed. These patched versions of git lead to this script not working. Google's corp-provided git is one such example.
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jul 27, 2021
Closed
@tlyu tlyu mentioned this pull request Jul 27, 2021
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 28, 2021
@JohnTitor
Rollup merge of rust-lang#87513 - hudson-ayers:bootstrap-py-fix, r=jy…
75f7a87 
…n514

bootstrap.py: change `git log` option to indicate desired behavior

When determining which LLVM artifacts to download, bootstrap.py calls: `git log --author=bors --format=%H -n1 -m --first-parent --
src/llvm-project src/bootstrap/download-ci-llvm-stamp src/version`. However, the `-m` option has no effect, per the `git log` help:

> -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 log.diffMerges
> configuration parameter, which default value is separate.

Accordingly, this commit removes use of the -m option in favor of ~~`--diff-merges=off`~~ `--no-patch`, since no diff information is needed, and in fact the presence of a diff breaks the command. Tested using git 2.32, this does not change the output of the command.

The motivation for this change is that some patched versions of git change the behavior of the `-m` flag to imply `-p`, rather than to do nothing unless `-p` is passed. These patched versions of git lead to this script not working. Google's corp-provided git is one such example.
This was referenced Jul 28, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2021
@bors
Auto merge of rust-lang#87540 - JohnTitor:rollup-8xc6bl5, r=JohnTitor
a28109a 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#87315 (Add docs for raw-dylib to unstable book)
 - rust-lang#87330 (Use hashbrown's `extend_reserve()` in `HashMap`)
 - rust-lang#87443 (Don't treat git repos as non-existent when `ignore_git` is set)
 - rust-lang#87453 (Suggest removing unnecessary &mut as help message)
 - rust-lang#87500 (Document math behind MIN/MAX consts on integers)
 - rust-lang#87501 (Remove min_type_alias_impl_trait in favor of type_alias_impl_trait)
 - rust-lang#87507 (SGX mutex is *not* moveable)
 - rust-lang#87513 (bootstrap.py: change `git log` option to indicate desired behavior)
 - rust-lang#87523 (Stop creating a reference then immediately dereferencing it.)
 - rust-lang#87524 (Fix ICE in `diagnostic_hir_wf_check`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1682382 into rust-lang:master Jul 28, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 28, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2021
@bors
Auto merge of rust-lang#87532 - tlyu:bootstrap-rev-list, r=jyn514
2ddb65c 
bootstrap.py: use `git rev-list` for robustness

Use `git rev-list` instead of `git log` to be more robust against
UI changes in git. Also, use the full email address for bors,
because `--author` uses a substring match.

Based on rust-lang#87513, but is separate because it's less minimal and may require additional manual testing.

~Open questions:~
* ~Should the `merge_base` search also use `--first-parent`?~
* ~Do we exclude non-merge commits from bors? There are a few, and I'm not sure what they have in common. Some of them look like squashes, and some look like they're in rollup branches.~

r? `@jyn514`
`@rustbot` label +A-rustbuild +C-cleanup
gitster pushed a commit to git/git that referenced this pull request Aug 9, 2021
@jrn @gitster
Revert 'diff-merges: let "-m" imply "-p"'
6a38e33 
This reverts commit f5bfcc8, which
made "git log -m" imply "--patch" by default.  The logic was that
"-m", which makes diff generation for merges perform a diff against
each parent, has no use unless I am viewing the diff, so we could save
the user some typing by turning on display of the resulting diff
automatically.  That wasn't expected to adversely affect scripts
because scripts would either be using a command like "git diff-tree"
that already emits diffs by default or would be combining -m with a
diff generation option such as --name-status.  By saving typing for
interactive use without adversely affecting scripts in the wild, it
would be a pure improvement.

The problem is that although diff generation options are only relevant
for the displayed diff, a script author can imagine them affecting
path limiting.  For example, I might run

	git log -w --format=%H -- README

hoping to list commits that edited README, excluding whitespace-only
changes.  In fact, a whitespace-only change is not TREESAME so the use
of -w here has no effect (since we don't apply these diff generation
flags to the diff_options struct rev_info::pruning used for this
purpose), but the documentation suggests that it should work

	Suppose you specified foo as the <paths>. We shall call
	commits that modify foo !TREESAME, and the rest TREESAME. (In
	a diff filtered for foo, they look different and equal,
	respectively.)

and a script author who has not tested whitespace-only changes
wouldn't notice.

Similarly, a script author could include

	git log -m --first-parent --format=%H -- README

to filter the first-parent history for commits that modified README.
The -m is a no-op but it reflects the script author's intent.  For
example, until 1e20a40 (stash list: stop passing "-m" to "git
log", 2021-05-21), "git stash list" did this.

As a result, we can't safely change "-m" to imply "-p" without fear of
breaking such scripts.  Restore the previous behavior.

Noticed because Rust's src/bootstrap/bootstrap.py made use of this
same construct: rust-lang/rust#87513.  That
script has been updated to omit the unnecessary "-m" option, but we
can expect other scripts in the wild to have similar expectations.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels

A-contributor-roadblock Area: Makes things more difficult for new or seasoned contributors to Rust S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Milestone

1.56.0

Development

Successfully merging this pull request may close these issues.

6 participants

@hudson-ayers @rust-highfive @jyn514 @bors @Mark-Simulacrum @rustbot