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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 27, 2017, 23:45 UTC
Message-ID: <xmqqk1yb9tgu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.21.1.1711272230490.6482@virtualbox>

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

Show 8 quoted lines

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

Show 16 quoted lines

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