replace variables and ~ in script paths by bebehei · Pull Request #351 · dunst-project/dunst

bebehei · 2017-08-04T11:20:30Z

wordexp replaces all variables and ~ in the string. TBH I don't quite like the thing, that it replaces all variables, but there is no other way.

Fixes #350

bebehei · 2017-08-04T11:40:13Z

Well, it might be good here, to add tests, too.

tsipinakis · 2017-08-05T05:01:36Z

Yeah, tests are a good idea. option_parser.c already has tests for most of the functions so it should be trivial to extend them for the path one.

bebehei · 2017-08-05T13:20:45Z

Well, it might be good here, to add tests, too.

Here they are.

bebehei · 2017-08-06T15:22:16Z

+
+        wordexp_t replaced_path;
+        if (path && 0 == wordexp(path, &replaced_path, WRDE_NOCMD)){
+                path = g_strdup(replaced_path.we_wordv[0]);


@tsipinakis I'm missing here a free(path);, right?

Yeah you are missing a free, path from init_get_string is never freed.

tsipinakis · 2017-08-06T18:36:29Z

+        free(ptr);
+
+        path = ini_get_string(section, "expand_home", "");
+        wordexp(path, &replaced_path, WRDE_NOCMD);


Since we know the contents of the expansion we should use the simplest method (read: getenv) to get the data we need to compile the test case. Using the exact same code the function you're testing is using is error prone and can miss quite a few bugs.

(hint: You can use the string_replace function from utils.c to do the actual replacement)

tsipinakis · 2017-08-06T18:41:34Z

+
+        path = ini_get_string(section, "expand_tilde", "");
+        wordexp(path, &replaced_path, WRDE_NOCMD);
+        ASSERT_STR_EQ(replaced_path.we_wordv[0], (ptr = ini_get_path(section, "expand_tilde", "")));


Same with this test, we should use the simplest method to setup the test case.

tsipinakis · 2017-08-06T18:43:02Z

+
+        wordexp_t replaced_path;
+        if (path && 0 == wordexp(path, &replaced_path, WRDE_NOCMD)){
+                path = g_strdup(replaced_path.we_wordv[0]);


Yeah you are missing a free, path from init_get_string is never freed.

tsipinakis · 2017-08-06T18:44:33Z

+        char *path = ini_get_string(section, key, def);
+
+        wordexp_t replaced_path;
+        if (path && 0 == wordexp(path, &replaced_path, WRDE_NOCMD)){


Some more error handling here would be nice, especially for WRDE_BADVAL and WRDE_CMDSUB.

tsipinakis · 2017-08-06T18:52:41Z

+
+        wordexp_t replaced_path;
+        if (path && 0 == wordexp(path, &replaced_path, WRDE_NOCMD)){
+                path = g_strdup(replaced_path.we_wordv[0]);


You're also only getting the first word here, we_wordv is a string array (char **), not a char *. It's a common mistake so please also add a regression test for this.

bebehei · 2017-08-07T21:19:50Z

Disclaimer: I'm currently not at my laptop for the next few days and can't test it. I read the wordexp manpage over and over again until I fully understood the Problems.

I have to conclude, that this is the shittiest approach possible.

Here is why:

You're also only getting the first word here, we_wordv is a string array

Yes this is correct. So if there are multiple words, we have to strcat all we_wordv elements together with $IFS.

Field splitting is done using the environment variable $IFS. If it is not set, the field separators are space, tab and newline.

Well, if someone sets `IFS=/', the returned path is broken.

Even worse: IFS is unset, the path contains spaces. So how should we deterministically chain the words again together?

Wordexp also does more stuff, I personally dislike. It also replaces other variables. IMO in our case only $HOME (maybe additionally $USER) should get expanded and nothing more.

(hint: You can use the string_replace function from utils.c to do the actual replacement)

What about using this in the actual ini_get_string? I think about replacing the first two characters if they match ~/ with getenv('HOME').

bebehei · 2017-08-13T19:53:22Z

Field splitting is done using the environment variable $IFS. If it is not set, the field separators are space, tab and newline.

Well, if someone sets `IFS=/', the returned path is broken.

Even worse: IFS is unset, the path contains spaces. So how should we deterministically chain the words again together?

Well, it turns out to be true in all cases. wordexp makes classical word. This is uesless.

What about using this in the actual ini_get_string? I think about replacing the first two characters if they match ~/ with getenv('HOME').

So, I went for that. Here are the commits. Sorry for the spam on travis. In my previous commits, I failed using strcat. 🤦

tsipinakis · 2017-08-15T09:03:50Z

So, I went for that. Here are the commits. Sorry for the spam on travis. In my previous commits, I failed using strcat.

No worries, you're not a C programmer until you've wasted a few hours debugging weird memory issues :p

It might be beneficial to also apply this to the dmenu and browser settings, they are also paths after all.

bebehei · 2017-08-15T09:05:20Z

It might be beneficial to also apply this to the dmenu and browser settings, they are also paths after all.

Thanks for the hint.

bebehei · 2017-08-15T22:03:15Z

It might be beneficial to also apply this to the dmenu and browser settings, they are also paths after all.

I moved the logics now to a function in utils.h, added cmdline support and dmenu and the browser use these settings now, too.

The tests from test_ini_get_path about correct expansion have moved now to a separate test in utils.h, while in test_(ini|cmdline|option)_get_path now only test, if expansion works. and the handling of cmdline precedence.

tsipinakis · 2017-08-19T07:12:12Z

LGTM 👍

Thank you for implementing this.

bebehei force-pushed the wordexp branch from 7065690 to d4b82ae Compare August 5, 2017 13:20

bebehei commented Aug 6, 2017

View reviewed changes

tsipinakis requested changes Aug 6, 2017

View reviewed changes

bebehei force-pushed the wordexp branch 7 times, most recently from 246a5c2 to 4e873fe Compare August 13, 2017 19:38

bebehei force-pushed the wordexp branch from 4e873fe to ae834f3 Compare August 13, 2017 20:36

tsipinakis approved these changes Aug 15, 2017

View reviewed changes

replace ~/ in script paths

8e3a758

bebehei force-pushed the wordexp branch from ae834f3 to 8b49775 Compare August 15, 2017 21:38

add tests for *_get_path functions

7eb99ea

bebehei force-pushed the wordexp branch from 8b49775 to 7eb99ea Compare August 15, 2017 21:56

tsipinakis merged commit 3c257ea into dunst-project:master Aug 19, 2017

tsipinakis mentioned this pull request Aug 19, 2017

add support for transient hints #343

Merged

bebehei deleted the wordexp branch August 19, 2017 10:56

bebehei mentioned this pull request Jun 19, 2018

Path in configs to resolve ( ~ ) #520

Closed

Conversation

bebehei commented Aug 4, 2017

Uh oh!

bebehei commented Aug 4, 2017

Uh oh!

tsipinakis commented Aug 5, 2017

Uh oh!

bebehei commented Aug 5, 2017

Uh oh!

bebehei Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

tsipinakis Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

tsipinakis Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

tsipinakis Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

tsipinakis Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

tsipinakis Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsipinakis Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bebehei commented Aug 7, 2017

Uh oh!

bebehei commented Aug 13, 2017

Uh oh!

tsipinakis commented Aug 15, 2017

Uh oh!

bebehei commented Aug 15, 2017

Uh oh!

bebehei commented Aug 15, 2017

Uh oh!

tsipinakis commented Aug 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tsipinakis Aug 6, 2017 •

edited

Loading

tsipinakis Aug 6, 2017 •

edited

Loading