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 support for .ipfsignore and hidden files #1204

Closed
wants to merge 8 commits into from
19 changes: 19 additions & 0 deletions commands/files/is_hidden.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// +build !windows

package files

import (
"path/filepath"
"strings"
)

func IsHidden(f File) bool {

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

if strings.HasPrefix(fName, ".") && len(fName) > 1 {
return true, nil
}

return false, nil
}
29 changes: 29 additions & 0 deletions commands/files/is_hidden_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// +build windows

package files

import (
"path/filepath"
"strings"
"syscall"
)

func IsHidden(f File) bool {

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

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

p, e := syscall.UTF16PtrFromString(f.FileName())
if e != nil {
return false
}

attrs, e := syscall.GetFileAttributes(p)
if e != nil {
return false
}
return attrs&syscall.FILE_ATTRIBUTE_HIDDEN != 0
}
2 changes: 1 addition & 1 deletion commands/files/serialfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package files
import (
"io"
"os"
fp "path"
fp "path/filepath"
"sort"
"syscall"
)
Expand Down
104 changes: 93 additions & 11 deletions core/commands/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ package commands
import (
"fmt"
"io"
"os"
"path"
"strings"

"github.com/ipfs/go-ipfs/Godeps/_workspace/src/github.com/cheggaaa/pb"
context "github.com/ipfs/go-ipfs/Godeps/_workspace/src/golang.org/x/net/context"
ignore "github.com/sabhiram/go-git-ignore"
Copy link
Member

Choose a reason for hiding this comment

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

vendoring-- try running:

make vendor

from top level? (i can also vendor for you, lmk if desired)

(4/100)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Obviously doesn't work from Windows, will setting this up on my Linux VM and going from there.


cmds "github.com/ipfs/go-ipfs/commands"
files "github.com/ipfs/go-ipfs/commands/files"
Expand All @@ -29,6 +31,7 @@ const progressReaderIncrement = 1024 * 256
const (
progressOptionName = "progress"
wrapOptionName = "wrap-with-directory"
hiddenOptionName = "hidden"
Copy link
Member

Choose a reason for hiding this comment

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

@chriscool @whyrusleeping @wking do you think we should do:

ipfs add --hidden path
ipfs add path # default to not hidden

# or

ipfs add --skip-hidden path
ipfs add path # default to yes hidden

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very found of talking about "hidden" files. It think it is confusing to talk about both "ignored" and "hidden" files.

If we want some files or directories to be ignored by default, then ipfs init should create a default .ipfsignore file that contains lines to ignore those files, though it is true that there may be a few hardcoded files or directories that are always ignored (for example in Git the .git directory is obviously always ignored).

By the way it is good anyway if ipfs init creates .ipfsignore even if it is empty, but it's even better if it is created with some comments that explain how it works.

By default ignored files should not be added by ipfs add except if a special option is passed. In Git you must use git add --force to add ignored files, so I would suggest --force too or maybe --force-ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Thu, May 07, 2015 at 05:56:13AM -0700, Christian Couder wrote:

@@ -29,6 +31,7 @@ const progressReaderIncrement = 1024 * 256
const (
progressOptionName = "progress"
wrapOptionName = "wrap-with-directory"

  • hiddenOptionName = "hidden"

I am not very found of talking about "hidden" files. It think it is
confusing to talk about both "ignored" and "hidden" files.

I agree. Folks can always add .* to their ignore list if they don't
want to track dot-prefixed files. I understand that this makes things
a bit wonky on Windows (where we'd be ignoring FILE_ATTRIBUTE_HIDDEN),
but I'm not sure that supporting that attribute is worth a parallel
parameter. Maybe we could have a magic-comment in .ipfsignore to
ignore that attribute?

$ cat .ipfsignore

windows: hidden

If we want some files or directories to be ignored by default, then
ipfs init should create a default .ipfsignore file that contains
lines to ignore those files, though it is true that there may be a
few hardcoded files or directories that are always ignored (for
example in Git the .git directory is obviously always ignored).

I'm not in favor of auto-creating an .ipfsignore in ipfs init, since
it's just extra weight for folks who don't need to ignore anything.

I'm also not in favor of hard-coded default ignores, since it seems
more consistent to track everything by default. I don't want to be in
the business of picking reasonable defaults for all users, and it's
easy enough for folks to add things they want ignored to their
per-user or per-tree ignore lists.

Since we already have a per-user ~/.ipfs directory, I think it's worth
calling the per-user file ~/.ipfs/ignore and only using .ipfsignore
for in-tree ignore configuration.

By the way it is good anyway if ipfs init creates .ipfsignore
even if it is empty, but it's even better if it is created with some
comments that explain how it works.

As I said above, I'd rather not create the file automatically. But
docs are great, so I'd like some explaining how the ignore feature
works, the ignore-file syntax, etc. It looks like the tour has a
stubby FileBasics chapter. Maybe a new entry there? At least until
we get the Merkle archive and can shift the tour into a bundle
(#1195).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chriscool

I am not very found of talking about "hidden" files. It think it is confusing to talk about both "ignored" and "hidden" files.

I agree that it is confusing.

It's also confusing for a Windows user to go ipfs add . -r and see a bunch of folders they didn't know existed. Just like Mac users are surprised to find the .Ds_Store files when they plug their USB drive into another computer.

So whose confusion do we want to suffer?

And if IPFS doesn't support one of the two notions, do we doom somebody else to have to write it instead?

Copy link
Member

Choose a reason for hiding this comment

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

i'm terrible at UX, but i expect dotfiles and hidden files to be ignored by default

Copy link
Contributor

Choose a reason for hiding this comment

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

On Thu, May 07, 2015 at 12:01:54PM -0700, Jeromy Johnson wrote:

@@ -29,6 +31,7 @@ const progressReaderIncrement = 1024 * 256
const (
progressOptionName = "progress"
wrapOptionName = "wrap-with-directory"

  • hiddenOptionName = "hidden"

i'm terrible at UX, but i expect dotfiles and hidden files to be
ignored by default

For what it's worth, Git does not ignore dotfiles by default. I'm not
sure how they handle Windows' hidden attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wking

I'm not sure how they handle Windows' hidden attribute

Just tested this locally and git respects the hidden attribute. It also creates the .git folder as a hidden folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Thu, May 07, 2015 at 01:27:53PM -0700, Gaëtan Voyer-Perrault wrote:

I'm not sure how they handle Windows' hidden attribute

Just tested this locally and git respects the hidden attribute. It
also creates the .git folder as a hidden folder.

That's probably a good model to emulate then. With IPFS, it would be
possible to store information like this in a Metadata wrapper around
files and directories, but I'm not sure how much like Window's native
filesystem we need IPFS to be.

Does Git provide a way to explicitly add hidden files and directories?
If they don't, I'd suggest we follow their lead in that too, at least
until we get around to storing that attribute in a Metadata wrapper.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gatesvp and @wking git init creates the .git/info/exclude file that contains:

$ cat .git/info/exclude 
# git ls-files --others --exclude-from=.git/info/exclude
# Lines that start with '#' are comments.
# For a project mostly in C, the following would be a good set of
# exclude patterns (uncomment them if you want to use them):
# *.[oa]
# *~

Git uses both the .git/info/exclude and the .gitignore files. The .gitignore files are usually tracked in Git and are not created by git init. I think ipfs should do something like that, maybe a .ipfs/ignore that is never tracked and some .ipfsignore that are usually tracked. So what I suggest is that ipfs init creates the .ipfs/ignore file with some comments and maybe some default uncommented lines that could be platform specific.

About what Git does on Windows, please note that Git on Windows is not made from exactly the same code base as Git for other platforms. There is a specific Git For Windows project that adds some Windows specific code and I don't know much about this code so I might be wrong about it.

Anyway some people will want hidden files tracked others will not, so we need a way to switch that on and off, and it seems to me that the .ipfs/ignore and .ipfsignore files are the right place for that.

)

type AddedObject struct {
Expand Down Expand Up @@ -57,6 +60,7 @@ remains to be implemented.
cmds.BoolOption(progressOptionName, "p", "Stream progress data"),
cmds.BoolOption(wrapOptionName, "w", "Wrap files with a directory object"),
cmds.BoolOption("t", "trickle", "Use trickle-dag format for dag generation"),
cmds.BoolOption(hiddenOptionName, "Include files that are hidden"),
},
PreRun: func(req cmds.Request) error {
if quiet, _, _ := req.Option("quiet").Bool(); quiet {
Expand Down Expand Up @@ -90,6 +94,17 @@ remains to be implemented.

progress, _, _ := req.Option(progressOptionName).Bool()
wrap, _, _ := req.Option(wrapOptionName).Bool()
hidden, _, _ := req.Option(hiddenOptionName).Bool()

var ignoreFilePatterns []ignore.GitIgnore

// Check the $IPFS_PATH
if ipfs_path := os.Getenv("$IPFS_PATH"); len(ipfs_path) > 0 {
baseFilePattern, err := ignore.CompileIgnoreFile(path.Join(ipfs_path, ".ipfsignore"))
if err == nil && baseFilePattern != nil {
ignoreFilePatterns = append(ignoreFilePatterns, *baseFilePattern)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

for env vars, want:

os.Getenv("IPFS_PATH") // no $

and i think we shouldn't be using the variable directly here-- we should be using the current repo's path:

req.Context().ConfigRoot
// we should perhaps have a way of getting this path from a .Repo.

It's also possible that we may want to pull it out of the repo with a function:

// maybe not this (may want it as part of config?), but something equivalent to:
ignoreFilePatterns := Repo.IgnoreFiles()

the point being that repos which are not fsrepos (i.e. not in the filesystem) could still implement the functionality.

cc @whyrusleeping @tv42 @wking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch on the $.

I agree with the premise of req.Context().ConfigRoot or something similar. I was asking myself that while coding this up. It seemed like adding a magic environment variable was sub-optimal given that we already had a path for a private folder for the IPFS instance.


outChan := make(chan interface{})
res.SetOutput((<-chan interface{})(outChan))
Expand All @@ -107,7 +122,15 @@ remains to be implemented.
return
}

rootnd, err := addFile(n, file, outChan, progress, wrap)
// If the file is not a folder, then let's get the root of that
// folder and attempt to load the appropriate .ipfsignore.
localIgnorePatterns := ignoreFilePatterns
if !file.IsDirectory() {
parentPath := path.Dir(file.FileName())
localIgnorePatterns = checkForLocalIgnorePatterns(parentPath, ignoreFilePatterns)
}

rootnd, err := addFile(n, file, outChan, progress, wrap, hidden, localIgnorePatterns)
if err != nil {
res.SetError(err, cmds.ErrNormal)
return
Expand Down Expand Up @@ -227,9 +250,24 @@ func add(n *core.IpfsNode, reader io.Reader) (*dag.Node, error) {
return node, nil
}

func addFile(n *core.IpfsNode, file files.File, out chan interface{}, progress bool, wrap bool) (*dag.Node, error) {
func addFile(n *core.IpfsNode, file files.File, out chan interface{}, progress bool, wrap bool, hidden bool, ignoreFilePatterns []ignore.GitIgnore) (*dag.Node, error) {
Copy link
Member

Choose a reason for hiding this comment

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

this function is getting crazy. it may be useful to construct an adder object that holds the configuration:

type adder struct {
  n *core.IpfsNode
  progress bool
  wrap bool
  hidden bool
  ignoreFilePatterns []ignore.GitIgnore
}

func (adr *adder) addFile(file files.File) (*dag.Node, error) { ... }

Copy link
Contributor

Choose a reason for hiding this comment

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

On Wed, May 06, 2015 at 10:25:36PM -0700, Juan Batiz-Benet wrote:

-func addFile(n _core.IpfsNode, file files.File, out chan interface{}, progress bool, wrap bool) (_dag.Node, error) {
+func addFile(n _core.IpfsNode, file files.File, out chan interface{}, progress bool, wrap bool, hidden bool, ignoreFilePatterns []ignore.GitIgnore) (_dag.Node, error) {

this function is getting crazy. it may be useful to construct an
adder object that holds the configuration:

Can we have a public function that uses a channel to support the
progress bar 1? I think we should be thinking over the adder UI
very carefully before altering a public API or creating functionality
that's not part of a public UI. But I haven't looked through the
rest of this PR yet. I'll try and do that tomorrow, but don't wait
for my review if I'm too slow ;).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbenet agreed, when I added two extra inputs, my brain said "time for struct". But I was hesitant to make/break any other existing patterns here.

It seems like most of the CLI methods are operating on "list of Parameters" rather than "struct that wraps Parameters". Honestly, that may be a good fix in general.

// Check if file is hidden
if fileIsHidden := files.IsHidden(file); fileIsHidden && !hidden {
log.Debugf("%s is hidden, skipping", file.FileName())
return nil, &hiddenFileError{file.FileName()}
}

// Check for ignore files matches
for i := range ignoreFilePatterns {
if ignoreFilePatterns[i].MatchesPath(file.FileName()) {
log.Debugf("%s is ignored file, skipping", file.FileName())
return nil, &ignoreFileError{file.FileName()}
}
}

// Check if "file" is actually a directory
if file.IsDirectory() {
return addDir(n, file, out, progress)
return addDir(n, file, out, progress, hidden, ignoreFilePatterns)
}

// if the progress flag was specified, wrap the file so that we can send
Expand Down Expand Up @@ -263,10 +301,13 @@ func addFile(n *core.IpfsNode, file files.File, out chan interface{}, progress b
return dagnode, nil
}

func addDir(n *core.IpfsNode, dir files.File, out chan interface{}, progress bool) (*dag.Node, error) {
log.Infof("adding directory: %s", dir.FileName())
func addDir(n *core.IpfsNode, dir files.File, out chan interface{}, progress bool, hidden bool, ignoreFilePatterns []ignore.GitIgnore) (*dag.Node, error) {
Copy link
Member

Choose a reason for hiding this comment

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

func (adr *adder) addDir(dir files.File) (*dag.Node, error) {...}


tree := &dag.Node{Data: ft.FolderPBData()}
log.Infof("adding directory: %s", dir.FileName())

// Check for an .ipfsignore file that is local to this Dir and append to the incoming
localIgnorePatterns := checkForLocalIgnorePatterns(dir.FileName(), ignoreFilePatterns)

for {
file, err := dir.NextFile()
Expand All @@ -277,16 +318,24 @@ func addDir(n *core.IpfsNode, dir files.File, out chan interface{}, progress boo
break
}

node, err := addFile(n, file, out, progress, false)
if err != nil {
node, err := addFile(n, file, out, progress, false, hidden, localIgnorePatterns)
if _, ok := err.(*hiddenFileError); ok {
// hidden file error, set the node to nil for below
node = nil
} else if _, ok := err.(*ignoreFileError); ok {
// ignore file error, set the node to nil for below
node = nil
} else if err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

so this just silently skips it? so would:

echo foo >.ipfsignore
ipfs add foo

just silently succeed without doing anything?

on this very specific case, git gives an error like:

echo foo >.gitignore
> git add foo
The following paths are ignored by one of your .gitignore files:
foo
Use -f if you really want to add them.
fatal: no files added

Copy link
Member

Choose a reason for hiding this comment

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

(it may be a matter of counting the files added, and in the root function check:

// when adding or ignoring, incr counters:
adr.filesAdded++
adr.filesIgnored++

func (adr *adder) ignoredAllFiles() bool {
  return adr.filesAdded == 0 && adr.filesIgnored > 0
}

if adder.IgnoredAllFiles {
  return errIgnoredAllFiles
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested this one on my box and it was working correctly. The recursive version eats the errors, but adding a specific file returns the error.

 > echo "ignore*" > .ipfsignore
 > ipfs add ignore_match.txt
Error: ignore_match.txt is an ignored file
 > ipfs add . -r
... list of stuff not including ignore_match.txt

return nil, err
}

_, name := path.Split(file.FileName())
if node != nil {
_, name := path.Split(file.FileName())

err = tree.AddNodeLink(name, node)
if err != nil {
return nil, err
err = tree.AddNodeLink(name, node)
if err != nil {
return nil, err
}
}
}

Expand All @@ -303,6 +352,23 @@ func addDir(n *core.IpfsNode, dir files.File, out chan interface{}, progress boo
return tree, nil
}

func checkForLocalIgnorePatterns(dir string, ignoreFilePatterns []ignore.GitIgnore) []ignore.GitIgnore {
Copy link
Member

Choose a reason for hiding this comment

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

may want to make this recursive (or really, iterative with a loop) so it walks up all the way to filesystem root.

var ignorePathname string
if dir == "." {
ignorePathname = ".ipfsignore"
} else {
ignorePathname = path.Join(dir, ".ipfsignore")
Copy link
Member

Choose a reason for hiding this comment

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

i believe this (second case) will always work, even with .? not sure.

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 think you are right, that line was written before I had the folder level recursion working correctly. The check inside of Run should now cover this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, just confirmed this locally over lunch. Removed on my local version.

}

localIgnore, ignoreErr := ignore.CompileIgnoreFile(ignorePathname)
if ignoreErr == nil && localIgnore != nil {
log.Debugf("found ignore file: %s", dir)
return append(ignoreFilePatterns, *localIgnore)
} else {
return ignoreFilePatterns
}
}

// outputDagnode sends dagnode info over the output channel
func outputDagnode(out chan interface{}, name string, dn *dag.Node) error {
o, err := getOutput(dn)
Expand All @@ -318,6 +384,22 @@ func outputDagnode(out chan interface{}, name string, dn *dag.Node) error {
return nil
}

type hiddenFileError struct {
fileName string
}

func (e *hiddenFileError) Error() string {
return fmt.Sprintf("%s is a hidden file", e.fileName)
}

type ignoreFileError struct {
fileName string
}

func (e *ignoreFileError) Error() string {
return fmt.Sprintf("%s is an ignored file", e.fileName)
}

type progressReader struct {
file files.File
out chan interface{}
Expand Down