From: Karthik Nayak <karthik.188@gmail.com>
Date: Tue, 14 Oct 2025 12:35:39 GMT
Subject: Re: [PATCH v4 08/12] add-patch: split out `struct interactive_options`
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:

> 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'.

> 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]

> 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.

>  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]