Re: [PATCH] archive-tar: fix sanity check in config parsing

[Jeff King <peff@peff.net>]

On Mon, Jan 14, 2013 at 09:17:57AM +0100, Joachim Schmitz wrote:

> >For the curious, the original version of the patch[1] read:
> >
> >+       if (prefixcmp(var, "tarfilter."))
> >+               return 0;
> >+       dot = strrchr(var, '.');
> >+       if (dot == var + 9)
> >+               return 0;
> >
> >and when I shortened the config section to "tar" in a re-roll of the
> >series, I missed the corresponding change to the offset.
> 
> Wouldn't it then be better ti use strlen("tar") rather than a 3? Or
> at least a comment?

Then you are relying on the two strings being the same, rather than the
string and the length being the same. If you wanted to DRY it up, it
would look like:

diff --git a/archive-tar.c b/archive-tar.c
index d1cce46..a7c0690 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -332,15 +332,17 @@ static int tar_filter_config(const char *var, const char *value, void *data)
 	const char *type;
 	int namelen;
 
-	if (prefixcmp(var, "tar."))
+#define SECTION "tar"
+	if (prefixcmp(var, SECTION "."))
 		return 0;
 	dot = strrchr(var, '.');
-	if (dot == var + 9)
+	if (dot == var + strlen(SECTION))
 		return 0;
 
-	name = var + 4;
+	name = var + strlen(SECTION) + 1;
 	namelen = dot - name;
 	type = dot + 1;
+#undef SECTION
 
 	ar = find_tar_filter(name, namelen);
 	if (!ar) {


(of course there are other variants where you do not use a macro, but
then you need to manually check for the "." after the prefixcmp call).
I dunno. It is technically more robust in that the offsets are computed,
but I think it is a little harder to read. Of course, I wrote the
original so I am probably not a good judge.

We could also potentially encapsulate it in a function. I think the diff
code has a very similar block.

-Peff