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.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html