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

Examine file ctime when checking if files have changed. #2212

Merged
merged 4 commits into from Apr 25, 2019

Conversation

@cbane
Copy link

cbane commented Mar 20, 2019

What is the purpose of this change? What does it change?

This makes restic compare file ctimes (in addition to all of the existing checks) when it is deciding if a file has been modified.

Was the change discussed in an issue or in the forum before?

Yes, closes #2179.

Checklist

  • I have read the Contribution Guidelines
  • I have added tests for all changes in this PR
  • I have added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here)
  • 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
@cbane

This comment has been minimized.

Copy link
Author

cbane commented Mar 20, 2019

It looks like this change is breaking #2205 (or at least its unit tests). How do we want these to interact? The only idea I've had so far is disabling the ctime check if the ignore inode flag is on, but I'm open to anything that makes sense.

@d3zd3z

This comment has been minimized.

Copy link
Contributor

d3zd3z commented Apr 19, 2019

The only idea I've had so far is disabling the ctime check if the ignore inode flag is on, but I'm open to anything that makes sense.

The ctime value really only makes sense on the exact same inode, so this seems to me like a reasonable way of handling this.

Copy link
Member

fd0 left a comment

Thanks for taking a stab at this! This is a change which may leave users wondering what's happening, so we need to be extra careful to address this in the changelog. As far as I understood, there are some situations in which restic did not read the contents of the file, but does now (file with different content, but same length and mtime and inode). So for people depending on this (there are always people who depend on any behavior, no matter how obscure) will see that restic reads and hashes more files than before. Also, when metadata is changed, the ctime is updated and the file is re-read.

In my opinion, being correct and re-reading the contents if in doubt is the better strategy, so I'm in favor for this. Maybe we should add a paragraph to the changelog that people should open an issue if this breaks anything for them, so we can decide if we need a flag for it?

We need to clarify the following things before it can be merged:

  • Is it a bug or an enhancement (important for the changelog entry)? I'm currently leaning towards the latter, but I'm open to arguments.
  • The code needs to be rebased on master, the conflict with the ignore-inode option needs to be resolved
  • When --ignore-inode is turned on, I think the ctime should not be considered so I agree with @d3zd3z
@cbane cbane force-pushed the cbane:check-ctime branch from 3e98d90 to 00b527f Apr 25, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 25, 2019

Codecov Report

Merging #2212 into master will decrease coverage by 4.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2212      +/-   ##
==========================================
- Coverage   50.92%   46.86%   -4.06%     
==========================================
  Files         178      178              
  Lines       14530    14534       +4     
==========================================
- Hits         7399     6812     -587     
- Misses       6058     6700     +642     
+ Partials     1073     1022      -51
Impacted Files Coverage Δ
internal/fs/stat.go 50% <ø> (ø) ⬆️
internal/archiver/archiver.go 68.78% <100%> (+0.15%) ⬆️
internal/fs/stat_unix.go 89.47% <100%> (+0.58%) ⬆️
internal/fs/stat_bsd.go 89.47% <100%> (+0.58%) ⬆️
internal/backend/b2/b2.go 0% <0%> (-80.69%) ⬇️
internal/backend/swift/swift.go 0% <0%> (-78.83%) ⬇️
internal/backend/gs/gs.go 0% <0%> (-74%) ⬇️
internal/backend/azure/azure.go 0% <0%> (-69.46%) ⬇️
internal/backend/swift/config.go 34.69% <0%> (-57.15%) ⬇️
internal/archiver/file_saver.go 84.21% <0%> (-4.39%) ⬇️
... and 2 more

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 fad9f65...00b527f. Read the comment docs.

@cbane

This comment has been minimized.

Copy link
Author

cbane commented Apr 25, 2019

OK, I rebased against master, and it now skips the ctime check when the --ignore-inode option is given. I also updated the text for the changelog entry.

@fd0
fd0 approved these changes Apr 25, 2019
Copy link
Member

fd0 left a comment

LGTM, thanks!

file should be noticed, and the modified file will be backed up. The ctime check
will be disabled if the --ignore-inode flag was given.

If this change causes problems for you, please open an issue, and we can look in

This comment has been minimized.

Copy link
@fd0

fd0 Apr 25, 2019

Member

I love the wording!

@@ -576,6 +577,19 @@ func TestFileChanged(t *testing.T) {
save(t, filename, defaultContent)
},
},
{
Name: "new-content-same-timestamp",

This comment has been minimized.

Copy link
@fd0

fd0 Apr 25, 2019

Member

Table-driven tests are awesome, easily extendable for contributors! :)

@fd0 fd0 merged commit 00b527f into restic:master Apr 25, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
fd0 added a commit that referenced this pull request Apr 25, 2019
Examine file ctime when checking if files have changed.
@cbane cbane deleted the cbane:check-ctime branch Apr 25, 2019
@fd0 fd0 mentioned this pull request May 5, 2019
7 of 7 tasks complete
@micw

This comment has been minimized.

Copy link

micw commented on 35b7607 May 20, 2019

Hello, what is the reason for this implicit behaviour? IMO this should be either done by an extra command line option or at least be documented.

@DurvalMenezes

This comment has been minimized.

Copy link

DurvalMenezes commented May 24, 2019

Please excuse me, but shouldn't we be giving more warning to users about this problem? Please see my rationale here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.