Re: [PATCH RFC 05/11] builtin/history: implement "drop" subcommand

From: Ben Knoble <ben.knoble@gmail.com>
Date: Aug 20, 2025, 20:39 UTC
Message-ID: <F0D09FB0-5978-4BFA-87D1-BE5FA49EA839@gmail.com>
In-Reply-To: <20250819-b4-pks-history-builtin-v1-5-9b77c32688fe@pks.im>

> Le 19 août 2025 à 06:57, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> It is a fairly common operation to perform an interactive rebase so that
> one of the commits can be dropped from history.

Show 35 quoted lines

> diff --git a/builtin/history.c b/builtin/history.c
> index d1a40368e0..183ab9d5f7 100644
> --- a/builtin/history.c
> +++ b/builtin/history.c
> @@ -1,20 +1,311 @@
> #include "builtin.h"
> +#include "commit.h"
> +#include "commit-reach.h"
> +#include "config.h"
> +#include "environment.h"
> #include "gettext.h"
> +#include "hex.h"
> +#include "object-name.h"
> #include "parse-options.h"
> +#include "refs.h"
> +#include "reset.h"
> +#include "revision.h"
> +#include "sequencer.h"
> +
> +static int collect_commits(struct repository *repo,
> +               struct commit *old_commit,
> +               struct commit *new_commit,
> +               struct strvec *out)
> +{
> +    struct setup_revision_opt revision_opts = {
> +        .assume_dashdash = 1,
> +    };
> +    struct strvec revisions = STRVEC_INIT;
> +    struct commit_list *from_list = NULL;
> +    struct commit *child;
> +    struct rev_info rev = { 0 };
> +    int ret;
> +
> +    /*
> +     * Check that the old actually is an ancestor of HEAD. If not

The “old commit” perhaps?

Show 45 quoted lines

> +     * the whole request becomes nonsensical.
> +    */
> +    if (old_commit) {
> +        commit_list_insert(old_commit, &from_list);
> +        if (!repo_is_descendant_of(repo, new_commit, from_list)) {
> +            ret = error(_("commit must be reachable from current HEAD commit"));
> +            goto out;
> +        }
> +    }
> +
> +    repo_init_revisions(repo, &rev, NULL);
> +    strvec_push(&revisions, "");
> +    strvec_push(&revisions, oid_to_hex(&new_commit->object.oid));
> +    if (old_commit)
> +        strvec_pushf(&revisions, "^%s", oid_to_hex(&old_commit->object.oid));
> +    if (setup_revisions(revisions.nr, revisions.v, &rev, &revision_opts) != 1 ||
> +        prepare_revision_walk(&rev)) {
> +        ret = error(_("revision walk setup failed"));
> +        goto out;
> +    }
> +
> +    while ((child = get_revision(&rev))) {
> +        if (old_commit && !child->parents)
> +            BUG("revision walk did not find child commit");
> +        if (child->parents && child->parents->next) {
> +            ret = error(_("cannot rearrange commit history with merges"));
> +            goto out;
> +        }
> +
> +        strvec_push(out, oid_to_hex(&child->object.oid));
> +
> +        if (child->parents && old_commit &&
> +            commit_list_contains(old_commit, child->parents))
> +            break;
> +    }
> +
> +    /*
> +     * Revisions are in newest-order-first. We have to reverse the
> +     * array though so that we pick the oldest commits first. Note
> +     * that we keep the first string untouched, as it is the
> +     * equivalent of `argv[0]` to `setup_revisions()`.
> +     */
> +    for (size_t i = 0, j = out->nr - 1; i < j; i++, j--)
> +        SWAP(out->v[i], out->v[j]);
> +

But doesn’t this swap out->v[0] on first iteration? I only skimmed the code that built it up, but it doesn’t look the comment is right 🤔

Rest looked reasonable, but I don’t know the sequencer APIs very well.