Replace newline to space with xargs

[saschanaz]

So that the resulting command would be eslint file1 file2 rather than eslint file1\nfile2.

[saschanaz]

(The lint failure here should be fixed by #3212, and the test failure by #3210)

[saschanaz]

Actually, files can have spaces in their names and in that case xargs is not enough.

[aborel]

The GNU xargs option to use only the new line as a file separator should do the trick: xargs -d '\n' instead of just xargs (so that it doesn't split filenames that contain spaces)

[saschanaz]

I'm not sure that's enough? foo bar.js\nbaz.js will still become foo bar.js baz.js which is not a proper input.

[aborel]

You are right, I had misunderstood the problem. Here's an idea: maybe you can use xargs in .ci/lint.sh to run lint on just one file at a time, while the final pipe in `.ci/helper.sh' would protect the spaces of filenames instead of processing the file list. Something like this:

TRANSLATORS_TO_CHECK=$(git diff HEAD^2 $branch_point --name-only | { grep -e "^[^/]*.js$" || true; } | sed 's/ /\\ /g')

then

if [ -n "$TRANSLATORS_TO_CHECK" ]; then
	echo  "$TRANSLATORS_TO_CHECK" | xargs npm run lint --
fi

[AbeJellinek]

xargs -d does do what we want - if you're testing with something like echo, you won't be able to see the difference, since it joins with a space, but you can test with e.g. touch:

echo $'foo bar.js\nbaz.js' | xargs -d $'\n' touch
ls

You'll end up with a file called foo bar.js and a file called baz.js.

Finally, xargs -d isn't actually supported on macOS, so we can use xargs -0 instead:

echo $'foo bar.js\nbaz.js' | tr '\n' '\0' | xargs -0 mkdir

That replaces the newlines with null characters and then tells xargs to split on null characters.

No need to run lint on one file at a time IMO.

[aborel]

Thanks @AbeJellinek , I think we can trust you on this one.

[saschanaz]

Hmm, in that case I don't think we need -d or tr at all, we can just move xargs to lint.sh and that should work.

[saschanaz]

Oh hmm, actually not. Let me try your suggestion...

[saschanaz]

(Still needs #3210 but otherwise sounds like it should work)

[AbeJellinek]

What's the status on this? Is this PR still needed after the other CI fixes?

[AbeJellinek]

(I'm assuming yes but I just want to be sure this is still something we want to merge. Hard to test CI!)

[saschanaz]

What's the status on this? Is this PR still needed after the other CI fixes?

Yes, otherwise changing multiple files would trigger the linter failure.

[AbeJellinek]

Thanks!