-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/a4
then 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
< QmSCXf8UwPR7uRUssz6GPLZ5VT8F9kH1nCQJkbdbY1TUr2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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