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

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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