Re: [PATCH 1/7] built-in add -p: prepare for patch modes other than "stage"

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 17 Dec 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +int run_add_p(struct repository *r, enum add_p_mode mode,
> > +	      const char *revision, const struct pathspec *ps);
>
> This makes readers wonder if "const struct object_id *" is more
> appropriate; "const char *revision" that holds human-readable name
> is better when the internal machinery uses it for reporting, so that
> may be what is going on here (so is the new field in add_p_state
> structure).

The main reason why this is a string instead of an object ID is to be able
to discern between the `HEAD` vs `non-HEAD` versions.

A secondary consideration was that we will want to keep the scripted
version of `add -p` as the default for now, and that needs the value as
the original string, not as the parsed object ID.

> >  #endif
> > diff --git a/add-patch.c b/add-patch.c
> > index 2c46fe5b33..8a691f07da 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -11,10 +11,33 @@ enum prompt_mode_type {
> >  	PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_HUNK
> >  };
> >
> > -static const char *prompt_mode[] = {
> > -	N_("Stage mode change [y,n,a,q,d%s,?]? "),
> > -	N_("Stage deletion [y,n,a,q,d%s,?]? "),
> > -	N_("Stage this hunk [y,n,a,q,d%s,?]? ")
> > +struct patch_mode {
> > +	const char *diff[4], *apply[4], *apply_check[4];
>
> Hardcoded "4" and not-quite descriptive names puzzle readers at the
> first glance.  Let's read on to see if they need any further
> improvement.

Good point. I added a code comment.

> > +	unsigned is_reverse:1, apply_for_checkout:1;
> > +	const char *prompt_mode[PROMPT_HUNK + 1];
>
> This relies on the enum value assignment (or listing) order to
> ensure that PROMPT_HUNK always comes at the end.  Perhaps that
> deserves a comment before "enum prompt_mode_type", e.g.
>
> 	+/* Keep PROMPT_HUNK at the end */
> 	 enum prompt_mode_type {
> 	 	PROMPT_MODE_CHANGE = 0, ...
> 	 };

I agree, and introduced `PROMPT_MODE_MAX` for that purpose.

>
> > +	const char *edit_hunk_hint, *help_patch_text;
> > +};
> > +
> > +static struct patch_mode patch_mode_stage = {
> > +	.diff = { "diff-files", NULL },
>
> Nice to see designated initializers used ;-)
>
> Mental note: the "diff" field is (probably) for "the command line
> to be used to generate the patch"
>
> > +	.apply = { "--cached", NULL },
> > +	.apply_check = { "--cached", NULL },
>
> Mental note: these "apply" and "apply_check" fields are (probably)
> not for the command line; unlike the "diff" field, these only have
> arguments.
>
> Mental note: if the three field names become confusing, perhaps we
> can clarify them by either (1) calling diff as diff_cmd[], or (2)
> calling the other as apply_args[] and apply_check_args[], or (3)
> rename both.

I renamed all three.

> > +	.is_reverse = 0,
>
> Wouldn't it be sufficient to apply the default initialization, just
> like it is done for apply_for_checkout bitfield?

Yep, I dropped those unnecessary initializations.

> > @@ -1310,6 +1345,9 @@ int run_add_p(struct repository *r, const struct pathspec *ps)
> >
> >  	init_add_i_state(&s.s, r);
> >
> > +	s.mode = &patch_mode_stage;
> > +	s.revision = revision;
>
> The phrase "mode_stage" may become problematic, as other modes that
> will be introduced, like "reset", "checkout" all will stage
> different contents to the index.  The only mode the machinery knows
> at this point in the series is how "add" stages contents to the
> index, so "patch_mode_add" might turn out to be a better choice of
> the phrase as we read the series along.  We'll see.
>
> > +		if (!strcmp(patch_mode, "--patch"))
> > +			mode = ADD_P_STAGE;
>
> The same comment applies to this enum token.

I renamed both to the `*_add` and `*_ADD` version, respectively.

Thanks,
Dscho