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

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:

quoted

quoted

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

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.