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

From: Jeff King <peff@peff.net>
Date: Sep 16, 2015, 10:50 UTC
Message-ID: <20150916105037.GI13966@sigill.intra.peff.net>
In-Reply-To: <CAPig+cRefYKBOLJMsu+yAp7D=GYh4Pr5eXEeHXR0b-4hhOfxPQ@mail.gmail.com>

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

Show 23 quoted lines

> > @@ -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.

-Peff