Re: [PATCH 07/22] sequencer: future-proof read_populate_todo()

From: Jakub Narębski <jnareb@gmail.com>
Date: Aug 30, 2016, 16:07 UTC
Message-ID: <cb4402e9-6765-961d-0d67-43b74db05ef1@gmail.com>
In-Reply-To: <4043fd4b955d6a3bed1b187d9529d5382db5b961.1472457609.git.johannes.schindelin@gmx.de>

W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:

Show 23 quoted lines

> Over the next commits, we will work on improving the sequencer to the
> point where it can process the edit script of an interactive rebase. To
> that end, we will need to teach the sequencer to read interactive
> rebase's todo file. In preparation, we consolidate all places where
> that todo file is needed to call a function that we will later extend.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 8d79091..982b6e9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -32,6 +32,11 @@ static const char *get_dir(const struct replay_opts *opts)
>  	return git_path_seq_dir();
>  }
>  
> +static const char *get_todo_path(const struct replay_opts *opts)
> +{
> +	return git_path_todo_file();
> +}

I guess that in the future commit the return value of get_todo_path() would change depending on what sequencer is used for, cherry-pick or interactive rebase, that is, contents of replay_opts...

Show 9 quoted lines

> +
>  static int is_rfc2822_line(const char *buf, int len)
>  {
>  	int i;
> @@ -772,25 +777,24 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
>  static int read_populate_todo(struct commit_list **todo_list,
>  			struct replay_opts *opts)
>  {
> +	const char *todo_file = get_todo_path(opts);

...and that's why you have added this temporary variable here, to not repeat get_todo_path(opts) calculations...

Show 9 quoted lines

>  	struct strbuf buf = STRBUF_INIT;
>  	int fd, res;
>  
> -	fd = open(git_path_todo_file(), O_RDONLY);
> +	fd = open(todo_file, O_RDONLY);
>  	if (fd < 0)
> -		return error_errno(_("Could not open %s"),
> -				   git_path_todo_file());
> +		return error_errno(_("Could not open %s"), todo_file);

... So that's why it is s/git_path_todo_file()/todo_file/ replacement, and not simply...

Show 23 quoted lines

>  	if (strbuf_read(&buf, fd, 0) < 0) {
>  		close(fd);
>  		strbuf_release(&buf);
> -		return error(_("Could not read %s."), git_path_todo_file());
> +		return error(_("Could not read %s."), todo_file);
>  	}
>  	close(fd);
>  
>  	res = parse_insn_buffer(buf.buf, todo_list, opts);
>  	strbuf_release(&buf);
>  	if (res)
> -		return error(_("Unusable instruction sheet: %s"),
> -			git_path_todo_file());
> +		return error(_("Unusable instruction sheet: %s"), todo_file);
>  	return 0;
>  }
>  
> @@ -1064,7 +1068,7 @@ static int sequencer_continue(struct replay_opts *opts)
>  {
>  	struct commit_list *todo_list = NULL;
>  
> -	if (!file_exists(git_path_todo_file()))
> +	if (!file_exists(get_todo_path(opts)))

...the s/git_path_todo_file()/git_todo_path(opts)/, isn't it?

>  		return continue_single_pick();
>  	if (read_populate_opts(opts) ||
>  			read_populate_todo(&todo_list, opts))
>

Looks good; though I have not checked if all calling sites were converted.

Good work,

-- 
Jakub Narębski