From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 02 Dec 2025 18:49:05 GMT
Subject: Re: [PATCH v6 09/11] add-patch: add support for in-memory index patching
Message-ID: <aS80oc61kHICD0zZ@pks.im>
In-Reply-To: <CABPp-BGRnx7+qvFcDeWCZEZm1aRn=kRezZ2KZA0E=8hji9Vjiw@mail.gmail.com>

On Wed, Nov 19, 2025 at 11:04:21PM -0800, Elijah Newren wrote:
> On Mon, Oct 27, 2025 at 4:34 AM Patrick Steinhardt <ps@pks.im> wrote:
> 
> > +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) {
> > +               ret = -1;
> > +               goto out;
> > +       }
> > +
> > +       for (size_t i = 0; i < s.file_diff_nr; i++) {
> > +               if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
> > +                       binary_count++;
> > +               else if (patch_update_file(&s, s.file_diff + i))
> > +                       break;
> > +       }
> > +
> > +       if (s.file_diff_nr == 0) {
> > +               err(&s, _("No changes."));
> > +               ret = -1;
> > +               goto out;
> > +       }
> > +
> > +       if (binary_count == s.file_diff_nr) {
> > +               err(&s, _("Only binary files changed."));
> > +               ret = -1;
> > +               goto out;
> > +       }
> > +
> > +       ret = 0;
> > +
> > +out:
> > +       strbuf_release(&parent_revision);
> > +       add_p_state_clear(&s);
> > +       return ret;
> > +}
> 
> I'm totally unfamiliar with add-patch.[ch] beyond what I've been
> reviewing in this series, so this may be a dumb/naive question, but
> why add a sibling run_add_p_index() to run_add_p() via
> copy+paste+modify?  (Or is it not copy+paste+modify in some
> interesting way?)  I'm worried the two will drift, and I'm curious
> whether run_add_p() should just be calling run_add_p_index() and just
> passing r->index for the index field.  Is there a reason that doesn't
> work?

Most of the function isn't actually copy-paste-modify. Out of the ~90
lines of code of the new function only ~20 are the same. We could of
course introduce a function to share those lines, but it doesn't save us
_that_ much.

I think the logic is non-trivial enough though to warrant it being
duplicated regardless of that.

Patrick