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

Fix refs -r -u for #1211 #1525

Closed
wants to merge 1 commit into from
Closed

Fix refs -r -u for #1211 #1525

wants to merge 1 commit into from

Conversation

gatesvp
Copy link
Contributor

@gatesvp gatesvp commented Jul 28, 2015

License: MIT
Signed-off-by: Gaetan Voyer-Perrault gatesvp@gmail.com

License: MIT
Signed-off-by: Gaetan Voyer-Perrault <gatesvp@gmail.com>
@jbenet jbenet added the backlog label Jul 28, 2015
echo "4 refs_output" > expected &&
test_cmp line_count expected
'

Copy link
Member

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 

Copy link
Member

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

Copy link
Contributor Author

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'.

Copy link
Member

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 &&

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

@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 :) 👍

@gatesvp
Copy link
Contributor Author

gatesvp commented Jul 28, 2015

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 :)

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

@gatesvp sgtm

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

@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

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

(@gatesvp if you want push access to the repo so we can collab on same braches directly here, happy to give it too.)

@jbenet
Copy link
Member

jbenet commented Jul 28, 2015

Closed in favor of #1527 -- thanks @gatesvp

@jbenet jbenet closed this Jul 28, 2015
@jbenet jbenet removed the backlog label Jul 28, 2015
@jbenet jbenet modified the milestone: IPFS 0.3.6 Jul 28, 2015
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 this pull request may close these issues.

None yet

2 participants