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

[Jeff King]

On Wed, Sep 16, 2015 at 12:38:12AM -0400, Eric Sunshine wrote:

> > @@ -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", ))
> > +   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, ))
> die("BUG: '%s' does not end with '%s', s, old);
> return xstrfmt("%.*s%s", (int)n, s, new);
> }
> 
> or something.

Yeah, that is tempting, but I think the "die" here is not at all
appropriate in a reusable function. I'd probably write it as:

  char *replace_suffix(const char *s, const char *old, const char *new)
  {
size_t n;
if (!strip_suffix(s, old, ))
return NULL;
return xstrfmt("%.*s%s", (int)n, s, new);
  }

and do:

  tmp_idx = replace_suffix(preq->tmpfile, ".pack.temp", ".idx.temp");
  if (!tmp_idx)
die("BUG: pack tmpfile does not end in .pack.temp?");

but then we are not really saving much. And it is not clear whether
that is even a sane output for replace_suffix. I can easily imagine
three behaviors when we do not end in the original suffix:

  - return NULL to signal error

  - return the original with no replacement

  - return the original with "new" appended

So I'm not sure it makes a good reusable function beyond these three
call-sites.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Wed, Sep 16, 2015 at 06:50:38AM -0400, Jeff King wrote:
> On Wed, Sep 16, 2015 at 12:38:12AM -0400, Eric Sunshine wrote:
> > > @@ -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", ))
> > > +   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, ))
> > die("BUG: '%s' does not end with '%s', s, old);
> > return xstrfmt("%.*s%s", (int)n, s, new);
> > }
> > 
> > or something.
> 
> Yeah, that is tempting, but I think the "die" here is not at all
> appropriate in a reusable function. I'd probably write it as:
> 
>   char *replace_suffix(const char *s, const char *old, const char *new)
>   {
>   size_t n;
>   if (!strip_suffix(s, old, ))
>   return NULL;
>   return xstrfmt("%.*s%s", (int)n, s, new);
>   }
> 
> and do:
> 
>   tmp_idx = replace_suffix(preq->tmpfile, ".pack.temp", ".idx.temp");
>   if (!tmp_idx)
>   die("BUG: pack tmpfile does not end in .pack.temp?");
> 
> but then we are not really saving much. And it is not clear whether
> that is even a sane output for replace_suffix. I can easily imagine
> three behaviors when we do not end in the original suffix:
> 
>   - return NULL to signal error
> 
>   - return the original with no replacement
> 
>   - return the original with "new" appended
> 
> So I'm not sure it makes a good reusable function beyond these three
> call-sites.

Indeed. I had a "gently" version in mind to satisfy callers not
interested in dying, but it's so little code that it likely isn't
worth it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Tue, Sep 15, 2015 at 11:47 AM, Jeff King  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 
> ---
> 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", ))
> +   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, ))
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", ))
> +   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", ))
> +   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html