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

lib/model: More precise deletion detection (fixes #2571, fixes #4573) #4765

Merged
merged 3 commits into from Feb 25, 2018

Conversation

imsodin
Copy link
Member

@imsodin imsodin commented Feb 14, 2018

This fixes the same issues as #4584 but without the combined db/fs scan. The basic idea is to only mark files as deleted, if we really are sure they are deleted. I.e. only if the lstat error is IsNotExist or a parent is a link/file.

Copy link
Member

@calmh calmh left a comment

Choose a reason for hiding this comment

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

LGTM apart from the missing dropothers

@@ -2066,6 +2064,14 @@ func (m *Model) internalScanFolderSubdirs(ctx context.Context, folder string, su
Deleted: true,
Version: f.Version.Update(m.shortID),
}
// We do not want to override the global version
Copy link
Member

Choose a reason for hiding this comment

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

Move the comment inside the if, I was super confused about why we would not want to override the global file until I read on and realized it was for a corner case.

It's also confusing that we are inside an !f.IsInvalid() case and yet it may be invalid, but I guess that's Ok...

Copy link
Member Author

Choose a reason for hiding this comment

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

I dropped the if entirely from this PR. That is the change of behaviour introduced in #4750 and thus should be done there (didn't realize it when splitting this PR from the other one).

@imsodin
Copy link
Member Author

imsodin commented Feb 24, 2018

I just run this PR together with the send-only metadata PR on my system and it ran smoothly.

Is this (still) good to go?

@calmh calmh merged commit 5822222 into syncthing:master Feb 25, 2018
@imsodin imsodin deleted the delImp branch February 25, 2018 09:54
imsodin added a commit to imsodin/syncthing that referenced this pull request Feb 25, 2018
calmh pushed a commit that referenced this pull request Feb 25, 2018
calmh added a commit to calmh/syncthing that referenced this pull request Mar 11, 2018
* master: (31 commits)
  cmd/syncthing, lib/db: Be nicer about dropping deltas on upgrade (syncthing#4798)
  gui, man: Update docs & translations
  cmd/stdiscosrv: Record time of failed lookup
  cmd/stdiscosrv: Expose process metrics
  test: Update conflict tests
  gui: In remote need use index and auto-expand if only one folder (fixes syncthing#4759) (syncthing#4781)
  gui, man: Update docs & translations
  cmd/syncthing: Reset delta indexes on upgrade
  lib/connections: Fix relay connections when two devices use the same relay (fixes syncthing#4778) (syncthing#4779)
  lib/scanner: Track modified by in symlinks (syncthing#4777)
  lib/model: Mark deleted file as conflicting when un-ignoring (syncthing#4776)
  lib/config, lib/model: Auto adjust pullers based on desired amount of pending data (syncthing#4748)
  lib: Handle metadata changes for send-only folders (fixes syncthing#4616, fixes syncthing#4627) (syncthing#4750)
  lib/model: More precise deletion detection (fixes syncthing#2571, fixes syncthing#4573) (syncthing#4765)
  lib/model: Don't panic if block size in index is larger than protocol block size
  all: Fix typos (syncthing#4772)
  cmd/strelaysrv: Don't patch the default HTTP client (fixes syncthing#4745)
  cmd/strelaypoolsrv: Return better error codes and messages (syncthing#4770)
  lib/fs, vendor: s/zillode/Zillode/
  cmd/syncthing: Fix help text for STRECHECKDBEVERY (fixes syncthing#4764)
  ...
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Feb 26, 2019
@syncthing syncthing locked and limited conversation to collaborators Feb 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants