various fixes#1462
Conversation
Let's not trade safety for some convenience.
This reduces performance by 5% at least (in the WebKit repository at 886077e077a496a6e398df52a4b7915d8cd68f76):
```
❯ hyperfine "/Users/byron/dev/github.com/Byron/gitoxide/target/release/gix index entries ':(attr:export-ignore)' -i" "gix index entries ':(attr:export-ignore)' -i"
Benchmark 1: /Users/byron/dev/github.com/Byron/gitoxide/target/release/gix index entries ':(attr:export-ignore)' -i
Time (mean ± σ): 820.1 ms ± 71.7 ms [User: 759.9 ms, System: 85.9 ms]
Range (min … max): 778.8 ms … 984.3 ms 10 runs
Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
Benchmark 2: gix index entries ':(attr:export-ignore)' -i
Time (mean ± σ): 782.1 ms ± 1.8 ms [User: 760.1 ms, System: 43.2 ms]
Range (min … max): 780.0 ms … 786.0 ms 10 runs
Summary
gix index entries ':(attr:export-ignore)' -i ran
1.05 ± 0.09 times faster than /Users/byron/dev/github.com/Byron/gitoxide/target/release/gix index entries ':(attr:export-ignore)' -i
```
7a98d8a to
1267712
Compare
This is quite typical in the wild where there ignore files are used as include lists, instead of exclude-lists.
If they were, they would more easily be deleted by tooling like `gix clean`.
432617e to
1e92d1e
Compare
Sebastian Thiel (Byron)
left a comment
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
It took a while to write the tests, but once they were available homing on a fix was quite straightforward and felt nice, despite the complexity of the algorithm.
| .map(|platform| platform.excluded_kind()) | ||
| }) | ||
| .map_err(Error::ExcludesAccess)? | ||
| .filter(|_| filename_start_idx > 0) |
There was a problem hiding this comment.
This line and the line below constitute the fix, it's quite neat.
The idea is that previously, the classifier didn't distinguish between the worktree root (and the contained repository) and sub-repositories. For the latter, the current classification is very much intentional, but the root should never ever be counted as ignored, and thus removable.
The commit with the fix also shows how the classification changes due to the fix, which is all it takes to prevent gix clean to go off the rail.
When looking for weaknesses, I also recommend using the pathspec, as it is quite integrated to the algorithm to have some (possibly unexpected) influence.
| assert_eq!( | ||
| entries, | ||
| &[ | ||
| entry_nokind(".git", Pruned).with_property(DotGit).with_match(Always), |
There was a problem hiding this comment.
Before the fix, this entry was ignored which was part of the problem. With the fix, it turned into a pruned DotGit entry, which naturally won't be touched by gix clean.
There was a problem hiding this comment.
I've checked manually on Windows 10 and Ubuntu 22.04 LTS, on both systems installing with cargo install --path ., that this fixes the cases covered in #1458. If I find other cases I'll open a new issue. Thanks!
|
Regarding the failure of the |
gix-attributes was found by @ssbr to be unsound, as reported in GitoxideLabs/gitoxide#1460. This adds an informational notice for that, as discussed in comments there. It looks like the affected code, having been introduced in GitoxideLabs/gitoxide#400, was present in all versions of the crate prior to the fix in 0.22.3 (which was one of the bugs fixed in GitoxideLabs/gitoxide#1462). Co-authored-by: Devin Jeanpierre <[email protected]>
* Unsoundness notice for gix-attributes (kstring integration) gix-attributes was found by @ssbr to be unsound, as reported in GitoxideLabs/gitoxide#1460. This adds an informational notice for that, as discussed in comments there. It looks like the affected code, having been introduced in GitoxideLabs/gitoxide#400, was present in all versions of the crate prior to the fix in 0.22.3 (which was one of the bugs fixed in GitoxideLabs/gitoxide#1462). Co-authored-by: Devin Jeanpierre <[email protected]> * Small adjustments for advisory This makes some minor changes to the advisory description to adapt the text from GitoxideLabs/gitoxide#1460 to be an advisory. For the most part it has remained the same. Changes: * Express the claim of unsoundness with more confidence, since it has been reviewed by the maintainer. * Modify the link to the affected code to point to the latest tag for gix-attributes that has that code. The original link was to a branch, so it was broken when the fix was applied. * Apply inline code formatting in a few more places, where doing so improves stylistic consistency. --------- Co-authored-by: Devin Jeanpierre <[email protected]>
2 participants
Tasks
/to assure it doesn't match all in root (or anywhere for that matter)gix clean -xdedeletes whole repo if.gitignorelists*or/#1458