On Mon, Jul 09, 2018 at 10:24:25PM +0200, Johannes Schindelin wrote:
> > diff --git a/sequencer.c b/sequencer.c
> > index f692b2ef44..234666b980 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3637,7 +3637,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
> > return error(_("revision walk setup failed"));
> > cmit = get_revision(opts->revs);
> > if (!cmit || get_revision(opts->revs))
> > - return error("BUG: expected exactly one commit from walk");
> > + return error(_("empty commit set passed"));
>
> Should this not rather be
>
> - if (!cmit || get_revision(opts->revs))
> - return error("BUG: expected exactly one commit from walk");
> + if (!cmit)
> + return error(_("empty commit set passed"));
> + if (get_revision(opts->revs))
> + return error(_("unexpected extra commit from walk"));
Yeah, you're right. I'm not sure how a single rev with no-walk would
ever turn up more than one commit, though. So I think we should probably
go with:
if (!cmit)
return error(_("empty commit set passed"));
if (get_revision(opts->revs))
BUG("unexpected extra commit from walk");
And then if we ever see that case, we can decide from there what the
right action is (though _probably_ it's just to emit an error like you
have above, it might be a sign that our single-pick logic is wrong).
I'll re-roll in that direction, and discuss further in the commit
message.
-Peff
