Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11445030:
Add sync of parent directory when creating or renaming a file. (Closed)
|
Created:
8 years ago by ericu Modified:
7 years, 6 months ago Reviewers:
dgrogan CC:
chromium-reviews, dgrogan, alecflett, jsbell Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd sync of parent directory when creating or renaming a file.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172237
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixed windows build, rebased. #Patch Set 3 : Correct flag to open() for directory. #
Total comments: 2
Messages
Total messages: 12 (0 generated)
I'm not sure who wants to review this. This should handle syncing of the parent dir on any operation that modifies it, on Linux/Mac. Windows shouldn't need it, according to Siggi.
lgtm but questions below https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... third_party/leveldatabase/env_chromium.cc:73: } This won't compile on windows, will it? https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... third_party/leveldatabase/env_chromium.cc:381: sync_parent(dst); Was ignoring the return value an oversight or conscious choice?
https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... third_party/leveldatabase/env_chromium.cc:34: #include <fcntl.h> Oh yeah, what's this for?
https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... third_party/leveldatabase/env_chromium.cc:34: #include <fcntl.h> On 2012/12/06 23:17:38, dgrogan wrote: > Oh yeah, what's this for? O_WRONLY https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... third_party/leveldatabase/env_chromium.cc:73: } On 2012/12/06 23:15:41, dgrogan wrote: > This won't compile on windows, will it? Fixed. However, I'm having trouble uploading a new version, possibly because this is in third_party and I shouldn't really be editing it here. We just need to swap the "#endif "and the "}", lines 73 and 74. I also had to merge out; I'll mail you the patch so you can land it. https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... third_party/leveldatabase/env_chromium.cc:381: sync_parent(dst); On 2012/12/06 23:15:41, dgrogan wrote: > Was ignoring the return value an oversight or conscious choice? Conscious choice. There's nothing we can do about it at this point, since we've already done the replacement, so returning an error doesn't seem useful. Shouting a log message to a file that's never going to be looked at seems not terribly useful either.
lgtm I'd say keep messing around with uploading the patch to (and committing from) this issue, it was probably an appengine problem. https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... File third_party/leveldatabase/env_chromium.cc (right): https://codereview.chromium.org/11445030/diff/1/third_party/leveldatabase/env... third_party/leveldatabase/env_chromium.cc:73: } Editing it here should work, env_chromium.cc lives in the chromium repository. The files in third_party/leveldatabase and third_party/leveldatabase/port live in chromium. The leveldb repo is pulled in at third_party/leveldatabase/src.
On 2012/12/10 21:53:46, dgrogan wrote: > I'd say keep messing around with uploading the patch to (and committing from) > this issue, it was probably an appengine problem. Sure enough, it worked this time. > Editing it here should work, env_chromium.cc lives in the chromium repository. Ah, OK, no problem, then.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/11445030/7001
Retried try job too often on linux_chromeos for step(s) content_unittests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/11445030/12003
Message was sent while issue was closed.
Change committed as 172237
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11445030/diff/12003/third_party/leveld... File third_party/leveldatabase/env_chromium.cc (right): https://chromiumcodereview.appspot.com/11445030/diff/12003/third_party/leveld... third_party/leveldatabase/env_chromium.cc:94: #if !defined(OS_WIN) What was the impetus for skipping this on windows? I'm interested in turning it on for windows too to see what happens.
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11445030/diff/12003/third_party/leveld... File third_party/leveldatabase/env_chromium.cc (right): https://chromiumcodereview.appspot.com/11445030/diff/12003/third_party/leveld... third_party/leveldatabase/env_chromium.cc:94: #if !defined(OS_WIN) On 2013/05/28 22:02:19, dgrogan wrote: > What was the impetus for skipping this on windows? I'm interested in turning it > on for windows too to see what happens. I asked Siggi, and he said that on Windows there was no need to sync the parent directory--that its metadata would be updated via the creation of the new file, without an explicit sync. So it's as if this is already done on Windows. |
