Add support for subtree-conditional prefix filter #961

RalfJung · 2022-10-02T09:12:34Z

This helps with the situation where a subfolder was merged into a monorepo via git subtree, but now one wants to use josh instead but re-use the existing history in the monorepo. git subtree works different than josh in the sense that the monorepo contains the original unaltered commits from the subrepo, and then links them up with the remaining monorepo history using merge commits. To get josh to extract the desired history from that, we need a way to tell josh to treat the subrepo commits as being in the subrepo -- that is basically the prefix filter, moving the commit contents into the appropriate folder, but only for those commits that come from the subrepo.

This PR achieves that with a new :subtree_prefix=commit,path filter. Here commit should be the last subrepo commit that was merged into the monorepo using git subtree; that commit and all its ancestors will then be treated with the :prefix=path filter while all other commits will be passed through unaltered.

To implement that I had to propagate commit information down to the filter implementation; I hope the way I did that makes sense. I am slightly worried that there might be caching somewhere that says "oh I already processed this tree with that filter, let me re-use the old result" -- which might be invalid now if different commits share the same tree, because the new filter is commit-dependent. I also don't quite know what to do in unapply_filter, but making this filter a NOP there made the --reverse part of the test pass. As consequence of that, the commit argument to the unapply functions is actually effectively unused.

I have verified that I can extract the Miri history from the rustc repo with this, using

git fetch http://localhost:8000/rust-lang/rust.git:subtree_prefix=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573,src/tools/miri:/src/tools/miri.git master

and (if I also include #958) this will extract a history that perfectly reproduces Miri until commit 75dd959a3a40eb5b4574f8d2e23aa6efbeb33573. I still have to test pushing from the Miri repo to the josh remote to re-integrate changes in rustc.

Also see the discussion in #952.

RalfJung · 2022-10-02T09:59:52Z

src/filter/opt.rs

+            path: _,
+        } => {
+            // We assume that new commits being added don't need the prefix fixup any more.
+            // FIXME(RalfJung): does that make sense?


The alternative would be to do the ancestor test as well here. Not sure if that makes sense though, it would only fire if the commits line up perfectly -- I don't quite know what even the commit ID that gets passed down here. (Or rather, currently it doesn't get passed down yet, the callers would have to propagate that.)

christian-schilling · 2022-10-02T10:07:58Z

Wow that was quick. Thanks for taking a stab at this!
I did some very initial (not yet working) prototyping for this as well and I think we don't need the is_ancester_of function:
When implementing the filter in apply_to_commit instead of apply(tree) all that really matters is the one commit that matches the given sha. For this one the prefix filter has to be used (wich will automatically transform it's parents) and all for all other commits it's Nop.

I have an open pr #948 that also introduces a first filter that has no implementation of apply(tree) but only apply_to_commit. That being said I'm also considering if your idea of passing the commit to apply(tree) might be a good thing after all...not sure yet.

Regarding the exposed API of this feature: I also have (very different) use case for this "filter that applies only to parts of the history" feature, but not with the :prefix filter. So I think the better approach here is to make a more generic filter like :start=sha[:filter] (in your case that would mean :start=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]).
That way we gain more flexibility in using that new feature.

I already have made the extended grammar for this. You can see it here: 99a5743

RalfJung · 2022-10-02T10:19:34Z

Regarding the exposed API of this feature: I also have (very different) use case for this "filter that applies only to parts of the history" feature, but not with the :prefix filter. So I think the better approach here is to make a more generic filter like :start=sha[:filter] (in your case that would mean :start=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]).
That way we gain more flexibility in using that new feature.

Yeah that definitely makes a lot of sense. I didn't want to get into higher-order filters or changing the filter grammar though. ;)

I'm totally fine with basically all of this code being thrown away. What I care about is that josh can express these kind of history transformations, not the implementation details.

I did some very initial (not yet working) prototyping for this as well and I think we don't need the is_ancester_of function:
When implementing the filter in apply_to_commit instead of apply(tree) all that really matters is the one commit that matches the given sha. For this one the prefix filter has to be used (wich will automatically transform it's parents) and all for all other commits it's Nop.

I don't understand how this can work, but I also don't understand josh very well. We want that commit and all of its parents to be extracted, after all, so how can it be enough to transform one commit? Or is the point that apply_to_commit is anyway recursively descending the commit chain and that's how the ancestor information is implicitly propagated? That sounds much more elegant indeed.

I can try your prototype on our history and tell you if the results are anything like what we need. ;) Let me know once it is pushed somewhere.

RalfJung · 2022-10-02T10:24:36Z

Also something odd seems to be happening when pushing to such a filter. It worked fine for pushing a tiny change, but now I am pushing a larger and more messy history and it is extremely slow. The only thing I see in the log is every so often a new

INFO http_request:call_service:hyper_cgi:http_request:call_service:hyper_cgi:http_request:call_service:hyper_cgi:http_request:call_service:repo_update_fn:repo update worker:unapply_filter josh::history: Didn't find original

Is that a message that appears for each pushed commit? That would put it to around 10-30s for each commit. Which is surprising giving that fetching and transforming the entire large rustc history on a pull only takes a few minutes.

RalfJung · 2022-10-02T11:39:17Z

Ah, that was a push affected by #950 -- pushing things without the bad josh sync that happened into the Miri repo works fine.

christian-schilling · 2022-10-03T13:16:47Z

Here is my implementation as "higher order filter" it passes the test case from this PR:
#965
Note that to look at the changes you should switch to single commit diff (as opposed to PR diff).

…o filter implementation

christian-schilling · 2022-10-06T14:15:06Z

Superseded by #965

initial josh subtree sync This demonstrates what a josh-based rustup would look like with my patched josh. To create it I did ``` git fetch http://localhost:8000/rust-lang/rust.git:start=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri.git master git merge FETCH_HEAD ./rustup-toolchain HEAD && ./miri fmt git commit -am rustup ``` Unlike the [previous attempt](#2554), this does not add a new root commit to the repo. Once we merge this, we committed to using josh for subtree syncing, and in particular a version of josh that includes josh-project/josh#961 (or something compatible).

initial josh subtree sync This demonstrates what a josh-based rustup would look like with my patched josh. To create it I did ``` git fetch http://localhost:8000/rust-lang/rust.git:start=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri.git master git merge FETCH_HEAD ./rustup-toolchain HEAD && ./miri fmt git commit -am rustup ``` Unlike the [previous attempt](rust-lang/miri#2554), this does not add a new root commit to the repo. Once we merge this, we committed to using josh for subtree syncing, and in particular a version of josh that includes josh-project/josh#961 (or something compatible).

同步社区最新master分支代码 Created-by: lizheng 30020856 Author-id: 11958 MR-id: 457385 Commit-by: bors;Michael Krasnitski;Mara Bos;KaDiWa;Alex Macleod;Manish Goregaokar;Tyler Weaver;Niki4tap;koka;Sylvain Desodt;ksaleem;Philipp Krones;Collin Styles;xFrednet;Martin Fischer;Evan Typanski;Vincenzo Palazzo;Samuel Moelius;chansuke;Michael Woerister;Kyle Matsuda;Maria José Solano;Michael Goulet;Yuki Okushi;Jason Newcomb;Joshua Nelson;Oli Scherer;Robert Bastian;navh;asquared31415;Andre Bogus;Albert Larsan;dswij;Raiki Tamura;Caio;blyxyas;Robin Schroer;Kyle Huey;Eric Wu;Nilstrieb;Andy Russell;Max Baumann;Ardis Lu;Trevor Gross;Ray Redondo;Matthias Krüger;Esteban Küber;Lukas Lueg;alexey semenyuk;dboso;Samuel Tardieu;feniljain;Gary Guo;Nicholas Nethercote;Fridtjof Stoldt;naosense;alex-semenyuk;Taiki Endo;Hannah Town;Jakob Degen;Lukas Wirth;Vadim Petrochenkov;mdgaziur;Eric Huss;Yuri Astrakhan;kraktus;Ralf Jung;Santiago Pastorino;Camille GILLOT;hkalbasi;Arpad Borsos;Maybe Waffle;ozkanonur;Sosthène Guédon;Deadbeef;Kartavya Vashishtha;Aphek;Nadir Fejzic;Lukas Markeffsky;hrxi;clubby789;yukang;Ryan Scheidter;Grachev Mikhail;Elliot Bobrow;Dylan DPC;Steven Casper;bebecue;Trevor Arjeski;Onur Özkan;Cameron Steffen;Guillaume Gomez;Matthew Ingwersen;Alex ✨ Cosmic Princess ✨;dswijj;mejrs;Rageking8;Alex;est31;oxalica;JT;Doru-Florin Blanzeanu;Andreu Botella;royrustdev;flip1995;lcnr;Kevin Per;Josh Stone;TennyZhuang;Marijn Schouten;Steven Nguyen;Cody;Urgau;ouz-a;Nahua Kang;Felix Kohlgrüber Merged-by: wangqilin 00349210 E2E-issues: Description: add syntax-tree-patterns RFC, Remove if_chain from equatable_if_let, Lint suggests matches macro if PartialEq trait is not implemented, Run cargo dev bless to update fixes & stderr, Merge commit 'ac0e10aa68325235069a842f47499852b2dee79e' into clippyup, Remove `mir::CastKind::Misc`, Merge commit '8f1ebdd18bdecc621f16baaf779898cc08cc2766' into clippyup, Introduce TypeErrCtxt TypeErrCtxt optionally has a TypeckResults so that InferCtxt doesn't need to., Change InferCtxtBuilder from enter to build, make const_err a hard error, Auto merge of #102091 - RalfJung:const_err, r=oli-obk make const_err a hard error This lint has been deny-by-default with future incompat wording since [Rust 1.51](rust-lang/rust#80394) and the stable release of this week starts showing it in cargo's future compat reports. I can't wait to finally get rid of at least some of the mess in our const-err-reporting-code. ;) r? `@oli-obk` Fixes rust-lang/rust#71800 Fixes rust-lang/rust#100114, Auto merge of rust-lang#2583 - RalfJung:rustup, r=oli-obk initial josh subtree sync This demonstrates what a josh-based rustup would look like with my patched josh. To create it I did ``` git fetch http://localhost:8000/rust-lang/rust.git:start=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri.git master git merge FETCH_HEAD ./rustup-toolchain HEAD && ./miri fmt git commit -am rustup ``` Unlike the [previous attempt](rust-lang/miri#2554), this does not add a new root commit to the repo. Once we merge this, we committed to using josh for subtree syncing, and in particular a version of josh that includes josh-project/josh#961 (or something compatible)., Stabilize half_open_range_patterns, Rollup merge of #102675 - ouz-a:mir-technical-debt, r=oli-obk Remove `mir::CastKind::Misc` As discussed in #97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast. r? ````@oli-obk````, [`unnecessary_cast`] Do not lint negative hexadecimal literals when cast as float Floats cannot be expressed as hexadecimal literals, ImplItemKind::TyAlias => ImplItemKind::Type, merge rustc history, Fix clippy tests that trigger `for_loop_over_fallibles` lint, fixup lint name, deprecate `clippy::for_loops_over_fallibles`, Rollup merge of #102829 - compiler-errors:rename-impl-item-kind, r=TaKO8Ki rename `ImplItemKind::TyAlias` to `ImplItemKind::Type` The naming of this variant seems inconsistent given that this is not really a "type alias", and the associated type variant for `TraitItemKind` is just called `Type`., Rollup merge of #102275 - Urgau:stabilize-half_open_range_patterns, r=cjgillot Stabilize `half_open_range_patterns` This PR stabilize `feature(half_open_range_patterns)`: ``` Allows using `..=X` as a pattern. ``` And adds a new `feature(half_open_range_patterns_in_slices)` for the slice part, rust-lang/rust#102275 (comment). The FCP was completed in rust-lang/rust#67264., Rename AssocItemKind::TyAlias to AssocItemKind::Type, Rollup merge of #99696 - WaffleLapkin:uplift, r=fee1-dead Uplift `clippy::for_loops_over_fallibles` lint into rustc This PR, as the title suggests, uplifts [`clippy::for_loops_over_fallibles`] lint into rustc. This lint warns for code like this: ```rust for _ in Some(1) {} for _ in Ok::<_, ()>(1) {} ``` i.e. directly iterating over `Option` and `Result` using `for` loop. There are a number of suggestions that this PR adds (on top of what clippy suggested): 1. If the argument (? is there a better name for that expression) of a `for` loop is a `.next()` call, then we can suggest removing it (or rather replacing with `.by_ref()` to allow iterator being used later) ```rust for _ in iter.next() {} // turns into for _ in iter.by_ref() {} ``` 2. (otherwise) We can suggest using `while let`, this is useful for non-iterator, iterator-like things like [async] channels ```rust for _ in rx.recv() {} // turns into while let Some(_) = rx.recv() {} ``` 3. If the argument type is `Result<impl IntoIterator, _>` and the body has a `Result<_, _>` type, we can suggest using `?` ```rust for _ in f() {} // turns into for _ in f()? {} ``` 4. To preserve the original behavior and clear intent, we can suggest using `if let` ```rust for _ in f() {} // turns into if let Some(_) = f() {} ``` (P.S. `Some` and `Ok` are interchangeable depending on the type) I still feel that the lint wording/look is somewhat off, so I'll be happy to hear suggestions (on how to improve suggestions :D)! Resolves #99272 [`clippy::for_loops_over_fallibles`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles, Rollup merge of #102868 - compiler-errors:rename-assoc-tyalias-to-ty, r=TaKO8Ki Rename `AssocItemKind::TyAlias` to `AssocItemKind::Type` Thanks `@camsteffen` for catching this in ast too, cc rust-lang/rust#102829 (comment), merge rustc history, Fix unclosed HTML tag in clippy doc, fix `box-default` ignoring trait objects' types, Fix allow_attributes_without_reason applying to external crate macros Previously the `clippy::allow_attributes_without_reason` lint would apply to external crate macros. Many macros in the Rust ecosystem include these `allow` attributes without adding a reason, making this lint pretty much unusable in any sizable Rust project. This commit fixes that by adding a check to the lint if the attribute is from an external crate macro and returning early., Fix bug in `referent_used_exactly_once`, merge rustc history, `default_numeric_fallback` do not lint on constants, refactor `default_numeric_fallback` We only need to store if the literal binding has an explicit type bound or not, Book: Small grammar + link a11y change, Remove CastCheckResult since it's unused, add missing comma, Auto merge of rust-lang#9644 - hkBst:patch-1, r=flip1995 add missing comma changelog: none, Auto merge of rust-lang#9643 - icecream17:patch-1, r=flip1995 Book: Small grammar + link a11y change *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: none --- Very minor For the link accessibility change, `here` and related don't provide context for screen readers who are reading a list of links. (Random supporting google links) https://www.w3.org/QA/Tips/noClickHere https://usability.yale.edu/web-accessibility/articles/links, Don't lint `ptr_arg` when used as an incompatible trait object, Auto merge of rust-lang#9645 - Jarcho:ptr_arg_9542, r=llogiq Don't lint `ptr_arg` when used as an incompatible trait object fixes rust-lang#9542 changelog: [`ptr_arg`](https://rust-lang.github.io/rust-clippy/master/#ptr_arg): Don't lint when used as an incompatible trait object, Add a suggestion and a note about orphan rules for `from_over_into`, Auto merge of rust-lang#9649 - Alexendoo:from-over-into-suggestion, r=llogiq Add a suggestion and a note about orphan rules for `from_over_into` Adds a machine applicable suggestion to convert the `Into` impl into a `From` one to `from_over_into` Also adds a note explaining that `impl From<Local> for Foreign` is fine if the `Into` type is foreign Closes rust-lang#7444 Addresses half of rust-lang#9638 changelog: [`from_over_into`] Add a suggestion and a note about orphan rules, Separate internal lints by pass, Move some things around, Expand `unnecessary_def_path` lint, Fix adjacent code, Format affected files, `explicit_ty_bound` code golf, [`zero_prefixed_literal`] Do not advise to use octal form if not possible, Enable test no_std_main_recursion, fix `box-default` linting `no_std` non-boxes, Auto merge of rust-lang#9655 - llogiq:unbox-default, r=dswij fix `box-default` linting `no_std` non-boxes This fixes rust-lang#9653 by doing the check against the `Box` type correctly even if `Box` isn't there, as in `no_std` code. Thanks to `@lukas-code` for opening the issue and supplying a reproducer! --- changelog: none, Auto merge of rust-lang#9636 - kraktus:numeric-fallback, r=dswij [`default_numeric_fallback`] do not lint on constants fix rust-lang#9632 changelog:[`default_numeric_fallback`] do not lint on constants, Auto merge of rust-lang#9566 - smoelius:diagnostic-item-path, r=dswij Expand internal lint `unnecessary_def_path` This PR does essentially two things: * Separates the internal lints into modules by pass. (`internal_lints.rs` was over 1400 lines, which is a little unruly IMHO.) * ~Adds a new~ Expands the `unnecessary_def_path` internal lint to flag hardcoded paths to diagnostic and language items. My understanding is that the latter is currently done by reviewers. Automating this process should make things easier for both reviewers and contributors. I could make the first bullet a separate PR, or remove it entirely, if desired. changelog: Add internal lint `diagnostic_item_path`, Add new lint `partial_pub_fields` Signed-off-by: TennyZhuang <[email protected]>, fix dogfood Signed-off-by: TennyZhuang <[email protected]>, add many tests Signed-off-by: TennyZhuang <[email protected]>, fix a doctest Signed-off-by: TennyZhuang <[email protected]>, Auto merge of rust-lang#9658 - TennyZhuang:partial-pub-fields, r=llogiq Add new lint `partial_pub_fields` Signed-off-by: TennyZhuang <[email protected]> *Please write a short comment explaining your change (or "none" for internal only changes)* changelog: `partial_pub_fields`: new lint to disallow partial fields of a struct be pub Resolve rust-lang#9604, Auto merge of rust-lang#9652 - kraktus:octo_89, r=xFrednet [`zero_prefixed_literal`] Do not advise to use octal form if not possible fix rust-lang#9651 changelog: [`zero_prefixed_literal`] Do not advise to use octal form if not possible, Auto merge of rust-lang#9609 - kraktus:hexa_f32, r=giraffate [`unnecessary_cast`] Do not lint negative he See merge request innersource/rust/toolset/rust-clippy!8

initial josh subtree sync This demonstrates what a josh-based rustup would look like with my patched josh. To create it I did ``` git fetch http://localhost:8000/rust-lang/rust.git:start=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri.git master git merge FETCH_HEAD ./rustup-toolchain HEAD && ./miri fmt git commit -am rustup ``` Unlike the [previous attempt](rust-lang/miri#2554), this does not add a new root commit to the repo. Once we merge this, we committed to using josh for subtree syncing, and in particular a version of josh that includes josh-project/josh#961 (or something compatible).

RalfJung commented Oct 2, 2022

View reviewed changes

RalfJung force-pushed the subtree_prefix branch from c15be82 to 6d18614 Compare October 2, 2022 10:31

RalfJung mentioned this pull request Oct 2, 2022

Josh generates lots of redundant merge commits #952

Closed

RalfJung mentioned this pull request Oct 3, 2022

Implement the :rev(...) filter #965

Merged

RalfJung added 3 commits October 4, 2022 14:59

add subtree_prefix stub filter, and forward commit information down t…

7e33efa

…o filter implementation

implement and test subtree_prefix filter

33674d7

also implement inverse for subtree_prefix

eb88617

RalfJung force-pushed the subtree_prefix branch from 6d18614 to eb88617 Compare October 4, 2022 12:59

RalfJung mentioned this pull request Oct 4, 2022

initial josh subtree sync rust-lang/miri#2583

Merged

christian-schilling closed this Oct 6, 2022

RalfJung mentioned this pull request Apr 27, 2024

Pushing to josh produces non-roundtrip commit #1325

Closed

RalfJung mentioned this pull request Apr 27, 2024

make rev() detect all parents of the given commits #1326

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for subtree-conditional prefix filter #961

Add support for subtree-conditional prefix filter #961

RalfJung commented Oct 2, 2022 •

edited

Loading

RalfJung Oct 2, 2022

christian-schilling commented Oct 2, 2022 •

edited

Loading

RalfJung commented Oct 2, 2022

RalfJung commented Oct 2, 2022 •

edited

Loading

RalfJung commented Oct 2, 2022

christian-schilling commented Oct 3, 2022 •

edited

Loading

christian-schilling commented Oct 6, 2022

Add support for subtree-conditional prefix filter #961

Add support for subtree-conditional prefix filter #961

Conversation

RalfJung commented Oct 2, 2022 • edited Loading

RalfJung Oct 2, 2022

Choose a reason for hiding this comment

christian-schilling commented Oct 2, 2022 • edited Loading

RalfJung commented Oct 2, 2022

RalfJung commented Oct 2, 2022 • edited Loading

RalfJung commented Oct 2, 2022

christian-schilling commented Oct 3, 2022 • edited Loading

christian-schilling commented Oct 6, 2022

RalfJung commented Oct 2, 2022 •

edited

Loading

christian-schilling commented Oct 2, 2022 •

edited

Loading

RalfJung commented Oct 2, 2022 •

edited

Loading

christian-schilling commented Oct 3, 2022 •

edited

Loading