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 option to ignore files when adding. #4013

Closed
wants to merge 3 commits into from
Closed

Add option to ignore files when adding. #4013

wants to merge 3 commits into from

Conversation

crackcomm
Copy link

@crackcomm crackcomm commented Jun 27, 2017

Current usage:

  -i,          --ignore              array  - List of ignore rules. (comma separated).
  -g,          --git-ignore          bool   - Respect .gitignore rules. (experimental).

Fixes #3643
Includes #4012

Breaking changes: files.NewSerialFile

License: MIT
Signed-off-by: Łukasz Kurowski <crackcomm@gmail.com>
License: MIT
Signed-off-by: Łukasz Kurowski <crackcomm@gmail.com>
@kevina kevina self-requested a review June 27, 2017 04:19
}
rulesFile = filepath.Join(cwd, ".gitignore")
} else if err != nil {
return nil, nil, u.ErrCast()
Copy link
Member

Choose a reason for hiding this comment

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

i would just return the err here

Copy link
Author

Choose a reason for hiding this comment

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

again, thanks


ignore, _, err := req.Option("ignore").Strings()
if err != nil {
return nil, nil, u.ErrCast()
Copy link
Member

Choose a reason for hiding this comment

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

just return nil, nil, err

Copy link
Author

Choose a reason for hiding this comment

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

thanks


// Filter - Returns true if file should be filtered.
func (filter *Filter) Filter(fpath string) bool {
if filter.Hidden && strings.HasPrefix(fpath, ".") {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this would catch hidden files in recursive directories. We might want to use filepath.Base here to get just the filename, then check if it has the dot prefix.

Copy link
Author

Choose a reason for hiding this comment

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

Input to Filter is always a filename actually.

Copy link
Member

Choose a reason for hiding this comment

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

Oh? okay, then thats fine

Copy link
Author

Choose a reason for hiding this comment

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

I discovered it should be the opposite - Hidden true means include hidden files.

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

changes mostly look good to me (noted a few small things). I would love to see these options tested out in a sharness test, you can probably either add more to test/sharness/t0042-something.sh or create a new test file just for testing these changes.

return parseArgs(inputs, stdin, argDefs, recursive, hidden, root)

var rulesFile string
if gitignore, _, err := req.Option("git-ignore").Bool(); gitignore {
Copy link
Author

Choose a reason for hiding this comment

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

Here we need to check for nil's anyway.

@@ -334,7 +334,7 @@ func (r *request) ConvertOptions() error {
}

kind := reflect.TypeOf(v).Kind()
if kind != opt.Type() {
if kind != opt.Type() && opt.Type() != Strings && kind != String {
Copy link
Author

@crackcomm crackcomm Jun 27, 2017

Choose a reason for hiding this comment

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

Here it should be just kind != opt.Type() && opt.Type() != Strings otherwise it doesn't convert numbers.

License: MIT
Signed-off-by: Łukasz Kurowski <crackcomm@gmail.com>
@ghost
Copy link

ghost commented Jun 28, 2017

Haven't reviewed the code itself, but I'm wondering why not stick with the convention of a .fooignore file?

@crackcomm
Copy link
Author

@lgierth I have no idea what is that.

@crackcomm
Copy link
Author

@lgierth did you mean to include rules from all .*ignore files?

@ghost
Copy link

ghost commented Jul 6, 2017

Yeah let me clarify this a bit more, @whyrusleeping asked me to, too. Although the solution proposed looks okay on first sight, we haven't looked at any other options. What I'm missing here a bit is the evaluation and weighing of different ways of solving our need for having ipfs add ignore files.

Here's another one, off the top of my head, one with an implicit .ipfsignore file.

> touch README.md foo
> echo foo > .ipfsignore
> ipfs add -r .
info: skipping files as defined in ./.ipfsignore
added QmA README.md
added QmB .ipfsignore
added QmDir
> ipfs add -f -r .
info: adding otherwise ignored files as defined in ./.ipfsignore
added QmA README.md
added QmB .ipfsignore
added QmC foo
added QmDir2

This could be extended to also process an ipfsignore in $HOME/.ipfsignore, or generally ipfsignore files in parent directories.

Other options include an explicit ignore file (--ignorefile=path/to/ipfsignore), and there's likely good ideas to pick from rsync, git, and other tools that operate on files.

I'm not saying the solution proposed here is bad, only that it's just the first and that we should look at more.

@crackcomm
Copy link
Author

crackcomm commented Jul 17, 2017

I personally do not like the idea of additional .ipfsignore. Maybe the idea of .gitignore is even worst than .ipfsignore. Specifying it in the flag seems most viable option for this kind of application.

@crackcomm
Copy link
Author

Let me know if I can do anything to move this forward also #4013.

Copy link
Member

@Kubuxu Kubuxu left a comment

Choose a reason for hiding this comment

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

This depends on #4012 and #3856

(cleaning out my Review queue).

@jacobheun
Copy link
Contributor

Support has been added #3643 (comment)

@jacobheun jacobheun closed this Jun 19, 2020
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.

ipfs add ignore, .ipfsignore
4 participants