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

> > 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?

> > +     * 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 🤔