Hi Junio,
On Tue, 17 Dec 2019, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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
