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

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

[Jeff King]

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 
---
 http.c|  7 ---
 pack-bitmap.c | 13 -
 sha1_file.c   |  6 --
 3 files changed, 12 insertions(+), 14 deletions(-)

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