Improve .pacsave handling. #29
Conversation
There were the "external" names that documentation talked about, then there were the internal names that the code used. That's silly, and confusing to would-be contributors. entity <- targetFile resource <- repoEntry/repoFile And the use of "target" internally has been normalized: They are now consistently properties of an entity. target (unchanged) base <- target base provisioned <- provisioned target <- last provisioned I did not rename the files in the tests, though the names are now confusing to anyone without knowledge of holo-files' internals before this commit.
Instead of being a pointer, it is now a direct struct. It stores the FileMode (filetype and permissions), as well as the owner; and writes these when you call .Write(). Because the path/basePath distinction is very holo-files specific, and "common" mostly isn't; I've opted to have it not take the basePath argument, and instead have the caller simply change the .Path of the returned buffer if they need to override it. I think it's clearer this way.
| fb.UID = int(stat.Uid) | ||
| fb.GID = int(stat.Gid) | ||
|
|
||
| if fb.Mode&os.ModeSymlink != 0 { |
There was a problem hiding this comment.
I think this will also fire for regular files. In a POSIX-compatible struct stat, the file-mode bits are 010000 for regular files and 012000 for symlinks, and 010000 & 012000 != 0. I suggest you use the same test as in IsFileInfoASymlink. (What confuses me is that if this really is a problem, it should've been caught by the tests many times.)
There was a problem hiding this comment.
No it won't; os.FileMode isn't POSIX, it's a Go-specific bitfield. The ModeSymlink bit is set to be equivalent with checking POSIX S_ISLNK(posix_mode).
| @@ -117,11 +121,17 @@ func (fb *FileBuffer) Write(path string) error { | |||
| } | |||
|
|
|||
| //a manageable file is either a regular file... | |||
| if fb.Contents != nil { | |||
| return ioutil.WriteFile(path, fb.Contents, 600) | |||
| if fb.Mode&os.ModeSymlink == 0 { | |||
| return fb.SymlinkTarget == other.SymlinkTarget | ||
| func (fb FileBuffer) EqualTo(fa FileBuffer) bool { | ||
| fb.Path = fa.Path | ||
| return fa == fb |
There was a problem hiding this comment.
Should reset fb.Path afterwards. It might be okay right now, but it will probably break in subtle ways if EqualTo is ever used somewhere else.
There was a problem hiding this comment.
fb and fa are passed by value, not by reference. Changing fb.path doesn't propagate to outside of the function.
| } | ||
|
|
||
| //result is the stdout of the script | ||
| return common.NewFileBufferFromContents(stdout.Bytes(), buffer.BasePath), nil | ||
| entityBuffer.Mode &^= os.ModeType |
There was a problem hiding this comment.
No mode at all? Wouldn't you set the bits for regular-file?
There was a problem hiding this comment.
In os.FileMode, cleared bits is regular-file.
| if err != nil && !os.IsNotExist(err) { | ||
| if pe, ok := err.(*os.PathError); ok { | ||
| pe.Op, pe.Path = "skipping", "target" | ||
| } |
There was a problem hiding this comment.
IMO this is needlessly obscure. Since the only thing that you want to keep is the pe.Err, I'd rather return false, NewErrSkippingTarget(pe.Err) where ErrSkippingTarget.Error() prepends the skipping target: part. Or just return false, errors.New("skipping target: " + pe.Err.Error()).
There was a problem hiding this comment.
Only one line would get changed:
-pe.Op, pe.Path = "skipping", "target"
+err = errors.New("skipping target: " + pe.Err.Error())
(or even add more lines if using ErrSkippingTarget). That might be a little clearer, but I'm not sure it's better. I'm a bit averse to turning error causes into opaque strings.
I don't think it's worth pulling in a dependency on "github.com/pkg/errors", but if it were there, I would use err = errors.Wrap(pe.Err, "skipping target") without a second thought.
There was a problem hiding this comment.
I would certainly say that errors.New(...) is better. I won't merge the current version because I stared at it for 5 minutes before understanding what's going on, and I'm sure I'll be doing the same when I touch that file again in 6 months. Re github.com/pkg/errors, I don't mind if you add it, but I don't see it adding any value over errors.New() either. In the end, what are we going to do with these errors anyway, if not stringifying them?
Because there is now more metadata associated with a FileBuffer, we
can just read everything all at once, instead of having to deal with a
tangle of pre-flight metadata checks.
Now, this patch actually adds code. There's still a tangle of checks
that I didn't simplify--but it's really easy to do so now. But I
figured I should keep this changeset smaller.
This pulls out some (small-ish) pieces into separate
.Get{Base,NewBaseProvisioned,Current,Desired} methods. I plan to
share these with the applyOrphan code.
…paghetti. The previous commit message teased simplifications that could be made; permform those. Between incremental bugfixes/features, and the unclear lifecycle of the metadata about objects, many of the checks had become spaghetti code. Front-loading the reads, and the unified FileBuffer metadata allow us to easily simplify the code.
I also had to adjust the existing Pacman tests to not trigger the changed behavior.
This improves the
.pacsavehandling by:.pacsavefile (Shouldn't holo-files only delete a.pacsavefile if it matches the file in${HOLO_STATE_DIR}/provisioned? #21).pacsave.Nfor numeric NOnly the last 4 commits actually directly do these things. The first 6 commits clean up the implementation, and share a bit more code between the orphan and non-orphan paths. That said, they are still separate, and could still stand to be combined.
The 2nd and 3rd commits rename things. I'd understand if you want me to rebase them out.