From 16ce39568b606e3c70c57831bee81f322e8c0744 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 13 Nov 2019 12:40:57 +0000 Subject: [PATCH 01/62] Start to implement a built-in version of `git add --interactive` Unlike previous conversions to C, where we started with a built-in helper, we start this conversion by adding an interception in the `run_add_interactive()` function when the new opt-in `add.interactive.useBuiltin` config knob is turned on (or the corresponding environment variable `GIT_TEST_ADD_I_USE_BUILTIN`), and calling the new internal API function `run_add_i()` that is implemented directly in libgit.a. At this point, the built-in version of `git add -i` only states that it cannot do anything yet. In subsequent patches/patch series, the `run_add_i()` function will gain more and more functionality, until it is feature complete. The whole arc of the conversion can be found in the PRs #170-175 at https://github.com/gitgitgadget/git. The "--helper approach" can unfortunately not be used here: on Windows 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`. Which prevents us from implementing the main loop in C and still trying to hand off to the Perl script. The very real downside of the approach we have to take here is that the test suite won't pass with `GIT_TEST_ADD_I_USE_BUILTIN=true` until the conversion is complete (the `--helper` approach would have let it pass, even at each of the incremental conversion steps). Signed-off-by: Johannes Schindelin --- Documentation/config/add.txt | 5 +++++ Makefile | 1 + add-interactive.c | 7 +++++++ add-interactive.h | 8 ++++++++ builtin/add.c | 12 ++++++++++++ t/README | 4 ++++ 6 files changed, 37 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 58b92af54b..6c4a1e0ee5 100644 --- a/Makefile +++ b/Makefile @@ -823,6 +823,7 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat -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..d4686d5218 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,16 @@ 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 (!patch_mode) { + if (use_builtin_add_i < 0) + git_config_get_bool("add.interactive.usebuiltin", + &use_builtin_add_i); + if (use_builtin_add_i == 1) + return !!run_add_i(the_repository, pathspec); + } argv_array_push(&argv, "add--interactive"); if (patch_mode) @@ -319,6 +330,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 60d5b77bcc..5132ec83f8 100644 --- a/t/README +++ b/t/README @@ -397,6 +397,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 +built-in 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 d22b86d4c6f0840d752f8f0587fde930323a8e0d Mon Sep 17 00:00:00 2001 From: Daniel Ferreira Date: Tue, 16 May 2017 01:00:31 -0300 Subject: [PATCH 02/62] diff: export diffstat interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Đukić Signed-off-by: Johannes Schindelin --- diff.c | 39 ++++++++++++++++----------------------- diff.h | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 23 deletions(-) diff --git a/diff.c b/diff.c index afe4400a60..5703a9b78f 100644 --- a/diff.c +++ b/diff.c @@ -2495,22 +2495,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) @@ -3157,7 +3141,7 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o gather_dirstat(options, &dir, changed, "", 0); } -static void free_diffstat_info(struct diffstat_t *diffstat) +void free_diffstat_info(struct diffstat_t *diffstat) { int i; for (i = 0; i < diffstat->nr; i++) { @@ -6283,12 +6267,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) @@ -6621,6 +6600,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 7f8f024feb..d986ddc3b5 100644 --- a/diff.h +++ b/diff.h @@ -245,6 +245,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, @@ -334,6 +350,10 @@ 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); +void free_diffstat_info(struct diffstat_t *diffstat); + #define DIFF_SETUP_REVERSE 1 #define DIFF_SETUP_USE_SIZE_CACHE 4 From 594a49985b411193db6c2fe1b3c2ca859e156e80 Mon Sep 17 00:00:00 2001 From: Daniel Ferreira Date: Tue, 16 May 2017 01:00:32 -0300 Subject: [PATCH 03/62] built-in add -i: implement the `status` command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This implements the `status` command of `git add -i`. The data structures introduced in this commit will be extended later, as needed. 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. Signed-off-by: Daniel Ferreira Signed-off-by: Slavica Đukić Signed-off-by: Johannes Schindelin --- add-interactive.c | 251 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 250 insertions(+), 1 deletion(-) diff --git a/add-interactive.c b/add-interactive.c index 482e458dc6..aa35184d87 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -1,7 +1,256 @@ #include "cache.h" #include "add-interactive.h" +#include "diffcore.h" +#include "revision.h" +#include "refs.h" +#include "string-list.h" + +struct add_i_state { + struct repository *r; +}; + +static void init_add_i_state(struct add_i_state *s, struct repository *r) +{ + s->r = r; +} + +struct list_options { + const char *header; + void (*print_item)(int i, struct string_list_item *item, void *print_item_data); + void *print_item_data; +}; + +static void list(struct string_list *list, struct list_options *opts) +{ + int i; + + if (!list->nr) + return; + + if (opts->header) + printf("%s\n", opts->header); + + for (i = 0; i < list->nr; i++) { + opts->print_item(i, list->items + i, opts->print_item_data); + putchar('\n'); + } +} + +struct adddel { + uintmax_t add, del; + unsigned seen:1, binary:1; +}; + +struct file_item { + struct adddel index, worktree; +}; + +static void add_file_item(struct string_list *files, const char *name) +{ + struct file_item *item = xcalloc(sizeof(*item), 1); + + string_list_append(files, name)->util = item; +} + +struct pathname_entry { + struct hashmap_entry ent; + const char *name; + struct file_item *item; +}; + +static int pathname_entry_cmp(const void *unused_cmp_data, + const struct hashmap_entry *he1, + const struct hashmap_entry *he2, + const void *name) +{ + const struct pathname_entry *e1 = + container_of(he1, const struct pathname_entry, ent); + const struct pathname_entry *e2 = + container_of(he2, const struct pathname_entry, ent); + + return strcmp(e1->name, name ? (const char *)name : e2->name); +} + +struct collection_status { + enum { FROM_WORKTREE = 0, FROM_INDEX = 1 } phase; + + const char *reference; + + struct string_list *files; + 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; + struct file_item *file_item; + struct adddel *adddel; + + entry = hashmap_get_entry_from_hash(&s->file_map, hash, name, + struct pathname_entry, ent); + if (!entry) { + add_file_item(s->files, name); + + entry = xcalloc(sizeof(*entry), 1); + hashmap_entry_init(&entry->ent, hash); + entry->name = s->files->items[s->files->nr - 1].string; + entry->item = s->files->items[s->files->nr - 1].util; + hashmap_add(&s->file_map, &entry->ent); + } + + file_item = entry->item; + adddel = s->phase == FROM_INDEX ? + &file_item->index : &file_item->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; + } + free_diffstat_info(&stat); +} + +static int get_modified_files(struct repository *r, struct string_list *files, + 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 (discard_index(r->index) < 0 || + repo_read_index_preload(r, ps, 0) < 0) + return error(_("could not read index")); + + string_list_clear(files, 1); + s.files = files; + 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_entries(&s.file_map, struct pathname_entry, ent); + + /* While the diffs are ordered already, we ran *two* diffs... */ + string_list_sort(files); + + return 0; +} + +static void render_adddel(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 string_list_item *item, + void *print_file_item_data) +{ + struct file_item *c = item->util; + struct print_file_item_data *d = print_file_item_data; + + strbuf_reset(&d->index); + strbuf_reset(&d->worktree); + strbuf_reset(&d->buf); + + render_adddel(&d->worktree, &c->worktree, _("nothing")); + render_adddel(&d->index, &c->index, _("unchanged")); + strbuf_addf(&d->buf, d->modified_fmt, + d->index.buf, d->worktree.buf, item->string); + + printf(" %2d: %s", i + 1, d->buf.buf); +} + +static int run_status(struct add_i_state *s, const struct pathspec *ps, + struct string_list *files, struct list_options *opts) +{ + if (get_modified_files(s->r, files, ps) < 0) + return -1; + + list(files, 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 add_i_state s = { NULL }; + 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 string_list files = STRING_LIST_INIT_DUP; + int res = 0; + + init_add_i_state(&s, r); + strbuf_addstr(&header, " "); + strbuf_addf(&header, print_file_item_data.modified_fmt, + _("staged"), _("unstaged"), _("path")); + opts.header = header.buf; + + if (discard_index(r->index) < 0 || + repo_read_index(r) < 0 || + repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1, + NULL, NULL, NULL) < 0) + warning(_("could not refresh index")); + + res = run_status(&s, ps, &files, &opts); + + string_list_clear(&files, 1); + 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 42118ac0fe476ed65af2a545242d33267528fc5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Slavica=20=C4=90uki=C4=87?= Date: Sun, 5 May 2019 23:10:52 +0200 Subject: [PATCH 04/62] built-in add -i: color the header in the `status` command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Đukić Signed-off-by: Johannes Schindelin --- add-interactive.c | 41 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index aa35184d87..174e07ce83 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -1,5 +1,7 @@ #include "cache.h" #include "add-interactive.h" +#include "color.h" +#include "config.h" #include "diffcore.h" #include "revision.h" #include "refs.h" @@ -7,11 +9,40 @@ 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 void init_add_i_state(struct add_i_state *s, struct repository *r) { - s->r = r; + 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); } struct list_options { @@ -20,7 +51,8 @@ struct list_options { void *print_item_data; }; -static void list(struct string_list *list, struct list_options *opts) +static void list(struct add_i_state *s, struct string_list *list, + struct list_options *opts) { int i; @@ -28,7 +60,8 @@ static void list(struct string_list *list, 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 < list->nr; i++) { opts->print_item(i, list->items + i, opts->print_item_data); @@ -213,7 +246,7 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, if (get_modified_files(s->r, files, ps) < 0) return -1; - list(files, opts); + list(s, files, opts); putchar('\n'); return 0; From 8197b4ef20a99681bed0208c0657dd107a5f5e2e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 13:05:42 +0100 Subject: [PATCH 05/62] 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. In contrast to the Perl version, in the built-in interactive `add`, we will keep the `list()` function (which only displays items) and the `list_and_choose()` function (which uses `list()` to display the items, and only takes care of the "and choose" part) separate. The `list_and_choose()` function, as implemented in `git-add--interactive.perl` knows a few more tricks than the function we introduce in this patch: - There is a flag to let the user select multiple items. - In multi-select mode, the list of items is prefixed with a marker indicating what items have been selected. - Initially, for each item 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 selection. These features will be implemented in the C version later. This patch does not add any new main loop command, of course, the built-in `git add -i` still only supports the `status` command. The remaining commands to follow over the course of the next commits. To accommodate for listing the commands in columns, preparing for the commands that will be implemented over the course of the next patches/patch series, we teach the `list()` function to do precisely that. 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 | 139 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 136 insertions(+), 3 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 174e07ce83..76d9824de0 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -46,6 +46,7 @@ static void init_add_i_state(struct add_i_state *s, struct repository *r) } struct list_options { + int columns; const char *header; void (*print_item)(int i, struct string_list_item *item, void *print_item_data); void *print_item_data; @@ -54,7 +55,7 @@ struct list_options { static void list(struct add_i_state *s, struct string_list *list, struct list_options *opts) { - int i; + int i, last_lf = 0; if (!list->nr) return; @@ -65,8 +66,98 @@ static void list(struct add_i_state *s, struct string_list *list, for (i = 0; i < list->nr; i++) { opts->print_item(i, list->items + 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 add_i_state *s, struct string_list *items, + struct list_and_choose_options *opts) +{ + struct strbuf input = STRBUF_INIT; + ssize_t res = LIST_AND_CHOOSE_ERROR; + + for (;;) { + char *p; + + strbuf_reset(&input); + + list(s, items, &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)) { + char *endp; + index = strtoul(p, &endp, 10) - 1; + if (endp != p + sep) + index = -1; + } + + if (p[sep]) + p[sep++] = '\0'; + if (index < 0 || index >= items->nr) + printf(_("Huh (%s)?\n"), p); + else { + res = index; + break; + } + + p += sep; + } + + if (res != LIST_AND_CHOOSE_ERROR) + break; + } + + strbuf_release(&input); + return res; } struct adddel { @@ -252,20 +343,48 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, return 0; } +typedef int (*command_t)(struct add_i_state *s, const struct pathspec *ps, + struct string_list *files, + struct list_options *opts); + +static void print_command_item(int i, struct string_list_item *item, + void *print_command_item_data) +{ + printf(" %2d: %s", i + 1, item->string); +} + 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 { + const char *string; + command_t command; + } command_list[] = { + { "status", run_status }, + }; + struct string_list commands = STRING_LIST_INIT_NODUP; + 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 string_list files = STRING_LIST_INIT_DUP; + ssize_t i; int res = 0; + for (i = 0; i < ARRAY_SIZE(command_list); i++) + string_list_append(&commands, command_list[i].string) + ->util = command_list[i].command; + init_add_i_state(&s, r); + strbuf_addstr(&header, " "); strbuf_addf(&header, print_file_item_data.modified_fmt, _("staged"), _("unstaged"), _("path")); @@ -279,11 +398,25 @@ int run_add_i(struct repository *r, const struct pathspec *ps) res = run_status(&s, ps, &files, &opts); + for (;;) { + i = list_and_choose(&s, &commands, &main_loop_opts); + if (i == LIST_AND_CHOOSE_QUIT) { + printf(_("Bye.\n")); + res = 0; + break; + } + if (i != LIST_AND_CHOOSE_ERROR) { + command_t command = commands.items[i].util; + res = command(&s, ps, &files, &opts); + } + } + string_list_clear(&files, 1); strbuf_release(&print_file_item_data.buf); strbuf_release(&print_file_item_data.index); strbuf_release(&print_file_item_data.worktree); strbuf_release(&header); + string_list_clear(&commands, 0); return res; } From 5fe1f01ea0cf9c10cd6a274a3d28461d45418824 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 18 Oct 2019 14:08:41 +0200 Subject: [PATCH 06/62] built-in add -i: show unique prefixes of the commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. To determine the unique prefixes, as well as to look up the command in question, we use a copy of the list and sort it. While this might seem like overkill for a single command, it will make much more sense when all the commands are implemented, and when we reuse the same logic to present a list of files to edit, with convenient unique prefixes. At the start of the development of this patch series, a dedicated data structure was introduced that imitated the Trie that the Perl version implements. However, this was deemed overkill, and we now simply sort the list before determining the length of the unique prefixes by looking at each item's neighbor. As a bonus, we now use the same sorted list to perform a binary search using the user-provided prefix as search key. Original-patch-by: Slavica Đukić Helped-by: SZEDER Gábor Signed-off-by: Johannes Schindelin --- add-interactive.c | 188 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 177 insertions(+), 11 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 76d9824de0..c5d95d4796 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -45,6 +45,132 @@ static void init_add_i_state(struct add_i_state *s, struct repository *r) init_color(r, s, "header", s->header_color, GIT_COLOR_BOLD); } +/* + * A "prefix item list" is a list of items that are identified by a string, and + * a unique prefix (if any) is determined for each item. + * + * It is implemented in the form of a pair of `string_list`s, the first one + * duplicating the strings, with the `util` field pointing at a structure whose + * first field must be `size_t prefix_length`. + * + * That `prefix_length` field will be computed by `find_unique_prefixes()`; It + * will be set to zero if no valid, unique prefix could be found. + * + * The second `string_list` is called `sorted` and does _not_ duplicate the + * strings but simply reuses the first one's, with the `util` field pointing at + * the `string_item_list` of the first `string_list`. It will be populated and + * sorted by `find_unique_prefixes()`. + */ +struct prefix_item_list { + struct string_list items; + struct string_list sorted; + size_t min_length, max_length; +}; +#define PREFIX_ITEM_LIST_INIT \ + { STRING_LIST_INIT_DUP, STRING_LIST_INIT_NODUP, 1, 4 } + +static void prefix_item_list_clear(struct prefix_item_list *list) +{ + string_list_clear(&list->items, 1); + string_list_clear(&list->sorted, 0); +} + +static void extend_prefix_length(struct string_list_item *p, + const char *other_string, size_t max_length) +{ + size_t *len = p->util; + + if (!*len || memcmp(p->string, other_string, *len)) + return; + + for (;;) { + char c = p->string[*len]; + + /* + * Is `p` a strict prefix of `other`? Or have we exhausted the + * maximal length of the prefix? Or is the current character a + * multi-byte UTF-8 one? If so, there is no valid, unique + * prefix. + */ + if (!c || ++*len > max_length || !isascii(c)) { + *len = 0; + break; + } + + if (c != other_string[*len - 1]) + break; + } +} + +static void find_unique_prefixes(struct prefix_item_list *list) +{ + size_t i; + + if (list->sorted.nr == list->items.nr) + return; + + string_list_clear(&list->sorted, 0); + /* Avoid reallocating incrementally */ + list->sorted.items = xmalloc(st_mult(sizeof(*list->sorted.items), + list->items.nr)); + list->sorted.nr = list->sorted.alloc = list->items.nr; + + for (i = 0; i < list->items.nr; i++) { + list->sorted.items[i].string = list->items.items[i].string; + list->sorted.items[i].util = list->items.items + i; + } + + string_list_sort(&list->sorted); + + for (i = 0; i < list->sorted.nr; i++) { + struct string_list_item *sorted_item = list->sorted.items + i; + struct string_list_item *item = sorted_item->util; + size_t *len = item->util; + + *len = 0; + while (*len < list->min_length) { + char c = item->string[(*len)++]; + + if (!c || !isascii(c)) { + *len = 0; + break; + } + } + + if (i > 0) + extend_prefix_length(item, sorted_item[-1].string, + list->max_length); + if (i + 1 < list->sorted.nr) + extend_prefix_length(item, sorted_item[1].string, + list->max_length); + } +} + +static ssize_t find_unique(const char *string, struct prefix_item_list *list) +{ + int index = string_list_find_insert_index(&list->sorted, string, 1); + struct string_list_item *item; + + if (list->items.nr != list->sorted.nr) + BUG("prefix_item_list in inconsistent state (%"PRIuMAX + " vs %"PRIuMAX")", + (uintmax_t)list->items.nr, (uintmax_t)list->sorted.nr); + + if (index < 0) + item = list->sorted.items[-1 - index].util; + else if (index > 0 && + starts_with(list->sorted.items[index - 1].string, string)) + return -1; + else if (index + 1 < list->sorted.nr && + starts_with(list->sorted.items[index + 1].string, string)) + return -1; + else if (index < list->sorted.nr) + item = list->sorted.items[index].util; + else + return -1; + return item - list->items.items; +} + struct list_options { int columns; const char *header; @@ -95,18 +221,21 @@ 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 add_i_state *s, struct string_list *items, +static ssize_t list_and_choose(struct add_i_state *s, + struct prefix_item_list *items, struct list_and_choose_options *opts) { struct strbuf input = STRBUF_INIT; ssize_t res = LIST_AND_CHOOSE_ERROR; + find_unique_prefixes(items); + for (;;) { char *p; strbuf_reset(&input); - list(s, items, &opts->list_opts); + list(s, &items->items, &opts->list_opts); printf("%s%s", opts->prompt, "> "); fflush(stdout); @@ -142,7 +271,10 @@ static ssize_t list_and_choose(struct add_i_state *s, struct string_list *items, if (p[sep]) p[sep++] = '\0'; - if (index < 0 || index >= items->nr) + if (index < 0) + index = find_unique(p, items); + + if (index < 0 || index >= items->items.nr) printf(_("Huh (%s)?\n"), p); else { res = index; @@ -308,6 +440,23 @@ static void render_adddel(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; @@ -347,10 +496,23 @@ typedef int (*command_t)(struct add_i_state *s, const struct pathspec *ps, struct string_list *files, struct list_options *opts); +struct command_item { + size_t prefix_length; + command_t command; +}; + static void print_command_item(int i, struct string_list_item *item, void *print_command_item_data) { - printf(" %2d: %s", i + 1, item->string); + struct command_item *util = item->util; + + if (!util->prefix_length || + !is_valid_prefix(item->string, util->prefix_length)) + printf(" %2d: %s", i + 1, item->string); + else + printf(" %2d: [%.*s]%s", i + 1, + (int)util->prefix_length, item->string, + item->string + util->prefix_length); } int run_add_i(struct repository *r, const struct pathspec *ps) @@ -366,7 +528,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) } command_list[] = { { "status", run_status }, }; - struct string_list commands = STRING_LIST_INIT_NODUP; + struct prefix_item_list commands = PREFIX_ITEM_LIST_INIT; struct print_file_item_data print_file_item_data = { "%12s %12s %s", STRBUF_INIT, STRBUF_INIT, STRBUF_INIT @@ -379,9 +541,12 @@ int run_add_i(struct repository *r, const struct pathspec *ps) ssize_t i; int res = 0; - for (i = 0; i < ARRAY_SIZE(command_list); i++) - string_list_append(&commands, command_list[i].string) - ->util = command_list[i].command; + for (i = 0; i < ARRAY_SIZE(command_list); i++) { + struct command_item *util = xcalloc(sizeof(*util), 1); + util->command = command_list[i].command; + string_list_append(&commands.items, command_list[i].string) + ->util = util; + } init_add_i_state(&s, r); @@ -406,8 +571,9 @@ int run_add_i(struct repository *r, const struct pathspec *ps) break; } if (i != LIST_AND_CHOOSE_ERROR) { - command_t command = commands.items[i].util; - res = command(&s, ps, &files, &opts); + struct command_item *util = + commands.items.items[i].util; + res = util->command(&s, ps, &files, &opts); } } @@ -416,7 +582,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) strbuf_release(&print_file_item_data.index); strbuf_release(&print_file_item_data.worktree); strbuf_release(&header); - string_list_clear(&commands, 0); + prefix_item_list_clear(&commands); return res; } From 59c734e0ca2bc0277d868f7a396fc3e02b70f806 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 6 Mar 2019 00:06:57 +0100 Subject: [PATCH 07/62] 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 c5d95d4796..0a03b9017d 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 void init_add_i_state(struct add_i_state *s, struct repository *r) 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); } /* @@ -210,6 +212,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) @@ -250,6 +253,11 @@ static ssize_t list_and_choose(struct add_i_state *s, 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,"); @@ -515,12 +523,24 @@ static void print_command_item(int i, struct string_list_item *item, item->string + util->prefix_length); } +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 { const char *string; From 24e34c3196fc9d36ae42529e501abf9e8a256b0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Slavica=20=C4=90uki=C4=87?= Date: Sun, 3 Mar 2019 13:19:27 +0100 Subject: [PATCH 08/62] built-in add -i: use color in the main loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Đukić Signed-off-by: Johannes Schindelin --- add-interactive.c | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 0a03b9017d..170a5800e3 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 void init_add_i_state(struct add_i_state *s, struct repository *r) 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); } /* @@ -240,7 +246,8 @@ static ssize_t list_and_choose(struct add_i_state *s, list(s, &items->items, &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) { @@ -283,7 +290,8 @@ static ssize_t list_and_choose(struct add_i_state *s, index = find_unique(p, items); if (index < 0 || index >= items->items.nr) - printf(_("Huh (%s)?\n"), p); + color_fprintf_ln(stdout, s->error_color, + _("Huh (%s)?"), p); else { res = index; break; @@ -509,18 +517,23 @@ struct command_item { command_t command; }; +struct print_command_item_data { + const char *color, *reset; +}; + static void print_command_item(int i, struct string_list_item *item, void *print_command_item_data) { + struct print_command_item_data *d = print_command_item_data; struct command_item *util = item->util; if (!util->prefix_length || !is_valid_prefix(item->string, util->prefix_length)) printf(" %2d: %s", i + 1, item->string); else - printf(" %2d: [%.*s]%s", i + 1, - (int)util->prefix_length, item->string, - item->string + util->prefix_length); + printf(" %2d: %s%.*s%s%s", i + 1, + d->color, (int)util->prefix_length, item->string, + d->reset, item->string + util->prefix_length); } static void command_prompt_help(struct add_i_state *s) @@ -538,8 +551,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 { @@ -570,6 +584,15 @@ int run_add_i(struct repository *r, const struct pathspec *ps) init_add_i_state(&s, r); + /* + * 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; + } + strbuf_addstr(&header, " "); strbuf_addf(&header, print_file_item_data.modified_fmt, _("staged"), _("unstaged"), _("path")); From 3b6f18080c669bb89dabfc6b441ac6cc44c93cf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Slavica=20=C4=90uki=C4=87?= Date: Thu, 14 Feb 2019 11:41:46 +0100 Subject: [PATCH 09/62] built-in add -i: implement the `help` command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Đukić Signed-off-by: Johannes Schindelin --- add-interactive.c | 21 +++++++++++++++++++++ t/t3701-add-interactive.sh | 25 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index 170a5800e3..d6cb98cd40 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -508,6 +508,26 @@ 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 *unused_ps, + struct string_list *unused_files, + struct list_options *unused_opts) +{ + color_fprintf_ln(stdout, s->help_color, "status - %s", + _("show paths with changes")); + color_fprintf_ln(stdout, s->help_color, "update - %s", + _("add working tree state to the staged set of changes")); + color_fprintf_ln(stdout, s->help_color, "revert - %s", + _("revert staged set of changes back to the HEAD version")); + color_fprintf_ln(stdout, s->help_color, "patch - %s", + _("pick hunks and update selectively")); + color_fprintf_ln(stdout, s->help_color, "diff - %s", + _("view diff between HEAD and index")); + color_fprintf_ln(stdout, s->help_color, "add untracked - %s", + _("add contents of untracked files to the staged set of changes")); + + return 0; +} + typedef int (*command_t)(struct add_i_state *s, const struct pathspec *ps, struct string_list *files, struct list_options *opts); @@ -561,6 +581,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) command_t command; } command_list[] = { { "status", run_status }, + { "help", run_help }, }; struct prefix_item_list commands = PREFIX_ITEM_LIST_INIT; diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index d50e165ca8..d4f9386621 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -647,4 +647,29 @@ test_expect_success 'checkout -p works with pathological context lines' ' test_write_lines a b a b a a b a b a >expect && test_cmp expect a ' + +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 1475284c93bde351ccab14b1826fe70b016f3e0d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Nov 2019 21:11:41 +0000 Subject: [PATCH 10/62] add-interactive: make sure to release `rev.prune_data` During a review, Junio Hamano pointed out that the `rev.prune_data` was copied from another pathspec but never cleaned up. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- add-interactive.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index d6cb98cd40..de2fccb0ef 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -435,6 +435,9 @@ static int get_modified_files(struct repository *r, struct string_list *files, rev.diffopt.flags.ignore_dirty_submodules = 1; run_diff_files(&rev, 0); } + + if (ps) + clear_pathspec(&rev.prune_data); } hashmap_free_entries(&s.file_map, struct pathname_entry, ent); From b3226c868f91f5b46b13367066525e570f6507f7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Nov 2019 21:11:42 +0000 Subject: [PATCH 11/62] built-in add -i: allow filtering the modified files list In the `update` command of `git add -i`, we are primarily interested in the list of modified files that have worktree (i.e. unstaged) changes. At the same time, we need to determine _also_ the staged changes, to be able to produce the full added/deleted information. 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` phases, and then skipping files in the second phase when they have not been seen in the first phase. Seeing as we change the meaning of the `phase` field, we rename it to `mode` to reflect that the order depends on the exact invocation of the `git add -i` command. Signed-off-by: Johannes Schindelin --- add-interactive.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index de2fccb0ef..c62d63e35b 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -344,10 +344,11 @@ static int pathname_entry_cmp(const void *unused_cmp_data, } struct collection_status { - enum { FROM_WORKTREE = 0, FROM_INDEX = 1 } phase; + enum { FROM_WORKTREE = 0, FROM_INDEX = 1 } mode; const char *reference; + unsigned skip_unseen:1; struct string_list *files; struct hashmap file_map; }; @@ -375,6 +376,9 @@ static void collect_changes_cb(struct diff_queue_struct *q, entry = hashmap_get_entry_from_hash(&s->file_map, hash, name, struct pathname_entry, ent); if (!entry) { + if (s->skip_unseen) + continue; + add_file_item(s->files, name); entry = xcalloc(sizeof(*entry), 1); @@ -385,7 +389,7 @@ static void collect_changes_cb(struct diff_queue_struct *q, } file_item = entry->item; - adddel = s->phase == FROM_INDEX ? + adddel = s->mode == FROM_INDEX ? &file_item->index : &file_item->worktree; adddel->seen = 1; adddel->add = stat.files[i]->added; @@ -396,13 +400,22 @@ static void collect_changes_cb(struct diff_queue_struct *q, free_diffstat_info(&stat); } -static int get_modified_files(struct repository *r, struct string_list *files, +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 string_list *files, 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 }; + struct collection_status s = { 0 }; + int i; if (discard_index(r->index) < 0 || repo_read_index_preload(r, ps, 0) < 0) @@ -412,10 +425,16 @@ static int get_modified_files(struct repository *r, struct string_list *files, s.files = files; 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.mode = (i == 0) ? FROM_INDEX : FROM_WORKTREE; + else + s.mode = (i == 0) ? FROM_WORKTREE : FROM_INDEX; + s.skip_unseen = filter && i; + opt.def = is_initial ? empty_tree_oid_hex() : oid_to_hex(&head_oid); @@ -429,7 +448,7 @@ static int get_modified_files(struct repository *r, struct string_list *files, if (ps) copy_pathspec(&rev.prune_data, ps); - if (s.phase == FROM_INDEX) + if (s.mode == FROM_INDEX) run_diff_index(&rev, 1); else { rev.diffopt.flags.ignore_dirty_submodules = 1; @@ -502,7 +521,7 @@ static void print_file_item(int i, struct string_list_item *item, static int run_status(struct add_i_state *s, const struct pathspec *ps, struct string_list *files, struct list_options *opts) { - if (get_modified_files(s->r, files, ps) < 0) + if (get_modified_files(s->r, NO_FILTER, files, ps) < 0) return -1; list(s, files, opts); From 39894ec4133a70a20b83eb43a5b393e246df9820 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 21:36:11 +0100 Subject: [PATCH 12/62] built-in add -i: prepare for multi-selection commands The `update`, `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 | 114 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 89 insertions(+), 25 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index c62d63e35b..ea406e903b 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -72,15 +72,17 @@ static void init_add_i_state(struct add_i_state *s, struct repository *r) struct prefix_item_list { struct string_list items; struct string_list sorted; + int *selected; /* for multi-selections */ size_t min_length, max_length; }; #define PREFIX_ITEM_LIST_INIT \ - { STRING_LIST_INIT_DUP, STRING_LIST_INIT_NODUP, 1, 4 } + { STRING_LIST_INIT_DUP, STRING_LIST_INIT_NODUP, NULL, 1, 4 } static void prefix_item_list_clear(struct prefix_item_list *list) { string_list_clear(&list->items, 1); string_list_clear(&list->sorted, 0); + FREE_AND_NULL(list->selected); } static void extend_prefix_length(struct string_list_item *p, @@ -182,11 +184,12 @@ static ssize_t find_unique(const char *string, struct prefix_item_list *list) struct list_options { int columns; const char *header; - void (*print_item)(int i, struct string_list_item *item, void *print_item_data); + void (*print_item)(int i, int selected, struct string_list_item *item, + void *print_item_data); void *print_item_data; }; -static void list(struct add_i_state *s, struct string_list *list, +static void list(struct add_i_state *s, struct string_list *list, int *selected, struct list_options *opts) { int i, last_lf = 0; @@ -199,7 +202,8 @@ static void list(struct add_i_state *s, struct string_list *list, "%s", opts->header); for (i = 0; i < list->nr; i++) { - opts->print_item(i, list->items + i, opts->print_item_data); + opts->print_item(i, selected ? selected[i] : 0, list->items + i, + opts->print_item_data); if ((opts->columns) && ((i + 1) % (opts->columns))) { putchar('\t'); @@ -218,6 +222,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); }; @@ -225,7 +233,8 @@ 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. @@ -234,8 +243,19 @@ static ssize_t list_and_choose(struct add_i_state *s, struct prefix_item_list *items, 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 (!singleton) { + free(items->selected); + CALLOC_ARRAY(items->selected, items->items.nr); + } + + if (singleton && !immediate) + BUG("singleton requires immediate"); find_unique_prefixes(items); @@ -244,15 +264,16 @@ static ssize_t list_and_choose(struct add_i_state *s, strbuf_reset(&input); - list(s, &items->items, &opts->list_opts); + list(s, &items->items, items->selected, &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); @@ -268,7 +289,9 @@ static ssize_t list_and_choose(struct add_i_state *s, 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) @@ -277,30 +300,70 @@ static ssize_t list_and_choose(struct add_i_state *s, continue; } - if (isdigit(*p)) { + /* Input that begins with '-'; de-select */ + if (*p == '-') { + choose = 0; + p++; + sep--; + } + + if (sep == 1 && *p == '*') { + from = 0; + to = items->items.nr; + } else if (isdigit(*p)) { char *endp; - index = strtoul(p, &endp, 10) - 1; - if (endp != p + sep) - index = -1; + /* + * A range can be specified like 5-7 or 5-. + * + * Note: `from` is 0-based while the user input + * is 1-based, hence we have to decrement by + * one. We do not have to decrement `to` even + * if it is 0-based because it is an exclusive + * boundary. + */ + 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; + } } if (p[sep]) p[sep++] = '\0'; - if (index < 0) - index = find_unique(p, items); + if (from < 0) { + from = find_unique(p, items); + if (from >= 0) + to = from + 1; + } - if (index < 0 || index >= items->items.nr) + if (from < 0 || from >= items->items.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 > items->items.nr) + to = items->items.nr; + + for (; from < to; from++) + if (items->selected[from] != choose) { + items->selected[from] = choose; + res += choose ? +1 : -1; + } + p += sep; } - if (res != LIST_AND_CHOOSE_ERROR) + if ((immediate && res != LIST_AND_CHOOSE_ERROR) || + !strcmp(input.buf, "*")) break; } @@ -500,7 +563,7 @@ struct print_file_item_data { struct strbuf buf, index, worktree; }; -static void print_file_item(int i, struct string_list_item *item, +static void print_file_item(int i, int selected, struct string_list_item *item, void *print_file_item_data) { struct file_item *c = item->util; @@ -515,7 +578,7 @@ static void print_file_item(int i, struct string_list_item *item, strbuf_addf(&d->buf, d->modified_fmt, d->index.buf, d->worktree.buf, item->string); - 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, @@ -524,7 +587,7 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, if (get_modified_files(s->r, NO_FILTER, files, ps) < 0) return -1; - list(s, files, opts); + list(s, files, NULL, opts); putchar('\n'); return 0; @@ -563,7 +626,8 @@ struct print_command_item_data { const char *color, *reset; }; -static void print_command_item(int i, struct string_list_item *item, +static void print_command_item(int i, int selected, + struct string_list_item *item, void *print_command_item_data) { struct print_command_item_data *d = print_command_item_data; @@ -596,7 +660,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 { const char *string; From ffd25e9080d95e7f9077b740f3555261a242ad9d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 5 Mar 2019 23:58:49 +0100 Subject: [PATCH 13/62] built-in add -i: implement the `update` command After `status` and `help`, it is now time 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. - As `get_modified_files()` clears the list of files, it now has to take care of clearing the _full_ `prefix_item_list` lest the `sorted` and `selected` fields go stale and inconsistent. Signed-off-by: Johannes Schindelin --- add-interactive.c | 130 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 110 insertions(+), 20 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index ea406e903b..1e34e88069 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -6,6 +6,7 @@ #include "revision.h" #include "refs.h" #include "string-list.h" +#include "lockfile.h" struct add_i_state { struct repository *r; @@ -377,6 +378,7 @@ struct adddel { }; struct file_item { + size_t prefix_length; struct adddel index, worktree; }; @@ -471,7 +473,7 @@ enum modified_files_filter { static int get_modified_files(struct repository *r, enum modified_files_filter filter, - struct string_list *files, + struct prefix_item_list *files, const struct pathspec *ps) { struct object_id head_oid; @@ -484,8 +486,8 @@ static int get_modified_files(struct repository *r, repo_read_index_preload(r, ps, 0) < 0) return error(_("could not read index")); - string_list_clear(files, 1); - s.files = files; + prefix_item_list_clear(files); + s.files = &files->items; hashmap_init(&s.file_map, pathname_entry_cmp, NULL, 0); for (i = 0; i < 2; i++) { @@ -524,7 +526,7 @@ static int get_modified_files(struct repository *r, hashmap_free_entries(&s.file_map, struct pathname_entry, ent); /* While the diffs are ordered already, we ran *two* diffs... */ - string_list_sort(files); + string_list_sort(&files->items); return 0; } @@ -559,8 +561,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 string_list_item *item, @@ -568,34 +570,96 @@ static void print_file_item(int i, int selected, struct string_list_item *item, { struct file_item *c = item->util; 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 (c->prefix_length > 0 && + is_valid_prefix(item->string, c->prefix_length)) { + strbuf_reset(&d->name); + strbuf_addf(&d->name, "%s%.*s%s%s", d->color, + (int)c->prefix_length, item->string, d->reset, + item->string + c->prefix_length); + highlighted = d->name.buf; + } + render_adddel(&d->worktree, &c->worktree, _("nothing")); render_adddel(&d->index, &c->index, _("unchanged")); - strbuf_addf(&d->buf, d->modified_fmt, - d->index.buf, d->worktree.buf, item->string); + + strbuf_addf(&d->buf, d->modified_fmt, d->index.buf, d->worktree.buf, + highlighted ? highlighted : item->string); printf("%c%2d: %s", selected ? '*' : ' ', i + 1, d->buf.buf); } static int run_status(struct add_i_state *s, const struct pathspec *ps, - struct string_list *files, struct list_options *opts) + struct prefix_item_list *files, + struct list_and_choose_options *opts) { if (get_modified_files(s->r, NO_FILTER, files, ps) < 0) return -1; - list(s, files, NULL, opts); + list(s, &files->items, NULL, &opts->list_opts); putchar('\n'); return 0; } +static int run_update(struct add_i_state *s, const struct pathspec *ps, + struct prefix_item_list *files, + struct list_and_choose_options *opts) +{ + int res = 0, fd; + size_t count, i; + struct lock_file index_lock; + + if (get_modified_files(s->r, WORKTREE_ONLY, files, ps) < 0) + return -1; + + if (!files->items.nr) { + putchar('\n'); + return 0; + } + + opts->prompt = N_("Update"); + count = list_and_choose(s, files, opts); + if (count <= 0) { + putchar('\n'); + return 0; + } + + fd = repo_hold_locked_index(s->r, &index_lock, LOCK_REPORT_ON_ERROR); + if (fd < 0) { + putchar('\n'); + return -1; + } + + for (i = 0; i < files->items.nr; i++) { + const char *name = files->items.items[i].string; + if (files->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'); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *unused_ps, - struct string_list *unused_files, - struct list_options *unused_opts) + struct prefix_item_list *unused_files, + struct list_and_choose_options *unused_opts) { color_fprintf_ln(stdout, s->help_color, "status - %s", _("show paths with changes")); @@ -613,9 +677,29 @@ static int run_help(struct add_i_state *s, const struct pathspec *unused_ps, return 0; } +static void choose_prompt_help(struct add_i_state *s) +{ + color_fprintf_ln(stdout, s->help_color, "%s", + _("Prompt help:")); + color_fprintf_ln(stdout, s->help_color, "1 - %s", + _("select a single item")); + color_fprintf_ln(stdout, s->help_color, "3-5 - %s", + _("select a range of items")); + color_fprintf_ln(stdout, s->help_color, "2-3,6-9 - %s", + _("select multiple ranges")); + color_fprintf_ln(stdout, s->help_color, "foo - %s", + _("select item based on unique prefix")); + color_fprintf_ln(stdout, s->help_color, "-... - %s", + _("unselect specified items")); + color_fprintf_ln(stdout, s->help_color, "* - %s", + _("choose all items")); + color_fprintf_ln(stdout, s->help_color, " - %s", + _("(empty) finish selecting")); +} + typedef int (*command_t)(struct add_i_state *s, const struct pathspec *ps, - struct string_list *files, - struct list_options *opts); + struct prefix_item_list *files, + struct list_and_choose_options *opts); struct command_item { size_t prefix_length; @@ -667,18 +751,21 @@ int run_add_i(struct repository *r, const struct pathspec *ps) command_t command; } command_list[] = { { "status", run_status }, + { "update", run_update }, { "help", run_help }, }; struct prefix_item_list commands = PREFIX_ITEM_LIST_INIT; 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 string_list files = STRING_LIST_INIT_DUP; + struct prefix_item_list files = PREFIX_ITEM_LIST_INIT; ssize_t i; int res = 0; @@ -699,11 +786,13 @@ int run_add_i(struct repository *r, const struct pathspec *ps) data.color = s.prompt_color; data.reset = s.reset_color; } + 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; if (discard_index(r->index) < 0 || repo_read_index(r) < 0 || @@ -727,8 +816,9 @@ int run_add_i(struct repository *r, const struct pathspec *ps) } } - string_list_clear(&files, 1); + prefix_item_list_clear(&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 fa3f59ecd5bedf51571cc5453ab76f7a9e15aca7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 6 Mar 2019 23:06:13 +0100 Subject: [PATCH 14/62] 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 | 109 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index 1e34e88069..adab17a635 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -657,6 +657,114 @@ 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 prefix_item_list *files, + struct list_and_choose_options *opts) +{ + int res = 0, fd; + 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 }; + + if (get_modified_files(s->r, INDEX_ONLY, files, ps) < 0) + return -1; + + if (!files->items.nr) { + putchar('\n'); + return 0; + } + + opts->prompt = N_("Revert"); + count = list_and_choose(s, files, 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->items.nr; i++) + if (files->selected[i]) + paths[j++] = files->items.items[i].string; + 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, 0, 1, + NULL, NULL, NULL); + + if (!res) + printf(Q_("reverted %d path\n", + "reverted %d paths\n", count), (int)count); + +finish_revert: + putchar('\n'); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *unused_ps, struct prefix_item_list *unused_files, struct list_and_choose_options *unused_opts) @@ -752,6 +860,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) } command_list[] = { { "status", run_status }, { "update", run_update }, + { "revert", run_revert }, { "help", run_help }, }; struct prefix_item_list commands = PREFIX_ITEM_LIST_INIT; From a8430530fea829603bd0f4d12e148002668cd704 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 7 Mar 2019 00:59:24 +0100 Subject: [PATCH 15/62] 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 | 91 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index adab17a635..a719d30b0b 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -7,6 +7,7 @@ #include "refs.h" #include "string-list.h" #include "lockfile.h" +#include "dir.h" struct add_i_state { struct repository *r; @@ -563,6 +564,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 string_list_item *item, @@ -586,6 +588,12 @@ static void print_file_item(int i, int selected, struct string_list_item *item, highlighted = d->name.buf; } + if (d->only_names) { + printf("%c%2d: %s", selected ? '*' : ' ', i + 1, + highlighted ? highlighted : item->string); + return; + } + render_adddel(&d->worktree, &c->worktree, _("nothing")); render_adddel(&d->index, &c->index, _("unchanged")); @@ -765,6 +773,88 @@ finish_revert: return res; } +static int get_untracked_files(struct repository *r, + struct prefix_item_list *files, + const struct pathspec *ps) +{ + struct dir_struct dir = { 0 }; + size_t i; + struct strbuf buf = STRBUF_INIT; + + if (repo_read_index(r) < 0) + return error(_("could not read index")); + + prefix_item_list_clear(files); + setup_standard_excludes(&dir); + add_pattern_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)) { + strbuf_reset(&buf); + strbuf_add(&buf, ent->name, ent->len); + add_file_item(&files->items, buf.buf); + } + } + + strbuf_release(&buf); + return 0; +} + +static int run_add_untracked(struct add_i_state *s, const struct pathspec *ps, + struct prefix_item_list *files, + struct list_and_choose_options *opts) +{ + struct print_file_item_data *d = opts->list_opts.print_item_data; + int res = 0, fd; + size_t count, i; + struct lock_file index_lock; + + if (get_untracked_files(s->r, files, ps) < 0) + return -1; + + if (!files->items.nr) { + printf(_("No untracked files.\n")); + goto finish_add_untracked; + } + + opts->prompt = N_("Add untracked"); + d->only_names = 1; + count = list_and_choose(s, files, 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->items.nr; i++) { + const char *name = files->items.items[i].string; + if (files->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'); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *unused_ps, struct prefix_item_list *unused_files, struct list_and_choose_options *unused_opts) @@ -861,6 +951,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) { "status", run_status }, { "update", run_update }, { "revert", run_revert }, + { "add untracked", run_add_untracked }, { "help", run_help }, }; struct prefix_item_list commands = PREFIX_ITEM_LIST_INIT; From e7efc5a3bcc755f4d89e1f9ea4fc097783e827f5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 11 Mar 2019 10:07:49 +0100 Subject: [PATCH 16/62] 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 | 91 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 84 insertions(+), 7 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index a719d30b0b..cba9688bb5 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -8,6 +8,7 @@ #include "string-list.h" #include "lockfile.h" #include "dir.h" +#include "run-command.h" struct add_i_state { struct repository *r; @@ -375,7 +376,7 @@ static ssize_t list_and_choose(struct add_i_state *s, struct adddel { uintmax_t add, del; - unsigned seen:1, binary:1; + unsigned seen:1, unmerged:1, binary:1; }; struct file_item { @@ -415,6 +416,7 @@ struct collection_status { const char *reference; unsigned skip_unseen:1; + size_t unmerged_count, binary_count; struct string_list *files; struct hashmap file_map; }; @@ -437,7 +439,7 @@ static void collect_changes_cb(struct diff_queue_struct *q, int hash = strhash(name); struct pathname_entry *entry; struct file_item *file_item; - struct adddel *adddel; + struct adddel *adddel, *other_adddel; entry = hashmap_get_entry_from_hash(&s->file_map, hash, name, struct pathname_entry, ent); @@ -457,11 +459,21 @@ static void collect_changes_cb(struct diff_queue_struct *q, file_item = entry->item; adddel = s->mode == FROM_INDEX ? &file_item->index : &file_item->worktree; + other_adddel = s->mode == FROM_INDEX ? + &file_item->worktree : &file_item->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; + } } free_diffstat_info(&stat); } @@ -475,7 +487,9 @@ enum modified_files_filter { static int get_modified_files(struct repository *r, enum modified_files_filter filter, struct prefix_item_list *files, - const struct pathspec *ps) + const struct pathspec *ps, + size_t *unmerged_count, + size_t *binary_count) { struct object_id head_oid; int is_initial = !resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, @@ -525,6 +539,10 @@ static int get_modified_files(struct repository *r, clear_pathspec(&rev.prune_data); } hashmap_free_entries(&s.file_map, struct pathname_entry, ent); + 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... */ string_list_sort(&files->items); @@ -607,7 +625,7 @@ static int run_status(struct add_i_state *s, const struct pathspec *ps, struct prefix_item_list *files, struct list_and_choose_options *opts) { - if (get_modified_files(s->r, NO_FILTER, files, ps) < 0) + if (get_modified_files(s->r, NO_FILTER, files, ps, NULL, NULL) < 0) return -1; list(s, &files->items, NULL, &opts->list_opts); @@ -624,7 +642,7 @@ static int run_update(struct add_i_state *s, const struct pathspec *ps, size_t count, i; struct lock_file index_lock; - if (get_modified_files(s->r, WORKTREE_ONLY, files, ps) < 0) + if (get_modified_files(s->r, WORKTREE_ONLY, files, ps, NULL, NULL) < 0) return -1; if (!files->items.nr) { @@ -703,7 +721,7 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps, struct tree *tree; struct diff_options diffopt = { NULL }; - if (get_modified_files(s->r, INDEX_ONLY, files, ps) < 0) + if (get_modified_files(s->r, INDEX_ONLY, files, ps, NULL, NULL) < 0) return -1; if (!files->items.nr) { @@ -855,6 +873,64 @@ finish_add_untracked: return res; } +static int run_patch(struct add_i_state *s, const struct pathspec *ps, + struct prefix_item_list *files, + struct list_and_choose_options *opts) +{ + int res = 0; + ssize_t count, i, j; + size_t unmerged_count = 0, binary_count = 0; + + if (get_modified_files(s->r, WORKTREE_ONLY, files, ps, + &unmerged_count, &binary_count) < 0) + return -1; + + if (unmerged_count || binary_count) { + for (i = j = 0; i < files->items.nr; i++) { + struct file_item *item = files->items.items[i].util; + + if (item->index.binary || item->worktree.binary) { + free(item); + free(files->items.items[i].string); + } else if (item->index.unmerged || + item->worktree.unmerged) { + color_fprintf_ln(stderr, s->error_color, + _("ignoring unmerged: %s"), + files->items.items[i].string); + free(item); + free(files->items.items[i].string); + } else + files->items.items[j++] = files->items.items[i]; + } + files->items.nr = j; + } + + if (!files->items.nr) { + if (binary_count) + fprintf(stderr, _("Only binary files changed.\n")); + else + fprintf(stderr, _("No changes.\n")); + return 0; + } + + opts->prompt = N_("Patch update"); + count = list_and_choose(s, files, 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->items.nr; i++) + if (files->selected[i]) + argv_array_push(&args, + files->items.items[i].string); + res = run_command_v_opt(args.argv, 0); + argv_array_clear(&args); + } + + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *unused_ps, struct prefix_item_list *unused_files, struct list_and_choose_options *unused_opts) @@ -952,6 +1028,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) { "update", run_update }, { "revert", run_revert }, { "add untracked", run_add_untracked }, + { "patch", run_patch }, { "help", run_help }, }; struct prefix_item_list commands = PREFIX_ITEM_LIST_INIT; From d524e75723a2b2ea0b0a31553af68a46969d829f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 8 Mar 2019 15:58:27 +0100 Subject: [PATCH 17/62] 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 | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index cba9688bb5..4d7d44a917 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -931,6 +931,47 @@ 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 prefix_item_list *files, + struct list_and_choose_options *opts) +{ + int res = 0; + ssize_t count, i; + + struct object_id oid; + int is_initial = !resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &oid, + NULL); + if (get_modified_files(s->r, INDEX_ONLY, files, ps, NULL, NULL) < 0) + return -1; + + if (!files->items.nr) { + putchar('\n'); + return 0; + } + + opts->prompt = N_("Review diff"); + opts->flags = IMMEDIATE; + count = list_and_choose(s, files, 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->items.nr; i++) + if (files->selected[i]) + argv_array_push(&args, + files->items.items[i].string); + res = run_command_v_opt(args.argv, 0); + argv_array_clear(&args); + } + + putchar('\n'); + return res; +} + static int run_help(struct add_i_state *s, const struct pathspec *unused_ps, struct prefix_item_list *unused_files, struct list_and_choose_options *unused_opts) @@ -1029,6 +1070,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) { "revert", run_revert }, { "add untracked", run_add_untracked }, { "patch", run_patch }, + { "diff", run_diff }, { "help", run_help }, }; struct prefix_item_list commands = PREFIX_ITEM_LIST_INIT; From 9ea76bb2c17ebe03c7e7d3540c7c843ccdcc87f0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 18 Oct 2019 16:29:10 +0200 Subject: [PATCH 18/62] 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 | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 4d7d44a917..f395d54c08 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -1071,6 +1071,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) { "add untracked", run_add_untracked }, { "patch", run_patch }, { "diff", run_diff }, + { "quit", NULL }, { "help", run_help }, }; struct prefix_item_list commands = PREFIX_ITEM_LIST_INIT; @@ -1122,17 +1123,22 @@ int run_add_i(struct repository *r, const struct pathspec *ps) res = run_status(&s, ps, &files, &opts); for (;;) { + struct command_item *util; + i = list_and_choose(&s, &commands, &main_loop_opts); - if (i == LIST_AND_CHOOSE_QUIT) { + if (i < 0 || i >= commands.items.nr) + util = NULL; + else + util = commands.items.items[i].util; + + if (i == LIST_AND_CHOOSE_QUIT || (util && !util->command)) { printf(_("Bye.\n")); res = 0; break; } - if (i != LIST_AND_CHOOSE_ERROR) { - struct command_item *util = - commands.items.items[i].util; + + if (util) res = util->command(&s, ps, &files, &opts); - } } prefix_item_list_clear(&files); From 4cfc2f6f78c97048e1bfbdbf4ca66b4b214394fb Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 6 Dec 2019 13:08:19 +0000 Subject: [PATCH 19/62] 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 sometimes even reduces the number of context lines. 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 d4f9386621..4da99e27af 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 67b690360f8adb057ad649ce30e4b6a9ff4deb2c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 6 Dec 2019 13:08:20 +0000 Subject: [PATCH 20/62] 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 for 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. While at it, we avoid the pipes, as we do not want a SIGPIPE to break the regression test cases (which will be much more likely when we do not run everything through Perl because that is inherently slower). Signed-off-by: Johannes Schindelin --- t/t3701-add-interactive.sh | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 4da99e27af..793ce28297 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,38 @@ 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 >y && + force_color git add -p >output 2>&1 test && test_config interactive.diffFilter "sed s/^/foo:/" && - printf y | test_terminal git add -p >output 2>&1 && + printf y >y && + force_color git add -p >output 2>&1 test && test_config interactive.diffFilter "echo too-short" && - printf y | test_must_fail test_terminal git add -p + printf y >y && + test_must_fail force_color git add -p 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 2adc6de23d496ca43f95d177c54cf1c89be3e3b7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 20:36:47 +0100 Subject: [PATCH 21/62] 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 793ce28297..c90aaa25b0 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/^\(([0-9/]*) Stage .*?\).*/\1/p" actual >actual.filtered && + cat >expect <<-\EOF && + (1/1) Stage deletion [y,n,q,a,d,?]? + (1/2) Stage mode change [y,n,q,a,d,j,J,g,/,?]? + (2/2) 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 a51ebcdf2518b2dd508ad14d4caf1678a0d7a5cd Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 22:51:02 +0100 Subject: [PATCH 22/62] 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 c90aaa25b0..797610e96d 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 11f0714004dfac4eac405efc3a5cdd9d3884f297 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 19:36:22 +0100 Subject: [PATCH 23/62] 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 797610e96d..f43634102e 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -524,6 +524,16 @@ test_expect_success 'detect bogus diffFilter output' ' test_must_fail force_color git add -p 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 03230bafe95088a3edbb759b91a3d858f742f224 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 17:37:45 +0100 Subject: [PATCH 24/62] 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 52659bb74c..10fd30ae16 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -177,7 +177,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; } } @@ -224,7 +226,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; } @@ -1127,7 +1129,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 f43634102e..5db6432e33 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -530,7 +530,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 261ea7b69f6d073b938a9379cab196ba5ff27f21 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 11 Mar 2019 22:01:22 +0100 Subject: [PATCH 25/62] 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. While it is tempting to use this conversion of `git add -p` as an excuse to work on `apply_all_patches()` so that it does _not_ want to read a file from `stdin` or from a file, but accepts, say, an `strbuf` instead, we will refrain from this particular rabbit hole at this stage. Signed-off-by: Johannes Schindelin --- Makefile | 1 + add-interactive.h | 1 + add-patch.c | 267 ++++++++++++++++++++++++++++++++++++++++++++++ builtin/add.c | 15 ++- 4 files changed, 279 insertions(+), 5 deletions(-) create mode 100644 add-patch.c diff --git a/Makefile b/Makefile index 6c4a1e0ee5..0345d7408b 100644 --- a/Makefile +++ b/Makefile @@ -824,6 +824,7 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat 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..c4846947f6 --- /dev/null +++ b/add-patch.c @@ -0,0 +1,267 @@ +#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("(%"PRIuMAX"/%"PRIuMAX") ", + (uintmax_t)hunk_index + 1, (uintmax_t)s->hunk_nr); + 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); + + discard_index(s->r->index); + 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")); + if (!repo_read_index(s->r)) + repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0, + 1, NULL, NULL, NULL); + } + + 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 (discard_index(r->index) < 0 || repo_read_index(r) < 0 || + repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1, + NULL, NULL, NULL) < 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 d4686d5218..1deb59a642 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -189,12 +189,17 @@ int run_add_interactive(const char *revision, const char *patch_mode, int use_builtin_add_i = git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1); - if (!patch_mode) { - if (use_builtin_add_i < 0) - git_config_get_bool("add.interactive.usebuiltin", - &use_builtin_add_i); - if (use_builtin_add_i == 1) + if (use_builtin_add_i < 0) + git_config_get_bool("add.interactive.usebuiltin", + &use_builtin_add_i); + + 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"); From 33c14001136a0c628f304110536315d48feb7036 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 20 Mar 2019 12:10:11 +0100 Subject: [PATCH 26/62] 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 line, and is followed by a hunk whose context overlaps with the first one's and which appends a line at the end. Note that this affects only the beginning of the file: when a hunk is plit into two, the first can change the context so that the second hunk thinks it should match the beginning of the file, but it no longer does because the first hunk was already applied. The same is not true for the end of the file, as the hunks are applied in order (by line numbers): the hunk that changes the end of the file is by definition the last hunk to be applied. Signed-off-by: Johannes Schindelin --- apply.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/apply.c b/apply.c index f8a046a6a5..720a631eaa 100644 --- a/apply.c +++ b/apply.c @@ -2661,6 +2661,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 3b06d95fcf29b972aad8f852133b990ad633ecad Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 12:52:51 +0100 Subject: [PATCH 27/62] 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 f395d54c08..034c1dc02f 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -917,15 +917,18 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps, count = list_and_choose(s, files, 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->items.nr; i++) if (files->selected[i]) argv_array_push(&args, files->items.items[i].string); - 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); } return res; From c2f0fa2640de7bac8a462b2aebe26e01c85aaf8f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 15:42:10 +0100 Subject: [PATCH 28/62] 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 c4846947f6..99df599cb7 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); @@ -254,6 +297,7 @@ int run_add_p(struct repository *r, const struct pathspec *ps) NULL, NULL, NULL) < 0 || parse_diff(&s, ps) < 0) { strbuf_release(&s.plain); + strbuf_release(&s.colored); return -1; } @@ -263,5 +307,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 cb941b671156cccf0c94e0559f69c25d915b0adb Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 21 Mar 2019 09:40:20 +0100 Subject: [PATCH 29/62] 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 | 14 +---- add-interactive.h | 15 +++++ add-patch.c | 145 ++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 151 insertions(+), 23 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 034c1dc02f..29356c5aa2 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -10,16 +10,6 @@ #include "dir.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) @@ -36,7 +26,7 @@ static void init_color(struct repository *r, struct add_i_state *s, free(key); } -static void init_add_i_state(struct add_i_state *s, struct repository *r) +void init_add_i_state(struct add_i_state *s, struct repository *r) { const char *value; @@ -54,6 +44,8 @@ static void init_add_i_state(struct add_i_state *s, struct repository *r) init_color(r, s, "prompt", s->prompt_color, GIT_COLOR_BOLD_BLUE); init_color(r, s, "error", s->error_color, GIT_COLOR_BOLD_RED); init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET); + init_color(r, s, "fraginfo", s->fraginfo_color, + diff_get_color(s->use_color, DIFF_FRAGINFO)); } /* diff --git a/add-interactive.h b/add-interactive.h index 0e3d93acc9..584f304a9a 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]; +}; + +void init_add_i_state(struct add_i_state *s, struct repository *r); + 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 99df599cb7..18e3be0b66 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,43 @@ 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; + unsigned long old_offset = header->old_offset; + unsigned long new_offset = header->new_offset; + + 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; + } + + new_offset += delta; + + strbuf_addf(out, "@@ -%lu,%lu +%lu,%lu @@", + old_offset, header->old_count, + new_offset, 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 +257,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 +295,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 +322,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); @@ -274,13 +391,13 @@ soft_increment: strbuf_reset(&s->buf); reassemble_patch(s, &s->buf); - discard_index(s->r->index); + discard_index(s->s.r->index); 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")); - if (!repo_read_index(s->r)) - repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0, + if (!repo_read_index(s->s.r)) + repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0, 1, NULL, NULL, NULL); } @@ -290,7 +407,11 @@ 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 + }; + + init_add_i_state(&s.s, r); if (discard_index(r->index) < 0 || repo_read_index(r) < 0 || repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1, From 9594f2143b799ee68e0df7abdcdbecd621db033a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 16:40:46 +0100 Subject: [PATCH 30/62] 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 | 12 ++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/add-interactive.h b/add-interactive.h index 584f304a9a..0a3ee3474a 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -16,6 +16,15 @@ struct add_i_state { void init_add_i_state(struct add_i_state *s, struct repository *r); +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 18e3be0b66..37d6ef3ddf 100644 --- a/add-patch.c +++ b/add-patch.c @@ -334,9 +334,12 @@ 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("(%"PRIuMAX"/%"PRIuMAX") ", - (uintmax_t)hunk_index + 1, (uintmax_t)s->hunk_nr); - printf(_("Stage this hunk [y,n,a,d%s,?]? "), s->buf.buf); + color_fprintf(stdout, s->s.prompt_color, + "(%"PRIuMAX"/%"PRIuMAX") ", + (uintmax_t)hunk_index + 1, (uintmax_t)s->hunk_nr); + 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; @@ -378,7 +381,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 f43eff07d75813ed1ec4719e50898f04bc68df32 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 17 Mar 2019 21:10:47 +0100 Subject: [PATCH 31/62] 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 37d6ef3ddf..2a7ad6b9c0 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, ...) { @@ -370,17 +382,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 687c75aabfe4eef7515f509398fb3cca34ec5df9 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 15 Mar 2019 11:11:43 +0100 Subject: [PATCH 32/62] 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 | 93 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 54 insertions(+), 39 deletions(-) diff --git a/add-patch.c b/add-patch.c index 2a7ad6b9c0..c4364a86b6 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; @@ -265,16 +275,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; @@ -294,7 +305,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; @@ -303,27 +315,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; } @@ -344,11 +356,12 @@ 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, "(%"PRIuMAX"/%"PRIuMAX") ", - (uintmax_t)hunk_index + 1, (uintmax_t)s->hunk_nr); + (uintmax_t)hunk_index + 1, + (uintmax_t)file_diff->hunk_nr); color_fprintf(stdout, s->s.prompt_color, _("Stage this hunk [y,n,a,d%s,?]? "), s->buf.buf); @@ -363,22 +376,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; } @@ -388,7 +401,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")); @@ -408,14 +421,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); discard_index(s->s.r->index); setup_child_process(&cp, s, "apply", "--cached", NULL); @@ -436,6 +449,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; init_add_i_state(&s.s, r); @@ -448,8 +462,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 26c9ceacfaa7a279de52e5af984581efb1f5a342 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 15 Mar 2019 17:32:44 +0100 Subject: [PATCH 33/62] 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 c4364a86b6..0edc39c928 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 2b8faac63488ca6498758bf7bf323fe35623c1ad Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 15 Mar 2019 17:59:11 +0100 Subject: [PATCH 34/62] 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 0edc39c928..8b26844363 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; @@ -284,6 +321,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) { @@ -291,9 +361,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 @@ -328,7 +398,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 bd2e5b5af204249b38c6584195476f89758b6b1a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 21:33:55 +0100 Subject: [PATCH 35/62] 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 8b26844363..888040b789 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; /* @@ -393,6 +403,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; @@ -437,13 +448,20 @@ 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, "(%"PRIuMAX"/%"PRIuMAX") ", (uintmax_t)hunk_index + 1, (uintmax_t)file_diff->hunk_nr); 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 971faf7a53b8a4f19534dfd1fb85f32207ff53f8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 17 Mar 2019 21:12:41 +0100 Subject: [PATCH 36/62] 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 | 210 ++++++++++++++++++++++++++++++++++++- t/t3701-add-interactive.sh | 12 +++ 2 files changed, 220 insertions(+), 2 deletions(-) diff --git a/add-patch.c b/add-patch.c index 888040b789..dee4a24cc6 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,11 @@ 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 == ' ') + hunk->splittable_into++; + if (marker && *p != '\\') + marker = *p; + p = eol == pend ? pend : eol + 1; hunk->end = p - plain->buf; @@ -282,9 +300,30 @@ 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; + + if (offset >= sb->len) + BUG("looking for next line beyond buffer (%d >= %d)\n%s", + (int)offset, (int)sb->len, sb->buf); + + 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) { @@ -383,6 +422,161 @@ 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; + + remaining = hunk->header; + + 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 (!ch) + BUG("buffer overrun while splitting hunks"); + + /* + * Is this the first context line after a chain of +/- lines? + * Then record the start of the next split hunk. + */ + if ((marker == '-' || marker == '+') && ch == ' ') { + first = 0; + hunk[1].start = current; + if (colored) + hunk[1].colored_start = colored_current; + context_line_count = 0; + } + + /* + * Was the previous line a +/- one? Alternatively, is this the + * first line (and not a +/- one)? + * + * Then just increment the appropriate counter and continue + * with the next line. + * + * Otherwise this is the first of a chain of +/- lines. + * neither the first of a chain of context lines? + */ + if (marker != ' ' || (ch != '-' && ch != '+')) { +next_hunk_line: + /* Comment lines are attached to the previous line */ + if (ch == '\\') + ch = marker ? marker : ' '; + + /* 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" @@ -392,6 +586,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, @@ -448,6 +643,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; @@ -512,6 +709,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)); diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 5db6432e33..fe383be50e 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -442,6 +442,18 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' ! grep "^+31" actual ' +test_expect_success 'split hunk with incomplete line at end' ' + git reset --hard && + printf "missing LF" >>test && + git add test && + test_write_lines before 10 20 30 40 50 60 70 >test && + git grep --cached missing && + test_write_lines s n y q | git add -p && + test_must_fail git grep --cached missing && + git grep before && + test_must_fail git grep --cached before +' + test_expect_failure 'edit, adding lines to the first hunk' ' test_write_lines 10 11 20 30 40 50 51 60 >test && git reset && From 795bf01ab7c45d8b0bf74a0a0b54109db0180ebe Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 19 Mar 2019 20:50:14 +0100 Subject: [PATCH 37/62] 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 | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/add-patch.c b/add-patch.c index dee4a24cc6..36537f8769 100644 --- a/add-patch.c +++ b/add-patch.c @@ -403,6 +403,55 @@ 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 *merged) +{ + size_t i = *hunk_index; + struct hunk *hunk = file_diff->hunk + i; + /* `header` corresponds to the merged hunk */ + struct hunk_header *header = &merged->header, *next; + + if (hunk->use != USE_HUNK) + return 0; + + *merged = *hunk; + /* We simply skip the colored part (if any) when merging hunks */ + merged->colored_start = merged->colored_end = 0; + + for (; i + 1 < file_diff->hunk_nr; i++) { + hunk++; + next = &hunk->header; + + /* + * Stop merging hunks when: + * + * - the hunk is not selected for use, or + * - the hunk does not overlap with the already-merged hunk(s) + */ + if (hunk->use != USE_HUNK || + header->new_offset >= next->new_offset || + header->new_offset + header->new_count < next->new_offset || + merged->start >= hunk->start || + merged->end < hunk->start) + break; + + merged->end = hunk->end; + merged->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) { @@ -413,12 +462,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 merged = { 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, &merged)) + hunk = &merged; + render_hunk(s, hunk, delta, 0, out); + } } } From 8e87eaf22783065138350cda4e26ff4d38461c27 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Mon, 26 Aug 2019 14:30:21 +0200 Subject: [PATCH 38/62] strbuf: add a helper function to call the editor "on an strbuf" This helper supports the scenario where Git has a populated `strbuf` and wants to let the user edit it interactively. In `git add -p`, we will use this to allow interactive hunk editing: the diff hunks are already in memory, but we need to write them out to a file so that an editor can be launched, then read everything back once the user is done editing. Signed-off-by: Johannes Schindelin --- strbuf.c | 28 ++++++++++++++++++++++++++++ strbuf.h | 11 +++++++++++ 2 files changed, 39 insertions(+) diff --git a/strbuf.c b/strbuf.c index aa48d179a9..f19da55b07 100644 --- a/strbuf.c +++ b/strbuf.c @@ -1125,3 +1125,31 @@ int strbuf_normalize_path(struct strbuf *src) strbuf_release(&dst); return 0; } + +int strbuf_edit_interactively(struct strbuf *buffer, const char *path, + const char *const *env) +{ + char *path2 = NULL; + int fd, res = 0; + + if (!is_absolute_path(path)) + path = path2 = xstrdup(git_path("%s", path)); + + fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666); + if (fd < 0) + res = error_errno(_("could not open '%s' for writing"), path); + else if (write_in_full(fd, buffer->buf, buffer->len) < 0) { + res = error_errno(_("could not write to '%s'"), path); + close(fd); + } else if (close(fd) < 0) + res = error_errno(_("could not close '%s'"), path); + else { + strbuf_reset(buffer); + if (launch_editor(path, buffer, env) < 0) + res = error_errno(_("could not edit '%s'"), path); + unlink(path); + } + + free(path2); + return res; +} diff --git a/strbuf.h b/strbuf.h index 84cf969721..bfa66569a4 100644 --- a/strbuf.h +++ b/strbuf.h @@ -621,6 +621,17 @@ int launch_editor(const char *path, struct strbuf *buffer, int launch_sequence_editor(const char *path, struct strbuf *buffer, const char *const *env); +/* + * In contrast to `launch_editor()`, this function writes out the contents + * of the specified file first, then clears the `buffer`, then launches + * the editor and reads back in the file contents into the `buffer`. + * Finally, it deletes the temporary file. + * + * If `path` is relative, it refers to a file in the `.git` directory. + */ +int strbuf_edit_interactively(struct strbuf *buffer, const char *path, + const char *const *env); + void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, From 40a14aaac5e82c2dc1adf6943d7ca8382a089d15 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 19 Mar 2019 22:27:51 +0100 Subject: [PATCH 39/62] 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 | 6 + add-interactive.h | 3 + add-patch.c | 336 +++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 328 insertions(+), 17 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 29356c5aa2..6a5048c83e 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -46,6 +46,12 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) init_color(r, s, "reset", s->reset_color, GIT_COLOR_RESET); init_color(r, s, "fraginfo", s->fraginfo_color, diff_get_color(s->use_color, DIFF_FRAGINFO)); + init_color(r, s, "context", s->context_color, + diff_get_color(s->use_color, DIFF_CONTEXT)); + init_color(r, s, "old", s->file_old_color, + diff_get_color(s->use_color, DIFF_FILE_OLD)); + init_color(r, s, "new", s->file_new_color, + diff_get_color(s->use_color, DIFF_FILE_NEW)); } /* diff --git a/add-interactive.h b/add-interactive.h index 0a3ee3474a..0f87fee209 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]; }; void init_add_i_state(struct add_i_state *s, struct repository *r); diff --git a/add-patch.c b/add-patch.c index 36537f8769..fe18e67cdf 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; }; @@ -405,14 +406,14 @@ 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 *merged) + size_t *hunk_index, int use_all, struct hunk *merged) { - size_t i = *hunk_index; + size_t i = *hunk_index, delta; struct hunk *hunk = file_diff->hunk + i; /* `header` corresponds to the merged hunk */ struct hunk_header *header = &merged->header, *next; - if (hunk->use != USE_HUNK) + if (!use_all && hunk->use != USE_HUNK) return 0; *merged = *hunk; @@ -429,20 +430,99 @@ static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff, * - the hunk is not selected for use, or * - the hunk does not overlap with the already-merged hunk(s) */ - if (hunk->use != USE_HUNK || - header->new_offset >= next->new_offset || - header->new_offset + header->new_count < next->new_offset || - merged->start >= hunk->start || - merged->end < hunk->start) + if ((!use_all && hunk->use != USE_HUNK) || + header->new_offset >= next->new_offset + merged->delta || + header->new_offset + header->new_count + < next->new_offset + merged->delta) break; - merged->end = hunk->end; - merged->colored_end = hunk->colored_end; + /* + * If the hunks were not edited, and overlap, we can simply + * extend the line range. + */ + if (merged->start < hunk->start && merged->end > hunk->start) { + merged->end = hunk->end; + merged->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 - merged->delta + - next->new_offset; + size_t overlap_end = hunk->start; + size_t overlap_start = overlap_end; + size_t overlap_next, len, j; + + /* + * One of the hunks was edited: the modified hunk was + * appended to the strbuf `s->plain`. + * + * 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 (j = 0; j < overlapping_line_count; j++) { + 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)(j + 1), + (int)(hunk->end + - hunk->start), + plain + hunk->start); + + overlap_start = overlap_end; + overlap_end = overlap_next; + } + len = overlap_end - overlap_start; + + if (len > merged->end - merged->start || + memcmp(plain + merged->end - len, + plain + overlap_start, len)) + return error(_("hunks do not overlap:\n%.*s\n" + "\tdoes not end with:\n%.*s"), + (int)(merged->end - merged->start), + plain + merged->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 (merged->end != s->plain.len) { + size_t start = s->plain.len; + + strbuf_add(&s->plain, plain + merged->start, + merged->end - merged->start); + plain = s->plain.buf; + merged->start = start; + merged->end = s->plain.len; + } + + strbuf_add(&s->plain, + plain + overlap_end, + hunk->end - overlap_end); + merged->end = s->plain.len; + merged->splittable_into += hunk->splittable_into; + delta = merged->delta; + merged->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) @@ -453,10 +533,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); @@ -465,15 +546,24 @@ static void reassemble_patch(struct add_p_state *s, struct hunk merged = { 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, &merged)) + if (merge_hunks(s, file_diff, &i, use_all, &merged)) hunk = &merged; 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; } } } @@ -633,6 +723,207 @@ 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) +{ + size_t i; + + strbuf_reset(&s->buf); + strbuf_commented_addf(&s->buf, _("Manual hunk edit mode -- see bottom for " + "a quick guide.\n")); + render_hunk(s, hunk, 0, 0, &s->buf); + strbuf_commented_addf(&s->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(&s->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(&s->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 (strbuf_edit_interactively(&s->buf, "addp-hunk-edit.diff", NULL) < 0) + return -1; + + /* strip out commented lines */ + hunk->start = s->plain.len; + for (i = 0; i < s->buf.len; ) { + const char *bol = s->buf.buf + i; + size_t rest = s->buf.len - i; + const char *eol = memchr(bol, '\n', rest); + size_t len = eol ? eol + 1 - bol : rest; + + if (*bol != comment_line_char) + strbuf_add(&s->plain, bol, len); + i += len; + } + + hunk->end = s->plain.len; + if (hunk->end == hunk->start) + /* The user aborted editing by deleting everything */ + return 0; + + recolor_hunk(s, hunk); + + /* + * If the hunk header is intact, parse it, otherwise simply use the + * hunk header prior to editing (which will adjust `hunk->start` to + * skip the hunk header). + */ + if (s->plain.buf[hunk->start] == '@' && + parse_hunk_header(s, hunk) < 0) + return error(_("could not parse hunk header")); + + return 1; +} + +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; + + backup = *hunk; + + for (;;) { + int res = edit_hunk_manually(s, hunk); + if (res == 0) { + /* abandonded */ + *hunk = 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); + *hunk = 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" @@ -643,6 +934,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, @@ -701,6 +993,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; @@ -774,6 +1069,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)); @@ -787,7 +1089,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); discard_index(s->s.r->index); setup_child_process(&cp, s, "apply", "--cached", NULL); From 1760696fac983a62567fe8bc92dfe54b106fe028 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 21 Mar 2019 23:50:53 +0100 Subject: [PATCH 40/62] 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 fe18e67cdf..5ad97c3d9a 100644 --- a/add-patch.c +++ b/add-patch.c @@ -924,6 +924,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" @@ -933,6 +981,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"); @@ -991,6 +1040,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 && @@ -1060,6 +1111,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 fe383be50e..57c656a20c 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 && + (2/2) 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 + (1/2) 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 8cf62b3a2dbc4f30f9798a82cb407a20cff11ad4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 21 Mar 2019 23:50:53 +0100 Subject: [PATCH 41/62] 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 5ad97c3d9a..b304939778 100644 --- a/add-patch.c +++ b/add-patch.c @@ -982,6 +982,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"); @@ -1041,7 +1042,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 && @@ -1146,6 +1147,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 57c656a20c..12ee321707 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 && + (2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? @@ -1,2 +1,3 @@ + _10 + +15 + _20 + (1/2) 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 4cab5033697b097fb1fa410331664b3d8fe6c907 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 22 Mar 2019 01:24:47 +0100 Subject: [PATCH 42/62] 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 b304939778..3ba0ef87d8 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 { @@ -975,6 +975,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" @@ -995,7 +996,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) @@ -1086,12 +1087,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--; @@ -1236,7 +1241,7 @@ soft_increment: } putchar('\n'); - return 0; + return quit; } int run_add_p(struct repository *r, const struct pathspec *ps) From 0cb68f7396de99e1bb3f1ed483c87b52292b0f2b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 22:33:49 +0100 Subject: [PATCH 43/62] 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 | 88 +++++++++++++++++++++++++++++++++-------------- builtin/add.c | 12 +++++-- 4 files changed, 81 insertions(+), 29 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 6a5048c83e..0e753d2acc 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -924,7 +924,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 0f87fee209..05d7505a6c 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 31b8314090..a012aabec2 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); @@ -352,7 +388,10 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk, - header->colored_extra_start; } - new_offset += delta; + if (s->mode->is_reverse) + old_offset -= delta; + else + new_offset += delta; strbuf_addf(out, "@@ -%lu,%lu +%lu,%lu @@", old_offset, header->old_count, @@ -771,11 +810,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(&s->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(&s->buf, "%s", _(s->mode->edit_hunk_hint)); /* * TRANSLATORS: 'it' refers to the patch mentioned in the previous * messages. @@ -859,7 +897,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")); @@ -974,13 +1013,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" @@ -1066,7 +1098,8 @@ static int patch_update_file(struct add_p_state *s, (uintmax_t)hunk_index + 1, (uintmax_t)file_diff->hunk_nr); 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; @@ -1223,7 +1256,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 @@ -1257,10 +1290,11 @@ soft_increment: reassemble_patch(s, file_diff, 0, &s->buf); discard_index(s->s.r->index); - 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")); if (!repo_read_index(s->s.r)) repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0, 1, NULL, NULL, NULL); @@ -1270,7 +1304,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 @@ -1279,6 +1314,9 @@ int run_add_p(struct repository *r, const struct pathspec *ps) init_add_i_state(&s.s, r); + s.mode = &patch_mode_stage; + s.revision = revision; + if (discard_index(r->index) < 0 || repo_read_index(r) < 0 || repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1, NULL, NULL, NULL) < 0 || diff --git a/builtin/add.c b/builtin/add.c index 1deb59a642..bdeb1b2a55 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) { @@ -194,12 +196,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 297df572a489cf60750006ea342dfadd1783c91e Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 23:26:58 +0100 Subject: [PATCH 44/62] 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 3ba0ef87d8..3868e99847 100644 --- a/add-patch.c +++ b/add-patch.c @@ -977,8 +977,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" @@ -1215,9 +1217,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 f202bd788548106f91d9802d3eff50aa1d6ec0c6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 22:38:02 +0100 Subject: [PATCH 45/62] 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 | 85 ++++++++++++++++++++++++++++++++++++++++++++--- builtin/add.c | 4 +++ 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/add-interactive.h b/add-interactive.h index 05d7505a6c..d03ed8a60c 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 a012aabec2..c254837a4d 100644 --- a/add-patch.c +++ b/add-patch.c @@ -13,7 +13,7 @@ enum prompt_mode_type { struct patch_mode { const char *diff[4], *apply[4], *apply_check[4]; - unsigned is_reverse:1, apply_for_checkout:1; + unsigned is_reverse:1, index_only:1, apply_for_checkout:1; const char *prompt_mode[PROMPT_HUNK + 1]; const char *edit_hunk_hint, *help_patch_text; }; @@ -40,6 +40,74 @@ 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, + .index_only = 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, + .index_only = 1, + .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; /* @@ -1314,12 +1382,21 @@ int run_add_p(struct repository *r, enum add_p_mode mode, init_add_i_state(&s.s, r); - 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 (discard_index(r->index) < 0 || repo_read_index(r) < 0 || - repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1, - NULL, NULL, NULL) < 0 || + (!s.mode->index_only && + repo_refresh_and_write_index(r, REFRESH_QUIET, 0, 1, + NULL, NULL, NULL) < 0) || parse_diff(&s, ps) < 0) { strbuf_release(&s.plain); strbuf_release(&s.colored); diff --git a/builtin/add.c b/builtin/add.c index bdeb1b2a55..5a92ad5393 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 621ae45ee8a7bf69cbc91a7ddb96ae8e552622cc Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 22:54:01 +0100 Subject: [PATCH 46/62] 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 3868e99847..31b8314090 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", @@ -1273,7 +1275,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; init_add_i_state(&s.s, r); @@ -1287,9 +1289,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 97449e90b78529b09c087053956badd0df71f68d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 24 Mar 2019 19:55:08 +0100 Subject: [PATCH 47/62] legacy stash -p: respect the add.interactive.usebuiltin setting As `git add` traditionally did not expose the `--patch=` modes via command-line options, the scripted version of `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 the scripted version of `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 5a92ad5393..df6a67ffe7 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; @@ -229,9 +230,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 | @@ -239,9 +242,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) @@ -319,7 +326,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 e588bc6ad3..ae2e1bb124 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -358,7 +358,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 07ad4a5459..5d9bcf77e0 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 7f02a79666d45e28933b9e4664dd641f6d0433d3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Apr 2019 15:33:03 +0200 Subject: [PATCH 48/62] 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 4e806176b0..2dafd97766 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -999,9 +999,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); @@ -1015,16 +1015,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 01b996b80809f3d8b224c29705be54a49d5a7af3 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 12 Mar 2019 15:45:03 +0100 Subject: [PATCH 49/62] 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 | 12 ++++++++++++ add-interactive.h | 4 ++++ add-patch.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/add-interactive.c b/add-interactive.c index 0e753d2acc..00c3bc9a1b 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -52,6 +52,17 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) diff_get_color(s->use_color, DIFF_FILE_OLD)); init_color(r, s, "new", s->file_new_color, diff_get_color(s->use_color, DIFF_FILE_NEW)); + + FREE_AND_NULL(s->interactive_diff_filter); + git_config_get_string("interactive.difffilter", + &s->interactive_diff_filter); +} + +void clear_add_i_state(struct add_i_state *s) +{ + FREE_AND_NULL(s->interactive_diff_filter); + memset(s, 0, sizeof(*s)); + s->use_color = -1; } /* @@ -1149,6 +1160,7 @@ int run_add_i(struct repository *r, const struct pathspec *ps) strbuf_release(&print_file_item_data.worktree); strbuf_release(&header); prefix_item_list_clear(&commands); + clear_add_i_state(&s); return res; } diff --git a/add-interactive.h b/add-interactive.h index 6852cc7804..55a5961755 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -15,9 +15,12 @@ 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; }; void init_add_i_state(struct add_i_state *s, struct repository *r); +void clear_add_i_state(struct add_i_state *s); enum color_add_i { COLOR_HEADER = 0, @@ -27,6 +30,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 fae2b3478e..9b256fec80 100644 --- a/add-patch.c +++ b/add-patch.c @@ -394,6 +394,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"); @@ -403,6 +404,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; @@ -503,6 +522,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; @@ -524,6 +546,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; } @@ -1580,6 +1611,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode, parse_diff(&s, ps) < 0) { strbuf_release(&s.plain); strbuf_release(&s.colored); + clear_add_i_state(&s.s); return -1; } @@ -1598,5 +1630,6 @@ int run_add_p(struct repository *r, enum add_p_mode mode, strbuf_release(&s.buf); strbuf_release(&s.plain); strbuf_release(&s.colored); + clear_add_i_state(&s.s); return 0; } From d860e0546d280fe02325f9113b2ca261296c4322 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 15:42:52 +0100 Subject: [PATCH 50/62] 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 00c3bc9a1b..77762d75d6 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -56,11 +56,16 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) FREE_AND_NULL(s->interactive_diff_filter); git_config_get_string("interactive.difffilter", &s->interactive_diff_filter); + + FREE_AND_NULL(s->interactive_diff_algorithm); + git_config_get_string("diff.algorithm", + &s->interactive_diff_algorithm); } void clear_add_i_state(struct add_i_state *s) { FREE_AND_NULL(s->interactive_diff_filter); + FREE_AND_NULL(s->interactive_diff_algorithm); memset(s, 0, sizeof(*s)); s->use_color = -1; } diff --git a/add-interactive.h b/add-interactive.h index 55a5961755..1de0251c1e 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; }; void init_add_i_state(struct add_i_state *s, struct repository *r); @@ -31,6 +31,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 9b256fec80..816d63d99b 100644 --- a/add-patch.c +++ b/add-patch.c @@ -356,6 +356,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'; @@ -365,6 +366,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 dc3593a9d0d9523e94a2ee568196faa4f5957cb6 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Apr 2019 22:17:07 +0200 Subject: [PATCH 51/62] 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 8dedc0c274ac911add8c337c90cc9e0b8c803a03 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 4 Apr 2019 22:21:20 +0200 Subject: [PATCH 52/62] 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 e6bd6ce7b0d980cdf70f8726d0d9be52d74e63f0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Mar 2019 21:28:10 +0100 Subject: [PATCH 53/62] 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 df62559fad7093b0de0e74580245e3a36efec4c1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 26 Mar 2019 21:37:27 +0100 Subject: [PATCH 54/62] 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 | 2 ++ add-interactive.h | 2 ++ add-patch.c | 21 +++++++++++++++++---- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 77762d75d6..01a2f92f0c 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -60,6 +60,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r) FREE_AND_NULL(s->interactive_diff_algorithm); git_config_get_string("diff.algorithm", &s->interactive_diff_algorithm); + + git_config_get_bool("interactive.singlekey", &s->use_single_key); } void clear_add_i_state(struct add_i_state *s) diff --git a/add-interactive.h b/add-interactive.h index 1de0251c1e..c269214da2 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; }; @@ -32,6 +33,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 816d63d99b..2a6b28c737 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 @@ -1117,14 +1118,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; @@ -1364,9 +1378,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 9205e053bd2c9454a66a08e37e7b8c279ab0bee4 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 22:38:02 +0100 Subject: [PATCH 55/62] 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 d03ed8a60c..c6b3d202e8 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 c254837a4d..6b3d587deb 100644 --- a/add-patch.c +++ b/add-patch.c @@ -108,6 +108,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; /* @@ -1033,6 +1099,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, @@ -1358,11 +1475,16 @@ soft_increment: reassemble_patch(s, file_diff, 0, &s->buf); discard_index(s->s.r->index); - 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")); + } if (!repo_read_index(s->s.r)) repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0, 1, NULL, NULL, NULL); @@ -1389,6 +1511,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 df6a67ffe7..10f9e61062 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -208,9 +208,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 1fd02e76d11200ce4eb975a97310b8592d4224b5 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 27 Mar 2019 17:14:02 +0100 Subject: [PATCH 56/62] 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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/compat/terminal.c b/compat/terminal.c index 1b2564042a..b7f58d1781 100644 --- a/compat/terminal.c +++ b/compat/terminal.c @@ -161,6 +161,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 +259,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 df0823d75d1a50ddc12aca80fb8b383537999196 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 23 Mar 2019 22:38:02 +0100 Subject: [PATCH 57/62] built-in add -p: implement the "worktree" patch modes This is a straight-forward port of 2f0896ec3ad4 (restore: support --patch, 2019-04-25) which added support for `git restore -p`. Signed-off-by: Johannes Schindelin --- add-interactive.h | 1 + add-patch.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ builtin/add.c | 2 ++ 3 files changed, 54 insertions(+) diff --git a/add-interactive.h b/add-interactive.h index c6b3d202e8..6852cc7804 100644 --- a/add-interactive.h +++ b/add-interactive.h @@ -37,6 +37,7 @@ enum add_p_mode { ADD_P_STASH, ADD_P_RESET, ADD_P_CHECKOUT, + ADD_P_WORKTREE, }; int run_add_p(struct repository *r, enum add_p_mode mode, diff --git a/add-patch.c b/add-patch.c index 6b3d587deb..fae2b3478e 100644 --- a/add-patch.c +++ b/add-patch.c @@ -174,6 +174,50 @@ static struct patch_mode patch_mode_checkout_nothead = { "the file\n"), }; +static struct patch_mode patch_mode_worktree_head = { + .diff = { "diff-index", NULL }, + .apply = { "-R", NULL }, + .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 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_worktree_nothead = { + .diff = { "diff-index", "-R", NULL }, + .apply = { NULL }, + .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 worktree\n" + "n - do not apply this hunk to 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; /* @@ -1518,6 +1562,13 @@ int run_add_p(struct repository *r, enum add_p_mode mode, s.mode = &patch_mode_checkout_head; else s.mode = &patch_mode_checkout_nothead; + } else if (mode == ADD_P_WORKTREE) { + if (!revision) + s.mode = &patch_mode_checkout_index; + else if (!strcmp(revision, "HEAD")) + s.mode = &patch_mode_worktree_head; + else + s.mode = &patch_mode_worktree_nothead; } else s.mode = &patch_mode_stage; s.revision = revision; diff --git a/builtin/add.c b/builtin/add.c index 10f9e61062..12a9ea785b 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -210,6 +210,8 @@ int run_add_interactive(const char *revision, const char *patch_mode, mode = ADD_P_RESET; else if (!strcmp(patch_mode, "--patch=checkout")) mode = ADD_P_CHECKOUT; + else if (!strcmp(patch_mode, "--patch=worktree")) + mode = ADD_P_WORKTREE; else die("'%s' not supported", patch_mode); From 368e1a1a2139ec976d33bc51ff9b6eda196b3698 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 27 Mar 2019 17:14:02 +0100 Subject: [PATCH 58/62] 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 | 73 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 72 insertions(+), 1 deletion(-) diff --git a/compat/terminal.c b/compat/terminal.c index b7f58d1781..35bca03d14 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 "hashmap.h" #if defined(HAVE_DEV_TTY) || defined(GIT_WINDOWS_NATIVE) @@ -238,6 +239,71 @@ 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->entry, + strhash(e->sequence)); + hashmap_add(&sequences, &e->entry); + } + 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; @@ -271,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 628932fde12d6657833b297d12ba1bdc8d64fd7d Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 6 Apr 2019 22:31:40 +0200 Subject: [PATCH 59/62] 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 0a0b327ed34f1b6a6663344dfa33314170da97a7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 29 Mar 2019 15:03:09 +0100 Subject: [PATCH 60/62] 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 ae2e1bb124..ede7c7f70f 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -347,7 +347,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); @@ -355,13 +355,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 32b35ac4ff918e9d70b278d67fdb8afecf2c6e6f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 28 Mar 2019 20:06:37 +0100 Subject: [PATCH 61/62] 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 ff0ef7f08e..4df54c4efe 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -20,6 +20,7 @@ linux-gcc) 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 ;; linux-gcc-4.8) From 5a415cb7ece11acbb2a76843e1169faf88192cfd Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sat, 6 Apr 2019 22:46:09 +0200 Subject: [PATCH 62/62] 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 2dafd97766..dbe964b30a 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1037,7 +1037,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 5d9bcf77e0..4f0479e071 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