bootstrap submodule sync overwrites local changes

[dotdot0]

Fixes #103485

[rustbot]

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.

[jyn514]

r? @jyn514

@pratushrai0309 how have you tested this change?

[dotdot0]

@jyn514 Can you explain what's the intended result should be?

[jyn514]

#103485 (comment)

[dotdot0]

@jyn514 After adding the git commands I run x.py build and my local changes to bootstrap/lib.rs were not overwritten

[jyn514]

Can you test with changes to the llvm-project submodule as well?

[dotdot0]

Can you test with changes to the llvm-project submodule as well?

Ok

[dotdot0]

@jyn514 I tried to add few debug messages in few llvm-project submodule files and when I run python ./x.py build my local changes were not overwritten. So is it working now?

[jyn514]

@nagisa @Aaron1011 can you cherry-pick these commits and see if it behaves about how you'd expect when modifying llvm?

[flba-eb]

Please consider using git stash push instead of get stash save. From the manual:

save [-p|--patch] [-S|--staged] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] []
This option is deprecated in favour of git stash push. It differs from "stash push" in that it cannot take pathspec. Instead, all non-option arguments are concatenated to form the stash message.

[flba-eb]

I guess git stash pop is more useful here:

apply [--index] [-q|--quiet] []
Like pop, but do not remove the state from the stash list. Unlike pop, may be any commit that looks like a commit created by stash push or stash create.

I don't want to have a new stash item whenever running x.py. When it as applied successfully, it should be removed.

Please also keep in mind that applying a stash can fail due to merge conflicts!

[nagisa]

I tried this out. Couple examples:

• cd src/llvm-project && git checkout -f HEAD~1 && echo "dontcompile" >> llvm/lib/Demangle/Demangle.cpp

  This is the base scenario and I can see the changes being preserved just fine. Unfortunately it is indeed the case that the the git stash log gets populated with changes I made, despite there being no merge conflict. We probably shouldn't be retaining these.

• cd src/llvm-project && git checkout -f origin/rustc/15.0-2022-08-09 && git reset --soft HEAD~3

  This scenario attempted to simulate a merge conflict. There wasn’t an actual merge conflict though. And what the implementation here went ahead and restored back the dontcompile change from the previous iteration, which is not what was intended! Probably fixed by using push and pop instead, as long as the stash is not used for anything else.

• cd src/llvm-project && git stash clear && git checkout -f origin/rustc/15.0-2022-08-09~4 && git cherry-pick origin/rustc/15.0-2022-08-09~2 -n && vim some-changed-lines

  Another attempt at a merge conflict. x.py will then complain:

  error: Your local changes to the following files would be overwritten by checkout:
      llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp
  Please commit your changes or stash them before you switch branches.
  Aborting
  

  before it even gets to stashing and popping? Seems weird. My intuition was that it should have succeeded at stashing my local changes, succeeded in checking out the appropriate commit and only then failed to apply my changes back. Things may be just fine the way they are though? That is ­– merge conflict handling in save/apply don't matter because they will already be reported by an earlier command.

---

Overall, after these experiments I’m not entirely sure if stash-pop are the right way to go. In particular, it seems to me that we could just drop the git reset/clean and git submodule sync take care of the rest, including all the reporting of any issues that arise, if the intent was to always preserve the local changes.

Unfortunately there are no comments to explain the purpose of reset, but my intuition is that the goal was to make sure that the submodules aren’t accidentally dirty! Unfortunately the commit that introduced the reset (holy moly was this many aeons ago!) did not really explain the motivation behind adding a reset either.

I think what we may want to do is instead adding an option to the configuration file that would enable/disable the git reset. Then:

1. Any uncommitted changes? Stash them away, marking them as being stashed by x.py;
2. If any previously stashed changes by x.py: Print out a big warning message telling users this has happened and what they can do to recover/keep their changes (set the option to disable git reset and git stash pop/git checkout -f their changes back)

Perhaps? It is pretty clear to me that we need to figure out what we really want the behaviour to be, because the current list of wants and needs is somewhat ill-defined.

[jyn514]

I think what we may want to do is instead adding an option to the configuration file that would enable/disable the git reset.

What would be the purpose of the option? What advantage is there to using reset?

Overall, after these experiments I’m not entirely sure if stash-pop are the right way to go. In particular, it seems to me that we could just drop the git reset/clean and git submodule sync take care of the rest, including all the reporting of any issues that arise, if the intent was to always preserve the local changes.

👍 That makes sense to me - do you have time to try out that change? No worries if not, the commands you posted here are very helpful to replicate your testing :)

[nagisa]

What would be the purpose of the option? What advantage is there to using reset?

I’m not actually sure what the reset&clean are here for. But I do believe that whatever they are trying to achieve is inherently incompatible with the expectations people trying to hack within the submodules may have (i.e. no reset or clean). The option then would be there to primarily switch between these two modes/expectations. At the very least I could see somebody using a checkout primarily for building artifacts (as opposed to development) getting a good benefit out of an option that makes sure all the submodules are clean and exactly on the commits they ought to be on (i.e. the current default behaviour.)

I would also say that in practice the local changes only make sense to preserve if they were made recently. If I pull out my experiments checkout from 3 months ago, and attempt to build master, various changes made back then are probably gonna be a pain. Having a shortcut to clean up the checkout would be nice too. Maybe it could be an x.py subcommand? Like for example x.py checkout GITREF would checkout a clean copy of the repository and the submodules, with a flag like -f forcing a reset+clean on top of that?

[jyn514]

@nagisa I want to avoid expanding the scope here too much. I agree with all the things you mentioned but I'm not sure how common they are in practice; I want to be respectful of @pratushrai0309's time and I certainly don't have time to work on them myself.

@pratushrai0309 if you change the commands here to use push and pop instead of save and apply I'm happy to approve the PR :)

[rustbot]

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

A change occurred in the Ayu theme.

cc @Cldfire

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

Some changes occurred in const_evaluatable.rs

cc @lcnr

Some changes occurred in src/tools/cargo

cc @ehuss

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

rustc_errors::translation was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

These commits modify compiler targets.
(See the Target Tier Policy.)

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[dotdot0]

@jyn514 Can you check this PR #104865