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