dunst-project / dunst

Lightweight and customizable notification daemon
https://dunst-project.org
Other
4.44k stars 338 forks source link

Use wordexp to resolve variables like $HOME in path like settings #1215

Closed zappolowski closed 6 months ago

zappolowski commented 9 months ago

This fixes #1173 - in combination with #1210

This is a spin-off of #1210 to properly handle environment variables for defining path like variables.

Open questions:

codecov-commenter commented 9 months ago

Codecov Report

Merging #1215 (b558997) into master (5ef093c) will decrease coverage by 0.04%. The diff coverage is 62.50%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #1215      +/-   ##
==========================================
- Coverage   66.19%   66.16%   -0.04%     
==========================================
  Files          46       46              
  Lines        7671     7698      +27     
==========================================
+ Hits         5078     5093      +15     
- Misses       2593     2605      +12     
Flag Coverage Δ
unittests 66.16% <62.50%> (-0.04%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
test/option_parser.c 99.44% <100.00%> (+<0.01%) :arrow_up:
test/utils.c 100.00% <100.00%> (ø)
src/utils.c 85.24% <47.82%> (-4.14%) :arrow_down:

:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

fwsmit commented 9 months ago

Are there convincing use cases for needing command expansion? If not, then I would not include it. It can always be added later.

I think both not expanding undefined variables and expanding them are fine. Not expanding might make it easier for the user to spot errors.

It would be nice if the error message would be a bit more specific depending on the return value of wordexp. Maybe you can use some implementation somewhere that has this already (for example in i3). Keep in mind the license of course.

zappolowski commented 9 months ago

I agree that command substitution is not that important at the moment and adding some functionality later on is easier than removing it (and breaking configs).

It would be nice if the error message would be a bit more specific depending on the return value of wordexp.

I've expanded the warnings for each possible case. Please check, whether that suffices. I've also expanded the documentation where paths are expanded (basically, browser and dmenu also use paths, but I left them out as they most probably are absolute paths anyhow).

PS: I've checked the build failure and AFAICT wordexp on musl doesn't support WRDE_UNDEF. I could disable this test when it runs on musl (but this would also mean, that this feature doesn't work properly on musl-based distros ... Void comes to mind).

zappolowski commented 8 months ago

@fwsmit any further actions required?

fwsmit commented 8 months ago

I'll take a look once I have the time

fwsmit commented 6 months ago

Thanks, the implementation and documentation look good. I'll go ahead and merge it.