Add ignore and hidden file support to add #1448

gatesvp · 2015-07-06T06:43:40Z

#653 support, attempt number 4.

License: MIT
Signed-off-by: Gaetan Voyer-Perrault gatesvp@gmail.com

jbenet · 2015-07-06T06:49:59Z

commands/files/is_hidden.go

+
+	fName := filepath.Base(f.FileName())
+
+	if strings.HasPrefix(fName, ".") && len(fName) > 1 {


this may capture .. but not .. is that intended? (maybe dont want it for either?)

So I'm assuming that filepath.Base(f.FileName()) will wipe out any instance of the "double dot" because we're just looking at the last element of the Path. http://golang.org/pkg/path/filepath/#Base

According to the docs, we only get a file name or a "single dot" for an empty path, hence the len(fName) > 1 check.

sounds good 👍

jbenet · 2015-07-06T06:57:54Z

@gatesvp this LGTM! i'll give one more pass manually (playing with it) to see if there's any major UX flaws, and merge. thank you very much for all the work on this changeset. it will be very useful to tons of people! \o/

jbenet · 2015-07-06T08:12:33Z

Hey @gatesvp am i doing something wrong?

> tree -a
.
├── .a
├── .b
├── .ipfsignore
├── a
├── aa
│   ├── a
│   └── b
├── b
└── bb
    ├── .ipfsignore
    ├── a
    └── b

2 directories, 10 files
> cat .ipfsignore
b
> cat bb/.ipfsignore
a
> ipfs add -r bb
added QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN bb/a
added QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN bb/b
added QmUvbZ6woqqny4kMTWYwH5Y7Rs2KwGnHYAwLt5DV9HKuZG bb
> echo "should have not added bb/a"
should have not added bb/a
> echo "(maybe) should have not added bb/b"
(maybe) should have not added bb/b
> ipfs add -r .
added QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN a
added QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN aa/a
added QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN aa/b
added QmUvbZ6woqqny4kMTWYwH5Y7Rs2KwGnHYAwLt5DV9HKuZG aa
added QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN b
added QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN bb/a
added QmZULkCELmmk5XNfCgTnCyFgAVxBRBXyDHGGMVoLFLiXEN bb/b
added QmUvbZ6woqqny4kMTWYwH5Y7Rs2KwGnHYAwLt5DV9HKuZG bb
added QmP3bmqWiPux3qRdEVJZhe12sYqJgqa2HoCCaKCQPfCjpH .
> echo "(maybe) should have not added aa/b"
(maybe) should have not added aa/b
> echo "should have not added ./b"
should have not added ./b

gatesvp · 2015-07-06T08:58:15Z

hmmm... it's not matching the exact folder name? There's no test for the scenario of an ignore file that exactly matches a folder name.

Let me add/run this test (after I sleep) and see if I can't figure out what we're missing? I suspect that addDir may be missing a check for ignoreFiles because I'm checking the children but not the folder itself.

jbenet · 2015-07-06T09:29:55Z

hmmm... it's not matching the exact folder name? There's no test for the scenario of an ignore file that exactly matches a folder name.

it's not skipping ignored files. added but shouldn't have been:

aa/b # should ignore "b" (parent)
b    # should ignore "b"
bb/a # should ignore "a"
bb/b # should ignore "b" (parent)

gatesvp · 2015-07-07T05:27:48Z

So I have my brain back and here's my test:

mkdir -p temp &&
cd temp &&
echo "blah" > .a &&
echo "blah" > .b &&
echo "b" > .ipfsignore &&
echo "blah" > a &&
echo "blah" > b &&
mkdir aa &&
echo "blah" > aa/a &&
echo "blah" > aa/b &&
mkdir bb &&
echo "blah" > bb/a &&
echo "blah" > bb/b &&
echo "a" > bb/.ipfsignore &&
ipfs add -r .

That should resemble your tree structure above. My output is

added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf a
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf aa/a
added QmcjpKziyj7YeFWztxXsjR8npDyi5WSfzz3sjPMLZ4k7nw aa
added QmQDngQu2bWLFDqmKGUQCbde2VfeCNZYsVsNmim1Pxz7Ke .

So it actually ignored the bb folder. Doing ipfs add -r bb actually returned the "ignored" error message. If I step into the bb folder and do the recursive add it correctly ignore both the a and b file within the bb folder.

So it's all working as expected on Linux box. Is there a possible configuration issue here?

jbenet · 2015-07-07T05:36:27Z

in osx, with your branch, i get:

> mkdir -p temp && \
cd temp && \
echo "blah" > .a && \
echo "blah" > .b && \
echo "b" > .ipfsignore && \
echo "blah" > a && \
echo "blah" > b && \
mkdir aa && \
echo "blah" > aa/a && \
echo "blah" > aa/b && \
mkdir bb && \
echo "blah" > bb/a && \
echo "blah" > bb/b && \
echo "a" > bb/.ipfsignore && \
ipfs add -r .
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf a
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf aa/a
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf aa/b
added QmbU4K1QsoFqnVZB8VZEJ9qdNXn14N2xNj227s24LoAYev aa
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf b
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf bb/a
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf bb/b
added QmbU4K1QsoFqnVZB8VZEJ9qdNXn14N2xNj227s24LoAYev bb
added QmcVAngmQaxmTrFQLC4K2aQAWtoooB473W2osMgJPKXzC8 .

in linux, i get:

mkdir -p temp && cd temp && echo "blah" > .a && echo "blah" > .b && echo "b" > .ipfsignore && echo "blah" > a && echo "blah" > b && mkdir aa && echo "blah" > aa/a && echo "blah" > aa/b && mkdir bb && echo "blah" > bb/a && echo "blah" > bb/b && echo "a" > bb/.ipfsignore && ipfs add -r .
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf a
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf aa/a
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf aa/b
added QmbU4K1QsoFqnVZB8VZEJ9qdNXn14N2xNj227s24LoAYev aa
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf b
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf bb/a
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf bb/b
added QmbU4K1QsoFqnVZB8VZEJ9qdNXn14N2xNj227s24LoAYev bb
added QmcVAngmQaxmTrFQLC4K2aQAWtoooB473W2osMgJPKXzC8 .

So it's all working as expected on Linux box. Is there a possible configuration issue here?

bizarre. wait, is there anything in need to do to the config? no, right? just build + install with gatesvp:master

jbenet · 2015-07-07T05:37:07Z

Aha! Found the problem.

Try it with the daemon on 😄

jbenet · 2015-07-07T05:38:01Z

To be clear, without the daemon, i got the expected:

added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf a
added QmfM9ic5LJj7V6ecozFx1MkSoaaiq3PXfhJoFvyqzpLXSf aa/a
added QmcjpKziyj7YeFWztxXsjR8npDyi5WSfzz3sjPMLZ4k7nw aa
added QmQDngQu2bWLFDqmKGUQCbde2VfeCNZYsVsNmim1Pxz7Ke .

jbenet · 2015-07-07T05:40:25Z

core/commands/add.go

-				rootnd, err := addFile(n, file, outChan, progress, wrap, trickle)
+				// If the file is not a folder, then let's get the root of that
+				// folder and attempt to load the appropriate .ipfsignore.
+				localIgnorePatterns := checkForParentIgnorePatterns(file.FileName(), ignoreFilePatterns)


the problem is that -- doing this here, in the Run function, means it's getting executed in the daemon with the daemon chdir.

we need to do all this part (the ignores) before hand, in the PreRun (or in the cmdslib? maybe the cmdslib needs an option to be given a function like os.Walk that decides whether to add files or not -- that way we can implement support for .ipfsignore outside the cmdslib)

the ignore logic should really go in commands/files

the hidden logic should be fine to stay here though, we encode the files attributes and send them over. But the enumeration of which files to add is done in either commands/cli/parse or commands/files, namely the serialFile struct (i think) which is basically a directory and when NextFile is called on it, it will return the next item under it...

This change is more difficult than I first thought it would be...

whyrusleeping · 2015-07-07T16:25:43Z

👍 to moving all the misc parameters into a struct. Makes things a tad cleaner.

jbenet · 2015-07-08T03:46:12Z

@whyrusleeping we had the unfortunate discovery that this needs to change the cmdslib. do you think we should get rid of some of the File abstractions and use an os.Walk? could you take a look @whyrusleeping and lmk how much effort it may be?

this test fails .ipfsignore - only with daemon on. See: #1448 License: MIT Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

jbenet · 2015-07-09T12:21:00Z

I split this PR into two parts:

the --hidden implementation, merged in Add hidden file support to add #1460
the .ipfsignore implementation, in http://github.com/ipfs/go-ipfs/commits/ipfsignore with the test that fails.

whyrusleeping · 2015-07-14T18:50:26Z

@jbenet I think it may be worthwhile to use the os.Walk instead of the files abstraction. It's gonna be a moderate amount of work to do the refactor, and we will have to figure out how to deal with input from stdin. But the increased (hopefully) simplicity will be well worth it IMO

jbenet · 2015-07-15T04:32:02Z

@jbenet I think it may be worthwhile to use the os.Walk instead of the files abstraction. It's gonna be a moderate amount of work to do the refactor, and we will have to figure out how to deal with input from stdin. But the increased (hopefully) simplicity will be well worth it IMO

@whyrusleeping i agree.

jbenet added the backlog label Jul 6, 2015

jbenet reviewed Jul 6, 2015
View reviewed changes

whyrusleeping mentioned this pull request Jul 6, 2015

ipfs-add: Add flag to exclude paths #1451

Open

whyrusleeping self-assigned this Jul 7, 2015

jbenet reviewed Jul 7, 2015
View reviewed changes

whyrusleeping removed their assignment Jul 7, 2015

jbenet mentioned this pull request Jul 9, 2015

Add hidden file support to add #1460

Merged

jbenet added a commit that referenced this pull request Jul 9, 2015

added test that fails .ipfsignore

4b9547d

this test fails .ipfsignore - only with daemon on. See: #1448 License: MIT Signed-off-by: Juan Batiz-Benet <juan@benet.ai>

gatesvp closed this Jul 28, 2015

gatesvp force-pushed the master branch from e776f16 to f417cb5 Compare July 28, 2015 06:19

jbenet removed the backlog label Jul 28, 2015

rht mentioned this pull request Sep 3, 2015

Use filepath.Walk to compute serialfile total size #1628

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ignore and hidden file support to add #1448

Add ignore and hidden file support to add #1448

gatesvp commented Jul 6, 2015

jbenet Jul 6, 2015

gatesvp Jul 6, 2015

jbenet Jul 6, 2015

jbenet commented Jul 6, 2015

jbenet commented Jul 6, 2015

gatesvp commented Jul 6, 2015

jbenet commented Jul 6, 2015

gatesvp commented Jul 7, 2015

jbenet commented Jul 7, 2015

jbenet commented Jul 7, 2015

jbenet commented Jul 7, 2015

jbenet Jul 7, 2015

whyrusleeping Jul 7, 2015

whyrusleeping Jul 7, 2015

whyrusleeping commented Jul 7, 2015

jbenet commented Jul 8, 2015

jbenet commented Jul 9, 2015

whyrusleeping commented Jul 14, 2015

jbenet commented Jul 15, 2015


		fName := filepath.Base(f.FileName())

		if strings.HasPrefix(fName, ".") && len(fName) > 1 {

Add ignore and hidden file support to add #1448

Add ignore and hidden file support to add #1448

Conversation

gatesvp commented Jul 6, 2015

jbenet Jul 6, 2015

Choose a reason for hiding this comment

gatesvp Jul 6, 2015

Choose a reason for hiding this comment

jbenet Jul 6, 2015

Choose a reason for hiding this comment

jbenet commented Jul 6, 2015

jbenet commented Jul 6, 2015

gatesvp commented Jul 6, 2015

jbenet commented Jul 6, 2015

gatesvp commented Jul 7, 2015

jbenet commented Jul 7, 2015

jbenet commented Jul 7, 2015

jbenet commented Jul 7, 2015

jbenet Jul 7, 2015

Choose a reason for hiding this comment

whyrusleeping Jul 7, 2015

Choose a reason for hiding this comment

whyrusleeping Jul 7, 2015

Choose a reason for hiding this comment

whyrusleeping commented Jul 7, 2015

jbenet commented Jul 8, 2015

jbenet commented Jul 9, 2015

whyrusleeping commented Jul 14, 2015

jbenet commented Jul 15, 2015