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: restore path on Windows #631

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

Nassivera
Copy link

@Nassivera Nassivera commented Jan 11, 2025

Fix restore path on Windows, issue #528

Now when you restore a path on Windows, the entire directory tree is not recreated but only the desired path.

@CLAassistant
Copy link

CLAassistant commented Jan 11, 2025

CLA assistant check
All committers have signed the CLA.

@garethgeorge garethgeorge changed the title Fix: restore path on Windows fix: restore path on Windows Jan 12, 2025
@garethgeorge
Copy link
Owner

Hey -- thanks for the interest in contributing! Making this consistent with the Linux behavior is a goal, appreciate you taking a look at it.

I'm seeing a windows test fail here: https://github.com/garethgeorge/backrest/blob/0dd360b4973b9f60ba706f869a1a6eb883713afd/internal/orchestrator/repo/repo_test.go#L136C1-L137C1

A few fixes might be needed for windows -- things that stand out to me is that this code was using path.Dir and path.Base , that might need to change to filepath.Dir and filepath.Base respectively which are the cross platform libraries.

There might be other issues with how the command is being crafted on windows, unfortunately it's truncated in the error message so it might need some debugging
image

@Nassivera Nassivera marked this pull request as draft January 15, 2025 10:14
@Nassivera Nassivera marked this pull request as ready for review January 25, 2025 15:34
if runtime.GOOS == "windows" {
opts = append(opts, restic.WithFlags("--include", snapshotPath))
if dir != "" {
Copy link
Owner

Choose a reason for hiding this comment

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

is this branch now the same as the branch for linux? It may be fine to combine them.

Copy link
Author

Choose a reason for hiding this comment

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

yes, it might be fine to combine them.

@garethgeorge
Copy link
Owner

Overall looks great, the path normalization makes sense to me. Thanks for digging into this / contributing a fix.

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.

3 participants