cipd: Extract high level file system operations into the separate interface.

cipd: Extract high level file system operations into the separate interface.

With the aim to implement this interface a bit differently on Windows.
In particular on Windows files can be locked, and functions like EnsureFileGone
must do "rename to a trash dir, delete whenever possible" instead of failing.

R=nodir@chromium.org
BUG=481661
Committed: https://chromium.googlesource.com/infra/infra/+/4513ccf52d0266b695d806a295402d335ad88de7

[nodir, 2015-06-03 19:30:28]

lgtm, comments are minor

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
File go/src/infra/tools/cipd/local/files.go (right):

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/files.go:248: // to extract a package to. Will use
provided FileSystem object to operate on
a provided

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/files.go:250: // must by in a subdirectory of the
given FileSystem root.
must be

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
File go/src/infra/tools/cipd/local/fs.go (right):

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:18: // FileSystem is a high level interface
for operations that touch single file
nit: high-level

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:116: if err == nil {
nit: join lines 115-116?

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:140: // Atomically replace a current symlink
with a new one.
use "the" instead of a

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:143: os.Remove(temp)
use defer for this kind of stuff

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:236: if err := f.EnsureDirectoryGone(temp);
err != nil {
use defer for cleanup

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:236: if err := f.EnsureDirectoryGone(temp);
err != nil {
no need for err variable here

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:253: if ts == lastUsedTime {
if ts <= lastUsedTime {
 ts = lastUsedTime + 1
}

otherwise 3 consequent calls in the same nanosecond will return (X, X + 1, X)
add a test

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:257: lastUsedTimeLock.Unlock()
You are not doing defer because Getpid is slow?
If yes, maybe explain this in a comment

[Vadim Sh., 2015-06-03 22:35:09]

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
File go/src/infra/tools/cipd/local/files.go (right):

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/files.go:248: // to extract a package to. Will use
provided FileSystem object to operate on
On 2015/06/03 19:30:27, nodir wrote:
> a provided

Done.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/files.go:250: // must by in a subdirectory of the
given FileSystem root.
On 2015/06/03 19:30:27, nodir wrote:
> must be

Done.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
File go/src/infra/tools/cipd/local/fs.go (right):

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:18: // FileSystem is a high level interface
for operations that touch single file
On 2015/06/03 19:30:27, nodir wrote:
> nit: high-level

Done.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:116: if err == nil {
On 2015/06/03 19:30:27, nodir wrote:
> nit: join lines 115-116?

Done.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:140: // Atomically replace a current symlink
with a new one.
On 2015/06/03 19:30:27, nodir wrote:
> use "the" instead of a

Done.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:143: os.Remove(temp)
On 2015/06/03 19:30:27, nodir wrote:
> use defer for this kind of stuff

In this case defer is not good, since cleanup needed only on error and there
isn't much code between where object is created and needs to be cleanup up (i.e.
putting defer will just complicate code).

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:236: if err := f.EnsureDirectoryGone(temp);
err != nil {
On 2015/06/03 19:30:27, nodir wrote:
> use defer for cleanup

again, cleanup path is different depending on circumstances, I think it's
cleaner to cleanup before return rather then in defer.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:236: if err := f.EnsureDirectoryGone(temp);
err != nil {
On 2015/06/03 19:30:27, nodir wrote:
> no need for err variable here

Why?.. It is logged.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:253: if ts == lastUsedTime {
On 2015/06/03 19:30:27, nodir wrote:
> if ts <= lastUsedTime {
>  ts = lastUsedTime + 1
> }
> 
> otherwise 3 consequent calls in the same nanosecond will return (X, X + 1, X)
> add a test

Done.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:257: lastUsedTimeLock.Unlock()
On 2015/06/03 19:30:27, nodir wrote:
> You are not doing defer because Getpid is slow?
> If yes, maybe explain this in a comment

I'm not using a defer because 4 lines of code is not that huge chunk of code to
forgot to unlock the lock after it. I like how Lock()/Unlock() pair visually
identifies critical section.

[Vadim Sh., 2015-06-03 22:36:19]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2015-06-03 22:36:19]

The patchset sent to the CQ was uploaded after l-g-t-m from nodir@chromium.org
Link to the patchset: https://codereview.chromium.org/1154423015/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-06-03 22:37:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154423015/20001

[commit-bot: I haz the power, 2015-06-03 23:56:14]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/infra/infra/+/4513ccf52d0266b695d806a295402...

[nodir, 2015-06-04 00:57:36]

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
File go/src/infra/tools/cipd/local/fs.go (right):

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:236: if err := f.EnsureDirectoryGone(temp);
err != nil {
On 2015/06/03 22:35:08, Vadim Sh. wrote:
> On 2015/06/03 19:30:27, nodir wrote:
> > no need for err variable here
> 
> Why?.. It is logged.

Acknowledged.

https://codereview.chromium.org/1154423015/diff/1/go/src/infra/tools/cipd/loc...
go/src/infra/tools/cipd/local/fs.go:257: lastUsedTimeLock.Unlock()
On 2015/06/03 22:35:08, Vadim Sh. wrote:
> On 2015/06/03 19:30:27, nodir wrote:
> > You are not doing defer because Getpid is slow?
> > If yes, maybe explain this in a comment
> 
> I'm not using a defer because 4 lines of code is not that huge chunk of code
to
> forgot to unlock the lock after it. I like how Lock()/Unlock() pair visually
> identifies critical section.

in general, defers are not syntax sugar. If something panics between Lock and
Unlock (here it won't), then defer is needed.