Add support for hidden files and .ipfsignore file #1232

gatesvp · 2015-05-14T17:34:29Z

Should resolve issue #653.

Should incorporate all of the feedback from the previous PR that was closed.

Manually tested / built on Linux and Windows with a set of folders of my own design.

…ate .ipfsignore files

…w methods on the struct

fix ipfs add to ensure that we are checking parent paths for appropriate .ipfsignore files [ reworded to conform to commit msg guidelines ]

ipfs#1204 [ reworded to conform to commit msg guidelines ]

Basic params are a struct and addFile/addDir are now methods on the struct [ reworded to conform to commit msg guidelines ]

jbenet · 2015-05-14T19:12:26Z

@gatesvp thanks! btw-- you can just git push -f <remote> <branch> to overwrite the old branch-- that way dont need to open a second PR.

gatesvp · 2015-05-14T20:28:18Z

Thank you, that makes sense.

Still a git newbie, so I'm always a little hesitant to do anything that involves a -f :)

jbenet · 2015-05-14T21:51:08Z

Testing all the commits

jbenet · 2015-05-15T00:31:44Z

Tests don't pass before the last commit -- i'm just going to squash everything.

jbenet · 2015-05-15T00:46:18Z

Squashed over at gatesvp-squashed

jbenet · 2015-05-15T00:51:36Z

Ok, @gatesvp this is looking good.

This doesn't have any tests though. I can't merge this without tests because this is a very important change of functionality. It will likely break right away without tests. We need at least some sharness tests for it so that we both (a) make sure it does what we expect it to, and (b) catches any problems.

It may be worth taking a look at git's .gitignore tests: https://github.com/git/git/blob/master/t/t0008-ignores.sh

jbenet · 2015-05-15T00:52:26Z

maybe can see if others who want this can help write tests.

gatesvp · 2015-05-15T02:52:45Z

So I actually support your desire for tests 100%. Honestly would have been easier to write with test file to work off of :) I basically built this convoluted folder of hidden files and .ipfsignore files and kept building and running against that.

However, it doesn't look like the CLI commands have any _test.go files, so I'm not really clear where to start on this.. of course, as I type this, I realize that's probably what the sharness is for.

I'll fire up my Linux Box and try building some extra tests in sharness based on my crazy folder.

gatesvp · 2015-05-20T23:10:56Z

Update on this item. I have some basic tests working on my local dev box.

However, I am trapped on one set of tests. I am trying to test that .ipfsignore files from parent directories are respected. This works on my personal tests but fails in the sharness temp folder.

On Ubuntu it looks like the test creates this \trash folder with a space in the name.

My code tries to walk the parent folders by splitting out the components and then gluing them back together one at a time with path.join(). However, this whole process fails inside of this weird \trash folder. Effectively doing a split then a join results in a bad directory :(
https://github.com/gatesvp/go-ipfs/blob/iss653/core/commands/add.go#L389

I will keep tinkering with it, but if we have a known workaround here, it would be appreciated.

whyrusleeping · 2015-05-20T23:50:20Z

@gatesvp i dont know how sold i am on respecting ipfsignores from parent directories in the first place... @jbenet thoughts?

whyrusleeping · 2015-05-21T00:51:01Z

So, a little bit more thought on this:

I think we should respect 1) the global .ipfsignore, 2) the one in our current working directory, and 3) any we encounter going downwards recursively (if its a recursive add).

An example of 3:

top/
  a/
    .ipfsignore // says to ignore '*.blah'
    testfile.txt
    testfile.blah
  b/
    testfile.txt
    testfile.blah

The testfile.blah under b/ would be added while the one under a/ would be skipped.

let me know what you think, and also lets gets @jbenet's input.

jbenet · 2015-05-21T02:00:54Z

My code tries to walk the parent folders by splitting out the components and then gluing them back together one at a time with path.join(). However, this whole process fails inside of this weird \trash folder. Effectively doing a split then a join results in a bad directory :(
https://github.com/gatesvp/go-ipfs/blob/iss653/core/commands/add.go#L389

sounds like it's not dealing with spaces correctly. to catch errors like this is precisely why the trash folders have a space. (thanks git and @chriscool)

i dont know how sold i am on respecting ipfsignores from parent directories in the first place... @jbenet thoughts?

i think the way @gatesvp has it -- which is 1-3 + parent dirs -- is the right way. the way git does it, too.

"doing parent dirs" should go along with "doing child dirs". it's a symmetric relationship. otherwise tool will behave differently depending on what dir in hierarchy you run it.

to give an example,

ipfs add -r foo
cd foo && ipfs add -r .
cd foo/bar && ipfs add -r ../
cd foo/bar/baz && ipfs add -r ../../

should all ignore the same files, not be dependent on where you started the call.. that'd be super confusing.

whyrusleeping · 2015-05-21T02:16:32Z

Oh, i agree with that. I thought that (using your example) it would look at 'foo's parent for an ipfsignore file.

gatesvp · 2015-05-21T02:37:16Z

Key difference here is that git has a known "root" which is the top-most directory under source control. ipfs does not have such a "root" hence the differences below.

What I have right now is actually recursive "to the root". There's no check of current working directory (CWD)

So with respect to @whyrusleeping 's notation, I have 1 & 3, but not really 2 (CWD).

If you have /root/foo/bar/baz and you run ipfs add -r /root/foo/bar or ipfs add -r /root/foo my code is actually looking for the following files:

/root/.ipfsignore
/root/foo/.ipfsignore => for everything under foo
/root/foo/bar/.ipfsignore => for everything in & under bar
/root/foo/bar/baz/.ipfsignore => for everything in & under baz

It doesn't sound like "to the root" is really desired behaviour... but it feels like all of the options are kind of awkward.

"Current Working Directory" would break @jbenet 5 line example (hence not implemented)
Travelling "to the root" means that ipfs add -r /root/code/project and ipfs add -r /root/code/project/subtask both ignore the sames files. But it may also result in unexpected changes if someone modifies /root/.ipfsignore

(extra note, it is the "to the root" functionality that is failing in my sharness tests)

whyrusleeping · 2015-05-21T15:08:10Z

yeah, kill my current working directory idea, that was bad. i'm quite skeptical about traveling to the root... youre right about unexpected behaviour if /root/.ipfsignore gets modified

This will let us handle functionality like ignored files (#1232) without needing to alter the adder API or chunk/trickle files that we know we're going to ignore ahead of time. I considered sticking with nOut *dag.Node instead of ignore bool to give folks more flexibility (e.g. "use nOut instead of chunking/trickling that path yourself"), but I can't think of an easy way to distinguish "please ignore this path" from "please handle this path using your usual chunking/trickling" if we're returning just nOut and err. So this callback just returns a Boolean distinguishing "please ignore" from "please chunk/trickle as usual", and we can add an additional nOut return if we find a use case for a callback-supplied substitute node.

jbenet · 2015-06-30T18:20:52Z

@gatesvp would love to have this in, any update on tests here?

gatesvp · 2015-06-30T22:17:02Z

Yes, I am still alive and think I have it figured out :)

Testing it on my windows box right now, will test some more tonight. And
give an update.

On Tue, Jun 30, 2015 at 11:20 AM, Juan Batiz-Benet <notifications@github.com

wrote:

@gatesvp https://github.com/gatesvp would love to have this in, any
update on tests here?

—
Reply to this email directly or view it on GitHub
#1232 (comment).

GitCop · 2015-07-02T07:56:55Z

There were the following issues with your Pull Request

Commit: ddaa9a6
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 9517911
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 88077f7
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 50e3b15
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 99f15e6
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: efae6b1
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
- Your subject line is longer than 80 characters
Commit: 3923aa8
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 1e66056
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 66a5f1f
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
- Your subject line is longer than 80 characters
Commit: f04ac6f
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 7f818e2
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 032cd3c
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 70dc7d9
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 8e9a477
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: bd7c8f5
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 9e5dfe2
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 1914a26
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 153886e
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 500e756
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 5cb34ee
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 441740a
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: e8d8642
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: ff0d236
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 954c0db
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 53fc454
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 372862b
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: 314bb95
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>
Commit: b86d5d4
- Invalid signoff. Commit message must end with License: MIT
  Signed-off-by: .* <.*>

Guidelines are available to help. Your feedback on GitCop is welcome on this issue

This message was auto-generated by https://gitcop.com

gatesvp · 2015-07-02T08:07:36Z

So that last changeset adds the required tests and also fixes an issue I found while building out the tests. Namely that filepath.Join was ignoring "blank" strings in a key location.

As it stands, that last check-in is a little wild because I am pulling in two months of work and trying to rebase over top.

@jbenet this may need some manual intervention. The only key files changed in that last set were t0040-... and core/commands/add.go. Their end state is correct

gatesvp added 26 commits May 3, 2015 22:59

Make ipfs add ignore files by default.

ddaa9a6

Fix my build

9517911

Merge branch 'master' of https://github.com/ipfs/go-ipfs into iss653

88077f7

ipfs add now respects .ipfsignore files

50e3b15

Merge branch 'master' of https://github.com/ipfs/go-ipfs into iss653

99f15e6

fix ipfs add to ensure that we are checking parent paths for appropri…

efae6b1

…ate .ipfsignore files

Merge branch 'master' of https://github.com/gatesvp/go-ipfs

3923aa8

Clean-up based on PR feedback ipfs#1204

1e66056

Clean-up add CLI. Basic params are a struct and addFile/addDir are no…

66a5f1f

…w methods on the struct

Add recursive check for parent .ipfsignore files

f04ac6f

Respect file path separator on all OSes

7f818e2

Fix is_hidden on non-Windows

032cd3c

Vendoring for .ipfsignore support

70dc7d9

Fix OS-specific slashing without strconv

8e9a477

Merge branch 'iss653' of https://github.com/gatesvp/go-ipfs into iss653

bd7c8f5

Make ipfs add ignore files by default.

9e5dfe2

Fix my build

1914a26

ipfs add now respects .ipfsignore files

153886e

check parent paths for .ipfsignore files

500e756

fix ipfs add to ensure that we are checking parent paths for appropriate .ipfsignore files [ reworded to conform to commit msg guidelines ]

Clean-up based on PR ipfs#1204 feedback

5cb34ee

ipfs#1204 [ reworded to conform to commit msg guidelines ]

Clean-up add CLI.

441740a

Basic params are a struct and addFile/addDir are now methods on the struct [ reworded to conform to commit msg guidelines ]

Add recursive check for parent .ipfsignore files

e8d8642

Respect file path separator on all OSes

ff0d236

Fix OS-specific slashing without strconv

954c0db

Fix is_hidden on non-Windows

53fc454

Vendoring for .ipfsignore support

372862b

jbenet added the backlog label May 14, 2015

jbenet mentioned this pull request May 19, 2015

Sprint H ipfs/team-mgmt#9

Closed

52 tasks

whyrusleeping mentioned this pull request May 26, 2015

Sprint I ipfs/team-mgmt#10

Closed

49 tasks

wking mentioned this pull request May 27, 2015

Spec for adder APIs (both low-level and high-level) #1291

Closed

whyrusleeping mentioned this pull request Jun 2, 2015

Sprint J ipfs/team-mgmt#11

Closed

58 tasks

whyrusleeping mentioned this pull request Jun 9, 2015

Sprint K ipfs/team-mgmt#12

Closed

50 tasks

gatesvp added 2 commits July 2, 2015 03:15

Add tests for the various hidden & recursive modes.

314bb95

Add tests for the various hidden & recursive modes.

b86d5d4

gatesvp closed this Jul 6, 2015

jbenet removed the backlog label Jul 6, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for hidden files and .ipfsignore file #1232

Add support for hidden files and .ipfsignore file #1232

gatesvp commented May 14, 2015

jbenet commented May 14, 2015

gatesvp commented May 14, 2015

jbenet commented May 14, 2015

jbenet commented May 15, 2015

jbenet commented May 15, 2015

jbenet commented May 15, 2015

jbenet commented May 15, 2015

gatesvp commented May 15, 2015

gatesvp commented May 20, 2015

whyrusleeping commented May 20, 2015

whyrusleeping commented May 21, 2015

jbenet commented May 21, 2015

whyrusleeping commented May 21, 2015

gatesvp commented May 21, 2015

whyrusleeping commented May 21, 2015

jbenet commented Jun 30, 2015

gatesvp commented Jun 30, 2015

GitCop commented Jul 2, 2015

gatesvp commented Jul 2, 2015

Add support for hidden files and .ipfsignore file #1232

Add support for hidden files and .ipfsignore file #1232

Conversation

gatesvp commented May 14, 2015

jbenet commented May 14, 2015

gatesvp commented May 14, 2015

jbenet commented May 14, 2015

jbenet commented May 15, 2015

jbenet commented May 15, 2015

jbenet commented May 15, 2015

jbenet commented May 15, 2015

gatesvp commented May 15, 2015

gatesvp commented May 20, 2015

whyrusleeping commented May 20, 2015

whyrusleeping commented May 21, 2015

jbenet commented May 21, 2015

whyrusleeping commented May 21, 2015

gatesvp commented May 21, 2015

whyrusleeping commented May 21, 2015

jbenet commented Jun 30, 2015

gatesvp commented Jun 30, 2015

GitCop commented Jul 2, 2015

gatesvp commented Jul 2, 2015