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