From d34e4502fa540ed0654df7dd75b3689c48015f1a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 10 Nov 2020 23:42:11 +0000 Subject: add -i (built-in): do show an error message for incorrect inputs There is a neat feature in `git add -i` where it allows users to select items via unique prefixes. In the built-in version of `git add -i`, we specifically sort the items (unless they are already sorted) and then perform a binary search to figure out whether the input constitutes a unique prefix. Unfortunately, by mistake this code misidentifies matches even if the input string is not actually a prefix of any item. For example, in the initial menu, where there is a `status` and an `update` command, the input `tadaa` was mistaken as a prefix of `update`. Let's fix this by looking a bit closer whether the input is actually a prefix of the item at the found insert index. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- add-interactive.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'add-interactive.c') diff --git a/add-interactive.c b/add-interactive.c index 555c4abf32..8ca503d803 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -194,7 +194,8 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list) else if (index + 1 < list->sorted.nr && starts_with(list->sorted.items[index + 1].string, string)) return -1; - else if (index < list->sorted.nr) + else if (index < list->sorted.nr && + starts_with(list->sorted.items[index].string, string)) item = list->sorted.items[index].util; else return -1; -- cgit v1.2.3 From cb581b16ef947a00f8e83f17c9fbe5b5aeed1d65 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 10 Nov 2020 23:42:12 +0000 Subject: add -i (built-in): send error messages to stderr The Perl version of that command already does that since a301973641f (add -p: print errors in separate color, 2009-02-05). The built-in version's development started by reimplementing the initial version from 5cde71d64af (git-add --interactive, 2006-12-10) for simplicity, though, which still printed error messages to stdout. Let's fix that by imitating the Perl version's behavior in the built-in version of that command. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- add-interactive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'add-interactive.c') diff --git a/add-interactive.c b/add-interactive.c index 8ca503d803..0f24992ca4 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -365,7 +365,7 @@ static ssize_t list_and_choose(struct add_i_state *s, if (from < 0 || from >= items->items.nr || (singleton && from + 1 != to)) { - color_fprintf_ln(stdout, s->error_color, + color_fprintf_ln(stderr, s->error_color, _("Huh (%s)?"), p); break; } else if (singleton) { -- cgit v1.2.3 From c62cd1720f5c9cf57955a78576ba7e862b7352e6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 11 Nov 2020 12:28:18 +0000 Subject: add -i (built-in): prevent the `reset` "color" from being configured The Perl version of that command sneakily uses `git config --get-color` to figure out the ANSI sequence to reset the color, but passes the empty string and therefore cannot actually match any config entry. This was missed when re-implementing the command as a built-in command. Let's fix this, preventing the `reset` sequence from being overridden via the config. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- add-interactive.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'add-interactive.c') diff --git a/add-interactive.c b/add-interactive.c index 0f24992ca4..f3a1d7456e 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -44,7 +44,6 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED); init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE); init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED); - init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET); init_color(r, s, "fraginfo", s->fraginfo_color, diff_get_color(s->use_color, DIFF_FRAGINFO)); init_color(r, s, "context", s->context_color, @@ -54,6 +53,9 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) init_color(r, s, "new", s->file_new_color, diff_get_color(s->use_color, DIFF_FILE_NEW)); + strlcpy(s->reset_color, + s->use_color ? GIT_COLOR_RESET : "", COLOR_MAXLEN); + FREE_AND_NULL(s->interactive_diff_filter); git_config_get_string("interactive.difffilter", &s->interactive_diff_filter); -- cgit v1.2.3 From 25d9e5ccba3fbcdb4109b6b2b2a7e721b0af6d77 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 11 Nov 2020 12:28:19 +0000 Subject: add -i (built-in): use correct names to load color.diff.* config The builtin version of add-interactive mistakenly loads diff colors from color.interactive.* instead of color.diff.*. It also accidentally spells `frag` as `fraginfo`. Let's fix that. Note also that we don't respect the historical `diff.color.*`. The perl version never did, and those have been deprecated since 2007. Reported-by: Philippe Blain Co-authored-by: Jeff King Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- add-interactive.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) (limited to 'add-interactive.c') diff --git a/add-interactive.c b/add-interactive.c index f3a1d7456e..9126684348 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -12,10 +12,10 @@ #include "prompt.h" static void init_color(struct repository *r, struct add_i_state *s, - const char *slot_name, char *dst, + const char *section_and_slot, char *dst, const char *default_color) { - char *key = xstrfmt("color.interactive.%s", slot_name); + char *key = xstrfmt("color.%s", section_and_slot); const char *value; if (!s->use_color) @@ -40,17 +40,20 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) git_config_colorbool("color.interactive", value); s->use_color = want_color(s->use_color); - init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD); - init_color(r, s, "help", s->help_color, GIT_COLOR_BOLD_RED); - init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE); - init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED); - init_color(r, s, "fraginfo", s->fraginfo_color, + init_color(r, s, "interactive.header", s->header_color, GIT_COLOR_BOLD); + init_color(r, s, "interactive.help", s->help_color, GIT_COLOR_BOLD_RED); + init_color(r, s, "interactive.prompt", s->prompt_color, + GIT_COLOR_BOLD_BLUE); + init_color(r, s, "interactive.error", s->error_color, + GIT_COLOR_BOLD_RED); + + init_color(r, s, "diff.frag", s->fraginfo_color, diff_get_color(s->use_color, DIFF_FRAGINFO)); - init_color(r, s, "context", s->context_color, + init_color(r, s, "diff.context", s->context_color, diff_get_color(s->use_color, DIFF_CONTEXT)); - init_color(r, s, "old", s->file_old_color, + init_color(r, s, "diff.old", s->file_old_color, diff_get_color(s->use_color, DIFF_FILE_OLD)); - init_color(r, s, "new", s->file_new_color, + init_color(r, s, "diff.new", s->file_new_color, diff_get_color(s->use_color, DIFF_FILE_NEW)); strlcpy(s->reset_color, -- cgit v1.2.3 From afae3cb6b036e8fb218e5699c8ad3c18b6d4b992 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 16 Nov 2020 16:08:29 +0000 Subject: add -i (built-in): use the same indentation as the Perl version When copying the spaces used to indent non-flat lists in `git add -i`, one space was appended by mistake. This makes the output of the built-in version of `git add -i` inconsistent with the Perl version. Let's adjust the built-in version to produce the same output as the Perl version. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- add-interactive.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'add-interactive.c') diff --git a/add-interactive.c b/add-interactive.c index 9126684348..c298a8b80f 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -1137,7 +1137,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) print_file_item_data.color = data.color; print_file_item_data.reset = data.reset; - strbuf_addstr(&header, " "); + strbuf_addstr(&header, " "); strbuf_addf(&header, print_file_item_data.modified_fmt, _("staged"), _("unstaged"), _("path")); opts.list_opts.header = header.buf; -- cgit v1.2.3 From 890b68b2637fcadb4b1017ecf50248d616c96b8b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 16 Nov 2020 16:08:31 +0000 Subject: add -p: prefer color.diff.context over color.diff.plain Git's diff machinery allows users to override the colors to use in diffs, even the plain-colored context lines. As of 8dbf3eb6850 (diff.h: rename DIFF_PLAIN color slot to DIFF_CONTEXT, 2015-05-27), the preferred name of the config setting is `color.diff.context`, although Git still allows `color.diff.plain`. In the context of `git add -p`, this logic is a bit hard to replicate: `git_diff_basic_config()` reads all config values sequentially and if it sees _any_ `color.diff.context` or `color.diff.plain`, it accepts the new color. The Perl version of `git add -p` needs to go through `git config --get-color`, though, which allows only one key to be specified. The same goes for the built-in version of `git add -p`, which has to go through `repo_config_get_value()`. The best we can do here is to look for `.context` and if none is found, fall back to looking for `.plain`, and if still not found, fall back to the hard-coded default (which in this case is simply the empty string, as context lines are typically rendered without colored). This still leads to inconsistencies when both config names are used: the initial diff will be colored by the diff machinery. Once edited by a user, a hunk has to be re-colored by `git add -p`, though, which would then use the other setting to color the context lines. In practice, this is not _all_ that bad. The `git config` manual says this in the `color.diff.`: `context` (context text - `plain` is a historical synonym) We should therefore assume that users use either one or the other, but not both names. Besides, it is relatively uncommon to look at a hunk after editing it because it is immediately staged by default. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- add-interactive.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'add-interactive.c') diff --git a/add-interactive.c b/add-interactive.c index c298a8b80f..54dfdc56f5 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -49,8 +49,10 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) init_color(r, s, "diff.frag", s->fraginfo_color, diff_get_color(s->use_color, DIFF_FRAGINFO)); - init_color(r, s, "diff.context", s->context_color, - diff_get_color(s->use_color, DIFF_CONTEXT)); + init_color(r, s, "diff.context", s->context_color, "fall back"); + if (!strcmp(s->context_color, "fall back")) + init_color(r, s, "diff.plain", s->context_color, + diff_get_color(s->use_color, DIFF_CONTEXT)); init_color(r, s, "diff.old", s->file_old_color, diff_get_color(s->use_color, DIFF_FILE_OLD)); init_color(r, s, "diff.new", s->file_new_color, -- cgit v1.2.3