Re: [PATCH 1/2] strbuf: add xstrdup_fmt helper

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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