Warning when function is not modified by the editor after calling `funced` by adambyrtek · Pull Request #3961 · fish-shell/fish-shell

adambyrtek · 2017-04-17T01:14:49Z

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 · 2017-04-17T01:15:51Z

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

krader1961 · 2017-04-17T02:43:02Z

    # 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


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

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.

Done, thanks for some really good advice.

krader1961 · 2017-04-17T02:45:05Z

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


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

faho · 2017-04-17T11:37:06Z

    # could not be edited, and we do not try again
    while true
+        if which md5sum > /dev/null
+            set checksum (md5sum $tmpname)


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.

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

faho · 2017-04-17T11:38:51Z

@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 · 2017-04-17T11:49:42Z

@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 · 2017-04-17T12:38:04Z

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

faho · 2017-04-17T13:25:10Z

@@ -1,3 +1,17 @@
+function __funced_md5


This is a good idea!

faho · 2017-04-17T13:25:48Z

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


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

adambyrtek · 2017-04-17T13:32:08Z

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

faho · 2017-04-17T14:03:04Z

+function __funced_md5
+    if type -q md5sum
+        # GNU systems
+        md5sum $argv[1] | cut -d' ' -f1


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

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

adambyrtek · 2017-04-17T15:08:54Z

@faho Changes made :)

faho · 2017-04-17T15:18:11Z

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`

krader1961 · 2017-04-17T20:04:10Z

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.

Check whether tmp file was modified in funced

c594a63

krader1961 reviewed Apr 17, 2017

View reviewed changes

More idiomatic error messages

3777104

faho reviewed Apr 17, 2017

View reviewed changes

Store the checksum in a local variable

6317c46

MD5 function supporting both GNU and BSD

a87d897

faho requested changes Apr 17, 2017

View reviewed changes

Use else if in MD5 function

689575b

faho added the enhancement label Apr 17, 2017

faho added this to the 2.6.0 milestone Apr 17, 2017

faho approved these changes Apr 17, 2017

View reviewed changes

faho requested changes Apr 17, 2017

View reviewed changes

Use string builtin instead of cut

b9e586f

faho merged commit dd69ca5 into fish-shell:master Apr 17, 2017

adambyrtek deleted the funced-checksum branch April 17, 2017 15:22

github-actions Bot locked as resolved and limited conversation to collaborators Apr 17, 2020

Uh oh!

Conversation

adambyrtek commented Apr 17, 2017

Description

TODOs:

Uh oh!

adambyrtek commented Apr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

faho commented Apr 17, 2017

Uh oh!

adambyrtek commented Apr 17, 2017

Uh oh!

adambyrtek commented Apr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambyrtek commented Apr 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambyrtek commented Apr 17, 2017

Uh oh!

faho commented Apr 17, 2017

Uh oh!

krader1961 commented Apr 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants