add: do not verify hardlink if file is empty

[skshetry]

Fixes #3390

• ❗ Have you followed the guidelines in the Contributing to DVC list?

• 📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

• ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

[codecov]

Codecov Report

Merging #3428 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3428   +/-   ##
=======================================
  Coverage   93.08%   93.08%           
=======================================
  Files         140      140           
  Lines        8515     8515           
=======================================
  Hits         7926     7926           
  Misses        589      589           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05cc023...4e9289f. Read the comment docs.

[gurobokum]

What do you think to fix the issue another way:

1. in _verify_link check the case when the file is not a link and has size 0 bytes
2. if it's True - just don't raise an Exception
3. put the comment with the issue and short description about the case

It allows to fix it in one place without passing inconsistent return value

UPD
I see @pared propsed the same in the comment above

[skshetry]

We (@efiop and I) discussed this on 1o1. A better way to fix this would be to try to create a temporary file to check if the System supports {ref,hard,sym}link. Though, it'll need some refactor, so, I'll create an issue for working on this later.

[efiop]

Not great that it will be checking the size for each link it creates, might get expensive. Though, it does that in hardlink anyway...

[skshetry]

We did that anyway on the hardlink() though.

[efiop]

@skshetry Maybe there is some nicer way to do this? Like making hardlink(and other link class methods) verify themselves? This is an honest question, I don't know myself either 🙂

[efiop]

@skshetry Could keep it as is and create a ticket for it to reconsider later. Just asking.

[skshetry]

I like this suggestion #3428 (comment) more. Running verification on hardlink itself for only once (by setting self.cache_type_confirmed) is also hackish.

I'd say, we revisit this on next sprint and fix it?

[efiop]

@skshetry Ok, please create an issue and add it to the next sprint.

[pared]

We could just check for cache_type_verified at the beggining again. I know that is duplication, but don't see any obvious workaround.

[efiop]

@skshetry Check your tests, travis failed.

[pared]

So, the conclusion for this issue is that we will crete issue to fix the way we handle confirmation of cache type?

[pared]

One additional comment in discussion with @efiop, not anything major.

[efiop]

@skshetry Please check the tests.