Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> +/* Insert into todo_list in same order; commit_list_insert reverses
> + * the order

Style: end the first line of multi-line comment at "/*".

As you say "in same order", you solicit a question "The same as what?".
As you say "insert reverses the order", you sound as if you are
complaining you do not want insert to do so.

And you do not need either of them. The function is "append", and if you
explain it as "append", you do not have to contrast it with "insert".
In other words, starting this comment with

        /*
         * Append a commit at the end of the commit_list.

is perfectly adequate, I think. More useful would be (although it could be
read from the usage example) to help callers what "next" means, perhaps
like:

         * next starts by pointing at the variable that holds the head of
         * the list when the for an empty commit_list, and is updated to
         * point at the "next" field of the last item on the list, as new
         * commits are appended to the list.

> + *
> + * Usage example:
> + *
> + *     struct commit_list *list;
> + *     struct commit_list **next = &list;
> + *
> + *     next = commit_list_append(c1, next);
> + *     next = commit_list_append(c2, next);
> + *     *next = NULL;
> + *     assert(commit_list_count(list) == 2);
> + *     return list;
> + *
> + * Don't forget to NULL-terminate!

I am still not convinced that making it the caller's responsibility to
NULL-terminate the list after it finishes to append is a good trade-off
between run-time performance and ease of API use.  If you are appending
thousands of commits to a commit list in a tight loop, surely you would
save the same thousands of assignment of NULL to the next field of the
element at the tail of the list, which may reduce the instruction count a
tiny bit, but that field was assigned in the last round in that tight loop
and the cacheline would likely to be owned by the CPU already, so it might
not make much practical difference.