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

Replace String with PathBuf for paths #59

Merged
merged 9 commits into from
Oct 14, 2020
Merged

Conversation

darksv
Copy link
Contributor

@darksv darksv commented Oct 10, 2020

This PR changes handling of paths so that String is replaced with PathBuf. The latter is more suitable for this purpose as it is cross-platform and is not required to be valid UTF8. Also provides nice convenience methods like extension or join that correctly handles separators on various platforms.

@qarmin
Copy link
Owner

qarmin commented Oct 10, 2020

Changes looks mostly good.

Seems that it even fixes not excluding folders like .git by default

Also probably this is better fix to #44 than 3b63798

The only thing for now which is broken is saving to file, because it prints records with one empty line between them

---- Size 133.40 KiB (136606) - 112 files 
/home/rafal/Pobrane/home/runner/work/czkawka/czkawka/gtk_app/share/icons/Adwaita/cursors/dotbox.cur 

/home/rafal/Pobrane/home/runner/work/czkawka/czkawka/gtk_app/share/icons/Adwaita/cursors/cell.cur 

/home/rafal/Pobrane/home/runner/work/czkawka/czkawka/gtk_app/share/icons/Adwaita/cursors/row-resize.cur 

/home/rafal/Pobrane/home/runner/work/czkawka/czkawka/gtk_app/share/icons/Adwaita/cursors/ns-resize.cur 

/home/rafal/Pobrane/home/runner/work/czkawka/czkawka/gtk_app/share/icons/Adwaita/cursors/size_hor.cur 

/home/rafal/Pobrane/home/runner/work/czkawka/czkawka/gtk_app/share/icons/Adwaita/cursors/circle.cur 

@qarmin qarmin added bug Something isn't working enhancement New feature or request labels Oct 10, 2020
@qarmin
Copy link
Owner

qarmin commented Oct 12, 2020

Windows excluded directories are broken
Sample project - Z.zip

Executing command .\czkawka_cli.exe dup -d "C:\Z" "c:/z" -e "c:/Z/Q"
finds duplicates from folder C:/z/q which should be

Found 88 duplicated files in 1 groups with same content which took 12.77 MiB:
Size - 150.29 KiB (153901) - 88 files
C:/z/fffffffffffffffffffffffffffffffffffffffffffffffffffff
C:/z/fffffffffffffffffffffffffffffffffffffffffffffffffffff (10. kopia)
C:/z/q/fffffffffffffffffffffffffffffffffffffffffffffffffffff (31. kopia)
C:/z/q/fffffffffffffffffffffffffffffffffffffffffffffffffffff (32. kopia)

On Linux ./czkawka_cli dup -d /home/rafal/Pulpit/Z /home/rafal/Pulpit/Z /home/rafal/Pulpit/Z -e /home/rafal/Pulpit/Z/Q
works as expected

for expression in &self.excluded_items.items {
if Common::regex_check(expression, &current_file_name) {
continue 'dir;
}
}

#[cfg(target_family = "windows")]
{
if cfg!(target_family = "windows") {
current_file_name = Common::prettier_windows_path(&current_file_name);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier_windows_path function was used only for normalizing path like c:/Av, c:\AV, C:/av to C:/av to allow easily remove duplicate path or overlap ones.
With PathBuf I don't think that this is necessary, and can be removed(of course if everything else will work fine)

@qarmin
Copy link
Owner

qarmin commented Oct 13, 2020

Pathbuf doesn't recognize that on Windows

"c:/z\\Q" is equal to :"c:/z/q"

and the only solution for me is to check every single time when using excluding directories if string path converted to lowerspace are equal

@darksv
Copy link
Contributor Author

darksv commented Oct 13, 2020

Right, I'll fix this when I got some time.

@qarmin
Copy link
Owner

qarmin commented Oct 13, 2020

You can try to rebase to current master, because I added recently CI job with checking simple regressions and it probably will allow you to test PR without needing using Windows or Wine(The longest job takes 6/7 minutes).

@darksv
Copy link
Contributor Author

darksv commented Oct 14, 2020

I have extracted some duplicated code related to exclusion into a separated methods. Please check whether it's OK.

@darksv darksv requested a review from qarmin October 14, 2020 15:14
@qarmin qarmin merged commit acfecd7 into qarmin:master Oct 14, 2020
@qarmin
Copy link
Owner

qarmin commented Oct 14, 2020

Seems that everything works now.
Thanks, especially for simplifying code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants