Re: [PATCH 3/5] rebase -i: add exec commands via the rebase--helper

Johannes Schindelin [off-list ref] writes:

quoted

As the name of a public function, it does not feel that this hints
it strongly enough that it is from and a part of sequencer.c API.

How about a "yes, and" instead? As in:

To further improve this patch, let's use the name
sequencer_add_exec_commands() for this function because it is defined
globally now.

I would do so when I have a single "this is strictly better"
suggestion.  In this case, I didn't, but somebody who does not have
a "better suggestion" can still have a good sense of smell to tell
something is "not right".

quoted

quoted

+	const char *todo_file = rebase_path_todo();
+	struct todo_list todo_list = TODO_LIST_INIT;
+	int fd, res, i, first = 1;
+	FILE *out;

Having had to scan backwards while trying to see what the loop that
uses this variable is doing and if it gets affected by things that
happened before we entered the loop, I'd rather not to see 'first'
initialized here, left unused for quite some time until the loop is
entered.  It would make it a lot easier to follow if it is declared
and left uninitilized here, and set to 1 immediately before the
for() loop that uses it.

Funny, I would have assumed it the other way round: since "first" always
has to be initialized with 1, I would have been surprised to see an
explicit assignment much later than it is declared.

Unfortunately that would force readers to see what happens before
the loop to see if there are cases where first is incremented, and
in this case there is not any.