Re: [PATCH 09/22] sequencer: completely revamp the "todo" script parsing

Hi Kuba,

On Fri, 2 Sep 2016, Jakub Narębski wrote:

W dniu 01.09.2016 o 09:49, Johannes Schindelin pisze:

quoted

On Wed, 31 Aug 2016, Jakub Narębski wrote: 

quoted

quoted

Here todo_list uses growable array implementation of list.  Which is
I guess better on current CPU architecture, with slow memory,
limited-size caches, and adjacency prefetching.

That is not the reason that an array is used here. The array allows us
much more flexibility.

It would be nice if this reasoning (behind the change from linked list
to growable array) was mentioned in appropriate commit message, and
perhaps also in the cover letter for the series.  It is IMVHO quite
important information (that you thought obvious).

Amended.

quoted

quoted

quoted

+struct todo_item *append_todo(struct todo_list *todo_list)

Errr... I don't quite understand the name of this function.
What are you appending here to the todo_list?

A new item.

quoted

Compare string_list_append() and string_list_append_nodup(),
where the second parameter is item to append.

Yes, that is correct. In the case of a todo_item, things are a lot more
complicated, though. Some of the values have to be determined tediously
(such as the offset and length of the oneline after the "pick <oid>"
command). I just put those values directly into the newly allocated item,
is all.

I would expect sth_append command to take a list (or other collection),
an element, and return [modified] collection with the new element added.
Such API would require temporary variable in caller and memcopy in the
sth_append() function.

This is not it.  It creates a new element, expanding a list (a collection),
and then expose this element.  Which spares us memcopy... on non-critical
path.

I don't know how to name operation "grow list and return new element".
But "append" it is not.

I renamed it to append_new_todo().

quoted

quoted

quoted

-	end_of_object_name = bol + strcspn(bol, " \t\n");
+	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");

Why is this cast needed?

Because bol is a "const char *" and we need to put "NUL" temporarily to
*end_of_object_name:

Would compiler complain without this const'ness-stripping cast?

Yes. I would not have added it otherwise.

Please note that this is only necessary because I changed the parameter
from "char *" to "const char *" (which was The Right Thing To Do).

quoted

quoted

quoted

 	saved = *end_of_object_name;
 	*end_of_object_name = '\0';
 	status = get_sha1(bol, commit_sha1);
 	*end_of_object_name = saved;

Technically, this would have made a fine excuse to teach get_sha1() a
mode where it expects a length parameter instead of relying on a
NUL-terminated string.

Practically, such fine excuses cost me months in this rebase--helper
project already, and I need to protect my time better.

Put it in TODO list (and perhaps add a TODO comment) ;-).

I am also a realist: I won't be able to do anything about this. If you
care enough, please go right to town.

Thanks again,
Dscho