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

Hi Junio,

On 2015-01-21 22:47, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

quoted

quoted

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