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

grep exit status and extra newlines #900

Closed
wyardley opened this issue Nov 7, 2018 · 8 comments · Fixed by #901
Closed

grep exit status and extra newlines #900

wyardley opened this issue Nov 7, 2018 · 8 comments · Fixed by #901

Comments

@wyardley
Copy link
Contributor

@wyardley wyardley commented Nov 7, 2018

Node version (or tell us if you're using electron or some other framework):

v8.11.3

ShellJS version (the most recent version/Github branch you see the bug on):

0.8.2

Operating system:

Mac OS X

Description of the bug:

grep() returns 0, even if a string is not found, and the resulting string also contains a newline.

Example ShellJS command to reproduce the error:

(given a file testfile without the string asdfasdf at the beginning of a line):

const shell = require('shelljs');

console.log(shell.grep(/^asdfasdf/, 'testfile'))

Output of above:

{ [String: '\n']
  stdout: '\n',
  stderr: null,
  code: 0,
  cat: [Function: bound ],
  exec: [Function: bound ],
  grep: [Function: bound ],
  head: [Function: bound ],
  sed: [Function: bound ],
  sort: [Function: bound ],
  tail: [Function: bound ],
  to: [Function: bound ],
  toEnd: [Function: bound ],
  uniq: [Function: bound ] }

I don't believe you'll have even a newline if you grep for something that isn't found, and you'll definitely get a non-0 status code if the string isn't found.

$ grep -q ^asdfasdf testfile 
$ echo $?
1
$ grep ^asdfasdf testfile > /tmp/asdf
$ cat -v -e /tmp/asdf 
$ file /tmp/asdf 
/tmp/asdf: empty

As best I could tell, the only reliable way to detect the absence of a string to use something like:

shell.grep(/^asdfasdf/, 'testfile').stdout.trim())

Of course, at this point, it might be easier to use regexp on the file directly, but this still seems inconsistent with the behavior of the grep command.

@nfischer
Copy link
Member

@nfischer nfischer commented Nov 7, 2018

Yup, this is broken. Can you send a PR?

shelljs/src/grep.js

Lines 69 to 71 in 4bd22e7

});
return grep.join('\n') + '\n';

⬆️ You can check if the grep array is empty and add a call to common.error() before the return statement. You'll need to use silent: true, otherwise it'll print a message to stderr.

Any chance sed() is broken similarly?

@wyardley
Copy link
Contributor Author

@wyardley wyardley commented Nov 7, 2018

@nfischer I think I'm close (see commits), but as it is now, it's creating test failures for the other failure cases in a way I don't totally understand (i.e., even though there's a return after
https://github.com/shelljs/shelljs/blob/master/src/grep.js#L50-L51
the test cases for that seem to be affected by this change)

Also, do we expect stdout / stderr to be nulls or empty strings in this case?

@nfischer
Copy link
Member

@nfischer nfischer commented Nov 8, 2018

@wyardley thanks, I added some comments. Can you create a PR? Then I can check any travis failures.

@wyardley
Copy link
Contributor Author

@wyardley wyardley commented Nov 8, 2018

Yes, can do. I think you were commenting on an earlier commit, so at least a couple of those items were already fixed. I’ll adjust the remaining stuff and throw up a PR

@wyardley
Copy link
Contributor Author

@wyardley wyardley commented Nov 9, 2018

@nfischer: also took a look at sed, and because it doesn't add the extra \n besides the join on newline in https://github.com/shelljs/shelljs/blob/master/src/sed.js#L85, it doesn't seem to be affected. In general, sed I think exits 0 even if nothing was matched and / or if no changes were made (though TIL there are some exceptions, which I assume this module wouldn't support -- see https://stackoverflow.com/questions/15965073/return-code-of-sed-for-no-match ).

I can create a test case for the case of an empty input file, though, if you want.... it would be something like this:

diff --git a/test/resources/sed/empty b/test/resources/sed/empty
new file mode 100644
index 0000000..e69de29
diff --git a/test/sed.js b/test/sed.js
index fc65c54..420cf69 100644
--- a/test/sed.js
+++ b/test/sed.js
@@ -174,3 +174,9 @@ test('glob file names, with in-place-replacement', t => {
   t.is(shell.cat(`${t.context.tmp}/file1.txt`).toString(), 'hello1\n');
   t.is(shell.cat(`${t.context.tmp}/file2.txt`).toString(), 'hello2\n');
 });
+
+test('empty file', t => {
+  const result = shell.sed('widget', 'wizzle', 'test/resources/sed/empty');
+  t.is(result.code, 0);
+  t.is(result.toString(), '');
+});

@nfischer
Copy link
Member

@nfischer nfischer commented Nov 9, 2018

@wyardley thanks for investigating sed (and thanks for the pointers). Glad to hear there's no bug.

Do we need to check an empty file, or do you think it's sufficient to check files that don't match the pattern? That patch seems reasonable for a separate PR (it can still target this bug).

@wyardley
Copy link
Contributor Author

@wyardley wyardley commented Nov 9, 2018

Do we need to check an empty file, or do you think it's sufficient to check files that don't match the pattern? That patch seems reasonable for a separate PR (it can still target this bug).

The behavior seems the same, so I don't think it's a big thing either way... easy enough to make a PR for the extra test case if you want it tho.

@nfischer
Copy link
Member

@nfischer nfischer commented Nov 10, 2018

Yeah, it would be nice to explicitly test the successful return code if we aren't testing it already.

nfischer added a commit that referenced this issue Nov 13, 2018
As discussed as an aside in #900, add test case with an empty file.
nfischer added a commit that referenced this issue Nov 13, 2018
When we can read all files, but none match, exit 1 and return no output. This
matches the behavior of grep more closely.

Fixes #900
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants