Hi Junio,
On 2015-01-21 22:47, Junio C Hamano wrote:
> 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);
But of course! This is what I did now:
-- snip --
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 8e6d1a1..08e3716 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -126,8 +126,8 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
}
if (skip_prefix(var, "receive.fsck.", &var)) {
- strbuf_addf(&fsck_severity, "%s%s=%s",
- fsck_severity.len ? "," : "", var, value);
+ strbuf_addf(&fsck_severity, "%c%s=%s",
+ fsck_severity.len ? ',' : '=', var, value);
return 0;
}
@@ -1487,8 +1487,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
if (quiet)
argv_array_push(&child.args, "-q");
if (fsck_objects)
- argv_array_pushf(&child.args, "--strict%s%s",
- fsck_severity.len ? "=" : "",
+ argv_array_pushf(&child.args, "--strict%s",
fsck_severity.buf);
child.no_stdout = 1;
child.err = err_fd;
@@ -1507,8 +1506,7 @@ 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_pushf(&child.args, "--strict%s%s",
- fsck_severity.len ? "=" : "",
+ argv_array_pushf(&child.args, "--strict%s",
fsck_severity.buf);
if (fix_thin)
argv_array_push(&child.args, "--fix-thin");
-- snap --
> Or even this:
>
> strbuf_addf(&fsck_strict_arg, "%s%s=%s",
> fsck_strict_arg.len ? "," : "--strict=", var, value);
Unfortunately not, because just `--strict` needs to be passed in case no severity levels were overridden.
> 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.
I am glad we agree!
Ciao,
Dscho
--
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