Re: [PATCH v4 08/12] add-patch: split out `struct interactive_options`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 21, 2025, 11:44 UTC
- 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:
Show 59 quoted lines
> 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