Re: [PATCH 2/2] sequencer: don't say BUG on bogus input

From: Jeff King <peff@peff.net>
Date: Jul 10, 2018, 02:15 UTC
Message-ID: <20180710021505.GA10248@sigill.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.1807092222300.75@tvgsbejvaqbjf.bet>

On Mon, Jul 09, 2018 at 10:24:25PM +0200, Johannes Schindelin wrote:

Show 19 quoted lines

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