On Wed, Aug 20, 2025 at 04:39:34PM -0400, Ben Knoble wrote:
> > 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?
Yup, indeed.
> > + * 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 🤔
Very true. This comment is indeed stale from a previous iteration, where
`out->v[0]` was indeed `argv[0]`. Will fix.
> Rest looked reasonable, but I don’t know the sequencer APIs very well.
The sequencer API is quite complex overall, I cannot blame you :) Took
me quite a while to get the hang of it.
Patrick
