# [PATCH 7/9] git-verify-pack: buffer overrun paranoia 18 messages from 2006-08-10 to 2006-08-10. Participants: Rene Scharfe, Johannes Schindelin, Fredrik Kuivinen. Thread: https://gitlist.dev/t/5198 ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 0/9] git-verify-pack: clean up and make builtin Message-ID: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221582769-git-send-email-rene.scharfe%40lsrfire.ath.cx ``` 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, 2006-08-10 15:02 Subject: [PATCH 1/9] Add has_extension() Message-ID: <11552221581171-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221581171-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` 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 --- 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 ``` ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 2/9] git-verify-pack: show usage when no pack was specified Message-ID: <11552221581434-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221581434-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` Signed-off-by: Rene Scharfe --- 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 ``` ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 3/9] git-verify-pack: more careful path handling Message-ID: <11552221581834-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221581834-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` 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 --- 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 ``` ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 4/9] git-verify-pack: insist on .idx extension Message-ID: <11552221583172-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221583172-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` 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 --- 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); ``` ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 5/9] git-verify-pack: get rid of while loop Message-ID: <11552221581038-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221581038-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` 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 --- 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); } ``` ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 6/9] git-verify-pack: free pack after use and a cleanup Message-ID: <11552221582064-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221582064-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` 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 --- 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] ..."; -- 1.4.2.rc2.g822a ``` ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 7/9] git-verify-pack: buffer overrun paranoia Message-ID: <11552221583008-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221583008-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` Signed-off-by: Rene Scharfe --- 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 ``` ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 8/9] git-verify-pack: no need to count errors Message-ID: <11552221583276-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221583276-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` Signed-off-by: Rene Scharfe --- 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 ``` ## Rene Scharfe, 2006-08-10 15:02 Subject: [PATCH 9/9] git-verify-pack: make builtin Message-ID: <11552221581436-git-send-email-rene.scharfe@lsrfire.ath.cx> URL: https://gitlist.dev/e/11552221581436-git-send-email-rene.scharfe%40lsrfire.ath.cx In-Reply-To: <11552221582769-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` Convert git-verify-pack to a builtin command. Also rename ac to argc and av to argv for consistancy. Signed-off-by: Rene Scharfe --- 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] ..."; + +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] ..."; - -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, 2006-08-10 16:23 Subject: Re: [PATCH 8/9] git-verify-pack: no need to count errors Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608101822550.10541%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <11552221583276-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` Hi, there was no harm done counting the errors, i.e. no need to "fix" that. Ciao, Dscho ``` ## Johannes Schindelin, 2006-08-10 16:24 Subject: Re: [PATCH 9/9] git-verify-pack: make builtin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608101823500.10541%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <11552221581436-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` 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 ``` ## Johannes Schindelin, 2006-08-10 16:25 Subject: Re: [PATCH 1/9] Add has_extension() Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0608101825000.10541%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <11552221581171-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` 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, 2006-08-10 17:37 Subject: Re: [PATCH 8/9] git-verify-pack: no need to count errors Message-ID: <44DB6ECE.10706@lsrfire.ath.cx> URL: https://gitlist.dev/e/44DB6ECE.10706%40lsrfire.ath.cx In-Reply-To: ``` 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, 2006-08-10 17:48 Subject: Re: [PATCH 1/9] Add has_extension() Message-ID: <44DB7178.4090706@lsrfire.ath.cx> URL: https://gitlist.dev/e/44DB7178.4090706%40lsrfire.ath.cx In-Reply-To: ``` 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é ``` ## Fredrik Kuivinen, 2006-08-10 18:21 Subject: Re: [PATCH 1/9] Add has_extension() Message-ID: <20060810182117.GA13446@c165.ib.student.liu.se> URL: https://gitlist.dev/e/20060810182117.GA13446%40c165.ib.student.liu.se In-Reply-To: <11552221581171-git-send-email-rene.scharfe@lsrfire.ath.cx> ``` 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 ``` ## Rene Scharfe, 2006-08-10 18:42 Subject: Re: [PATCH 1/9] Add has_extension() Message-ID: <44DB7E01.4000901@lsrfire.ath.cx> URL: https://gitlist.dev/e/44DB7E01.4000901%40lsrfire.ath.cx In-Reply-To: <20060810182117.GA13446@c165.ib.student.liu.se> ``` 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é ``` ## Fredrik Kuivinen, 2006-08-10 21:47 Subject: Re: [PATCH 1/9] Add has_extension() Message-ID: <20060810214718.GD13446@c165.ib.student.liu.se> URL: https://gitlist.dev/e/20060810214718.GD13446%40c165.ib.student.liu.se In-Reply-To: <44DB7E01.4000901@lsrfire.ath.cx> ``` 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 ```