Re: [PATCH RFC 10/11] add-patch: add support for in-memory index patching

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 20, 2025 at 05:15:03PM -0400, D. Ben Knoble wrote:
> On Wed, Aug 20, 2025 at 4:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > diff --git a/add-patch.c b/add-patch.c
> > index 1bcbc91de9..adef20c02b 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -1849,9 +1853,12 @@ static int patch_update_file(struct add_p_state *s,
> >                                          NULL, 0, NULL, 0))
> >                                 error(_("'git apply' failed"));
> >                 }
> > -               if (repo_read_index(s->r) >= 0)
> > +               read_index_from(s->index, s->index_file, s->r->gitdir);
> > +               if (read_index_from(s->index, s->index_file, s->r->gitdir) >= 0 &&
> > +                   s->index == s->r->index) {
> >                         repo_refresh_and_write_index(s->r, REFRESH_QUIET, 0,
> >                                                      1, NULL, NULL, NULL);
> > +               }
> >         }
> 
> Is this call to read_index_from duplicated? I don't see anything that
> indicates that would be desirable here.

Indeed, good catch!

> > @@ -1922,3 +1931,99 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> >         add_p_state_clear(&s);
> >         return 0;
> >  }
> > +
> > +int run_add_p_index(struct repository *r,
> > +                   struct index_state *index,
> > +                   const char *index_file,
> > +                   struct interactive_options *opts,
> > +                   const char *revision,
> > +                   const struct pathspec *ps)
> > +{
> > +       struct patch_mode mode = {
> > +               .apply_args = { "--cached", NULL },
> > +               .apply_check_args = { "--cached", NULL },
> > +               .prompt_mode = {
> > +                       N_("Stage mode change [y,n,q,a,d%s,?]? "),
> > +                       N_("Stage deletion [y,n,q,a,d%s,?]? "),
> > +                       N_("Stage addition [y,n,q,a,d%s,?]? "),
> > +                       N_("Stage this hunk [y,n,q,a,d%s,?]? ")
> > +               },
> > +               .edit_hunk_hint = N_("If the patch applies cleanly, the edited hunk "
> > +                                    "will immediately be marked for staging."),
> > +               .help_patch_text =
> > +                       N_("y - stage this hunk\n"
> > +                          "n - do not stage this hunk\n"
> > +                          "q - quit; do not stage this hunk or any of the remaining "
> > +                               "ones\n"
> > +                          "a - stage this hunk and all later hunks in the file\n"
> > +                          "d - do not stage this hunk or any of the later hunks in "
> > +                               "the file\n"),
> > +               .index_only = 1,
> > +       };
> > +       struct add_p_state s = {
> > +               .r = r,
> > +               .index = index,
> > +               .index_file = index_file,
> > +               .answer = STRBUF_INIT,
> > +               .buf = STRBUF_INIT,
> > +               .plain = STRBUF_INIT,
> > +               .colored = STRBUF_INIT,
> > +               .mode = &mode,
> > +               .revision = revision,
> > +       };
> > +       struct strbuf parent_revision = STRBUF_INIT;
> > +       char parent_tree_oid[GIT_MAX_HEXSZ + 1];
> > +       size_t binary_count = 0;
> > +       struct commit *commit;
> > +       int ret;
> > +
> > +       commit = lookup_commit_reference_by_name(revision);
> > +       if (!commit) {
> > +               err(&s, _("Revision does not refer to a commit"));
> > +               ret = -1;
> > +               goto out;
> > +       }
> > +
> > +       if (commit->parents)
> > +               oid_to_hex_r(parent_tree_oid, get_commit_tree_oid(commit->parents->item));
> > +       else
> > +               oid_to_hex_r(parent_tree_oid, r->hash_algo->empty_tree);
> > +
> > +       strbuf_addf(&parent_revision, "%s~", revision);
> > +       mode.diff_cmd[0] = "diff-tree";
> > +       mode.diff_cmd[1] = "-r";
> > +       mode.diff_cmd[2] = parent_tree_oid;
> > +
> > +       interactive_config_init(&s.cfg, r, opts);
> > +
> > +       if (parse_diff(&s, ps) < 0) {
> 
> I noticed run_add_p() calls discard_index() right before parse_diff()
> [but it also reads/refreshes the index there]. Sounds like that's not
> something we need for in-memory indices?

No. We don't want to discard contents, as it might even be that somebody
has an index that we want to apply multiple revisions to. Discarding it
is not sensible in that case.

Patrick