Fix refs -r -u for #1211 #1525
Fix refs -r -u for #1211 #1525
Conversation
License: MIT Signed-off-by: Gaetan Voyer-Perrault <gatesvp@gmail.com>
| echo "4 refs_output" > expected && | ||
| test_cmp line_count expected | ||
| ' | ||
|
|
There was a problem hiding this comment.
maybe we could have 1 more test with a larger hierarchy? i'm always skeptical of small tests. maybe:
mkdir -p a/b/c/d &&
echo "content1" >a/f &&
echo "content1" >a/b/f1 &&
echo "content1" >a/b/c/f2 &&
echo "content2" >a/b/f2 &&
echo "content2" >a/b/c/f1 &&
echo "content2" >a/b/c/d/f &&
cp -r a a2 && mv a2 a/a2 &&
cp -r a a3 && mv a3 a/a3 &&
cp -r a a4 && mv a4 a/a4then can test with things like:
ipfs refs -r $root >all
ipfs refs -r -u $root >unique
# and
ipfs refs -r -u $root >unique_actual_sorted &&
ipfs refs -r $root | sort | uniq >unique_expected &&
test_cmp unique_expected unique_actual_sorted There was a problem hiding this comment.
and, yes. the current code (master, not your PR) is wrong:
jbenet @ lorien : /tmp % ipfs add -r a | tail -n1
added QmSN9gHTBs4MDKKD6wyTsgVGepRw1fCKN27PpneZ1oyjWB a
jbenet @ lorien : /tmp % ipfs refs -r -u QmSN9gHTBs4MDKKD6wyTsgVGepRw1fCKN27PpneZ1oyjWB
QmXbALpzZ5Ghu6Lccyj6esgEmwvhsXDY198TmQiZe7W6UX
QmXyTNoorD7YLJfwxjxA4jvpQxscBDtptrrcN71JSGrDYH
QmY7RxwyFwZ8zGXYST8RGq3A6AuNFxNEmCbDx61D4qCWjR
Qmd8hgEkeP3frTWPMwmdCFYep7VeM2B7bU2b23j5vBCBX5
QmadnaxhXBa27sTesQaZaCmGFoCXJ9qmUN4LunBqQTWqaY
jbenet @ lorien : /tmp % ipfs refs -r QmSN9gHTBs4MDKKD6wyTsgVGepRw1fCKN27PpneZ1oyjWB | sort | uniq
QmNg9AHX8fVfwqtxvMUWEvsvPvvT46KpC82VdDpW3pLMwQ
QmNxsbYDQssNZ6GQTXwihvoqE9Qs7u7SuHHbGCasGRXwAm
QmSCXf8UwPR7uRUssz6GPLZ5VT8F9kH1nCQJkbdbY1TUr2
QmXbALpzZ5Ghu6Lccyj6esgEmwvhsXDY198TmQiZe7W6UX
QmXyTNoorD7YLJfwxjxA4jvpQxscBDtptrrcN71JSGrDYH
QmY7RxwyFwZ8zGXYST8RGq3A6AuNFxNEmCbDx61D4qCWjR
QmadnaxhXBa27sTesQaZaCmGFoCXJ9qmUN4LunBqQTWqaY
Qmd8hgEkeP3frTWPMwmdCFYep7VeM2B7bU2b23j5vBCBX5
jbenet @ lorien : /tmp % ipfs refs -r -u QmSN9gHTBs4MDKKD6wyTsgVGepRw1fCKN27PpneZ1oyjWB | sort >actual
jbenet @ lorien : /tmp % ipfs refs -r QmSN9gHTBs4MDKKD6wyTsgVGepRw1fCKN27PpneZ1oyjWB | sort | uniq >expected
jbenet @ lorien : /tmp % diff expected actual
1,3d0
< QmNg9AHX8fVfwqtxvMUWEvsvPvvT46KpC82VdDpW3pLMwQ
< QmNxsbYDQssNZ6GQTXwihvoqE9Qs7u7SuHHbGCasGRXwAm
< QmSCXf8UwPR7uRUssz6GPLZ5VT8F9kH1nCQJkbdbY1TUr2There was a problem hiding this comment.
Hey I could absolutely add more tests. This was just the bare minimum to ensure that the ticket was actually closeable :)
That stated, I don't think this line will work ipfs refs -r -u $root >unique_actual_sorted && because refs -r -u does not actually sort. It drills in to the structure and outputs as it goes. This was actually the bug, the code was escaping early on a duplicate because it assumed the duplicate was 'folder' instead of a 'file'.
There was a problem hiding this comment.
@gatesvp ah yeah, in my example above i did the correct:
ipfs refs -r -u $root | sort >actual &&
|
@gatesvp LGTM. im ok to merge this, though i'll probably write another test on top if so. if you have time, go for it, else I can merge now :) 👍 |
|
I am in now way offended if there are more tests. I like the tests proposed above with the duplicate trees. Given that you've already written it, it may be easier for you to just add yours on top :) |
|
@gatesvp sgtm |
|
@gatesvp: i may just grab this branch and do another PR, because there seems to be an error on osx: https://travis-ci.org/ipfs/go-ipfs/jobs/72960389#L2643 |
|
(@gatesvp if you want push access to the repo so we can collab on same braches directly here, happy to give it too.) |
License: MIT
Signed-off-by: Gaetan Voyer-Perrault gatesvp@gmail.com