Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
>>> @@ -1488,8 +1501,13 @@ static const char *unpack(int err_fd, struct shallow_info *si)
>>>
>>> argv_array_pushl(&child.args, "index-pack",
>>> "--stdin", hdr_arg, keep_arg, NULL);
>>> - if (fsck_objects)
>>> - argv_array_push(&child.args, "--strict");
>>> + if (fsck_objects) {
>>> + if (fsck_severity.len)
>>> + argv_array_pushf(&child.args, "--strict=%s",
>>> + fsck_severity.buf);
>>> + else
>>> + argv_array_push(&child.args, "--strict");
>>> + }
>>
>> Hmm. The above two hunks look suspiciously similar. Would it be
>> worth to give them a single helper function?
>
> Hmm. Not sure. I see what you mean, but for now I found
>
> + argv_array_pushf(&child.args, "--strict%s%s",
> + fsck_severity.len ? "=" : "",
> + fsck_severity.buf);
>
> to be more elegant than to add a fully-fledged new function. But if
> you feel strongly, I will gladly implement a separate function; I
> would appreciate suggestions as to the function name...
Peff first introduced that trick elsewhere in our codebase, I think,
but I find it a bit too ugly.
As you accumulate fsck_severity strbuf like this anyway:
strbuf_addf(&fsck_severity, "%s%s=%s",
fsck_severity.len ? "," : "", var, value);
to flip what to prefix each element on the list with, I wonder if it
is simpler to change that empty string to "=", which will allow you
to say this:
argv_array_pushf(&child.args, "--strict%s", fsck_severity.buf);
Or even this:
strbuf_addf(&fsck_strict_arg, "%s%s=%s",
fsck_strict_arg.len ? "," : "--strict=", var, value);
and then the child.args stuff can become
if (fsck_strict_arg.len)
argv_array_push(&child.args, fsck_strict_arg.buf);
In any case, I tend to agree with you that it is overkill to add a
helper function for just to add a single element to the argument
list.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html