From 8c0bcc0ebadbc1ea8c6fa5ac6d29987fb60cc23d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 13:05:42 +0100 Subject: [PATCH 01/61] Start to implement a built-in version of `git add --interactive` This is hardly the first conversion of a Git command that is implemented as a script to a built-in. So far, the most successful strategy for such conversions has been to add a built-in helper and call that for more and more functionality from the script, as more and more parts are converted. With the interactive add, we choose a different strategy. The sole reason for this is that on Windows (where such a conversion has the most benefits in terms of speed and robustness) we face the very specific problem that a `system()` call in Perl seems to close `stdin` in the parent process when the spawned process consumes even one character from `stdin`. And that just does not work for us here, as it would stop the main loop as soon as any interactive command was performed by the helper. Which is almost all of the commands in `git add -i`. It is almost as if Perl told us once again that it does not want us to use it on Windows. Instead, we follow the opposite route where we start with a bare-bones version of the built-in interactive add, guarded by the new `add.interactive.useBuiltin` config variable, and then add more and more functionality to it, until it is feature complete. At this point, the built-in version of `git add -i` only states that it cannot do anything yet ;-) Signed-off-by: Johannes Schindelin --- Documentation/config/add.txt | 5 +++++ Makefile | 1 + add-interactive.c | 7 +++++++ add-interactive.h | 8 ++++++++ builtin/add.c | 10 ++++++++++ t/README | 4 ++++ 6 files changed, 35 insertions(+) create mode 100644 add-interactive.c create mode 100644 add-interactive.h diff --git a/Documentation/config/add.txt b/Documentation/config/add.txt index 4d753f006e..c9f748f81c 100644 --- a/Documentation/config/add.txt +++ b/Documentation/config/add.txt @@ -5,3 +5,8 @@ add.ignore-errors (deprecated):: option of linkgit:git-add[1]. `add.ignore-errors` is deprecated, as it does not follow the usual naming convention for configuration variables. + +add.interactive.useBuiltin:: + [EXPERIMENTAL] Set to `true` to use the experimental built-in + implementation of the interactive version of linkgit:git-add[1] + instead of the Perl script version. Is `false` by default. diff --git a/Makefile b/Makefile index 8a7e235352..9eff9117dd 100644 --- a/Makefile +++ b/Makefile @@ -828,6 +828,7 @@ LIB_H := $(sort $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null -name '*.h' -print)) LIB_OBJS += abspath.o +LIB_OBJS += add-interactive.o LIB_OBJS += advice.o LIB_OBJS += alias.o LIB_OBJS += alloc.o diff --git a/add-interactive.c b/add-interactive.c new file mode 100644 index 0000000000..482e458dc6 --- /dev/null +++ b/add-interactive.c @@ -0,0 +1,7 @@ +#include "cache.h" +#include "add-interactive.h" + +int run_add_i(struct repository *r, const struct pathspec *ps) +{ + die(_("No commands are available in the built-in `git add -i` yet!")); +} diff --git a/add-interactive.h b/add-interactive.h new file mode 100644 index 0000000000..7043b8741d --- /dev/null +++ b/add-interactive.h @@ -0,0 +1,8 @@ +#ifndef ADD_INTERACTIVE_H +#define ADD_INTERACTIVE_H + +struct repository; +struct pathspec; +int run_add_i(struct repository *r, const struct pathspec *ps); + +#endif diff --git a/builtin/add.c b/builtin/add.c index dd18e5c9b6..4f625691b5 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -20,6 +20,7 @@ #include "bulk-checkin.h" #include "argv-array.h" #include "submodule.h" +#include "add-interactive.h" static const char * const builtin_add_usage[] = { N_("git add [] [--] ..."), @@ -185,6 +186,14 @@ int run_add_interactive(const char *revision, const char *patch_mode, { int status, i; struct argv_array argv = ARGV_ARRAY_INIT; + int use_builtin_add_i = + git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1); + if (use_builtin_add_i < 0) + git_config_get_bool("add.interactive.usebuiltin", + &use_builtin_add_i); + + if (use_builtin_add_i == 1 && !patch_mode) + return !!run_add_i(the_repository, pathspec); argv_array_push(&argv, "add--interactive"); if (patch_mode) @@ -319,6 +328,7 @@ static int add_config(const char *var, const char *value, void *cb) ignore_add_errors = git_config_bool(var, value); return 0; } + return git_default_config(var, value, cb); } diff --git a/t/README b/t/README index 6404f33e19..e22edffaff 100644 --- a/t/README +++ b/t/README @@ -388,6 +388,10 @@ GIT_TEST_STASH_USE_BUILTIN=, when false, disables the built-in version of git-stash. See 'stash.useBuiltin' in git-config(1). +GIT_TEST_ADD_I_USE_BUILTIN=, when true, enables the +builtin version of git add -i. See 'add.interactive.useBuiltin' in +git-config(1). + GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading of the index for the whole test suite by bypassing the default number of cache entries and thread minimums. Setting this to 1 will make the From 287421f9abc09246388da9244691e553cbd45a46 Mon Sep 17 00:00:00 2001 From: Daniel Ferreira Date: Tue, 16 May 2017 01:00:31 -0300 Subject: [PATCH 02/61] diff: export diffstat interface Make the diffstat interface (namely, the diffstat_t struct and compute_diffstat) no longer be internal to diff.c and allow it to be used by other parts of git. This is helpful for code that may want to easily extract information from files using the diff machinery, while flushing it differently from how the show_* functions used by diff_flush() do it. One example is the builtin implementation of git-add--interactive's status. Signed-off-by: Daniel Ferreira Signed-off-by: Slavica Djukic Signed-off-by: Johannes Schindelin --- diff.c | 37 +++++++++++++++---------------------- diff.h | 19 +++++++++++++++++++ 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/diff.c b/diff.c index 4d3cf83a27..7d0e15dd99 100644 --- a/diff.c +++ b/diff.c @@ -2489,22 +2489,6 @@ static void pprint_rename(struct strbuf *name, const char *a, const char *b) } } -struct diffstat_t { - int nr; - int alloc; - struct diffstat_file { - char *from_name; - char *name; - char *print_name; - const char *comments; - unsigned is_unmerged:1; - unsigned is_binary:1; - unsigned is_renamed:1; - unsigned is_interesting:1; - uintmax_t added, deleted; - } **files; -}; - static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat, const char *name_a, const char *name_b) @@ -6249,12 +6233,7 @@ void diff_flush(struct diff_options *options) dirstat_by_line) { struct diffstat_t diffstat; - memset(&diffstat, 0, sizeof(struct diffstat_t)); - for (i = 0; i < q->nr; i++) { - struct diff_filepair *p = q->queue[i]; - if (check_pair_status(p)) - diff_flush_stat(p, options, &diffstat); - } + compute_diffstat(options, &diffstat, q); if (output_format & DIFF_FORMAT_NUMSTAT) show_numstat(&diffstat, options); if (output_format & DIFF_FORMAT_DIFFSTAT) @@ -6587,6 +6566,20 @@ static int is_submodule_ignored(const char *path, struct diff_options *options) return ignored; } +void compute_diffstat(struct diff_options *options, + struct diffstat_t *diffstat, + struct diff_queue_struct *q) +{ + int i; + + memset(diffstat, 0, sizeof(struct diffstat_t)); + for (i = 0; i < q->nr; i++) { + struct diff_filepair *p = q->queue[i]; + if (check_pair_status(p)) + diff_flush_stat(p, options, diffstat); + } +} + void diff_addremove(struct diff_options *options, int addremove, unsigned mode, const struct object_id *oid, diff --git a/diff.h b/diff.h index b20cbcc091..d2954d13ae 100644 --- a/diff.h +++ b/diff.h @@ -244,6 +244,22 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err); void diff_emit_submodule_pipethrough(struct diff_options *o, const char *line, int len); +struct diffstat_t { + int nr; + int alloc; + struct diffstat_file { + char *from_name; + char *name; + char *print_name; + const char *comments; + unsigned is_unmerged:1; + unsigned is_binary:1; + unsigned is_renamed:1; + unsigned is_interesting:1; + uintmax_t added, deleted; + } **files; +}; + enum color_diff { DIFF_RESET = 0, DIFF_CONTEXT = 1, @@ -333,6 +349,9 @@ void diff_change(struct diff_options *, struct diff_filepair *diff_unmerge(struct diff_options *, const char *path); +void compute_diffstat(struct diff_options *options, struct diffstat_t *diffstat, + struct diff_queue_struct *q); + #define DIFF_SETUP_REVERSE 1 #define DIFF_SETUP_USE_SIZE_CACHE 4 From 554af1dc263afe7b61662c559be42120fb464f35 Mon Sep 17 00:00:00 2001 From: Daniel Ferreira Date: Tue, 16 May 2017 01:00:32 -0300 Subject: [PATCH 03/61] built-in add -i: implement the `status` command This implements the `status` command of `git add -i`. The data structures introduced in this commit will be extended as needed later. At this point, we re-implement only part of the `list_and_choose()` function of the Perl script `git-add--interactive.perl` and call it `list()`. It does not yet color anything, or do columns, or allow user input. Over the course of the next commits, we will introduce a `list_and_choose()` function that uses `list()` to display the list of options and let the user choose one or more of the displayed items. This will be used to implement the main loop of the built-in `git add -i`, at which point the new `status` command can actually be used. Note that we pass the list of items as a `struct item **` as opposed to a `struct item *`, to allow for the actual items to contain much more information than merely the name. Signed-off-by: Daniel Ferreira Signed-off-by: Slavica Djukic Signed-off-by: Johannes Schindelin --- add-interactive.c | 265 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 264 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index 482e458dc6..59b28011f7 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -1,7 +1,270 @@ #include "cache.h" #include "add-interactive.h" +#include "diffcore.h" +#include "revision.h" +#include "refs.h" + +struct item { + const char *name; +}; + +struct list_options { + const char *header; + void (*print_item)(int i, struct item *item, void *print_item_data); + void *print_item_data; +}; + +static void list(struct item **list, size_t nr, struct list_options *opts) +{ + int i; + + if (!nr) + return; + + if (opts->header) + printf("%s\n", opts->header); + + for (i = 0; i < nr; i++) { + opts->print_item(i, list[i], opts->print_item_data); + putchar('\n'); + } +} + +struct adddel { + uintmax_t add, del; + unsigned seen:1, binary:1; +}; + +struct file_list { + struct file_item { + struct item item; + struct adddel index, worktree; + } **file; + size_t nr, alloc; +}; + +static void add_file_item(struct file_list *list, const char *name) +{ + struct file_item *item; + + FLEXPTR_ALLOC_STR(item, item.name, name); + + ALLOC_GROW(list->file, list->nr + 1, list->alloc); + list->file[list->nr++] = item; +} + +static void reset_file_list(struct file_list *list) +{ + size_t i; + + for (i = 0; i < list->nr; i++) + free(list->file[i]); + list->nr = 0; +} + +static void release_file_list(struct file_list *list) +{ + reset_file_list(list); + FREE_AND_NULL(list->file); + list->alloc = 0; +} + +static int file_item_cmp(const void *a, const void *b) +{ + const struct file_item * const *f1 = a; + const struct file_item * const *f2 = b; + + return strcmp((*f1)->item.name, (*f2)->item.name); +} + +struct pathname_entry { + struct hashmap_entry ent; + size_t index; + char pathname[FLEX_ARRAY]; +}; + +static int pathname_entry_cmp(const void *unused_cmp_data, + const void *entry, const void *entry_or_key, + const void *pathname) +{ + const struct pathname_entry *e1 = entry, *e2 = entry_or_key; + + return strcmp(e1->pathname, + pathname ? (const char *)pathname : e2->pathname); +} + +struct collection_status { + enum { FROM_WORKTREE = 0, FROM_INDEX = 1 } phase; + + const char *reference; + + struct file_list *list; + struct hashmap file_map; +}; + +static void collect_changes_cb(struct diff_queue_struct *q, + struct diff_options *options, + void *data) +{ + struct collection_status *s = data; + struct diffstat_t stat = { 0 }; + int i; + + if (!q->nr) + return; + + compute_diffstat(options, &stat, q); + + for (i = 0; i < stat.nr; i++) { + const char *name = stat.files[i]->name; + int hash = strhash(name); + struct pathname_entry *entry; + size_t file_index; + struct file_item *file; + struct adddel *adddel; + + entry = hashmap_get_from_hash(&s->file_map, hash, name); + if (entry) + file_index = entry->index; + else { + FLEX_ALLOC_STR(entry, pathname, name); + hashmap_entry_init(entry, hash); + entry->index = file_index = s->list->nr; + hashmap_add(&s->file_map, entry); + + add_file_item(s->list, name); + } + file = s->list->file[file_index]; + + adddel = s->phase == FROM_INDEX ? &file->index : &file->worktree; + adddel->seen = 1; + adddel->add = stat.files[i]->added; + adddel->del = stat.files[i]->deleted; + if (stat.files[i]->is_binary) + adddel->binary = 1; + } +} + +static int get_modified_files(struct repository *r, struct file_list *list, + const struct pathspec *ps) +{ + struct object_id head_oid; + int is_initial = !resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, + &head_oid, NULL); + struct collection_status s = { FROM_WORKTREE }; + + if (repo_read_index_preload(r, ps, 0) < 0) + return error(_("could not read index")); + + s.list = list; + hashmap_init(&s.file_map, pathname_entry_cmp, NULL, 0); + + for (s.phase = FROM_WORKTREE; s.phase <= FROM_INDEX; s.phase++) { + struct rev_info rev; + struct setup_revision_opt opt = { 0 }; + + opt.def = is_initial ? + empty_tree_oid_hex() : oid_to_hex(&head_oid); + + init_revisions(&rev, NULL); + setup_revisions(0, NULL, &rev, &opt); + + rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; + rev.diffopt.format_callback = collect_changes_cb; + rev.diffopt.format_callback_data = &s; + + if (ps) + copy_pathspec(&rev.prune_data, ps); + + if (s.phase == FROM_INDEX) + run_diff_index(&rev, 1); + else { + rev.diffopt.flags.ignore_dirty_submodules = 1; + run_diff_files(&rev, 0); + } + } + hashmap_free(&s.file_map, 1); + + /* While the diffs are ordered already, we ran *two* diffs... */ + QSORT(list->file, list->nr, file_item_cmp); + + return 0; +} + +static void populate_wi_changes(struct strbuf *buf, + struct adddel *ad, const char *no_changes) +{ + if (ad->binary) + strbuf_addstr(buf, _("binary")); + else if (ad->seen) + strbuf_addf(buf, "+%"PRIuMAX"/-%"PRIuMAX, + (uintmax_t)ad->add, (uintmax_t)ad->del); + else + strbuf_addstr(buf, no_changes); +} + +struct print_file_item_data { + const char *modified_fmt; + struct strbuf buf, index, worktree; +}; + +static void print_file_item(int i, struct item *item, + void *print_file_item_data) +{ + struct file_item *c = (struct file_item *)item; + struct print_file_item_data *d = print_file_item_data; + + strbuf_reset(&d->index); + strbuf_reset(&d->worktree); + strbuf_reset(&d->buf); + + populate_wi_changes(&d->worktree, &c->worktree, _("nothing")); + populate_wi_changes(&d->index, &c->index, _("unchanged")); + strbuf_addf(&d->buf, d->modified_fmt, + d->index.buf, d->worktree.buf, item->name); + + printf(" %2d: %s", i + 1, d->buf.buf); +} + +static int run_status(struct repository *r, const struct pathspec *ps, + struct file_list *files, struct list_options *opts) +{ + reset_file_list(files); + + if (get_modified_files(r, files, ps) < 0) + return -1; + + if (files->nr) + list((struct item **)files->file, files->nr, opts); + putchar('\n'); + + return 0; +} int run_add_i(struct repository *r, const struct pathspec *ps) { - die(_("No commands are available in the built-in `git add -i` yet!")); + struct print_file_item_data print_file_item_data = { + "%12s %12s %s", STRBUF_INIT, STRBUF_INIT, STRBUF_INIT + }; + struct list_options opts = { + NULL, print_file_item, &print_file_item_data + }; + struct strbuf header = STRBUF_INIT; + struct file_list files = { NULL }; + int res = 0; + + strbuf_addstr(&header, " "); + strbuf_addf(&header, print_file_item_data.modified_fmt, + _("staged"), _("unstaged"), _("path")); + opts.header = header.buf; + + res = run_status(r, ps, &files, &opts); + + release_file_list(&files); + strbuf_release(&print_file_item_data.buf); + strbuf_release(&print_file_item_data.index); + strbuf_release(&print_file_item_data.worktree); + strbuf_release(&header); + + return res; } From 3fd4ac32c5b4d9edfdb34d660c358f81c570d2d0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 27 Mar 2019 23:46:46 +0100 Subject: [PATCH 04/61] built-in add -i: refresh the index before running `status` This is what the Perl version does, and therefore it is what the built-in version should do, too. Signed-off-by: Johannes Schindelin --- add-interactive.c | 4 +++- repository.c | 19 +++++++++++++++++++ repository.h | 7 +++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index 59b28011f7..2dbf29dee2 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -258,7 +258,9 @@ int run_add_i(struct repository *r, const struct pathspec *ps) _("staged"), _("unstaged"), _("path")); opts.header = header.buf; - res = run_status(r, ps, &files, &opts); + repo_refresh_and_write_index(r, REFRESH_QUIET, 1); + if (run_status(r, ps, &files, &opts) < 0) + res = -1; release_file_list(&files); strbuf_release(&print_file_item_data.buf); diff --git a/repository.c b/repository.c index 682c239fe3..def35c40fc 100644 --- a/repository.c +++ b/repository.c @@ -275,3 +275,22 @@ int repo_hold_locked_index(struct repository *repo, BUG("the repo hasn't been setup"); return hold_lock_file_for_update(lf, repo->index_file, flags); } + +int repo_refresh_and_write_index(struct repository *r, + unsigned int flags, int gentle) +{ + struct lock_file lock_file = LOCK_INIT; + int fd; + + if (repo_read_index_preload(r, NULL, 0) < 0) + return error(_("could not read index")); + fd = repo_hold_locked_index(r, &lock_file, 0); + if (!gentle && fd < 0) + return error(_("could not lock index for writing")); + refresh_index(r->index, flags, NULL, NULL, NULL); + if (0 <= fd) + repo_update_index_if_able(r, &lock_file); + rollback_lock_file(&lock_file); + + return 0; +} diff --git a/repository.h b/repository.h index 4fb6a5885f..cf5d5bab48 100644 --- a/repository.h +++ b/repository.h @@ -157,5 +157,12 @@ int repo_read_index_unmerged(struct repository *); */ void repo_update_index_if_able(struct repository *, struct lock_file *); +/* + * Refresh the index and write it out. If the index file could not be + * locked, error out, except in gentle mode. The flags will be passed + * through to refresh_index(). + */ +int repo_refresh_and_write_index(struct repository *r, + unsigned int flags, int gentle); #endif /* REPOSITORY_H */ From 8c20fa7e7f3372ad4f2cc97d3b58afdc3517c7f4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 5 May 2019 23:10:52 +0200 Subject: [PATCH 05/61] built-in add -i: color the header in the `status` command For simplicity, we only implemented the `status` command without colors. This patch starts adding color, matching what the Perl script `git-add--interactive.perl` does. Original-Patch-By: Daniel Ferreira Signed-off-by: Slavica Djukic Signed-off-by: Johannes Schindelin --- add-interactive.c | 60 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 54 insertions(+), 6 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 2dbf29dee2..6c2fca12c1 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -1,9 +1,51 @@ #include "cache.h" #include "add-interactive.h" +#include "color.h" +#include "config.h" #include "diffcore.h" #include "revision.h" #include "refs.h" +struct add_i_state { + struct repository *r; + int use_color; + char header_color[COLOR_MAXLEN]; +}; + +static void init_color(struct repository *r, struct add_i_state *s, + const char *slot_name, char *dst, + const char *default_color) +{ + char *key = xstrfmt("color.interactive.%s", slot_name); + const char *value; + + if (!s->use_color) + dst[0] = '\0'; + else if (repo_config_get_value(r, key, &value) || + color_parse(value, dst)) + strlcpy(dst, default_color, COLOR_MAXLEN); + + free(key); +} + +static int init_add_i_state(struct repository *r, struct add_i_state *s) +{ + const char *value; + + s->r = r; + + if (repo_config_get_value(r, "color.interactive", &value)) + s->use_color = -1; + else + s->use_color = + 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); + + return 0; +} + struct item { const char *name; }; @@ -14,7 +56,8 @@ struct list_options { void *print_item_data; }; -static void list(struct item **list, size_t nr, struct list_options *opts) +static void list(struct item **list, size_t nr, + struct add_i_state *s, struct list_options *opts) { int i; @@ -22,7 +65,8 @@ static void list(struct item **list, size_t nr, struct list_options *opts) return; if (opts->header) - printf("%s\n", opts->header); + color_fprintf_ln(stdout, s->header_color, + "%s", opts->header); for (i = 0; i < nr; i++) { opts->print_item(i, list[i], opts->print_item_data); @@ -226,16 +270,16 @@ static void print_file_item(int i, struct item *item, printf(" %2d: %s", i + 1, d->buf.buf); } -static int run_status(struct repository *r, const struct pathspec *ps, +static int run_status(struct add_i_state *s, const struct pathspec *ps, struct file_list *files, struct list_options *opts) { reset_file_list(files); - if (get_modified_files(r, files, ps) < 0) + if (get_modified_files(s->r, files, ps) < 0) return -1; if (files->nr) - list((struct item **)files->file, files->nr, opts); + list((struct item **)files->file, files->nr, s, opts); putchar('\n'); return 0; @@ -243,6 +287,7 @@ static int run_status(struct repository *r, const struct pathspec *ps, int run_add_i(struct repository *r, const struct pathspec *ps) { + struct add_i_state s = { NULL }; struct print_file_item_data print_file_item_data = { "%12s %12s %s", STRBUF_INIT, STRBUF_INIT, STRBUF_INIT }; @@ -253,13 +298,16 @@ int run_add_i(struct repository *r, const struct pathspec *ps) struct file_list files = { NULL }; int res = 0; + if (init_add_i_state(r, &s)) + return error("could not parse `add -i` config"); + strbuf_addstr(&header, " "); strbuf_addf(&header, print_file_item_data.modified_fmt, _("staged"), _("unstaged"), _("path")); opts.header = header.buf; repo_refresh_and_write_index(r, REFRESH_QUIET, 1); - if (run_status(r, ps, &files, &opts) < 0) + if (run_status(&s, ps, &files, &opts) < 0) res = -1; release_file_list(&files); From 573d0b574c492f33a6e004f438b412457bd56270 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 13:05:42 +0100 Subject: [PATCH 06/61] built-in add -i: implement the main loop The reason why we did not start with the main loop to begin with is that it is the first user of `list_and_choose()`, which uses the `list()` function that we conveniently introduced for use by the `status` command. Apart from the "and choose" part, there are more differences between the way the `status` command calls the `list_and_choose()` function in the Perl version of `git add -i` compared to the other callers of said function. The most important ones: - The list is not only shown, but the user is also asked to make a choice, possibly selecting multiple entries. - The list of items is prefixed with a marker indicating what items have been selected, if multi-selection is allowed. - Initially, for each item a unique prefix (if there exists any within the given parameters) is determined, and shown in the list, and accepted as a shortcut for the selection. These features will be implemented later, except the part where the user can choose a command. At this stage, though, the built-in `git add -i` still only supports the `status` command, with the remaining commands to follow over the course of the next commits. In addition, we also modify `list()` to support displaying the commands in columns, even if there is currently only one. The Perl script `git-add--interactive.perl` mixed the purposes of the "list" and the "and choose" part into the same function. In the C version, we will keep them separate instead, calling the `list()` function from the `list_and_choose()` function. Note that we only have a prompt ending in a single ">" at this stage; later commits will add commands that display a double ">>" to indicate that the user is in a different loop than the main one. Signed-off-by: Johannes Schindelin --- add-interactive.c | 131 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 128 insertions(+), 3 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 6c2fca12c1..359f1e2e0a 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -51,6 +51,7 @@ struct item { }; struct list_options { + int columns; const char *header; void (*print_item)(int i, struct item *item, void *print_item_data); void *print_item_data; @@ -59,7 +60,7 @@ struct list_options { static void list(struct item **list, size_t nr, struct add_i_state *s, struct list_options *opts) { - int i; + int i, last_lf = 0; if (!nr) return; @@ -70,8 +71,97 @@ static void list(struct item **list, size_t nr, for (i = 0; i < nr; i++) { opts->print_item(i, list[i], opts->print_item_data); - putchar('\n'); + + if ((opts->columns) && ((i + 1) % (opts->columns))) { + putchar('\t'); + last_lf = 0; + } + else { + putchar('\n'); + last_lf = 1; + } } + + if (!last_lf) + putchar('\n'); +} +struct list_and_choose_options { + struct list_options list_opts; + + const char *prompt; +}; + +#define LIST_AND_CHOOSE_ERROR (-1) +#define LIST_AND_CHOOSE_QUIT (-2) + +/* + * Returns the selected index. + * + * If an error occurred, returns `LIST_AND_CHOOSE_ERROR`. Upon EOF, + * `LIST_AND_CHOOSE_QUIT` is returned. + */ +static ssize_t list_and_choose(struct item **items, size_t nr, + struct add_i_state *s, + struct list_and_choose_options *opts) +{ + struct strbuf input = STRBUF_INIT; + ssize_t res = LIST_AND_CHOOSE_ERROR; + + for (;;) { + char *p, *endp; + + strbuf_reset(&input); + + list(items, nr, s, &opts->list_opts); + + printf("%s%s", opts->prompt, "> "); + fflush(stdout); + + if (strbuf_getline(&input, stdin) == EOF) { + putchar('\n'); + res = LIST_AND_CHOOSE_QUIT; + break; + } + strbuf_trim(&input); + + if (!input.len) + break; + + p = input.buf; + for (;;) { + size_t sep = strcspn(p, " \t\r\n,"); + ssize_t index = -1; + + if (!sep) { + if (!*p) + break; + p++; + continue; + } + + if (isdigit(*p)) { + index = strtoul(p, &endp, 10) - 1; + if (endp != p + sep) + index = -1; + } + + p[sep] = '\0'; + if (index < 0 || index >= nr) + printf(_("Huh (%s)?\n"), p); + else { + res = index; + break; + } + + p += sep + 1; + } + + if (res != LIST_AND_CHOOSE_ERROR) + break; + } + + strbuf_release(&input); + return res; } struct adddel { @@ -285,17 +375,40 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, return 0; } +static void print_command_item(int i, struct item *item, + void *print_command_item_data) +{ + printf(" %2d: %s", i + 1, item->name); +} + +struct command_item { + struct item item; + int (*command)(struct add_i_state *s, const struct pathspec *ps, + struct file_list *files, struct list_options *opts); +}; + int run_add_i(struct repository *r, const struct pathspec *ps) { struct add_i_state s = { NULL }; + struct list_and_choose_options main_loop_opts = { + { 4, N_("*** Commands ***"), print_command_item, NULL }, + N_("What now") + }; + struct command_item + status = { { "status" }, run_status }; + struct command_item *commands[] = { + &status + }; + struct print_file_item_data print_file_item_data = { "%12s %12s %s", STRBUF_INIT, STRBUF_INIT, STRBUF_INIT }; struct list_options opts = { - NULL, print_file_item, &print_file_item_data + 0, NULL, print_file_item, &print_file_item_data }; struct strbuf header = STRBUF_INIT; struct file_list files = { NULL }; + ssize_t i; int res = 0; if (init_add_i_state(r, &s)) @@ -310,6 +423,18 @@ int run_add_i(struct repository *r, const struct pathspec *ps) if (run_status(&s, ps, &files, &opts) < 0) res = -1; + for (;;) { + i = list_and_choose((struct item **)commands, + ARRAY_SIZE(commands), &s, &main_loop_opts); + if (i == LIST_AND_CHOOSE_QUIT) + printf(_("Bye.\n")); + res = 0; + break; + } + if (i != LIST_AND_CHOOSE_ERROR) + res = commands[i]->command(&s, ps, &files, &opts); + } + release_file_list(&files); strbuf_release(&print_file_item_data.buf); strbuf_release(&print_file_item_data.index); From ef2b69a8252d51f6f34de6751f464ded15d9e020 Mon Sep 17 00:00:00 2001 From: Slavica Djukic Date: Wed, 27 Mar 2019 01:08:50 +0100 Subject: [PATCH 07/61] Add a function to determine unique prefixes for a list of strings In the `git add -i` command, we show unique prefixes of the commands and files, to give an indication what prefix would select them. Naturally, the C implementation looks a lot different than the Perl implementation: in Perl, a trie is much easier implemented, while we already have a pretty neat hashmap implementation in C that we use for the purpose of storing (not necessarily unique) prefixes. The idea: for each item that we add, we generate prefixes starting with the first letter, then the first two letters, then three, etc, until we find a prefix that is unique (or until the prefix length would be longer than we want). If we encounter a previously-unique prefix on the way, we adjust that item's prefix to make it unique again (or we mark it as having no unique prefix if we failed to find one). These partial prefixes are stored in a hash map (for quick lookup times). To make sure that this function works as expected, we add a test using a special-purpose test helper that was added for that purpose. Note: We expect the list of prefix items to be passed in as a list of pointers rather than as regular list to avoid having to copy information (the actual items will most likely contain more information than just the name and the length of the unique prefix, but passing in `struct prefix_item *` would not allow for that). Signed-off-by: Slavica Djukic Signed-off-by: Johannes Schindelin --- Makefile | 2 + prefix-map.c | 109 +++++++++++++++++++++++++++++++++++++ prefix-map.h | 40 ++++++++++++++ t/helper/test-prefix-map.c | 58 ++++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0016-prefix-map.sh | 10 ++++ 7 files changed, 221 insertions(+) create mode 100644 prefix-map.c create mode 100644 prefix-map.h create mode 100644 t/helper/test-prefix-map.c create mode 100755 t/t0016-prefix-map.sh diff --git a/Makefile b/Makefile index 9eff9117dd..a350f63ab0 100644 --- a/Makefile +++ b/Makefile @@ -731,6 +731,7 @@ TEST_BUILTINS_OBJS += test-online-cpus.o TEST_BUILTINS_OBJS += test-parse-options.o TEST_BUILTINS_OBJS += test-path-utils.o TEST_BUILTINS_OBJS += test-pkt-line.o +TEST_BUILTINS_OBJS += test-prefix-map.o TEST_BUILTINS_OBJS += test-prio-queue.o TEST_BUILTINS_OBJS += test-reach.o TEST_BUILTINS_OBJS += test-read-cache.o @@ -947,6 +948,7 @@ LIB_OBJS += patch-ids.o LIB_OBJS += path.o LIB_OBJS += pathspec.o LIB_OBJS += pkt-line.o +LIB_OBJS += prefix-map.o LIB_OBJS += preload-index.o LIB_OBJS += pretty.o LIB_OBJS += prio-queue.o diff --git a/prefix-map.c b/prefix-map.c new file mode 100644 index 0000000000..747ddb4ebc --- /dev/null +++ b/prefix-map.c @@ -0,0 +1,109 @@ +#include "cache.h" +#include "prefix-map.h" + +static int map_cmp(const void *unused_cmp_data, + const void *entry, + const void *entry_or_key, + const void *unused_keydata) +{ + const struct prefix_map_entry *a = entry; + const struct prefix_map_entry *b = entry_or_key; + + return a->prefix_length != b->prefix_length || + strncmp(a->name, b->name, a->prefix_length); +} + +static void add_prefix_entry(struct hashmap *map, const char *name, + size_t prefix_length, struct prefix_item *item) +{ + struct prefix_map_entry *result = xmalloc(sizeof(*result)); + result->name = name; + result->prefix_length = prefix_length; + result->item = item; + hashmap_entry_init(result, memhash(name, prefix_length)); + hashmap_add(map, result); +} + +static void init_prefix_map(struct prefix_map *prefix_map, + int min_prefix_length, int max_prefix_length) +{ + hashmap_init(&prefix_map->map, map_cmp, NULL, 0); + prefix_map->min_length = min_prefix_length; + prefix_map->max_length = max_prefix_length; +} + +static void add_prefix_item(struct prefix_map *prefix_map, + struct prefix_item *item) +{ + struct prefix_map_entry e = { { NULL } }, *e2; + int j; + + e.item = item; + e.name = item->name; + + for (j = prefix_map->min_length; + j <= prefix_map->max_length && e.name[j]; j++) { + /* Avoid breaking UTF-8 multi-byte sequences */ + if (!isascii(e.name[j])) + break; + + e.prefix_length = j; + hashmap_entry_init(&e, memhash(e.name, j)); + e2 = hashmap_get(&prefix_map->map, &e, NULL); + if (!e2) { + /* prefix is unique at this stage */ + item->prefix_length = j; + add_prefix_entry(&prefix_map->map, e.name, j, item); + break; + } + + if (!e2->item) + continue; /* non-unique prefix */ + + if (j != e2->item->prefix_length || memcmp(e.name, e2->name, j)) + BUG("unexpected prefix length: %d != %d (%s != %s)", + j, (int)e2->item->prefix_length, e.name, e2->name); + + /* skip common prefix */ + for (; j < prefix_map->max_length && e.name[j]; j++) { + if (e.item->name[j] != e2->item->name[j]) + break; + add_prefix_entry(&prefix_map->map, e.name, j + 1, + NULL); + } + + /* e2 no longer refers to a unique prefix */ + if (j < prefix_map->max_length && e2->name[j]) { + /* found a new unique prefix for e2's item */ + e2->item->prefix_length = j + 1; + add_prefix_entry(&prefix_map->map, e2->name, j + 1, + e2->item); + } + else + e2->item->prefix_length = 0; + e2->item = NULL; + + if (j < prefix_map->max_length && e.name[j]) { + /* found a unique prefix for the item */ + e.item->prefix_length = j + 1; + add_prefix_entry(&prefix_map->map, e.name, j + 1, + e.item); + } else + /* item has no (short enough) unique prefix */ + e.item->prefix_length = 0; + + break; + } +} + +void find_unique_prefixes(struct prefix_item **list, size_t nr, + int min_length, int max_length) +{ + int i; + struct prefix_map prefix_map; + + init_prefix_map(&prefix_map, min_length, max_length); + for (i = 0; i < nr; i++) + add_prefix_item(&prefix_map, list[i]); + hashmap_free(&prefix_map.map, 1); +} diff --git a/prefix-map.h b/prefix-map.h new file mode 100644 index 0000000000..ce3b8a4a32 --- /dev/null +++ b/prefix-map.h @@ -0,0 +1,40 @@ +#ifndef PREFIX_MAP_H +#define PREFIX_MAP_H + +#include "hashmap.h" + +struct prefix_item { + const char *name; + size_t prefix_length; +}; + +struct prefix_map_entry { + struct hashmap_entry e; + const char *name; + size_t prefix_length; + /* if item is NULL, the prefix is not unique */ + struct prefix_item *item; +}; + +struct prefix_map { + struct hashmap map; + int min_length, max_length; +}; + +/* + * Find unique prefixes in a given list of strings. + * + * Typically, the `struct prefix_item` information will be but a field in the + * actual item struct; For this reason, the `list` parameter is specified as a + * list of pointers to the items. + * + * The `min_length`/`max_length` parameters define what length the unique + * prefixes should have. + * + * If no unique prefix could be found for a given item, its `prefix_length` + * will be set to 0. + */ +void find_unique_prefixes(struct prefix_item **list, size_t nr, + int min_length, int max_length); + +#endif diff --git a/t/helper/test-prefix-map.c b/t/helper/test-prefix-map.c new file mode 100644 index 0000000000..3f1c90eaf0 --- /dev/null +++ b/t/helper/test-prefix-map.c @@ -0,0 +1,58 @@ +#include "test-tool.h" +#include "cache.h" +#include "prefix-map.h" + +static size_t test_count, failed_count; + +static void check(int succeeded, const char *file, size_t line_no, + const char *fmt, ...) +{ + va_list ap; + + test_count++; + if (succeeded) + return; + + va_start(ap, fmt); + fprintf(stderr, "%s:%d: ", file, (int)line_no); + vfprintf(stderr, fmt, ap); + fputc('\n', stderr); + va_end(ap); + + failed_count++; +} + +#define EXPECT_SIZE_T_EQUALS(expect, actual, hint) \ + check(expect == actual, __FILE__, __LINE__, \ + "size_t's do not match: %" \ + PRIdMAX " != %" PRIdMAX " (%s) (%s)", \ + (intmax_t)expect, (intmax_t)actual, #actual, hint) + +int cmd__prefix_map(int argc, const char **argv) +{ +#define NR 5 + struct prefix_item items[NR] = { + { "unique" }, + { "hell" }, + { "hello" }, + { "wok" }, + { "world" }, + }; + struct prefix_item *list[NR] = { + items, items + 1, items + 2, items + 3, items + 4 + }; + + find_unique_prefixes(list, NR, 1, 3); + +#define EXPECT_PREFIX_LENGTH_EQUALS(expect, index) \ + EXPECT_SIZE_T_EQUALS(expect, list[index]->prefix_length, \ + list[index]->name) + + EXPECT_PREFIX_LENGTH_EQUALS(1, 0); + EXPECT_PREFIX_LENGTH_EQUALS(0, 1); + EXPECT_PREFIX_LENGTH_EQUALS(0, 2); + EXPECT_PREFIX_LENGTH_EQUALS(3, 3); + EXPECT_PREFIX_LENGTH_EQUALS(3, 4); + + return !!failed_count; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 087a8c0cc9..e076771b96 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -39,6 +39,7 @@ static struct test_cmd cmds[] = { { "parse-options", cmd__parse_options }, { "path-utils", cmd__path_utils }, { "pkt-line", cmd__pkt_line }, + { "prefix-map", cmd__prefix_map }, { "prio-queue", cmd__prio_queue }, { "reach", cmd__reach }, { "read-cache", cmd__read_cache }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 7e703f3038..891bac703b 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -29,6 +29,7 @@ int cmd__online_cpus(int argc, const char **argv); int cmd__parse_options(int argc, const char **argv); int cmd__path_utils(int argc, const char **argv); int cmd__pkt_line(int argc, const char **argv); +int cmd__prefix_map(int argc, const char **argv); int cmd__prio_queue(int argc, const char **argv); int cmd__reach(int argc, const char **argv); int cmd__read_cache(int argc, const char **argv); diff --git a/t/t0016-prefix-map.sh b/t/t0016-prefix-map.sh new file mode 100755 index 0000000000..187fa92aec --- /dev/null +++ b/t/t0016-prefix-map.sh @@ -0,0 +1,10 @@ +#!/bin/sh + +test_description='basic tests for prefix map' +. ./test-lib.sh + +test_expect_success 'prefix map' ' + test-tool prefix-map +' + +test_done From 2f0b1c6ee7062ab949e7b0a98c9af4c1f6ac00d2 Mon Sep 17 00:00:00 2001 From: Slavica Djukic Date: Wed, 27 Feb 2019 12:31:53 +0100 Subject: [PATCH 08/61] built-in add -i: show unique prefixes of the commands Just like in the Perl script `git-add--interactive.perl`, for each command a unique prefix is determined (if there exists any within the given parameters), and shown in the list, and accepted as a shortcut for the command. We use the prefix map implementation that we just added in the previous commit for that purpose. Signed-off-by: Slavica Djukic Signed-off-by: Johannes Schindelin --- add-interactive.c | 69 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 56 insertions(+), 13 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 359f1e2e0a..d8a6abd56d 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -5,6 +5,7 @@ #include "diffcore.h" #include "revision.h" #include "refs.h" +#include "prefix-map.h" struct add_i_state { struct repository *r; @@ -46,18 +47,32 @@ static int init_add_i_state(struct repository *r, struct add_i_state *s) return 0; } -struct item { - const char *name; -}; +static ssize_t find_unique(const char *string, + struct prefix_item **list, size_t nr) +{ + ssize_t found = -1, i; + + for (i = 0; i < nr; i++) { + struct prefix_item *item = list[i]; + if (!starts_with(item->name, string)) + continue; + if (found >= 0) + return -1; + found = i; + } + + return found; +} struct list_options { int columns; const char *header; - void (*print_item)(int i, struct item *item, void *print_item_data); + void (*print_item)(int i, struct prefix_item *item, + void *print_item_data); void *print_item_data; }; -static void list(struct item **list, size_t nr, +static void list(struct prefix_item **list, size_t nr, struct add_i_state *s, struct list_options *opts) { int i, last_lf = 0; @@ -100,13 +115,15 @@ struct list_and_choose_options { * If an error occurred, returns `LIST_AND_CHOOSE_ERROR`. Upon EOF, * `LIST_AND_CHOOSE_QUIT` is returned. */ -static ssize_t list_and_choose(struct item **items, size_t nr, +static ssize_t list_and_choose(struct prefix_item **items, size_t nr, struct add_i_state *s, struct list_and_choose_options *opts) { struct strbuf input = STRBUF_INIT; ssize_t res = LIST_AND_CHOOSE_ERROR; + find_unique_prefixes(items, nr, 1, 4); + for (;;) { char *p, *endp; @@ -146,6 +163,9 @@ static ssize_t list_and_choose(struct item **items, size_t nr, } p[sep] = '\0'; + if (index < 0) + index = find_unique(p, items, nr); + if (index < 0 || index >= nr) printf(_("Huh (%s)?\n"), p); else { @@ -171,7 +191,7 @@ struct adddel { struct file_list { struct file_item { - struct item item; + struct prefix_item item; struct adddel index, worktree; } **file; size_t nr, alloc; @@ -337,12 +357,29 @@ static void populate_wi_changes(struct strbuf *buf, strbuf_addstr(buf, no_changes); } +/* filters out prefixes which have special meaning to list_and_choose() */ +static int is_valid_prefix(const char *prefix, size_t prefix_len) +{ + return prefix_len && prefix && + /* + * We expect `prefix` to be NUL terminated, therefore this + * `strcspn()` call is okay, even if it might do much more + * work than strictly necessary. + */ + strcspn(prefix, " \t\r\n,") >= prefix_len && /* separators */ + *prefix != '-' && /* deselection */ + !isdigit(*prefix) && /* selection */ + (prefix_len != 1 || + (*prefix != '*' && /* "all" wildcard */ + *prefix != '?')); /* prompt help */ +} + struct print_file_item_data { const char *modified_fmt; struct strbuf buf, index, worktree; }; -static void print_file_item(int i, struct item *item, +static void print_file_item(int i, struct prefix_item *item, void *print_file_item_data) { struct file_item *c = (struct file_item *)item; @@ -369,20 +406,26 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, return -1; if (files->nr) - list((struct item **)files->file, files->nr, s, opts); + list((struct prefix_item **)files->file, files->nr, s, opts); putchar('\n'); return 0; } -static void print_command_item(int i, struct item *item, +static void print_command_item(int i, struct prefix_item *item, void *print_command_item_data) { - printf(" %2d: %s", i + 1, item->name); + if (!item->prefix_length || + !is_valid_prefix(item->name, item->prefix_length)) + printf(" %2d: %s", i + 1, item->name); + else + printf(" %3d: [%.*s]%s", i + 1, + (int)item->prefix_length, item->name, + item->name + item->prefix_length); } struct command_item { - struct item item; + struct prefix_item item; int (*command)(struct add_i_state *s, const struct pathspec *ps, struct file_list *files, struct list_options *opts); }; @@ -424,7 +467,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) res = -1; for (;;) { - i = list_and_choose((struct item **)commands, + i = list_and_choose((struct prefix_item **)commands, ARRAY_SIZE(commands), &s, &main_loop_opts); if (i == LIST_AND_CHOOSE_QUIT) printf(_("Bye.\n")); From 7bf2ea736979072f4b9b55fcb234ceb49340a683 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 6 Mar 2019 00:06:57 +0100 Subject: [PATCH 09/61] built-in add -i: support `?` (prompt help) With this change, we print out the same colored help text that the Perl-based `git add -i` prints in the main loop when question mark is entered. Signed-off-by: Johannes Schindelin --- add-interactive.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index d8a6abd56d..f7008dcfbc 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -11,6 +11,7 @@ struct add_i_state { struct repository *r; int use_color; char header_color[COLOR_MAXLEN]; + char help_color[COLOR_MAXLEN]; }; static void init_color(struct repository *r, struct add_i_state *s, @@ -43,6 +44,7 @@ static int init_add_i_state(struct repository *r, struct add_i_state *s) 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); return 0; } @@ -104,6 +106,7 @@ struct list_and_choose_options { struct list_options list_opts; const char *prompt; + void (*print_help)(struct add_i_state *s); }; #define LIST_AND_CHOOSE_ERROR (-1) @@ -144,6 +147,11 @@ static ssize_t list_and_choose(struct prefix_item **items, size_t nr, if (!input.len) break; + if (!strcmp(input.buf, "?")) { + opts->print_help(s); + continue; + } + p = input.buf; for (;;) { size_t sep = strcspn(p, " \t\r\n,"); @@ -430,12 +438,24 @@ struct command_item { struct file_list *files, struct list_options *opts); }; +static void command_prompt_help(struct add_i_state *s) +{ + const char *help_color = s->help_color; + color_fprintf_ln(stdout, help_color, "%s", _("Prompt help:")); + color_fprintf_ln(stdout, help_color, "1 - %s", + _("select a numbered item")); + color_fprintf_ln(stdout, help_color, "foo - %s", + _("select item based on unique prefix")); + color_fprintf_ln(stdout, help_color, " - %s", + _("(empty) select nothing")); +} + int run_add_i(struct repository *r, const struct pathspec *ps) { struct add_i_state s = { NULL }; struct list_and_choose_options main_loop_opts = { { 4, N_("*** Commands ***"), print_command_item, NULL }, - N_("What now") + N_("What now"), command_prompt_help }; struct command_item status = { { "status" }, run_status }; From a23859dfa249ba2e7f896c114555324a54a57081 Mon Sep 17 00:00:00 2001 From: Slavica Djukic Date: Sun, 3 Mar 2019 13:19:27 +0100 Subject: [PATCH 10/61] built-in add -i: use color in the main loop The error messages as well as the unique prefixes are colored in `git add -i` by default; We need to do the same in the built-in version. Signed-off-by: Slavica Djukic Signed-off-by: Johannes Schindelin --- add-interactive.c | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index f7008dcfbc..4c9d3c743b 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -12,6 +12,9 @@ struct add_i_state { int use_color; char header_color[COLOR_MAXLEN]; char help_color[COLOR_MAXLEN]; + char prompt_color[COLOR_MAXLEN]; + char error_color[COLOR_MAXLEN]; + char reset_color[COLOR_MAXLEN]; }; static void init_color(struct repository *r, struct add_i_state *s, @@ -45,6 +48,9 @@ static int init_add_i_state(struct repository *r, struct add_i_state *s) 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, "reset", s->reset_color, GIT_COLOR_RESET); return 0; } @@ -134,7 +140,8 @@ static ssize_t list_and_choose(struct prefix_item **items, size_t nr, list(items, nr, s, &opts->list_opts); - printf("%s%s", opts->prompt, "> "); + color_fprintf(stdout, s->prompt_color, "%s", opts->prompt); + fputs("> ", stdout); fflush(stdout); if (strbuf_getline(&input, stdin) == EOF) { @@ -175,7 +182,8 @@ static ssize_t list_and_choose(struct prefix_item **items, size_t nr, index = find_unique(p, items, nr); if (index < 0 || index >= nr) - printf(_("Huh (%s)?\n"), p); + color_fprintf_ln(stdout, s->error_color, + _("Huh (%s)?"), p); else { res = index; break; @@ -420,15 +428,21 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, return 0; } +struct print_command_item_data { + const char *color, *reset; +}; + static void print_command_item(int i, struct prefix_item *item, void *print_command_item_data) { + struct print_command_item_data *d = print_command_item_data; + if (!item->prefix_length || !is_valid_prefix(item->name, item->prefix_length)) printf(" %2d: %s", i + 1, item->name); else - printf(" %3d: [%.*s]%s", i + 1, - (int)item->prefix_length, item->name, + printf(" %2d: %s%.*s%s%s", i + 1, + d->color, (int)item->prefix_length, item->name, d->reset, item->name + item->prefix_length); } @@ -453,8 +467,9 @@ static void command_prompt_help(struct add_i_state *s) int run_add_i(struct repository *r, const struct pathspec *ps) { struct add_i_state s = { NULL }; + struct print_command_item_data data; struct list_and_choose_options main_loop_opts = { - { 4, N_("*** Commands ***"), print_command_item, NULL }, + { 4, N_("*** Commands ***"), print_command_item, &data }, N_("What now"), command_prompt_help }; struct command_item @@ -477,6 +492,18 @@ int run_add_i(struct repository *r, const struct pathspec *ps) if (init_add_i_state(r, &s)) return error("could not parse `add -i` config"); + /* + * When color was asked for, use the prompt color for + * highlighting, otherwise use square brackets. + */ + if (s.use_color) { + data.color = s.prompt_color; + data.reset = s.reset_color; + } else { + data.color = "["; + data.reset = "]"; + } + strbuf_addstr(&header, " "); strbuf_addf(&header, print_file_item_data.modified_fmt, _("staged"), _("unstaged"), _("path")); From 5b804f91e4ba509701fed3fb483e0bd5a7eb3937 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 5 Mar 2019 23:33:55 +0100 Subject: [PATCH 11/61] built-in add -i: allow filtering the modified files list In `update` command of `git add -i`, we are primarily interested in the list of modified files that have worktree (i.e. unstaged) changes. The Perl script version of `git add -i` has a parameter of the `list_modified()` function for that matter. In C, we can be a lot more precise, using an `enum`. The C implementation of the filter also has an easier time to avoid unnecessary work, simply by using an adaptive order of the `diff-index` and `diff-files` calls, and then not adding unnecessary entries in the first place. Signed-off-by: Johannes Schindelin --- add-interactive.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index e6cea2a30d..d95184b757 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -268,6 +268,7 @@ struct collection_status { const char *reference; + unsigned skip_unseen:1; struct file_list *list; struct hashmap file_map; }; @@ -296,6 +297,8 @@ static void collect_changes_cb(struct diff_queue_struct *q, entry = hashmap_get_from_hash(&s->file_map, hash, name); if (entry) file_index = entry->index; + else if (s->skip_unseen) + continue; else { FLEX_ALLOC_STR(entry, pathname, name); hashmap_entry_init(entry, hash); @@ -315,13 +318,22 @@ static void collect_changes_cb(struct diff_queue_struct *q, } } -static int get_modified_files(struct repository *r, struct file_list *list, +enum modified_files_filter { + NO_FILTER = 0, + WORKTREE_ONLY = 1, + INDEX_ONLY = 2, +}; + +static int get_modified_files(struct repository *r, + enum modified_files_filter filter, + struct file_list *list, const struct pathspec *ps) { struct object_id head_oid; int is_initial = !resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL); struct collection_status s = { FROM_WORKTREE }; + int i; if (repo_read_index_preload(r, ps, 0) < 0) return error(_("could not read index")); @@ -329,10 +341,16 @@ static int get_modified_files(struct repository *r, struct file_list *list, s.list = list; hashmap_init(&s.file_map, pathname_entry_cmp, NULL, 0); - for (s.phase = FROM_WORKTREE; s.phase <= FROM_INDEX; s.phase++) { + for (i = 0; i < 2; i++) { struct rev_info rev; struct setup_revision_opt opt = { 0 }; + if (filter == INDEX_ONLY) + s.phase = i ? FROM_WORKTREE : FROM_INDEX; + else + s.phase = i ? FROM_INDEX : FROM_WORKTREE; + s.skip_unseen = filter && i; + opt.def = is_initial ? empty_tree_oid_hex() : oid_to_hex(&head_oid); @@ -418,7 +436,7 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, { reset_file_list(files); - if (get_modified_files(s->r, files, ps) < 0) + if (get_modified_files(s->r, 0, files, ps) < 0) return -1; if (files->nr) From 4671e601b2e4494adc89b06f5c4a3fdaf270074f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 21:36:11 +0100 Subject: [PATCH 12/61] built-in add -i: prepare for multi-selection commands The `upgrade`, `revert` and `add-untracked` commands allow selecting multiple entries. Let's extend the `list_and_choose()` function to accommodate those use cases. Signed-off-by: Johannes Schindelin --- add-interactive.c | 105 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 78 insertions(+), 27 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index d95184b757..c3f6073c94 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -75,12 +75,12 @@ static ssize_t find_unique(const char *string, struct list_options { int columns; const char *header; - void (*print_item)(int i, struct prefix_item *item, + void (*print_item)(int i, int selected, struct prefix_item *item, void *print_item_data); void *print_item_data; }; -static void list(struct prefix_item **list, size_t nr, +static void list(struct prefix_item **list, int *selected, size_t nr, struct add_i_state *s, struct list_options *opts) { int i, last_lf = 0; @@ -93,7 +93,8 @@ static void list(struct prefix_item **list, size_t nr, "%s", opts->header); for (i = 0; i < nr; i++) { - opts->print_item(i, list[i], opts->print_item_data); + opts->print_item(i, selected ? selected[i] : 0, list[i], + opts->print_item_data); if ((opts->columns) && ((i + 1) % (opts->columns))) { putchar('\t'); @@ -112,6 +113,10 @@ struct list_and_choose_options { struct list_options list_opts; const char *prompt; + enum { + SINGLETON = (1<<0), + IMMEDIATE = (1<<1), + } flags; void (*print_help)(struct add_i_state *s); }; @@ -119,17 +124,27 @@ struct list_and_choose_options { #define LIST_AND_CHOOSE_QUIT (-2) /* - * Returns the selected index. + * Returns the selected index in singleton mode, the number of selected items + * otherwise. * * If an error occurred, returns `LIST_AND_CHOOSE_ERROR`. Upon EOF, * `LIST_AND_CHOOSE_QUIT` is returned. */ -static ssize_t list_and_choose(struct prefix_item **items, size_t nr, - struct add_i_state *s, +static ssize_t list_and_choose(struct prefix_item **items, int *selected, + size_t nr, struct add_i_state *s, struct list_and_choose_options *opts) { + int singleton = opts->flags & SINGLETON; + int immediate = opts->flags & IMMEDIATE; + struct strbuf input = STRBUF_INIT; - ssize_t res = LIST_AND_CHOOSE_ERROR; + ssize_t res = singleton ? LIST_AND_CHOOSE_ERROR : 0; + + if (!selected && !singleton) + BUG("need a selected array in non-singleton mode"); + + if (singleton && !immediate) + BUG("singleton requires immediate"); find_unique_prefixes(items, nr, 1, 4); @@ -138,15 +153,16 @@ static ssize_t list_and_choose(struct prefix_item **items, size_t nr, strbuf_reset(&input); - list(items, nr, s, &opts->list_opts); + list(items, selected, nr, s, &opts->list_opts); color_fprintf(stdout, s->prompt_color, "%s", opts->prompt); - fputs("> ", stdout); + fputs(singleton ? "> " : ">> ", stdout); fflush(stdout); if (strbuf_getline(&input, stdin) == EOF) { putchar('\n'); - res = LIST_AND_CHOOSE_QUIT; + if (immediate) + res = LIST_AND_CHOOSE_QUIT; break; } strbuf_trim(&input); @@ -162,7 +178,9 @@ static ssize_t list_and_choose(struct prefix_item **items, size_t nr, p = input.buf; for (;;) { size_t sep = strcspn(p, " \t\r\n,"); - ssize_t index = -1; + int choose = 1; + /* `from` is inclusive, `to` is exclusive */ + ssize_t from = -1, to = -1; if (!sep) { if (!*p) @@ -171,28 +189,60 @@ static ssize_t list_and_choose(struct prefix_item **items, size_t nr, continue; } - if (isdigit(*p)) { - index = strtoul(p, &endp, 10) - 1; - if (endp != p + sep) - index = -1; + /* Input that begins with '-'; unchoose */ + if (*p == '-') { + choose = 0; + p++; + sep--; + } + + if (sep == 1 && *p == '*') { + from = 0; + to = nr; + } else if (isdigit(*p)) { + /* A range can be specified like 5-7 or 5-. */ + from = strtoul(p, &endp, 10) - 1; + if (endp == p + sep) + to = from + 1; + else if (*endp == '-') { + to = strtoul(++endp, &endp, 10); + /* extra characters after the range? */ + if (endp != p + sep) + from = -1; + } } p[sep] = '\0'; - if (index < 0) - index = find_unique(p, items, nr); + if (from < 0) { + from = find_unique(p, items, nr); + if (from >= 0) + to = from + 1; + } - if (index < 0 || index >= nr) + if (from < 0 || from >= nr || + (singleton && from + 1 != to)) { color_fprintf_ln(stdout, s->error_color, _("Huh (%s)?"), p); - else { - res = index; + break; + } else if (singleton) { + res = from; break; } + if (to > nr) + to = nr; + + for (; from < to; from++) + if (selected[from] != choose) { + selected[from] = choose; + res += choose ? +1 : -1; + } + p += sep + 1; } - if (res != LIST_AND_CHOOSE_ERROR) + if ((immediate && res != LIST_AND_CHOOSE_ERROR) || + !strcmp(input.buf, "*")) break; } @@ -413,7 +463,7 @@ struct print_file_item_data { struct strbuf buf, index, worktree; }; -static void print_file_item(int i, struct prefix_item *item, +static void print_file_item(int i, int selected, struct prefix_item *item, void *print_file_item_data) { struct file_item *c = (struct file_item *)item; @@ -428,7 +478,7 @@ static void print_file_item(int i, struct prefix_item *item, strbuf_addf(&d->buf, d->modified_fmt, d->index.buf, d->worktree.buf, item->name); - printf(" %2d: %s", i + 1, d->buf.buf); + printf("%c%2d: %s", selected ? '*' : ' ', i + 1, d->buf.buf); } static int run_status(struct add_i_state *s, const struct pathspec *ps, @@ -440,7 +490,8 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, return -1; if (files->nr) - list((struct prefix_item **)files->file, files->nr, s, opts); + list((struct prefix_item **)files->file, NULL, files->nr, + s, opts); putchar('\n'); return 0; @@ -471,7 +522,7 @@ struct print_command_item_data { const char *color, *reset; }; -static void print_command_item(int i, struct prefix_item *item, +static void print_command_item(int i, int selected, struct prefix_item *item, void *print_command_item_data) { struct print_command_item_data *d = print_command_item_data; @@ -509,7 +560,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) struct print_command_item_data data; struct list_and_choose_options main_loop_opts = { { 4, N_("*** Commands ***"), print_command_item, &data }, - N_("What now"), command_prompt_help + N_("What now"), SINGLETON | IMMEDIATE, command_prompt_help }; struct command_item status = { { "status" }, run_status }, @@ -555,7 +606,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) res = -1; for (;;) { - i = list_and_choose((struct prefix_item **)commands, + i = list_and_choose((struct prefix_item **)commands, NULL, ARRAY_SIZE(commands), &s, &main_loop_opts); if (i == LIST_AND_CHOOSE_QUIT) printf(_("Bye.\n")); From ae801945d2e99906970f82c696e719851fff9944 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 5 Mar 2019 23:58:49 +0100 Subject: [PATCH 13/61] built-in add -i: implement the `update` command After `status` and `help`, it is now turn to port the `update` command to C, the second command that is shown in the main loop menu of `git add -i`. This `git add -i` command is the first one which lets the user choose a subset of a list of files, and as such, this patch lays the groundwork for the other commands of that category: - It teaches the `print_file_item()` function to show a unique prefix if we found any (the code to find it had been added already in the previous patch where we colored the unique prefixes of the main loop commands, but that patch uses the `print_command_item()` function to display the menu items). - This patch also adds the help text that is shown when the user input to select items from the shown list could not be parsed. Signed-off-by: Johannes Schindelin --- add-interactive.c | 124 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 112 insertions(+), 12 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index c3f6073c94..6c91fe4f4e 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -6,6 +6,7 @@ #include "revision.h" #include "refs.h" #include "prefix-map.h" +#include "lockfile.h" struct add_i_state { struct repository *r; @@ -459,8 +460,8 @@ static int is_valid_prefix(const char *prefix, size_t prefix_len) } struct print_file_item_data { - const char *modified_fmt; - struct strbuf buf, index, worktree; + const char *modified_fmt, *color, *reset; + struct strbuf buf, name, index, worktree; }; static void print_file_item(int i, int selected, struct prefix_item *item, @@ -468,21 +469,34 @@ static void print_file_item(int i, int selected, struct prefix_item *item, { struct file_item *c = (struct file_item *)item; struct print_file_item_data *d = print_file_item_data; + const char *highlighted = NULL; strbuf_reset(&d->index); strbuf_reset(&d->worktree); strbuf_reset(&d->buf); + /* Format the item with the prefix highlighted. */ + if (item->prefix_length > 0 && + is_valid_prefix(item->name, item->prefix_length)) { + strbuf_reset(&d->name); + strbuf_addf(&d->name, "%s%.*s%s%s", d->color, + (int)item->prefix_length, item->name, d->reset, + item->name + item->prefix_length); + highlighted = d->name.buf; + } + populate_wi_changes(&d->worktree, &c->worktree, _("nothing")); populate_wi_changes(&d->index, &c->index, _("unchanged")); strbuf_addf(&d->buf, d->modified_fmt, - d->index.buf, d->worktree.buf, item->name); + d->index.buf, d->worktree.buf, + highlighted ? highlighted : item->name); printf("%c%2d: %s", selected ? '*' : ' ', i + 1, d->buf.buf); } static int run_status(struct add_i_state *s, const struct pathspec *ps, - struct file_list *files, struct list_options *opts) + struct file_list *files, + struct list_and_choose_options *opts) { reset_file_list(files); @@ -491,14 +505,72 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, if (files->nr) list((struct prefix_item **)files->file, NULL, files->nr, - s, opts); + s, &opts->list_opts); putchar('\n'); return 0; } +static int run_update(struct add_i_state *s, const struct pathspec *ps, + struct file_list *files, + struct list_and_choose_options *opts) +{ + int res = 0, fd, *selected = NULL; + size_t count, i; + struct lock_file index_lock; + + reset_file_list(files); + + if (get_modified_files(s->r, WORKTREE_ONLY, files, ps) < 0) + return -1; + + if (!files->nr) { + putchar('\n'); + return 0; + } + + opts->prompt = N_("Update"); + CALLOC_ARRAY(selected, files->nr); + + count = list_and_choose((struct prefix_item **)files->file, + selected, files->nr, s, opts); + if (count <= 0) { + putchar('\n'); + free(selected); + return 0; + } + + fd = repo_hold_locked_index(s->r, &index_lock, LOCK_REPORT_ON_ERROR); + if (fd < 0) { + putchar('\n'); + free(selected); + return -1; + } + + for (i = 0; i < files->nr; i++) { + const char *name = files->file[i]->item.name; + if (selected[i] && + add_file_to_index(s->r->index, name, 0) < 0) { + res = error(_("could not stage '%s'"), name); + break; + } + } + + if (!res && write_locked_index(s->r->index, &index_lock, COMMIT_LOCK) < 0) + res = error(_("could not write index")); + + if (!res) + printf(Q_("updated %d path\n", + "updated %d paths\n", count), (int)count); + + putchar('\n'); + free(selected); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *ps, - struct file_list *files, struct list_options *opts) + struct file_list *files, + struct list_and_choose_options *opts) { const char *help_color = s->help_color; @@ -518,6 +590,27 @@ static int run_help(struct add_i_state *s, const struct pathspec *ps, return 0; } +static void choose_prompt_help(struct add_i_state *s) +{ + const char *help_color = s->help_color; + color_fprintf_ln(stdout, help_color, "%s", + _("Prompt help:")); + color_fprintf_ln(stdout, help_color, "1 - %s", + _("select a single item")); + color_fprintf_ln(stdout, help_color, "3-5 - %s", + _("select a range of items")); + color_fprintf_ln(stdout, help_color, "2-3,6-9 - %s", + _("select multiple ranges")); + color_fprintf_ln(stdout, help_color, "foo - %s", + _("select item based on unique prefix")); + color_fprintf_ln(stdout, help_color, "-... - %s", + _("unselect specified items")); + color_fprintf_ln(stdout, help_color, "* - %s", + _("choose all items")); + color_fprintf_ln(stdout, help_color, " - %s", + _("(empty) finish selecting")); +} + struct print_command_item_data { const char *color, *reset; }; @@ -539,7 +632,8 @@ static void print_command_item(int i, int selected, struct prefix_item *item, struct command_item { struct prefix_item item; int (*command)(struct add_i_state *s, const struct pathspec *ps, - struct file_list *files, struct list_options *opts); + struct file_list *files, + struct list_and_choose_options *opts); }; static void command_prompt_help(struct add_i_state *s) @@ -564,17 +658,20 @@ int run_add_i(struct repository *r, const struct pathspec *ps) }; struct command_item status = { { "status" }, run_status }, + update = { { "update" }, run_update }, help = { { "help" }, run_help }; struct command_item *commands[] = { - &status, + &status, &update, &help }; struct print_file_item_data print_file_item_data = { - "%12s %12s %s", STRBUF_INIT, STRBUF_INIT, STRBUF_INIT + "%12s %12s %s", NULL, NULL, + STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT }; - struct list_options opts = { - 0, NULL, print_file_item, &print_file_item_data + struct list_and_choose_options opts = { + { 0, NULL, print_file_item, &print_file_item_data }, + NULL, 0, choose_prompt_help }; struct strbuf header = STRBUF_INIT; struct file_list files = { NULL }; @@ -595,11 +692,13 @@ int run_add_i(struct repository *r, const struct pathspec *ps) data.color = "["; data.reset = "]"; } + print_file_item_data.color = data.color; + print_file_item_data.reset = data.reset; strbuf_addstr(&header, " "); strbuf_addf(&header, print_file_item_data.modified_fmt, _("staged"), _("unstaged"), _("path")); - opts.header = header.buf; + opts.list_opts.header = header.buf; repo_refresh_and_write_index(r, REFRESH_QUIET, 1); if (run_status(&s, ps, &files, &opts) < 0) @@ -619,6 +718,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) release_file_list(&files); strbuf_release(&print_file_item_data.buf); + strbuf_release(&print_file_item_data.name); strbuf_release(&print_file_item_data.index); strbuf_release(&print_file_item_data.worktree); strbuf_release(&header); From 4b9d5dea0f3489491b277d25e9883a0502f4c22b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 01:12:07 +0100 Subject: [PATCH 14/61] built-in add -i: implement the `help` command This imitates the code to show the help text from the Perl script `git-add--interactive.perl` in the built-in version. To make sure that it renders exactly like the Perl version of `git add -i`, we also add a test case for that to `t3701-add-interactive.sh`. Signed-off-by: Slavica Djukic Signed-off-by: Johannes Schindelin --- add-interactive.c | 27 +++++++++++++++++++++++++-- t/t3701-add-interactive.sh | 24 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 4c9d3c743b..e6cea2a30d 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -428,6 +428,27 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, return 0; } +static int run_help(struct add_i_state *s, const struct pathspec *ps, + struct file_list *files, struct list_options *opts) +{ + const char *help_color = s->help_color; + + color_fprintf_ln(stdout, help_color, "status - %s", + _("show paths with changes")); + color_fprintf_ln(stdout, help_color, "update - %s", + _("add working tree state to the staged set of changes")); + color_fprintf_ln(stdout, help_color, "revert - %s", + _("revert staged set of changes back to the HEAD version")); + color_fprintf_ln(stdout, help_color, "patch - %s", + _("pick hunks and update selectively")); + color_fprintf_ln(stdout, help_color, "diff - %s", + _("view diff between HEAD and index")); + color_fprintf_ln(stdout, help_color, "add untracked - %s", + _("add contents of untracked files to the staged set of changes")); + + return 0; +} + struct print_command_item_data { const char *color, *reset; }; @@ -473,9 +494,11 @@ int run_add_i(struct repository *r, const struct pathspec *ps) N_("What now"), command_prompt_help }; struct command_item - status = { { "status" }, run_status }; + status = { { "status" }, run_status }, + help = { { "help" }, run_help }; struct command_item *commands[] = { - &status + &status, + &help }; struct print_file_item_data print_file_item_data = { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 65dfbc033a..91aaef2932 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -639,4 +639,28 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'show help from add--helper' ' + git reset --hard && + cat >expect <<-EOF && + + *** Commands *** + 1: status 2: update 3: revert 4: add untracked + 5: patch 6: diff 7: quit 8: help + What now> status - show paths with changes + update - add working tree state to the staged set of changes + revert - revert staged set of changes back to the HEAD version + patch - pick hunks and update selectively + diff - view diff between HEAD and index + add untracked - add contents of untracked files to the staged set of changes + *** Commands *** + 1: status 2: update 3: revert 4: add untracked + 5: patch 6: diff 7: quit 8: help + What now>$SP + Bye. + EOF + test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_decode_color actual && + test_i18ncmp expect actual +' + test_done From 85284106d742aded3e20db938d65b93ae144beaa Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 6 Mar 2019 23:06:13 +0100 Subject: [PATCH 15/61] built-in add -i: re-implement `revert` in C This is a relatively straight-forward port from the Perl version, with the notable exception that we imitate `git reset -- ` in the C version rather than the convoluted `git ls-tree HEAD -- | git update-index --index-info` followed by `git update-index --force-remove -- ` for the missed ones. While at it, we fix the pretty obvious bug where the `revert` command offers to unstage files that do not have staged changes. Signed-off-by: Johannes Schindelin --- add-interactive.c | 114 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 113 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index 6c91fe4f4e..6faa514879 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -568,6 +568,117 @@ static int run_update(struct add_i_state *s, const struct pathspec *ps, return res; } +static void revert_from_diff(struct diff_queue_struct *q, + struct diff_options *opt, void *data) +{ + int i, add_flags = ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE; + + for (i = 0; i < q->nr; i++) { + struct diff_filespec *one = q->queue[i]->one; + struct cache_entry *ce; + + if (!(one->mode && !is_null_oid(&one->oid))) { + remove_file_from_index(opt->repo->index, one->path); + printf(_("note: %s is untracked now.\n"), one->path); + } else { + ce = make_cache_entry(opt->repo->index, one->mode, + &one->oid, one->path, 0, 0); + if (!ce) + die(_("make_cache_entry failed for path '%s'"), + one->path); + add_index_entry(opt->repo->index, ce, add_flags); + } + } +} + +static int run_revert(struct add_i_state *s, const struct pathspec *ps, + struct file_list *files, + struct list_and_choose_options *opts) +{ + int res = 0, fd, *selected = NULL; + size_t count, i, j; + + struct object_id oid; + int is_initial = !resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &oid, + NULL); + struct lock_file index_lock; + const char **paths; + struct tree *tree; + struct diff_options diffopt = { NULL }; + + reset_file_list(files); + if (get_modified_files(s->r, INDEX_ONLY, files, ps) < 0) + return -1; + + if (!files->nr) { + putchar('\n'); + return 0; + } + + opts->prompt = N_("Revert"); + CALLOC_ARRAY(selected, files->nr); + + count = list_and_choose((struct prefix_item **)files->file, + selected, files->nr, s, opts); + if (count <= 0) + goto finish_revert; + + fd = repo_hold_locked_index(s->r, &index_lock, LOCK_REPORT_ON_ERROR); + if (fd < 0) { + res = -1; + goto finish_revert; + } + + if (is_initial) + oidcpy(&oid, s->r->hash_algo->empty_tree); + else { + tree = parse_tree_indirect(&oid); + if (!tree) { + res = error(_("Could not parse HEAD^{tree}")); + goto finish_revert; + } + oidcpy(&oid, &tree->object.oid); + } + + ALLOC_ARRAY(paths, count + 1); + for (i = j = 0; i < files->nr; i++) + if (selected[i]) + paths[j++] = files->file[i]->item.name; + paths[j] = NULL; + + parse_pathspec(&diffopt.pathspec, 0, + PATHSPEC_PREFER_FULL | PATHSPEC_LITERAL_PATH, + NULL, paths); + + diffopt.output_format = DIFF_FORMAT_CALLBACK; + diffopt.format_callback = revert_from_diff; + diffopt.flags.override_submodule_config = 1; + diffopt.repo = s->r; + + if (do_diff_cache(&oid, &diffopt)) + res = -1; + else { + diffcore_std(&diffopt); + diff_flush(&diffopt); + } + free(paths); + clear_pathspec(&diffopt.pathspec); + + if (!res && write_locked_index(s->r->index, &index_lock, + COMMIT_LOCK) < 0) + res = -1; + else + res = repo_refresh_and_write_index(s->r, REFRESH_QUIET, 1); + if (!res) + printf(Q_("reverted %d path\n", + "reverted %d paths\n", count), (int)count); + +finish_revert: + putchar('\n'); + free(selected); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *ps, struct file_list *files, struct list_and_choose_options *opts) @@ -659,9 +770,10 @@ int run_add_i(struct repository *r, const struct pathspec *ps) struct command_item status = { { "status" }, run_status }, update = { { "update" }, run_update }, + revert = { { "revert" }, run_revert }, help = { { "help" }, run_help }; struct command_item *commands[] = { - &status, &update, + &status, &update, &revert, &help }; From 6d5bf2b3c7272bfb22c359428dd1dad7b84f2a7e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 00:59:24 +0100 Subject: [PATCH 16/61] built-in add -i: re-implement `add-untracked` in C This is yet another command, ported to C. It builds nicely on the support functions introduced for other commands, with the notable difference that only names are displayed for untracked files, no file type or diff summary. Signed-off-by: Johannes Schindelin --- add-interactive.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index 6faa514879..fb4e81c033 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -7,6 +7,8 @@ #include "refs.h" #include "prefix-map.h" #include "lockfile.h" +#include "pathspec.h" +#include "dir.h" struct add_i_state { struct repository *r; @@ -462,6 +464,7 @@ static int is_valid_prefix(const char *prefix, size_t prefix_len) struct print_file_item_data { const char *modified_fmt, *color, *reset; struct strbuf buf, name, index, worktree; + unsigned only_names:1; }; static void print_file_item(int i, int selected, struct prefix_item *item, @@ -485,6 +488,12 @@ static void print_file_item(int i, int selected, struct prefix_item *item, highlighted = d->name.buf; } + if (d->only_names) { + printf("%c%2d: %s", selected ? '*' : ' ', i + 1, + highlighted ? highlighted : item->name); + return; + } + populate_wi_changes(&d->worktree, &c->worktree, _("nothing")); populate_wi_changes(&d->index, &c->index, _("unchanged")); strbuf_addf(&d->buf, d->modified_fmt, @@ -679,6 +688,92 @@ finish_revert: return res; } +static int get_untracked_files(struct repository *r, struct file_list *list, + const struct pathspec *ps) +{ + struct dir_struct dir = { 0 }; + size_t i; + + if (repo_read_index(r) < 0) + return error(_("could not read index")); + + setup_standard_excludes(&dir); + add_exclude_list(&dir, EXC_CMDL, "--exclude option"); + fill_directory(&dir, r->index, ps); + + for (i = 0; i < dir.nr; i++) { + struct dir_entry *ent = dir.entries[i]; + + if (index_name_is_other(r->index, ent->name, ent->len)) { + struct file_item *item; + + FLEXPTR_ALLOC_MEM(item, item.name, ent->name, ent->len); + + ALLOC_GROW(list->file, list->nr + 1, list->alloc); + list->file[list->nr++] = item; + } + } + + return 0; +} + +static int run_add_untracked(struct add_i_state *s, const struct pathspec *ps, + struct file_list *files, + struct list_and_choose_options *opts) +{ + struct print_file_item_data *d = opts->list_opts.print_item_data; + int res = 0, fd, *selected = NULL; + size_t count, i; + + struct lock_file index_lock; + + reset_file_list(files); + if (get_untracked_files(s->r, files, ps) < 0) + return -1; + + if (!files->nr) { + printf(_("No untracked files.\n")); + goto finish_add_untracked; + } + + opts->prompt = N_("Add untracked"); + CALLOC_ARRAY(selected, files->nr); + + d->only_names = 1; + count = list_and_choose((struct prefix_item **)files->file, + selected, files->nr, s, opts); + d->only_names = 0; + if (count <= 0) + goto finish_add_untracked; + + fd = repo_hold_locked_index(s->r, &index_lock, LOCK_REPORT_ON_ERROR); + if (fd < 0) { + res = -1; + goto finish_add_untracked; + } + + for (i = 0; i < files->nr; i++) { + const char *name = files->file[i]->item.name; + if (selected[i] && + add_file_to_index(s->r->index, name, 0) < 0) { + res = error(_("could not stage '%s'"), name); + break; + } + } + + if (!res && write_locked_index(s->r->index, &index_lock, COMMIT_LOCK) < 0) + res = error(_("could not write index")); + + if (!res) + printf(Q_("added %d path\n", + "added %d paths\n", count), (int)count); + +finish_add_untracked: + putchar('\n'); + free(selected); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *ps, struct file_list *files, struct list_and_choose_options *opts) @@ -771,9 +866,10 @@ int run_add_i(struct repository *r, const struct pathspec *ps) status = { { "status" }, run_status }, update = { { "update" }, run_update }, revert = { { "revert" }, run_revert }, + add_untracked = { { "add untracked" }, run_add_untracked }, help = { { "help" }, run_help }; struct command_item *commands[] = { - &status, &update, &revert, + &status, &update, &revert, &add_untracked, &help }; From c12f09eaa60dfb62e0b314a20a4478fee7d538ed Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 11 Mar 2019 10:07:49 +0100 Subject: [PATCH 17/61] built-in add -i: implement the `patch` command Well, it is not a full implementation yet. In the interest of making this easy to review (and easy to keep bugs out), we still hand off to the Perl script to do the actual work. The `patch` functionality actually makes up for more than half of the 1,800+ lines of `git-add--interactive.perl`. It will be ported from Perl to C incrementally, later. Signed-off-by: Johannes Schindelin --- add-interactive.c | 97 ++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 88 insertions(+), 9 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index fb4e81c033..2783247389 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -9,6 +9,8 @@ #include "lockfile.h" #include "pathspec.h" #include "dir.h" +#include "argv-array.h" +#include "run-command.h" struct add_i_state { struct repository *r; @@ -255,7 +257,7 @@ static ssize_t list_and_choose(struct prefix_item **items, int *selected, struct adddel { uintmax_t add, del; - unsigned seen:1, binary:1; + unsigned seen:1, unmerged:1, binary:1; }; struct file_list { @@ -322,6 +324,7 @@ struct collection_status { const char *reference; unsigned skip_unseen:1; + size_t unmerged_count, binary_count; struct file_list *list; struct hashmap file_map; }; @@ -345,12 +348,14 @@ static void collect_changes_cb(struct diff_queue_struct *q, struct pathname_entry *entry; size_t file_index; struct file_item *file; - struct adddel *adddel; + struct adddel *adddel, *other_adddel; entry = hashmap_get_from_hash(&s->file_map, hash, name); - if (entry) + if (entry) { + if (entry->index == (size_t)-1) + continue; file_index = entry->index; - else if (s->skip_unseen) + } else if (s->skip_unseen) continue; else { FLEX_ALLOC_STR(entry, pathname, name); @@ -363,11 +368,20 @@ static void collect_changes_cb(struct diff_queue_struct *q, file = s->list->file[file_index]; adddel = s->phase == FROM_INDEX ? &file->index : &file->worktree; + other_adddel = s->phase == FROM_INDEX ? &file->worktree : &file->index; adddel->seen = 1; adddel->add = stat.files[i]->added; adddel->del = stat.files[i]->deleted; - if (stat.files[i]->is_binary) + if (stat.files[i]->is_binary) { + if (!other_adddel->binary) + s->binary_count++; adddel->binary = 1; + } + if (stat.files[i]->is_unmerged) { + if (!other_adddel->unmerged) + s->unmerged_count++; + adddel->unmerged = 1; + } } } @@ -379,6 +393,8 @@ enum modified_files_filter { static int get_modified_files(struct repository *r, enum modified_files_filter filter, + size_t *unmerged_count, + size_t *binary_count, struct file_list *list, const struct pathspec *ps) { @@ -425,6 +441,10 @@ static int get_modified_files(struct repository *r, } } hashmap_free(&s.file_map, 1); + if (unmerged_count) + *unmerged_count = s.unmerged_count; + if (binary_count) + *binary_count = s.binary_count; /* While the diffs are ordered already, we ran *two* diffs... */ QSORT(list->file, list->nr, file_item_cmp); @@ -509,7 +529,7 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, { reset_file_list(files); - if (get_modified_files(s->r, 0, files, ps) < 0) + if (get_modified_files(s->r, 0, NULL, NULL, files, ps) < 0) return -1; if (files->nr) @@ -530,7 +550,7 @@ static int run_update(struct add_i_state *s, const struct pathspec *ps, reset_file_list(files); - if (get_modified_files(s->r, WORKTREE_ONLY, files, ps) < 0) + if (get_modified_files(s->r, WORKTREE_ONLY, NULL, NULL, files, ps) < 0) return -1; if (!files->nr) { @@ -616,7 +636,7 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps, struct diff_options diffopt = { NULL }; reset_file_list(files); - if (get_modified_files(s->r, INDEX_ONLY, files, ps) < 0) + if (get_modified_files(s->r, INDEX_ONLY, NULL, NULL, files, ps) < 0) return -1; if (!files->nr) { @@ -774,6 +794,64 @@ finish_add_untracked: return res; } +static int run_patch(struct add_i_state *s, const struct pathspec *ps, + struct file_list *files, + struct list_and_choose_options *opts) +{ + struct prefix_item **items = (struct prefix_item **)files->file; + int res = 0, *selected = NULL; + ssize_t count, i, j; + size_t unmerged_count = 0, binary_count = 0; + + reset_file_list(files); + if (get_modified_files(s->r, WORKTREE_ONLY, &unmerged_count, + &binary_count, files, ps) < 0) + return -1; + + if (unmerged_count || binary_count) { + for (i = j = 0; i < files->nr; i++) + if (files->file[i]->index.binary || + files->file[i]->worktree.binary) + free(items[i]); + else if (files->file[i]->index.unmerged || + files->file[i]->worktree.unmerged) { + color_fprintf_ln(stderr, s->error_color, + _("ignoring unmerged: %s"), + files->file[i]->item.name); + free(items[i]); + } else + items[j++] = items[i]; + files->nr = j; + } + + if (!files->nr) { + if (binary_count) + fprintf(stderr, _("Only binary files changed.\n")); + else + fprintf(stderr, _("No changes.\n")); + return 0; + } + + opts->prompt = N_("Patch update"); + CALLOC_ARRAY(selected, files->nr); + + count = list_and_choose(items, selected, files->nr, s, opts); + if (count >= 0) { + struct argv_array args = ARGV_ARRAY_INIT; + + argv_array_pushl(&args, "git", "add--interactive", "--patch", + "--", NULL); + for (i = 0; i < files->nr; i++) + if (selected[i]) + argv_array_push(&args, items[i]->name); + res = run_command_v_opt(args.argv, 0); + argv_array_clear(&args); + } + + free(selected); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *ps, struct file_list *files, struct list_and_choose_options *opts) @@ -867,10 +945,11 @@ int run_add_i(struct repository *r, const struct pathspec *ps) update = { { "update" }, run_update }, revert = { { "revert" }, run_revert }, add_untracked = { { "add untracked" }, run_add_untracked }, + patch = { { "patch" }, run_patch }, help = { { "help" }, run_help }; struct command_item *commands[] = { &status, &update, &revert, &add_untracked, - &help + &patch, &help }; struct print_file_item_data print_file_item_data = { From e753d1fef52f3bc96057d8255b9bcfe4f865d2b1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 8 Mar 2019 15:58:27 +0100 Subject: [PATCH 18/61] built-in add -i: re-implement the `diff` command It is not only laziness that we simply spawn `git diff -p --cached` here: this command needs to use the pager, and the pager needs to exit when the diff is done. Currently we do not have any way to make that happen if we run the diff in-process. So let's just spawn. Signed-off-by: Johannes Schindelin --- add-interactive.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index 2783247389..639e2b1149 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -852,6 +852,51 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps, return res; } +static int run_diff(struct add_i_state *s, const struct pathspec *ps, + struct file_list *files, + struct list_and_choose_options *opts) +{ + struct prefix_item **items = (struct prefix_item **)files->file; + int res = 0, *selected = NULL; + ssize_t count, i; + + struct object_id oid; + int is_initial = !resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &oid, + NULL); + reset_file_list(files); + if (get_modified_files(s->r, INDEX_ONLY, NULL, NULL, files, ps) < 0) + return -1; + + if (!files->nr) { + putchar('\n'); + return 0; + } + + opts->prompt = N_("Review diff"); + CALLOC_ARRAY(selected, files->nr); + + opts->flags = IMMEDIATE; + count = list_and_choose(items, selected, files->nr, s, opts); + opts->flags = 0; + if (count >= 0) { + struct argv_array args = ARGV_ARRAY_INIT; + + argv_array_pushl(&args, "git", "diff", "-p", "--cached", + oid_to_hex(!is_initial ? &oid : + s->r->hash_algo->empty_tree), + "--", NULL); + for (i = 0; i < files->nr; i++) + if (selected[i]) + argv_array_push(&args, items[i]->name); + res = run_command_v_opt(args.argv, 0); + argv_array_clear(&args); + } + + putchar('\n'); + free(selected); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *ps, struct file_list *files, struct list_and_choose_options *opts) @@ -946,10 +991,11 @@ int run_add_i(struct repository *r, const struct pathspec *ps) revert = { { "revert" }, run_revert }, add_untracked = { { "add untracked" }, run_add_untracked }, patch = { { "patch" }, run_patch }, + diff = { { "diff" }, run_diff }, help = { { "help" }, run_help }; struct command_item *commands[] = { &status, &update, &revert, &add_untracked, - &patch, &help + &patch, &diff, &help }; struct print_file_item_data print_file_item_data = { From ebeb73ac2917fecca3cfc771c481908ab2052d4b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 3 Apr 2019 22:15:39 +0200 Subject: [PATCH 19/61] t3701: add a test for advanced split-hunk editing In this developer's workflows, it often happens that a hunk needs to be edited in a way that adds lines, and even reduces the context Let's add a regression test for this. Note that just like the preceding test case, the new test case is *not* handled gracefully by the current `git add -p`. It will be handled correctly by the upcoming built-in `git add -p`, though. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 91aaef2932..986b48f3ad 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -403,6 +403,28 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' ! grep "^+31" actual ' +test_expect_failure 'edit, adding lines to the first hunk' ' + test_write_lines 10 11 20 30 40 50 51 60 >test && + git reset && + tr _ " " >patch <<-EOF && + @@ -1,5 +1,6 @@ + _10 + +11 + +12 + _20 + +21 + +22 + _30 + EOF + # test sequence is s(plit), e(dit), n(o) + # q n q q is there to make sure we exit at the end. + printf "%s\n" s e n q n q q | + EDITOR=./fake_editor.sh git add -p 2>error && + test_must_be_empty error && + git diff --cached >actual && + grep "^+22" actual +' + test_expect_success 'patch mode ignores unmerged entries' ' git reset --hard && test_commit conflict && From d7e18e953775c762338540f652eebf73bb980234 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 23:37:23 +0100 Subject: [PATCH 20/61] built-in add -i: offer the `quit` command We do not really want to `exit()` here, of course, as this is safely libified code. Signed-off-by: Johannes Schindelin --- add-interactive.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 639e2b1149..7bc999e929 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -992,10 +992,11 @@ int run_add_i(struct repository *r, const struct pathspec *ps) add_untracked = { { "add untracked" }, run_add_untracked }, patch = { { "patch" }, run_patch }, diff = { { "diff" }, run_diff }, + quit = { { "quit" }, NULL }, help = { { "help" }, run_help }; struct command_item *commands[] = { &status, &update, &revert, &add_untracked, - &patch, &diff, &help + &patch, &diff, &quit, &help }; struct print_file_item_data print_file_item_data = { @@ -1040,7 +1041,8 @@ int run_add_i(struct repository *r, const struct pathspec *ps) for (;;) { i = list_and_choose((struct prefix_item **)commands, NULL, ARRAY_SIZE(commands), &s, &main_loop_opts); - if (i == LIST_AND_CHOOSE_QUIT) + if (i == LIST_AND_CHOOSE_QUIT || + (i != LIST_AND_CHOOSE_ERROR && !commands[i]->command)) { printf(_("Bye.\n")); res = 0; break; From a51ce5f1263e9c254767bed08c4ae6ff4cb83586 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 20:05:24 +0100 Subject: [PATCH 21/61] t3701: avoid depending on the TTY prerequisite The TTY prerequisite is a rather heavy one: it not only requires Perl to work, but also the IO/Pty.pm module (with native support, and it requires pseudo terminals, too). In particular, test cases marked with the TTY prerequisite would be skipped in Git for Windows' SDK. In the case of `git add -p`, we do not actually need that big a hammer, as we do not want to test any functionality that requires a pseudo terminal; all we want is to talk the interactive add command to use color, even when being called from within the test suite. And we found exactly such a trick earlier already: when we added a test case to verify that the main loop of `git add -i` is colored appropriately. Let's use that trick instead of the TTY prerequisite. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 986b48f3ad..26ef510581 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -23,6 +23,17 @@ diff_cmp () { test_cmp "$1.filtered" "$2.filtered" } +# This function uses a trick to manipulate the interactive add to use color: +# the `want_color()` function special-cases the situation where a pager was +# spawned and Git now wants to output colored text: to detect that situation, +# the environment variable `GIT_PAGER_IN_USE` is set. However, color is +# suppressed despite that environment variable if the `TERM` variable +# indicates a dumb terminal, so we set that variable, too. + +force_color () { + env GIT_PAGER_IN_USE=true TERM=vt100 "$@" +} + test_expect_success 'setup (initial)' ' echo content >file && git add file && @@ -451,35 +462,35 @@ test_expect_success 'patch mode ignores unmerged entries' ' diff_cmp expected diff ' -test_expect_success TTY 'diffs can be colorized' ' +test_expect_success 'diffs can be colorized' ' git reset --hard && echo content >test && - printf y | test_terminal git add -p >output 2>&1 && + printf y | force_color git add -p >output 2>&1 && # We do not want to depend on the exact coloring scheme # git uses for diffs, so just check that we saw some kind of color. grep "$(printf "\\033")" output ' -test_expect_success TTY 'diffFilter filters diff' ' +test_expect_success 'diffFilter filters diff' ' git reset --hard && echo content >test && test_config interactive.diffFilter "sed s/^/foo:/" && - printf y | test_terminal git add -p >output 2>&1 && + printf y | force_color git add -p >output 2>&1 && # avoid depending on the exact coloring or content of the prompts, # and just make sure we saw our diff prefixed grep foo:.*content output ' -test_expect_success TTY 'detect bogus diffFilter output' ' +test_expect_success 'detect bogus diffFilter output' ' git reset --hard && echo content >test && test_config interactive.diffFilter "echo too-short" && - printf y | test_must_fail test_terminal git add -p + printf y | test_must_fail force_color git add -p ' test_expect_success 'patch-mode via -i prompts for files' ' @@ -680,7 +691,7 @@ test_expect_success 'show help from add--helper' ' What now>$SP Bye. EOF - test_write_lines h | GIT_PAGER_IN_USE=true TERM=vt100 git add -i >actual.colored && + test_write_lines h | force_color git add -i >actual.colored && test_decode_color actual && test_i18ncmp expect actual ' From 190f00bf3d912b2a1f400ae18feb5c03781fc130 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 20:36:47 +0100 Subject: [PATCH 22/61] t3701: add a test for the different `add -p` prompts The `git add -p` command offers different prompts for regular diff hunks vs mode change pseudo hunks vs diffs deleting files. Let's cover this in the regresion test suite, in preparation for re-implementing `git add -p` in C. For the mode change prompt, we use a trick that lets this test case pass even on systems without executable bit, i.e. where `core.filemode = false` (such as Windows): we first add the file to the index with `git add --chmod=+x`, and then call `git add -p` with `core.filemode` forced to `true`. The file on disk has no executable bit set, therefore we will see a mode change. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 26ef510581..f2a34b3c41 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -105,7 +105,6 @@ test_expect_success 'revert works (commit)' ' grep "unchanged *+3/-0 file" output ' - test_expect_success 'setup expected' ' cat >expected <<-\EOF EOF @@ -274,6 +273,24 @@ test_expect_success FILEMODE 'stage mode and hunk' ' # end of tests disabled when filemode is not usable +test_expect_success 'different prompts for mode change/deleted' ' + git reset --hard && + >file && + >deleted && + git add --chmod=+x file deleted && + echo changed >file && + rm deleted && + test_write_lines n n n | + git -c core.filemode=true add -p >actual && + sed -n "s/^\(Stage .*?\).*/\1/p" actual >actual.filtered && + cat >expect <<-\EOF && + Stage deletion [y,n,q,a,d,?]? + Stage mode change [y,n,q,a,d,j,J,g,/,?]? + Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + EOF + test_cmp expect actual.filtered +' + test_expect_success 'setup again' ' git reset --hard && test_chmod +x file && From c765ebaa25012465ce98316c6ec175ae89316f39 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 22:51:02 +0100 Subject: [PATCH 23/61] t3701: verify the shown messages when nothing can be added In preparation for re-implementing `git add -p` in pure C (where we will purposefully keep the implementation of `git add -p` separate from the implementation of `git add -i`), let's verify that the user is told the same things as in the Perl version when the diff file is either empty or contains only entries about binary files. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index f2a34b3c41..8e26fb9844 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -291,6 +291,17 @@ test_expect_success 'different prompts for mode change/deleted' ' test_cmp expect actual.filtered ' +test_expect_success 'correct message when there is nothing to do' ' + git reset --hard && + git add -p 2>err && + test_i18ngrep "No changes" err && + printf "\\0123" >binary && + git add binary && + printf "\\0abc" >binary && + git add -p 2>err && + test_i18ngrep "Only binary files changed" err +' + test_expect_success 'setup again' ' git reset --hard && test_chmod +x file && From 5c03929a6fff2f0bad40a7efe05376f464283351 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 19:36:22 +0100 Subject: [PATCH 24/61] t3701: verify that the diff.algorithm config setting is handled Without this patch, there is actually no test in Git's test suite that covers the diff.algorithm feature. Let's add one. We do this by passing a bogus value and then expecting `git diff-files` to produce the appropriate error message. Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 8e26fb9844..de8060dc78 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -521,6 +521,16 @@ test_expect_success 'detect bogus diffFilter output' ' printf y | test_must_fail force_color git add -p ' +test_expect_success 'diff.algorithm is passed to `git diff-files`' ' + git reset --hard && + + >file && + git add file && + echo changed >file && + git -c diff.algorithm=bogus add -p 2>err && + test_i18ngrep "error: option diff-algorithm accepts " err +' + test_expect_success 'patch-mode via -i prompts for files' ' git reset --hard && From 200c59e38d41274e1d4f4063bd9c98b1becb2ed3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 17:37:45 +0100 Subject: [PATCH 25/61] git add -p: use non-zero exit code when the diff generation failed The first thing `git add -p` does is to generate a diff. If this diff cannot be generated, `git add -p` should not continue as if nothing happened, but instead fail. What we *actually* do here is much broader: we now verify for *every* `run_cmd_pipe()` call that the spawned process actually succeeded. Note that we have to change two callers in this patch, as we need to store the spawned process' output in a local variable, which means that the callers can no longer decide whether to interpret the `return <$fh>` in array or in scalar context. This bug was noticed while writing a test case for the diff.algorithm feature, and we let that test case double as a regression test for this fixed bug, too. Signed-off-by: Johannes Schindelin --- git-add--interactive.perl | 8 +++++--- t/t3701-add-interactive.sh | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 20eb81cc92..841a33b630 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -163,7 +163,9 @@ sub run_cmd_pipe { } else { my $fh = undef; open($fh, '-|', @_) or die; - return <$fh>; + my @out = <$fh>; + close $fh || die "Cannot close @_ ($!)"; + return @out; } } @@ -210,7 +212,7 @@ my $status_head = sprintf($status_fmt, __('staged'), __('unstaged'), __('path')) sub get_empty_tree { return $empty_tree if defined $empty_tree; - $empty_tree = run_cmd_pipe(qw(git hash-object -t tree /dev/null)); + ($empty_tree) = run_cmd_pipe(qw(git hash-object -t tree /dev/null)); chomp $empty_tree; return $empty_tree; } @@ -1103,7 +1105,7 @@ aborted and the hunk is left unchanged. EOF2 close $fh; - chomp(my $editor = run_cmd_pipe(qw(git var GIT_EDITOR))); + chomp(my ($editor) = run_cmd_pipe(qw(git var GIT_EDITOR))); system('sh', '-c', $editor.' "$@"', $editor, $hunkfile); if ($? != 0) { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index de8060dc78..852a9f8516 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -527,7 +527,7 @@ test_expect_success 'diff.algorithm is passed to `git diff-files`' ' >file && git add file && echo changed >file && - git -c diff.algorithm=bogus add -p 2>err && + test_must_fail git -c diff.algorithm=bogus add -p 2>err && test_i18ngrep "error: option diff-algorithm accepts " err ' From 5f27f70fdfb67fa0be6d25cbdc889c1392dad3ba Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 20 Mar 2019 12:10:11 +0100 Subject: [PATCH 26/61] apply --allow-overlap: fix a corner case Yes, yes, this is supposed to be only a band-aid option for `git add -p` not Doing The Right Thing. But as long as we carry the `--allow-overlap` option, we might just as well get it right. This fixes the case where one hunk inserts a line before the first one, and a hunk whose context overlaps with the first one's appends a line at the end. Signed-off-by: Johannes Schindelin --- apply.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/apply.c b/apply.c index f15afa9f6a..2ab2f3bb99 100644 --- a/apply.c +++ b/apply.c @@ -2680,6 +2680,16 @@ static int find_pos(struct apply_state *state, unsigned long backwards, forwards, current; int backwards_lno, forwards_lno, current_lno; + /* + * When running with --allow-overlap, it is possible that a hunk is + * seen that pretends to start at the beginning (but no longer does), + * and that *still* needs to match the end. So trust `match_end` more + * than `match_beginning`. + */ + if (state->allow_overlap && match_beginning && match_end && + img->nr - preimage->nr != 0) + match_beginning = 0; + /* * If match_beginning or match_end is specified, there is no * point starting from a wrong line that will never match and From 537a885b1d0cd6b4bc297cb243bde34aa43206ae Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 11 Mar 2019 22:01:22 +0100 Subject: [PATCH 27/61] built-in add -i: start implementing the `patch` functionality in C In the previous steps, we re-implemented the main loop of `git add -i` in C, and most of the commands. Notably, we left out the actual functionality of `patch`, as the relevant code makes up more than half of `git-add--interactive.perl`, and is actually pretty independent of the rest of the commands. With this commit, we start to tackle that `patch` part. For better separation of concerns, we keep the code in a separate file, `add-patch.c`. The new code is still guarded behind the `add.interactive.useBuiltin` config setting, and for the moment, it can only be called via `git add -p`. The actual functionality follows the original implementation of 5cde71d64aff (git-add --interactive, 2006-12-10), but not too closely (for example, we use string offsets rather than copying strings around, and we also remember which previous/next hunk was undecided, rather than looking again when the user asked to jump there). As a further deviation from that commit, We also use a comma instead of a slash to separate the available commands in the prompt, as the current version of the Perl script does this, and we also add a line about the question mark ("print help") to the help text. Signed-off-by: Johannes Schindelin --- Makefile | 1 + add-interactive.h | 1 + add-patch.c | 260 ++++++++++++++++++++++++++++++++++++++++++++++ builtin/add.c | 10 +- 4 files changed, 270 insertions(+), 2 deletions(-) create mode 100644 add-patch.c diff --git a/Makefile b/Makefile index a350f63ab0..e42e8c6f8a 100644 --- a/Makefile +++ b/Makefile @@ -830,6 +830,7 @@ LIB_H := $(sort $(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null LIB_OBJS += abspath.o LIB_OBJS += add-interactive.o +LIB_OBJS += add-patch.o LIB_OBJS += advice.o LIB_OBJS += alias.o LIB_OBJS += alloc.o diff --git a/add-interactive.h b/add-interactive.h index 7043b8741d..0e3d93acc9 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -4,5 +4,6 @@ struct repository; struct pathspec; int run_add_i(struct repository *r, const struct pathspec *ps); +int run_add_p(struct repository *r, const struct pathspec *ps); #endif diff --git a/add-patch.c b/add-patch.c new file mode 100644 index 0000000000..7ff5d9e764 --- /dev/null +++ b/add-patch.c @@ -0,0 +1,260 @@ +#include "cache.h" +#include "add-interactive.h" +#include "strbuf.h" +#include "run-command.h" +#include "argv-array.h" +#include "pathspec.h" + +struct hunk { + size_t start, end; + enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use; +}; + +struct add_p_state { + struct repository *r; + struct strbuf answer, buf; + + /* parsed diff */ + struct strbuf plain; + struct hunk head; + struct hunk *hunk; + size_t hunk_nr, hunk_alloc; +}; + +static void setup_child_process(struct child_process *cp, + struct add_p_state *s, ...) +{ + va_list ap; + const char *arg; + + va_start(ap, s); + while((arg = va_arg(ap, const char *))) + argv_array_push(&cp->args, arg); + va_end(ap); + + cp->git_cmd = 1; + argv_array_pushf(&cp->env_array, + INDEX_ENVIRONMENT "=%s", s->r->index_file); +} + +static int parse_diff(struct add_p_state *s, const struct pathspec *ps) +{ + struct strbuf *plain = &s->plain; + struct child_process cp = CHILD_PROCESS_INIT; + char *p, *pend; + size_t i; + struct hunk *hunk = NULL; + int res; + + /* Use `--no-color` explicitly, just in case `diff.color = always`. */ + setup_child_process(&cp, s, + "diff-files", "-p", "--no-color", "--", NULL); + for (i = 0; i < ps->nr; i++) + argv_array_push(&cp.args, ps->items[i].original); + + res = capture_command(&cp, plain, 0); + if (res) + return error(_("could not parse diff")); + if (!plain->len) + return 0; + strbuf_complete_line(plain); + + /* parse hunks */ + p = plain->buf; + pend = p + plain->len; + while (p != pend) { + char *eol = memchr(p, '\n', pend - p); + if (!eol) + eol = pend; + + if (starts_with(p, "diff ")) { + if (p != plain->buf) + BUG("multi-file diff not yet handled"); + hunk = &s->head; + } else if (p == plain->buf) + BUG("diff starts with unexpected line:\n" + "%.*s\n", (int)(eol - p), p); + else if (starts_with(p, "@@ ")) { + s->hunk_nr++; + ALLOC_GROW(s->hunk, s->hunk_nr, + s->hunk_alloc); + hunk = s->hunk + s->hunk_nr - 1; + memset(hunk, 0, sizeof(*hunk)); + + hunk->start = p - plain->buf; + } + + p = eol == pend ? pend : eol + 1; + hunk->end = p - plain->buf; + } + + return 0; +} + +static void render_hunk(struct add_p_state *s, struct hunk *hunk, + struct strbuf *out) +{ + strbuf_add(out, s->plain.buf + hunk->start, + hunk->end - hunk->start); +} + +static void reassemble_patch(struct add_p_state *s, struct strbuf *out) +{ + struct hunk *hunk; + size_t i; + + render_hunk(s, &s->head, out); + + for (i = 0; i < s->hunk_nr; i++) { + hunk = s->hunk + i; + if (hunk->use == USE_HUNK) + render_hunk(s, hunk, out); + } +} + +static const char help_patch_text[] = +N_("y - stage this hunk\n" + "n - do not stage this hunk\n" + "a - stage this and all the remaining hunks\n" + "d - do not stage this hunk nor any of the remaining hunks\n" + "j - leave this hunk undecided, see next undecided hunk\n" + "J - leave this hunk undecided, see next hunk\n" + "k - leave this hunk undecided, see previous undecided hunk\n" + "K - leave this hunk undecided, see previous hunk\n" + "? - print help\n"); + +static int patch_update_file(struct add_p_state *s) +{ + size_t hunk_index = 0; + ssize_t i, undecided_previous, undecided_next; + struct hunk *hunk; + char ch; + struct child_process cp = CHILD_PROCESS_INIT; + + if (!s->hunk_nr) + return 0; + + strbuf_reset(&s->buf); + render_hunk(s, &s->head, &s->buf); + fputs(s->buf.buf, stdout); + for (;;) { + if (hunk_index >= s->hunk_nr) + hunk_index = 0; + hunk = s->hunk + hunk_index; + + undecided_previous = -1; + for (i = hunk_index - 1; i >= 0; i--) + if (s->hunk[i].use == UNDECIDED_HUNK) { + undecided_previous = i; + break; + } + + undecided_next = -1; + for (i = hunk_index + 1; i < s->hunk_nr; i++) + if (s->hunk[i].use == UNDECIDED_HUNK) { + undecided_next = i; + break; + } + + /* Everything decided? */ + if (undecided_previous < 0 && undecided_next < 0 && + hunk->use != UNDECIDED_HUNK) + break; + + strbuf_reset(&s->buf); + render_hunk(s, hunk, &s->buf); + fputs(s->buf.buf, stdout); + + strbuf_reset(&s->buf); + if (undecided_previous >= 0) + strbuf_addstr(&s->buf, ",k"); + if (hunk_index) + strbuf_addstr(&s->buf, ",K"); + if (undecided_next >= 0) + strbuf_addstr(&s->buf, ",j"); + if (hunk_index + 1 < s->hunk_nr) + strbuf_addstr(&s->buf, ",J"); + printf(_("Stage this hunk [y,n,a,d%s,?]? "), s->buf.buf); + fflush(stdout); + if (strbuf_getline(&s->answer, stdin) == EOF) + break; + strbuf_trim_trailing_newline(&s->answer); + + if (!s->answer.len) + continue; + ch = tolower(s->answer.buf[0]); + if (ch == 'y') { + hunk->use = USE_HUNK; +soft_increment: + while (++hunk_index < s->hunk_nr && + s->hunk[hunk_index].use + != UNDECIDED_HUNK) + ; /* continue looking */ + } else if (ch == 'n') { + hunk->use = SKIP_HUNK; + goto soft_increment; + } else if (ch == 'a') { + for (; hunk_index < s->hunk_nr; hunk_index++) { + hunk = s->hunk + hunk_index; + if (hunk->use == UNDECIDED_HUNK) + hunk->use = USE_HUNK; + } + } else if (ch == 'd') { + for (; hunk_index < s->hunk_nr; hunk_index++) { + hunk = s->hunk + hunk_index; + if (hunk->use == UNDECIDED_HUNK) + hunk->use = SKIP_HUNK; + } + } else if (hunk_index && s->answer.buf[0] == 'K') + hunk_index--; + else if (hunk_index + 1 < s->hunk_nr && + s->answer.buf[0] == 'J') + hunk_index++; + else if (undecided_previous >= 0 && + s->answer.buf[0] == 'k') + hunk_index = undecided_previous; + else if (undecided_next >= 0 && s->answer.buf[0] == 'j') + hunk_index = undecided_next; + else + puts(_(help_patch_text)); + } + + /* Any hunk to be used? */ + for (i = 0; i < s->hunk_nr; i++) + if (s->hunk[i].use == USE_HUNK) + break; + + if (i < s->hunk_nr) { + /* At least one hunk selected: apply */ + strbuf_reset(&s->buf); + reassemble_patch(s, &s->buf); + + setup_child_process(&cp, s, "apply", "--cached", NULL); + if (pipe_command(&cp, s->buf.buf, s->buf.len, + NULL, 0, NULL, 0)) + error(_("'git apply --cached' failed")); + repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0); + } + + putchar('\n'); + return 0; +} + +int run_add_p(struct repository *r, const struct pathspec *ps) +{ + struct add_p_state s = { r, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT }; + + if (repo_refresh_and_write_index(r, REFRESH_QUIET, 0) < 0 || + parse_diff(&s, ps) < 0) { + strbuf_release(&s.plain); + return -1; + } + + if (s.hunk_nr) + patch_update_file(&s); + + strbuf_release(&s.answer); + strbuf_release(&s.buf); + strbuf_release(&s.plain); + return 0; +} diff --git a/builtin/add.c b/builtin/add.c index 4f625691b5..cf02e63ae0 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -192,8 +192,14 @@ int run_add_interactive(const char *revision, const char *patch_mode, git_config_get_bool("add.interactive.usebuiltin", &use_builtin_add_i); - if (use_builtin_add_i == 1 && !patch_mode) - return !!run_add_i(the_repository, pathspec); + if (use_builtin_add_i == 1) { + if (!patch_mode) + return !!run_add_i(the_repository, pathspec); + if (strcmp(patch_mode, "--patch")) + die("'%s' not yet supported in the built-in add -p", + patch_mode); + return !!run_add_p(the_repository, pathspec); + } argv_array_push(&argv, "add--interactive"); if (patch_mode) From 9f495468397b7f1ba43cd6344c6fea431d1fa60c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 12:52:51 +0100 Subject: [PATCH 28/61] built-in add -i: wire up the new C code for the `patch` command The code in `git-add--interactive.perl` that takes care of the `patch` command can look quite intimidating. There are so many modes in which it can be called, for example. But for the `patch` command in `git add -i`, only one mode is relevant: the `stage` mode. And we just implemented the beginnings of that mode in C so far. So let's use it when `add.interactive.useBuiltin=true`. Now, while the code in `add-patch.c` is far from reaching feature parity with the code in `git-add--interactive.perl` (color is not implemented, the diff algorithm cannot be configured, the colored diff cannot be post-processed via `interactive.diffFilter`, many commands are unimplemented yet, etc), hooking it all up with the part of `git add -i` that is already converted to C makes it easier to test and develop it. Note: at this stage, both the `add.interactive.useBuiltin` config setting is still safely opt-in, and will probably be fore quite some time, to allow for thorough testing "in the wild" without adversely affecting existing users. Signed-off-by: Johannes Schindelin --- add-interactive.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 7bc999e929..051581fca5 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -838,14 +838,17 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps, count = list_and_choose(items, selected, files->nr, s, opts); if (count >= 0) { struct argv_array args = ARGV_ARRAY_INIT; + struct pathspec ps_selected = { 0 }; - argv_array_pushl(&args, "git", "add--interactive", "--patch", - "--", NULL); for (i = 0; i < files->nr; i++) if (selected[i]) argv_array_push(&args, items[i]->name); - res = run_command_v_opt(args.argv, 0); + parse_pathspec(&ps_selected, + PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL, + PATHSPEC_LITERAL_PATH, "", args.argv); + res = run_add_p(s->r, &ps_selected); argv_array_clear(&args); + clear_pathspec(&ps_selected); } free(selected); From e50e57b20154fe3e3d20098e280b7cd24f2adcf1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 15:42:10 +0100 Subject: [PATCH 29/61] built-in add -p: show colored hunks by default Just like the Perl version, we now generate two diffs if `color.diff` is set: one with and one without color. Then we parse them in parallel and record which hunks start at which offsets in both. Note that this is a (slight) deviation from the way the Perl version did it: we are no longer reading the output of `diff-files` line by line (which is more natural for Perl than for C), but in one go, and parse everything later, so we might just as well do it in synchrony. Signed-off-by: Johannes Schindelin --- add-patch.c | 79 +++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 62 insertions(+), 17 deletions(-) diff --git a/add-patch.c b/add-patch.c index 7ff5d9e764..a137036a7e 100644 --- a/add-patch.c +++ b/add-patch.c @@ -4,9 +4,10 @@ #include "run-command.h" #include "argv-array.h" #include "pathspec.h" +#include "color.h" struct hunk { - size_t start, end; + size_t start, end, colored_start, colored_end; enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use; }; @@ -15,7 +16,7 @@ struct add_p_state { struct strbuf answer, buf; /* parsed diff */ - struct strbuf plain; + struct strbuf plain, colored; struct hunk head; struct hunk *hunk; size_t hunk_nr, hunk_alloc; @@ -39,26 +40,50 @@ static void setup_child_process(struct child_process *cp, static int parse_diff(struct add_p_state *s, const struct pathspec *ps) { - struct strbuf *plain = &s->plain; + struct argv_array args = ARGV_ARRAY_INIT; + struct strbuf *plain = &s->plain, *colored = NULL; struct child_process cp = CHILD_PROCESS_INIT; - char *p, *pend; - size_t i; + char *p, *pend, *colored_p = NULL, *colored_pend = NULL; + size_t i, color_arg_index; struct hunk *hunk = NULL; int res; /* Use `--no-color` explicitly, just in case `diff.color = always`. */ - setup_child_process(&cp, s, - "diff-files", "-p", "--no-color", "--", NULL); + argv_array_pushl(&args, "diff-files", "-p", "--no-color", "--", NULL); + color_arg_index = args.argc - 2; for (i = 0; i < ps->nr; i++) - argv_array_push(&cp.args, ps->items[i].original); + argv_array_push(&args, ps->items[i].original); + setup_child_process(&cp, s, NULL); + cp.argv = args.argv; res = capture_command(&cp, plain, 0); - if (res) + if (res) { + argv_array_clear(&args); return error(_("could not parse diff")); - if (!plain->len) + } + if (!plain->len) { + argv_array_clear(&args); return 0; + } strbuf_complete_line(plain); + if (want_color_fd(1, -1)) { + struct child_process colored_cp = CHILD_PROCESS_INIT; + + setup_child_process(&colored_cp, s, NULL); + xsnprintf((char *)args.argv[color_arg_index], 8, "--color"); + colored_cp.argv = args.argv; + colored = &s->colored; + res = capture_command(&colored_cp, colored, 0); + argv_array_clear(&args); + if (res) + return error(_("could not parse colored diff")); + strbuf_complete_line(colored); + colored_p = colored->buf; + colored_pend = colored_p + colored->len; + } + argv_array_clear(&args); + /* parse hunks */ p = plain->buf; pend = p + plain->len; @@ -82,20 +107,37 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) memset(hunk, 0, sizeof(*hunk)); hunk->start = p - plain->buf; + if (colored) + hunk->colored_start = colored_p - colored->buf; } p = eol == pend ? pend : eol + 1; hunk->end = p - plain->buf; + + if (colored) { + char *colored_eol = memchr(colored_p, '\n', + colored_pend - colored_p); + if (colored_eol) + colored_p = colored_eol + 1; + else + colored_p = colored_pend; + + hunk->colored_end = colored_p - colored->buf; + } } return 0; } static void render_hunk(struct add_p_state *s, struct hunk *hunk, - struct strbuf *out) + int colored, struct strbuf *out) { - strbuf_add(out, s->plain.buf + hunk->start, - hunk->end - hunk->start); + if (colored) + strbuf_add(out, s->colored.buf + hunk->colored_start, + hunk->colored_end - hunk->colored_start); + else + strbuf_add(out, s->plain.buf + hunk->start, + hunk->end - hunk->start); } static void reassemble_patch(struct add_p_state *s, struct strbuf *out) @@ -103,12 +145,12 @@ static void reassemble_patch(struct add_p_state *s, struct strbuf *out) struct hunk *hunk; size_t i; - render_hunk(s, &s->head, out); + render_hunk(s, &s->head, 0, out); for (i = 0; i < s->hunk_nr; i++) { hunk = s->hunk + i; if (hunk->use == USE_HUNK) - render_hunk(s, hunk, out); + render_hunk(s, hunk, 0, out); } } @@ -130,12 +172,13 @@ static int patch_update_file(struct add_p_state *s) struct hunk *hunk; char ch; struct child_process cp = CHILD_PROCESS_INIT; + int colored = !!s->colored.len; if (!s->hunk_nr) return 0; strbuf_reset(&s->buf); - render_hunk(s, &s->head, &s->buf); + render_hunk(s, &s->head, colored, &s->buf); fputs(s->buf.buf, stdout); for (;;) { if (hunk_index >= s->hunk_nr) @@ -162,7 +205,7 @@ static int patch_update_file(struct add_p_state *s) break; strbuf_reset(&s->buf); - render_hunk(s, hunk, &s->buf); + render_hunk(s, hunk, colored, &s->buf); fputs(s->buf.buf, stdout); strbuf_reset(&s->buf); @@ -247,6 +290,7 @@ int run_add_p(struct repository *r, const struct pathspec *ps) if (repo_refresh_and_write_index(r, REFRESH_QUIET, 0) < 0 || parse_diff(&s, ps) < 0) { strbuf_release(&s.plain); + strbuf_release(&s.colored); return -1; } @@ -256,5 +300,6 @@ int run_add_p(struct repository *r, const struct pathspec *ps) strbuf_release(&s.answer); strbuf_release(&s.buf); strbuf_release(&s.plain); + strbuf_release(&s.colored); return 0; } From 913afe166e40bd7b317076e8b1b6a5496631cf0a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 21 Mar 2019 09:40:20 +0100 Subject: [PATCH 30/61] built-in add -p: adjust hunk headers as needed When skipping a hunk that adds a different number of lines than it removes, we need to adjust the subsequent hunk headers of non-skipped hunks: in pathological cases, the context is not enough to determine precisely where the patch should be applied. This problem was identified in 23fea4c240 (t3701: add failing test for pathological context lines, 2018-03-01) and fixed in the Perl version in fecc6f3a68 (add -p: adjust offsets of subsequent hunks when one is skipped, 2018-03-01). And this patch fixes it in the C version of `git add -p`. In contrast to the Perl version, we try to keep the extra text on the hunk header (which typically contains the signature of the function whose code is changed in the hunk) intact. Note: while the C version does not support staging mode changes at this stage, we already prepare for this by simply skipping the hunk header if both old and new offset is 0 (this cannot happen for regular hunks, and we will use this as an indicator that we are looking at a special hunk). Likewise, we already prepare for hunk splitting by handling the absence of extra text in the hunk header gracefully: only the first split hunk will have that text, the others will not (indicated by an empty extra text start/end range). Preparing for hunk splitting already at this stage avoids an indentation change of the entire hunk header-printing block later, and is almost as easy to review as without that handling. Signed-off-by: Johannes Schindelin --- add-interactive.c | 15 ++--- add-interactive.h | 15 +++++ add-patch.c | 139 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 148 insertions(+), 21 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 051581fca5..5020d09732 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -12,16 +12,6 @@ #include "argv-array.h" #include "run-command.h" -struct add_i_state { - struct repository *r; - int use_color; - char header_color[COLOR_MAXLEN]; - char help_color[COLOR_MAXLEN]; - char prompt_color[COLOR_MAXLEN]; - char error_color[COLOR_MAXLEN]; - char reset_color[COLOR_MAXLEN]; -}; - static void init_color(struct repository *r, struct add_i_state *s, const char *slot_name, char *dst, const char *default_color) @@ -38,7 +28,7 @@ static void init_color(struct repository *r, struct add_i_state *s, free(key); } -static int init_add_i_state(struct repository *r, struct add_i_state *s) +int init_add_i_state(struct repository *r, struct add_i_state *s) { const char *value; @@ -57,6 +47,9 @@ static int init_add_i_state(struct repository *r, struct add_i_state *s) init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED); init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET); + strlcpy(s->fraginfo_color, + diff_get_color(s->use_color, DIFF_FRAGINFO), COLOR_MAXLEN); + return 0; } diff --git a/add-interactive.h b/add-interactive.h index 0e3d93acc9..e3c6a8bff0 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -1,6 +1,21 @@ #ifndef ADD_INTERACTIVE_H #define ADD_INTERACTIVE_H +#include "color.h" + +struct add_i_state { + struct repository *r; + int use_color; + char header_color[COLOR_MAXLEN]; + char help_color[COLOR_MAXLEN]; + char prompt_color[COLOR_MAXLEN]; + char error_color[COLOR_MAXLEN]; + char reset_color[COLOR_MAXLEN]; + char fraginfo_color[COLOR_MAXLEN]; +}; + +int init_add_i_state(struct repository *r, struct add_i_state *s); + struct repository; struct pathspec; int run_add_i(struct repository *r, const struct pathspec *ps); diff --git a/add-patch.c b/add-patch.c index a137036a7e..88826ebedb 100644 --- a/add-patch.c +++ b/add-patch.c @@ -5,14 +5,26 @@ #include "argv-array.h" #include "pathspec.h" #include "color.h" +#include "diff.h" + +struct hunk_header { + unsigned long old_offset, old_count, new_offset, new_count; + /* + * Start/end offsets to the extra text after the second `@@` in the + * hunk header, e.g. the function signature. This is expected to + * include the newline. + */ + size_t extra_start, extra_end, colored_extra_start, colored_extra_end; +}; struct hunk { size_t start, end, colored_start, colored_end; enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use; + struct hunk_header header; }; struct add_p_state { - struct repository *r; + struct add_i_state s; struct strbuf answer, buf; /* parsed diff */ @@ -35,7 +47,70 @@ static void setup_child_process(struct child_process *cp, cp->git_cmd = 1; argv_array_pushf(&cp->env_array, - INDEX_ENVIRONMENT "=%s", s->r->index_file); + INDEX_ENVIRONMENT "=%s", s->s.r->index_file); +} + +static int parse_range(const char **p, + unsigned long *offset, unsigned long *count) +{ + char *pend; + + *offset = strtoul(*p, &pend, 10); + if (pend == *p) + return -1; + if (*pend != ',') { + *count = 1; + *p = pend; + return 0; + } + *count = strtoul(pend + 1, (char **)p, 10); + return *p == pend + 1 ? -1 : 0; +} + +static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk) +{ + struct hunk_header *header = &hunk->header; + const char *line = s->plain.buf + hunk->start, *p = line; + char *eol = memchr(p, '\n', s->plain.len - hunk->start); + + if (!eol) + eol = s->plain.buf + s->plain.len; + + if (!skip_prefix(p, "@@ -", &p) || + parse_range(&p, &header->old_offset, &header->old_count) < 0 || + !skip_prefix(p, " +", &p) || + parse_range(&p, &header->new_offset, &header->new_count) < 0 || + !skip_prefix(p, " @@", &p)) + return error(_("could not parse hunk header '%.*s'"), + (int)(eol - line), line); + + hunk->start = eol - s->plain.buf + (*eol == '\n'); + header->extra_start = p - s->plain.buf; + header->extra_end = hunk->start; + + if (!s->colored.len) { + header->colored_extra_start = header->colored_extra_end = 0; + return 0; + } + + /* Now find the extra text in the colored diff */ + line = s->colored.buf + hunk->colored_start; + eol = memchr(line, '\n', s->colored.len - hunk->colored_start); + if (!eol) + eol = s->colored.buf + s->colored.len; + p = memmem(line, eol - line, "@@ -", 4); + if (!p) + return error(_("could not parse colored hunk header '%.*s'"), + (int)(eol - line), line); + p = memmem(p + 4, eol - p - 4, " @@", 3); + if (!p) + return error(_("could not parse colored hunk header '%.*s'"), + (int)(eol - line), line); + hunk->colored_start = eol - s->colored.buf + (*eol == '\n'); + header->colored_extra_start = p + 3 - s->colored.buf; + header->colored_extra_end = hunk->colored_start; + + return 0; } static int parse_diff(struct add_p_state *s, const struct pathspec *ps) @@ -109,6 +184,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) hunk->start = p - plain->buf; if (colored) hunk->colored_start = colored_p - colored->buf; + + if (parse_hunk_header(s, hunk) < 0) + return -1; } p = eol == pend ? pend : eol + 1; @@ -130,8 +208,40 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) } static void render_hunk(struct add_p_state *s, struct hunk *hunk, - int colored, struct strbuf *out) + ssize_t delta, int colored, struct strbuf *out) { + struct hunk_header *header = &hunk->header; + + if (hunk->header.old_offset != 0 || hunk->header.new_offset != 0) { + /* + * Generate the hunk header dynamically, except for special + * hunks (such as the diff header). + */ + const char *p; + size_t len; + + if (!colored) { + p = s->plain.buf + header->extra_start; + len = header->extra_end - header->extra_start; + } else { + strbuf_addstr(out, s->s.fraginfo_color); + p = s->colored.buf + header->colored_extra_start; + len = header->colored_extra_end + - header->colored_extra_start; + } + + strbuf_addf(out, "@@ -%lu,%lu +%lu,%lu @@", + header->old_offset, header->old_count, + (unsigned long)(header->new_offset + delta), + header->new_count); + if (len) + strbuf_add(out, p, len); + else if (colored) + strbuf_addf(out, "%s\n", GIT_COLOR_RESET); + else + strbuf_addch(out, '\n'); + } + if (colored) strbuf_add(out, s->colored.buf + hunk->colored_start, hunk->colored_end - hunk->colored_start); @@ -144,13 +254,17 @@ static void reassemble_patch(struct add_p_state *s, struct strbuf *out) { struct hunk *hunk; size_t i; + ssize_t delta = 0; - render_hunk(s, &s->head, 0, out); + render_hunk(s, &s->head, 0, 0, out); for (i = 0; i < s->hunk_nr; i++) { hunk = s->hunk + i; - if (hunk->use == USE_HUNK) - render_hunk(s, hunk, 0, out); + if (hunk->use != USE_HUNK) + delta += hunk->header.old_count + - hunk->header.new_count; + else + render_hunk(s, hunk, delta, 0, out); } } @@ -178,7 +292,7 @@ static int patch_update_file(struct add_p_state *s) return 0; strbuf_reset(&s->buf); - render_hunk(s, &s->head, colored, &s->buf); + render_hunk(s, &s->head, 0, colored, &s->buf); fputs(s->buf.buf, stdout); for (;;) { if (hunk_index >= s->hunk_nr) @@ -205,7 +319,7 @@ static int patch_update_file(struct add_p_state *s) break; strbuf_reset(&s->buf); - render_hunk(s, hunk, colored, &s->buf); + render_hunk(s, hunk, 0, colored, &s->buf); fputs(s->buf.buf, stdout); strbuf_reset(&s->buf); @@ -276,7 +390,7 @@ soft_increment: if (pipe_command(&cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) error(_("'git apply --cached' failed")); - repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0); + repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0); } putchar('\n'); @@ -285,7 +399,12 @@ soft_increment: int run_add_p(struct repository *r, const struct pathspec *ps) { - struct add_p_state s = { r, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT }; + struct add_p_state s = { + { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT + }; + + if (init_add_i_state(r, &s.s)) + return error("Could not read `add -i` config"); if (repo_refresh_and_write_index(r, REFRESH_QUIET, 0) < 0 || parse_diff(&s, ps) < 0) { From f07e33795c025c209f708b41b05667e785c93951 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 16:40:46 +0100 Subject: [PATCH 31/61] built-in add -p: color the prompt and the help text ... just like the Perl version ;-) Note that this requires the `get_add_i_color()` function being defined globally, which is the entire reason why we gave it such a descriptive name in the first place. Signed-off-by: Johannes Schindelin --- add-interactive.h | 9 +++++++++ add-patch.c | 7 +++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/add-interactive.h b/add-interactive.h index e3c6a8bff0..eb024066ba 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -16,6 +16,15 @@ struct add_i_state { int init_add_i_state(struct repository *r, struct add_i_state *s); +enum color_add_i { + COLOR_HEADER = 0, + COLOR_HELP, + COLOR_PROMPT, + COLOR_ERROR, + COLOR_RESET, +}; +const char *get_add_i_color(enum color_add_i ix); + struct repository; struct pathspec; int run_add_i(struct repository *r, const struct pathspec *ps); diff --git a/add-patch.c b/add-patch.c index 88826ebedb..300d4cd1f5 100644 --- a/add-patch.c +++ b/add-patch.c @@ -331,7 +331,9 @@ static int patch_update_file(struct add_p_state *s) strbuf_addstr(&s->buf, ",j"); if (hunk_index + 1 < s->hunk_nr) strbuf_addstr(&s->buf, ",J"); - printf(_("Stage this hunk [y,n,a,d%s,?]? "), s->buf.buf); + color_fprintf(stdout, s->s.prompt_color, + _("Stage this hunk [y,n,a,d%s,?]? "), + s->buf.buf); fflush(stdout); if (strbuf_getline(&s->answer, stdin) == EOF) break; @@ -373,7 +375,8 @@ soft_increment: else if (undecided_next >= 0 && s->answer.buf[0] == 'j') hunk_index = undecided_next; else - puts(_(help_patch_text)); + color_fprintf(stdout, s->s.help_color, + _(help_patch_text)); } /* Any hunk to be used? */ From 35ed250e982030adecdafab5c86fa6a7276d840f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 17 Mar 2019 21:10:47 +0100 Subject: [PATCH 32/61] built-in add -p: offer a helpful error message when hunk navigation failed ... just like the Perl version currently does... Signed-off-by: Johannes Schindelin --- add-patch.c | 44 +++++++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/add-patch.c b/add-patch.c index 300d4cd1f5..19c3a0f6ea 100644 --- a/add-patch.c +++ b/add-patch.c @@ -34,6 +34,18 @@ struct add_p_state { size_t hunk_nr, hunk_alloc; }; +static void err(struct add_p_state *s, const char *fmt, ...) +{ + va_list args; + + va_start(args, fmt); + fputs(s->s.error_color, stderr); + vfprintf(stderr, fmt, args); + fputs(s->s.reset_color, stderr); + fputc('\n', stderr); + va_end(args); +} + static void setup_child_process(struct child_process *cp, struct add_p_state *s, ...) { @@ -364,17 +376,27 @@ soft_increment: if (hunk->use == UNDECIDED_HUNK) hunk->use = SKIP_HUNK; } - } else if (hunk_index && s->answer.buf[0] == 'K') - hunk_index--; - else if (hunk_index + 1 < s->hunk_nr && - s->answer.buf[0] == 'J') - hunk_index++; - else if (undecided_previous >= 0 && - s->answer.buf[0] == 'k') - hunk_index = undecided_previous; - else if (undecided_next >= 0 && s->answer.buf[0] == 'j') - hunk_index = undecided_next; - else + } else if (s->answer.buf[0] == 'K') { + if (hunk_index) + hunk_index--; + else + err(s, _("No previous hunk")); + } else if (s->answer.buf[0] == 'J') { + if (hunk_index + 1 < s->hunk_nr) + hunk_index++; + else + err(s, _("No next hunk")); + } else if (s->answer.buf[0] == 'k') { + if (undecided_previous >= 0) + hunk_index = undecided_previous; + else + err(s, _("No previous hunk")); + } else if (s->answer.buf[0] == 'j') { + if (undecided_next >= 0) + hunk_index = undecided_next; + else + err(s, _("No next hunk")); + } else color_fprintf(stdout, s->s.help_color, _(help_patch_text)); } From 28750154121c503ec63faa4526333a287b02185a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 15 Mar 2019 11:11:43 +0100 Subject: [PATCH 33/61] built-in add -p: support multi-file diffs For simplicity, the initial implementation in C handled only a single modified file. Now it handles an arbitrary number of files. Signed-off-by: Johannes Schindelin --- add-patch.c | 90 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 38 deletions(-) diff --git a/add-patch.c b/add-patch.c index 19c3a0f6ea..518c1daf0f 100644 --- a/add-patch.c +++ b/add-patch.c @@ -29,9 +29,12 @@ struct add_p_state { /* parsed diff */ struct strbuf plain, colored; - struct hunk head; - struct hunk *hunk; - size_t hunk_nr, hunk_alloc; + struct file_diff { + struct hunk head; + struct hunk *hunk; + size_t hunk_nr, hunk_alloc; + } *file_diff; + size_t file_diff_nr; }; static void err(struct add_p_state *s, const char *fmt, ...) @@ -131,7 +134,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) struct strbuf *plain = &s->plain, *colored = NULL; struct child_process cp = CHILD_PROCESS_INIT; char *p, *pend, *colored_p = NULL, *colored_pend = NULL; - size_t i, color_arg_index; + size_t file_diff_alloc = 0, i, color_arg_index; + struct file_diff *file_diff = NULL; struct hunk *hunk = NULL; int res; @@ -171,7 +175,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) } argv_array_clear(&args); - /* parse hunks */ + /* parse files and hunks */ p = plain->buf; pend = p + plain->len; while (p != pend) { @@ -180,17 +184,23 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) eol = pend; if (starts_with(p, "diff ")) { - if (p != plain->buf) - BUG("multi-file diff not yet handled"); - hunk = &s->head; + s->file_diff_nr++; + ALLOC_GROW(s->file_diff, s->file_diff_nr, + file_diff_alloc); + file_diff = s->file_diff + s->file_diff_nr - 1; + memset(file_diff, 0, sizeof(*file_diff)); + hunk = &file_diff->head; + hunk->start = p - plain->buf; + if (colored_p) + hunk->colored_start = colored_p - colored->buf; } else if (p == plain->buf) BUG("diff starts with unexpected line:\n" "%.*s\n", (int)(eol - p), p); else if (starts_with(p, "@@ ")) { - s->hunk_nr++; - ALLOC_GROW(s->hunk, s->hunk_nr, - s->hunk_alloc); - hunk = s->hunk + s->hunk_nr - 1; + file_diff->hunk_nr++; + ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, + file_diff->hunk_alloc); + hunk = file_diff->hunk + file_diff->hunk_nr - 1; memset(hunk, 0, sizeof(*hunk)); hunk->start = p - plain->buf; @@ -262,16 +272,17 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk, hunk->end - hunk->start); } -static void reassemble_patch(struct add_p_state *s, struct strbuf *out) +static void reassemble_patch(struct add_p_state *s, + struct file_diff *file_diff, struct strbuf *out) { struct hunk *hunk; size_t i; ssize_t delta = 0; - render_hunk(s, &s->head, 0, 0, out); + render_hunk(s, &file_diff->head, 0, 0, out); - for (i = 0; i < s->hunk_nr; i++) { - hunk = s->hunk + i; + for (i = 0; i < file_diff->hunk_nr; i++) { + hunk = file_diff->hunk + i; if (hunk->use != USE_HUNK) delta += hunk->header.old_count - hunk->header.new_count; @@ -291,7 +302,8 @@ N_("y - stage this hunk\n" "K - leave this hunk undecided, see previous hunk\n" "? - print help\n"); -static int patch_update_file(struct add_p_state *s) +static int patch_update_file(struct add_p_state *s, + struct file_diff *file_diff) { size_t hunk_index = 0; ssize_t i, undecided_previous, undecided_next; @@ -300,27 +312,27 @@ static int patch_update_file(struct add_p_state *s) struct child_process cp = CHILD_PROCESS_INIT; int colored = !!s->colored.len; - if (!s->hunk_nr) + if (!file_diff->hunk_nr) return 0; strbuf_reset(&s->buf); - render_hunk(s, &s->head, 0, colored, &s->buf); + render_hunk(s, &file_diff->head, 0, colored, &s->buf); fputs(s->buf.buf, stdout); for (;;) { - if (hunk_index >= s->hunk_nr) + if (hunk_index >= file_diff->hunk_nr) hunk_index = 0; - hunk = s->hunk + hunk_index; + hunk = file_diff->hunk + hunk_index; undecided_previous = -1; for (i = hunk_index - 1; i >= 0; i--) - if (s->hunk[i].use == UNDECIDED_HUNK) { + if (file_diff->hunk[i].use == UNDECIDED_HUNK) { undecided_previous = i; break; } undecided_next = -1; - for (i = hunk_index + 1; i < s->hunk_nr; i++) - if (s->hunk[i].use == UNDECIDED_HUNK) { + for (i = hunk_index + 1; i < file_diff->hunk_nr; i++) + if (file_diff->hunk[i].use == UNDECIDED_HUNK) { undecided_next = i; break; } @@ -341,7 +353,7 @@ static int patch_update_file(struct add_p_state *s) strbuf_addstr(&s->buf, ",K"); if (undecided_next >= 0) strbuf_addstr(&s->buf, ",j"); - if (hunk_index + 1 < s->hunk_nr) + if (hunk_index + 1 < file_diff->hunk_nr) strbuf_addstr(&s->buf, ",J"); color_fprintf(stdout, s->s.prompt_color, _("Stage this hunk [y,n,a,d%s,?]? "), @@ -357,22 +369,22 @@ static int patch_update_file(struct add_p_state *s) if (ch == 'y') { hunk->use = USE_HUNK; soft_increment: - while (++hunk_index < s->hunk_nr && - s->hunk[hunk_index].use + while (++hunk_index < file_diff->hunk_nr && + file_diff->hunk[hunk_index].use != UNDECIDED_HUNK) ; /* continue looking */ } else if (ch == 'n') { hunk->use = SKIP_HUNK; goto soft_increment; } else if (ch == 'a') { - for (; hunk_index < s->hunk_nr; hunk_index++) { - hunk = s->hunk + hunk_index; + for (; hunk_index < file_diff->hunk_nr; hunk_index++) { + hunk = file_diff->hunk + hunk_index; if (hunk->use == UNDECIDED_HUNK) hunk->use = USE_HUNK; } } else if (ch == 'd') { - for (; hunk_index < s->hunk_nr; hunk_index++) { - hunk = s->hunk + hunk_index; + for (; hunk_index < file_diff->hunk_nr; hunk_index++) { + hunk = file_diff->hunk + hunk_index; if (hunk->use == UNDECIDED_HUNK) hunk->use = SKIP_HUNK; } @@ -382,7 +394,7 @@ soft_increment: else err(s, _("No previous hunk")); } else if (s->answer.buf[0] == 'J') { - if (hunk_index + 1 < s->hunk_nr) + if (hunk_index + 1 < file_diff->hunk_nr) hunk_index++; else err(s, _("No next hunk")); @@ -402,14 +414,14 @@ soft_increment: } /* Any hunk to be used? */ - for (i = 0; i < s->hunk_nr; i++) - if (s->hunk[i].use == USE_HUNK) + for (i = 0; i < file_diff->hunk_nr; i++) + if (file_diff->hunk[i].use == USE_HUNK) break; - if (i < s->hunk_nr) { + if (i < file_diff->hunk_nr) { /* At least one hunk selected: apply */ strbuf_reset(&s->buf); - reassemble_patch(s, &s->buf); + reassemble_patch(s, file_diff, &s->buf); setup_child_process(&cp, s, "apply", "--cached", NULL); if (pipe_command(&cp, s->buf.buf, s->buf.len, @@ -427,6 +439,7 @@ int run_add_p(struct repository *r, const struct pathspec *ps) struct add_p_state s = { { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT }; + size_t i; if (init_add_i_state(r, &s.s)) return error("Could not read `add -i` config"); @@ -438,8 +451,9 @@ int run_add_p(struct repository *r, const struct pathspec *ps) return -1; } - if (s.hunk_nr) - patch_update_file(&s); + for (i = 0; i < s.file_diff_nr; i++) + if (patch_update_file(&s, s.file_diff + i)) + break; strbuf_release(&s.answer); strbuf_release(&s.buf); From d26f063d6c8f8396f5fbd7cd5d09197aa4811a9d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 15 Mar 2019 17:32:44 +0100 Subject: [PATCH 34/61] built-in add -p: handle deleted empty files This addresses the same problem as 24ab81ae4d (add-interactive: handle deletion of empty files, 2009-10-27), although in a different way: we not only stick the "deleted file" line into its own pseudo hunk, but also the entire remainder (if any) of the same diff. That way, we do not have to play any funny games with regards to coalescing the diff after the user selected what (possibly pseudo-)hunks to stage. Signed-off-by: Johannes Schindelin --- add-patch.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/add-patch.c b/add-patch.c index 518c1daf0f..00ef92f871 100644 --- a/add-patch.c +++ b/add-patch.c @@ -33,6 +33,7 @@ struct add_p_state { struct hunk head; struct hunk *hunk; size_t hunk_nr, hunk_alloc; + unsigned deleted:1; } *file_diff; size_t file_diff_nr; }; @@ -180,6 +181,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) pend = p + plain->len; while (p != pend) { char *eol = memchr(p, '\n', pend - p); + const char *deleted = NULL; + if (!eol) eol = pend; @@ -196,7 +199,11 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) } else if (p == plain->buf) BUG("diff starts with unexpected line:\n" "%.*s\n", (int)(eol - p), p); - else if (starts_with(p, "@@ ")) { + else if (file_diff->deleted) + ; /* keep the rest of the file in a single "hunk" */ + else if (starts_with(p, "@@ ") || + (hunk == &file_diff->head && + skip_prefix(p, "deleted file", &deleted))) { file_diff->hunk_nr++; ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc); @@ -207,7 +214,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) if (colored) hunk->colored_start = colored_p - colored->buf; - if (parse_hunk_header(s, hunk) < 0) + if (deleted) + file_diff->deleted = 1; + else if (parse_hunk_header(s, hunk) < 0) return -1; } From 99452543fe03a7acb58bb7a9c46e9022b1d8b5f0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 15 Mar 2019 17:59:11 +0100 Subject: [PATCH 35/61] built-in app -p: allow selecting a mode change as a "hunk" This imitates the way the Perl version treats mode changes: it offers the mode change up for the user to decide, as if it was a diff hunk. In contrast to the Perl version, we make use of the fact that the mode line is the first hunk, and explicitly strip out that line from the diff header if that "hunk" was not selected to be applied, and skipping that hunk while coalescing the diff. The Perl version plays some kind of diff line lego instead. Signed-off-by: Johannes Schindelin --- add-patch.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 75 insertions(+), 5 deletions(-) diff --git a/add-patch.c b/add-patch.c index 00ef92f871..3569e6f67e 100644 --- a/add-patch.c +++ b/add-patch.c @@ -33,7 +33,7 @@ struct add_p_state { struct hunk head; struct hunk *hunk; size_t hunk_nr, hunk_alloc; - unsigned deleted:1; + unsigned deleted:1, mode_change:1; } *file_diff; size_t file_diff_nr; }; @@ -129,6 +129,14 @@ static int parse_hunk_header(struct add_p_state *s, struct hunk *hunk) return 0; } +static int is_octal(const char *p, size_t len) +{ + while (len--) + if (*p < '0' || *(p++) > '7') + return 0; + return 1; +} + static int parse_diff(struct add_p_state *s, const struct pathspec *ps) { struct argv_array args = ARGV_ARRAY_INIT; @@ -181,7 +189,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) pend = p + plain->len; while (p != pend) { char *eol = memchr(p, '\n', pend - p); - const char *deleted = NULL; + const char *deleted = NULL, *mode_change = NULL; if (!eol) eol = pend; @@ -218,8 +226,30 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) file_diff->deleted = 1; else if (parse_hunk_header(s, hunk) < 0) return -1; + } else if (hunk == &file_diff->head && + ((skip_prefix(p, "old mode ", &mode_change) || + skip_prefix(p, "new mode ", &mode_change)) && + is_octal(mode_change, eol - mode_change))) { + if (!file_diff->mode_change) { + if (file_diff->hunk_nr++) + BUG("mode change before first hunk"); + ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, + file_diff->hunk_alloc); + memset(file_diff->hunk, 0, sizeof(struct hunk)); + file_diff->hunk->start = p - plain->buf; + if (colored_p) + file_diff->hunk->colored_start = + colored_p - colored->buf; + file_diff->mode_change = 1; + } else if (file_diff->hunk_nr != 1) + BUG("mode change after first hunk?"); } + if (file_diff->deleted && file_diff->mode_change) + BUG("diff contains delete *and* a mode change?!?\n%.*s", + (int)(eol - (plain->buf + file_diff->head.start)), + plain->buf + file_diff->head.start); + p = eol == pend ? pend : eol + 1; hunk->end = p - plain->buf; @@ -233,6 +263,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) hunk->colored_end = colored_p - colored->buf; } + + if (mode_change) { + file_diff->hunk->end = hunk->end; + if (colored_p) + file_diff->hunk->colored_end = + hunk->colored_end; + } } return 0; @@ -281,6 +318,39 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk, hunk->end - hunk->start); } +static void render_diff_header(struct add_p_state *s, + struct file_diff *file_diff, int colored, + struct strbuf *out) +{ + /* + * If there was a mode change, the first hunk is a pseudo hunk that + * corresponds to the mode line in the header. If the user did not want + * to stage that "hunk", we actually have to cut it out from the header. + */ + int skip_mode_change = + file_diff->mode_change && file_diff->hunk->use != USE_HUNK; + struct hunk *head = &file_diff->head, *first = file_diff->hunk; + + if (!skip_mode_change) { + render_hunk(s, head, 0, colored, out); + return; + } + + if (colored) { + const char *p = s->colored.buf; + + strbuf_add(out, p + head->colored_start, + first->colored_start - head->colored_start); + strbuf_add(out, p + first->colored_end, + head->colored_end - first->colored_end); + } else { + const char *p = s->plain.buf; + + strbuf_add(out, p + head->start, first->start - head->start); + strbuf_add(out, p + first->end, head->end - first->end); + } +} + static void reassemble_patch(struct add_p_state *s, struct file_diff *file_diff, struct strbuf *out) { @@ -288,9 +358,9 @@ static void reassemble_patch(struct add_p_state *s, size_t i; ssize_t delta = 0; - render_hunk(s, &file_diff->head, 0, 0, out); + render_diff_header(s, file_diff, 0, out); - for (i = 0; i < file_diff->hunk_nr; i++) { + for (i = file_diff->mode_change; i < file_diff->hunk_nr; i++) { hunk = file_diff->hunk + i; if (hunk->use != USE_HUNK) delta += hunk->header.old_count @@ -325,7 +395,7 @@ static int patch_update_file(struct add_p_state *s, return 0; strbuf_reset(&s->buf); - render_hunk(s, &file_diff->head, 0, colored, &s->buf); + render_diff_header(s, file_diff, colored, &s->buf); fputs(s->buf.buf, stdout); for (;;) { if (hunk_index >= file_diff->hunk_nr) From b3023a5d109ba3a5537a2576330464230a220413 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 21:33:55 +0100 Subject: [PATCH 36/61] built-in add -p: show different prompts for mode changes and deletions Just like the Perl version, we now helpfully ask the user whether they want to stage a mode change, or a deletion. Note that we define the prompts in an array, in preparation for a later patch that changes those prompts to yet different versions for `git reset -p`, `git stash -p` and `git checkout -p` (which all call the `git add -p` machinery to do the actual work). Signed-off-by: Johannes Schindelin --- add-patch.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/add-patch.c b/add-patch.c index 3569e6f67e..98048a56e9 100644 --- a/add-patch.c +++ b/add-patch.c @@ -7,6 +7,16 @@ #include "color.h" #include "diff.h" +enum prompt_mode_type { + PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK +}; + +static const char *prompt_mode[] = { + N_("Stage mode change [y,n,a,d%s,?]? "), + N_("Stage deletion [y,n,a,d%s,?]? "), + N_("Stage this hunk [y,n,a,d%s,?]? ") +}; + struct hunk_header { unsigned long old_offset, old_count, new_offset, new_count; /* @@ -390,6 +400,7 @@ static int patch_update_file(struct add_p_state *s, char ch; struct child_process cp = CHILD_PROCESS_INIT; int colored = !!s->colored.len; + enum prompt_mode_type prompt_mode_type; if (!file_diff->hunk_nr) return 0; @@ -434,9 +445,16 @@ static int patch_update_file(struct add_p_state *s, strbuf_addstr(&s->buf, ",j"); if (hunk_index + 1 < file_diff->hunk_nr) strbuf_addstr(&s->buf, ",J"); + + if (file_diff->deleted) + prompt_mode_type = PROMPT_DELETION; + else if (file_diff->mode_change && !hunk_index) + prompt_mode_type = PROMPT_MODE_CHANGE; + else + prompt_mode_type = PROMPT_HUNK; + color_fprintf(stdout, s->s.prompt_color, - _("Stage this hunk [y,n,a,d%s,?]? "), - s->buf.buf); + _(prompt_mode[prompt_mode_type]), s->buf.buf); fflush(stdout); if (strbuf_getline(&s->answer, stdin) == EOF) break; From 1775dfb6d698034a8dfbb6d495cb8816a27f19d4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 17 Mar 2019 21:12:41 +0100 Subject: [PATCH 37/61] built-in add -p: implement the hunk splitting feature If this developer's workflow is any indication, then this is *the* most useful feature of Git's interactive `add `command. Note: once again, this is not a verbatim conversion from the Perl code to C: the `hunk_splittable()` function, for example, essentially did all the work of splitting the hunk, just to find out whether more than one hunk would have been the result (and then tossed that result into the trash). In C we instead count the number of resulting hunks (without actually doing the work of splitting, but just counting the transitions from non-context lines to context lines), and store that information with the hunk, and we do that *while* parsing the diff in the first place. Another deviation: the built-in `git add -p` was designed with a single strbuf holding the diff (and another one holding the colored diff, if that one was asked for) in mind, and hunks essentially store just the start and end offsets pointing into that strbuf. As a consequence, when we split hunks, we now use a special mode where the hunk header is generated dynamically, and only the rest of the hunk is stored using such start/end offsets. This way, we also avoid the frequent formatting/re-parsing of the hunk header of the Perl version. Signed-off-by: Johannes Schindelin --- add-patch.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 182 insertions(+), 2 deletions(-) diff --git a/add-patch.c b/add-patch.c index 98048a56e9..4be4652cfd 100644 --- a/add-patch.c +++ b/add-patch.c @@ -28,7 +28,7 @@ struct hunk_header { }; struct hunk { - size_t start, end, colored_start, colored_end; + size_t start, end, colored_start, colored_end, splittable_into; enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use; struct hunk_header header; }; @@ -152,7 +152,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) struct argv_array args = ARGV_ARRAY_INIT; struct strbuf *plain = &s->plain, *colored = NULL; struct child_process cp = CHILD_PROCESS_INIT; - char *p, *pend, *colored_p = NULL, *colored_pend = NULL; + char *p, *pend, *colored_p = NULL, *colored_pend = NULL, marker = '\0'; size_t file_diff_alloc = 0, i, color_arg_index; struct file_diff *file_diff = NULL; struct hunk *hunk = NULL; @@ -222,6 +222,13 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) else if (starts_with(p, "@@ ") || (hunk == &file_diff->head && skip_prefix(p, "deleted file", &deleted))) { + if (marker == '-' || marker == '+') + /* + * Should not happen; previous hunk did not end + * in a context line? Handle it anyway. + */ + hunk->splittable_into++; + file_diff->hunk_nr++; ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc); @@ -236,6 +243,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) file_diff->deleted = 1; else if (parse_hunk_header(s, hunk) < 0) return -1; + + /* + * Start counting into how many hunks this one can be + * split + */ + marker = *p; } else if (hunk == &file_diff->head && ((skip_prefix(p, "old mode ", &mode_change) || skip_prefix(p, "new mode ", &mode_change)) && @@ -260,6 +273,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) (int)(eol - (plain->buf + file_diff->head.start)), plain->buf + file_diff->head.start); + if ((marker == '-' || marker == '+') && + (*p == ' ' || *p == '\\')) + hunk->splittable_into++; + if (marker) + marker = *p; + p = eol == pend ? pend : eol + 1; hunk->end = p - plain->buf; @@ -282,9 +301,25 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) } } + if (marker == '-' || marker == '+') + /* + * Last hunk ended in non-context line (i.e. it appended lines + * to the file, so there are no trailing context lines). + */ + hunk->splittable_into++; + return 0; } +static size_t find_next_line(struct strbuf *sb, size_t offset) +{ + char *eol = memchr(sb->buf + offset, '\n', sb->len - offset); + + if (!eol) + return sb->len; + return eol - sb->buf + 1; +} + static void render_hunk(struct add_p_state *s, struct hunk *hunk, ssize_t delta, int colored, struct strbuf *out) { @@ -380,6 +415,139 @@ static void reassemble_patch(struct add_p_state *s, } } +static int split_hunk(struct add_p_state *s, struct file_diff *file_diff, + size_t hunk_index) +{ + int colored = !!s->colored.len, first = 1; + struct hunk *hunk = file_diff->hunk + hunk_index; + size_t splittable_into; + size_t end, colored_end, current, colored_current = 0, context_line_count; + struct hunk_header remaining, *header; + char marker, ch; + + if (hunk_index >= file_diff->hunk_nr) + BUG("invalid hunk index: %d (must be >= 0 and < %d)", + (int)hunk_index, (int)file_diff->hunk_nr); + + if (hunk->splittable_into < 2) + return 0; + splittable_into = hunk->splittable_into; + + end = hunk->end; + colored_end = hunk->colored_end; + + memcpy(&remaining, &hunk->header, sizeof(remaining)); + + file_diff->hunk_nr += splittable_into - 1; + ALLOC_GROW(file_diff->hunk, file_diff->hunk_nr, file_diff->hunk_alloc); + if (hunk_index + splittable_into < file_diff->hunk_nr) + memmove(file_diff->hunk + hunk_index + splittable_into, + file_diff->hunk + hunk_index + 1, + (file_diff->hunk_nr - hunk_index - splittable_into) + * sizeof(*hunk)); + hunk = file_diff->hunk + hunk_index; + hunk->splittable_into = 1; + memset(hunk + 1, 0, (splittable_into - 1) * sizeof(*hunk)); + + header = &hunk->header; + header->old_count = header->new_count = 0; + + current = hunk->start; + if (colored) + colored_current = hunk->colored_start; + marker = '\0'; + context_line_count = 0; + + while (splittable_into > 1) { + ch = s->plain.buf[current]; + if ((marker == '-' || marker == '+') && ch == ' ') { + first = 0; + hunk[1].start = current; + if (colored) + hunk[1].colored_start = colored_current; + context_line_count = 0; + } + + if (marker != ' ' || (ch != '-' && ch != '+')) { +next_hunk_line: + /* current hunk not done yet */ + if (ch == ' ') + context_line_count++; + else if (ch == '-') + header->old_count++; + else if (ch == '+') + header->new_count++; + else + BUG("unhandled diff marker: '%c'", ch); + marker = ch; + current = find_next_line(&s->plain, current); + if (colored) + colored_current = + find_next_line(&s->colored, + colored_current); + continue; + } + + if (first) { + if (header->old_count || header->new_count) + BUG("counts are off: %d/%d", + (int)header->old_count, + (int)header->new_count); + + header->old_count = context_line_count; + header->new_count = context_line_count; + context_line_count = 0; + first = 0; + goto next_hunk_line; + } + + remaining.old_offset += header->old_count; + remaining.old_count -= header->old_count; + remaining.new_offset += header->new_count; + remaining.new_count -= header->new_count; + + /* initialize next hunk header's offsets */ + hunk[1].header.old_offset = + header->old_offset + header->old_count; + hunk[1].header.new_offset = + header->new_offset + header->new_count; + + /* add one split hunk */ + header->old_count += context_line_count; + header->new_count += context_line_count; + + hunk->end = current; + if (colored) + hunk->colored_end = colored_current; + + hunk++; + hunk->splittable_into = 1; + hunk->use = hunk[-1].use; + header = &hunk->header; + + header->old_count = header->new_count = context_line_count; + context_line_count = 0; + + splittable_into--; + marker = ch; + } + + /* last hunk simply gets the rest */ + if (header->old_offset != remaining.old_offset) + BUG("miscounted old_offset: %lu != %lu", + header->old_offset, remaining.old_offset); + if (header->new_offset != remaining.new_offset) + BUG("miscounted new_offset: %lu != %lu", + header->new_offset, remaining.new_offset); + header->old_count = remaining.old_count; + header->new_count = remaining.new_count; + hunk->end = end; + if (colored) + hunk->colored_end = colored_end; + + return 0; +} + static const char help_patch_text[] = N_("y - stage this hunk\n" "n - do not stage this hunk\n" @@ -389,6 +557,7 @@ N_("y - stage this hunk\n" "J - leave this hunk undecided, see next hunk\n" "k - leave this hunk undecided, see previous undecided hunk\n" "K - leave this hunk undecided, see previous hunk\n" + "s - split the current hunk into smaller hunks\n" "? - print help\n"); static int patch_update_file(struct add_p_state *s, @@ -445,6 +614,8 @@ static int patch_update_file(struct add_p_state *s, strbuf_addstr(&s->buf, ",j"); if (hunk_index + 1 < file_diff->hunk_nr) strbuf_addstr(&s->buf, ",J"); + if (hunk->splittable_into > 1) + strbuf_addstr(&s->buf, ",s"); if (file_diff->deleted) prompt_mode_type = PROMPT_DELETION; @@ -505,6 +676,15 @@ soft_increment: hunk_index = undecided_next; else err(s, _("No next hunk")); + } else if (s->answer.buf[0] == 's') { + size_t splittable_into = hunk->splittable_into; + if (splittable_into < 2) + err(s, _("Sorry, cannot split this hunk")); + else if (!split_hunk(s, file_diff, + hunk - file_diff->hunk)) + color_fprintf_ln(stdout, s->s.header_color, + _("Split into %d hunks."), + (int)splittable_into); } else color_fprintf(stdout, s->s.help_color, _(help_patch_text)); From 376a9a0f0e6bcba3c6d358ded26c96b54d504776 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 19 Mar 2019 20:50:14 +0100 Subject: [PATCH 38/61] built-in add -p: coalesce hunks after splitting them This is considered "the right thing to do", according to 933e44d3a0 ("add -p": work-around an old laziness that does not coalesce hunks, 2011-04-06). Note: we cannot simply modify the hunks while merging them; Once we implement hunk editing, we will call `reassemble_patch()` whenever a hunk is edited, therefore we must not modify the hunks (because the user might e.g. hit `K` and change their mind whether to stage the previous hunk). Signed-off-by: Johannes Schindelin --- add-patch.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/add-patch.c b/add-patch.c index 4be4652cfd..29513a7cdf 100644 --- a/add-patch.c +++ b/add-patch.c @@ -396,6 +396,48 @@ static void render_diff_header(struct add_p_state *s, } } +/* Coalesce hunks again that were split */ +static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff, + size_t *hunk_index, struct hunk *temp) +{ + size_t i = *hunk_index; + struct hunk *hunk = file_diff->hunk + i; + struct hunk_header *header = &temp->header, *next; + + if (hunk->use != USE_HUNK) + return 0; + + memcpy(temp, hunk, sizeof(*temp)); + /* We simply skip the colored part (if any) when merging hunks */ + temp->colored_start = temp->colored_end = 0; + + for (; i + 1 < file_diff->hunk_nr; i++) { + hunk++; + next = &hunk->header; + + if (hunk->use != USE_HUNK || + header->new_offset >= next->new_offset || + header->new_offset + header->new_count < next->new_offset || + temp->start >= hunk->start || + temp->end < hunk->start) + break; + + temp->end = hunk->end; + temp->colored_end = hunk->colored_end; + + header->old_count = next->old_offset + next->old_count + - header->old_offset; + header->new_count = next->new_offset + next->new_count + - header->new_offset; + } + + if (i == *hunk_index) + return 0; + + *hunk_index = i; + return 1; +} + static void reassemble_patch(struct add_p_state *s, struct file_diff *file_diff, struct strbuf *out) { @@ -406,12 +448,19 @@ static void reassemble_patch(struct add_p_state *s, render_diff_header(s, file_diff, 0, out); for (i = file_diff->mode_change; i < file_diff->hunk_nr; i++) { + struct hunk temp = { 0 }; + hunk = file_diff->hunk + i; if (hunk->use != USE_HUNK) delta += hunk->header.old_count - hunk->header.new_count; - else + else { + /* merge overlapping hunks into a temporary hunk */ + if (merge_hunks(s, file_diff, &i, &temp)) + hunk = &temp; + render_hunk(s, hunk, delta, 0, out); + } } } From 25ef8bb460098e69bb4bd827f1d5b2db8fa66fc2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 19 Mar 2019 22:27:51 +0100 Subject: [PATCH 39/61] built-in add -p: implement hunk editing Just like `git add --edit` allows the user to edit the diff before it is being applied to the index, this feature allows the user to edit the diff *hunk*. Naturally, it gets a bit more complicated here because the result has to play well with the remaining hunks of the overall diff. Therefore, we have to do a loop in which we let the user edit the hunk, then test whether the result would work, and if not, drop the edits and let the user decide whether to try editing the hunk again. Note: in contrast to the Perl version, we use the same diff "coalescing" (i.e. merging overlapping hunks into a single one) also for the check after editing, and we introduce a new flag for that purpose that asks the `reassemble_patch()` function to pretend that all hunks were selected for use. This allows us to continue to run `git apply` *without* the `--allow-overlap` option (unlike the Perl version), and it also fixes two known breakages in `t3701-add-interactive.sh` (which we cannot mark as resolved so far because the Perl script version is still the default and continues to have those breakages). Signed-off-by: Johannes Schindelin --- add-interactive.c | 7 + add-interactive.h | 3 + add-patch.c | 351 +++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 344 insertions(+), 17 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 5020d09732..2e262a7cba 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -49,6 +49,13 @@ int init_add_i_state(struct repository *r, struct add_i_state *s) strlcpy(s->fraginfo_color, diff_get_color(s->use_color, DIFF_FRAGINFO), COLOR_MAXLEN); + strlcpy(s->context_color, + diff_get_color(s->use_color, DIFF_CONTEXT), COLOR_MAXLEN); + strlcpy(s->file_old_color, + diff_get_color(s->use_color, DIFF_FILE_OLD), COLOR_MAXLEN); + strlcpy(s->file_new_color, + diff_get_color(s->use_color, DIFF_FILE_NEW), COLOR_MAXLEN); + return 0; } diff --git a/add-interactive.h b/add-interactive.h index eb024066ba..cc16b9436b 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -12,6 +12,9 @@ struct add_i_state { char error_color[COLOR_MAXLEN]; char reset_color[COLOR_MAXLEN]; char fraginfo_color[COLOR_MAXLEN]; + char context_color[COLOR_MAXLEN]; + char file_old_color[COLOR_MAXLEN]; + char file_new_color[COLOR_MAXLEN]; }; int init_add_i_state(struct repository *r, struct add_i_state *s); diff --git a/add-patch.c b/add-patch.c index 29513a7cdf..7ac7c16391 100644 --- a/add-patch.c +++ b/add-patch.c @@ -29,6 +29,7 @@ struct hunk_header { struct hunk { size_t start, end, colored_start, colored_end, splittable_into; + ssize_t delta; enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use; struct hunk_header header; }; @@ -398,13 +399,13 @@ static void render_diff_header(struct add_p_state *s, /* Coalesce hunks again that were split */ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff, - size_t *hunk_index, struct hunk *temp) + size_t *hunk_index, int use_all, struct hunk *temp) { - size_t i = *hunk_index; + size_t i = *hunk_index, delta; struct hunk *hunk = file_diff->hunk + i; struct hunk_header *header = &temp->header, *next; - if (hunk->use != USE_HUNK) + if (!use_all && hunk->use != USE_HUNK) return 0; memcpy(temp, hunk, sizeof(*temp)); @@ -415,20 +416,94 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff, hunk++; next = &hunk->header; - if (hunk->use != USE_HUNK || - header->new_offset >= next->new_offset || - header->new_offset + header->new_count < next->new_offset || - temp->start >= hunk->start || - temp->end < hunk->start) + if ((!use_all && hunk->use != USE_HUNK) || + header->new_offset >= next->new_offset + temp->delta || + header->new_offset + header->new_count + < next->new_offset + temp->delta) break; - temp->end = hunk->end; - temp->colored_end = hunk->colored_end; + if (temp->start < hunk->start && temp->end > hunk->start) { + temp->end = hunk->end; + temp->colored_end = hunk->colored_end; + delta = 0; + } else { + const char *plain = s->plain.buf; + size_t overlapping_line_count = header->new_offset + + header->new_count - temp->delta + - next->new_offset; + size_t overlap_end = hunk->start; + size_t overlap_start = overlap_end; + size_t overlap_next, len, i; + + /* + * One of the hunks was edited; let's ensure that at + * least the last context line of the first hunk + * overlaps with the corresponding line of the second + * hunk, and then merge. + */ + + for (i = 0; i < overlapping_line_count; i++) { + overlap_next = find_next_line(&s->plain, + overlap_end); + + if (overlap_next > hunk->end) + BUG("failed to find %d context lines " + "in:\n%.*s", + (int)overlapping_line_count, + (int)(hunk->end - hunk->start), + plain + hunk->start); + + if (plain[overlap_end] != ' ') + return error(_("expected context line " + "#%d in\n%.*s"), + (int)(i + 1), + (int)(hunk->end + - hunk->start), + plain + hunk->start); + + overlap_start = overlap_end; + overlap_end = overlap_next; + } + len = overlap_end - overlap_start; + + if (len > temp->end - temp->start || + memcmp(plain + temp->end - len, + plain + overlap_start, len)) + return error(_("hunks do not overlap:\n%.*s\n" + "\tdoes not end with:\n%.*s"), + (int)(temp->end - temp->start), + plain + temp->start, + (int)len, plain + overlap_start); + + /* + * Since the start-end ranges are not adjacent, we + * cannot simply take the union of the ranges. To + * address that, we temporarily append the union of the + * lines to the `plain` strbuf. + */ + if (temp->end != s->plain.len) { + size_t start = s->plain.len; + + strbuf_add(&s->plain, plain + temp->start, + temp->end - temp->start); + plain = s->plain.buf; + temp->start = start; + temp->end = s->plain.len; + } + + strbuf_add(&s->plain, + plain + overlap_end, + hunk->end - overlap_end); + temp->end = s->plain.len; + temp->splittable_into += hunk->splittable_into; + delta = temp->delta; + temp->delta += hunk->delta; + } header->old_count = next->old_offset + next->old_count - header->old_offset; - header->new_count = next->new_offset + next->new_count - - header->new_offset; + header->new_count = next->new_offset + delta + + next->new_count - header->new_offset; } if (i == *hunk_index) @@ -439,10 +514,11 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff, } static void reassemble_patch(struct add_p_state *s, - struct file_diff *file_diff, struct strbuf *out) + struct file_diff *file_diff, int use_all, + struct strbuf *out) { struct hunk *hunk; - size_t i; + size_t save_len = s->plain.len, i; ssize_t delta = 0; render_diff_header(s, file_diff, 0, out); @@ -451,15 +527,24 @@ static void reassemble_patch(struct add_p_state *s, struct hunk temp = { 0 }; hunk = file_diff->hunk + i; - if (hunk->use != USE_HUNK) + if (!use_all && hunk->use != USE_HUNK) delta += hunk->header.old_count - hunk->header.new_count; else { /* merge overlapping hunks into a temporary hunk */ - if (merge_hunks(s, file_diff, &i, &temp)) + if (merge_hunks(s, file_diff, &i, use_all, &temp)) hunk = &temp; render_hunk(s, hunk, delta, 0, out); + + /* + * In case `merge_hunks()` used `plain` as a scratch + * pad (this happens when an edited hunk had to be + * coalesced with another hunk). + */ + strbuf_setlen(&s->plain, save_len); + + delta += hunk->delta; } } } @@ -597,6 +682,227 @@ next_hunk_line: return 0; } +static void recolor_hunk(struct add_p_state *s, struct hunk *hunk) +{ + const char *plain = s->plain.buf; + size_t current, eol, next; + + if (!s->colored.len) + return; + + hunk->colored_start = s->colored.len; + for (current = hunk->start; current < hunk->end; ) { + for (eol = current; eol < hunk->end; eol++) + if (plain[eol] == '\n') + break; + next = eol + (eol < hunk->end); + if (eol > current && plain[eol - 1] == '\r') + eol--; + + strbuf_addstr(&s->colored, + plain[current] == '-' ? + s->s.file_old_color : + plain[current] == '+' ? + s->s.file_new_color : + s->s.context_color); + strbuf_add(&s->colored, plain + current, eol - current); + strbuf_addstr(&s->colored, GIT_COLOR_RESET); + if (next > eol) + strbuf_add(&s->colored, plain + eol, next - eol); + current = next; + } + hunk->colored_end = s->colored.len; +} + +static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk) +{ + char *path = xstrdup(git_path("addp-hunk-edit.diff")); + int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666); + struct strbuf buf = STRBUF_INIT; + size_t i, j; + int res, copy; + + if (fd < 0) { + res = error_errno(_("could not open '%s' for writing"), path); + goto edit_hunk_manually_finish; + } + + strbuf_commented_addf(&buf, _("Manual hunk edit mode -- see bottom for " + "a quick guide.\n")); + render_hunk(s, hunk, 0, 0, &buf); + strbuf_commented_addf(&buf, + _("---\n" + "To remove '%c' lines, make them ' ' lines " + "(context).\n" + "To remove '%c' lines, delete them.\n" + "Lines starting with %c will be removed.\n"), + '-', '+', comment_line_char); + strbuf_commented_addf(&buf, + _("If the patch applies cleanly, the edited hunk " + "will immediately be\n" + "marked for staging.\n")); + /* + * TRANSLATORS: 'it' refers to the patch mentioned in the previous + * messages. + */ + strbuf_commented_addf(&buf, + _("If it does not apply cleanly, you will be " + "given an opportunity to\n" + "edit again. If all lines of the hunk are " + "removed, then the edit is\n" + "aborted and the hunk is left unchanged.\n")); + if (write_in_full(fd, buf.buf, buf.len) < 0) { + res = error_errno(_("could not write to '%s'"), path); + goto edit_hunk_manually_finish; + } + + res = close(fd); + fd = -1; + if (res < 0) + goto edit_hunk_manually_finish; + + hunk->start = s->plain.len; + if (launch_editor(path, &s->plain, NULL) < 0) { + res = error_errno(_("could not edit '%s'"), path); + goto edit_hunk_manually_finish; + } + unlink(path); + + /* strip out commented lines */ + copy = s->plain.buf[hunk->start] != comment_line_char; + for (i = j = hunk->start; i < s->plain.len; ) { + if (copy) + s->plain.buf[j++] = s->plain.buf[i]; + if (s->plain.buf[i++] == '\n') + copy = s->plain.buf[i] != comment_line_char; + } + + if (j == hunk->start) + /* User aborted by deleting everything */ + goto edit_hunk_manually_finish; + + res = 1; + strbuf_setlen(&s->plain, j); + hunk->end = j; + recolor_hunk(s, hunk); + if (s->plain.buf[hunk->start] == '@' && + /* If the hunk header was deleted, simply use the original one. */ + parse_hunk_header(s, hunk) < 0) + res = -1; + +edit_hunk_manually_finish: + if (fd >= 0) + close(fd); + free(path); + strbuf_release(&buf); + + return res; +} + +static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk, + size_t orig_old_count, size_t orig_new_count) +{ + struct hunk_header *header = &hunk->header; + size_t i; + + header->old_count = header->new_count = 0; + for (i = hunk->start; i < hunk->end; ) { + switch (s->plain.buf[i]) { + case '-': + header->old_count++; + break; + case '+': + header->new_count++; + break; + case ' ': case '\r': case '\n': + header->old_count++; + header->new_count++; + break; + } + + i = find_next_line(&s->plain, i); + } + + return orig_old_count - orig_new_count + - header->old_count + header->new_count; +} + +static int run_apply_check(struct add_p_state *s, + struct file_diff *file_diff) +{ + struct child_process cp = CHILD_PROCESS_INIT; + + strbuf_reset(&s->buf); + reassemble_patch(s, file_diff, 1, &s->buf); + + setup_child_process(&cp, s, + "apply", "--cached", "--check", NULL); + if (pipe_command(&cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) + return error(_("'git apply --cached' failed")); + + return 0; +} + +static int prompt_yesno(struct add_p_state *s, const char *prompt) +{ + for (;;) { + color_fprintf(stdout, s->s.prompt_color, "%s", _(prompt)); + fflush(stdout); + if (strbuf_getline(&s->answer, stdin) == EOF) + return -1; + strbuf_trim_trailing_newline(&s->answer); + switch (tolower(s->answer.buf[0])) { + case 'n': return 0; + case 'y': return 1; + } + } +} + +static int edit_hunk_loop(struct add_p_state *s, + struct file_diff *file_diff, struct hunk *hunk) +{ + size_t plain_len = s->plain.len, colored_len = s->colored.len; + struct hunk backup; + + memcpy(&backup, hunk, sizeof(backup)); + + for (;;) { + int res = edit_hunk_manually(s, hunk); + if (res == 0) { + /* abandonded */ + memcpy(hunk, &backup, sizeof(backup)); + return -1; + } + + if (res > 0) { + hunk->delta += + recount_edited_hunk(s, hunk, + backup.header.old_count, + backup.header.new_count); + if (!run_apply_check(s, file_diff)) + return 0; + } + + /* Drop edits (they were appended to s->plain) */ + strbuf_setlen(&s->plain, plain_len); + strbuf_setlen(&s->colored, colored_len); + memcpy(hunk, &backup, sizeof(backup)); + + /* + * TRANSLATORS: do not translate [y/n] + * The program will only accept that input at this point. + * Consider translating (saying "no" discards!) as + * (saying "n" for "no" discards!) if the translation + * of the word "no" does not start with n. + */ + res = prompt_yesno(s, _("Your edited hunk does not apply. " + "Edit again (saying \"no\" discards!) " + "[y/n]? ")); + if (res < 1) + return -1; + } +} + static const char help_patch_text[] = N_("y - stage this hunk\n" "n - do not stage this hunk\n" @@ -607,6 +913,7 @@ N_("y - stage this hunk\n" "k - leave this hunk undecided, see previous undecided hunk\n" "K - leave this hunk undecided, see previous hunk\n" "s - split the current hunk into smaller hunks\n" + "e - manually edit the current hunk\n" "? - print help\n"); static int patch_update_file(struct add_p_state *s, @@ -665,6 +972,9 @@ static int patch_update_file(struct add_p_state *s, strbuf_addstr(&s->buf, ",J"); if (hunk->splittable_into > 1) strbuf_addstr(&s->buf, ",s"); + if (hunk_index + 1 > file_diff->mode_change && + !file_diff->deleted) + strbuf_addstr(&s->buf, ",e"); if (file_diff->deleted) prompt_mode_type = PROMPT_DELETION; @@ -734,6 +1044,13 @@ soft_increment: color_fprintf_ln(stdout, s->s.header_color, _("Split into %d hunks."), (int)splittable_into); + } else if (s->answer.buf[0] == 'e') { + if (hunk_index + 1 == file_diff->mode_change) + err(s, _("Sorry, cannot edit this hunk")); + else if (edit_hunk_loop(s, file_diff, hunk) >= 0) { + hunk->use = USE_HUNK; + goto soft_increment; + } } else color_fprintf(stdout, s->s.help_color, _(help_patch_text)); @@ -747,7 +1064,7 @@ soft_increment: if (i < file_diff->hunk_nr) { /* At least one hunk selected: apply */ strbuf_reset(&s->buf); - reassemble_patch(s, file_diff, &s->buf); + reassemble_patch(s, file_diff, 0, &s->buf); setup_child_process(&cp, s, "apply", "--cached", NULL); if (pipe_command(&cp, s->buf.buf, s->buf.len, From b95296b88ba492f3899c9edaca5475656c525939 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 21 Mar 2019 23:50:53 +0100 Subject: [PATCH 40/61] built-in add -p: implement the 'g' ("goto") command With this patch, it is now possible to see a summary of the available hunks and to navigate between them (by number). A test is added to verify that this behavior matches the one of the Perl version of `git add -p`. Signed-off-by: Johannes Schindelin --- add-patch.c | 86 ++++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 16 +++++++ 2 files changed, 102 insertions(+) diff --git a/add-patch.c b/add-patch.c index 7ac7c16391..cc990c5fc6 100644 --- a/add-patch.c +++ b/add-patch.c @@ -903,6 +903,54 @@ static int edit_hunk_loop(struct add_p_state *s, } } +#define SUMMARY_HEADER_WIDTH 20 +#define SUMMARY_LINE_WIDTH 80 +static void summarize_hunk(struct add_p_state *s, struct hunk *hunk, + struct strbuf *out) +{ + struct hunk_header *header = &hunk->header; + struct strbuf *plain = &s->plain; + size_t len = out->len, i; + + strbuf_addf(out, " -%lu,%lu +%lu,%lu ", + header->old_offset, header->old_count, + header->new_offset, header->new_count); + if (out->len - len < SUMMARY_HEADER_WIDTH) + strbuf_addchars(out, ' ', + SUMMARY_HEADER_WIDTH + len - out->len); + for (i = hunk->start; i < hunk->end; i = find_next_line(plain, i)) + if (plain->buf[i] != ' ') + break; + if (i < hunk->end) + strbuf_add(out, plain->buf + i, find_next_line(plain, i) - i); + if (out->len - len > SUMMARY_LINE_WIDTH) + strbuf_setlen(out, len + SUMMARY_LINE_WIDTH); + strbuf_complete_line(out); +} + +#define DISPLAY_HUNKS_LINES 20 +static size_t display_hunks(struct add_p_state *s, + struct file_diff *file_diff, size_t start_index) +{ + size_t end_index = start_index + DISPLAY_HUNKS_LINES; + + if (end_index > file_diff->hunk_nr) + end_index = file_diff->hunk_nr; + + while (start_index < end_index) { + struct hunk *hunk = file_diff->hunk + start_index++; + + strbuf_reset(&s->buf); + strbuf_addf(&s->buf, "%c%2d: ", hunk->use == USE_HUNK ? '+' + : hunk->use == SKIP_HUNK ? '-' : ' ', + (int)start_index); + summarize_hunk(s, hunk, &s->buf); + fputs(s->buf.buf, stdout); + } + + return end_index; +} + static const char help_patch_text[] = N_("y - stage this hunk\n" "n - do not stage this hunk\n" @@ -912,6 +960,7 @@ N_("y - stage this hunk\n" "J - leave this hunk undecided, see next hunk\n" "k - leave this hunk undecided, see previous undecided hunk\n" "K - leave this hunk undecided, see previous hunk\n" + "g - select a hunk to go to\n" "s - split the current hunk into smaller hunks\n" "e - manually edit the current hunk\n" "? - print help\n"); @@ -970,6 +1019,8 @@ static int patch_update_file(struct add_p_state *s, strbuf_addstr(&s->buf, ",j"); if (hunk_index + 1 < file_diff->hunk_nr) strbuf_addstr(&s->buf, ",J"); + if (file_diff->hunk_nr > 1) + strbuf_addstr(&s->buf, ",g"); if (hunk->splittable_into > 1) strbuf_addstr(&s->buf, ",s"); if (hunk_index + 1 > file_diff->mode_change && @@ -1035,6 +1086,41 @@ soft_increment: hunk_index = undecided_next; else err(s, _("No next hunk")); + } else if (s->answer.buf[0] == 'g') { + char *pend; + unsigned long response; + + if (file_diff->hunk_nr < 2) { + err(s, _("No other hunks to goto")); + continue; + } + strbuf_remove(&s->answer, 0, 1); + strbuf_trim(&s->answer); + i = hunk_index > 10 ? hunk_index - 10 : 0; + while (s->answer.len == 0) { + i = display_hunks(s, file_diff, i); + printf("%s", i < file_diff->hunk_nr ? + _("go to which hunk ( to see " + "more)? ") : _("go to which hunk? ")); + fflush(stdout); + if (strbuf_getline(&s->answer, + stdin) == EOF) + break; + strbuf_trim_trailing_newline(&s->answer); + } + + strbuf_trim(&s->answer); + response = strtoul(s->answer.buf, &pend, 10); + if (*pend || pend == s->answer.buf) + err(s, _("Invalid number: '%s'"), + s->answer.buf); + else if (0 < response && response <= file_diff->hunk_nr) + hunk_index = response - 1; + else + err(s, Q_("Sorry, only %d hunk available.", + "Sorry, only %d hunks available.", + file_diff->hunk_nr), + (int)file_diff->hunk_nr); } else if (s->answer.buf[0] == 's') { size_t splittable_into = hunk->splittable_into; if (splittable_into < 2) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 852a9f8516..32095d1cf9 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -413,6 +413,22 @@ test_expect_success 'split hunk setup' ' test_write_lines 10 15 20 21 22 23 24 30 40 50 60 >test ' +test_expect_success 'goto hunk' ' + test_when_finished "git reset" && + tr _ " " >expect <<-EOF && + Stage this hunk [y,n,q,a,d,K,g,/,e,?]? + 1: -1,2 +1,3 +15 + _ 2: -2,4 +3,8 +21 + go to which hunk? @@ -1,2 +1,3 @@ + _10 + +15 + _20 + Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_ + EOF + test_write_lines s y g 1 | git add -p >actual && + tail -n 7 actual.trimmed && + test_cmp expect actual.trimmed +' + test_expect_success 'split hunk "add -p (edit)"' ' # Split, say Edit and do nothing. Then: # From ce4a21898f085af37deb5aef38c906194f4a6105 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 21 Mar 2019 23:50:53 +0100 Subject: [PATCH 41/61] built-in add -p: implement the '/' ("search regex") command This patch implements the hunk searching feature in the C version of `git add -p`. A test is added to verify that this behavior matches the one of the Perl version of `git add -p`. Note that this involves a change of behavior: the Perl version uses (of course) the Perl flavor of regular expressions, while this patch uses the regcomp()/regexec(), i.e. POSIX extended regular expressions. In practice, this behavior change is unlikely to matter. Signed-off-by: Johannes Schindelin --- add-patch.c | 50 +++++++++++++++++++++++++++++++++++++- t/t3701-add-interactive.sh | 14 +++++++++++ 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/add-patch.c b/add-patch.c index cc990c5fc6..823874217b 100644 --- a/add-patch.c +++ b/add-patch.c @@ -961,6 +961,7 @@ N_("y - stage this hunk\n" "k - leave this hunk undecided, see previous undecided hunk\n" "K - leave this hunk undecided, see previous hunk\n" "g - select a hunk to go to\n" + "/ - search for a hunk matching the given regex\n" "s - split the current hunk into smaller hunks\n" "e - manually edit the current hunk\n" "? - print help\n"); @@ -1020,7 +1021,7 @@ static int patch_update_file(struct add_p_state *s, if (hunk_index + 1 < file_diff->hunk_nr) strbuf_addstr(&s->buf, ",J"); if (file_diff->hunk_nr > 1) - strbuf_addstr(&s->buf, ",g"); + strbuf_addstr(&s->buf, ",g,/"); if (hunk->splittable_into > 1) strbuf_addstr(&s->buf, ",s"); if (hunk_index + 1 > file_diff->mode_change && @@ -1121,6 +1122,53 @@ soft_increment: "Sorry, only %d hunks available.", file_diff->hunk_nr), (int)file_diff->hunk_nr); + } else if (s->answer.buf[0] == '/') { + regex_t regex; + int ret; + + if (file_diff->hunk_nr < 2) { + err(s, _("No other hunks to search")); + continue; + } + strbuf_remove(&s->answer, 0, 1); + strbuf_trim_trailing_newline(&s->answer); + if (s->answer.len == 0) { + printf("%s", _("search for regex? ")); + fflush(stdout); + if (strbuf_getline(&s->answer, + stdin) == EOF) + break; + strbuf_trim_trailing_newline(&s->answer); + if (s->answer.len == 0) + continue; + } + ret = regcomp(®ex, s->answer.buf, + REG_EXTENDED | REG_NOSUB | REG_NEWLINE); + if (ret) { + char errbuf[1024]; + + regerror(ret, ®ex, errbuf, sizeof(errbuf)); + err(s, _("Malformed search regexp %s: %s"), + s->answer.buf, errbuf); + continue; + } + i = hunk_index; + for (;;) { + /* render the hunk into a scratch buffer */ + render_hunk(s, file_diff->hunk + i, 0, 0, + &s->buf); + if (regexec(®ex, s->buf.buf, 0, NULL, 0) + != REG_NOMATCH) + break; + i++; + if (i == file_diff->hunk_nr) + i = 0; + if (i != hunk_index) + continue; + err(s, _("No hunk matches the given pattern")); + break; + } + hunk_index = i; } else if (s->answer.buf[0] == 's') { size_t splittable_into = hunk->splittable_into; if (splittable_into < 2) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 32095d1cf9..5f23d71aed 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -429,6 +429,20 @@ test_expect_success 'goto hunk' ' test_cmp expect actual.trimmed ' +test_expect_success 'navigate to hunk via regex' ' + test_when_finished "git reset" && + tr _ " " >expect <<-EOF && + Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,2 +1,3 @@ + _10 + +15 + _20 + Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]?_ + EOF + test_write_lines s y /1,2 | git add -p >actual && + tail -n 5 actual.trimmed && + test_cmp expect actual.trimmed +' + test_expect_success 'split hunk "add -p (edit)"' ' # Split, say Edit and do nothing. Then: # From 33e8225ca9f3498d27f1c1c68c0700e0f7c55851 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 22 Mar 2019 01:24:47 +0100 Subject: [PATCH 42/61] built-in add -p: implement the 'q' ("quit") command This command is actually very similar to the 'd' ("do not stage this hunk or any of the later hunks in the file") command: it just does something on top, namely leave the loop and return a value indicating that we're quittin'. Signed-off-by: Johannes Schindelin --- add-patch.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/add-patch.c b/add-patch.c index 823874217b..629861ce2a 100644 --- a/add-patch.c +++ b/add-patch.c @@ -12,9 +12,9 @@ enum prompt_mode_type { }; static const char *prompt_mode[] = { - N_("Stage mode change [y,n,a,d%s,?]? "), - N_("Stage deletion [y,n,a,d%s,?]? "), - N_("Stage this hunk [y,n,a,d%s,?]? ") + N_("Stage mode change [y,n,a,q,d%s,?]? "), + N_("Stage deletion [y,n,a,q,d%s,?]? "), + N_("Stage this hunk [y,n,a,q,d%s,?]? ") }; struct hunk_header { @@ -954,6 +954,7 @@ static size_t display_hunks(struct add_p_state *s, static const char help_patch_text[] = N_("y - stage this hunk\n" "n - do not stage this hunk\n" + "q - quit; do not stage this hunk or any of the remaining ones\n" "a - stage this and all the remaining hunks\n" "d - do not stage this hunk nor any of the remaining hunks\n" "j - leave this hunk undecided, see next undecided hunk\n" @@ -974,7 +975,7 @@ static int patch_update_file(struct add_p_state *s, struct hunk *hunk; char ch; struct child_process cp = CHILD_PROCESS_INIT; - int colored = !!s->colored.len; + int colored = !!s->colored.len, quit = 0; enum prompt_mode_type prompt_mode_type; if (!file_diff->hunk_nr) @@ -1061,12 +1062,16 @@ soft_increment: if (hunk->use == UNDECIDED_HUNK) hunk->use = USE_HUNK; } - } else if (ch == 'd') { + } else if (ch == 'd' || ch == 'q') { for (; hunk_index < file_diff->hunk_nr; hunk_index++) { hunk = file_diff->hunk + hunk_index; if (hunk->use == UNDECIDED_HUNK) hunk->use = SKIP_HUNK; } + if (ch == 'q') { + quit = 1; + break; + } } else if (s->answer.buf[0] == 'K') { if (hunk_index) hunk_index--; @@ -1208,7 +1213,7 @@ soft_increment: } putchar('\n'); - return 0; + return quit; } int run_add_p(struct repository *r, const struct pathspec *ps) From 074fbb1f5d0acd8958cd991f825321c39fd09218 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 23:26:58 +0100 Subject: [PATCH 43/61] built-in add -p: only show the applicable parts of the help text When displaying the only hunk in a file's diff, the prompt already excludes the commands to navigate to the previous/next hunk. Let's also let the `?` command show only the help lines corresponding to the commands that are displayed in the prompt. Signed-off-by: Johannes Schindelin --- add-patch.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/add-patch.c b/add-patch.c index 629861ce2a..c9257120a2 100644 --- a/add-patch.c +++ b/add-patch.c @@ -956,8 +956,10 @@ N_("y - stage this hunk\n" "n - do not stage this hunk\n" "q - quit; do not stage this hunk or any of the remaining ones\n" "a - stage this and all the remaining hunks\n" - "d - do not stage this hunk nor any of the remaining hunks\n" - "j - leave this hunk undecided, see next undecided hunk\n" + "d - do not stage this hunk nor any of the remaining hunks\n"); + +static const char help_patch_remainder[] = +N_("j - leave this hunk undecided, see next undecided hunk\n" "J - leave this hunk undecided, see next hunk\n" "k - leave this hunk undecided, see previous undecided hunk\n" "K - leave this hunk undecided, see previous hunk\n" @@ -1190,9 +1192,31 @@ soft_increment: hunk->use = USE_HUNK; goto soft_increment; } - } else - color_fprintf(stdout, s->s.help_color, + } else { + const char *p = _(help_patch_remainder), *eol = p; + + color_fprintf(stdout, s->s.help_color, "%s", _(help_patch_text)); + + /* + * Show only those lines of the remainder that are + * actually applicable with the current hunk. + */ + for (; *p; p = eol + (*eol == '\n')) { + eol = strchrnul(p, '\n'); + + /* + * `s->buf` still contains the part of the + * commands shown in the prompt that are not + * always available. + */ + if (*p != '?' && !strchr(s->buf.buf, *p)) + continue; + + color_fprintf_ln(stdout, s->s.help_color, + "%.*s", (int)(eol - p), p); + } + } } /* Any hunk to be used? */ From a2f2942a12d173be323a8d7eb5b049ee3f86737d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 22:54:01 +0100 Subject: [PATCH 44/61] built-in add -p: show helpful hint when nothing can be staged This patch will make `git add -p` show "No changes." or "Only binary files changed." in that case. Signed-off-by: Johannes Schindelin --- add-patch.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/add-patch.c b/add-patch.c index c9257120a2..4f9cb13017 100644 --- a/add-patch.c +++ b/add-patch.c @@ -44,7 +44,7 @@ struct add_p_state { struct hunk head; struct hunk *hunk; size_t hunk_nr, hunk_alloc; - unsigned deleted:1, mode_change:1; + unsigned deleted:1, mode_change:1,binary:1; } *file_diff; size_t file_diff_nr; }; @@ -267,7 +267,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) file_diff->mode_change = 1; } else if (file_diff->hunk_nr != 1) BUG("mode change after first hunk?"); - } + } else if (hunk == &file_diff->head && + starts_with(p, "Binary files ")) + file_diff->binary = 1; if (file_diff->deleted && file_diff->mode_change) BUG("diff contains delete *and* a mode change?!?\n%.*s", @@ -1245,7 +1247,7 @@ int run_add_p(struct repository *r, const struct pathspec *ps) struct add_p_state s = { { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT }; - size_t i; + size_t i, binary_count = 0; if (init_add_i_state(r, &s.s)) return error("Could not read `add -i` config"); @@ -1258,9 +1260,16 @@ int run_add_p(struct repository *r, const struct pathspec *ps) } for (i = 0; i < s.file_diff_nr; i++) - if (patch_update_file(&s, s.file_diff + i)) + if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) + binary_count++; + else if (patch_update_file(&s, s.file_diff + i)) break; + if (s.file_diff_nr == 0) + fprintf(stderr, _("No changes.\n")); + else if (binary_count == s.file_diff_nr) + fprintf(stderr, _("Only binary files changed.\n")); + strbuf_release(&s.answer); strbuf_release(&s.buf); strbuf_release(&s.plain); From 89a9789d57892ebae5edbc130cfc0c63dfe07db4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 22:33:49 +0100 Subject: [PATCH 45/61] built-in add -p: prepare for patch modes other than "stage" The Perl script backing `git add -p` is used not only for that command, but also for `git stash -p`, `git reset -p` and `git checkout -p`. In preparation for teaching the C version of `git add -p` to support also the latter commands, let's abstract away what is "stage" specific into a dedicated data structure describing the differences between the patch modes. As we prepare for calling the built-in `git add -p` in `run_add_interactive()` via code paths that have not let `add_config()` do its work, we have to make sure to re-parse the config using that function in those cases. Finally, please note that the Perl version tries to make sure that the diffs are only generated for the modified files. This is not actually necessary, as the calls to Git's diff machinery already perform that work, and perform it well. This makes it unnecessary to port the `FILTER` field of the `%patch_modes` struct, as well as the `get_diff_reference()` function. Signed-off-by: Johannes Schindelin --- add-interactive.c | 2 +- add-interactive.h | 8 ++++- add-patch.c | 83 +++++++++++++++++++++++++++++++++-------------- builtin/add.c | 12 +++++-- 4 files changed, 77 insertions(+), 28 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 2e262a7cba..c9336416fe 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -846,7 +846,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps, parse_pathspec(&ps_selected, PATHSPEC_ALL_MAGIC & ~PATHSPEC_LITERAL, PATHSPEC_LITERAL_PATH, "", args.argv); - res = run_add_p(s->r, &ps_selected); + res = run_add_p(s->r, ADD_P_STAGE, NULL, &ps_selected); argv_array_clear(&args); clear_pathspec(&ps_selected); } diff --git a/add-interactive.h b/add-interactive.h index cc16b9436b..77c47b7ad1 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -31,6 +31,12 @@ const char *get_add_i_color(enum color_add_i ix); struct repository; struct pathspec; int run_add_i(struct repository *r, const struct pathspec *ps); -int run_add_p(struct repository *r, const struct pathspec *ps); + +enum add_p_mode { + ADD_P_STAGE, +}; + +int run_add_p(struct repository *r, enum add_p_mode mode, + const char *revision, const struct pathspec *ps); #endif diff --git a/add-patch.c b/add-patch.c index 4f9cb13017..853fa90e75 100644 --- a/add-patch.c +++ b/add-patch.c @@ -11,10 +11,33 @@ enum prompt_mode_type { PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK }; -static const char *prompt_mode[] = { - N_("Stage mode change [y,n,a,q,d%s,?]? "), - N_("Stage deletion [y,n,a,q,d%s,?]? "), - N_("Stage this hunk [y,n,a,q,d%s,?]? ") +struct patch_mode { + const char *diff[4], *apply[4], *apply_check[4]; + unsigned is_reverse:1, apply_for_checkout:1; + const char *prompt_mode[PROMPT_HUNK + 1]; + const char *edit_hunk_hint, *help_patch_text; +}; + +static struct patch_mode patch_mode_stage = { + .diff = { "diff-files", NULL }, + .apply = { "--cached", NULL }, + .apply_check = { "--cached", NULL }, + .is_reverse = 0, + .prompt_mode = { + N_("Stage mode change [y,n,q,a,d%s,?]? "), + N_("Stage deletion [y,n,q,a,d%s,?]? "), + N_("Stage this hunk [y,n,q,a,d%s,?]? ") + }, + .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk " + "will immediately be marked for staging."), + .help_patch_text = + N_("y - stage this hunk\n" + "n - do not stage this hunk\n" + "q - quit; do not stage this hunk or any of the remaining " + "ones\n" + "a - stage this hunk and all later hunks in the file\n" + "d - do not stage this hunk or any of the later hunks in " + "the file\n") }; struct hunk_header { @@ -47,6 +70,10 @@ struct add_p_state { unsigned deleted:1, mode_change:1,binary:1; } *file_diff; size_t file_diff_nr; + + /* patch mode */ + struct patch_mode *mode; + const char *revision; }; static void err(struct add_p_state *s, const char *fmt, ...) @@ -159,9 +186,18 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) struct hunk *hunk = NULL; int res; + argv_array_pushv(&args, s->mode->diff); + if (s->revision) { + struct object_id oid; + argv_array_push(&args, + /* could be on an unborn branch */ + !strcmp("HEAD", s->revision) && + get_oid("HEAD", &oid) ? + empty_tree_oid_hex() : s->revision); + } + color_arg_index = args.argc; /* Use `--no-color` explicitly, just in case `diff.color = always`. */ - argv_array_pushl(&args, "diff-files", "-p", "--no-color", "--", NULL); - color_arg_index = args.argc - 2; + argv_array_pushl(&args, "--no-color", "-p", "--", NULL); for (i = 0; i < ps->nr; i++) argv_array_push(&args, ps->items[i].original); @@ -738,11 +774,10 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk) "(context).\n" "To remove '%c' lines, delete them.\n" "Lines starting with %c will be removed.\n"), - '-', '+', comment_line_char); - strbuf_commented_addf(&buf, - _("If the patch applies cleanly, the edited hunk " - "will immediately be\n" - "marked for staging.\n")); + s->mode->is_reverse ? '+' : '-', + s->mode->is_reverse ? '-' : '+', + comment_line_char); + strbuf_commented_addf(&buf, "%s", _(s->mode->edit_hunk_hint)); /* * TRANSLATORS: 'it' refers to the patch mentioned in the previous * messages. @@ -838,7 +873,8 @@ static int run_apply_check(struct add_p_state *s, reassemble_patch(s, file_diff, 1, &s->buf); setup_child_process(&cp, s, - "apply", "--cached", "--check", NULL); + "apply", "--check", NULL); + argv_array_pushv(&cp.args, s->mode->apply_check); if (pipe_command(&cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) return error(_("'git apply --cached' failed")); @@ -953,13 +989,6 @@ static size_t display_hunks(struct add_p_state *s, return end_index; } -static const char help_patch_text[] = -N_("y - stage this hunk\n" - "n - do not stage this hunk\n" - "q - quit; do not stage this hunk or any of the remaining ones\n" - "a - stage this and all the remaining hunks\n" - "d - do not stage this hunk nor any of the remaining hunks\n"); - static const char help_patch_remainder[] = N_("j - leave this hunk undecided, see next undecided hunk\n" "J - leave this hunk undecided, see next hunk\n" @@ -1041,7 +1070,8 @@ static int patch_update_file(struct add_p_state *s, prompt_mode_type = PROMPT_HUNK; color_fprintf(stdout, s->s.prompt_color, - _(prompt_mode[prompt_mode_type]), s->buf.buf); + _(s->mode->prompt_mode[prompt_mode_type]), + s->buf.buf); fflush(stdout); if (strbuf_getline(&s->answer, stdin) == EOF) break; @@ -1198,7 +1228,7 @@ soft_increment: const char *p = _(help_patch_remainder), *eol = p; color_fprintf(stdout, s->s.help_color, "%s", - _(help_patch_text)); + _(s->mode->help_patch_text)); /* * Show only those lines of the remainder that are @@ -1231,10 +1261,11 @@ soft_increment: strbuf_reset(&s->buf); reassemble_patch(s, file_diff, 0, &s->buf); - setup_child_process(&cp, s, "apply", "--cached", NULL); + setup_child_process(&cp, s, "apply", NULL); + argv_array_pushv(&cp.args, s->mode->apply); if (pipe_command(&cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0)) - error(_("'git apply --cached' failed")); + error(_("'git apply' failed")); repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0); } @@ -1242,7 +1273,8 @@ soft_increment: return quit; } -int run_add_p(struct repository *r, const struct pathspec *ps) +int run_add_p(struct repository *r, enum add_p_mode mode, + const char *revision, const struct pathspec *ps) { struct add_p_state s = { { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT @@ -1252,6 +1284,9 @@ int run_add_p(struct repository *r, const struct pathspec *ps) if (init_add_i_state(r, &s.s)) return error("Could not read `add -i` config"); + s.mode = &patch_mode_stage; + s.revision = revision; + if (repo_refresh_and_write_index(r, REFRESH_QUIET, 0) < 0 || parse_diff(&s, ps) < 0) { strbuf_release(&s.plain); diff --git a/builtin/add.c b/builtin/add.c index cf02e63ae0..6806637185 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -181,6 +181,8 @@ static void refresh(int verbose, const struct pathspec *pathspec) free(seen); } +static int add_config(const char *var, const char *value, void *cb); + int run_add_interactive(const char *revision, const char *patch_mode, const struct pathspec *pathspec) { @@ -193,12 +195,18 @@ int run_add_interactive(const char *revision, const char *patch_mode, &use_builtin_add_i); if (use_builtin_add_i == 1) { + enum add_p_mode mode; + if (!patch_mode) return !!run_add_i(the_repository, pathspec); - if (strcmp(patch_mode, "--patch")) + + if (!strcmp(patch_mode, "--patch")) + mode = ADD_P_STAGE; + else die("'%s' not yet supported in the built-in add -p", patch_mode); - return !!run_add_p(the_repository, pathspec); + + return !!run_add_p(the_repository, mode, revision, pathspec); } argv_array_push(&argv, "add--interactive"); From a54aeb0535918d28ea3a633c467ab03e3cc01a1d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 19:55:08 +0100 Subject: [PATCH 46/61] stash -p: respect the add.interactive.usebuiltin setting As `git add` traditionally did not expose the `--patch=` modes via command-line options, `git stash` had to call `git add--interactive` directly. But this prevents the built-in `add -p` from kicking in, as `add--interactive` is the Perl script. So let's introduce support for an optional `` argument in `git add --patch[=]`, and use that in `git stash -p`, so that the built-in interactive add can do its job if configured. Signed-off-by: Johannes Schindelin --- builtin/add.c | 21 +++++++++++++++------ builtin/commit.c | 3 ++- commit.h | 3 ++- git-legacy-stash.sh | 2 +- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index 6806637185..fc76019743 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -26,7 +26,8 @@ static const char * const builtin_add_usage[] = { N_("git add [] [--] ..."), NULL }; -static int patch_interactive, add_interactive, edit_interactive; +static const char *patch_interactive; +static int add_interactive, edit_interactive; static int take_worktree_changes; static int add_renormalize; @@ -224,9 +225,11 @@ int run_add_interactive(const char *revision, const char *patch_mode, return status; } -int interactive_add(int argc, const char **argv, const char *prefix, int patch) +int interactive_add(int argc, const char **argv, const char *prefix, + const char *patch_mode) { struct pathspec pathspec; + char buffer[64]; parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_FULL | @@ -234,9 +237,13 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch) PATHSPEC_PREFIX_ORIGIN, prefix, argv); - return run_add_interactive(NULL, - patch ? "--patch" : NULL, - &pathspec); + if (patch_mode) { + xsnprintf(buffer, sizeof(buffer), "--patch%s%s", + *patch_mode ? "=" : "", patch_mode); + patch_mode = buffer; + } + + return run_add_interactive(NULL, patch_mode, &pathspec); } static int edit_patch(int argc, const char **argv, const char *prefix) @@ -314,7 +321,9 @@ static struct option builtin_add_options[] = { OPT__VERBOSE(&verbose, N_("be verbose")), OPT_GROUP(""), OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")), - OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")), + { OPTION_STRING, 'p', "patch", &patch_interactive, N_("patch-mode"), + N_("select hunks interactively"), PARSE_OPT_OPTARG, NULL, + (intptr_t) "" }, OPT_BOOL('e', "edit", &edit_interactive, N_("edit current diff and apply")), OPT__FORCE(&ignored_too, N_("allow adding otherwise ignored files"), 0), OPT_BOOL('u', "update", &take_worktree_changes, N_("update tracked files")), diff --git a/builtin/commit.c b/builtin/commit.c index 1c9e8e2228..997b85624b 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -355,7 +355,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix old_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT)); setenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1); - if (interactive_add(argc, argv, prefix, patch_interactive) != 0) + if (interactive_add(argc, argv, prefix, + patch_interactive ? "" : NULL) != 0) die(_("interactive add failed")); if (old_index_env && *old_index_env) diff --git a/commit.h b/commit.h index f5295ca7f3..e9f96fefd7 100644 --- a/commit.h +++ b/commit.h @@ -295,7 +295,8 @@ int delayed_reachability_test(struct shallow_info *si, int c); void prune_shallow(unsigned options); extern struct trace_key trace_shallow; -int interactive_add(int argc, const char **argv, const char *prefix, int patch); +int interactive_add(int argc, const char **argv, const char *prefix, + const char *patch_mode); int run_add_interactive(const char *revision, const char *patch_mode, const struct pathspec *pathspec); diff --git a/git-legacy-stash.sh b/git-legacy-stash.sh index f60e9b3e87..5f01910818 100755 --- a/git-legacy-stash.sh +++ b/git-legacy-stash.sh @@ -206,7 +206,7 @@ create_stash () { # find out what the user wants GIT_INDEX_FILE="$TMP-index" \ - git add--interactive --patch=stash -- "$@" && + git add --patch=stash -- "$@" && # state of the working tree w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) || From 68438c3a5dbde1650710ca5c195fb5768323ff14 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 22:38:02 +0100 Subject: [PATCH 47/61] built-in add -p: implement the "stash" and "reset" patch modes The `git stash` and `git reset` commands support a `--patch` option, and both simply hand off to `git add -p` to perform that work. Let's teach the built-in version of `git add -p` do perform that work, too. Signed-off-by: Johannes Schindelin --- add-interactive.h | 2 ++ add-patch.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++- builtin/add.c | 4 +++ 3 files changed, 81 insertions(+), 1 deletion(-) diff --git a/add-interactive.h b/add-interactive.h index 77c47b7ad1..fc182c72eb 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -34,6 +34,8 @@ int run_add_i(struct repository *r, const struct pathspec *ps); enum add_p_mode { ADD_P_STAGE, + ADD_P_STASH, + ADD_P_RESET, }; int run_add_p(struct repository *r, enum add_p_mode mode, diff --git a/add-patch.c b/add-patch.c index 853fa90e75..781cb14163 100644 --- a/add-patch.c +++ b/add-patch.c @@ -40,6 +40,72 @@ static struct patch_mode patch_mode_stage = { "the file\n") }; +static struct patch_mode patch_mode_stash = { + .diff = { "diff-index", "HEAD", NULL }, + .apply = { "--cached", NULL }, + .apply_check = { "--cached", NULL }, + .is_reverse = 0, + .prompt_mode = { + N_("Stash mode change [y,n,q,a,d%s,?]? "), + N_("Stash deletion [y,n,q,a,d%s,?]? "), + N_("Stash this hunk [y,n,q,a,d%s,?]? "), + }, + .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk " + "will immediately be marked for stashing."), + .help_patch_text = + N_("y - stash this hunk\n" + "n - do not stash this hunk\n" + "q - quit; do not stash this hunk or any of the remaining " + "ones\n" + "a - stash this hunk and all later hunks in the file\n" + "d - do not stash this hunk or any of the later hunks in " + "the file\n"), +}; + +static struct patch_mode patch_mode_reset_head = { + .diff = { "diff-index", "--cached", NULL }, + .apply = { "-R", "--cached", NULL }, + .apply_check = { "-R", "--cached", NULL }, + .is_reverse = 1, + .prompt_mode = { + N_("Unstage mode change [y,n,q,a,d%s,?]? "), + N_("Unstage deletion [y,n,q,a,d%s,?]? "), + N_("Unstage this hunk [y,n,q,a,d%s,?]? "), + }, + .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk " + "will immediately be marked for unstaging."), + .help_patch_text = + N_("y - unstage this hunk\n" + "n - do not unstage this hunk\n" + "q - quit; do not unstage this hunk or any of the remaining " + "ones\n" + "a - unstage this hunk and all later hunks in the file\n" + "d - do not unstage this hunk or any of the later hunks in " + "the file\n"), +}; + +static struct patch_mode patch_mode_reset_nothead = { + .diff = { "diff-index", "-R", "--cached", NULL }, + .apply = { "--cached", NULL }, + .apply_check = { "--cached", NULL }, + .is_reverse = 0, + .prompt_mode = { + N_("Apply mode change to index [y,n,q,a,d%s,?]? "), + N_("Apply deletion to index [y,n,q,a,d%s,?]? "), + N_("Apply this hunk to index [y,n,q,a,d%s,?]? "), + }, + .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk " + "will immediately be marked for applying."), + .help_patch_text = + N_("y - apply this hunk to index\n" + "n - do not apply this hunk to index\n" + "q - quit; do not apply this hunk or any of the remaining " + "ones\n" + "a - apply this hunk and all later hunks in the file\n" + "d - do not apply this hunk or any of the later hunks in " + "the file\n"), +}; + struct hunk_header { unsigned long old_offset, old_count, new_offset, new_count; /* @@ -1284,7 +1350,15 @@ int run_add_p(struct repository *r, enum add_p_mode mode, if (init_add_i_state(r, &s.s)) return error("Could not read `add -i` config"); - s.mode = &patch_mode_stage; + if (mode == ADD_P_STASH) + s.mode = &patch_mode_stash; + else if (mode == ADD_P_RESET) { + if (!revision || !strcmp(revision, "HEAD")) + s.mode = &patch_mode_reset_head; + else + s.mode = &patch_mode_reset_nothead; + } else + s.mode = &patch_mode_stage; s.revision = revision; if (repo_refresh_and_write_index(r, REFRESH_QUIET, 0) < 0 || diff --git a/builtin/add.c b/builtin/add.c index fc76019743..0d81247e80 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -203,6 +203,10 @@ int run_add_interactive(const char *revision, const char *patch_mode, if (!strcmp(patch_mode, "--patch")) mode = ADD_P_STAGE; + else if (!strcmp(patch_mode, "--patch=stash")) + mode = ADD_P_STASH; + else if (!strcmp(patch_mode, "--patch=reset")) + mode = ADD_P_RESET; else die("'%s' not yet supported in the built-in add -p", patch_mode); From 82b6ed839877bd167de897a549a1cbb7dc3a7564 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 22:38:02 +0100 Subject: [PATCH 48/61] built-in add -p: implement the "checkout" patch modes This patch teaches the built-in `git add -p` machinery all the tricks it needs to know in order to act as the work horse for `git checkout -p`. Apart from the minor changes (slightly reworded messages, different `diff` and `apply --check` invocations), it requires a new function to actually apply the changes, as `git checkout -p` is a bit special in that respect: when the desired changes do not apply to the index, but apply to the work tree, Git does not fail straight away, but asks the user whether to apply the changes to the worktree at least. Signed-off-by: Johannes Schindelin --- add-interactive.h | 1 + add-patch.c | 139 ++++++++++++++++++++++++++++++++++++++++++++-- builtin/add.c | 5 +- 3 files changed, 138 insertions(+), 7 deletions(-) diff --git a/add-interactive.h b/add-interactive.h index fc182c72eb..2c04ada1a3 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -36,6 +36,7 @@ enum add_p_mode { ADD_P_STAGE, ADD_P_STASH, ADD_P_RESET, + ADD_P_CHECKOUT, }; int run_add_p(struct repository *r, enum add_p_mode mode, diff --git a/add-patch.c b/add-patch.c index 781cb14163..c944d4c415 100644 --- a/add-patch.c +++ b/add-patch.c @@ -106,6 +106,72 @@ static struct patch_mode patch_mode_reset_nothead = { "the file\n"), }; +static struct patch_mode patch_mode_checkout_index = { + .diff = { "diff-files", NULL }, + .apply = { "-R", NULL }, + .apply_check = { "-R", NULL }, + .is_reverse = 1, + .prompt_mode = { + N_("Discard mode change from worktree [y,n,q,a,d%s,?]? "), + N_("Discard deletion from worktree [y,n,q,a,d%s,?]? "), + N_("Discard this hunk from worktree [y,n,q,a,d%s,?]? "), + }, + .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk " + "will immediately be marked for discarding."), + .help_patch_text = + N_("y - discard this hunk from worktree\n" + "n - do not discard this hunk from worktree\n" + "q - quit; do not discard this hunk or any of the remaining " + "ones\n" + "a - discard this hunk and all later hunks in the file\n" + "d - do not discard this hunk or any of the later hunks in " + "the file\n"), +}; + +static struct patch_mode patch_mode_checkout_head = { + .diff = { "diff-index", NULL }, + .apply_for_checkout = 1, + .apply_check = { "-R", NULL }, + .is_reverse = 1, + .prompt_mode = { + N_("Discard mode change from index and worktree [y,n,q,a,d%s,?]? "), + N_("Discard deletion from index and worktree [y,n,q,a,d%s,?]? "), + N_("Discard this hunk from index and worktree [y,n,q,a,d%s,?]? "), + }, + .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk " + "will immediately be marked for discarding."), + .help_patch_text = + N_("y - discard this hunk from index and worktree\n" + "n - do not discard this hunk from index and worktree\n" + "q - quit; do not discard this hunk or any of the remaining " + "ones\n" + "a - discard this hunk and all later hunks in the file\n" + "d - do not discard this hunk or any of the later hunks in " + "the file\n"), +}; + +static struct patch_mode patch_mode_checkout_nothead = { + .diff = { "diff-index", "-R", NULL }, + .apply_for_checkout = 1, + .apply_check = { NULL }, + .is_reverse = 0, + .prompt_mode = { + N_("Apply mode change to index and worktree [y,n,q,a,d%s,?]? "), + N_("Apply deletion to index and worktree [y,n,q,a,d%s,?]? "), + N_("Apply this hunk to index and worktree [y,n,q,a,d%s,?]? "), + }, + .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk " + "will immediately be marked for applying."), + .help_patch_text = + N_("y - apply this hunk to index and worktree\n" + "n - do not apply this hunk to index and worktree\n" + "q - quit; do not apply this hunk or any of the remaining " + "ones\n" + "a - apply this hunk and all later hunks in the file\n" + "d - do not apply this hunk or any of the later hunks in " + "the file\n"), +}; + struct hunk_header { unsigned long old_offset, old_count, new_offset, new_count; /* @@ -1007,6 +1073,57 @@ static int edit_hunk_loop(struct add_p_state *s, } } +static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff, + int is_reverse) +{ + const char *reverse = is_reverse ? "-R" : NULL; + struct child_process check_index = CHILD_PROCESS_INIT; + struct child_process check_worktree = CHILD_PROCESS_INIT; + struct child_process apply_index = CHILD_PROCESS_INIT; + struct child_process apply_worktree = CHILD_PROCESS_INIT; + int applies_index, applies_worktree; + + setup_child_process(&check_index, s, + "apply", "--cached", "--check", reverse, NULL); + applies_index = !pipe_command(&check_index, diff->buf, diff->len, + NULL, 0, NULL, 0); + + setup_child_process(&check_worktree, s, + "apply", "--check", reverse, NULL); + applies_worktree = !pipe_command(&check_worktree, diff->buf, diff->len, + NULL, 0, NULL, 0); + + if (applies_worktree && applies_index) { + setup_child_process(&apply_index, s, + "apply", "--cached", reverse, NULL); + pipe_command(&apply_index, diff->buf, diff->len, + NULL, 0, NULL, 0); + + setup_child_process(&apply_worktree, s, + "apply", reverse, NULL); + pipe_command(&apply_worktree, diff->buf, diff->len, + NULL, 0, NULL, 0); + + return 1; + } + + if (!applies_index) { + err(s, _("The selected hunks do not apply to the index!")); + if (prompt_yesno(s, _("Apply them to the worktree " + "anyway? ")) > 0) { + setup_child_process(&apply_worktree, s, + "apply", reverse, NULL); + return pipe_command(&apply_worktree, diff->buf, + diff->len, NULL, 0, NULL, 0); + } + err(s, _("Nothing was applied.\n")); + } else + /* As a last resort, show the diff to the user */ + fwrite(diff->buf, diff->len, 1, stderr); + + return 0; +} + #define SUMMARY_HEADER_WIDTH 20 #define SUMMARY_LINE_WIDTH 80 static void summarize_hunk(struct add_p_state *s, struct hunk *hunk, @@ -1327,11 +1444,16 @@ soft_increment: strbuf_reset(&s->buf); reassemble_patch(s, file_diff, 0, &s->buf); - setup_child_process(&cp, s, "apply", NULL); - argv_array_pushv(&cp.args, s->mode->apply); - if (pipe_command(&cp, s->buf.buf, s->buf.len, - NULL, 0, NULL, 0)) - error(_("'git apply' failed")); + if (s->mode->apply_for_checkout) + apply_for_checkout(s, &s->buf, + s->mode->is_reverse); + else { + setup_child_process(&cp, s, "apply", NULL); + argv_array_pushv(&cp.args, s->mode->apply); + if (pipe_command(&cp, s->buf.buf, s->buf.len, + NULL, 0, NULL, 0)) + error(_("'git apply' failed")); + } repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0); } @@ -1357,6 +1479,13 @@ int run_add_p(struct repository *r, enum add_p_mode mode, s.mode = &patch_mode_reset_head; else s.mode = &patch_mode_reset_nothead; + } else if (mode == ADD_P_CHECKOUT) { + if (!revision) + s.mode = &patch_mode_checkout_index; + else if (!strcmp(revision, "HEAD")) + s.mode = &patch_mode_checkout_head; + else + s.mode = &patch_mode_checkout_nothead; } else s.mode = &patch_mode_stage; s.revision = revision; diff --git a/builtin/add.c b/builtin/add.c index 0d81247e80..79bc14033b 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -207,9 +207,10 @@ int run_add_interactive(const char *revision, const char *patch_mode, mode = ADD_P_STASH; else if (!strcmp(patch_mode, "--patch=reset")) mode = ADD_P_RESET; + else if (!strcmp(patch_mode, "--patch=checkout")) + mode = ADD_P_CHECKOUT; else - die("'%s' not yet supported in the built-in add -p", - patch_mode); + die("'%s' not supported", patch_mode); return !!run_add_p(the_repository, mode, revision, pathspec); } From 044ad92502c15ec54496853451b052788a84516c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 15:45:03 +0100 Subject: [PATCH 49/61] built-in add -p: support interactive.diffFilter The Perl version supports post-processing the colored diff (that is generated in addition to the uncolored diff, intended to offer a prettier user experience) by a command configured via that config setting, and now the built-in version does that, too. Signed-off-by: Johannes Schindelin --- add-interactive.c | 4 ++++ add-interactive.h | 3 +++ add-patch.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index c9336416fe..4d7428e4ea 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -56,6 +56,10 @@ int init_add_i_state(struct repository *r, struct add_i_state *s) strlcpy(s->file_new_color, diff_get_color(s->use_color, DIFF_FILE_NEW), COLOR_MAXLEN); + free(s->interactive_diff_filter); + if (git_config_get_string("interactive.difffilter", + &s->interactive_diff_filter)) + s->interactive_diff_filter = NULL; return 0; } diff --git a/add-interactive.h b/add-interactive.h index 2c04ada1a3..013c8b5b99 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -15,6 +15,8 @@ struct add_i_state { char context_color[COLOR_MAXLEN]; char file_old_color[COLOR_MAXLEN]; char file_new_color[COLOR_MAXLEN]; + + char *interactive_diff_filter; }; int init_add_i_state(struct repository *r, struct add_i_state *s); @@ -27,6 +29,7 @@ enum color_add_i { COLOR_RESET, }; const char *get_add_i_color(enum color_add_i ix); +const char *get_interactive_diff_filter(void); struct repository; struct pathspec; diff --git a/add-patch.c b/add-patch.c index c944d4c415..8b4907f329 100644 --- a/add-patch.c +++ b/add-patch.c @@ -348,6 +348,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) if (want_color_fd(1, -1)) { struct child_process colored_cp = CHILD_PROCESS_INIT; + const char *diff_filter = s->s.interactive_diff_filter; setup_child_process(&colored_cp, s, NULL); xsnprintf((char *)args.argv[color_arg_index], 8, "--color"); @@ -357,6 +358,24 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) argv_array_clear(&args); if (res) return error(_("could not parse colored diff")); + + if (diff_filter) { + struct child_process filter_cp = CHILD_PROCESS_INIT; + + setup_child_process(&filter_cp, s, + diff_filter, NULL); + filter_cp.git_cmd = 0; + filter_cp.use_shell = 1; + strbuf_reset(&s->buf); + if (pipe_command(&filter_cp, + colored->buf, colored->len, + &s->buf, colored->len, + NULL, 0) < 0) + return error(_("failed to run '%s'"), + diff_filter); + strbuf_swap(colored, &s->buf); + } + strbuf_complete_line(colored); colored_p = colored->buf; colored_pend = colored_p + colored->len; @@ -458,6 +477,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) colored_pend - colored_p); if (colored_eol) colored_p = colored_eol + 1; + else if (p != pend) + /* colored shorter than non-colored? */ + goto mismatched_output; else colored_p = colored_pend; @@ -479,6 +501,15 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) */ hunk->splittable_into++; + /* non-colored shorter than colored? */ + if (colored_p != colored_pend) { +mismatched_output: + error(_("mismatched output from interactive.diffFilter")); + advise(_("Your filter must maintain a one-to-one correspondence\n" + "between its input and output lines.")); + return -1; + } + return 0; } From 7f96a63787fc25bd2d9b66d121addeddcc8cee3a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Mar 2019 15:03:09 +0100 Subject: [PATCH 50/61] commit --interactive: make it work with the built-in `add -i` The built-in `git add -i` machinery obviously has its `the_repository` structure initialized at the point where `cmd_commit()` calls it, and therefore does not look at the environment variable `GIT_INDEX_FILE`. But it has to, because the index was already locked, and we want to ask the interactive add machinery to work on the `index.lock` file instead of the `index` file. Technically, we could teach `run_add_i()` (and `run_add_p()`) to look specifically at that environment variable, but the entire idea of passing in a parameter of type `struct repository *` is to allow working on multiple repositories (and their index files) independently. So let's instead override the `index_file` field of that structure temporarily. Signed-off-by: Johannes Schindelin --- builtin/commit.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 997b85624b..bd12a10460 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -344,7 +344,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix die(_("index file corrupt")); if (interactive) { - char *old_index_env = NULL; + char *old_index_env = NULL, *old_repo_index_file; hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR); refresh_cache_or_die(refresh_flags); @@ -352,13 +352,17 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix if (write_locked_index(&the_index, &index_lock, 0)) die(_("unable to create temporary index")); + old_repo_index_file = the_repository->index_file; + the_repository->index_file = + (char *)get_lock_file_path(&index_lock); old_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT)); - setenv(INDEX_ENVIRONMENT, get_lock_file_path(&index_lock), 1); + setenv(INDEX_ENVIRONMENT, the_repository->index_file, 1); if (interactive_add(argc, argv, prefix, patch_interactive ? "" : NULL) != 0) die(_("interactive add failed")); + the_repository->index_file = old_repo_index_file; if (old_index_env && *old_index_env) setenv(INDEX_ENVIRONMENT, old_index_env, 1); else From 3a5c5684e55c4a269b1da92b3dc8225304549df1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 15:42:52 +0100 Subject: [PATCH 51/61] built-in add -p: handle diff.algorithm The Perl version of `git add -p` reads the config setting `diff.algorithm` and if set, uses it to generate the diff using the specified algorithm. This patch ports that functionality to the C version. To make sure that this works as intended, we add a regression test case that tries to specify a bogus diff algorithm and then verifies that `git diff-files` produced the expected error message. Note: In that new test case, we actually ignore the exit code of `git add -p`. The reason is that the C version exits with failure (as one might expect), but the Perl version does not. In fact, the Perl version continues happily after the uncolored diff failed, trying to generate the colored diff, still not catching the problem, and then it pretends to have succeeded (with exit code 0). This is arguably a bug in the Perl version, and fixing it is safely outside the scope of this patch. Signed-off-by: Johannes Schindelin --- add-interactive.c | 5 +++++ add-interactive.h | 3 ++- add-patch.c | 3 +++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index 4d7428e4ea..041e990aae 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -61,6 +61,11 @@ int init_add_i_state(struct repository *r, struct add_i_state *s) &s->interactive_diff_filter)) s->interactive_diff_filter = NULL; + free(s->interactive_diff_algorithm); + if (git_config_get_string("diff.algorithm", + &s->interactive_diff_algorithm)) + s->interactive_diff_algorithm = NULL; + return 0; } diff --git a/add-interactive.h b/add-interactive.h index 013c8b5b99..25e300b4a0 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -16,7 +16,7 @@ struct add_i_state { char file_old_color[COLOR_MAXLEN]; char file_new_color[COLOR_MAXLEN]; - char *interactive_diff_filter; + char *interactive_diff_filter, *interactive_diff_algorithm; }; int init_add_i_state(struct repository *r, struct add_i_state *s); @@ -30,6 +30,7 @@ enum color_add_i { }; const char *get_add_i_color(enum color_add_i ix); const char *get_interactive_diff_filter(void); +const char *get_interactive_diff_algorithm(void); struct repository; struct pathspec; diff --git a/add-patch.c b/add-patch.c index 8b4907f329..ecef863f85 100644 --- a/add-patch.c +++ b/add-patch.c @@ -310,6 +310,7 @@ static int is_octal(const char *p, size_t len) static int parse_diff(struct add_p_state *s, const struct pathspec *ps) { struct argv_array args = ARGV_ARRAY_INIT; + const char *diff_algorithm = s->s.interactive_diff_algorithm; struct strbuf *plain = &s->plain, *colored = NULL; struct child_process cp = CHILD_PROCESS_INIT; char *p, *pend, *colored_p = NULL, *colored_pend = NULL, marker = '\0'; @@ -319,6 +320,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) int res; argv_array_pushv(&args, s->mode->diff); + if (diff_algorithm) + argv_array_pushf(&args, "--diff-algorithm=%s", diff_algorithm); if (s->revision) { struct object_id oid; argv_array_push(&args, From df6da219bb765da835599471594e40dd91050fe6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Apr 2019 15:33:03 +0200 Subject: [PATCH 52/61] built-in stash: use the built-in `git add -p` if so configured The scripted version of `git stash` called directly into the Perl script `git-add--interactive.perl`, and this was faithfully converted to C. However, we have a much better way to do this now: call `git add --patch=`, which incidentally also respects the config setting `add.interactive.useBuiltin`. Let's do this. Signed-off-by: Johannes Schindelin --- builtin/stash.c | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index 2a8e6d09b4..b51438aa27 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -992,9 +992,9 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, { int ret = 0; struct child_process cp_read_tree = CHILD_PROCESS_INIT; - struct child_process cp_add_i = CHILD_PROCESS_INIT; struct child_process cp_diff_tree = CHILD_PROCESS_INIT; struct index_state istate = { NULL }; + char *old_index_env = NULL, *old_repo_index_file; remove_path(stash_index_path.buf); @@ -1008,16 +1008,19 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, } /* Find out what the user wants. */ - cp_add_i.git_cmd = 1; - argv_array_pushl(&cp_add_i.args, "add--interactive", "--patch=stash", - "--", NULL); - add_pathspecs(&cp_add_i.args, ps); - argv_array_pushf(&cp_add_i.env_array, "GIT_INDEX_FILE=%s", - stash_index_path.buf); - if (run_command(&cp_add_i)) { - ret = -1; - goto done; - } + old_repo_index_file = the_repository->index_file; + the_repository->index_file = stash_index_path.buf; + old_index_env = xstrdup_or_null(getenv(INDEX_ENVIRONMENT)); + setenv(INDEX_ENVIRONMENT, the_repository->index_file, 1); + + ret = run_add_interactive(NULL, "--patch=stash", ps); + + the_repository->index_file = old_repo_index_file; + if (old_index_env && *old_index_env) + setenv(INDEX_ENVIRONMENT, old_index_env, 1); + else + unsetenv(INDEX_ENVIRONMENT); + FREE_AND_NULL(old_index_env); /* State of the working tree. */ if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0, From 075409e14ccfef54d598b967a3224d1febaf62b3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Apr 2019 22:17:07 +0200 Subject: [PATCH 53/61] terminal: make the code of disable_echo() reusable We are about to introduce the function `enable_non_canonical()`, which shares almost the complete code with `disable_echo()`. Let's prepare for that, by refactoring out that shared code. Signed-off-by: Johannes Schindelin --- compat/terminal.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/compat/terminal.c b/compat/terminal.c index fa13ee672d..1fb40b3a0a 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -32,7 +32,7 @@ static void restore_term(void) term_fd = -1; } -static int disable_echo(void) +static int disable_bits(tcflag_t bits) { struct termios t; @@ -43,7 +43,7 @@ static int disable_echo(void) old_term = t; sigchain_push_common(restore_term_on_signal); - t.c_lflag &= ~ECHO; + t.c_lflag &= ~bits; if (!tcsetattr(term_fd, TCSAFLUSH, &t)) return 0; @@ -53,6 +53,11 @@ error: return -1; } +static int disable_echo(void) +{ + return disable_bits(ECHO); +} + #elif defined(GIT_WINDOWS_NATIVE) #define INPUT_PATH "CONIN$" @@ -72,7 +77,7 @@ static void restore_term(void) hconin = INVALID_HANDLE_VALUE; } -static int disable_echo(void) +static int disable_bits(DWORD bits) { hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ, NULL, OPEN_EXISTING, @@ -82,7 +87,7 @@ static int disable_echo(void) GetConsoleMode(hconin, &cmode); sigchain_push_common(restore_term_on_signal); - if (!SetConsoleMode(hconin, cmode & (~ENABLE_ECHO_INPUT))) { + if (!SetConsoleMode(hconin, cmode & ~bits)) { CloseHandle(hconin); hconin = INVALID_HANDLE_VALUE; return -1; @@ -91,6 +96,12 @@ static int disable_echo(void) return 0; } +static int disable_echo(void) +{ + return disable_bits(ENABLE_ECHO_INPUT); +} + + #endif #ifndef FORCE_TEXT From b49e05199a70f256c44916a6a6b4cc8cfed30f44 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Apr 2019 22:21:20 +0200 Subject: [PATCH 54/61] terminal: accommodate Git for Windows' default terminal Git for Windows' Git Bash runs in MinTTY by default, which does not have a Win32 Console instance, but uses MSYS2 pseudo terminals instead. This is a problem, as Git for Windows does not want to use the MSYS2 emulation layer for Git itself, and therefore has no direct way to interact with that pseudo terminal. As a workaround, use the `stty` utility (which is included in Git for Windows, and which *is* an MSYS2 program, so it knows how to deal with the pseudo terminal). Note: If Git runs in a regular CMD or PowerShell window, there *is* a regular Win32 Console to work with. This is not a problem for the MSYS2 `stty`: it copes with this scenario just fine. Also note that we introduce support for more bits than would be necessary for a mere `disable_echo()` here, in preparation for the upcoming `enable_non_canonical()` function. Signed-off-by: Johannes Schindelin --- compat/terminal.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/compat/terminal.c b/compat/terminal.c index 1fb40b3a0a..16e9949da1 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -2,6 +2,8 @@ #include "compat/terminal.h" #include "sigchain.h" #include "strbuf.h" +#include "run-command.h" +#include "string-list.h" #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) @@ -64,11 +66,28 @@ static int disable_echo(void) #define OUTPUT_PATH "CONOUT$" #define FORCE_TEXT "t" +static int use_stty = 1; +static struct string_list stty_restore = STRING_LIST_INIT_DUP; static HANDLE hconin = INVALID_HANDLE_VALUE; static DWORD cmode; static void restore_term(void) { + if (use_stty) { + int i; + struct child_process cp = CHILD_PROCESS_INIT; + + if (stty_restore.nr == 0) + return; + + argv_array_push(&cp.args, "stty"); + for (i = 0; i < stty_restore.nr; i++) + argv_array_push(&cp.args, stty_restore.items[i].string); + run_command(&cp); + string_list_clear(&stty_restore, 0); + return; + } + if (hconin == INVALID_HANDLE_VALUE) return; @@ -79,6 +98,37 @@ static void restore_term(void) static int disable_bits(DWORD bits) { + if (use_stty) { + struct child_process cp = CHILD_PROCESS_INIT; + + argv_array_push(&cp.args, "stty"); + + if (bits & ENABLE_LINE_INPUT) { + string_list_append(&stty_restore, "icanon"); + argv_array_push(&cp.args, "-icanon"); + } + + if (bits & ENABLE_ECHO_INPUT) { + string_list_append(&stty_restore, "echo"); + argv_array_push(&cp.args, "-echo"); + } + + if (bits & ENABLE_PROCESSED_INPUT) { + string_list_append(&stty_restore, "-ignbrk"); + string_list_append(&stty_restore, "intr"); + string_list_append(&stty_restore, "^c"); + argv_array_push(&cp.args, "ignbrk"); + argv_array_push(&cp.args, "intr"); + argv_array_push(&cp.args, ""); + } + + if (run_command(&cp) == 0) + return 0; + + /* `stty` could not be executed; access the Console directly */ + use_stty = 0; + } + hconin = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); From 53855145e7e98ede3ecf2a74253bffd8da72cd64 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Mar 2019 21:28:10 +0100 Subject: [PATCH 55/61] terminal: add a new function to read a single keystroke Typically, input on the command-line is line-based. It is actually not really easy to get single characters (or better put: keystrokes). We provide two implementations here: - One that handles `/dev/tty` based systems as well as native Windows. The former uses the `tcsetattr()` function to put the terminal into "raw mode", which allows us to read individual keystrokes, one by one. The latter uses `stty.exe` to do the same, falling back to direct Win32 Console access. Thanks to the refactoring leading up to this commit, this is a single function, with the platform-specific details hidden away in conditionally-compiled code blocks. - A fall-back which simply punts and reads back an entire line. Note that the function writes the keystroke into an `strbuf` rather than a `char`, in preparation for reading Escape sequences (e.g. when the user hit an arrow key). This is also required for UTF-8 sequences in case the keystroke corresponds to a non-ASCII letter. Signed-off-by: Johannes Schindelin --- compat/terminal.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ compat/terminal.h | 3 +++ 2 files changed, 58 insertions(+) diff --git a/compat/terminal.c b/compat/terminal.c index 16e9949da1..1b2564042a 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -60,6 +60,11 @@ static int disable_echo(void) return disable_bits(ECHO); } +static int enable_non_canonical(void) +{ + return disable_bits(ICANON | ECHO); +} + #elif defined(GIT_WINDOWS_NATIVE) #define INPUT_PATH "CONIN$" @@ -151,6 +156,10 @@ static int disable_echo(void) return disable_bits(ENABLE_ECHO_INPUT); } +static int enable_non_canonical(void) +{ + return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT); +} #endif @@ -198,6 +207,33 @@ char *git_terminal_prompt(const char *prompt, int echo) return buf.buf; } +int read_key_without_echo(struct strbuf *buf) +{ + static int warning_displayed; + int ch; + + if (warning_displayed || enable_non_canonical() < 0) { + if (!warning_displayed) { + warning("reading single keystrokes not supported on " + "this platform; reading line instead"); + warning_displayed = 1; + } + + return strbuf_getline(buf, stdin); + } + + strbuf_reset(buf); + ch = getchar(); + if (ch == EOF) { + restore_term(); + return EOF; + } + + strbuf_addch(buf, ch); + restore_term(); + return 0; +} + #else char *git_terminal_prompt(const char *prompt, int echo) @@ -205,4 +241,23 @@ char *git_terminal_prompt(const char *prompt, int echo) return getpass(prompt); } +int read_key_without_echo(struct strbuf *buf) +{ + static int warning_displayed; + const char *res; + + if (!warning_displayed) { + warning("reading single keystrokes not supported on this " + "platform; reading line instead"); + warning_displayed = 1; + } + + res = getpass(""); + strbuf_reset(buf); + if (!res) + return EOF; + strbuf_addstr(buf, res); + return 0; +} + #endif diff --git a/compat/terminal.h b/compat/terminal.h index 97db7cd69d..a9d52b8464 100644 --- a/compat/terminal.h +++ b/compat/terminal.h @@ -3,4 +3,7 @@ char *git_terminal_prompt(const char *prompt, int echo); +/* Read a single keystroke, without echoing it to the terminal */ +int read_key_without_echo(struct strbuf *buf); + #endif /* COMPAT_TERMINAL_H */ From 04bbf2b6696001526aaff7dba0e0f20799625c32 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Mar 2019 21:37:27 +0100 Subject: [PATCH 56/61] built-in add -p: respect the `interactive.singlekey` config setting The Perl version of `git add -p` supports this config setting to allow users to input commands via single characters (as opposed to having to press the key afterwards). This is an opt-in feature because it requires Perl packages (Term::ReadKey and Term::Cap, where it tries to handle an absence of the latter package gracefully) to work. Note that at least on Ubuntu, that Perl package is not installed by default (it needs to be installed via `sudo apt-get install libterm-readkey-perl`), so this feature is probably not used a whole lot. In C, we obviously do not have these packages available, but we just introduced `read_single_keystroke()` that is similar to what Term::ReadKey provides, and we use that here. Signed-off-by: Johannes Schindelin --- add-interactive.c | 4 ++++ add-interactive.h | 2 ++ add-patch.c | 21 +++++++++++++++++---- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 041e990aae..f0beeefa79 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -66,6 +66,10 @@ int init_add_i_state(struct repository *r, struct add_i_state *s) &s->interactive_diff_algorithm)) s->interactive_diff_algorithm = NULL; + if (git_config_get_bool("interactive.singlekey", + &s->use_single_key)) + s->use_single_key = 0; + return 0; } diff --git a/add-interactive.h b/add-interactive.h index 25e300b4a0..41b9b2fa38 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -16,6 +16,7 @@ struct add_i_state { char file_old_color[COLOR_MAXLEN]; char file_new_color[COLOR_MAXLEN]; + int use_single_key; char *interactive_diff_filter, *interactive_diff_algorithm; }; @@ -31,6 +32,7 @@ enum color_add_i { const char *get_add_i_color(enum color_add_i ix); const char *get_interactive_diff_filter(void); const char *get_interactive_diff_algorithm(void); +int get_interactive_use_single_key(void); struct repository; struct pathspec; diff --git a/add-patch.c b/add-patch.c index ecef863f85..e8e7cfa48d 100644 --- a/add-patch.c +++ b/add-patch.c @@ -6,6 +6,7 @@ #include "pathspec.h" #include "color.h" #include "diff.h" +#include "compat/terminal.h" enum prompt_mode_type { PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK @@ -1047,14 +1048,27 @@ static int run_apply_check(struct add_p_state *s, return 0; } +static int read_single_character(struct add_p_state *s) +{ + if (s->s.use_single_key) { + int res = read_key_without_echo(&s->answer); + printf("%s\n", res == EOF ? "" : s->answer.buf); + return res; + } + + if (strbuf_getline(&s->answer, stdin) == EOF) + return EOF; + strbuf_trim_trailing_newline(&s->answer); + return 0; +} + static int prompt_yesno(struct add_p_state *s, const char *prompt) { for (;;) { color_fprintf(stdout, s->s.prompt_color, "%s", _(prompt)); fflush(stdout); - if (strbuf_getline(&s->answer, stdin) == EOF) + if (read_single_character(s) == EOF) return -1; - strbuf_trim_trailing_newline(&s->answer); switch (tolower(s->answer.buf[0])) { case 'n': return 0; case 'y': return 1; @@ -1290,9 +1304,8 @@ static int patch_update_file(struct add_p_state *s, _(s->mode->prompt_mode[prompt_mode_type]), s->buf.buf); fflush(stdout); - if (strbuf_getline(&s->answer, stdin) == EOF) + if (read_single_character(s) == EOF) break; - strbuf_trim_trailing_newline(&s->answer); if (!s->answer.len) continue; From 2f7f66674f2e00746bf52739112b33866f610d39 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 27 Mar 2019 17:14:02 +0100 Subject: [PATCH 57/61] built-in add -p: handle Escape sequences in interactive.singlekey mode This recapitulates part of b5cc003253c8 (add -i: ignore terminal escape sequences, 2011-05-17): add -i: ignore terminal escape sequences On the author's terminal, the up-arrow input sequence is ^[[A, and thus fat-fingering an up-arrow into 'git checkout -p' is quite dangerous: git-add--interactive.perl will ignore the ^[ and [ characters and happily treat A as "discard everything". As a band-aid fix, use Term::Cap to get all terminal capabilities. Then use the heuristic that any capability value that starts with ^[ (i.e., \e in perl) must be a key input sequence. Finally, given an input that starts with ^[, read more characters until we have read a full escape sequence, then return that to the caller. We use a timeout of 0.5 seconds on the subsequent reads to avoid getting stuck if the user actually input a lone ^[. Since none of the currently recognized keys start with ^[, the net result is that the sequence as a whole will be ignored and the help displayed. Note that we leave part for later which uses "Term::Cap to get all terminal capabilities", for several reasons: 1. it is actually not really necessary, as the timeout of 0.5 seconds should be plenty sufficient to catch Escape sequences, 2. it is cleaner to keep the change to special-case Escape sequences separate from the change that reads all terminal capabilities to speed things up, and 3. in practice, relying on the terminal capabilities is a bit overrated, as the information could be incomplete, or plain wrong. For example, in this developer's tmux sessions, the terminal capabilities claim that the "cursor up" sequence is ^[M, but the actual sequence produced by the "cursor up" key is ^[[A. Signed-off-by: Johannes Schindelin --- compat/terminal.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/compat/terminal.c b/compat/terminal.c index 1b2564042a..ee7dff079c 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -4,6 +4,7 @@ #include "strbuf.h" #include "run-command.h" #include "string-list.h" +#include "argv-array.h" #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) @@ -161,6 +162,37 @@ static int enable_non_canonical(void) return disable_bits(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT | ENABLE_PROCESSED_INPUT); } +/* + * Override `getchar()`, as the default implementation does not use + * `ReadFile()`. + * + * This poses a problem when we want to see whether the standard + * input has more characters, as the default of Git for Windows is to start the + * Bash in a MinTTY, which uses a named pipe to emulate a pty, in which case + * our `poll()` emulation calls `PeekNamedPipe()`, which seems to require + * `ReadFile()` to be called first to work properly (it only reports 0 + * available bytes, otherwise). + * + * So let's just override `getchar()` with a version backed by `ReadFile()` and + * go our merry ways from here. + */ +static int mingw_getchar(void) +{ + DWORD read = 0; + unsigned char ch; + + if (!ReadFile(GetStdHandle(STD_INPUT_HANDLE), &ch, 1, &read, NULL)) + return EOF; + + if (!read) { + error("Unexpected 0 read"); + return EOF; + } + + return ch; +} +#define getchar mingw_getchar + #endif #ifndef FORCE_TEXT @@ -228,8 +260,31 @@ int read_key_without_echo(struct strbuf *buf) restore_term(); return EOF; } - strbuf_addch(buf, ch); + + if (ch == '\033' /* ESC */) { + /* + * We are most likely looking at an Escape sequence. Let's try + * to read more bytes, waiting at most half a second, assuming + * that the sequence is complete if we did not receive any byte + * within that time. + * + * Start by replacing the Escape byte with ^[ */ + strbuf_splice(buf, buf->len - 1, 1, "^[", 2); + + for (;;) { + struct pollfd pfd = { .fd = 0, .events = POLLIN }; + + if (poll(&pfd, 1, 500) < 1) + break; + + ch = getchar(); + if (ch == EOF) + return 0; + strbuf_addch(buf, ch); + } + } + restore_term(); return 0; } From 67b44be3471754c556b78019f5e018571ae29470 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 27 Mar 2019 17:14:02 +0100 Subject: [PATCH 58/61] built-in add -p: handle Escape sequences more efficiently When `interactive.singlekey = true`, we react immediately to keystrokes, even to Escape sequences (e.g. when pressing a cursor key). The problem with Escape sequences is that we do not really know when they are done, and as a heuristic we poll standard input for half a second to make sure that we got all of it. While waiting half a second is not asking for a whole lot, it can become quite annoying over time, therefore with this patch, we read the terminal capabilities (if available) and extract known Escape sequences from there, then stop polling immediately when we detected that the user pressed a key that generated such a known sequence. This recapitulates the remaining part of b5cc003253c8 (add -i: ignore terminal escape sequences, 2011-05-17). Note: We do *not* query the terminal capabilities directly. That would either require a lot of platform-specific code, or it would require linking to a library such as ncurses. Linking to a library in the built-ins is something we try very hard to avoid (we even kicked the libcurl dependency to a non-built-in remote helper, just to shave off a tiny fraction of a second from Git's startup time). And the platform-specific code would be a maintenance nightmare. Even worse: in Git for Windows' case, we would need to query MSYS2 pseudo terminals, which `git.exe` simply cannot do (because it is intentionally *not* an MSYS2 program). To address this, we simply spawn `infocmp -L -1` and parse its output (which works even in Git for Windows, because that helper is included in the end-user facing installations). This is done only once, as in the Perl version, but it is done only when the first Escape sequence is encountered, not upon startup of `git add -i`; This saves on startup time, yet makes reacting to the first Escape sequence slightly more sluggish. But it allows us to keep the terminal-related code encapsulated in the `compat/terminal.c` file. Signed-off-by: Johannes Schindelin --- compat/terminal.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/compat/terminal.c b/compat/terminal.c index ee7dff079c..cb08d7a7fc 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -5,6 +5,7 @@ #include "run-command.h" #include "string-list.h" #include "argv-array.h" +#include "hashmap.h" #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) @@ -239,6 +240,70 @@ char *git_terminal_prompt(const char *prompt, int echo) return buf.buf; } +/* + * The `is_known_escape_sequence()` function returns 1 if the passed string + * corresponds to an Escape sequence that the terminal capabilities contains. + * + * To avoid depending on ncurses or other platform-specific libraries, we rely + * on the presence of the `infocmp` executable to do the job for us (failing + * silently if the program is not available or refused to run). + */ +struct escape_sequence_entry { + struct hashmap_entry entry; + char sequence[FLEX_ARRAY]; +}; + +static int sequence_entry_cmp(const void *hashmap_cmp_fn_data, + const struct escape_sequence_entry *e1, + const struct escape_sequence_entry *e2, + const void *keydata) +{ + return strcmp(e1->sequence, keydata ? keydata : e2->sequence); +} + +static int is_known_escape_sequence(const char *sequence) +{ + static struct hashmap sequences; + static int initialized; + + if (!initialized) { + struct child_process cp = CHILD_PROCESS_INIT; + struct strbuf buf = STRBUF_INIT; + char *p, *eol; + + hashmap_init(&sequences, (hashmap_cmp_fn)sequence_entry_cmp, + NULL, 0); + + argv_array_pushl(&cp.args, "infocmp", "-L", "-1", NULL); + if (pipe_command(&cp, NULL, 0, &buf, 0, NULL, 0)) + strbuf_setlen(&buf, 0); + + for (eol = p = buf.buf; *p; p = eol + 1) { + p = strchr(p, '='); + if (!p) + break; + p++; + eol = strchrnul(p, '\n'); + + if (starts_with(p, "\\E")) { + char *comma = memchr(p, ',', eol - p); + struct escape_sequence_entry *e; + + p[0] = '^'; + p[1] = '['; + FLEX_ALLOC_MEM(e, sequence, p, comma - p); + hashmap_entry_init(e, strhash(e->sequence)); + hashmap_add(&sequences, e); + } + if (!*eol) + break; + } + initialized = 1; + } + + return !!hashmap_get_from_hash(&sequences, strhash(sequence), sequence); +} + int read_key_without_echo(struct strbuf *buf) { static int warning_displayed; @@ -272,7 +337,12 @@ int read_key_without_echo(struct strbuf *buf) * Start by replacing the Escape byte with ^[ */ strbuf_splice(buf, buf->len - 1, 1, "^[", 2); - for (;;) { + /* + * Query the terminal capabilities once about all the Escape + * sequences it knows about, so that we can avoid waiting for + * half a second when we know that the sequence is complete. + */ + while (!is_known_escape_sequence(buf->buf)) { struct pollfd pfd = { .fd = 0, .events = POLLIN }; if (poll(&pfd, 1, 500) < 1) From bd6755ddd899ecfe192391e230e34e88f45fb579 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 6 Apr 2019 22:31:40 +0200 Subject: [PATCH 59/61] t3904: fix incorrect demonstration of a bug In 7e9e048661 (stash -p: demonstrate failure of split with mixed y/n, 2015-04-16), a regression test for a known breakage that was added to the test script `t3904-stash-patch.sh` that demonstrated that splitting a hunk and trying to stash only part of that split hunk fails (but shouldn't). As expected, it still fails, but for the wrong reason: once the bug is fixed, we would expect stderr to show nothing, yet the regression test expects stderr to show something. Let's fix that by telling that regression test case to expect nothing to be printed to stderr. While at it, also drop the obvious left-over from debugging where the regression test did not mind `git stash -p` to return a non-zero exit status. Of course, the regression test still fails, but this time for the correct reason. Signed-off-by: Johannes Schindelin --- t/t3904-stash-patch.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh index 9546b6f8a4..ab7d7aa6de 100755 --- a/t/t3904-stash-patch.sh +++ b/t/t3904-stash-patch.sh @@ -106,8 +106,8 @@ test_expect_failure 'stash -p with split hunk' ' ccc EOF printf "%s\n" s n y q | - test_might_fail git stash -p 2>error && - ! test_must_be_empty error && + git stash -p 2>error && + test_must_be_empty error && grep "added line 1" test && ! grep "added line 2" test ' From 49305fad6078e6bc746c3406aa2db65a81ede061 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 28 Mar 2019 20:06:37 +0100 Subject: [PATCH 60/61] ci: include the built-in `git add -i` in the `linux-gcc` job This job runs the test suite twice, once in regular mode, and once with a whole slew of `GIT_TEST_*` variables set. Now that the built-in version of `git add --interactive` is feature-complete, let's also throw `GIT_TEST_MULTI_PACK_INDEX` into that fray. Signed-off-by: Johannes Schindelin --- ci/run-build-and-tests.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index cdd2913440..044a142742 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -20,6 +20,7 @@ then export GIT_TEST_OE_DELTA_SIZE=5 export GIT_TEST_COMMIT_GRAPH=1 export GIT_TEST_MULTI_PACK_INDEX=1 + export GIT_TEST_ADD_I_USE_BUILTIN=1 make test fi From 75b6b22019bafb0c83e6cdac61a5255420c65f7a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 6 Apr 2019 22:46:09 +0200 Subject: [PATCH 61/61] stash -p: (partially) fix bug concerning split hunks When trying to stash part of the worktree changes by splitting a hunk and then only partially accepting the split bits and pieces, the user is presented with a rather cryptic error: error: patch failed: : error: test: patch does not apply Cannot remove worktree changes and the command would fail to stash the desired parts of the worktree changes (even if the `stash` ref was actually updated correctly). We even have a test case demonstrating that failure, carrying it for four years already. The explanation: when splitting a hunk, the changed lines are no longer separated by more than 3 lines (which is the amount of context lines Git's diffs use by default), but less than that. So when staging only part of the diff hunk for stashing, the resulting diff that we want to apply to the worktree in reverse will contain those changes to be dropped surrounded by three context lines, but since the diff is relative to HEAD rather than to the worktree, these context lines will not match. Example time. Let's assume that the file README contains these lines: We the people and the worktree added some lines so that it contains these lines instead: We are the kind people and the user tries to stash the line containing "are", then the command will internally stage this line to a temporary index file and try to revert the diff between HEAD and that index file. The diff hunk that `git stash` tries to revert will look somewhat like this: @@ -1776,3 +1776,4 We +are the people It is obvious, now, that the trailing context lines overlap with the part of the original diff hunk that the user did *not* want to stash. Keeping in mind that context lines in diffs serve the primary purpose of finding the exact location when the diff does not apply precisely (but when the exact line number in the file to be patched differs from the line number indicated in the diff), we work around this by reducing the amount of context lines: the diff was just generated. Note: this is not a *full* fix for the issue. Just as demonstrated in t3701's 'add -p works with pathological context lines' test case, there are ambiguities in the diff format. It is very rare in practice, of course, to encounter such repeated lines. The full solution for such cases would be to replace the approach of generating a diff from the stash and then applying it in reverse by emulating `git revert` (i.e. doing a 3-way merge). However, in `git stash -p` it would not apply to `HEAD` but instead to the worktree, which makes this non-trivial to implement as long as we also maintain a scripted version of `add -i`. Signed-off-by: Johannes Schindelin --- builtin/stash.c | 2 +- git-legacy-stash.sh | 2 +- t/t3904-stash-patch.sh | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index b51438aa27..4dcf3047c1 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1030,7 +1030,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps, } cp_diff_tree.git_cmd = 1; - argv_array_pushl(&cp_diff_tree.args, "diff-tree", "-p", "HEAD", + argv_array_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL); if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) { ret = -1; diff --git a/git-legacy-stash.sh b/git-legacy-stash.sh index 5f01910818..cc85114dd6 100755 --- a/git-legacy-stash.sh +++ b/git-legacy-stash.sh @@ -212,7 +212,7 @@ create_stash () { w_tree=$(GIT_INDEX_FILE="$TMP-index" git write-tree) || die "$(gettext "Cannot save the current worktree state")" - git diff-tree -p HEAD $w_tree -- >"$TMP-patch" && + git diff-tree -p -U1 HEAD $w_tree -- >"$TMP-patch" && test -s "$TMP-patch" || die "$(gettext "No changes selected")" diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh index ab7d7aa6de..accfe3845c 100755 --- a/t/t3904-stash-patch.sh +++ b/t/t3904-stash-patch.sh @@ -89,7 +89,7 @@ test_expect_success 'none of this moved HEAD' ' verify_saved_head ' -test_expect_failure 'stash -p with split hunk' ' +test_expect_success 'stash -p with split hunk' ' git reset --hard && cat >test <<-\EOF && aaa