Hi Kuba,

On Tue, 30 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> 
> > 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...

Right.

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

... and to repeat only 9 characters instead of 19...

> > -	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...
> 
> > @@ -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?

Correct.

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

Thanks for the review!
Johannes