Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()

[Junio C Hamano <gitster@pobox.com>]

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

> To be truly useful, the sequencer should never die() but always return
> an error.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Instead of dying there, you let the caller high up in the callchain
to notice the error and handle it (by dying).

The only caller of read_populate_todo(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, you make it notice an
error return from this function.  So this is a safe conversion to
make read_populate_todo() callable from new callers that want it not
to die, without changing the external behaviour of anything
existing.

Good.

By the way, I am writing these as review comments because I do not
want to keep repeating this kind of analysis as a reviewer.  I am
demonstrating what should have been in the commit log message
instead, so that the reviewer does not have to spend extra time, if
the reviewer trusts the author's diligence well enough, to see if
the conversion makes sense.

Please follow the example when/if you have to reroll.  I want the
patches to show the evidence of careful analysis to reviewers so
that they can gauge the trustworthiness of the patches.  With this
round of patches, honestly, I cannot tell if it is a mechanical
substitution alone, or such a substitution followed by a careful
verification of the callers.

> diff --git a/sequencer.c b/sequencer.c
> index a8c3a48..5f6b020 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -746,7 +746,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
>  	return 0;
>  }
>  
> -static void read_populate_todo(struct commit_list **todo_list,
> +static int read_populate_todo(struct commit_list **todo_list,
>  			struct replay_opts *opts)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> @@ -754,18 +754,21 @@ static void read_populate_todo(struct commit_list **todo_list,
>  
>  	fd = open(git_path_todo_file(), O_RDONLY);
>  	if (fd < 0)
> -		die_errno(_("Could not open %s"), git_path_todo_file());
> +		return error(_("Could not open %s (%s)"),
> +			git_path_todo_file(), strerror(errno));
>  	if (strbuf_read(&buf, fd, 0) < 0) {
>  		close(fd);
>  		strbuf_release(&buf);
> -		die(_("Could not read %s."), git_path_todo_file());
> +		return error(_("Could not read %s."), git_path_todo_file());
>  	}
>  	close(fd);
>  
>  	res = parse_insn_buffer(buf.buf, todo_list, opts);
>  	strbuf_release(&buf);
>  	if (res)
> -		die(_("Unusable instruction sheet: %s"), git_path_todo_file());
> +		return error(_("Unusable instruction sheet: %s"),
> +			git_path_todo_file());
> +	return 0;
>  }
>  
>  static int populate_opts_cb(const char *key, const char *value, void *data)
> @@ -1015,7 +1018,8 @@ static int sequencer_continue(struct replay_opts *opts)
>  	if (!file_exists(git_path_todo_file()))
>  		return continue_single_pick();
>  	read_populate_opts(&opts);
> -	read_populate_todo(&todo_list, opts);
> +	if (read_populate_todo(&todo_list, opts))
> +		return -1;
>  
>  	/* Verify that the conflict has been resolved */
>  	if (file_exists(git_path_cherry_pick_head()) ||