threads / patch / 5198

patch, 9 partsgit-verify-pack: buffer overrun paranoia

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

## tl;dr

18 messages between Aug 10, 2006 and Aug 10, 2006. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Rene Scharfe· Aug 10, 2006, 15:02 UTC · lore

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

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

Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore

[PATCH 1/9] Add has_extension()

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(-)
Show changes to 8 files +13 −7

git-compat-util.h, help.c, http-fetch.c, index-pack.c, local-fetch.c, refs.c, sha1_file.c, verify-pack.c

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
Johannes Schindelin· Aug 10, 2006, 16:25 UTC · re: Rene Scharfe · lore

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

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

Rene Scharfe· Aug 10, 2006, 17:48 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 6 quoted lines
> 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é

Fredrik Kuivinen· Aug 10, 2006, 18:21 UTC · re: Rene Scharfe · lore

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

On Thu, Aug 10, 2006 at 05:02:30PM +0200, Rene Scharfe wrote:
Show 6 quoted lines
> +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
Rene Scharfe· Aug 10, 2006, 18:42 UTC · re: Fredrik Kuivinen · lore

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

Fredrik Kuivinen schrieb:
Show 8 quoted lines
> 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é

Fredrik Kuivinen· Aug 10, 2006, 21:47 UTC · re: Rene Scharfe · lore

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

On Thu, Aug 10, 2006 at 08:42:09PM +0200, Rene Scharfe wrote:
Show 25 quoted lines
> 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
Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore

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

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Show changes to verify-pack.c +6 −0
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
Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore

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

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(-)
Show changes to verify-pack.c +8 −6
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
Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore

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

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
Show changes to verify-pack.c +5 −6
--- 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);
Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore

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

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
Show changes to verify-pack.c +16 −16
--- 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);
 }
 
Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore

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

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(-)
Show changes to verify-pack.c +8 −3
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
Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
Show changes to verify-pack.c +9 −0
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
Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore

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

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 verify-pack.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to verify-pack.c +3 −3
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
Rene Scharfe· Aug 10, 2006, 17:37 UTC · re: Johannes Schindelin · lore

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

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é

Rene Scharfe· Aug 10, 2006, 15:02 UTC · re: Rene Scharfe · lore

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

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(-)
Show changes to 5 files +82 −78

Makefile, builtin-verify-pack.c, builtin.h, git.c, verify-pack.c

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
Johannes Schindelin· Aug 10, 2006, 16:24 UTC · re: Rene Scharfe · lore

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

Hi,
On Thu, 10 Aug 2006, Rene Scharfe wrote:
Show 6 quoted lines
>  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

← back to recent threads