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

ipfs add --help is broken #373

Closed
chriscool opened this issue Nov 22, 2014 · 8 comments · Fixed by #378
Closed

ipfs add --help is broken #373

chriscool opened this issue Nov 22, 2014 · 8 comments · Fixed by #378

Comments

@chriscool
Copy link
Contributor

Right now on master () I get:

$ ipfs add --help
Error: Argument 'path' is required

USAGE:

    ipfs add <path>... - Add an object to ipfs.

    Adds contents of <path> to ipfs. Use -r to add directories.
    Note that directories are added recursively, to form the ipfs
    MerkleDAG. A smarter partial add with a staging area (like git)
    remains to be implemented.

Use 'ipfs add --help' for more information about this command.

It is not so easy to find where this problem appeared because ipfs2 became ipfs, but I think that the problem comes from the merge of PR #357.

It doesn't work with ipfs2 at:

105f232 (HEAD) Merge pull request #357 from jbenet/multifile

It used to work with ipfs2 at:

1c36d52 Merge pull request #359 from jbenet/feat/swarm-cmd

I tried to bisect between the above two commits but I get too many compilation errors on Linux.
I had to use git bisect skip a lot to try to overcome that, but in the end I get:

There are only 'skip'ped commits left to test.
The first bad commit could be any of:
5614234d1d77952f9a7e7b71235e3261d199e113
a9d784cb21c8788a1e65e242d83067229a509c82
c598673b4cda659038209a076881556d1c384f96
bfdf7951b39f238e09d49f6d86aecf36d27bf395
c9abc6b546c11d94f3d21a0447ec2d6f70dc3833
380337b76b24751af4b512646939760717a8322c
8900229579ff244c062ceb875aa528f0c3530049
015bd06cffe4003773f3bafaf01f1198055accca
bde015616f30482bb661a3051ba8845cf6a4db0f
0b509098aa39d8d8acdca75e8c08409d22c8ddfe
c904e6c46da68bf9470eb3e1891465f58df77ac0
c14bd98f7a2057977f3a72b140c498637e5a8df5
d4ac442838aec986891b7efc5e6f6f11ae5df407
6681c5037114b99a4c122ef498c51e4be295df7d
ce49541f130e736639ff270ad0132245f2938ee8
57c48adfcff1237deafb2188db69e507dc41097f
9333c504c12b76d1577e478e5e013b37e211ae7f
bc8a97c119393060b242605ec05ec770b68825ba
c19bdf18b2044a254d0fc490c85ce26ca89e1790
8968b98cf356625440e44a24a4154a55795314f1
d1f1d2f5bf2adcc726856de0c681b24ba8fc6651
0709621a2c8436afa90c80e147985cc05723a73e
f8be26810ae1359f10f263b461d8bdba61daf56e
a0bd29d5beb34ea7e23736525a5050525365f0e6
We cannot bisect more!
@chriscool
Copy link
Contributor Author

By the way, it would be nice if we had some automated tests that check that all the commits in a PR pass the tests, before merging the PR.

It can be easily tested using: GIT_EDITOR=true git rebase -i --exec "make test" master

I know it would take some time and computing resources (see issue #283), but we would avoid problems like this.

@jbenet
Copy link
Member

jbenet commented Nov 22, 2014

Hmm @mappum can you take a look at this? Looks like its coming from incorrect arg parsing.

I get too many compilation errors on Linux.

By the way, it would be nice if we had some automated tests that check that all the commits in a PR pass the tests, before merging the PR.

Yes, I entirely agree. I recently had fun with a similar situation -- i ended up rebasing just to fix the compile errors before I could find the bug. I'd like to transition towards make sure all commits compile + pass tests.

Thoughts @whyrusleeping @maybebtc ?

@jbenet
Copy link
Member

jbenet commented Nov 22, 2014

I'll also make sure to note this in the contributing doc.

@btc
Copy link
Contributor

btc commented Nov 22, 2014

Yes, I entirely agree. I recently had fun with a similar situation -- i ended up rebasing just to fix the compile errors before I could find the bug. I'd like to transition towards make sure all commits compile + pass tests.

It's a perfectly reasonable proposal. All it requires is a little extra caution when making commits and/or rebasing to change history. I can make sure my contribs conform.

@chriscool
Copy link
Contributor Author

I rebased to fix the compile errors and it looks like the first bad commit is:

ae33905 commands: Added global -r/--recursive flag

It contains the following:

commit ae33905ccd9b51d72c5ae9075ccdf4fa5315cb10
Author: Matt Bell <mappum@gmail.com>
Date:   Sun Nov 16 19:13:11 2014 -0800

    commands: Added global -r/--recursive flag

diff --git a/commands/option.go b/commands/option.go
index 3a679d9..d4bd75e 100644
--- a/commands/option.go
+++ b/commands/option.go
@@ -136,12 +136,14 @@ func (ov OptionValue) String() (value string, found bool, err error) {
 const (
        EncShort = "enc"
        EncLong  = "encoding"
+       RecShort = "r"
+       RecLong  = "recursive"
 )

 // options that are used by this package
 var globalOptions = []Option{
-       Option{[]string{EncShort, EncLong}, String,
-               "The encoding type the output should be encoded with (json, xml, or text)"},
+       StringOption(EncShort, EncLong, "The encoding type the output should be encoded with (json, xml, or text)"),
+       BoolOption(RecShort, RecLong, "Add directory paths recursively"),
 }

 // the above array of Options, wrapped in a Command

@dborzov
Copy link
Contributor

dborzov commented Nov 23, 2014

Check out the fix: #378

@chriscool
Copy link
Contributor Author

Yeah, your fix works for me! Thanks!

@jbenet
Copy link
Member

jbenet commented Nov 23, 2014

#378 should have fixed this issue. pls reopen if not. thanks to @chriscool + @dborzov !

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 a pull request may close this issue.

4 participants