Skip to content

Update snapshot summary on rewrite#4905

Merged
MichaelEischer merged 1 commit intorestic:masterfrom
alex-kattathra-johnson:issue-4902
Jul 16, 2024
Merged

Update snapshot summary on rewrite#4905
MichaelEischer merged 1 commit intorestic:masterfrom
alex-kattathra-johnson:issue-4902

Conversation

@alex-kattathra-johnson
Copy link
Copy Markdown
Contributor

@alex-kattathra-johnson alex-kattathra-johnson commented Jul 6, 2024 •
edited by MichaelEischer
Loading

What does this PR change? What problem does it solve?

The snapshot summary doesn't reflect the files that are tracked after a rewrite

Was the change previously discussed in an issue or on the forum?

Fixes #4902

Checklist

  • I have read the contribution guidelines.
  • I have enabled maintainer edits.
  • I have added tests for all code changes.
  • I have added documentation for relevant changes (in the manual).
  • There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
  • I have run gofmt on the code in all commits.
  • All commit messages are formatted in the same style as the other commits in the repo.
  • I'm done! This pull request is ready for review.

@alex-kattathra-johnson alex-kattathra-johnson force-pushed the issue-4902 branch 3 times, most recently from fd5a092 to 0a2220c Compare July 8, 2024 01:13
MichaelEischer
MichaelEischer requested changes Jul 8, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

Thanks for giving this a try. Recalculating the snapshot size is definitely useful. However, the calculation itself does not belong into cmd_rewrite.go, see comments below for more details.

@alex-kattathra-johnson alex-kattathra-johnson force-pushed the issue-4902 branch 2 times, most recently from a9b9561 to 2874f8f Compare July 8, 2024 19:19
@alex-kattathra-johnson
Copy link
Copy Markdown
Contributor Author

alex-kattathra-johnson commented Jul 8, 2024

@MichaelEischer : Thanks for the feedback! Updated the PR to make the change to the summary only if it's non-nil and moved the update to the RewriteNode callback function.

MichaelEischer
MichaelEischer reviewed Jul 10, 2024
View reviewed changes
@alex-kattathra-johnson alex-kattathra-johnson force-pushed the issue-4902 branch 5 times, most recently from 34783a8 to b0905a4 Compare July 15, 2024 18:43
MichaelEischer
MichaelEischer reviewed Jul 15, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

Thanks for the changes so far. I have only a few small nitpicks left, see below.

I'd also like to have a minimal functionality test in cmd/restic/cmd_rewrite_integration_test.go, maybe by extending TestRewriteReplace to check the snapshot's stats before and after rewriting. I can take care of that if you want.

@alex-kattathra-johnson alex-kattathra-johnson force-pushed the issue-4902 branch 3 times, most recently from 316c380 to 210c2e9 Compare July 15, 2024 20:13
@alex-kattathra-johnson alex-kattathra-johnson force-pushed the issue-4902 branch 2 times, most recently from 0a7b905 to 1259449 Compare July 16, 2024 13:46
Update snapshot summary on rewrite
3bf2927 
Signed-off-by: Alex Johnson <hello@alex-johnson.net>
MichaelEischer
MichaelEischer approved these changes Jul 16, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@MichaelEischer MichaelEischer enabled auto-merge July 16, 2024 18:04
@MichaelEischer MichaelEischer added this pull request to the merge queue Jul 16, 2024
Merged via the queue into restic:master with commit 4f4598a Jul 16, 2024
@alex-kattathra-johnson alex-kattathra-johnson deleted the issue-4902 branch July 16, 2024 18:26
@BrewTestBot BrewTestBot mentioned this pull request Jul 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MichaelEischer MichaelEischer MichaelEischer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rewrite copies "Size" metadata instead of recalculating it if files have been excluded

2 participants

@alex-kattathra-johnson @MichaelEischer