Re: [PATCH 04/15] sequencer: lib'ify prepare_revs()

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2016, 22:51 UTC
Message-ID: <xmqqinuojwc4.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <ce6a7cac9f0bd53e44129c8f72818adc4bb4a934.1471968378.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 5 quoted lines

> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

I am still looking at sequencer.c in 'master', but I do not think that the sole caller of this function, walk_revs_populate_todo(), is prepared to act on an error return from this function and instead it expects this to die() when in trouble. And I do not think I saw the function touched in the steps so far.

So this step smells like a fishy conversion to me.

Show 30 quoted lines

>  sequencer.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 6ac2187..b90294f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -621,7 +621,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  	return res;
>  }
>  
> -static void prepare_revs(struct replay_opts *opts)
> +static int prepare_revs(struct replay_opts *opts)
>  {
>  	/*
>  	 * picking (but not reverting) ranges (but not individual revisions)
> @@ -631,10 +631,11 @@ static void prepare_revs(struct replay_opts *opts)
>  		opts->revs->reverse ^= 1;
>  
>  	if (prepare_revision_walk(opts->revs))
> -		die(_("revision walk setup failed"));
> +		return error(_("revision walk setup failed"));
>  
>  	if (!opts->revs->commits)
> -		die(_("empty commit set passed"));
> +		return error(_("empty commit set passed"));
> +	return 0;
>  }
>  
>  static void read_and_refresh_cache(struct replay_opts *opts)