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