Skip to content

Commit cd96b64

Browse files
authored
Merge pull request #1851 from GitoxideLabs/fix-1850
reproduce and fix 1850
2 parents 32b54b3 + 5f8bff8 commit cd96b64

File tree

11 files changed

+101
-150
lines changed

11 files changed

+101
-150
lines changed

.github/workflows/ci.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ jobs:
391391
- name: features of gix-features
392392
run: |
393393
set +x
394-
for feature in progress fs-walkdir-parallel parallel io-pipe crc32 zlib zlib-rust-backend fast-sha1 rustsha1 cache-efficiency-debug; do
394+
for feature in progress parallel io-pipe crc32 zlib zlib-rust-backend fast-sha1 rustsha1 cache-efficiency-debug; do
395395
(cd gix-features && cargo build --features "$feature" --target "$TARGET")
396396
done
397397
- name: crates with 'wasm' feature

Cargo.lock

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

gix-features/Cargo.toml

-4
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,6 @@ progress-unit-human-numbers = ["prodash?/unit-human"]
2626
## Provide human readable byte units for progress bars.
2727
progress-unit-bytes = ["dep:bytesize", "prodash?/unit-bytes"]
2828

29-
## If set, walkdir iterators will be multi-threaded.
30-
fs-walkdir-parallel = ["dep:jwalk", "dep:gix-utils"]
31-
3229
## Provide utilities suitable for working with the `std::fs::read_dir()`.
3330
fs-read-dir = ["dep:gix-utils"]
3431

@@ -131,7 +128,6 @@ gix-utils = { version = "^0.1.14", path = "../gix-utils", optional = true }
131128
crossbeam-channel = { version = "0.5.0", optional = true }
132129
parking_lot = { version = "0.12.0", default-features = false, optional = true }
133130

134-
jwalk = { version = "0.8.1", optional = true }
135131
walkdir = { version = "2.3.2", optional = true } # used when parallel is off
136132

137133
# hashing and 'fast-sha1' feature

gix-features/src/fs.rs

+21-129
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
//! * [`jwalk::WalkDir`](https://docs.rs/jwalk/0.5.1/jwalk/type.WalkDir.html) if `parallel` feature is enabled
88
//! * [walkdir::WalkDir](https://docs.rs/walkdir/2.3.1/walkdir/struct.WalkDir.html) otherwise
99
10-
#[cfg(any(feature = "walkdir", feature = "fs-walkdir-parallel"))]
10+
#[cfg(feature = "walkdir")]
1111
mod shared {
1212
/// The desired level of parallelism.
1313
pub enum Parallelism {
@@ -21,7 +21,7 @@ mod shared {
2121
}
2222
}
2323

24-
#[cfg(any(feature = "walkdir", feature = "fs-walkdir-parallel", feature = "fs-read-dir"))]
24+
#[cfg(any(feature = "walkdir", feature = "fs-read-dir"))]
2525
mod walkdir_precompose {
2626
use std::borrow::Cow;
2727
use std::ffi::OsStr;
@@ -83,13 +83,13 @@ mod walkdir_precompose {
8383

8484
/// A platform over entries in a directory, which may or may not precompose unicode after retrieving
8585
/// paths from the file system.
86-
#[cfg(any(feature = "walkdir", feature = "fs-walkdir-parallel"))]
86+
#[cfg(feature = "walkdir")]
8787
pub struct WalkDir<T> {
8888
pub(crate) inner: Option<T>,
8989
pub(crate) precompose_unicode: bool,
9090
}
9191

92-
#[cfg(any(feature = "walkdir", feature = "fs-walkdir-parallel"))]
92+
#[cfg(feature = "walkdir")]
9393
pub struct WalkDirIter<T, I, E>
9494
where
9595
T: Iterator<Item = Result<I, E>>,
@@ -99,7 +99,7 @@ mod walkdir_precompose {
9999
pub(crate) precompose_unicode: bool,
100100
}
101101

102-
#[cfg(any(feature = "walkdir", feature = "fs-walkdir-parallel"))]
102+
#[cfg(feature = "walkdir")]
103103
impl<T, I, E> Iterator for WalkDirIter<T, I, E>
104104
where
105105
T: Iterator<Item = Result<I, E>>,
@@ -142,128 +142,7 @@ pub mod read_dir {
142142
}
143143

144144
///
145-
#[cfg(feature = "fs-walkdir-parallel")]
146-
pub mod walkdir {
147-
use std::borrow::Cow;
148-
use std::ffi::OsStr;
149-
use std::fs::FileType;
150-
use std::path::Path;
151-
152-
use jwalk::WalkDir as WalkDirImpl;
153-
pub use jwalk::{DirEntry as DirEntryGeneric, DirEntryIter as DirEntryIterGeneric, Error};
154-
155-
pub use super::shared::Parallelism;
156-
157-
type DirEntryImpl = DirEntryGeneric<((), ())>;
158-
159-
/// A directory entry returned by [DirEntryIter].
160-
pub type DirEntry = super::walkdir_precompose::DirEntry<DirEntryImpl>;
161-
/// A platform to create a [DirEntryIter] from.
162-
pub type WalkDir = super::walkdir_precompose::WalkDir<WalkDirImpl>;
163-
164-
impl super::walkdir_precompose::DirEntryApi for DirEntryImpl {
165-
fn path(&self) -> Cow<'_, Path> {
166-
self.path().into()
167-
}
168-
169-
fn file_name(&self) -> Cow<'_, OsStr> {
170-
self.file_name().into()
171-
}
172-
173-
fn file_type(&self) -> std::io::Result<FileType> {
174-
Ok(self.file_type())
175-
}
176-
}
177-
178-
impl IntoIterator for WalkDir {
179-
type Item = Result<DirEntry, jwalk::Error>;
180-
type IntoIter = DirEntryIter;
181-
182-
fn into_iter(self) -> Self::IntoIter {
183-
DirEntryIter {
184-
inner: self.inner.expect("always set (builder fix)").into_iter(),
185-
precompose_unicode: self.precompose_unicode,
186-
}
187-
}
188-
}
189-
190-
impl WalkDir {
191-
/// Set the minimum component depth of paths of entries.
192-
pub fn min_depth(mut self, min: usize) -> Self {
193-
self.inner = Some(self.inner.take().expect("always set").min_depth(min));
194-
self
195-
}
196-
/// Set the maximum component depth of paths of entries.
197-
pub fn max_depth(mut self, max: usize) -> Self {
198-
self.inner = Some(self.inner.take().expect("always set").max_depth(max));
199-
self
200-
}
201-
/// Follow symbolic links.
202-
pub fn follow_links(mut self, toggle: bool) -> Self {
203-
self.inner = Some(self.inner.take().expect("always set").follow_links(toggle));
204-
self
205-
}
206-
}
207-
208-
impl From<Parallelism> for jwalk::Parallelism {
209-
fn from(v: Parallelism) -> Self {
210-
match v {
211-
Parallelism::Serial => jwalk::Parallelism::Serial,
212-
Parallelism::ThreadPoolPerTraversal { thread_name } => std::thread::available_parallelism()
213-
.map_or_else(
214-
|_| Parallelism::Serial.into(),
215-
|threads| {
216-
let pool = jwalk::rayon::ThreadPoolBuilder::new()
217-
.num_threads(threads.get().min(16))
218-
.stack_size(128 * 1024)
219-
.thread_name(move |idx| format!("{thread_name} {idx}"))
220-
.build()
221-
.expect("we only set options that can't cause a build failure");
222-
jwalk::Parallelism::RayonExistingPool {
223-
pool: pool.into(),
224-
busy_timeout: None,
225-
}
226-
},
227-
),
228-
}
229-
}
230-
}
231-
232-
/// Instantiate a new directory iterator which will not skip hidden files, with the given level of `parallelism`.
233-
///
234-
/// Use `precompose_unicode` to represent the `core.precomposeUnicode` configuration option.
235-
pub fn walkdir_new(root: &Path, parallelism: Parallelism, precompose_unicode: bool) -> WalkDir {
236-
WalkDir {
237-
inner: WalkDirImpl::new(root)
238-
.skip_hidden(false)
239-
.parallelism(parallelism.into())
240-
.into(),
241-
precompose_unicode,
242-
}
243-
}
244-
245-
/// Instantiate a new directory iterator which will not skip hidden files and is sorted
246-
///
247-
/// Use `precompose_unicode` to represent the `core.precomposeUnicode` configuration option.
248-
pub fn walkdir_sorted_new(root: &Path, parallelism: Parallelism, precompose_unicode: bool) -> WalkDir {
249-
WalkDir {
250-
inner: WalkDirImpl::new(root)
251-
.skip_hidden(false)
252-
.sort(true)
253-
.parallelism(parallelism.into())
254-
.into(),
255-
precompose_unicode,
256-
}
257-
}
258-
259-
type DirEntryIterImpl = DirEntryIterGeneric<((), ())>;
260-
261-
/// The Iterator yielding directory items
262-
pub type DirEntryIter = super::walkdir_precompose::WalkDirIter<DirEntryIterImpl, DirEntryImpl, jwalk::Error>;
263-
}
264-
265-
///
266-
#[cfg(all(feature = "walkdir", not(feature = "fs-walkdir-parallel")))]
145+
#[cfg(feature = "walkdir")]
267146
pub mod walkdir {
268147
use std::borrow::Cow;
269148
use std::ffi::OsStr;
@@ -338,8 +217,21 @@ pub mod walkdir {
338217
///
339218
/// Use `precompose_unicode` to represent the `core.precomposeUnicode` configuration option.
340219
pub fn walkdir_sorted_new(root: &Path, _: Parallelism, precompose_unicode: bool) -> WalkDir {
220+
fn ft_to_number(ft: std::fs::FileType) -> usize {
221+
if ft.is_file() {
222+
1
223+
} else {
224+
2
225+
}
226+
}
341227
WalkDir {
342-
inner: WalkDirImpl::new(root).sort_by_file_name().into(),
228+
inner: WalkDirImpl::new(root)
229+
.sort_by(|a, b| {
230+
ft_to_number(a.file_type())
231+
.cmp(&ft_to_number(b.file_type()))
232+
.then_with(|| a.file_name().cmp(b.file_name()))
233+
})
234+
.into(),
343235
precompose_unicode,
344236
}
345237
}
@@ -348,7 +240,7 @@ pub mod walkdir {
348240
pub type DirEntryIter = super::walkdir_precompose::WalkDirIter<walkdir::IntoIter, DirEntryImpl, walkdir::Error>;
349241
}
350242

351-
#[cfg(any(feature = "walkdir", feature = "fs-walkdir-parallel"))]
243+
#[cfg(feature = "walkdir")]
352244
pub use self::walkdir::{walkdir_new, walkdir_sorted_new, WalkDir};
353245

354246
/// Prepare open options which won't follow symlinks when the file is opened.

gix-ref/tests/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -32,3 +32,4 @@ gix-hash = { path = "../../gix-hash" }
3232
gix-validate = { path = "../../gix-validate" }
3333
gix-lock = { path = "../../gix-lock" }
3434
gix-object = { path = "../../gix-object" }
35+
insta = "1.42.1"
Binary file not shown.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
#!/usr/bin/env bash
2+
set -eu -o pipefail
3+
4+
git init -q
5+
6+
cat <<EOF >.git/packed-refs
7+
# pack-refs with: peeled fully-peeled sorted
8+
17dad46c0ce3be4d4b6d45def031437ab2e40666 refs/heads/ig-branch-remote
9+
83a70366fcc1255d35a00102138293bac673b331 refs/heads/ig-inttest
10+
21b57230833a1733f6685e14eabe936a09689a1b refs/heads/ig-pr4021
11+
d773228d0ee0012fcca53fffe581b0fce0b1dc56 refs/heads/ig/aliases
12+
ba37abe04f91fec76a6b9a817d40ee2daec47207 refs/heads/ig/cifail
13+
EOF
14+
15+
mkdir -p .git/refs/heads/ig/pr
16+
echo d22f46f3d7d2504d56c573b5fe54919bd16be48a >.git/refs/heads/ig/push-name
17+
echo 4dec145966c546402c5a9e28b932e7c8c939e01e >.git/refs/heads/ig-pr4021

gix-ref/tests/refs/file/store/iter.rs

+59-8
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,8 @@ mod with_namespace {
3131
.map(|r: gix_ref::Reference| r.name)
3232
.collect::<Vec<_>>();
3333
let expected_namespaced_refs = vec![
34-
"refs/namespaces/bar/refs/heads/multi-link-target1",
3534
"refs/namespaces/bar/refs/multi-link",
35+
"refs/namespaces/bar/refs/heads/multi-link-target1",
3636
"refs/namespaces/bar/refs/remotes/origin/multi-link-target3",
3737
"refs/namespaces/bar/refs/tags/multi-link-target2",
3838
];
@@ -50,8 +50,8 @@ mod with_namespace {
5050
.map(|r| r.name.into_inner())
5151
.collect::<Vec<_>>(),
5252
[
53-
"refs/namespaces/bar/refs/heads/multi-link-target1",
5453
"refs/namespaces/bar/refs/multi-link",
54+
"refs/namespaces/bar/refs/heads/multi-link-target1",
5555
"refs/namespaces/bar/refs/tags/multi-link-target2"
5656
]
5757
);
@@ -149,8 +149,8 @@ mod with_namespace {
149149
let packed = ns_store.open_packed_buffer()?;
150150

151151
let expected_refs = vec![
152-
"refs/heads/multi-link-target1",
153152
"refs/multi-link",
153+
"refs/heads/multi-link-target1",
154154
"refs/remotes/origin/multi-link-target3",
155155
"refs/tags/multi-link-target2",
156156
];
@@ -198,8 +198,8 @@ mod with_namespace {
198198
.map(|r| r.name.into_inner())
199199
.collect::<Vec<_>>(),
200200
[
201-
"refs/heads/multi-link-target1",
202201
"refs/multi-link",
202+
"refs/heads/multi-link-target1",
203203
"refs/tags/multi-link-target2",
204204
],
205205
"loose iterators have namespace support as well"
@@ -214,8 +214,8 @@ mod with_namespace {
214214
.map(|r| r.name.into_inner())
215215
.collect::<Vec<_>>(),
216216
[
217-
"refs/namespaces/bar/refs/heads/multi-link-target1",
218217
"refs/namespaces/bar/refs/multi-link",
218+
"refs/namespaces/bar/refs/heads/multi-link-target1",
219219
"refs/namespaces/bar/refs/tags/multi-link-target2",
220220
"refs/namespaces/foo/refs/remotes/origin/HEAD"
221221
],
@@ -291,14 +291,14 @@ fn loose_iter_with_broken_refs() -> crate::Result {
291291
ref_paths,
292292
vec![
293293
"d1",
294+
"loop-a",
295+
"loop-b",
296+
"multi-link",
294297
"heads/A",
295298
"heads/d1",
296299
"heads/dt1",
297300
"heads/main",
298301
"heads/multi-link-target1",
299-
"loop-a",
300-
"loop-b",
301-
"multi-link",
302302
"remotes/origin/HEAD",
303303
"remotes/origin/main",
304304
"remotes/origin/multi-link-target3",
@@ -413,6 +413,57 @@ fn overlay_iter() -> crate::Result {
413413
Ok(())
414414
}
415415

416+
#[test]
417+
fn overlay_iter_reproduce_1850() -> crate::Result {
418+
let store = store_at("make_repo_for_1850_repro.sh")?;
419+
let ref_names = store
420+
.iter()?
421+
.all()?
422+
.map(|r| r.map(|r| (r.name.as_bstr().to_owned(), r.target)))
423+
.collect::<Result<Vec<_>, _>>()?;
424+
insta::assert_debug_snapshot!(ref_names, @r#"
425+
[
426+
(
427+
"refs/heads/ig-branch-remote",
428+
Object(
429+
Sha1(17dad46c0ce3be4d4b6d45def031437ab2e40666),
430+
),
431+
),
432+
(
433+
"refs/heads/ig-inttest",
434+
Object(
435+
Sha1(83a70366fcc1255d35a00102138293bac673b331),
436+
),
437+
),
438+
(
439+
"refs/heads/ig-pr4021",
440+
Object(
441+
Sha1(4dec145966c546402c5a9e28b932e7c8c939e01e),
442+
),
443+
),
444+
(
445+
"refs/heads/ig/aliases",
446+
Object(
447+
Sha1(d773228d0ee0012fcca53fffe581b0fce0b1dc56),
448+
),
449+
),
450+
(
451+
"refs/heads/ig/cifail",
452+
Object(
453+
Sha1(ba37abe04f91fec76a6b9a817d40ee2daec47207),
454+
),
455+
),
456+
(
457+
"refs/heads/ig/push-name",
458+
Object(
459+
Sha1(d22f46f3d7d2504d56c573b5fe54919bd16be48a),
460+
),
461+
),
462+
]
463+
"#);
464+
Ok(())
465+
}
466+
416467
#[test]
417468
fn overlay_iter_with_prefix_wont_allow_absolute_paths() -> crate::Result {
418469
let store = store_with_packed_refs()?;

0 commit comments

Comments
 (0)