Re: [PATCH v2 05/18] fsck: Allow demoting errors to warnings via receive.fsck.warn = <key>

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 21, 2015, 21:47 UTC
Message-ID: <xmqqa91bx0v5.fsf@gitster.dls.corp.google.com>
In-Reply-To: <07b285ef702031fdebcf9def59528b9c@www.dscho.org>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 26 quoted lines

>>> @@ -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.