[PATCH 0/9] git-verify-pack: clean up and make builtin

[PATCH 0/9] git-verify-pack: clean up and make builtin

[Rene Scharfe]

This patch series makes git-verify-pack handle user supplied parameters
much more carefully and converts it to a builtin command.  As a bonus it
plugs a memory leak and makes some comments clearer.  It applies to the
current next branch (a934882).

[PATCH 1/9] Add has_extension()

[Rene Scharfe]

The little helper has_extension() documents through its name what we are
trying to do and makes sure we don't forget the underrun check.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 git-compat-util.h |    6 ++++++
 help.c            |    2 +-
 http-fetch.c      |    2 +-
 index-pack.c      |    2 +-
 local-fetch.c     |    2 +-
 refs.c            |    2 +-
 sha1_file.c       |    2 +-
 verify-pack.c     |    2 +-
 8 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 3bcf5b1..dd92093 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -139,6 +139,12 @@ static inline ssize_t xwrite(int fd, con
 	}
 }
 
+static inline int has_extension(const char *filename, int len, const char *ext)
+{
+	int extlen = strlen(ext);
+	return len > extlen && !memcmp(filename + len - extlen, ext, extlen);
+}
+
 /* Sane ctype - no locale, and works with signed chars */
 #undef isspace
 #undef isdigit
diff --git a/help.c b/help.c
index fb731cc..7a7f775 100644
--- a/help.c
+++ b/help.c
@@ -140,7 +140,7 @@ static void list_commands(const char *ex
 			continue;
 
 		entlen = strlen(de->d_name);
-		if (4 < entlen && !strcmp(de->d_name + entlen - 4, ".exe"))
+		if (has_extension(de->d_name, entlen, ".exe"))
 			entlen -= 4;
 
 		if (longest < entlen)
diff --git a/http-fetch.c b/http-fetch.c
index 36af3e5..6ea39f0 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -870,7 +870,7 @@ static void process_ls_pack(struct remot
 
 	if (strlen(ls->dentry_name) == 63 &&
 	    !strncmp(ls->dentry_name, "objects/pack/pack-", 18) &&
-	    !strncmp(ls->dentry_name+58, ".pack", 5)) {
+	    has_extension(ls->dentry_name, 63, ".pack")) {
 		get_sha1_hex(ls->dentry_name + 18, sha1);
 		setup_index(ls->repo, sha1);
 	}
diff --git a/index-pack.c b/index-pack.c
index b39953d..a91e39e 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -447,7 +447,7 @@ int main(int argc, char **argv)
 		usage(index_pack_usage);
 	if (!index_name) {
 		int len = strlen(pack_name);
-		if (len < 5 || strcmp(pack_name + len - 5, ".pack"))
+		if (!has_extension(pack_name, len, ".pack"))
 			die("packfile name '%s' does not end with '.pack'",
 			    pack_name);
 		index_name_buf = xmalloc(len);
diff --git a/local-fetch.c b/local-fetch.c
index 4bf86fb..59af57c 100644
--- a/local-fetch.c
+++ b/local-fetch.c
@@ -44,7 +44,7 @@ static int setup_indices(void)
 	while ((de = readdir(dir)) != NULL) {
 		int namelen = strlen(de->d_name);
 		if (namelen != 50 || 
-		    strcmp(de->d_name + namelen - 5, ".pack"))
+		    !has_extension(de->d_name, namelen, ".pack"))
 			continue;
 		get_sha1_hex(de->d_name + 5, sha1);
 		setup_index(sha1);
diff --git a/refs.c b/refs.c
index 02850b6..b01835f 100644
--- a/refs.c
+++ b/refs.c
@@ -147,7 +147,7 @@ static int do_for_each_ref(const char *b
 			namelen = strlen(de->d_name);
 			if (namelen > 255)
 				continue;
-			if (namelen>5 && !strcmp(de->d_name+namelen-5,".lock"))
+			if (has_extension(de->d_name, namelen, ".lock"))
 				continue;
 			memcpy(path + baselen, de->d_name, namelen+1);
 			if (stat(git_path("%s", path), &st) < 0)
diff --git a/sha1_file.c b/sha1_file.c
index 3d7a7d4..f0a4a7e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -608,7 +608,7 @@ static void prepare_packed_git_one(char 
 		int namelen = strlen(de->d_name);
 		struct packed_git *p;
 
-		if (strcmp(de->d_name + namelen - 4, ".idx"))
+		if (!has_extension(de->d_name, namelen, ".idx"))
 			continue;
 
 		/* we have .idx.  Is it a file we can map? */
diff --git a/verify-pack.c b/verify-pack.c
index c99db9d..ef00204 100644
--- a/verify-pack.c
+++ b/verify-pack.c
@@ -10,7 +10,7 @@ static int verify_one_pack(char *arg, in
 		/* Should name foo.idx, but foo.pack may be named;
 		 * convert it to foo.idx
 		 */
-		if (!strcmp(arg + len - 5, ".pack")) {
+		if (has_extension(arg, len, ".pack")) {
 			strcpy(arg + len - 5, ".idx");
 			len--;
 		}
-- 
1.4.2.rc2.g822a

[PATCH 2/9] git-verify-pack: show usage when no pack was specified

[Rene Scharfe]

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/verify-pack.c b/verify-pack.c
index ef00204..7201596 100644
--- a/verify-pack.c
+++ b/verify-pack.c
@@ -34,6 +34,7 @@ int main(int ac, char **av)
 	int errs = 0;
 	int verbose = 0;
 	int no_more_options = 0;
+	int nothing_done = 1;
 
 	while (1 < ac) {
 		char path[PATH_MAX];
@@ -50,8 +51,13 @@ int main(int ac, char **av)
 			strcpy(path, av[1]);
 			if (verify_one_pack(path, verbose))
 				errs++;
+			nothing_done = 0;
 		}
 		ac--; av++;
 	}
+
+	if (nothing_done)
+		usage(verify_pack_usage);
+
 	return !!errs;
 }
-- 
1.4.2.rc2.g822a

[PATCH 3/9] git-verify-pack: more careful path handling

[Rene Scharfe]

Use strlcpy() to copy the filename into a buffer and complain if it
doesn't fit.  Also move the path buffer into verify_one_pack(); it is
used only there.  Now we can const'ify the first argument of this
function.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/verify-pack.c b/verify-pack.c
index 7201596..2c814a6 100644
--- a/verify-pack.c
+++ b/verify-pack.c
@@ -1,10 +1,15 @@
 #include "cache.h"
 #include "pack.h"
 
-static int verify_one_pack(char *arg, int verbose)
+static int verify_one_pack(const char *path, int verbose)
 {
-	int len = strlen(arg);
+	char arg[PATH_MAX];
+	int len;
 	struct packed_git *g;
+
+	len = strlcpy(arg, path, PATH_MAX);
+	if (len >= PATH_MAX)
+		return error("name too long: %s", path);
 	
 	while (1) {
 		/* Should name foo.idx, but foo.pack may be named;
@@ -37,8 +42,6 @@ int main(int ac, char **av)
 	int nothing_done = 1;
 
 	while (1 < ac) {
-		char path[PATH_MAX];
-
 		if (!no_more_options && av[1][0] == '-') {
 			if (!strcmp("-v", av[1]))
 				verbose = 1;
@@ -48,8 +51,7 @@ int main(int ac, char **av)
 				usage(verify_pack_usage);
 		}
 		else {
-			strcpy(path, av[1]);
-			if (verify_one_pack(path, verbose))
+			if (verify_one_pack(av[1], verbose))
 				errs++;
 			nothing_done = 0;
 		}
-- 
1.4.2.rc2.g822a

[PATCH 4/9] git-verify-pack: insist on .idx extension

[Rene Scharfe]

git-verify-pack can be called with a filename without .idx extension.
add_packed_git() on the other hand depends on its presence.  So
instead of trying to call it with whatever the user gave us check for
that extension and add it if it's missing.

That means that you can't name your index file "blah" and your pack
file ".pack" anymore ("git-verify-pack blah" currently works in that
case).  I think this regression is a good change. ;-)

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff a/verify-pack.c b/verify-pack.c
--- a/verify-pack.c
+++ b/verify-pack.c
@@ -18,13 +18,12 @@
 		if (has_extension(arg, len, ".pack")) {
 			strcpy(arg + len - 5, ".idx");
 			len--;
+		} else if (!has_extension(arg, len, ".idx")) {
+			if (len + 4 >= PATH_MAX)
+				return error("name too long: %s.idx", arg);
+			strcpy(arg + len, ".idx");
+			len += 4;
 		}
-		/* Should name foo.idx now */
-		if ((g = add_packed_git(arg, len, 1)))
-			break;
-		/* No?  did you name just foo? */
-		strcpy(arg + len, ".idx");
-		len += 4;
 		if ((g = add_packed_git(arg, len, 1)))
 			break;
 		return error("packfile %s not found.", arg);

[PATCH 5/9] git-verify-pack: get rid of while loop

[Rene Scharfe]

Get rid of that while loop which was apparently used as a way to avoid
goto's (why?).  It's easy now because there is only one break left at
the end of it.  Also make the comment clearer.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff a/verify-pack.c b/verify-pack.c
--- a/verify-pack.c
+++ b/verify-pack.c
@@ -11,23 +11,23 @@
 	if (len >= PATH_MAX)
 		return error("name too long: %s", path);
 	
-	while (1) {
-		/* Should name foo.idx, but foo.pack may be named;
-		 * convert it to foo.idx
-		 */
-		if (has_extension(arg, len, ".pack")) {
-			strcpy(arg + len - 5, ".idx");
-			len--;
-		} else if (!has_extension(arg, len, ".idx")) {
-			if (len + 4 >= PATH_MAX)
-				return error("name too long: %s.idx", arg);
-			strcpy(arg + len, ".idx");
-			len += 4;
-		}
-		if ((g = add_packed_git(arg, len, 1)))
-			break;
-		return error("packfile %s not found.", arg);
+	/*
+	 * In addition to "foo.idx" we accept "foo.pack" and "foo";
+	 * normalize these forms to "foo.idx" for add_packed_git().
+	 */
+	if (has_extension(arg, len, ".pack")) {
+		strcpy(arg + len - 5, ".idx");
+		len--;
+	} else if (!has_extension(arg, len, ".idx")) {
+		if (len + 4 >= PATH_MAX)
+			return error("name too long: %s.idx", arg);
+		strcpy(arg + len, ".idx");
+		len += 4;
 	}
+
+	if (!(g = add_packed_git(arg, len, 1)))
+		return error("packfile %s not found.", arg);
+
 	return verify_pack(g, verbose);
 }
 

[PATCH 6/9] git-verify-pack: free pack after use and a cleanup

[Rene Scharfe]

Plug memory leak in verify_one_pack() by freeing the struct packed_git
we got from add_packed_git().  Also rename g to pack and pull an
assignment out of an if statement while we're at it.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/verify-pack.c b/verify-pack.c
index 1dcef52..c7bed1e 100644
--- a/verify-pack.c
+++ b/verify-pack.c
@@ -5,7 +5,8 @@ static int verify_one_pack(const char *p
 {
 	char arg[PATH_MAX];
 	int len;
-	struct packed_git *g;
+	struct packed_git *pack;
+	int err;
 
 	len = strlcpy(arg, path, PATH_MAX);
 	if (len >= PATH_MAX)
@@ -25,10 +26,14 @@ static int verify_one_pack(const char *p
 		len += 4;
 	}
 
-	if (!(g = add_packed_git(arg, len, 1)))
+	pack = add_packed_git(arg, len, 1);
+	if (!pack)
 		return error("packfile %s not found.", arg);
 
-	return verify_pack(g, verbose);
+	err = verify_pack(pack, verbose);
+	free(pack);
+
+	return err;
 }
 
 static const char verify_pack_usage[] = "git-verify-pack [-v] <pack>...";
-- 
1.4.2.rc2.g822a

[PATCH 7/9] git-verify-pack: buffer overrun paranoia

[Rene Scharfe]

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/verify-pack.c b/verify-pack.c
index 94fe0f3..1076001 100644
--- a/verify-pack.c
+++ b/verify-pack.c
@@ -26,6 +26,15 @@ static int verify_one_pack(const char *p
 		len += 4;
 	}
 
+	/*
+	 * add_packed_git() uses our buffer (containing "foo.idx") to
+	 * build the pack filename ("foo.pack").  Make sure it fits.
+	 */
+	if (len + 1 >= PATH_MAX) {
+		arg[len - 4] = '\0';
+		return error("name too long: %s.pack", arg);
+	}
+
 	pack = add_packed_git(arg, len, 1);
 	if (!pack)
 		return error("packfile %s not found.", arg);
-- 
1.4.2.rc2.g822a

[PATCH 8/9] git-verify-pack: no need to count errors

[Rene Scharfe]

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/verify-pack.c b/verify-pack.c
index 1076001..f12cefe 100644
--- a/verify-pack.c
+++ b/verify-pack.c
@@ -49,7 +49,7 @@ static const char verify_pack_usage[] = 
 
 int main(int ac, char **av)
 {
-	int errs = 0;
+	int err = 0;
 	int verbose = 0;
 	int no_more_options = 0;
 	int nothing_done = 1;
@@ -65,7 +65,7 @@ int main(int ac, char **av)
 		}
 		else {
 			if (verify_one_pack(av[1], verbose))
-				errs++;
+				err = 1;
 			nothing_done = 0;
 		}
 		ac--; av++;
@@ -74,5 +74,5 @@ int main(int ac, char **av)
 	if (nothing_done)
 		usage(verify_pack_usage);
 
-	return !!errs;
+	return err;
 }
-- 
1.4.2.rc2.g822a

[PATCH 9/9] git-verify-pack: make builtin

[Rene Scharfe]

Convert git-verify-pack to a builtin command.  Also rename ac to argc
and av to argv for consistancy.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Makefile              |    1 +
 builtin-verify-pack.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++
 builtin.h             |    1 +
 git.c                 |    1 +
 verify-pack.c         |   76 ------------------------------------------------
 5 files changed, 80 insertions(+), 76 deletions(-)

diff --git a/Makefile b/Makefile
index faa118a..07c421b 100644
--- a/Makefile
+++ b/Makefile
@@ -295,6 +295,7 @@ BUILTIN_OBJS = \
 	builtin-update-index.o \
 	builtin-update-ref.o \
 	builtin-upload-tar.o \
+	builtin-verify-pack.o \
 	builtin-write-tree.o
 
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
diff --git a/builtin-verify-pack.c b/builtin-verify-pack.c
new file mode 100644
index 0000000..15f2502
--- /dev/null
+++ b/builtin-verify-pack.c
@@ -0,0 +1,79 @@
+#include "builtin.h"
+#include "cache.h"
+#include "pack.h"
+
+static int verify_one_pack(const char *path, int verbose)
+{
+	char arg[PATH_MAX];
+	int len;
+	struct packed_git *pack;
+	int err;
+
+	len = strlcpy(arg, path, PATH_MAX);
+	if (len >= PATH_MAX)
+		return error("name too long: %s", path);
+
+	/*
+	 * In addition to "foo.idx" we accept "foo.pack" and "foo";
+	 * normalize these forms to "foo.idx" for add_packed_git().
+	 */
+	if (has_extension(arg, len, ".pack")) {
+		strcpy(arg + len - 5, ".idx");
+		len--;
+	} else if (!has_extension(arg, len, ".idx")) {
+		if (len + 4 >= PATH_MAX)
+			return error("name too long: %s.idx", arg);
+		strcpy(arg + len, ".idx");
+		len += 4;
+	}
+
+	/*
+	 * add_packed_git() uses our buffer (containing "foo.idx") to
+	 * build the pack filename ("foo.pack").  Make sure it fits.
+	 */
+	if (len + 1 >= PATH_MAX) {
+		arg[len - 4] = '\0';
+		return error("name too long: %s.pack", arg);
+	}
+
+	pack = add_packed_git(arg, len, 1);
+	if (!pack)
+		return error("packfile %s not found.", arg);
+
+	err = verify_pack(pack, verbose);
+	free(pack);
+
+	return err;
+}
+
+static const char verify_pack_usage[] = "git-verify-pack [-v] <pack>...";
+
+int cmd_verify_pack(int argc, const char **argv, const char *prefix)
+{
+	int err = 0;
+	int verbose = 0;
+	int no_more_options = 0;
+	int nothing_done = 1;
+
+	while (1 < argc) {
+		if (!no_more_options && argv[1][0] == '-') {
+			if (!strcmp("-v", argv[1]))
+				verbose = 1;
+			else if (!strcmp("--", argv[1]))
+				no_more_options = 1;
+			else
+				usage(verify_pack_usage);
+		}
+		else {
+			if (verify_one_pack(argv[1], verbose))
+				err = 1;
+			nothing_done = 0;
+		}
+		argc--; argv++;
+	}
+
+	if (nothing_done)
+		usage(verify_pack_usage);
+
+	return err;
+}
diff --git a/builtin.h b/builtin.h
index c0bdb05..ade58c4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -59,5 +59,6 @@ extern int cmd_upload_tar(int argc, cons
 extern int cmd_version(int argc, const char **argv, const char *prefix);
 extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
 extern int cmd_write_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_pack(int argc, const char **argv, const char *prefix);
 
 #endif
diff --git a/git.c b/git.c
index db0f867..5da7787 100644
--- a/git.c
+++ b/git.c
@@ -270,6 +270,7 @@ static void handle_internal_command(int 
 		{ "version", cmd_version },
 		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
 		{ "write-tree", cmd_write_tree, RUN_SETUP },
+		{ "verify-pack", cmd_verify_pack },
 	};
 	int i;
 
diff --git a/verify-pack.c b/verify-pack.c
deleted file mode 100644
index f12cefe..0000000
--- a/verify-pack.c
+++ /dev/null
@@ -1,78 +0,0 @@
-#include "cache.h"
-#include "pack.h"
-
-static int verify_one_pack(const char *path, int verbose)
-{
-	char arg[PATH_MAX];
-	int len;
-	struct packed_git *pack;
-	int err;
-
-	len = strlcpy(arg, path, PATH_MAX);
-	if (len >= PATH_MAX)
-		return error("name too long: %s", path);
-	
-	/*
-	 * In addition to "foo.idx" we accept "foo.pack" and "foo";
-	 * normalize these forms to "foo.idx" for add_packed_git().
-	 */
-	if (has_extension(arg, len, ".pack")) {
-		strcpy(arg + len - 5, ".idx");
-		len--;
-	} else if (!has_extension(arg, len, ".idx")) {
-		if (len + 4 >= PATH_MAX)
-			return error("name too long: %s.idx", arg);
-		strcpy(arg + len, ".idx");
-		len += 4;
-	}
-
-	/*
-	 * add_packed_git() uses our buffer (containing "foo.idx") to
-	 * build the pack filename ("foo.pack").  Make sure it fits.
-	 */
-	if (len + 1 >= PATH_MAX) {
-		arg[len - 4] = '\0';
-		return error("name too long: %s.pack", arg);
-	}
-
-	pack = add_packed_git(arg, len, 1);
-	if (!pack)
-		return error("packfile %s not found.", arg);
-
-	err = verify_pack(pack, verbose);
-	free(pack);
-
-	return err;
-}
-
-static const char verify_pack_usage[] = "git-verify-pack [-v] <pack>...";
-
-int main(int ac, char **av)
-{
-	int err = 0;
-	int verbose = 0;
-	int no_more_options = 0;
-	int nothing_done = 1;
-
-	while (1 < ac) {
-		if (!no_more_options && av[1][0] == '-') {
-			if (!strcmp("-v", av[1]))
-				verbose = 1;
-			else if (!strcmp("--", av[1]))
-				no_more_options = 1;
-			else
-				usage(verify_pack_usage);
-		}
-		else {
-			if (verify_one_pack(av[1], verbose))
-				err = 1;
-			nothing_done = 0;
-		}
-		ac--; av++;
-	}
-
-	if (nothing_done)
-		usage(verify_pack_usage);
-
-	return err;
-}
-- 
1.4.2.rc2.g822a

Re: [PATCH 8/9] git-verify-pack: no need to count errors

[Johannes Schindelin]

Hi,

there was no harm done counting the errors, i.e. no need to "fix" that.

Ciao,
Dscho

Re: [PATCH 9/9] git-verify-pack: make builtin

[Johannes Schindelin]

Hi,

On Thu, 10 Aug 2006, Rene Scharfe wrote:

>  Makefile              |    1 +
>  builtin-verify-pack.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++
>  builtin.h             |    1 +
>  git.c                 |    1 +
>  verify-pack.c         |   76 ------------------------------------------------
>  5 files changed, 80 insertions(+), 76 deletions(-)

This would look much nicer with "-M". Maybe we should make that a default 
for git-format-patch?

Ciao,
Dscho

Re: [PATCH 1/9] Add has_extension()

[Johannes Schindelin]

Hi,

On Thu, 10 Aug 2006, Rene Scharfe wrote:

> The little helper has_extension() documents through its name what we are
> trying to do and makes sure we don't forget the underrun check.

While I think it is a good change, it is independent of verify-pack, no?

Ciao,
Dscho

Re: [PATCH 8/9] git-verify-pack: no need to count errors

[Rene Scharfe]

Johannes Schindelin schrieb:
> there was no harm done counting the errors, i.e. no need to "fix" that.

Yes, it's more of a style thing.  Why count when you don't need the
number?  Why use "!!" when you can use "1" explicitly?

And then there's the danger of counter wrap-around. ;-)

It's surely one of those bikeshed colour issues, admittedly.

Thanks,
René

Re: [PATCH 1/9] Add has_extension()

[Rene Scharfe]

Johannes Schindelin schrieb:
> On Thu, 10 Aug 2006, Rene Scharfe wrote:
> 
>> The little helper has_extension() documents through its name what we are
>> trying to do and makes sure we don't forget the underrun check.
> 
> While I think it is a good change, it is independent of verify-pack, no?

I could have limited has_extension() to verify-pack.c initially and then
sent a conversion for the other files after this series had been
accepted, yes.  It's one of the main cleanups for verify_one_pack(),
though, because it allowed me to drop some comments -- without losing
readability (IMHO).

I could have documented it broader scope better in the commit message. :-/

Thanks,
René

Re: [PATCH 1/9] Add has_extension()

[Fredrik Kuivinen]

On Thu, Aug 10, 2006 at 05:02:30PM +0200, Rene Scharfe wrote:
> +static inline int has_extension(const char *filename, int len, const char *ext)
> +{
> +	int extlen = strlen(ext);
> +	return len > extlen && !memcmp(filename + len - extlen, ext, extlen);
> +}
> +

Wouldn't this function be much easier to use if len is computed from
filename with strlen? (after a quick look through the other patches I
couldn't find a call site where filename wasn't NUL-terminated)

- Fredrik

Re: [PATCH 1/9] Add has_extension()

[Rene Scharfe]

Fredrik Kuivinen schrieb:
> On Thu, Aug 10, 2006 at 05:02:30PM +0200, Rene Scharfe wrote:
>> +static inline int has_extension(const char *filename, int len,
>> const char *ext) +{ +	int extlen = strlen(ext); +	return len >
>> extlen && !memcmp(filename + len - extlen, ext, extlen); +} +
> 
> Wouldn't this function be much easier to use if len is computed from 
> filename with strlen? (after a quick look through the other patches I
>  couldn't find a call site where filename wasn't NUL-terminated)

Yes, it would be a bit easier, and my first version had only two
arguments.  Then I found out that the length of the first string is
already known at _all_ potential callsites, using this command to
identify candidates:

	$ grep 'cmp.*"\..*"' *.[ch]

We could add something like this:

	#define has_ext(a, b) has_extension(a, strlen(a), b)

to make it easier to use for code that doesn't already determine the
string length.  I think we should add it only after a user has been
identified, though.

Thanks,
René

Re: [PATCH 1/9] Add has_extension()

[Fredrik Kuivinen]

On Thu, Aug 10, 2006 at 08:42:09PM +0200, Rene Scharfe wrote:
> Fredrik Kuivinen schrieb:
> > On Thu, Aug 10, 2006 at 05:02:30PM +0200, Rene Scharfe wrote:
> >> +static inline int has_extension(const char *filename, int len,
> >> const char *ext) +{ +	int extlen = strlen(ext); +	return len >
> >> extlen && !memcmp(filename + len - extlen, ext, extlen); +} +
> > 
> > Wouldn't this function be much easier to use if len is computed from 
> > filename with strlen? (after a quick look through the other patches I
> >  couldn't find a call site where filename wasn't NUL-terminated)
> 
> Yes, it would be a bit easier, and my first version had only two
> arguments.  Then I found out that the length of the first string is
> already known at _all_ potential callsites, using this command to
> identify candidates:
> 
> 	$ grep 'cmp.*"\..*"' *.[ch]
> 
> We could add something like this:
> 
> 	#define has_ext(a, b) has_extension(a, strlen(a), b)
> 
> to make it easier to use for code that doesn't already determine the
> string length.  I think we should add it only after a user has been
> identified, though.
> 

IMHO the small speed-up isn't worth it. Drop the extra argument and
avoid a possible future bug.

- Fredrik