Conversation
Turns out, `git merge-tool` wants the base version in the _second_ argument, and not the first! This was never noticed, because for changes _only_ related to formatting, it won't matter, since for usual rebases and merges, the actual first argument, the local version, only contains the formatting changes, which can be entirely ignored anyways. So this only causes problems if there's not exclusively formatting-related changes, which can of course happen.
Turns out, if the mergetool tries to write files larger than the buffer size, it gets stuck, because in the previous code there was nothing that reads that buffer asynchronously. This now fixes that by writing to a file instead of a handle.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
main/Main.hs
Outdated
| (proc "git" ["merge-file", "--stdout", local, base, remote]) | ||
| { std_out = CreatePipe | ||
| } | ||
| exitCode <- lift $ withFile merged WriteMode $ \out -> do |
There was a problem hiding this comment.
Feel free to ignore this comment:
My haskell is too weak to really grok this code. Is there any issue if we crash partway through invoking git merge-file? Could we end up with a cut off file?
I was expecting to see something like "write to a temp file and then atomically move". But I guess we didn't have that in the old code, or at least I don't see anything like that.
There was a problem hiding this comment.
Good point! I now added a commit that both simplifies the code and takes care of this. It works by not using the --stdout flag, causing git merge-file to write it to the local file directly (which is a temporary file created by git mergetool), and then move that file to merged after formatting :D
More resilient in case e.g. git merge-file fails in the middle
Probably the last unstable update before stabilising nixfmt. Includes: - Crucial fixes for the merge tool: NixOS/nixfmt#291 - Recursive mode deprecation warning informing of nixfmt-tree: NixOS/nixfmt#287
3 participants
Turns out there are some problems to fix, see the commit messages for details. I made sure to add regression tests.
This work is funded by Antithesis and Tweag ✨