[PATCH 0/2] dropping manual malloc calculations

[PATCH 0/2] dropping manual malloc calculations

[Jeff King]

While working on the skip_prefix series, I ended up grepping for:

  + strlen("

to find spots in need of skip_prefix. Of course, it turns up many other
nasty ad-hoc calculations. Here's a short series that addresses a few.
There are many more, but hopefully the first patch provides a tool that
can help us in the future.

  [1/2]: strbuf: add xstrdup_fmt helper
  [2/2]: use xstrdup_fmt in favor of manual size calculations

-Peff

[PATCH 1/2] strbuf: add xstrdup_fmt helper

[Jeff King]

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

 strbuf.c | 19 +++++++++++++++++++
 strbuf.h |  9 +++++++++
 2 files changed, 28 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index ac62982..6674d74 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -600,3 +600,22 @@ char *xstrdup_tolower(const char *string)
 	result[i] = '\0';
 	return result;
 }
+
+char *xstrdup_vfmt(const char *fmt, va_list ap)
+{
+	struct strbuf buf = STRBUF_INIT;
+	strbuf_vaddf(&buf, fmt, ap);
+	return strbuf_detach(&buf, NULL);
+}
+
+char *xstrdup_fmt(const char *fmt, ...)
+{
+	va_list ap;
+	char *ret;
+
+	va_start(ap, fmt);
+	ret = xstrdup_vfmt(fmt, ap);
+	va_end(ap);
+
+	return ret;
+}
diff --git a/strbuf.h b/strbuf.h
index e9ad03e..61818f9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -187,4 +187,13 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);
 
 char *xstrdup_tolower(const char *);
 
+/*
+ * Create a newly allocated string using printf format. You can do this easily
+ * with a strbuf, but this provides a shortcut to save a few lines.
+ */
+__attribute__((format (printf, 1, 0)))
+char *xstrdup_vfmt(const char *fmt, va_list ap);
+__attribute__((format (printf, 1, 2)))
+char *xstrdup_fmt(const char *fmt, ...);
+
 #endif /* STRBUF_H */
-- 
2.0.0.566.gfe3e6b2

[PATCH 2/2] use xstrdup_fmt in favor of manual size calculations

[Jeff King]

In many parts of the code, we do an ugly and error-prone
malloc like:

  const char *fmt = "something %s";
  buf = xmalloc(strlen(foo) + 10 + 1);
  sprintf(buf, fmt, foo);

This makes the code brittle, and if we ever get the
allocation wrong, is a potential heap overflow. Let's
instead favor xstrdup_fmt, which handles the allocation
automatically, and makes the code shorter and more readable.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c       |  6 +-----
 unpack-trees.c | 17 ++++++-----------
 2 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/remote.c b/remote.c
index 0e9459c..792dcee 100644
--- a/remote.c
+++ b/remote.c
@@ -170,7 +170,6 @@ static struct branch *make_branch(const char *name, int len)
 {
 	struct branch *ret;
 	int i;
-	char *refname;
 
 	for (i = 0; i < branches_nr; i++) {
 		if (len ? (!strncmp(name, branches[i]->name, len) &&
@@ -186,10 +185,7 @@ static struct branch *make_branch(const char *name, int len)
 		ret->name = xstrndup(name, len);
 	else
 		ret->name = xstrdup(name);
-	refname = xmalloc(strlen(name) + strlen("refs/heads/") + 1);
-	strcpy(refname, "refs/heads/");
-	strcpy(refname + strlen("refs/heads/"), ret->name);
-	ret->refname = refname;
+	ret->refname = xstrdup_fmt("refs/heads/%s", ret->name);
 
 	return ret;
 }
diff --git a/unpack-trees.c b/unpack-trees.c
index 97fc995..dd1e06e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -56,17 +56,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 	int i;
 	const char **msgs = opts->msgs;
 	const char *msg;
-	char *tmp;
 	const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches";
+
 	if (advice_commit_before_merge)
 		msg = "Your local changes to the following files would be overwritten by %s:\n%%s"
 			"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";
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);
-	sprintf(tmp, msg, cmd, cmd2);
-	msgs[ERROR_WOULD_OVERWRITE] = tmp;
-	msgs[ERROR_NOT_UPTODATE_FILE] = tmp;
+	msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
+		xstrdup_fmt(msg, cmd, cmd2);
 
 	msgs[ERROR_NOT_UPTODATE_DIR] =
 		"Updating the following directories would lose untracked files in it:\n%s";
@@ -76,12 +74,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 			"Please move or remove them before you can %s.";
 	else
 		msg = "The following untracked working tree files would be %s by %s:\n%%s";
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") + strlen(cmd2) - 4);
-	sprintf(tmp, msg, "removed", cmd, cmd2);
-	msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") + strlen(cmd2) - 4);
-	sprintf(tmp, msg, "overwritten", cmd, cmd2);
-	msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;
+
+	msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrdup_fmt(msg, "removed", cmd, cmd2);
+	msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrdup_fmt(msg, "overwritten", cmd, cmd2);
 
 	/*
 	 * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
-- 
2.0.0.566.gfe3e6b2

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

[Junio C Hamano]

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

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

[Jeff King]

On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote:

> >   str = xstrdup_fmt(fmt, some, args);
> > ---
> > 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.

I was somewhat inspired by "mkpathdup" and friends. It is quite simialr
to mkpathdup, except that it is not limited to paths (and does not do
any path-specific munging). I agree something with "printf" in the name
is probably more descriptive, though.

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

I considered that, but I do find asprintf's interface unnecessarily
annoying (you can't return a value directly, and you run afoul of const
issues when passing pointers to pointers). As you noted, it's not _too_
bad, but we really get nothing from the integer return type. AFAICT, it
helps with:

  1. You know how many characters are in the string. If you cared about
     that here, you would just use a strbuf directly, which is much more
     flexible.

  2. The error handling is different. But our x-variant would just die()
     on errors anyway, so we do not care.

So modeling after asprintf feels like carrying around unnecessary
baggage (and I am not convinced asprintf is in wide enough use, nor that
the function is complicated enough to care about developer familiarity).
Part of me is tempted to call it xasprintf anyway, and use our own
interface. GNU does not get to squat on that useful name. ;)

That is probably unnecessarily confusing, though.

> As usual, I would expect we would have xasprintf wrapper around it
> to die instead of returning -1 upon allocation failure.

Would it be crazy to just call it xprintf? By our current scheme that
would be "a wrapper for printf", which means it is potentially
confusing.

Literally any other unused letter would be fine. dprintf for detach? I
dunno.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote:
>
>> >   str = xstrdup_fmt(fmt, some, args);
>> > ---
>> > 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.
>
> I was somewhat inspired by "mkpathdup" and friends.

That name is from "mkpath gives its result in a static buffer and
forces the callers to xstrdup() if they want to keep the result;
this is a thin wrapper to do so for the caller".  As there is no
str_fmt() that gives its result in a static, so...

> I considered that, but I do find asprintf's interface unnecessarily
> annoying...

Yes, it is annoying.

> Would it be crazy to just call it xprintf? By our current scheme that
> would be "a wrapper for printf", which means it is potentially
> confusing.
>
> Literally any other unused letter would be fine. dprintf for detach? I
> dunno.

If we twist the logic behind the 'mkpathdup' name a little bit,
perhaps we can call it sprintf_dup or something.  That is, "sprintf
wants a fixed preallocated piece of memory to print into, and we
relieve the callers of having to do so", perhaps?

dprintf, mprintf, etc. are also fine by me.

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

[René Scharfe]

Am 19.06.2014 11:05, schrieb Jeff King:
> On Wed, Jun 18, 2014 at 03:32:08PM -0700, Junio C Hamano wrote:
>
>>>    str = xstrdup_fmt(fmt, some, args);
>>> ---
>>> 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.
>
> I was somewhat inspired by "mkpathdup" and friends. It is quite simialr
> to mkpathdup, except that it is not limited to paths (and does not do
> any path-specific munging). I agree something with "printf" in the name
> is probably more descriptive, though.
>
>> 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.
>
> I considered that, but I do find asprintf's interface unnecessarily
> annoying (you can't return a value directly, and you run afoul of const
> issues when passing pointers to pointers). As you noted, it's not _too_
> bad, but we really get nothing from the integer return type. AFAICT, it
> helps with:
>
>    1. You know how many characters are in the string. If you cared about
>       that here, you would just use a strbuf directly, which is much more
>       flexible.
>
>    2. The error handling is different. But our x-variant would just die()
>       on errors anyway, so we do not care.
>
> So modeling after asprintf feels like carrying around unnecessary
> baggage (and I am not convinced asprintf is in wide enough use, nor that
> the function is complicated enough to care about developer familiarity).
> Part of me is tempted to call it xasprintf anyway, and use our own
> interface. GNU does not get to squat on that useful name. ;)
>
> That is probably unnecessarily confusing, though.
>
>> As usual, I would expect we would have xasprintf wrapper around it
>> to die instead of returning -1 upon allocation failure.
>
> Would it be crazy to just call it xprintf? By our current scheme that
> would be "a wrapper for printf", which means it is potentially
> confusing.
>
> Literally any other unused letter would be fine. dprintf for detach? I
> dunno.

I agree that "dup" doesn't fit and that potential callers don't need the 
length of the generated string (or should use strbuf otherwise).

Including "print" in the name is not necessary, though -- die(), error() 
and friends work fine without them.

What about simply removing the "dup_" part and using xstrfmt?

René

[PATCH v2] dropping manual malloc calculations

[Jeff King]

On Thu, Jun 19, 2014 at 09:49:41AM -0700, Junio C Hamano wrote:

> If we twist the logic behind the 'mkpathdup' name a little bit,
> perhaps we can call it sprintf_dup or something.  That is, "sprintf
> wants a fixed preallocated piece of memory to print into, and we
> relieve the callers of having to do so", perhaps?

Right, that is the "dup" I was thinking of. Anyway, I think René's
"xstrfmt" is my favorite of the suggestions, and you seemed to like it,
too. Here is a re-roll using that name.

The first two patches are the same except for the name change. The rest
are more applications of it (and other techniques) found by grepping for
"malloc.*strlen". The diffstat is very encouraging, and I think the
results are much more readable (in addition to being more obviously
correct). The last two also fix real "bugs", but I doubt either is a
problem in practice.

    builtin/apply.c         |  4 +--
    builtin/fetch.c         |  9 ++----
    builtin/fmt-merge-msg.c |  7 ++---
    builtin/name-rev.c      |  5 +---
    builtin/receive-pack.c  |  5 +---
    builtin/show-branch.c   | 10 +++----
    environment.c           | 12 +++-----
    http-push.c             | 24 +++++-----------
    http-walker.c           |  6 ++--
    match-trees.c           |  9 ++----
    merge-recursive.c       | 53 ++++++++++++++++++++---------------
    merge.c                 | 42 +++++++++------------------
    remote.c                |  6 +---
    sequencer.c             |  9 ++----
    sha1_name.c             |  5 +---
    shell.c                 |  6 +---
    strbuf.c                | 19 +++++++++++++
    strbuf.h                |  9 ++++++
    unpack-trees.c          | 17 ++++-------
    walker.c                | 18 ++++++------
    20 files changed, 117 insertions(+), 158 deletions(-)

  [01/10]: strbuf: add xstrfmt helper
  [02/10]: use xstrfmt in favor of manual size calculations
  [03/10]: use xstrdup instead of xmalloc + strcpy
  [04/10]: use xstrfmt to replace xmalloc + sprintf
  [05/10]: use xstrfmt to replace xmalloc + strcpy/strcat
  [06/10]: setup_git_env: use git_pathdup instead of xmalloc + sprintf
  [07/10]: sequencer: use argv_array_pushf
  [08/10]: merge: use argv_array when spawning merge strategy
  [09/10]: walker_fetch: fix minor memory leak
  [10/10]: unique_path: fix unlikely heap overflow

[PATCH v2 01/10] strbuf: add xstrfmt helper

[Jeff King]

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 = xstrfmt(fmt, some, args);

Signed-off-by: Jeff King <peff@peff.net>
---
 strbuf.c | 19 +++++++++++++++++++
 strbuf.h |  9 +++++++++
 2 files changed, 28 insertions(+)

diff --git a/strbuf.c b/strbuf.c
index ac62982..12c7865 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -600,3 +600,22 @@ char *xstrdup_tolower(const char *string)
 	result[i] = '\0';
 	return result;
 }
+
+char *xstrvfmt(const char *fmt, va_list ap)
+{
+	struct strbuf buf = STRBUF_INIT;
+	strbuf_vaddf(&buf, fmt, ap);
+	return strbuf_detach(&buf, NULL);
+}
+
+char *xstrfmt(const char *fmt, ...)
+{
+	va_list ap;
+	char *ret;
+
+	va_start(ap, fmt);
+	ret = xstrvfmt(fmt, ap);
+	va_end(ap);
+
+	return ret;
+}
diff --git a/strbuf.h b/strbuf.h
index e9ad03e..a594c24 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -187,4 +187,13 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);
 
 char *xstrdup_tolower(const char *);
 
+/*
+ * Create a newly allocated string using printf format. You can do this easily
+ * with a strbuf, but this provides a shortcut to save a few lines.
+ */
+__attribute__((format (printf, 1, 0)))
+char *xstrvfmt(const char *fmt, va_list ap);
+__attribute__((format (printf, 1, 2)))
+char *xstrfmt(const char *fmt, ...);
+
 #endif /* STRBUF_H */
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 02/10] use xstrfmt in favor of manual size calculations

[Jeff King]

In many parts of the code, we do an ugly and error-prone
malloc like:

  const char *fmt = "something %s";
  buf = xmalloc(strlen(foo) + 10 + 1);
  sprintf(buf, fmt, foo);

This makes the code brittle, and if we ever get the
allocation wrong, is a potential heap overflow. Let's
instead favor xstrfmt, which handles the allocation
automatically, and makes the code shorter and more readable.

Signed-off-by: Jeff King <peff@peff.net>
---
These could actually be squashed into later commits, I suppose, but I
left it separate since it had already been reviewed.

 remote.c       |  6 +-----
 unpack-trees.c | 17 ++++++-----------
 2 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/remote.c b/remote.c
index 0e9459c..bf27e44 100644
--- a/remote.c
+++ b/remote.c
@@ -170,7 +170,6 @@ static struct branch *make_branch(const char *name, int len)
 {
 	struct branch *ret;
 	int i;
-	char *refname;
 
 	for (i = 0; i < branches_nr; i++) {
 		if (len ? (!strncmp(name, branches[i]->name, len) &&
@@ -186,10 +185,7 @@ static struct branch *make_branch(const char *name, int len)
 		ret->name = xstrndup(name, len);
 	else
 		ret->name = xstrdup(name);
-	refname = xmalloc(strlen(name) + strlen("refs/heads/") + 1);
-	strcpy(refname, "refs/heads/");
-	strcpy(refname + strlen("refs/heads/"), ret->name);
-	ret->refname = refname;
+	ret->refname = xstrfmt("refs/heads/%s", ret->name);
 
 	return ret;
 }
diff --git a/unpack-trees.c b/unpack-trees.c
index 97fc995..c237370 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -56,17 +56,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 	int i;
 	const char **msgs = opts->msgs;
 	const char *msg;
-	char *tmp;
 	const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches";
+
 	if (advice_commit_before_merge)
 		msg = "Your local changes to the following files would be overwritten by %s:\n%%s"
 			"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";
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);
-	sprintf(tmp, msg, cmd, cmd2);
-	msgs[ERROR_WOULD_OVERWRITE] = tmp;
-	msgs[ERROR_NOT_UPTODATE_FILE] = tmp;
+	msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
+		xstrfmt(msg, cmd, cmd2);
 
 	msgs[ERROR_NOT_UPTODATE_DIR] =
 		"Updating the following directories would lose untracked files in it:\n%s";
@@ -76,12 +74,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 			"Please move or remove them before you can %s.";
 	else
 		msg = "The following untracked working tree files would be %s by %s:\n%%s";
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") + strlen(cmd2) - 4);
-	sprintf(tmp, msg, "removed", cmd, cmd2);
-	msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;
-	tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") + strlen(cmd2) - 4);
-	sprintf(tmp, msg, "overwritten", cmd, cmd2);
-	msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;
+
+	msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrfmt(msg, "removed", cmd, cmd2);
+	msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrfmt(msg, "overwritten", cmd, cmd2);
 
 	/*
 	 * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 03/10] use xstrdup instead of xmalloc + strcpy

[Jeff King]

This is one line shorter, and makes sure the length in the
malloc and copy steps match.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c | 5 +----
 http-push.c            | 6 ++----
 http-walker.c          | 3 +--
 3 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..18458e8 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -614,12 +614,9 @@ static void run_update_post_hook(struct command *commands)
 	argv[0] = hook;
 
 	for (argc = 1, cmd = commands; cmd; cmd = cmd->next) {
-		char *p;
 		if (cmd->error_string || cmd->did_not_exist)
 			continue;
-		p = xmalloc(strlen(cmd->ref_name) + 1);
-		strcpy(p, cmd->ref_name);
-		argv[argc] = p;
+		argv[argc] = xstrdup(cmd->ref_name);
 		argc++;
 	}
 	argv[argc] = NULL;
diff --git a/http-push.c b/http-push.c
index de00d16..95650a0 100644
--- a/http-push.c
+++ b/http-push.c
@@ -767,15 +767,13 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed)
 
 	if (tag_closed && ctx->cdata) {
 		if (!strcmp(ctx->name, DAV_ACTIVELOCK_OWNER)) {
-			lock->owner = xmalloc(strlen(ctx->cdata) + 1);
-			strcpy(lock->owner, ctx->cdata);
+			lock->owner = xstrdup(ctx->cdata);
 		} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TIMEOUT)) {
 			if (starts_with(ctx->cdata, "Second-"))
 				lock->timeout =
 					strtol(ctx->cdata + 7, NULL, 10);
 		} else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) {
-			lock->token = xmalloc(strlen(ctx->cdata) + 1);
-			strcpy(lock->token, ctx->cdata);
+			lock->token = xstrdup(ctx->cdata);
 
 			git_SHA1_Init(&sha_ctx);
 			git_SHA1_Update(&sha_ctx, lock->token, strlen(lock->token));
diff --git a/http-walker.c b/http-walker.c
index 1516c5e..ab6a4fe 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -566,8 +566,7 @@ struct walker *get_http_walker(const char *url)
 	struct walker *walker = xmalloc(sizeof(struct walker));
 
 	data->alt = xmalloc(sizeof(*data->alt));
-	data->alt->base = xmalloc(strlen(url) + 1);
-	strcpy(data->alt->base, url);
+	data->alt->base = xstrdup(url);
 	for (s = data->alt->base + strlen(data->alt->base) - 1; *s == '/'; --s)
 		*s = 0;
 
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 04/10] use xstrfmt to replace xmalloc + sprintf

[Jeff King]

This is one line shorter, and makes sure the length in the
malloc and sprintf steps match.

These conversions are very straightforward; we can drop the
malloc entirely, and replace the sprintf with xstrfmt.

Signed-off-by: Jeff King <peff@peff.net>
---
Just a note on one thing I would look for as a reviewer:

  In theory this could introduce a time-of-use error (either we xstrfmt
  at the malloc, in which case the arguments to format might not be
  ready yet, or we xstrfmt where the old sprintf was, in which case the
  pointer is uninitialized earlier). In practice, this is not an issue.
  The two are almost always right next to each other. And even when they
  are not, the xmalloc already runs strlen() on the arguments, so it
  should be safe to do xstrfmt there, too. I.e., if there is a bug, it
  was already there. :)

 builtin/fmt-merge-msg.c |  7 ++-----
 builtin/show-branch.c   | 10 ++++------
 http-push.c             | 18 +++++-------------
 http-walker.c           |  3 +--
 match-trees.c           |  9 ++-------
 merge-recursive.c       | 12 ++++--------
 6 files changed, 18 insertions(+), 41 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3906eda..c462e19 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -178,11 +178,8 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
 		int len = strlen(origin);
 		if (origin[0] == '\'' && origin[len - 1] == '\'')
 			origin = xmemdupz(origin + 1, len - 2);
-	} else {
-		char *new_origin = xmalloc(strlen(origin) + strlen(src) + 5);
-		sprintf(new_origin, "%s of %s", origin, src);
-		origin = new_origin;
-	}
+	} else
+		origin = xstrfmt("%s of %s", origin, src);
 	if (strcmp(".", src))
 		origin_data->is_local_branch = 0;
 	string_list_append(&origins, origin)->util = origin_data;
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d873172..5fd4e4e 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -755,7 +755,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		}
 
 		for (i = 0; i < reflog; i++) {
-			char *logmsg, *m;
+			char *logmsg;
 			const char *msg;
 			unsigned long timestamp;
 			int tz;
@@ -770,11 +770,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				msg = "(none)";
 			else
 				msg++;
-			m = xmalloc(strlen(msg) + 200);
-			sprintf(m, "(%s) %s",
-				show_date(timestamp, tz, 1),
-				msg);
-			reflog_msg[i] = m;
+			reflog_msg[i] = xstrfmt("(%s) %s",
+						show_date(timestamp, tz, 1),
+						msg);
 			free(logmsg);
 			sprintf(nth_desc, "%s@{%d}", *av, base+i);
 			append_ref(nth_desc, sha1, 1);
diff --git a/http-push.c b/http-push.c
index 95650a0..390f74c 100644
--- a/http-push.c
+++ b/http-push.c
@@ -854,8 +854,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	struct xml_ctx ctx;
 	char *escaped;
 
-	url = xmalloc(strlen(repo->url) + strlen(path) + 1);
-	sprintf(url, "%s%s", repo->url, path);
+	url = xstrfmt("%s%s", repo->url, path);
 
 	/* Make sure leading directories exist for the remote ref */
 	ep = strchr(url + strlen(repo->url) + 1, '/');
@@ -1115,7 +1114,7 @@ static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
 		      void *userData)
 {
-	char *url = xmalloc(strlen(repo->url) + strlen(path) + 1);
+	char *url = xstrfmt("%s%s", repo->url, path);
 	struct active_request_slot *slot;
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
@@ -1131,8 +1130,6 @@ static void remote_ls(const char *path, int flags,
 	ls.userData = userData;
 	ls.userFunc = userFunc;
 
-	sprintf(url, "%s%s", repo->url, path);
-
 	strbuf_addf(&out_buffer.buf, PROPFIND_ALL_REQUEST);
 
 	dav_headers = curl_slist_append(dav_headers, "Depth: 1");
@@ -1534,10 +1531,9 @@ static void update_remote_info_refs(struct remote_lock *lock)
 
 static int remote_exists(const char *path)
 {
-	char *url = xmalloc(strlen(repo->url) + strlen(path) + 1);
+	char *url = xstrfmt("%s%s", repo->url, path);
 	int ret;
 
-	sprintf(url, "%s%s", repo->url, path);
 
 	switch (http_get_strbuf(url, NULL, NULL)) {
 	case HTTP_OK:
@@ -1557,12 +1553,9 @@ static int remote_exists(const char *path)
 
 static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 {
-	char *url;
+	char *url = xstrfmt("%s%s", repo->url, path);
 	struct strbuf buffer = STRBUF_INIT;
 
-	url = xmalloc(strlen(repo->url) + strlen(path) + 1);
-	sprintf(url, "%s%s", repo->url, path);
-
 	if (http_get_strbuf(url, &buffer, NULL) != HTTP_OK)
 		die("Couldn't get %s for remote symref\n%s", url,
 		    curl_errorstr);
@@ -1671,8 +1664,7 @@ static int delete_remote_branch(const char *pattern, int force)
 	fprintf(stderr, "Removing remote branch '%s'\n", remote_ref->name);
 	if (dry_run)
 		return 0;
-	url = xmalloc(strlen(repo->url) + strlen(remote_ref->name) + 1);
-	sprintf(url, "%s%s", repo->url, remote_ref->name);
+	url = xstrfmt("%s%s", repo->url, remote_ref->name);
 	slot = get_active_slot();
 	slot->results = &results;
 	curl_setup_http_get(slot->curl, url, DAV_DELETE);
diff --git a/http-walker.c b/http-walker.c
index ab6a4fe..dbddfaa 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -341,8 +341,7 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	if (walker->get_verbosely)
 		fprintf(stderr, "Getting alternates list for %s\n", base);
 
-	url = xmalloc(strlen(base) + 31);
-	sprintf(url, "%s/objects/info/http-alternates", base);
+	url = xstrfmt("%s/objects/info/http-alternates", base);
 
 	/*
 	 * Use a callback to process the result, since another request
diff --git a/match-trees.c b/match-trees.c
index e80b4af..1ce0954 100644
--- a/match-trees.c
+++ b/match-trees.c
@@ -140,17 +140,12 @@ static void match_trees(const unsigned char *hash1,
 			goto next;
 		score = score_trees(elem, hash2);
 		if (*best_score < score) {
-			char *newpath;
-			newpath = xmalloc(strlen(base) + strlen(path) + 1);
-			sprintf(newpath, "%s%s", base, path);
 			free(*best_match);
-			*best_match = newpath;
+			*best_match = xstrfmt("%s%s", base, path);
 			*best_score = score;
 		}
 		if (recurse_limit) {
-			char *newbase;
-			newbase = xmalloc(strlen(base) + strlen(path) + 2);
-			sprintf(newbase, "%s%s/", base, path);
+			char *newbase = xstrfmt("%s%s/", base, path);
 			match_trees(elem, hash2, best_score, best_match,
 				    newbase, recurse_limit - 1);
 			free(newbase);
diff --git a/merge-recursive.c b/merge-recursive.c
index cab16fa..532a1da 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -969,14 +969,10 @@ merge_file_special_markers(struct merge_options *o,
 	char *side2 = NULL;
 	struct merge_file_info mfi;
 
-	if (filename1) {
-		side1 = xmalloc(strlen(branch1) + strlen(filename1) + 2);
-		sprintf(side1, "%s:%s", branch1, filename1);
-	}
-	if (filename2) {
-		side2 = xmalloc(strlen(branch2) + strlen(filename2) + 2);
-		sprintf(side2, "%s:%s", branch2, filename2);
-	}
+	if (filename1)
+		side1 = xstrfmt("%s:%s", branch1, filename1);
+	if (filename2)
+		side2 = xstrfmt("%s:%s", branch2, filename2);
 
 	mfi = merge_file_1(o, one, a, b,
 			   side1 ? side1 : branch1, side2 ? side2 : branch2);
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 05/10] use xstrfmt to replace xmalloc + strcpy/strcat

[Jeff King]

It's easy to get manual allocation calculations wrong, and
the use of strcpy/strcat raise red flags for people looking
for buffer overflows (though in this case each site was
fine).

It's also shorter to use xstrfmt, and the printf-format
tends to be easier for a reader to see what the final string
will look like.

Signed-off-by: Jeff King <peff@peff.net>
---
By the way, I think that the tip_name allocation in name_rev leaks
badly, but it's a little tricky to fix (we sometimes hand off ownership
of the variable, and sometimes not). However, this patch does not make
it any worse, and nobody is complaining, so I left it for now.

 builtin/apply.c    | 4 +---
 builtin/fetch.c    | 9 ++-------
 builtin/name-rev.c | 5 +----
 sha1_name.c        | 5 +----
 shell.c            | 6 +-----
 5 files changed, 6 insertions(+), 23 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 9c5724e..b796910 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1281,9 +1281,7 @@ static int parse_git_header(const char *line, int len, unsigned int size, struct
 	 */
 	patch->def_name = git_header_name(line, len);
 	if (patch->def_name && root) {
-		char *s = xmalloc(root_len + strlen(patch->def_name) + 1);
-		strcpy(s, root);
-		strcpy(s + root_len, patch->def_name);
+		char *s = xstrfmt("%s%s", root, patch->def_name);
 		free(patch->def_name);
 		patch->def_name = s;
 	}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..40d989f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1053,16 +1053,11 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 		refs = xcalloc(argc + 1, sizeof(const char *));
 		for (i = 0; i < argc; i++) {
 			if (!strcmp(argv[i], "tag")) {
-				char *ref;
 				i++;
 				if (i >= argc)
 					die(_("You need to specify a tag name."));
-				ref = xmalloc(strlen(argv[i]) * 2 + 22);
-				strcpy(ref, "refs/tags/");
-				strcat(ref, argv[i]);
-				strcat(ref, ":refs/tags/");
-				strcat(ref, argv[i]);
-				refs[j++] = ref;
+				refs[j++] = xstrfmt("refs/tags/%s:refs/tags/%s",
+						    argv[i], argv[i]);
 			} else
 				refs[j++] = argv[i];
 		}
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index c824d4e..3c8f319 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -33,10 +33,7 @@ static void name_rev(struct commit *commit,
 		return;
 
 	if (deref) {
-		char *new_name = xmalloc(strlen(tip_name)+3);
-		strcpy(new_name, tip_name);
-		strcat(new_name, "^0");
-		tip_name = new_name;
+		tip_name = xstrfmt("%s^0", tip_name);
 
 		if (generation)
 			die("generation: %d, but deref?", generation);
diff --git a/sha1_name.c b/sha1_name.c
index 2b6322f..5e95690 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1252,10 +1252,7 @@ static void diagnose_invalid_sha1_path(const char *prefix,
 		die("Path '%s' exists on disk, but not in '%.*s'.",
 		    filename, object_name_len, object_name);
 	if (errno == ENOENT || errno == ENOTDIR) {
-		char *fullname = xmalloc(strlen(filename)
-					     + strlen(prefix) + 1);
-		strcpy(fullname, prefix);
-		strcat(fullname, filename);
+		char *fullname = xstrfmt("%s%s", prefix, filename);
 
 		if (!get_tree_entry(tree_sha1, fullname,
 				    sha1, &mode)) {
diff --git a/shell.c b/shell.c
index 5c0d47a..ace62e4 100644
--- a/shell.c
+++ b/shell.c
@@ -46,11 +46,7 @@ static int is_valid_cmd_name(const char *cmd)
 
 static char *make_cmd(const char *prog)
 {
-	char *prefix = xmalloc((strlen(prog) + strlen(COMMAND_DIR) + 2));
-	strcpy(prefix, COMMAND_DIR);
-	strcat(prefix, "/");
-	strcat(prefix, prog);
-	return prefix;
+	return xstrfmt("%s/%s", COMMAND_DIR, prog);
 }
 
 static void cd_to_homedir(void)
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Jeff King]

This is shorter, harder to get wrong, and more clearly
captures the intent.

Signed-off-by: Jeff King <peff@peff.net>
---
I wondered if there was a reason to avoid this (because we are in
setup_git_env, which can potentially be called by git_pathdup). But the
git_graft_file initialization below already uses it, and I
double-checked that it is safe once git_dir is set.

 environment.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/environment.c b/environment.c
index 4dac5e9..4de7b81 100644
--- a/environment.c
+++ b/environment.c
@@ -135,15 +135,11 @@ static void setup_git_env(void)
 	gitfile = read_gitfile(git_dir);
 	git_dir = xstrdup(gitfile ? gitfile : git_dir);
 	git_object_dir = getenv(DB_ENVIRONMENT);
-	if (!git_object_dir) {
-		git_object_dir = xmalloc(strlen(git_dir) + 9);
-		sprintf(git_object_dir, "%s/objects", git_dir);
-	}
+	if (!git_object_dir)
+		git_object_dir = git_pathdup("objects");
 	git_index_file = getenv(INDEX_ENVIRONMENT);
-	if (!git_index_file) {
-		git_index_file = xmalloc(strlen(git_dir) + 7);
-		sprintf(git_index_file, "%s/index", git_dir);
-	}
+	if (!git_index_file)
+		git_index_file = git_pathdup("index");
 	git_graft_file = getenv(GRAFT_ENVIRONMENT);
 	if (!git_graft_file)
 		git_graft_file = git_pathdup("info/grafts");
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 07/10] sequencer: use argv_array_pushf

[Jeff King]

This avoids a manual allocation calculation, and is shorter
to boot.

Signed-off-by: Jeff King <peff@peff.net>
---
 sequencer.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0a80c58..2fea824 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -396,18 +396,13 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 {
 	struct argv_array array;
 	int rc;
-	char *gpg_sign;
 
 	argv_array_init(&array);
 	argv_array_push(&array, "commit");
 	argv_array_push(&array, "-n");
 
-	if (opts->gpg_sign) {
-		gpg_sign = xmalloc(3 + strlen(opts->gpg_sign));
-		sprintf(gpg_sign, "-S%s", opts->gpg_sign);
-		argv_array_push(&array, gpg_sign);
-		free(gpg_sign);
-	}
+	if (opts->gpg_sign)
+		argv_array_pushf(&array, "-S%s", opts->gpg_sign);
 	if (opts->signoff)
 		argv_array_push(&array, "-s");
 	if (!opts->edit) {
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 08/10] merge: use argv_array when spawning merge strategy

[Jeff King]

This is shorter, and avoids a rather complicated set of
allocation and free steps.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge.c | 42 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/merge.c b/merge.c
index 70f1000..1fa6e52 100644
--- a/merge.c
+++ b/merge.c
@@ -18,39 +18,23 @@ int try_merge_command(const char *strategy, size_t xopts_nr,
 		      const char **xopts, struct commit_list *common,
 		      const char *head_arg, struct commit_list *remotes)
 {
-	const char **args;
-	int i = 0, x = 0, ret;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	int i, ret;
 	struct commit_list *j;
-	struct strbuf buf = STRBUF_INIT;
 
-	args = xmalloc((4 + xopts_nr + commit_list_count(common) +
-			commit_list_count(remotes)) * sizeof(char *));
-	strbuf_addf(&buf, "merge-%s", strategy);
-	args[i++] = buf.buf;
-	for (x = 0; x < xopts_nr; x++) {
-		char *s = xmalloc(strlen(xopts[x])+2+1);
-		strcpy(s, "--");
-		strcpy(s+2, xopts[x]);
-		args[i++] = s;
-	}
-	for (j = common; j; j = j->next)
-		args[i++] = xstrdup(merge_argument(j->item));
-	args[i++] = "--";
-	args[i++] = head_arg;
-	for (j = remotes; j; j = j->next)
-		args[i++] = xstrdup(merge_argument(j->item));
-	args[i] = NULL;
-	ret = run_command_v_opt(args, RUN_GIT_CMD);
-	strbuf_release(&buf);
-	i = 1;
-	for (x = 0; x < xopts_nr; x++)
-		free((void *)args[i++]);
+	argv_array_pushf(&args, "merge-%s", strategy);
+	for (i = 0; i < xopts_nr; i++)
+		argv_array_pushf(&args, "--%s", xopts[i]);
 	for (j = common; j; j = j->next)
-		free((void *)args[i++]);
-	i += 2;
+		argv_array_push(&args, merge_argument(j->item));
+	argv_array_push(&args, "--");
+	argv_array_push(&args, head_arg);
 	for (j = remotes; j; j = j->next)
-		free((void *)args[i++]);
-	free(args);
+		argv_array_push(&args, merge_argument(j->item));
+
+	ret = run_command_v_opt(args.argv, RUN_GIT_CMD);
+	argv_array_clear(&args);
+
 	discard_cache();
 	if (read_cache() < 0)
 		die(_("failed to read the cache"));
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 09/10] walker_fetch: fix minor memory leak

[Jeff King]

We sometimes allocate "msg" on the heap, but will fail to
free it if we hit the failure code path. We can instead keep
a separate variable that is safe to be freed no matter how
we get to the failure code path.

While we're here, we can also do two readability
improvements:

  1. Use xstrfmt instead of a manual malloc/sprintf

  2. Due to the "maybe we allocate msg, maybe we don't"
     strategy, the logic for deciding which message to show
     was split into two parts. Since the deallocation is now
     pushed onto a separate variable, this is no longer a
     concern, and we can keep all of the logic in the same
     place.

Signed-off-by: Jeff King <peff@peff.net>
---
 walker.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/walker.c b/walker.c
index 1dd86b8..0148264 100644
--- a/walker.c
+++ b/walker.c
@@ -253,7 +253,8 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 {
 	struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
 	unsigned char *sha1 = xmalloc(targets * 20);
-	char *msg;
+	const char *msg;
+	char *to_free = NULL;
 	int ret;
 	int i;
 
@@ -285,21 +286,19 @@ int walker_fetch(struct walker *walker, int targets, char **target,
 	if (loop(walker))
 		goto unlock_and_fail;
 
-	if (write_ref_log_details) {
-		msg = xmalloc(strlen(write_ref_log_details) + 12);
-		sprintf(msg, "fetch from %s", write_ref_log_details);
-	} else {
-		msg = NULL;
-	}
+	if (write_ref_log_details)
+		msg = to_free = xstrfmt("fetch from %s", write_ref_log_details);
+	else
+		msg = "fetch (unknown)";
 	for (i = 0; i < targets; i++) {
 		if (!write_ref || !write_ref[i])
 			continue;
-		ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)");
+		ret = write_ref_sha1(lock[i], &sha1[20 * i], msg);
 		lock[i] = NULL;
 		if (ret)
 			goto unlock_and_fail;
 	}
-	free(msg);
+	free(to_free);
 
 	return 0;
 
@@ -307,6 +306,7 @@ unlock_and_fail:
 	for (i = 0; i < targets; i++)
 		if (lock[i])
 			unlock_ref(lock[i]);
+	free(to_free);
 
 	return -1;
 }
-- 
2.0.0.566.gfe3e6b2

[PATCH v2 10/10] unique_path: fix unlikely heap overflow

[Jeff King]

When merge-recursive creates a unique filename, it uses a
template like:

  path~branch_%d

where the final "_%d" is filled by an incrementing counter
until we find a unique name. We allocate 8 characters for
the counter, but there is no logic to limit the size of the
integer.

Of course, this is extremely unlikely, as you would need a
hundred million collisions to trigger the problem.  Even if
an attacker constructed a specialized repo, it is unlikely
that the victim would have the patience to run the merge.

However, we can make it trivially correct (and hopefully
more readable) by using a strbuf.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-recursive.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 532a1da..398a734 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -601,25 +601,36 @@ static int remove_file(struct merge_options *o, int clean,
 	return 0;
 }
 
+/* add a string to a strbuf, but converting "/" to "_" */
+static void add_flattened_path(struct strbuf *out, const char *s)
+{
+	size_t i = out->len;
+	strbuf_addstr(out, s);
+	for (; i < out->len; i++)
+		if (out->buf[i] == '/')
+			out->buf[i] = '_';
+}
+
 static char *unique_path(struct merge_options *o, const char *path, const char *branch)
 {
-	char *newpath = xmalloc(strlen(path) + 1 + strlen(branch) + 8 + 1);
+	struct strbuf newpath = STRBUF_INIT;
 	int suffix = 0;
 	struct stat st;
-	char *p = newpath + strlen(path);
-	strcpy(newpath, path);
-	*(p++) = '~';
-	strcpy(p, branch);
-	for (; *p; ++p)
-		if ('/' == *p)
-			*p = '_';
-	while (string_list_has_string(&o->current_file_set, newpath) ||
-	       string_list_has_string(&o->current_directory_set, newpath) ||
-	       lstat(newpath, &st) == 0)
-		sprintf(p, "_%d", suffix++);
-
-	string_list_insert(&o->current_file_set, newpath);
-	return newpath;
+	size_t base_len;
+
+	strbuf_addf(&newpath, "%s~", path);
+	add_flattened_path(&newpath, branch);
+
+	base_len = newpath.len;
+	while (string_list_has_string(&o->current_file_set, newpath.buf) ||
+	       string_list_has_string(&o->current_directory_set, newpath.buf) ||
+	       lstat(newpath.buf, &st) == 0) {
+		strbuf_setlen(&newpath, base_len);
+		strbuf_addf(&newpath, "_%d", suffix++);
+	}
+
+	string_list_insert(&o->current_file_set, newpath.buf);
+	return strbuf_detach(&newpath, NULL);
 }
 
 static int dir_in_way(const char *path, int check_working_copy)
-- 
2.0.0.566.gfe3e6b2

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Eric Sunshine]

On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:
> This is shorter, harder to get wrong, and more clearly
> captures the intent.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I wondered if there was a reason to avoid this (because we are in
> setup_git_env, which can potentially be called by git_pathdup). But the
> git_graft_file initialization below already uses it, and I
> double-checked that it is safe once git_dir is set.

This patch will conflict textually with patch 6/28 of Duy's
nd/multiple-work-trees series [1].

[1]: http://thread.gmane.org/gmane.comp.version-control.git/242300/focus=243649

>  environment.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 4dac5e9..4de7b81 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -135,15 +135,11 @@ static void setup_git_env(void)
>         gitfile = read_gitfile(git_dir);
>         git_dir = xstrdup(gitfile ? gitfile : git_dir);
>         git_object_dir = getenv(DB_ENVIRONMENT);
> -       if (!git_object_dir) {
> -               git_object_dir = xmalloc(strlen(git_dir) + 9);
> -               sprintf(git_object_dir, "%s/objects", git_dir);
> -       }
> +       if (!git_object_dir)
> +               git_object_dir = git_pathdup("objects");
>         git_index_file = getenv(INDEX_ENVIRONMENT);
> -       if (!git_index_file) {
> -               git_index_file = xmalloc(strlen(git_dir) + 7);
> -               sprintf(git_index_file, "%s/index", git_dir);
> -       }
> +       if (!git_index_file)
> +               git_index_file = git_pathdup("index");
>         git_graft_file = getenv(GRAFT_ENVIRONMENT);
>         if (!git_graft_file)
>                 git_graft_file = git_pathdup("info/grafts");
> --
> 2.0.0.566.gfe3e6b2

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:
>> This is shorter, harder to get wrong, and more clearly
>> captures the intent.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> I wondered if there was a reason to avoid this (because we are in
>> setup_git_env, which can potentially be called by git_pathdup). But the
>> git_graft_file initialization below already uses it, and I
>> double-checked that it is safe once git_dir is set.
>
> This patch will conflict textually with patch 6/28 of Duy's
> nd/multiple-work-trees series [1].

Thanks; I noticed that and dropped the other topic tentatively, as
it is being rerolled anyway.  In addition to that, because this
series seems fairly focused and well done, and the owners of two
topics known to be competent and active folks, I do not think there
is not much to be worried about ;-).

>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/242300/focus=243649
>
>>  environment.c | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/environment.c b/environment.c
>> index 4dac5e9..4de7b81 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -135,15 +135,11 @@ static void setup_git_env(void)
>>         gitfile = read_gitfile(git_dir);
>>         git_dir = xstrdup(gitfile ? gitfile : git_dir);
>>         git_object_dir = getenv(DB_ENVIRONMENT);
>> -       if (!git_object_dir) {
>> -               git_object_dir = xmalloc(strlen(git_dir) + 9);
>> -               sprintf(git_object_dir, "%s/objects", git_dir);
>> -       }
>> +       if (!git_object_dir)
>> +               git_object_dir = git_pathdup("objects");
>>         git_index_file = getenv(INDEX_ENVIRONMENT);
>> -       if (!git_index_file) {
>> -               git_index_file = xmalloc(strlen(git_dir) + 7);
>> -               sprintf(git_index_file, "%s/index", git_dir);
>> -       }
>> +       if (!git_index_file)
>> +               git_index_file = git_pathdup("index");
>>         git_graft_file = getenv(GRAFT_ENVIRONMENT);
>>         if (!git_graft_file)
>>                 git_graft_file = git_pathdup("info/grafts");
>> --
>> 2.0.0.566.gfe3e6b2

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Duy Nguyen]

On Mon, Jun 23, 2014 at 5:21 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:
>> This is shorter, harder to get wrong, and more clearly
>> captures the intent.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> I wondered if there was a reason to avoid this (because we are in
>> setup_git_env, which can potentially be called by git_pathdup). But the
>> git_graft_file initialization below already uses it, and I
>> double-checked that it is safe once git_dir is set.
>
> This patch will conflict textually with patch 6/28 of Duy's
> nd/multiple-work-trees series [1].

I'll just steal the conflicted bit about git_object_dir and put in the
re-roll. It's a better way anyway. If git_index_file change still
results in conflicts, Junio can resolve it easily (I don't touch it in
nd/multiple-work-trees).
-- 
Duy

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Duy Nguyen]

While it's about malloc..

On Fri, Jun 20, 2014 at 4:28 AM, Jeff King <peff@peff.net> wrote:
> diff --git a/environment.c b/environment.c
> index 4dac5e9..4de7b81 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -135,15 +135,11 @@ static void setup_git_env(void)
>         gitfile = read_gitfile(git_dir);
>         git_dir = xstrdup(gitfile ? gitfile : git_dir);
>         git_object_dir = getenv(DB_ENVIRONMENT);
> -       if (!git_object_dir) {
> -               git_object_dir = xmalloc(strlen(git_dir) + 9);
> -               sprintf(git_object_dir, "%s/objects", git_dir);
> -       }

If DB_ENVIRONMENT is set, we should xstrdup(git_object_dir) because
getenv's return value is not guaranteed persistent. Since you're touch
this area, perhaps do it too (in this, or another patch)?
-- 
Duy

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Jeff King]

On Tue, Jun 24, 2014 at 08:30:26PM +0700, Duy Nguyen wrote:

> On Fri, Jun 20, 2014 at 4:28 AM, Jeff King <peff@peff.net> wrote:
> > diff --git a/environment.c b/environment.c
> > index 4dac5e9..4de7b81 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -135,15 +135,11 @@ static void setup_git_env(void)
> >         gitfile = read_gitfile(git_dir);
> >         git_dir = xstrdup(gitfile ? gitfile : git_dir);
> >         git_object_dir = getenv(DB_ENVIRONMENT);
> > -       if (!git_object_dir) {
> > -               git_object_dir = xmalloc(strlen(git_dir) + 9);
> > -               sprintf(git_object_dir, "%s/objects", git_dir);
> > -       }
> 
> If DB_ENVIRONMENT is set, we should xstrdup(git_object_dir) because
> getenv's return value is not guaranteed persistent. Since you're touch
> this area, perhaps do it too (in this, or another patch)?

Here's a replacement patch that handles this (and just drops the ugly
mallocs as a side effect).

-- >8 --
Subject: [PATCH] setup_git_env: copy getenv return value

The return value of getenv is not guaranteed to survive
across further invocations of setenv or even getenv. When we
are assigning it to globals that last the lifetime of the
program, we should make our own copy.

Signed-off-by: Jeff King <peff@peff.net>
---
 environment.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/environment.c b/environment.c
index 4dac5e9..efb2fa0 100644
--- a/environment.c
+++ b/environment.c
@@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace)
 	return strbuf_detach(&buf, NULL);
 }
 
+static char *git_path_from_env(const char *envvar, const char *path)
+{
+	const char *value = getenv(envvar);
+	return value ? xstrdup(value) : git_pathdup(path);
+}
+
 static void setup_git_env(void)
 {
 	const char *gitfile;
@@ -134,19 +140,9 @@ static void setup_git_env(void)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	gitfile = read_gitfile(git_dir);
 	git_dir = xstrdup(gitfile ? gitfile : git_dir);
-	git_object_dir = getenv(DB_ENVIRONMENT);
-	if (!git_object_dir) {
-		git_object_dir = xmalloc(strlen(git_dir) + 9);
-		sprintf(git_object_dir, "%s/objects", git_dir);
-	}
-	git_index_file = getenv(INDEX_ENVIRONMENT);
-	if (!git_index_file) {
-		git_index_file = xmalloc(strlen(git_dir) + 7);
-		sprintf(git_index_file, "%s/index", git_dir);
-	}
-	git_graft_file = getenv(GRAFT_ENVIRONMENT);
-	if (!git_graft_file)
-		git_graft_file = git_pathdup("info/grafts");
+	git_object_dir = git_path_from_env(DB_ENVIRONMENT, "objects");
+	git_index_file = git_path_from_env(INDEX_ENVIRONMENT, "index");
+	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, "info/grafts");
 	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
 		check_replace_refs = 0;
 	namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));
-- 
2.0.0.566.gfe3e6b2

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Duy Nguyen]

On Wed, Jun 25, 2014 at 3:58 AM, Jeff King <peff@peff.net> wrote:
> Here's a replacement patch that handles this (and just drops the ugly
> mallocs as a side effect).

Shortly after I wrote my email, I thought about getenvdup() and look
for similar getenv/xstrdup patterns. But I saw only one in config.c.
So let's forget about it. Your patch looks good.

>
> -- >8 --
> Subject: [PATCH] setup_git_env: copy getenv return value
>
> The return value of getenv is not guaranteed to survive
> across further invocations of setenv or even getenv. When we
> are assigning it to globals that last the lifetime of the
> program, we should make our own copy.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  environment.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 4dac5e9..efb2fa0 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace)
>         return strbuf_detach(&buf, NULL);
>  }
>
> +static char *git_path_from_env(const char *envvar, const char *path)
> +{
> +       const char *value = getenv(envvar);
> +       return value ? xstrdup(value) : git_pathdup(path);
> +}
> +
>  static void setup_git_env(void)
>  {
>         const char *gitfile;
> @@ -134,19 +140,9 @@ static void setup_git_env(void)
>                 git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
>         gitfile = read_gitfile(git_dir);
>         git_dir = xstrdup(gitfile ? gitfile : git_dir);
> -       git_object_dir = getenv(DB_ENVIRONMENT);
> -       if (!git_object_dir) {
> -               git_object_dir = xmalloc(strlen(git_dir) + 9);
> -               sprintf(git_object_dir, "%s/objects", git_dir);
> -       }
> -       git_index_file = getenv(INDEX_ENVIRONMENT);
> -       if (!git_index_file) {
> -               git_index_file = xmalloc(strlen(git_dir) + 7);
> -               sprintf(git_index_file, "%s/index", git_dir);
> -       }
> -       git_graft_file = getenv(GRAFT_ENVIRONMENT);
> -       if (!git_graft_file)
> -               git_graft_file = git_pathdup("info/grafts");
> +       git_object_dir = git_path_from_env(DB_ENVIRONMENT, "objects");
> +       git_index_file = git_path_from_env(INDEX_ENVIRONMENT, "index");
> +       git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, "info/grafts");
>         if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
>                 check_replace_refs = 0;
>         namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));
> --
> 2.0.0.566.gfe3e6b2
>



-- 
Duy

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's a replacement patch that handles this (and just drops the ugly
> mallocs as a side effect).
>
> -- >8 --
> Subject: [PATCH] setup_git_env: copy getenv return value
>
> The return value of getenv is not guaranteed to survive
> across further invocations of setenv or even getenv. When we
> are assigning it to globals that last the lifetime of the
> program, we should make our own copy.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'
into next, 2014-06-23) with about 20 hours of lag.

I'd make it relative like the attached on top of the series.  Note
that I tweaked the args to git_pathdup() to avoid the "are you sure
you want to give a variable format string to git_pathdup() which you
said is like printf(3)?" warning from the compiler.

Thanks.

-- >8 --
From: Jeff King <peff@peff.net>
Date: Tue, 24 Jun 2014 16:58:15 -0400
Subject: [PATCH] setup_git_env(): introduce git_path_from_env() helper

"Check the value of an environment and fall back to a known path
inside $GIT_DIR" is repeated a few times to determine the location
of the data store, the index and the graft file, but the return
value of getenv is not guaranteed to survive across further
invocations of setenv or even getenv.

Make sure to xstrdup() the value we receive from getenv(3), and
encapsulate the pattern into a helper function.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 environment.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/environment.c b/environment.c
index 4de7b81..565f652 100644
--- a/environment.c
+++ b/environment.c
@@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace)
 	return strbuf_detach(&buf, NULL);
 }
 
+static char *git_path_from_env(const char *envvar, const char *path)
+{
+	const char *value = getenv(envvar);
+	return value ? xstrdup(value) : git_pathdup("%s", path);
+}
+
 static void setup_git_env(void)
 {
 	const char *gitfile;
@@ -134,15 +140,9 @@ static void setup_git_env(void)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	gitfile = read_gitfile(git_dir);
 	git_dir = xstrdup(gitfile ? gitfile : git_dir);
-	git_object_dir = getenv(DB_ENVIRONMENT);
-	if (!git_object_dir)
-		git_object_dir = git_pathdup("objects");
-	git_index_file = getenv(INDEX_ENVIRONMENT);
-	if (!git_index_file)
-		git_index_file = git_pathdup("index");
-	git_graft_file = getenv(GRAFT_ENVIRONMENT);
-	if (!git_graft_file)
-		git_graft_file = git_pathdup("info/grafts");
+	git_object_dir = git_path_from_env(DB_ENVIRONMENT, "objects");
+	git_index_file = git_path_from_env(INDEX_ENVIRONMENT, "index");
+	git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, "info/grafts");
 	if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT))
 		check_replace_refs = 0;
 	namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));
-- 
2.0.0-641-g934bf98

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Jeff King]

On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Here's a replacement patch that handles this (and just drops the ugly
> > mallocs as a side effect).
> >
> > -- >8 --
> > Subject: [PATCH] setup_git_env: copy getenv return value
> >
> > The return value of getenv is not guaranteed to survive
> > across further invocations of setenv or even getenv. When we
> > are assigning it to globals that last the lifetime of the
> > program, we should make our own copy.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> 
> Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'
> into next, 2014-06-23) with about 20 hours of lag.

Ah, sorry. I had checked yesterday that jk/xstrfmt hadn't been merged
yet, but I didn't check when responding to Duy.

> I'd make it relative like the attached on top of the series.  Note
> that I tweaked the args to git_pathdup() to avoid the "are you sure
> you want to give a variable format string to git_pathdup() which you
> said is like printf(3)?" warning from the compiler.

Both changes look good to me. Thanks for taking care of it.

-Peff

Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Here's a replacement patch that handles this (and just drops the ugly
>> > mallocs as a side effect).
>> >
>> > -- >8 --
>> > Subject: [PATCH] setup_git_env: copy getenv return value
>> >
>> > The return value of getenv is not guaranteed to survive
>> > across further invocations of setenv or even getenv. When we
>> > are assigning it to globals that last the lifetime of the
>> > program, we should make our own copy.
>> >
>> > Signed-off-by: Jeff King <peff@peff.net>
>> > ---
>> 
>> Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'
>> into next, 2014-06-23) with about 20 hours of lag.
>
> Ah, sorry. I had checked yesterday that jk/xstrfmt hadn't been merged
> yet, but I didn't check when responding to Duy.

Sorry to have sighed --- crossing e-mails happen all the time.  No
need to feel sorry.

>> I'd make it relative like the attached on top of the series.  Note
>> that I tweaked the args to git_pathdup() to avoid the "are you sure
>> you want to give a variable format string to git_pathdup() which you
>> said is like printf(3)?" warning from the compiler.
>
> Both changes look good to me. Thanks for taking care of it.

Thanks.