Fix --hash-unmatched behavior #615
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
intelfx
force-pushed
the
work/fix-614
branch
5 times, most recently
from
77f32ed to
0daf500
Compare
intelfx
force-pushed
the
work/fix-614
branch
4 times, most recently
from
b0d836b to
a989ed2
Compare
The intention behind this criterion seems to handle partially-hashed (i. e. child) groups that do not fulfill requirements to continue hashing them (i. e. only contain a single file after sifting). Normally such groups are left alone (because we have hashed enough of the file to understand it is unique), however by definition of --hash-unmatched we have to hash these files to the end. The problem is that when rm_shred_group_update_status() is called, group->held_files does not contain the currently pushed file. Thus, because this condition is only executed for single-file groups (group->n_clusters == 1), group->held_files will never be non-empty. Anyway, even if it was non-empty, head->digest will always be NULL because held files have digests freed in rm_shred_group_push_file(). Therefore, this condition will never be fulfilled. The only reason why --hash-unmatched even works at all is due to the broken hardlinked files condition that matches all single-inode groups. (Another consequence is that --hash-unmatched is broken if --merge-directories is not used.)
`group->n_inodes == 1` is also true for groups that simply consist of a single file. This condition will cause all single-file groups to be hashed if `--merge-directories` is also set. Additionally, the whole `group->n_inodes == 1` condition is redundant because not following on the branch means that `group->n_clusters == 1` and therefore `group->n_inodes == 1`. Thus, check that we are actually dealing with a group of hardlinks (and not just a single-file group that did not cause an early return because we are also doing `--hash-unmatched`). Fixes sahib#614.
intelfx
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem with
--hash-unmatchedis twofold.First, the criterion in
rm_shred_group_update_status()that is responsible for hashing partially-hashed groups to the end is broken. It can never be fulfilled becausegroup->head_filesis always empty at this point, and even if it was not,head->digestis always NULL because that's whatrm_shred_group_push_file()does.The only reason why
--hash-unmatchedeven works is because the--merge-directoriescondition above is broken as well and it always fires for all single-file groups. (Consequently,--hash-unmatchedwithout--merge-directoriesis broken in develop.) However, the same bug also means that all unique files will always get hashed to the end if--merge-directoriesis set, thus making--hash-unmatchedbehave like--hash-uniques.This function only works as designed if neither
--hash-unmatchednor--hash-uniquesis set, because in this case all single-file groups get filtered early by therm_shred_group_qualifies()check.Fixes #614.