Re: [PATCH 2/5] path.c: Don't discard the return value of vsnpath()

2012-09-07 Thread Ramsay Jones 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] path.c: Don't discard the return value of vsnpath()

2012-09-04 Thread Ramsay Jones 

The git_snpath() and git_pathdup() functions both use the (static)
function vsnpath() in their implementation. Also, they both discard
the return value of vsnpath(), which has the effect of ignoring the
side effect of calling cleanup_path() in the non-error return path.

In order to ensure that the required cleanup happens, we use the
pointer returned by vsnpath(), rather than the buffer passed into
vsnpath(), to derive the return value from git_snpath() and
git_pathdup().

Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
---
 path.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index 9eb5333..741ae77 100644
--- a/path.c
+++ b/path.c
@@ -70,21 +70,22 @@ bad:
 
 char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 {
+   char *ret;
va_list args;
va_start(args, fmt);
-   (void)vsnpath(buf, n, fmt, args);
+   ret = vsnpath(buf, n, fmt, args);
va_end(args);
-   return buf;
+   return ret;
 }
 
 char *git_pathdup(const char *fmt, ...)
 {
-   char path[PATH_MAX];
+   char path[PATH_MAX], *ret;
va_list args;
va_start(args, fmt);
-   (void)vsnpath(path, sizeof(path), fmt, args);
+   ret = vsnpath(path, sizeof(path), fmt, args);
va_end(args);
-   return xstrdup(path);
+   return xstrdup(ret);
 }
 
 char *mkpathdup(const char *fmt, ...)
-- 
1.7.12

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] path.c: Don't discard the return value of vsnpath()

2012-09-04 Thread Junio C Hamano 
Ramsay Jones ram...@ramsay1.demon.co.uk writes:

 The git_snpath() and git_pathdup() functions both use the (static)
 function vsnpath() in their implementation. Also, they both discard
 the return value of vsnpath(), which has the effect of ignoring the
 side effect of calling cleanup_path() in the non-error return path.

 In order to ensure that the required cleanup happens, we use the
 pointer returned by vsnpath(), rather than the buffer passed into
 vsnpath(), to derive the return value from git_snpath() and
 git_pathdup().

 Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
 ---
  path.c | 11 ++-
  1 file changed, 6 insertions(+), 5 deletions(-)

 diff --git a/path.c b/path.c
 index 9eb5333..741ae77 100644
 --- a/path.c
 +++ b/path.c
 @@ -70,21 +70,22 @@ bad:
  
  char *git_snpath(char *buf, size_t n, const char *fmt, ...)
  {
 + char *ret;
   va_list args;
   va_start(args, fmt);
 - (void)vsnpath(buf, n, fmt, args);
 + ret = vsnpath(buf, n, fmt, args);
   va_end(args);
 - return buf;
 + return ret;
  }
  
  char *git_pathdup(const char *fmt, ...)
  {
 - char path[PATH_MAX];
 + char path[PATH_MAX], *ret;
   va_list args;
   va_start(args, fmt);
 - (void)vsnpath(path, sizeof(path), fmt, args);
 + ret = vsnpath(path, sizeof(path), fmt, args);
   va_end(args);
 - return xstrdup(path);
 + return xstrdup(ret);
  }
  
  char *mkpathdup(const char *fmt, ...)

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

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.

As long as the callers are contained within this file, it is not
making things _too_ bad, so...


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html