path/resolver: Fix recursive path resolution #1212
Conversation
|
@wking thanks for fixing. Nasty bug. Yeah we need a test, we should probably include it in this PR. |
wking
force-pushed
the
fix-recursive-path-resolution
branch
from
03db594 to
31b25c2
Compare
|
On Fri, May 08, 2015 at 04:18:31PM -0700, Juan Batiz-Benet wrote:
Do you know of any two+ layer trees in the example data that could be |
wking
force-pushed
the
fix-recursive-path-resolution
branch
from
31b25c2 to
10669e8
Compare
I'm not entirely clear on Go's scoping (there's some text I can't quite parse here [1]), but it seems like the := version (because this is the first time we use 'err') was masking the function-level 'nd' just for this if block. That means that after we get out of the if block and return to the start of the for-loop for the next pass, nd.Links would still be pointing at the original object's links. This commit drops the :=, which fixes the earlier: $ ipfs ls QmXX7YRpU7nNBKfw75VG7Y1c3GwpSAGHRev67XVPgZFv9R/static/css Error: no link named "css" under QmXX7YRpU7nNBKfw75VG7Y1c3GwpSAGHRev67XVPgZFv9R so we get the intended: $ ipfs ls QmXX7YRpU7nNBKfw75VG7Y1c3GwpSAGHRev67XVPgZFv9R/static/css Qme4r3eA4h1revFBgCEv1HF1U7sLL4vvAyzRLWJhCFhwg2 7051 style.css It also means we're probably missing (or are unreliably using) a multi-level-path-resolving test. [1]: https://golang.org/ref/spec#Declarations_and_scope
|
Probably create it (see the pinning test for example) |
Setup a three-level graph: a -(child)-> b -(grandchild)-> c and then try and resolve: /ipfs/<hash-of-a>/child/grandchild Before 10669e8 (path/resolver: Fix recursive path resolution, 2015-05-08) this failed with: resolver_test.go:71: no link named "grandchild" under QmSomeRandomHash The boilerplate for this test is from pin/pin_test.go, and I make no claims that it's the best way to setup the test graph ;).
|
On Fri, May 08, 2015 at 04:30:54PM -0700, Juan Batiz-Benet wrote:
Tests pushed, and I've confirmed that they fail on cd37b67 (the The tests seem to have a high boilerplate-to-meat ratio, so if anyone |
| @@ -122,7 +122,8 @@ func (s *Resolver) ResolveLinks(ctx context.Context, ndd *merkledag.Node, names | |||
| // fetch object for link and assign to nd | |||
| ctx, cancel := context.WithTimeout(ctx, time.Minute) | |||
| defer cancel() | |||
| nd, err := s.DAG.Get(ctx, next) | |||
| var err error | |||
There was a problem hiding this comment.
this change seems unnecessary, but harmless i guess
There was a problem hiding this comment.
Ah, wait, i see now. tricky.
|
LGTM, RFM |
|
this LGTM. is think some of the sharness tests should exercise this too -- in particular |
path/resolver: Fix recursive path resolution
I'm not entirely clear on Go's scoping (there's some text I can't
quite parse here), but it seems like the := version (because this
is the first time we use 'err') was masking the function-level 'nd'
just for this if block. That means that after we get out of the if
block and return to the start of the for-loop for the next pass,
nd.Links would still be pointing at the original object's links.
This commit drops the :=, which fixes the earlier:
so we get the intended:
It also means we're probably missing (or are unreliably using) a
multi-level-path-resolving test.