holo-files: Support patch files (#5) #42
Conversation
This simplifies the code; as most of the code from the old separate Disambiguator() and EntityPath() was identical.
This makes the minimal textual changes to accomplish this (don't move
things between files, add a bogus `if true {` to avoid indentation
change, ...), to make rebasing it easier. The next commit will clean
that up.
- shorten rawResource's methods - remove the bogus 'if true' from StaticResource.ApplyTo()
This affects patch files with the "\ No newline at end of file" footer.
Alone, this is probably a bad change. But, this structure is amenable to adding other resource types; which a subsequent commit will do. I wanted these wording changes to be review-able separately from wording for new behavior.
This ended up being a lot more work than I thought it would be... though a good chunk of that was slowly realizing all of the ways that I could mess up writing patch files (in the tests) by hand. Apparently, when applying `--git` style patches, GNU patch is pretty finicky about the "index" line, which I had expected it to basically ignore. BusyBox patch doesn't implement the `-d` flag, despite it being in POSIX for as long as `patch` has (2001; before that patch was standardized in XDG/SUS, before that merged with POSIX; but I don't have pre-merge copies handy to see if they had the `-d` flag). So use `cmd.Dir` instead of `-d`. That doesn't mean that BusyBox patch will pass the tests, just that it will work. It will still fail the tests: - The tests check for `--git` style changing of symlinks. BusyBox patch does not support patching symlinks - The tests check for `--git` style changing of and file permissions. BusyBox patch does not support that - The tests expect it to say "patching file FILENAME" when creating a file (GNU patch behavior); BusyBox patch says "creating FILENAME". - The BusyBox fuzz detector seems to be less forgiving than the GNU one; it rejects my with-fuzz test. That said, BusyBox patch will still work for users writing ordinary patch files. I will 100% not be surprised of the tests don't pass with BSD patch for similar reasons. This breaks backward compatibility in a very minor way: previously, a resource file with the ".patch" suffix was understood to be a verbatim file; this changes that, obviously. The target file is operated on in a temporary directory, and without specifying the `-p` flag or the target filename. This gives the patch the freedom to do directory-type operations (change file type). If the patch creates other files, they will be ignored.
|
I had thought about avoiding portability concerns with different versions of But now I realize obvious answer is to just run |
|
But |
doc/holo-files.8.pod
Outdated
| =item Otherwise, the resource file is a plain file or symlink that will just | ||
| overwrite the contents of the target base (and all previous resource application | ||
| steps). The ownership and file permissions are not set from the resource file, | ||
| and are inherited from the target base. |
There was a problem hiding this comment.
Nitpick: "but are inherited"
| return common.FileBuffer{}, err | ||
| } | ||
| defer os.RemoveAll(targetDir) | ||
| targetPath := filepath.Join(targetDir, filepath.Base(entityBuffer.Path)) |
There was a problem hiding this comment.
targetPath is a bit confusing since we already have a "target path" in our jargon at some other point. Maybe workDir and workPath, or tempDir and tempPath?
| "-i", patchfile, | ||
| ) | ||
| cmd.Dir = targetDir | ||
| cmd.Stdout = os.Stderr |
There was a problem hiding this comment.
We should swallow the success-case output (patching file $filename) here, instead of just sending it to our stderr. Writing something on stderr causes Holo (the frontend) to always show the full "Working on $entity" display, even if we're not changing the entity.
Swallowing the output in the happy path would also make the tests slightly more portable since we can match and swallow both the GNU patch success message and the Busybox patch success message (and later also the BSD one).
|
|
||
| =item C<.patch> The resource file is understood to be a patch file that can be | ||
| fed to the L<patch(1)> program. Some patch formats have the ability to change | ||
| file type and file permissions; this is respected, making this is the only |
|
|
||
| patching file txtfile-with-garbage | ||
| patching file garbage | ||
| patching file ls |
There was a problem hiding this comment.
I don't know how hard it would be to implement, but to avoid confusion and/or surprises, I'd rather have the apply step fail if some parts of the patch are not applicable to the target file.
|
I just looked at this again, thinking that this had been stuck because I did not come back to it, but there are in fact unreviewed comments from me. @LukeShu Do you want to address these? Also, since parsing the output of GNU patch is finicky (and, according to your PR message, not portable to other patch's), I wonder if you considered using a library instead of calling an application. A quick internet search brings up https://github.com/sergi/go-diff which looks like it could fit the bill from a cursory glance. Opinions? |
|
Yeah, this is stuck because I got busy with other things and never came back to it. It definitely still needs some work. A library is probably the way to go, but I haven't looked in to any of them too much. |
Creating a Pull Request for this one, since it definitely needs reviewed.
I'm fairly confident that the included tests will only pass with GNU patch; different patch implementations have too much freedom in how they format their informative output to be able to do verbatim checks portably. Additionally, the tests check that we implement it in such a way that various extensions (git-style changing of file permissions or file type) are able to run--but this means we're also checking for the presence of those extensions in the system's patch implementation. I know that BusyBox patch does not implement them.
patchis called in an directory that is empty except for the input file it will be operating on, and is given-N -i /absolute/path/to/file.patchas arguments. This means that it will apply any segments in the patch file that match the basename of the file being operated on. The patch could create other files, but it can't escape the temporary working directory, and any other files it creates will be ignored (this handles both.origfiles created by fuzzy patches, as well as other files created by misbehaving patches).I think that the clean-ups in all but the last 2 commits (which actually implement patch support) are maybe worth while independently of the patch support itself.