Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement --batch-size #866

Merged
merged 1 commit into from
Oct 22, 2021
Merged

Conversation

devonhollowood
Copy link
Contributor

Fixes #410

@devonhollowood
Copy link
Contributor Author

Hmm, it looks like the directory traversal order is different on Mac OS, so the output is different (normalize_line doesn't help because we get multiple lines of output). I guess one solution is to add a new normalization function in the test environment (which I can implement tomorrow), but I'm a little curious if you have any better ideas for how to handle this. And how do the existing --exec tests not run into the same issue?

@tmccombs
Copy link
Collaborator

tmccombs commented Oct 21, 2021

it looks like the directory traversal order is different on Mac OS

yes, it definitely is. Even on linux, the order the directory entries are traversed is dependent on the filesystem and isn't guaranteed to be in any particular order.

And how do the existing --exec tests not run into the same issue?

The current tests either output each entry on it's own line, or all of them on a single line. And TestEnv::normalize_output sorts the lines, and if normalize_line is true, it sorts the words on each line. For this test we need to test two things:

  1. Each line contains two paths
  2. The output contains all the paths.

I'm not sure that it makes sense to add a new normalization function for this, unless we add additional tests that use --batch-size.

I think you could write a more robust test something like:

let output = te.assert_success_and_get_output(".", &["foo", "-j1", "--batch-size=2", "--exec-batch", "echo", "{}"]);
let stdout = String::from_utf8_lossy(&output.stdout);
let paths: Vec<_> = line.split_whitespace().collect();
paths.sort_unstable();
assert_eq!(&["a.foo", "one/b.foo", "one/two/C.Foo2", "one/two/c.foo", "one/two/three/d.foo", "one/two/three/directory_foo"], paths);

for line in stdout.lines() {
   assert_eq!(2, line.split_whitespace().count());
}

On another note, could you add an entry to the CHANGELOG?

@devonhollowood
Copy link
Contributor Author

Thanks for the explanation and advice! I've updated the test and the changelog. The tests pass on all platforms now.

@tmccombs tmccombs merged commit 17dd2a6 into sharkdp:master Oct 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-X should batch the number of passed files to the maximum supported by the shell
2 participants