Override options from command line #584

Manually merged
dnkl merged 1 commits from r_c_f/foot:config-cli-override into master 4 months ago
r_c_f commented 4 months ago

Allow any configuration option to be overridden with -o/--override
'section.key=value' arguments, as suggested in #554

Allow any configuration option to be overridden with -o/--override 'section.key=value' arguments, as suggested in #554
r_c_f force-pushed config-cli-override from a4aa0b5cb2 to 30f7e24fbf 4 months ago
dnkl added the
enhancement
label 4 months ago
dnkl added this to the 1.8.0 milestone 4 months ago
dnkl requested changes 4 months ago
dnkl left a comment

Overall a really nice implementation. There are some minor issues below, that needs to be addressed. But in general, this is looking really good. Gave it a quick test and appears to behave as expected.

Have you given any thought to adding support for -o,--override to footclient? I think it's something we want, but I also think it's ok to push that to a separate PR.

config.c Outdated
}
static bool parse_key_value(char *kv, char **section, char **key, char **value)
{
dnkl commented 4 months ago
Poster
Owner

nit: function name on next line:

-static bool parse_key_value(char *kv, char **section, char **key, char **value)
+static bool
+parse_key_value(char *kv, char **section, char **key, char **value)
nit: function name on next line: ```diff -static bool parse_key_value(char *kv, char **section, char **key, char **value) +static bool +parse_key_value(char *kv, char **section, char **key, char **value) ```
dnkl marked this conversation as resolved
config.c Outdated
for (i = 0; i < kvlen; ++i) {
if (kv[i] == '.') {
if ((section != NULL) && (*section == NULL)) {
*section = kv;
dnkl commented 4 months ago
Poster
Owner

nit: can we remove the two inner parenthesis?

nit: can we remove the two inner parenthesis?
dnkl marked this conversation as resolved
config.c Outdated
}
} else if (kv[i] == '=') {
if ((section != NULL) && (*section == NULL))
*section = "main";
dnkl commented 4 months ago
Poster
Owner

same here

same here
dnkl marked this conversation as resolved
config.c Outdated
LOG_AND_NOTIFY_ERR("syntax error: %s", it->item);
if (errors_are_fatal)
goto err;
continue;
dnkl commented 4 months ago
Poster
Owner

Breaking out of the loop here triggers a memory leak:

==5163==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x7f4dda597279 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x55b41c0e74eb in main ../../main.c:233
    #2 0x7f4dd955cb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

This happens since the overrides linked list isn't consumed. The easiest fix is probably to skip tll_remove() here, and let main() handle it by calling tll_free() afterwards (on both fail/success).

Breaking out of the loop here triggers a memory leak: ``` ==5163==ERROR: LeakSanitizer: detected memory leaks Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x7f4dda597279 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145 #1 0x55b41c0e74eb in main ../../main.c:233 #2 0x7f4dd955cb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24) ``` This happens since the `overrides` linked list isn't consumed. The easiest fix is probably to skip `tll_remove()` here, and let `main()` handle it by calling `tll_free()` afterwards (on both fail/success).
dnkl marked this conversation as resolved
config.c Outdated
for (enum section i = 0; i < SECTION_COUNT; i++) {
if (strcmp(section_str, section_info[i].name) == 0) {
section = i;
}
dnkl commented 4 months ago
Poster
Owner

Minor optimization: add a break here, to skip looping the rest of the array when we've found a match.

Minor optimization: add a `break` here, to skip looping the rest of the array when we've found a match.
dnkl marked this conversation as resolved
config.c Outdated
if (section == SECTION_COUNT) {
LOG_AND_NOTIFY_ERR("override: invalid section name: %s", section_str);
error_or_continue();
}
dnkl commented 4 months ago
Poster
Owner

Same possible memory leak here

Same possible memory leak here
dnkl marked this conversation as resolved
config.c Outdated
xassert(section_parser != NULL);
if (!section_parser(key, value, conf, path, lineno, errors_are_fatal))
if (!section_parser(key, value, conf, "override", 0, errors_are_fatal))
dnkl commented 4 months ago
Poster
Owner

I'm wondering if it would be useful (or perhaps just confusing) to use the "override" index as line number, instead of always passing 0?

I'm wondering if it would be useful (or perhaps just confusing) to use the "override" index as line number, instead of always passing `0`?
dnkl marked this conversation as resolved
config.c Outdated
if (!section_parser(key, value, conf, path, lineno, errors_are_fatal))
if (!section_parser(key, value, conf, "override", 0, errors_are_fatal))
error_or_continue();
dnkl commented 4 months ago
Poster
Owner

And here

edit: memory leak

And here **edit**: memory leak
dnkl marked this conversation as resolved
main.c Outdated
static const struct option longopts[] = {
{"config", required_argument, NULL, 'c'},
{"check-config", no_argument, NULL, 'C'},
{"override", required_argument, NULL, 'o'},
dnkl commented 4 months ago
Poster
Owner

nit: required_argument is unaligned.

nit: `required_argument` is unaligned.
dnkl marked this conversation as resolved
main.c Outdated
check_config = true;
break;
case 'o':
dnkl commented 4 months ago
Poster
Owner

nit: replace the \t here with spaces.

nit: replace the `\t` here with spaces.
dnkl marked this conversation as resolved
dnkl requested changes 4 months ago
dnkl left a comment

A couple of things below:

config.c Outdated
bool ret = parse_config_file(
include, conf, include_path, errors_are_fatal);
include, conf, include_path, NULL, errors_are_fatal);
dnkl commented 4 months ago
Poster
Owner

Passing NULL for overrides here triggers the following crash:

Program received signal SIGSEGV, Segmentation fault.
0x000055555557465d in parse_config_file (f=0x555555645ea0, conf=0x7fffffffd8f0, path=0x55555562e598 "/tmp/test.conf", overrides=0x0, 
    errors_are_fatal=false) at ../../config.c:2331
2331	   tll_foreach(*overrides, it) {

when the config has an include= statement.

Passing `NULL` for `overrides` here triggers the following crash: ```gdb Program received signal SIGSEGV, Segmentation fault. 0x000055555557465d in parse_config_file (f=0x555555645ea0, conf=0x7fffffffd8f0, path=0x55555562e598 "/tmp/test.conf", overrides=0x0, errors_are_fatal=false) at ../../config.c:2331 2331 tll_foreach(*overrides, it) { ``` when the config has an `include=` statement.
dnkl marked this conversation as resolved
main.c Outdated
config_free(conf);
return ret;
}
tll_free(overrides);
dnkl commented 4 months ago
Poster
Owner

tll_free() expands to a fairly large amount of code; I would suggest doing something like:

bool conf_successful = config_load(...);
tll_free(overrides);
if (!conf_successful) {
    config_free(conf);
    return ret;
}
`tll_free()` expands to a fairly large amount of code; I would suggest doing something like: ```c bool conf_successful = config_load(...); tll_free(overrides); if (!conf_successful) { config_free(conf); return ret; } ```
dnkl marked this conversation as resolved
dnkl reviewed 4 months ago
config.c Outdated
xassert(section_parser != NULL);
if (!section_parser(key, value, conf, path, lineno, errors_are_fatal))
if (!section_parser(key, value, conf, "override", i, errors_are_fatal))
dnkl commented 4 months ago
Poster
Owner

Perhaps we should say "cli" instead of "override"? Or "--override"?

Perhaps we should say "cli" instead of "override"? Or "--override"?
dnkl marked this conversation as resolved
dnkl requested changes 4 months ago
dnkl left a comment

Looks good. I was just about to merge, when I realized we have forgotten to update the shell completions in completions/*/. Can you add those? Let me know if some of them are troublesome.

config.c Outdated
size_t kvlen = strlen(kv);
size_t i;
for (i = 0; i < kvlen; ++i) {
if (kv[i] == '.') {
dnkl commented 4 months ago
Poster
Owner

super nit: declare i in the for-loop (for (size_t i = 0...))

super nit: declare `i` in the `for`-loop (`for (size_t i = 0...)`)
dnkl marked this conversation as resolved
Owner

Regarding the shell completions: I don't think it's meaningful to try to complete all possible key/values. Just add -o,--override, and tell the shells the option requires an argument, but don't provide any completions for the argument.

Regarding the shell completions: I don't think it's meaningful to try to complete all possible key/values. Just add `-o,--override`, and tell the shells the option requires an argument, but don't provide any completions for the argument.
Poster

~~As far as footclient goes, that actually wouldn't be too difficult to add, and it might as well go here, it will be pushed shortly ~~

On second thought it probably should be separate, under the guise of harmonizing the options between foot and footclient in general. It's definitely desirable, though, as it would greatly simplify the client protocol if nothing else (no bespoke fields need to be sent, just the override corresponding to the given command line argument).

~~As far as `footclient` goes, that actually wouldn't be too difficult to add, and it might as well go here, it will be pushed shortly ~~ On second thought it probably should be separate, under the guise of harmonizing the options between `foot` and `footclient` in general. It's definitely desirable, though, as it would greatly simplify the client protocol if nothing else (no bespoke fields need to be sent, just the override corresponding to the given command line argument).
r_c_f force-pushed config-cli-override from e3ed03fb4b to 76ca6bd2d7 4 months ago
dnkl requested changes 4 months ago
dnkl left a comment

Looks mostly good. I think the refactoring is an improvement, even if we didn't intend to add -o,--override to footclient.

I'll run some tests on this tomorrow, but wanted to point out the issues I've found so far, to give you a headstart ;)

"--log-no-syslog"
"--login-shell"
"--maximized"
"--option"
dnkl commented 4 months ago
Poster
Owner

s/option/override/

s/option/override/
dnkl marked this conversation as resolved
elif [[ ${prev} == '--log-colorize' ]] ; then
COMPREPLY=( $(compgen -W "never always auto" -- ${cur}) )
elif [[ ${prev} =~ ^(--app-id|--help|--title|--version|--window-size-chars|--window-size-pixels|--check-config)$ ]] ; then
elif [[ ${prev} =~ ^(--app-id|--help|--option|--title|--version|--window-size-chars|--window-size-pixels|--check-config)$ ]] ; then
dnkl commented 4 months ago
Poster
Owner

s/option/override

s/option/override
dnkl marked this conversation as resolved
complete -c foot -x -a "(__fish_complete_subcommand)"
complete -c foot -r -s c -l config -d "path to configuration file (XDG_CONFIG_HOME/foot/foot.ini)"
complete -c foot -s C -l check-config -d "verify configuration and exit with 0 if ok, otherwise exit with 1"
complete -c foot -s o -l override -d "configuration option to override, in form SECTION.KEY=VALUE"
dnkl commented 4 months ago
Poster
Owner

Typing foot --override <tab> will return file name completions. I think you need to add -x, like in e.g. -T,--title.

Typing `foot --override <tab>` will return file name completions. I think you need to add `-x`, like in e.g. `-T,--title`.
dnkl marked this conversation as resolved
-s -S -C \
'(-c --config)'{-c,--config}'[path to configuration file (XDG_CONFIG_HOME/foot/foot.ini)]:config:_files' \
'(-C --check-config)'{-C,--check-config}'[verify configuration and exit with 0 if ok, otherwise exit with 1]' \
'(-o --override)'{-o,--override}'[configuration option to override, in form SECTION.KEY=VALUE]' \
dnkl commented 4 months ago
Poster
Owner

Same here, you need to add :() at the end (see -T,--title).

Same here, you need to add `:()` at the end (see `-T,--title`).
dnkl marked this conversation as resolved
config.c Outdated
enum section section;
for (section = SECTION_MAIN; section < SECTION_COUNT; ++section) {
if (strcmp(str, section_info[section].name) == 0)
break;
dnkl commented 4 months ago
Poster
Owner

nit: I think this would be somewhat clearer if you did return section here, instead of break, and replaced the current return with an explicit return SECTION_COUNT.

nit: I think this would be somewhat clearer if you did `return section` here, instead of `break`, and replaced the current `return` with an explicit `return SECTION_COUNT`.
dnkl marked this conversation as resolved
dnkl approved these changes 4 months ago
dnkl left a comment

Alright, I think this one's good to go! Can you please rebase and squash the commits?

r_c_f force-pushed config-cli-override from f592f4c98b to f379ffb8ed 4 months ago
dnkl merged commit 6a0a6b0b01 into master manually 4 months ago
Owner

Thanks!

Thanks!

Reviewers

dnkl approved these changes 4 months ago
The pull request has been manually merged as 6a0a6b0b01.
Sign in to join this conversation.
Loading…
There is no content yet.