Introduce ReverseItemList() to avoid quadratic costs.

Using PrependItem might build the list in the reverse of the order you
want; but it's O(n) in the number of items in the list and so is
reversing the list, whereas using AppendItem() is O(n*n), so Bad.
Use ReverseItemList() in some of the item-list functions.

Four of the Item-list functions used AppendItem() to spend quadratic
effort on building lists.  Eat my new dog-food and make these both
only linear in the size of their data.
Add cycle-detection code to item lists.

All disabled unless you define CYCLE_DETECTION when compiling.
Rearranges for loops into while loops to support what it needs to make
this work, and replace a conditional continue with applying the
reverse condition to the loop body, but these changes should be no-ops
(without the define).

Also changes some functions to call others instead of duplicating
their code (this saves me the need to duplicate the checks).  In
particular, simplify ItemList2CSV() by using other functions.  Other
code already knows how to compute the size of buffer we need.  Don't
duplicate it.
Fix leaky potential list-mangling bug.

DeleteItemGeneral's item deletion code depended on a variable last
that was meant to record the node with last->next == ip; however, due
to the use of continue (eliminated last commit, but faithfully
preserving what the code does) to skip the loop body if ip->name is
NULL, the code to update last would be skipped.  Consequently last
could be NULL when ip wasn't the list's head; and, when non-NULL,
might have several steps of ->next, via entries with NULL name, before
ip.  Changing its ->next to ip->next would drop the intermediate
entries, and leave them pointing at ip after it's free()d, with
potentially scary results.  Skipping the update of a relevant item's
->next when last was NULL and ip wasn't the list head would leave ip
in the list after it's deletion.

So move setting of last = ip out of the conditional (previously
implicit in the continue).  This ensures we can simply assert(last)
instead of checking it, in the place where it's needed; the same
code's check on ip was superfluous anyway, so assert(ip), too; and
avoid a bunch of code duplication around free()ing ip and its members.
Further tidy-up in DeleteItemGeneral.

One pair of cases in its switch (each pair is a test and its negation)
was sharing the code to compute the value, then flipping the answer
for one of the cases.  This involved retesting type after switching on
it; a little thought revealed how to avoid that; and the result can
readily be applied to every pair of cases.  While we're at it, match
may as well be a bool and a few blank lines can make it clearer what's
happening.
Add ItemIsInList() for use in an assertion.

Checks for an Item being in a list (in contrast to IsItemIn, which
checks for the list having an entry whose ->name strcmp()s the same as
a given string); this is needed in ConcatLists() to check we're not
creating a cycle.  It could also be used in reverse and delete
functions to check for cycles there, but at quadratic cost, where
concat only incurs linear cost (which it already had anyway).
Fixes and clean-up in ItemList2CSV_bound().

It's meant to concatenate a bunch of strings, joining them with a
separator.  However, it actually copied each string in turn to the
start of the buffer, while writing separators, and appending a final
'\0', in all the right places, had the content but been between them.

It also checked whether the space remaining was more than two less
than the length of the string it was about to copy into that space;
its comments make clear that it meant to leave two extra spaces, but
it actually left two too few.  It doesn't actually need two anyway, so
would have truncated the last byte if the buffer were just exactly big
enough.  Check separately, when appending a separator, for whether
there's space for it.

Its documentation also wrongly claimed that the string stored in the
buffer would have length equal to the buffer size on truncation (the
actual strlen() shall be one less) when it meant that the *return* is
the buffer size - hence *bigger* than strlen(buf), since we really do
'\0'-terminate - when we truncate.

... and re-write to use a pointer to where to write next, while
keeping track of remaining space as we go, with the -1 for '\0' taken
into account at the start.  It's just simpler that way ...
Lots of petty simplification and clarification in item_lib.c

Testing strlen(x) == 0 is just an expensive way to check x[0]; and,
while there is a case for extra parentheses to make operator
precedence overt, it's just ugly to over-do it.  Initialization is
best combined with declaration, in most cases.  Group related
activities together (e.g. alloc/free as one group, setting ->next as
another) and trust free(NULL) to be a safe no-op as specified.  Use
non-trivial booleans to avoid duplicating code.  Simplify !
Simplify SplitString.

Not every loop wants to be a for; if you find yourself having to undo
the increment step in some paths through the loop body, it's a good
hint that it doesn't.  Restructured the loop body as a simple
if/else-if/else chain in which a separator at the start is handled
just as a later unescaped separator, instead of duplicating that code.
Simplify SplitStringAsItemList().

Instead of using snprintf() to insert a character in a string, set the
string up sensibly in the first place so that we can simply copy the
character into the right place in it.  Instead of memset()ing 4k
in hopes that a failing sscanf won't touch the destination buffer,
actually check sscanf's return.
Potential nightmare-fix in DeleteItem().

When searching a list for the entry whose ->next is the one we're
about to delete, we'd stop on the last item, which doesn't have the
given entry as ->next (it has NULL).  We would then assume that, as it
(as opposed to its ->next) is non-NULL, we should hang our item's
->next off it, where it never did belong.  If the item we're deleting
had our supposed list head somewhere down->next of it, this could have
created a cycle.  So let the loop variable fall off the end of the
loop as NULL and do at least assert that a non-NULL alleged
predecessor has the node to be deleted as its ->next, before fixing
that.
Purge code for unused backup file name.

While there may be some sense in backing up a file before over-writing
it, and computing the name for that file would be useful if we did, we
don't.  So don't bother cooking up an unused name.
Log() on error when reading an item list.
Document what ListsCompare really tests (approximating equality).
Eliminate ByteSizeList in favour of ItemListSize.

Both computed the sum of lengths of strings in a list.  The sole
difference is that the latter checks for, and skips, NULL entries, on
which the former would crash.  Code outside core uses the latter, only
code inside core uses the former, so this change is self-contained to
core (easier to git administer).

Also make ItemList2CSV() robust against NULL entries, now that it's
delegating size computation to ItemListSize().
Merge pull request #1539 from ediosyncratic/item-clean-up

Clean up the code to handle Item lists

I see no complaints, so in it goes !