Justin Tobler <jltobler@xxxxxxxxx> 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.
