Re: [PATCH v3 09/10] abspath: convert absolute_path() to strbuf

2014-07-28 Thread René Scharfe

Am 28.07.2014 um 21:15 schrieb Jeff King:

On Mon, Jul 28, 2014 at 08:33:55PM +0200, René Scharfe wrote:


  const char *absolute_path(const char *path)
  {
-   static char buf[PATH_MAX + 1];
-
-   if (!*path) {
-   die("The empty string is not a valid path");
-   } else if (is_absolute_path(path)) {
-   if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-   die("Too long path: %.*s", 60, path);
-   } else {
-   size_t len;
-   const char *fmt;
-   const char *cwd = get_pwd_cwd();
-   if (!cwd)
-   die_errno("Cannot determine the current working 
directory");
-   len = strlen(cwd);
-   fmt = (len > 0 && is_dir_sep(cwd[len - 1])) ? "%s%s" : "%s/%s";
-   if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
-   die("Too long path: %.*s", 60, path);
-   }
-   return buf;
+   static struct strbuf sb;
+   strbuf_init(&sb, 0);
+   strbuf_add_absolute_path(&sb, path);
+   return sb.buf;
  }


Is it right to strbuf_init here? That means that we are throwing away
the old buffer for each call. I would think you want instead:

   static struct strbuf sb = STRBUF_INIT;
   strbuf_reset(&sb);
   ...


I changed it from _reset to _init, but I can't remember why. :(  Perhaps 
it's the summer heat.  Your version makes more sense to me now.


René
--
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 v3 09/10] abspath: convert absolute_path() to strbuf

2014-07-28 Thread Jeff King
On Mon, Jul 28, 2014 at 08:33:55PM +0200, René Scharfe wrote:

>  const char *absolute_path(const char *path)
>  {
> - static char buf[PATH_MAX + 1];
> -
> - if (!*path) {
> - die("The empty string is not a valid path");
> - } else if (is_absolute_path(path)) {
> - if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> - die("Too long path: %.*s", 60, path);
> - } else {
> - size_t len;
> - const char *fmt;
> - const char *cwd = get_pwd_cwd();
> - if (!cwd)
> - die_errno("Cannot determine the current working 
> directory");
> - len = strlen(cwd);
> - fmt = (len > 0 && is_dir_sep(cwd[len - 1])) ? "%s%s" : "%s/%s";
> - if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
> - die("Too long path: %.*s", 60, path);
> - }
> - return buf;
> + static struct strbuf sb;
> + strbuf_init(&sb, 0);
> + strbuf_add_absolute_path(&sb, path);
> + return sb.buf;
>  }

Is it right to strbuf_init here? That means that we are throwing away
the old buffer for each call. I would think you want instead:

  static struct strbuf sb = STRBUF_INIT;
  strbuf_reset(&sb);
  ...

-Peff
--
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 v3 09/10] abspath: convert absolute_path() to strbuf

2014-07-28 Thread René Scharfe
Move most of the code of absolute_path() into the new function
strbuf_add_absolute_path() and in the process transform it to use
struct strbuf and xgetcwd() instead of a PATH_MAX-sized buffer,
which can be too small on some file systems.

Signed-off-by: Rene Scharfe 
---
 Documentation/technical/api-strbuf.txt |  6 +
 abspath.c  | 46 +++---
 strbuf.c   | 25 ++
 strbuf.h   |  2 ++
 4 files changed, 37 insertions(+), 42 deletions(-)

diff --git a/Documentation/technical/api-strbuf.txt 
b/Documentation/technical/api-strbuf.txt
index 49e477d..430302c 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -311,6 +311,12 @@ same behaviour as well.
 
Set the buffer to the path of the current working directory.
 
+`strbuf_add_absolute_path`
+
+   Add a path to a buffer, converting a relative path to an
+   absolute one in the process.  Symbolic links are not
+   resolved.
+
 `stripspace`::
 
Strip whitespace from a buffer. The second parameter controls if
diff --git a/abspath.c b/abspath.c
index 16e7fa2..197af68 100644
--- a/abspath.c
+++ b/abspath.c
@@ -140,54 +140,16 @@ const char *real_path_if_valid(const char *path)
return real_path_internal(path, 0);
 }
 
-static const char *get_pwd_cwd(void)
-{
-   static char cwd[PATH_MAX + 1];
-   char *pwd;
-   struct stat cwd_stat, pwd_stat;
-   if (getcwd(cwd, PATH_MAX) == NULL)
-   return NULL;
-   pwd = getenv("PWD");
-   if (pwd && strcmp(pwd, cwd)) {
-   stat(cwd, &cwd_stat);
-   if ((cwd_stat.st_dev || cwd_stat.st_ino) &&
-   !stat(pwd, &pwd_stat) &&
-   pwd_stat.st_dev == cwd_stat.st_dev &&
-   pwd_stat.st_ino == cwd_stat.st_ino) {
-   strlcpy(cwd, pwd, PATH_MAX);
-   }
-   }
-   return cwd;
-}
-
 /*
  * Use this to get an absolute path from a relative one. If you want
  * to resolve links, you should use real_path.
- *
- * If the path is already absolute, then return path. As the user is
- * never meant to free the return value, we're safe.
  */
 const char *absolute_path(const char *path)
 {
-   static char buf[PATH_MAX + 1];
-
-   if (!*path) {
-   die("The empty string is not a valid path");
-   } else if (is_absolute_path(path)) {
-   if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-   die("Too long path: %.*s", 60, path);
-   } else {
-   size_t len;
-   const char *fmt;
-   const char *cwd = get_pwd_cwd();
-   if (!cwd)
-   die_errno("Cannot determine the current working 
directory");
-   len = strlen(cwd);
-   fmt = (len > 0 && is_dir_sep(cwd[len - 1])) ? "%s%s" : "%s/%s";
-   if (snprintf(buf, PATH_MAX, fmt, cwd, path) >= PATH_MAX)
-   die("Too long path: %.*s", 60, path);
-   }
-   return buf;
+   static struct strbuf sb;
+   strbuf_init(&sb, 0);
+   strbuf_add_absolute_path(&sb, path);
+   return sb.buf;
 }
 
 /*
diff --git a/strbuf.c b/strbuf.c
index 2bf4dfa..4d31443 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -576,6 +576,31 @@ void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes)
}
 }
 
+void strbuf_add_absolute_path(struct strbuf *sb, const char *path)
+{
+   if (!*path)
+   die("The empty string is not a valid path");
+   if (!is_absolute_path(path)) {
+   struct stat cwd_stat, pwd_stat;
+   size_t orig_len = sb->len;
+   char *cwd = xgetcwd();
+   char *pwd = getenv("PWD");
+   if (pwd && strcmp(pwd, cwd) &&
+   !stat(cwd, &cwd_stat) &&
+   (cwd_stat.st_dev || cwd_stat.st_ino) &&
+   !stat(pwd, &pwd_stat) &&
+   pwd_stat.st_dev == cwd_stat.st_dev &&
+   pwd_stat.st_ino == cwd_stat.st_ino)
+   strbuf_addstr(sb, pwd);
+   else
+   strbuf_addstr(sb, cwd);
+   if (sb->len > orig_len && !is_dir_sep(sb->buf[sb->len - 1]))
+   strbuf_addch(sb, '/');
+   free(cwd);
+   }
+   strbuf_addstr(sb, path);
+}
+
 int printf_ln(const char *fmt, ...)
 {
int ret;
diff --git a/strbuf.h b/strbuf.h
index bc38bb9..7bdc1da 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -190,6 +190,8 @@ extern void strbuf_addstr_urlencode(struct strbuf *, const 
char *,
int reserved);
 extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);
 
+extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
+
 __attribute__((format (printf,1,2)))
 extern int printf_ln(const char *fmt, ...);
 __attribute__((