Re: [PATCH 29/67] use strip_suffix and xstrfmt to replace suffix

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Sep 15, 2015 at 11:47 AM, Jeff King <peff@peff.net> 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@peff.net>
> ---
> 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