jschlatow / taskopen

Tool for taking notes and open urls with taskwarrior
GNU General Public License v2.0
364 stars 31 forks source link

Options for quoting (to avoid $ from being expanded) #89

Closed scottkosty closed 8 years ago

scottkosty commented 9 years ago

If an annotation has a dollar sign in it, it is expanded in the shell command that taskopen runs. I imagine this can be a nice feature, but it would be nice to have some control of this (perhaps at the command level; that is, for one command you might want the variables expanded and for another command you might not).

scottkosty commented 8 years ago

One way to address this:

diff --git a/taskopen b/taskopen
index 70bba90..4dbfe2b 100755
--- a/taskopen
+++ b/taskopen
@@ -445,6 +445,9 @@ sub _execute {
     my $fork = $_[0];
     my $cmd  = $_[1];

+    # see #89
+    $cmd =~ s/\$/\\\$/g;
+
     if ($DEBUG > 0) {
         if ($fork > 0) {
             print "forking: ";
jschlatow commented 8 years ago

@scottkosty, thanks for your patch. In general, I agree. I'm not sure, however, for which (if any) commands variable expansion should be the default. I thus applied it to the devel branch for a start.

scottkosty commented 8 years ago

@ValiValpas thanks for the reply and for looking at the patch. I understand and agree with your concern. I'm open to working more on the patch if you have specific ideas to improve it. A couple of ideas are to have a global option that could be flipped on in .taskrc, or a command-specific option (e.g. CUSTOM1_VAREXPAND).

The only other idea I have is to suggest a way for the user to temporarily turn off variable expansion used by the shell. For example, I imagine if I change my system's default shell, then perl's system and exec commands would use that and I could define special rules for my shell without having to touch taskopen. This seems too complicated to me but I just wanted to mention it in case you prefer something like this.

(not directly relevant, can skip) By the way, the use case where this comes up is I'm working on integration of taskwarrior and mutt and am using taskopen as the middleman. Many message IDs contain $. I could escape the message IDs myself before making the annotations and this works, but I have other scripts that parse the annotations without going through taskopen and I have to remember to unescape the $ in these scripts because they treat annotations as literal. I did this a few times but then I stopped myself realizing that I would eventually get into trouble and forget to do this and it was more simple to change taskopen.

jschlatow commented 8 years ago

@scottkosty: Well, what if we turn this around and rather think about the escaping problem in general?! What I have in my mind right now are global and custom options to specify which characters need escaping and which dont (e.g. ESCAPE, CUSTOM1_ESCAPE, etc.). Does this sound right for you?

scottkosty commented 8 years ago

@ValiValpas yes that sounds like a better, more general fix than a binary option.

jschlatow commented 8 years ago

Then I guess we just have to come up with a smart regex for this.

scottkosty commented 8 years ago

So if

ESCAPE='$*('

Then each of those characters would be escaped. Did I understand correctly? By a smart regex I'm guessing you mean something like we can make a character class and then use a backreference?

Also, do you like the location of the patch I suggested before? It should be noted that the regex acts on the full command and not just the argument. I don't know actually what is desired.

jschlatow commented 8 years ago

Correct.

About the location: The create_cmd function might be better suited because this is where the command is determined anyway.

scottkosty commented 8 years ago

OK sounds good.

jschlatow commented 8 years ago

@scottkosty I rethought this issue and updated the devel branch.

I now think the problem was caused by the annotation not being quoted properly when building the command line. The annotation part is now quoted with single quotes. I also think I found a solution that should even work with the unmodified master branch, as the annotation (after the label) is also available in the environment variable $FILE (see manpage), e.g.:

taskopen -x 'echo $FILE'

scottkosty commented 8 years ago

I think the single quote idea is good. I will test the devel branch. Thanks for taking another look at this! I will also look at the documentation regarding $FILE. That is good to know.