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

Change the behavior of file.recurse so that it will log an error if it p... #1170

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ian-plunkett-ck
Copy link

...rocesses a broken symlink instead of causing a fatal error. We wrap the call to fs.statSync(filepath).isDirectory() in a try/catch block, then log on the error condition.

This changes relates to this issue I filled - #1165

…t processes a broken symlink instead of causing a fatal error. We wrap the call to fs.statSync(filepath).isDirectory() in a try/catch block, then log on the error condition.
package.json Outdated
@@ -1,7 +1,7 @@
{
"name": "grunt",
"description": "The JavaScript Task Runner",
"version": "0.4.6-0",
"version": "0.4.6-1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't bump the version

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, reverted the version bump.

@vladikoff vladikoff added this to the 0.4.6 milestone Jun 20, 2014
Reverting version bump
} catch (e) {
grunt.log.error(e);
}
if (isDirectory) {
recurse(rootdir, callback, unixifyPath(path.join(subdir || '', filename || '')));
} else {
callback(unixifyPath(filepath), rootdir, subdir, filename);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callback would still fire if there is an error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is, yeah. I am not sure what would be best to do in the situation. Current behavior is a fatal error and a hard stop if it encounters a broken symlink. With this change, it will log an error and continue on. Unfortunately, fs.statSync(filepath) just throws a generic error, so we can't get more specific with the catch clause.

@cowboy
Copy link
Member

cowboy commented Jun 20, 2014

Right now, this is what happens for each file:

  1. stat the file. if that fails, throw an ENOENT exception and burst into flames.
  2. is the file a directory? if so, recurse.
  3. otherwise, assume it's a file and pass it into the callback.

Should this be the desired behavior?

  1. stat the file. if that fails, skip the file. (verbose log that the file was skipped?)
  2. is the file a directory? if so, recurse.
  3. otherwise, assume it's a file and pass it into the callback.

Will this cause problems for anyone?

Getting rid of this new line.  Emacs keeps adding them.
@vladikoff vladikoff added the fs label Jun 20, 2014
@vladikoff
Copy link
Member

ping @iancreditkarma ^

@ian-plunkett-ck
Copy link
Author

Ah sorry. I was waiting for additional comment from others. The description @cowboy gave is almost correct. The only difference is that the file would not be skipped if it failed the stat. It would be passed to the callback and an error level log would be produced.

I was thinking about this more and this should probably function more or less like the bare invocation of the unix utility find. find will simply list the file (without warning).

It should probably be up to the callback to handle any issues that arise with the files passed during the directory recursion.

@cowboy
Copy link
Member

cowboy commented Jul 10, 2014

@iancreditkarma what benefit is there if the callback is called for a file that can't be statted? It seems like it should be skipped.

@vladikoff
Copy link
Member

ping @iancreditkarma ^

@ian-plunkett-ck
Copy link
Author

@cowboy @vladikoff - That sounds pretty reasonable - skipping files that can't be statted. I'm out of town until tomorrow. I'll update the pull request as soon as I get back in town.

@cowboy
Copy link
Member

cowboy commented Sep 19, 2014

Ping @iancreditkarma any updates?

Base automatically changed from master to main March 22, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants