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

add primaries to _test #23

Merged
merged 8 commits into from
Sep 15, 2012
Merged

add primaries to _test #23

merged 8 commits into from
Sep 15, 2012

Conversation

aeosynth
Copy link
Contributor

@aeosynth aeosynth commented Sep 8, 2012

I started adding tests, but the special files caused the tests for mv to fail. Also I couldn't figure out how to make a socket.

@arturadib
Copy link
Collaborator

hey many thanks for adding this. yeah ideally we'd merge with tests and docs (the docs are automatically extracted from the comments above the _test() function).

if you can't figure out why mv is failing can you please push the tests so I can take a crack at it? thanks again.

@aeosynth
Copy link
Contributor Author

aeosynth commented Sep 8, 2012

Just create special files (socket, fifo, block, character, not sure which
specifically) in the resources directory to make mv fall.

I can add docs and tests.

@arturadib
Copy link
Collaborator

thanks, looking forward to it

@aeosynth
Copy link
Contributor Author

aeosynth commented Sep 8, 2012

a block device (mknod test/resources/block b 1 1) causes the following error:

Running test: mv.js
shell.js: internal error
Error: UNKNOWN, unknown error
    at Object.fs.readSync (fs.js:381:19)
    at copyFileSync (/Users/james/src/assets/shelljs/shell.js:1119:20)
    at /Users/james/src/assets/shelljs/shell.js:311:5
    at Array.forEach (native)
    at Object._cp (/Users/james/src/assets/shelljs/shell.js:270:11)
    at Object.cp (/Users/james/src/assets/shelljs/shell.js:1074:23)
    at Object.<anonymous> (/Users/james/src/assets/shelljs/test/mv.js:20:7)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)

a character device (mknod test/resources/character c 1 1) and fifo (mkfifo test/resources/fifo) causes the mv test to hang

symlink, file, directory work fine. haven't tested sockets.

@aeosynth
Copy link
Contributor Author

aeosynth commented Sep 9, 2012

added docs and (repetitive) tests. git apparently doesn't track block / character / fifo files, so those have to be created somehow.

@aeosynth
Copy link
Contributor Author

ok removed tests for files git doesn't track, the mv tests all pass now

@aeosynth
Copy link
Contributor Author

http://superuser.com/questions/440873/git-unable-add-device-file

https://github.com/git/git/blob/master/dir.c#L1028-1036

/*
 * Read a directory tree. We currently ignore anything but
 * directories, regular files and symlinks. That's because git
 * doesn't handle them at all yet. Maybe that will change some
 * day.
 *
 * Also, we ignore the name ".git" (even if it is not a directory).
 * That likely will not change.
 */


var result = shell.test('-f', 'resources/link');
assert.equal(shell.error(), null);
assert.equal(result, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry I missed this before - I think that on Unix test -f link returns true (i.e. it's a link and a file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested and you're right, although the man page states

 -f file       True if file exists and is a regular file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like a wtf and dilutes the power of this test; to test if a file is really a regular file, you'd have to test both -f and ! -L.

@aeosynth
Copy link
Contributor Author

fixed

arturadib added a commit that referenced this pull request Sep 15, 2012
add further types to test(), -L, -e etc
@arturadib arturadib merged commit 0be8a1c into shelljs:master Sep 15, 2012
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.

2 participants