Modifying the input is surprising
pwaller opened this issue · 4 comments
This is one of those 'obvious in hindsight' things.
goreduce deleted my testcase. I don't know what I expected, but I wasn't expecting it to be destructive to my input. It seems that this is coupled with another issue (or PEBKAC), because goreduce didn't respect my -match string as I expected either, so it managed to delete just almost the entire input.
Luckily, I had a copy of it around. But I think if you're going to modify the input in place there should be a bright red warning somewhere - at least it should be mentioned in the README - or a flag where the user can signal that they understand what is going to happen (--inplace).
I also note that the text of the README might leave one to believe the input would be preserved, because it says that the runs should be reproducible for same input (implying to me that you should be able to repeatedly run it from the same input).
That made my morning :) I'm curious; exactly what did you run?
I should certainly add a warning, at least. In my mind, this was okay because I generally always work within git directories. But for one-off reproducers, I can see how there'd just be the one file on disk.
I'd rather not add flags. What do you think about creating a backup file next to all modified files, like original.go.bk? The tool would error out if such a file already exists, to avoid removing its own backup files from previous runs; then it's up to the user to consciously delete code forever.
What about the reverse: reduced/original.go or original.go.reduced? This would leave the input files where they were and enable it to be reproduced by re-running the original command.
The command I ran was goreduce -match=StructOf ., with the intent of keeping reproducers which have StructOf in the traceback.
I'm aware that goreduce might not be capable of making edits to this struct - if it wasn't, I was planning of growing a rule which shrinks input arrays.
I ended up manually reducing the file in a text editor.
What about the reverse:
reduced/original.goororiginal.go.reduced? This would leave the input files where they were and enable it to be reproduced by re-running the original command.
Maybe. Part of the idea was that, after running goreduce, one could easily go build or go run to try out the result. If one is running the tool on many files or packages, having to copy the files to their correct locations would be difficult, and commands like git diff would be hard to replace.
The internal implementation would also be trickier; if we don't modify in-place, we suddenly need to place the Go packages elsewhere, like an entirely new module in a temporary directory. Certainly doable, but it's more code.
The only situation I see this not-inplace feature to be useful is when one is not inside a VCS clone. And I think that should only happen when one has very few files. In those cases, if something goes wrong, it's trivial to recover the backups with a couple of lines of shell.
I think the simplest fix would be a warning in the README, stating that changes are made in-place while writing backups (and that using a git clone is recommended), and implementing the backups.