Re: [PATCH 2/5] bundle: support fsck message configuration

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

Justin Tobler <jltobler@gmail.com> writes:

> I was considering making it the responsibility of the `fsck_msg_types`
> consumer to conditionally preprend the '='.

In my initial reading, I too thought it might be logically more
clear, but I changed my mind and the patch posted as is is fine
(and that last part of the sentence is what I wanted to say).

Because fsck_msg_types starts as an empty strbuf and accumulates
elements one at a time, each time we add something to it, we'd need
to check if we are adding the first element (in which case we do not
want to terminate the existing list with ",") or we already have
something in there (in which case we do want to add "," before our
new element).  Because we are doing the check anyway, instead of
saying "ah, this is the first one, so let's not add ','", saying
"the first one?  we need '='" is not too bad.

And the consuming side would not have to have a conditional "if the
string is empty, do nothing, otherwise add '=' and then the string".
The consumers can just "concatenate the string, which is possibly
empty, after the option".

So in the end, the complexity for the producer is the same, and the
consumer becomes much simpler.

And this exactly pattern (which I personally find a bit ugly) is
used by receive-pack to drive unpack-objects and index-pack, which
makes it doubly OK to use it.

Adding a comment to describe what is expected in the variable is
indeed very much appreciated, though.

Thanks.