Re: [PATCH v4 08/12] add-patch: split out `struct interactive_options`
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Oct 14, 2025, 12:35 UTC
- Message-ID
- <CAOLa=ZRh8LDu=-PAxiAV9QxFtjuQtC8sOojZm-4=CgN6t4vJFg@mail.gmail.com>
- In-Reply-To
- <20251001-b4-pks-history-builtin-v4-8-8e61ddb86317@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 5 quoted lines
> The `struct add_p_opt` is reused both by our the infra for "git add -p" > and "git add -i". Users of `run_add_i()` for example are expected to > pass `struct add_p_opt`. This is somewhat confusing and raises the > question which options apply to what part of the stack. >
Okay. So seems like `struct add_p_opt` is defined in 'add-patch.h' and `struct add_i_state` in 'add-interactive.h'.
Show 13 quoted lines
> But things are even more confusing than that: while callers are expected > to pass in `struct add_p_opt`, these options ultimately get used to > initialize a `struct add_i_state` that is used by both subsystems. So we > are basically going full circle here. > > Refactor the code and split out a new `struct interactive_options` that > hosts common options used by both. These options are then applied to a > `struct interactive_config` that hosts common configuration. > > This refactoring doesn't yet fully detangle the two subsystems from one > another, as we still end up calling `init_add_i_state()` in the "git add > -p" subsystem. This will be fixed in a subsequent commit. >
[snip]
Show 50 quoted lines
> diff --git a/add-patch.h b/add-patch.h
> index 4394c74107..a4a05d9d14 100644
> --- a/add-patch.h
> +++ b/add-patch.h
> @@ -1,15 +1,45 @@
> #ifndef ADD_PATCH_H
> #define ADD_PATCH_H
>
> +#include "color.h"
> +
> struct pathspec;
> struct repository;
>
> -struct add_p_opt {
> +struct interactive_options {
> int context;
> int interhunkcontext;
> };
>
> -#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }
> +#define INTERACTIVE_OPTIONS_INIT { \
> + .context = -1, \
> + .interhunkcontext = -1, \
> +}
> +
> +struct interactive_config {
> + enum git_colorbool use_color_interactive;
> + enum git_colorbool use_color_diff;
> + char header_color[COLOR_MAXLEN];
> + char help_color[COLOR_MAXLEN];
> + char prompt_color[COLOR_MAXLEN];
> + char error_color[COLOR_MAXLEN];
> + char reset_color_interactive[COLOR_MAXLEN];
> +
> + char fraginfo_color[COLOR_MAXLEN];
> + char context_color[COLOR_MAXLEN];
> + char file_old_color[COLOR_MAXLEN];
> + char file_new_color[COLOR_MAXLEN];
> + char reset_color_diff[COLOR_MAXLEN];
> +
> + int use_single_key;
> + char *interactive_diff_filter, *interactive_diff_algorithm;
> + int context, interhunkcontext;
> +};
> +
> +void interactive_config_init(struct interactive_config *cfg,
> + struct repository *r,
> + struct interactive_options *opts);
> +void interactive_config_clear(struct interactive_config *cfg);
>It feels a little odd that the `interactive_*` code lies in the 'add-patch.h' and not in the 'add-interactive.h'.
Should we also consider moving this or renaming the structs?
Nit: might be nice to make add the 'add_' prefix to them while we're here.
Show 11 quoted lines
> enum add_p_mode {
> ADD_P_ADD,
> @@ -20,7 +50,7 @@ enum add_p_mode {
> };
>
> int run_add_p(struct repository *r, enum add_p_mode mode,
> - struct add_p_opt *o, const char *revision,
> + struct interactive_options *opts, const char *revision,
> const struct pathspec *ps);
>
> #endif[snip]