From: Elijah Newren <newren@gmail.com>
Date: Thu, 20 Nov 2025 07:04:21 GMT
Subject: Re: [PATCH v6 09/11] add-patch: add support for in-memory index patching
Message-ID: <CABPp-BGRnx7+qvFcDeWCZEZm1aRn=kRezZ2KZA0E=8hji9Vjiw@mail.gmail.com>
In-Reply-To: <20251027-b4-pks-history-builtin-v6-9-407dd3f57ad3@pks.im>

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?