Re: [PATCH 1/2] strbuf: add xstrdup_fmt helper
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 18, 2014, 22:32 UTC
- Message-ID
- <xmqq7g4dj1cn.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20140618200133.GA23057@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 23 quoted lines
> You can use a strbuf to build up a string from parts, and > then detach it. In the general case, you might use multiple > strbuf_add* functions to do the building. However, in many > cases, a single strbuf_addf is sufficient, and we end up > with: > > struct strbuf buf = STRBUF_INIT; > ... > strbuf_addf(&buf, fmt, some, args); > str = strbuf_detach(&buf, NULL); > > We can make this much more readable (and avoid introducing > an extra variable, which can clutter the code) by > introducing a convenience function: > > str = xstrdup_fmt(fmt, some, args); > > Signed-off-by: Jeff King <peff@peff.net> > --- > I'm open to suggestions on the name. This really is the same thing > conceptually as the GNU asprintf(), but the interface is different (that > function takes a pointer-to-pointer as an out-parameter, and returns the > number of characters return).
Naming it with anything "dup" certainly feels wrong. The returned string is not a duplicate of anything.
To me, the function feels like an "sprintf done right"; as you said, the best name for "printf-like format into an allocated piece of memory" is unfortunately taken as asprintf(3).
I wonder if our callers can instead use asprintf(3) with its slightly more quirky API (and then we supply compat/asprintf.c for non-GNU platforms). Right now we only have three call sites, but if we anticipate that "printf-like format into an allocated piece of memory" will prove be generally useful in our code base, following an API that other people already have established may give our developers one less thing that they have to learn.
As usual, I would expect we would have xasprintf wrapper around it to die instead of returning -1 upon allocation failure.
The call sites do not look too bad (see below) if we were to go that route instead.
remote.c | 2 +- unpack-trees.c | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/remote.c b/remote.c index b46f467..87fa7ec 100644 --- a/remote.c +++ b/remote.c @@ -185,7 +185,7 @@ static struct branch *make_branch(const char *name, int len) ret->name = xstrndup(name, len); else ret->name = xstrdup(name); - ret->refname = xstrdup_fmt("refs/heads/%s", ret->name); + asprintf(&ret->refname, "refs/heads/%s", ret->name); return ret; } diff --git a/unpack-trees.c b/unpack-trees.c index dd1e06e..d6a07b8 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -63,8 +63,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, "Please, commit your changes or stash them before you can %s."; else msg = "Your local changes to the following files would be overwritten by %s:\n%%s"; - msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] = - xstrdup_fmt(msg, cmd, cmd2); + xasprintf(&msgs[ERROR_WOULD_OVERWRITE], msg, cmd, cmd2); + msgs[ERROR_NOT_UPTODATE_FILE] = msgs[ERROR_WOULD_OVERWRITE]; msgs[ERROR_NOT_UPTODATE_DIR] = "Updating the following directories would lose untracked files in it:\n%s"; @@ -75,8 +75,10 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, else msg = "The following untracked working tree files would be %s by %s:\n%%s"; - msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrdup_fmt(msg, "removed", cmd, cmd2); - msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrdup_fmt(msg, "overwritten", cmd, cmd2); + xasprintf(&msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED], + msg, "removed", cmd, cmd2); + xasprintf(&msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN], + msg, "overwritten", cmd, cmd2); /* * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we