New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Warning when function is not modified by the editor after calling funced
#3961
Conversation
@krader1961 Review please. Not sure whether this is significant enough for a CHANGELOG entry. |
share/functions/funced.fish
Outdated
# Repeatedly edit until it either parses successfully, or the user cancels | ||
# If the editor command itself fails, we assume the user cancelled or the file | ||
# could not be edited, and we do not try again | ||
while true | ||
if which md5sum > /dev/null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice this will only be run once but it's still better to move it outside the while
loop. Also, all vars local to the function should be declared as such. Otherwise you risk modifying a global var by the same name. That is,
set -l checksum
if type -q md5sum
set checksum (md5sum $tmpname)
end
Note tht it's more idiomatic to use if type -q md5sum
which will do the right thing if md5sum is a function and doesn't require the redirection. Below you'll need to modify the test to read if set -q checksum[1]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use which
since that itself is not guaranteed to be installed. If something needs to be a command (and I'd expect it would be here), use command -sq
. If it doesn't need to be, use type -q
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for some really good advice.
share/functions/funced.fish
Outdated
# with the editor command | ||
if set -q checksum | ||
if echo "$checksum" | md5sum --check --status | ||
_ "Editor exited but the function was not modified" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you're just copying the pattern from a few lines above but it is not idiomatic. Please rewrite this, and the other instances in this function, as echo (_ "message")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
share/functions/funced.fish
Outdated
# Repeatedly edit until it either parses successfully, or the user cancels | ||
# If the editor command itself fails, we assume the user cancelled or the file | ||
# could not be edited, and we do not try again | ||
while true | ||
if which md5sum > /dev/null | ||
set checksum (md5sum $tmpname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that it appears BSDs (and macOS) don't have an "md5sum" program, but an md5
one. So this function would be disabled on those systems. Which is a good way of failing, but it would be nicer if it didn't.
I'll leave it to those using those systems to come up with the correct incantation to use here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into making this work on macOS as well.
@krader1961's comments are pretty much spot on:
Also, please don't use |
@krader1961 @faho Thanks for your review, I replied to most of your comments inline. When it comes to the |
@faho Please take another look. I've added a private MD5 function with support for both GNU and BSD to make this more portable. I kept the checksum calculation inside the loop (see comment above) but I'm happy to move it outside if that's still your preference. |
@@ -1,3 +1,17 @@ | |||
function __funced_md5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea!
share/functions/funced.fish
Outdated
md5sum $argv[1] | cut -d' ' -f1 | ||
return 0 | ||
end | ||
if type -q md5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an "else if", otherwise if both exist, both will be run, which is at the very least a waste of resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@faho Well spotted, I made the change you requested. |
share/functions/funced.fish
Outdated
function __funced_md5 | ||
if type -q md5sum | ||
# GNU systems | ||
md5sum $argv[1] | cut -d' ' -f1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, this would use string replace -r ' .*' ''
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point but after reading some docs I realised that string split
would be more readable :)
@faho Changes made :) |
Merged, thanks! |
…d` (fish-shell#3961) * Check whether tmp file was modified in `funced` * More idiomatic error messages * Store the checksum in a local variable * MD5 function supporting both GNU and BSD * Use `else if` in MD5 function * Use `string` builtin instead of `cut`
Whether the initial calculation should be inside or outside the while loop depends on what we're trying to warn the user about. The original justification was to protect against a GUI editor automatically going into the background which could confuse the user into thinking that any change they make will be honored. That argues for doing the check once outside the loop. However, if we want to warn the user that they didn't make a change every time the editor returns control to the function then it belongs inside the loop. I'm fine with it being inside the loop. |
Description
funced
runs the editor command with a temporary file and quits silently once the process completes. However, in some cases the editor command might return immediately (see #1081). This change shows a warning if the file wasn't modified (according to the MD5 checksum) to make it easier to spot problems like that.Fixes issue #1081
TODOs: