From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 21 Oct 2025 11:44:00 GMT
Subject: Re: [PATCH v4 08/12] add-patch: split out `struct interactive_options`
Message-ID: <aPdyAOhVjSMB9Csb@pks.im>
In-Reply-To: <CAOLa=ZRh8LDu=-PAxiAV9QxFtjuQtC8sOojZm-4=CgN6t4vJFg@mail.gmail.com>

On Tue, Oct 14, 2025 at 08:35:39AM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > 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.

The proper name for this struct would be `add_patch_interactive_config`,
but I decided against that name for now as it feels like a mouthful. I
think these two patches improve the status quo regardless of that, so
I'd prefer to just keep those as-is if you don't mind?

Patrick