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

> 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", &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.
> 
> 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, &n))
> 		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.