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

Hi Kuba,

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

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

quoted

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

quoted

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

quoted

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

quoted

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