Daemon panics if no path is given

[travisperson]

If no path after /ipfs/ or /ipns/ is given, then the daemon will
panic with a slice bounds out of range error. This checks to see if we
have anything after ipfs or ipns.

[whyrusleeping]

good catch!

[travisperson]

It was actually already there, just removed 13 days ago.

3ead244#diff-f666ab4b28c2e912b2339140db6061eeL50

Might put back in that same logic?

It looks like a pretty complex commit and parts of it might of been removed for a particular reason. The length check needs to be there someplace though I believe still.

[jbenet]

paging @wking and @cryptix -- who touched this part last

[wking]

On Wed, May 20, 2015 at 07:12:43PM -0700, Travis Person wrote:

It was actually already there, just removed 13 days ago.

3ead244#diff-f666ab4b28c2e912b2339140db6061eeL50

Might put back in that same logic?

Oops, sorry about that. And maybe a test to make sure I don't break
this again in a future refactor?

[travisperson]

@wking is adding back these lines alright, or should it be modified?

if len(seg) < 2 || seg[1] == "" { // just "/ipns/"
    return nil, fmt.Errorf("invalid path: %s", string(r.p))
}

[wking]

On Wed, May 20, 2015 at 07:55:10PM -0700, Travis Person wrote:

@wking is adding back these lines alright, or should it be modified?

if len(seg) < 2 || seg[1] == "" { // just "/ipns/"
  return nil, fmt.Errorf("invalid path: %s", string(r.p))
}

Well, r isn't defined anymore, now that resolveRecurse is gone. You
should just use the p from Resolve's arguments. And you'll also
want to restore the "fmt" import. And perhaps the comment should be:

// just "//" without further segments

because the problem isn't restricted to /ipns/ (e.g. it could be
‘/ipfs/’ or in the future maybe ‘/my-wonky-protocol/’).

[jbenet]

this LGTM. will merge after tests

[travisperson]

I agree, I enjoy named errors. We loose the formatting with named errors though. I'm not sure if it's all that important in this case though and am 100% okay with creating a new error instead.

[wking]

On Wed, May 20, 2015 at 09:18:30PM -0700, Travis Person wrote:

I agree, I enjoy named errors. We loose the formatting with named
errors though.

Is there a way around that? I'm not fluent in Go yet, but the idea
behind “does the string form of this error start with …?” seems
straightforward enough. Is there an idiomatic way to ask that? Or is
that not the sort of thing that people do?

[travisperson]

At the very least we could do this:

if !strings.HasPrefix(err.Error(), "invalid path:") {

Which is probably a good idea, as the test I wrote actually fails on core/resolve: no Namesys on IpfsNode - can't resolve ipns entry

@jbenet hold off on merging this.

[jbenet]

(aside: error handling is clunky in general. feels unfinished.)

[jbenet]

@jbenet hold off on merging this.

ack

[travisperson]

As long as there are no further comments I think this is good to go.

[whyrusleeping]

@travisperson i fixed the namesys issue you were running into.

[jbenet]

thanks @travisperson

[jbenet]

@travisperson looks like this PR made osx go tests fail here:

• https://travis-ci.org/ipfs/go-ipfs/jobs/63627091
• https://travis-ci.org/ipfs/go-ipfs/builds/63627799

so i reverted :(

previous merge head was 78defff

Also noted commits are behind master. maybe rebase, PR again and see if it does indeed fail? (i don't want to keep introducing intermittent testing failures)

[whyrusleeping]

the tests passed fine when it merged, and this isnt a thing that can be intermittent.