Hi Kuba,

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

> W dniu 01.09.2016 o 09:49, Johannes Schindelin pisze:
> > On Wed, 31 Aug 2016, Jakub Narębski wrote: 
> 
> >> 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.

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

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

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