bootstrap.py: change git log option to indicate desired behavior
#87513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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
added
the
S-waiting-on-review
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
force-pushed
the
bootstrap-py-fix
branch
from
771a32e to
1259742
Compare
|
Updated to use |
|
@bors r+ |
|
📌 Commit 1259742 has been approved by |
bors
added
S-waiting-on-bors
jyn514
added
A-contributor-roadblock
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jul 27, 2021
…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.
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Jul 28, 2021
…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
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jul 28, 2021
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
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 5, 2021
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
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 <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-moption has no effect, per thegit loghelp: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
-mflag to imply-p, rather than to do nothing unless-pis passed. These patched versions of git lead to this script not working. Google's corp-provided git is one such example.