Skip to content
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

Merged
merged 6 commits into from Apr 17, 2017

Conversation

@adambyrtek
Copy link
Contributor

@adambyrtek adambyrtek commented Apr 17, 2017

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:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md
@adambyrtek
Copy link
Contributor Author

@adambyrtek adambyrtek commented Apr 17, 2017

@krader1961 Review please. Not sure whether this is significant enough for a CHANGELOG entry.

# 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

This comment has been minimized.

@krader1961

krader1961 Apr 17, 2017
Contributor

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].

This comment has been minimized.

@faho

faho Apr 17, 2017
Member

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.

This comment has been minimized.

@adambyrtek

adambyrtek Apr 17, 2017
Author Contributor

Done, thanks for some really good advice.

# with the editor command
if set -q checksum
if echo "$checksum" | md5sum --check --status
_ "Editor exited but the function was not modified"

This comment has been minimized.

@krader1961

krader1961 Apr 17, 2017
Contributor

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").

This comment has been minimized.

@adambyrtek

adambyrtek Apr 17, 2017
Author Contributor

Done

# 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)

This comment has been minimized.

@faho

faho Apr 17, 2017
Member

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.

This comment has been minimized.

@adambyrtek

adambyrtek Apr 17, 2017
Author Contributor

I'll look into making this work on macOS as well.

@faho
Copy link
Member

@faho faho commented Apr 17, 2017

@krader1961's comments are pretty much spot on:

  • set -l your variables before using them, or risk clobbering global ones

  • Check for existence of md5sum outside of the loop

Also, please don't use which.

@adambyrtek
Copy link
Contributor Author

@adambyrtek adambyrtek commented Apr 17, 2017

@krader1961 @faho Thanks for your review, I replied to most of your comments inline.

When it comes to the while loop, I put the checksum calculation inside the loop on purpose, so that we check for modifications against the previous version (the one sent to the last instance of the editor). This all happens interactively so I don't think there is a performance problem, but it's pretty much an edge case anyway, so I'll follow you advice as long as you understand my motivations for putting the check inside the loop.

@adambyrtek
Copy link
Contributor Author

@adambyrtek adambyrtek commented Apr 17, 2017

@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

This comment has been minimized.

@faho

faho Apr 17, 2017
Member

This is a good idea!

md5sum $argv[1] | cut -d' ' -f1
return 0
end
if type -q md5

This comment has been minimized.

@faho

faho Apr 17, 2017
Member

This should be an "else if", otherwise if both exist, both will be run, which is at the very least a waste of resources.

This comment has been minimized.

@adambyrtek

adambyrtek Apr 17, 2017
Author Contributor

Done

@adambyrtek
Copy link
Contributor Author

@adambyrtek adambyrtek commented Apr 17, 2017

@faho Well spotted, I made the change you requested.

@faho faho added the enhancement label Apr 17, 2017
@faho faho added this to the 2.6.0 milestone Apr 17, 2017
@faho
faho approved these changes Apr 17, 2017
function __funced_md5
if type -q md5sum
# GNU systems
md5sum $argv[1] | cut -d' ' -f1

This comment has been minimized.

@faho

faho Apr 17, 2017
Member

Ideally, this would use string replace -r ' .*' ''.

This comment has been minimized.

@adambyrtek

adambyrtek Apr 17, 2017
Author Contributor

Good point but after reading some docs I realised that string split would be more readable :)

@adambyrtek
Copy link
Contributor Author

@adambyrtek adambyrtek commented Apr 17, 2017

@faho Changes made :)

@faho faho merged commit dd69ca5 into fish-shell:master Apr 17, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@faho
Copy link
Member

@faho faho commented Apr 17, 2017

Merged, thanks!

@adambyrtek adambyrtek deleted the adambyrtek:funced-checksum branch Apr 17, 2017
develop7 added a commit to develop7/fish-shell that referenced this pull request Apr 17, 2017
…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`
@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Apr 17, 2017

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.