From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Fri, 07 Sep 2012 19:52:31 GMT
Subject: Re: [PATCH 2/5] path.c: Don't discard the return value of vsnpath()
Message-ID: <504A507F.9030802@ramsay1.demon.co.uk>
In-Reply-To: <7vipbt8q7r.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> cleanup_path() is a quick-and-dirty hack that only deals with
> leading ".///" (e.g. "foo//bar" is not reduced to "foo/bar"), and
> the callers that allocate 4 bytes for buf to hold "foo" may not be
> able to fit it if for some reason there are some bytes that must be
> cleaned, e.g. ".///foo".
> 
> The worst part of its use is that buf and ret could be different,
> which means you cannot safely do:
> 
> 	char *buf = xmalloc(...);
>         buf = git_snpath(buf, n, "%s", ...);
>         ... use buf ...
>         free(buf);

Hmm, this seems very unnatural to me; it would never have occurred to
me to use anything other than a stack allocated buffer (or *maybe* a
global buffer) when calling git_snpath().

In this situation (ie a dynamically allocated buffer used to hold the
result), why would you not use git_pathdup()? (which does not suffer
this problem.)

I guess it does not matter what I find unnatural, ... :(

> but instead have to do something like:
> 
> 	char *path, *buf = xmalloc(...);
>         path = git_snpath(buf, n, "%s", ...);
>         ... use path ...
>         free(buf);
> 
> And this series goes in a direction of making more callers aware of
> the twisted calling convention, which smells really bad.

Note that, prior to commit aba13e7c ("git_pathdup: returns xstrdup-ed
copy of the formatted path", 27-10-2008), git_snpath() was calling
cleanup_path() in the non-error return path. I suspect that this
commit did not intend to "fix the git_snpath() interface" and only
did so by accident. (That's a guess, of course)

However, I much prefer git_snpath(), git_pathdup() and git_path()
return the same result string given the same inputs! :D

ATB,
Ramsay Jones