Re: [PATCH 06/15] sequencer: lib'ify read_populate_todo()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 25, 2016, 22:59 UTC
- Message-ID
- <xmqqeg5cjw02.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <0de75bbce8ade0c6e5cf87d3647faa71a89c6275.1471968378.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 7 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> > --- > 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.
Show 48 quoted lines
> 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()) ||