From 5088ad7f1c9b6680b5e448c58def3d87d311b4b2 Mon Sep 17 00:00:00 2001 From: Steven Chan Date: Fri, 10 May 2019 12:46:40 -0700 Subject: [PATCH 1/6] Improve argv_to_string_alloc() 1. Fix bug in argv_to_string_alloc(), which did not increase buffer size to account for number of separators. 2. Improve coding a. Initialize buffer size to one to account for terminator, which avoids the need to later increment by one. b. Call concat_argv() directly, instead of calling argv_to_string(), which is a wrapper. 3. Add similar function argv_to_string_alloc_prefix(), which preloads string buffer with a prefix string. --- include/tig/argv.h | 1 + src/argv.c | 37 ++++++++++++++++++++++++++++++++----- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/include/tig/argv.h b/include/tig/argv.h index aaab7236e..cfe0498ac 100644 --- a/include/tig/argv.h +++ b/include/tig/argv.h @@ -24,6 +24,7 @@ #define DIFF_ARGS "%(diffargs)" bool argv_to_string(const char *argv[], char *buf, size_t buflen, const char *sep); +char *argv_to_string_alloc_prefix(const char *argv[], const char *sep, const char *prefix); char *argv_to_string_alloc(const char *argv[], const char *sep); bool argv_to_string_quoted(const char *argv[SIZEOF_ARG], char *buf, size_t buflen, const char *sep); bool argv_from_string_no_quotes(const char *argv[SIZEOF_ARG], int *argc, char *cmd); diff --git a/src/argv.c b/src/argv.c index 0e68358c5..f28b84912 100644 --- a/src/argv.c +++ b/src/argv.c @@ -56,16 +56,43 @@ concat_argv(const char *argv[], char *buf, size_t buflen, const char *sep, bool } char * -argv_to_string_alloc(const char *argv[], const char *sep) +argv_to_string_alloc_prefix(const char *argv[], const char *sep, const char *prefix) { - size_t i, size = 0; - char *buf; + size_t i, size = 1; + for (i = 0; argv[i]; i++) + size += strlen(argv[i]); + + /* Increase buffer size by separator size and number of separators */ + if (sep) if (i) size += strlen(sep) * (i - 1); + + /* Increase buffer size by prefix size */ + if (prefix) size += strlen(prefix); + + char *buf = malloc(size); + if (buf) { + + /* Preload buffer with prefix */ + for (i = 0; prefix[i]; i++) buf[i] = prefix[i]; + + /* Load buffer after prefix with concatenated version of argv */ + if (concat_argv(argv, buf + i, size, sep, false)) return buf; + } + free(buf); + return NULL; +} +char * +argv_to_string_alloc(const char *argv[], const char *sep) +{ + size_t i, size = 1; for (i = 0; argv[i]; i++) size += strlen(argv[i]); - buf = malloc(size + 1); - if (buf && argv_to_string(argv, buf, size + 1, sep)) + /* Increase buffer size by separator size and number of separators */ + if (sep) if (i) size += strlen(sep) * (i - 1); + + char *buf = malloc(size); + if (buf && concat_argv(argv, buf, size, sep, false)) return buf; free(buf); return NULL; From b8751cbbaf7de1a04ad4286158de259a31961663 Mon Sep 17 00:00:00 2001 From: Steven Chan Date: Tue, 7 May 2019 11:02:13 -0700 Subject: [PATCH 2/6] Ability to add comments into help view [#809] 1. keys.h Add a run_request.help field, analogous to the req_info.help field. 2. options.h Add a parameter to set_option() for passing any comment text. 3. options.c a. read_option() Identify any #comment text string that may have accompanied an option command, and pass it into set_option(). The #comment text string is trimmed of any white space. b. set_option() set_option() dispatches to specific bind commands. Pass any given comment text into the option_bind_command(), and ignore comment text for the other bind commands. c. option_bind_command() Pass comment text into add_run_request(). d. read_repo_config_option() Specify the option command to use with "color", "bind", or "set" names instead of specific function names. e. set_repo_config_option() Refactor to call set_option(), the dispatcher for specific bind commands, and call with comment text pointer set to NULL, because comment text is never available from a repo configuration file. 4. prompt.c a. run_prompt_command() Call set_option() with comment text pointer set to NULL, because comment text is not parsed on a prompt line. 5. keys.c a. add_run_request() Duplicate comment text into the new run_request.help field. 6. help.c a. help_draw() Draw the run_request.help text using draw_text(), analogous to drawing req_info.help. --- include/tig/keys.h | 3 ++- include/tig/options.h | 2 +- src/help.c | 1 + src/keys.c | 8 +++++++- src/options.c | 36 ++++++++++++++++++------------------ src/prompt.c | 2 +- 6 files changed, 30 insertions(+), 22 deletions(-) diff --git a/include/tig/keys.h b/include/tig/keys.h index 493e58d1a..06158284d 100644 --- a/include/tig/keys.h +++ b/include/tig/keys.h @@ -90,10 +90,11 @@ struct run_request { struct keymap *keymap; struct run_request_flags flags; const char **argv; + char *help; }; struct run_request *get_run_request(enum request request); -enum status_code add_run_request(struct keymap *keymap, const struct key key[], size_t keys, const char **argv); +enum status_code add_run_request(struct keymap *keymap, const struct key key[], size_t keys, const char **argv, const char *help); enum status_code parse_run_request_flags(struct run_request_flags *flags, const char **argv); const char *format_run_request_flags(const struct run_request *req); diff --git a/include/tig/options.h b/include/tig/options.h index c43f4b877..afb8f35a9 100644 --- a/include/tig/options.h +++ b/include/tig/options.h @@ -216,7 +216,7 @@ struct option_info *find_column_option_info(enum view_column_type type, union vi const char *option, struct option_info *column_info, const char **column_name); enum status_code parse_int(int *opt, const char *arg, int min, int max); enum status_code parse_step(double *opt, const char *arg); -enum status_code set_option(const char *opt, int argc, const char *argv[]); +enum status_code set_option(const char *opt, int argc, const char *argv[], const char *help); enum status_code load_options(void); enum status_code load_git_config(void); enum status_code save_options(const char *path); diff --git a/src/help.c b/src/help.c index 1b90cfcc7..d6d6a537a 100644 --- a/src/help.c +++ b/src/help.c @@ -62,6 +62,7 @@ help_draw(struct view *view, struct line *line, unsigned int lineno) return true; sep = " "; } + if (req->help) draw_text(view, LINE_DEFAULT, req->help); } else { const struct request_info *req_info = help->data.req_info; diff --git a/src/keys.c b/src/keys.c index a1c82ef55..c746e9168 100644 --- a/src/keys.c +++ b/src/keys.c @@ -486,7 +486,7 @@ parse_run_request_flags(struct run_request_flags *flags, const char **argv) enum status_code add_run_request(struct keymap *keymap, const struct key key[], - size_t keys, const char **argv) + size_t keys, const char **argv, const char *help) { struct run_request *req; struct run_request_flags flags = {0}; @@ -505,6 +505,12 @@ add_run_request(struct keymap *keymap, const struct key key[], req->flags = flags; req->keymap = keymap; + /* If there is help text, then dupe it into the run_request struct */ + req->help = NULL; + if (help) + if ((req->help = strdup(help)) == NULL) + return ERROR_OUT_OF_MEMORY; + return add_keybinding(keymap, REQ_RUN_REQUESTS + run_requests, key, keys); } diff --git a/src/options.c b/src/options.c index cb1177a8d..58d1c5248 100644 --- a/src/options.c +++ b/src/options.c @@ -790,7 +790,7 @@ option_set_command(int argc, const char *argv[]) /* Wants: mode request key */ static enum status_code -option_bind_command(int argc, const char *argv[]) +option_bind_command(int argc, const char *argv[], const char *help) { struct key key[16]; size_t keys = 0; @@ -870,7 +870,7 @@ option_bind_command(int argc, const char *argv[]) const char *toggle[] = { ":toggle", mapped, arg, NULL}; const char *other[] = { mapped, NULL }; const char **prompt = *mapped == ':' ? other : toggle; - enum status_code code = add_run_request(keymap, key, keys, prompt); + enum status_code code = add_run_request(keymap, key, keys, prompt, NULL); if (code == SUCCESS) code = error("%s has been replaced by `%s%s%s%s'", @@ -882,7 +882,7 @@ option_bind_command(int argc, const char *argv[]) } if (request == REQ_UNKNOWN) - return add_run_request(keymap, key, keys, argv + 2); + return add_run_request(keymap, key, keys, argv + 2, help); return add_keybinding(keymap, request, key, keys); } @@ -916,7 +916,7 @@ option_source_command(int argc, const char *argv[]) } enum status_code -set_option(const char *opt, int argc, const char *argv[]) +set_option(const char *opt, int argc, const char *argv[], const char *help) { if (!strcmp(opt, "color")) return option_color_command(argc, argv); @@ -925,7 +925,7 @@ set_option(const char *opt, int argc, const char *argv[]) return option_set_command(argc, argv); if (!strcmp(opt, "bind")) - return option_bind_command(argc, argv); + return option_bind_command(argc, argv, help); if (!strcmp(opt, "source")) return option_source_command(argc, argv); @@ -957,7 +957,8 @@ read_option(char *opt, size_t optlen, char *value, size_t valuelen, void *data) const char *argv[SIZEOF_ARG]; int argc = 0; - if (len < valuelen) { + bool have_comments = len < valuelen; + if (have_comments) { valuelen = len; value[valuelen] = 0; } @@ -965,7 +966,7 @@ read_option(char *opt, size_t optlen, char *value, size_t valuelen, void *data) if (!argv_from_string(argv, &argc, value)) status = error("Too many option arguments for %s", opt); else - status = set_option(opt, argc, argv); + status = set_option(opt, argc, argv, have_comments ? string_trim(value + len + 1) : NULL); } if (status != SUCCESS) { @@ -1322,16 +1323,15 @@ set_remote_branch(const char *name, const char *value, size_t valuelen) } static void -set_repo_config_option(char *name, char *value, enum status_code (*cmd)(int, const char **)) +set_repo_config_option(char *name, char *value, const char *cmd) { - const char *argv[SIZEOF_ARG] = { name, "=" }; - int argc = 1 + (cmd == option_set_command); - enum status_code code; + const char *argv[SIZEOF_ARG]; int argc; + if (!strcmp(cmd, "set")) { argv[0] = name; argv[1] = "="; argc = 2; } + else { argv[0] = name; argc = 1; } - if (!argv_from_string(argv, &argc, value)) - code = error("Too many arguments"); - else - code = cmd(argc, argv); + enum status_code code = !argv_from_string(argv, &argc, value) + ? error("Too many arguments") + : set_option(cmd, argc, argv, NULL); if (code != SUCCESS) warn("Option 'tig.%s': %s", name, get_status_message(code)); @@ -1443,13 +1443,13 @@ read_repo_config_option(char *name, size_t namelen, char *value, size_t valuelen parse_bool(&opt_status_show_untracked_files, value); else if (!prefixcmp(name, "tig.color.")) - set_repo_config_option(name + 10, value, option_color_command); + set_repo_config_option(name + 10, value, "color"); else if (!prefixcmp(name, "tig.bind.")) - set_repo_config_option(name + 9, value, option_bind_command); + set_repo_config_option(name + 9, value, "bind"); else if (!prefixcmp(name, "tig.")) - set_repo_config_option(name + 4, value, option_set_command); + set_repo_config_option(name + 4, value, "set"); else if (!prefixcmp(name, "color.")) set_git_color_option(name + STRING_SIZE("color."), value); diff --git a/src/prompt.c b/src/prompt.c index 831e4c9c6..f5eee663a 100644 --- a/src/prompt.c +++ b/src/prompt.c @@ -1051,7 +1051,7 @@ run_prompt_command(struct view *view, const char *argv[]) if (request != REQ_UNKNOWN) return request; - code = set_option(argv[0], argv_size(argv + 1), &argv[1]); + code = set_option(cmd, argv_size(argv + 1), &argv[1], NULL); if (code != SUCCESS) { report("%s", get_status_message(code)); return REQ_NONE; From ddae6be8d3563b7f55b5ce3919f3c50b69e3ea69 Mon Sep 17 00:00:00 2001 From: Steven Chan Date: Thu, 30 May 2019 10:26:32 -0700 Subject: [PATCH 3/6] Draw comment text in help view in alignment Add code to align comment text when shown in the help screen. Comment text are drawn on the right-hand side of an 'action' field. Change the calculation of field width: only key bindings with comment text will participate in calculating the field width. Consequently, key bindings with no comments but with unusually long action names are allowed to overflow the field. 1. keys.h Add a name field to struct run_request, analogous to the name field in struct req_info. 2. keys.c a. add_run_request() * Load run_request.name with a displayable version of **argv, using new function argv_to_string_alloc_prefix(). * Include run_request.flags as a string prefix. 3. help.c a. help_keys_visitor() * If run_request.help text is present, update help_state.name_width with strlen(run_request.name), so that drawing run_request.help will be aligned. * We can also do the same for req_info.help text: calculate the maximum field width only if req_info.help text is available. b. help_draw() * Analogous to drawing req_info.name as a LINE_HELP_ACTION, draw run_request.name using draw_field(), which replaces the current method of drawing **argv using draw_formatted(). We do this only if run_request.help text is present, otherwise, we draw run_request.name as free-form text. * Do the same for req_info.name. In this way, the code for drawing both types of requests are more similar. --- include/tig/keys.h | 1 + src/help.c | 34 ++++++++++++++++++++++++---------- src/keys.c | 9 ++++++++- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/include/tig/keys.h b/include/tig/keys.h index 06158284d..bad427ca5 100644 --- a/include/tig/keys.h +++ b/include/tig/keys.h @@ -90,6 +90,7 @@ struct run_request { struct keymap *keymap; struct run_request_flags flags; const char **argv; + char *name; char *help; }; diff --git a/src/help.c b/src/help.c index d6d6a537a..45185a9b9 100644 --- a/src/help.c +++ b/src/help.c @@ -51,18 +51,19 @@ help_draw(struct view *view, struct line *line, unsigned int lineno) } else if (help->request > REQ_RUN_REQUESTS) { struct run_request *req = get_run_request(help->request); const char *key = get_keys(keymap, help->request, true); - const char *sep = format_run_request_flags(req); - int i; if (draw_field(view, LINE_DEFAULT, key, state->keys_width + 2, ALIGN_RIGHT, false)) return true; - for (i = 0; req->argv[i]; i++) { - if (draw_formatted(view, LINE_HELP_ACTION, "%s%s", sep, req->argv[i])) + /* If there is req->help text to draw, then first draw req->name as a fixed-width field */ + if (req->help) { + if (draw_field(view, LINE_HELP_ACTION, req->name, state->name_width, ALIGN_LEFT, false)) return true; - sep = " "; + draw_text(view, LINE_DEFAULT, req->help); } - if (req->help) draw_text(view, LINE_DEFAULT, req->help); + + /* Else just draw req->name as free-form text */ + else draw_text(view, LINE_HELP_ACTION, req->name); } else { const struct request_info *req_info = help->data.req_info; @@ -71,10 +72,15 @@ help_draw(struct view *view, struct line *line, unsigned int lineno) if (draw_field(view, LINE_DEFAULT, key, state->keys_width + 2, ALIGN_RIGHT, false)) return true; - if (draw_field(view, LINE_HELP_ACTION, enum_name(req_info->name), state->name_width, ALIGN_LEFT, false)) - return true; + /* If there is req_info->help text to draw, then first draw req_info->name as a fixed-width field */ + if (req_info->help) { + if (draw_field(view, LINE_HELP_ACTION, enum_name(req_info->name), state->name_width, ALIGN_LEFT, false)) + return true; + draw_text(view, LINE_DEFAULT, req_info->help); + } - draw_text(view, LINE_DEFAULT, req_info->help); + /* Else just draw req_info->name as free-form text */ + else draw_text(view, LINE_HELP_ACTION, enum_name(req_info->name)); } return true; @@ -166,8 +172,16 @@ help_keys_visitor(void *data, const char *group, struct keymap *keymap, help->request = request; if (req_info) { - state->name_width = MAX(state->name_width, strlen(enum_name(req_info->name))); help->data.req_info = req_info; + /* Include req_info->name in the MAX calculation but only if there is help text */ + if (req_info->help && strlen(req_info->help) > 0) + state->name_width = MAX(state->name_width, strlen(enum_name(req_info->name))); + } + + if (run_req) { + /* Include run_req->name in the MAX calculation but only if there is help text */ + if (run_req->help && strlen(run_req->help) > 0) + state->name_width = MAX(state->name_width, strlen(run_req->name)); } return true; diff --git a/src/keys.c b/src/keys.c index c746e9168..8c22d484c 100644 --- a/src/keys.c +++ b/src/keys.c @@ -490,8 +490,8 @@ add_run_request(struct keymap *keymap, const struct key key[], { struct run_request *req; struct run_request_flags flags = {0}; - enum status_code code = parse_run_request_flags(&flags, argv); + enum status_code code = parse_run_request_flags(&flags, argv); if (code != SUCCESS) return code; @@ -505,6 +505,13 @@ add_run_request(struct keymap *keymap, const struct key key[], req->flags = flags; req->keymap = keymap; + /* Duplicate a displayable version of **argv into the run_request struct */ + req->name = NULL; + if (argv) { + if ((req->name = argv_to_string_alloc_prefix(argv, " ", format_run_request_flags(req))) == NULL) + return ERROR_OUT_OF_MEMORY; + } + /* If there is help text, then dupe it into the run_request struct */ req->help = NULL; if (help) From 54db1afe13521e5c4c56580e7a8aafebd26b32de Mon Sep 17 00:00:00 2001 From: Steven Chan Date: Fri, 26 Jul 2019 11:38:50 -0700 Subject: [PATCH 4/6] Update tests 1. help/default-test The action name field is wider causing re-alignment of help text. 2. tigrc/parse-test Comment for a key binding is now exposed as help text. 3. help/user-command-comment-test Add a new test for defining user comments for user commands. --- test/help/user-command-comment-test | 53 +++++++++++++++++++++++++++++ test/tigrc/parse-test | 2 +- 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100755 test/help/user-command-comment-test diff --git a/test/help/user-command-comment-test b/test/help/user-command-comment-test new file mode 100755 index 000000000..20cbce130 --- /dev/null +++ b/test/help/user-command-comment-test @@ -0,0 +1,53 @@ +#!/bin/sh + +. libtest.sh + +export TIGRC_SYSTEM=does-not-exist + +test_case user-command-comment \ + --args='status' \ + --tigrc=" + bind generic : prompt # User comment for a builtin command is not exposed + bind generic , none # User comment does not interfere with a null binding + bind generic u1 !user + bind generic u2 !user # This is a user commentary for a simple user command + bind generic u3 !user 1 2 3 # This is a user commentary for a user command with parameters + bind generic u4 !user command with a long parameter list but no user commentary + " \ + --script=" + :bind generic p1 !true + :view-help + " \ + < Date: Thu, 30 May 2019 10:42:25 -0700 Subject: [PATCH 5/6] Ensure that comment text in all keybindings is searchable 1. help.c a. help_grep() For keybinding for each run request, include any help text in the text line so that it can participate in grep operations. --- src/help.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/help.c b/src/help.c index 45185a9b9..a571f8f94 100644 --- a/src/help.c +++ b/src/help.c @@ -105,11 +105,8 @@ help_grep(struct view *view, struct line *line) } else if (help->request > REQ_RUN_REQUESTS) { struct run_request *req = get_run_request(help->request); const char *key = get_keys(keymap, help->request, true); - char buf[SIZEOF_STR] = ""; - const char *text[] = { key, buf, NULL }; - if (!argv_to_string(req->argv, buf, sizeof(buf), " ")) - return false; + const char *text[] = { key, req->name, req->help, NULL }; return grep_text(view, text); From dcfe1fd2e0a11ae0b5daf4ab469fcca9cdd50099 Mon Sep 17 00:00:00 2001 From: Steven Chan Date: Thu, 30 May 2019 11:07:35 -0700 Subject: [PATCH 6/6] Cache key strings calculated during help_open() so that help_draw() does not need to recalculate them 1. help.c a. struct help Add a key field to cache key strings calculated during help_open() so that help_draw() does not need to recalculate. b. help_keys_visitor() Cache the available key string in help.key. c. help_draw(), help_grep() Use cached help.key string instead of recalculating the key string. d. help_open() Free any key strings that have been cached in the help struct to prevent memory leak during a refresh of help screen. --- src/help.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/help.c b/src/help.c index a571f8f94..07ad2e10e 100644 --- a/src/help.c +++ b/src/help.c @@ -28,6 +28,7 @@ struct help_state { struct help { struct keymap *keymap; enum request request; + const char *key; union { const char *text; const struct request_info *req_info; @@ -50,9 +51,8 @@ help_draw(struct view *view, struct line *line, unsigned int lineno) } else if (help->request > REQ_RUN_REQUESTS) { struct run_request *req = get_run_request(help->request); - const char *key = get_keys(keymap, help->request, true); - if (draw_field(view, LINE_DEFAULT, key, state->keys_width + 2, ALIGN_RIGHT, false)) + if (draw_field(view, LINE_DEFAULT, help->key, state->keys_width + 2, ALIGN_RIGHT, false)) return true; /* If there is req->help text to draw, then first draw req->name as a fixed-width field */ @@ -67,9 +67,8 @@ help_draw(struct view *view, struct line *line, unsigned int lineno) } else { const struct request_info *req_info = help->data.req_info; - const char *key = get_keys(keymap, req_info->request, true); - if (draw_field(view, LINE_DEFAULT, key, state->keys_width + 2, ALIGN_RIGHT, false)) + if (draw_field(view, LINE_DEFAULT, help->key, state->keys_width + 2, ALIGN_RIGHT, false)) return true; /* If there is req_info->help text to draw, then first draw req_info->name as a fixed-width field */ @@ -104,16 +103,13 @@ help_grep(struct view *view, struct line *line) } else if (help->request > REQ_RUN_REQUESTS) { struct run_request *req = get_run_request(help->request); - const char *key = get_keys(keymap, help->request, true); - - const char *text[] = { key, req->name, req->help, NULL }; + const char *text[] = { help->key, req->name, req->help, NULL }; return grep_text(view, text); } else { const struct request_info *req_info = help->data.req_info; - const char *key = get_keys(keymap, req_info->request, true); - const char *text[] = { key, enum_name(req_info->name), req_info->help, NULL }; + const char *text[] = { help->key, enum_name(req_info->name), req_info->help, NULL }; return grep_text(view, text); } @@ -165,7 +161,11 @@ help_keys_visitor(void *data, const char *group, struct keymap *keymap, if (!add_help_line(view, &help, keymap, LINE_DEFAULT)) return false; - state->keys_width = MAX(state->keys_width, strlen(key)); + int len = strlen(key); + state->keys_width = MAX(state->keys_width, len); + /* Since the key string is available, cache it for when help lines need to be drawn */ + help->key = len > 0 ? strdup(key) : NULL; + help->request = request; if (req_info) { @@ -190,6 +190,12 @@ help_open(struct view *view, enum open_flags flags) struct help_request_iterator iterator = { view }; struct help *help; + /* Need to free any key strings that have been cached */ + int i; for (i = 0; i < view->lines; i++) { + const struct help *h = view->line[i].data; + free((void *)h->key); + } + reset_view(view); if (!add_help_line(view, &help, NULL, LINE_HEADER))