From: Phillip Wood Date: Thu, 20 Nov 2025 15:05:35 GMT Subject: Re: [PATCH v6 08/11] add-patch: remove dependency on "add-interactive" subsystem Message-ID: <44f8f04f-825a-474c-8c01-35d0cfbd8073@gmail.com> In-Reply-To: <20251027-b4-pks-history-builtin-v6-8-407dd3f57ad3@pks.im> On 27/10/2025 11:33, Patrick Steinhardt wrote: > With the preceding commit we have split out interactive configuration > that is used by both "git add -p" and "git add -i". But we still > initialize that configuration in the "add -p" subsystem by calling > `init_add_i_state()`, even though we only do so to initialize the > interactive configuration as well as a repository pointer. > > Stop doing so and instead store and initialize the interactive > configuration in `struct add_p_state` directly. Makes sense Thanks Phillip > > Signed-off-by: Patrick Steinhardt > --- > add-patch.c | 70 ++++++++++++++++++++++++++++++++----------------------------- > 1 file changed, 37 insertions(+), 33 deletions(-) > > diff --git a/add-patch.c b/add-patch.c > index 5c6969927a..790c848e79 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -2,7 +2,6 @@ > #define DISABLE_SIGN_COMPARE_WARNINGS > > #include "git-compat-util.h" > -#include "add-interactive.h" > #include "add-patch.h" > #include "advice.h" > #include "config.h" > @@ -263,7 +262,8 @@ struct hunk { > }; > > struct add_p_state { > - struct add_i_state s; > + struct repository *r; > + struct interactive_config cfg; > struct strbuf answer, buf; > > /* parsed diff */ > @@ -408,7 +408,7 @@ static void add_p_state_clear(struct add_p_state *s) > for (i = 0; i < s->file_diff_nr; i++) > free(s->file_diff[i].hunk); > free(s->file_diff); > - clear_add_i_state(&s->s); > + interactive_config_clear(&s->cfg); > } > > __attribute__((format (printf, 2, 3))) > @@ -417,9 +417,9 @@ static void err(struct add_p_state *s, const char *fmt, ...) > va_list args; > > va_start(args, fmt); > - fputs(s->s.cfg.error_color, stdout); > + fputs(s->cfg.error_color, stdout); > vprintf(fmt, args); > - puts(s->s.cfg.reset_color_interactive); > + puts(s->cfg.reset_color_interactive); > va_end(args); > } > > @@ -437,7 +437,7 @@ static void setup_child_process(struct add_p_state *s, > > cp->git_cmd = 1; > strvec_pushf(&cp->env, > - INDEX_ENVIRONMENT "=%s", s->s.r->index_file); > + INDEX_ENVIRONMENT "=%s", s->r->index_file); > } > > static int parse_range(const char **p, > @@ -542,12 +542,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) > int res; > > strvec_pushv(&args, s->mode->diff_cmd); > - if (s->s.cfg.context != -1) > - strvec_pushf(&args, "--unified=%i", s->s.cfg.context); > - if (s->s.cfg.interhunkcontext != -1) > - strvec_pushf(&args, "--inter-hunk-context=%i", s->s.cfg.interhunkcontext); > - if (s->s.cfg.interactive_diff_algorithm) > - strvec_pushf(&args, "--diff-algorithm=%s", s->s.cfg.interactive_diff_algorithm); > + if (s->cfg.context != -1) > + strvec_pushf(&args, "--unified=%i", s->cfg.context); > + if (s->cfg.interhunkcontext != -1) > + strvec_pushf(&args, "--inter-hunk-context=%i", s->cfg.interhunkcontext); > + if (s->cfg.interactive_diff_algorithm) > + strvec_pushf(&args, "--diff-algorithm=%s", s->cfg.interactive_diff_algorithm); > if (s->revision) { > struct object_id oid; > strvec_push(&args, > @@ -576,9 +576,9 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) > } > strbuf_complete_line(plain); > > - if (want_color_fd(1, s->s.cfg.use_color_diff)) { > + if (want_color_fd(1, s->cfg.use_color_diff)) { > struct child_process colored_cp = CHILD_PROCESS_INIT; > - const char *diff_filter = s->s.cfg.interactive_diff_filter; > + const char *diff_filter = s->cfg.interactive_diff_filter; > > setup_child_process(s, &colored_cp, NULL); > xsnprintf((char *)args.v[color_arg_index], 8, "--color"); > @@ -811,7 +811,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk, > hunk->colored_end - hunk->colored_start); > return; > } else { > - strbuf_addstr(out, s->s.cfg.fraginfo_color); > + strbuf_addstr(out, s->cfg.fraginfo_color); > p = s->colored.buf + header->colored_extra_start; > len = header->colored_extra_end > - header->colored_extra_start; > @@ -833,7 +833,7 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk, > if (len) > strbuf_add(out, p, len); > else if (colored) > - strbuf_addf(out, "%s\n", s->s.cfg.reset_color_diff); > + strbuf_addf(out, "%s\n", s->cfg.reset_color_diff); > else > strbuf_addch(out, '\n'); > } > @@ -1222,12 +1222,12 @@ static void recolor_hunk(struct add_p_state *s, struct hunk *hunk) > > strbuf_addstr(&s->colored, > plain[current] == '-' ? > - s->s.cfg.file_old_color : > + s->cfg.file_old_color : > plain[current] == '+' ? > - s->s.cfg.file_new_color : > - s->s.cfg.context_color); > + s->cfg.file_new_color : > + s->cfg.context_color); > strbuf_add(&s->colored, plain + current, eol - current); > - strbuf_addstr(&s->colored, s->s.cfg.reset_color_diff); > + strbuf_addstr(&s->colored, s->cfg.reset_color_diff); > if (next > eol) > strbuf_add(&s->colored, plain + eol, next - eol); > current = next; > @@ -1356,7 +1356,7 @@ static int run_apply_check(struct add_p_state *s, > > static int read_single_character(struct add_p_state *s) > { > - if (s->s.cfg.use_single_key) { > + if (s->cfg.use_single_key) { > int res = read_key_without_echo(&s->answer); > printf("%s\n", res == EOF ? "" : s->answer.buf); > return res; > @@ -1370,7 +1370,7 @@ static int read_single_character(struct add_p_state *s) > static int prompt_yesno(struct add_p_state *s, const char *prompt) > { > for (;;) { > - color_fprintf(stdout, s->s.cfg.prompt_color, "%s", _(prompt)); > + color_fprintf(stdout, s->cfg.prompt_color, "%s", _(prompt)); > fflush(stdout); > if (read_single_character(s) == EOF) > return -1; > @@ -1678,15 +1678,15 @@ static int patch_update_file(struct add_p_state *s, > else > prompt_mode_type = PROMPT_HUNK; > > - printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->s.cfg.prompt_color, > + printf("%s(%"PRIuMAX"/%"PRIuMAX") ", s->cfg.prompt_color, > (uintmax_t)hunk_index + 1, > (uintmax_t)(file_diff->hunk_nr > ? file_diff->hunk_nr > : 1)); > printf(_(s->mode->prompt_mode[prompt_mode_type]), > s->buf.buf); > - if (*s->s.cfg.reset_color_interactive) > - fputs(s->s.cfg.reset_color_interactive, stdout); > + if (*s->cfg.reset_color_interactive) > + fputs(s->cfg.reset_color_interactive, stdout); > fflush(stdout); > if (read_single_character(s) == EOF) > break; > @@ -1848,7 +1848,7 @@ static int patch_update_file(struct add_p_state *s, > err(s, _("Sorry, cannot split this hunk")); > } else if (!split_hunk(s, file_diff, > hunk - file_diff->hunk)) { > - color_fprintf_ln(stdout, s->s.cfg.header_color, > + color_fprintf_ln(stdout, s->cfg.header_color, > _("Split into %d hunks."), > (int)splittable_into); > rendered_hunk_index = -1; > @@ -1866,7 +1866,7 @@ static int patch_update_file(struct add_p_state *s, > } else if (s->answer.buf[0] == '?') { > const char *p = _(help_patch_remainder), *eol = p; > > - color_fprintf(stdout, s->s.cfg.help_color, "%s", > + color_fprintf(stdout, s->cfg.help_color, "%s", > _(s->mode->help_patch_text)); > > /* > @@ -1884,7 +1884,7 @@ static int patch_update_file(struct add_p_state *s, > if (*p != '?' && !strchr(s->buf.buf, *p)) > continue; > > - color_fprintf_ln(stdout, s->s.cfg.help_color, > + color_fprintf_ln(stdout, s->cfg.help_color, > "%.*s", (int)(eol - p), p); > } > } else { > @@ -1904,7 +1904,7 @@ static int patch_update_file(struct add_p_state *s, > strbuf_reset(&s->buf); > reassemble_patch(s, file_diff, 0, &s->buf); > > - discard_index(s->s.r->index); > + discard_index(s->r->index); > if (s->mode->apply_for_checkout) > apply_for_checkout(s, &s->buf, > s->mode->is_reverse); > @@ -1915,8 +1915,8 @@ static int patch_update_file(struct add_p_state *s, > NULL, 0, NULL, 0)) > error(_("'git apply' failed")); > } > - if (repo_read_index(s->s.r) >= 0) > - repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0, > + if (repo_read_index(s->r) >= 0) > + repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0, > 1, NULL, NULL, NULL); > } > > @@ -1929,11 +1929,15 @@ int run_add_p(struct repository *r, enum add_p_mode mode, > const struct pathspec *ps) > { > struct add_p_state s = { > - { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT > + .r = r, > + .answer = STRBUF_INIT, > + .buf = STRBUF_INIT, > + .plain = STRBUF_INIT, > + .colored = STRBUF_INIT, > }; > size_t i, binary_count = 0; > > - init_add_i_state(&s.s, r, opts); > + interactive_config_init(&s.cfg, r, opts); > > if (mode == ADD_P_STASH) > s.mode = &patch_mode_stash; >