On Tue, Sep 15, 2015 at 11:47 AM, Jeff King <peff@xxxxxxxx> wrote:
> When we want to convert "foo.pack" to "foo.idx", we do it by
> duplicating the original string and then munging the bytes
> in place. Let's use strip_suffix and xstrfmt instead, which
> has several advantages:
>
> 1. It's more clear what the intent is.
>
> 2. It does not implicitly rely on the fact that
> strlen(".idx") <= strlen(".pack") to avoid an overflow.
>
> 3. We communicate the assumption that the input file ends
> with ".pack" (and get a run-time check that this is so).
>
> 4. We drop calls to strcpy, which makes auditing the code
> base easier.
>
> Likewise, we can do this to convert ".pack" to ".bitmap",
> avoiding some manual memory computation.
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> diff --git a/http.c b/http.c
> index 7b02259..e0ff876 100644
> --- a/http.c
> +++ b/http.c
> @@ -1511,6 +1511,7 @@ int finish_http_pack_request(struct http_pack_request *preq)
> struct packed_git **lst;
> struct packed_git *p = preq->target;
> char *tmp_idx;
> + size_t len;
> struct child_process ip = CHILD_PROCESS_INIT;
> const char *ip_argv[8];
>
> @@ -1524,9 +1525,9 @@ int finish_http_pack_request(struct http_pack_request *preq)
> lst = &((*lst)->next);
> *lst = (*lst)->next;
>
> - tmp_idx = xstrdup(preq->tmpfile);
> - strcpy(tmp_idx + strlen(tmp_idx) - strlen(".pack.temp"),
> - ".idx.temp");
> + if (!strip_suffix(preq->tmpfile, ".pack.temp", &len))
> + die("BUG: pack tmpfile does not end in .pack.temp?");
> + tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile);
These instances of repeated replacement code may argue in favor of a
general purpose replace_suffix() function:
char *replace_suffix(const char *s, const char *old, const char *new)
{
size_t n;
if (!strip_suffix(s, old, &n))
die("BUG: '%s' does not end with '%s', s, old);
return xstrfmt("%.*s%s", (int)n, s, new);
}
or something.
> ip_argv[0] = "index-pack";
> ip_argv[1] = "-o";
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 637770a..7dfcb34 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -252,16 +252,11 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
>
> static char *pack_bitmap_filename(struct packed_git *p)
> {
> - char *idx_name;
> - int len;
> -
> - len = strlen(p->pack_name) - strlen(".pack");
> - idx_name = xmalloc(len + strlen(".bitmap") + 1);
> -
> - memcpy(idx_name, p->pack_name, len);
> - memcpy(idx_name + len, ".bitmap", strlen(".bitmap") + 1);
> + size_t len;
>
> - return idx_name;
> + if (!strip_suffix(p->pack_name, ".pack", &len))
> + die("BUG: pack_name does not end in .pack");
> + return xstrfmt("%.*s.bitmap", (int)len, p->pack_name);
> }
>
> static int open_pack_bitmap_1(struct packed_git *packfile)
> diff --git a/sha1_file.c b/sha1_file.c
> index 28352a5..88996f0 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -671,13 +671,15 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)
> int open_pack_index(struct packed_git *p)
> {
> char *idx_name;
> + size_t len;
> int ret;
>
> if (p->index_data)
> return 0;
>
> - idx_name = xstrdup(p->pack_name);
> - strcpy(idx_name + strlen(idx_name) - strlen(".pack"), ".idx");
> + if (!strip_suffix(p->pack_name, ".pack", &len))
> + die("BUG: pack_name does not end in .pack");
> + idx_name = xstrfmt("%.*s.idx", (int)len, p->pack_name);
> ret = check_packed_git_idx(idx_name, p);
> free(idx_name);
> return ret;
> --
> 2.6.0.rc2.408.ga2926b9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html