Re: [PATCH v14 03/16] quote.c: remove path_relative, use relative_path instead

2013-06-25 Thread Jiang Xin 
2013/6/25 Junio C Hamano gits...@pobox.com:
 Jiang Xin worldhello@gmail.com writes:

 Since there is an enhanced version of relative_path() in path.c,
 remove duplicate counterpart path_relative() in quote.c.

 There is no nice comparison chart before and after like you had in
 patch 02/16?


You mean drawing a table to compare output of path_relative and
relative_path?

I will rewrite the commit log for patch 03/16 like the following. Need
to polish spellings and grammars.

quote.c: substitute path_relative with relative_path

Substitute the function path_relative in quote.c with the function
relative_path. Function relative_path can be treated as an enhanced
and robust version of path_relative.

Outputs of path_relative and it's replacement (relative_path) are the
same for the following cases:

path  prefix output of path_relative  output of relative_path
  =  ===  ===
/a/b/c/   /a/b/  c/   c/
/a/b/c/a/b/  cc
/a/   /a/b/  ../  ../
/ /a/b/  ../../   ../../
/a/c  /a/b/  ../c ../c
/x/y  /a/b/  ../../x/y../../x/y
a/b/c/a/b/   c/   c/
a/a/b/   ../  ../
x/y   a/b/  ../../x/y ../../x/y
/a/b  (empty)/a/b /a/b
/a/b  (null) /a/b /a/b
a/b   (empty)a/b  a/b
a/b   (null) a/b  a/b

But if both of the path and the prefix are the same, or the returned
relative path should be the current directory, the outputs of both
functions are different. Function relative_path returns ./, while
function path_relative returns empty string.

path  prefix output of path_relative  output of relative_path
  =  ===  ===
/a/b/ /a/b/  (empty)  ./
a/b/  a/b/   (empty)  ./
(empty)   (null) (empty)  ./
(empty)   (empty)(empty)  ./

But not panic, the callers of path_relative can handle such cases, or
never encounter this issue at all. E.g.

 * In function quote_path_relative, if the output of path_relative is
   empty, append ./ to it, like:

   if (!out-len)
   strbuf_addstr(out, ./);

 * Another caller is write_name_quoted_relative, which is only used by
   builtin/ls-files.c. git-ls-files only show files, so path of files
   will never be identical with the prefix of a directory.

The following differences show that path_relative can not handle
extra slashes properly.

path  prefix output of path_relative  output of relative_path
  =  ===  ===
/a//b//c/ //a/b//../../../../a//b//c/ c/
a/b//ca//b   ../b//c  c

And if prefix has no trailing slash, path_relative can not work properly
either. But since prefix always has a trailing slash, so it's not a
problem.

path  prefix output of path_relative  output of relative_path
  =  ===  ===
/a/b/c/   /a/b   b/c/ c/
/a/b  /a/b   b./
/a/b/ /a/b   b/   ./
/a/a/b/  ../../a  ../
a/b/c/a/bb/c/ c/
a/b/  a/bb/   ./
a a/b../a ../
x/y   a/b/   ../x/y   ../../x/y
a/c   a/bc../c
/a/   /a/b   (empty)  ../
(empty)   /a/b   ../../   ./


  void write_name_quoted_relative(const char *name, size_t len,
   const char *prefix, size_t prefix_len,
   FILE *fp, int terminator)
  {
   struct strbuf sb = STRBUF_INIT;

 - name = path_relative(name, len, sb, prefix, prefix_len);
 + name = relative_path(name, prefix, sb);

 Are we sure nobody calls prefix_len pointing into the middle of
 string, not at the end of prefix?  This is unsafe for such a
 caller, and to make sure we catch them, we should remove the
 now-unused prefix_len parameter from this function.


Next two commits will remove the unused parameters, and
make this series of patches easy to review. But indeed this
commit has flaws, next two commits are fixes. Should I squash
them back?


-- 
Jiang Xin
--
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 v14 03/16] quote.c: remove path_relative, use relative_path instead

2013-06-25 Thread Junio C Hamano 
Jiang Xin worldhello@gmail.com writes:

 2013/6/25 Junio C Hamano gits...@pobox.com:
 Jiang Xin worldhello@gmail.com writes:

 Since there is an enhanced version of relative_path() in path.c,
 remove duplicate counterpart path_relative() in quote.c.

 There is no nice comparison chart before and after like you had in
 patch 02/16?


 You mean drawing a table to compare output of path_relative and
 relative_path?

I was interested in comparison between the behaviour the current
callers of path_relative() gets, and the behaviour the same callers
get from the consolidated helper function after your patch.
--
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 v14 03/16] quote.c: remove path_relative, use relative_path instead

2013-06-24 Thread Jiang Xin 
Since there is an enhanced version of relative_path() in path.c,
remove duplicate counterpart path_relative() in quote.c.

Signed-off-by: Jiang Xin worldhello@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 quote.c | 55 ++-
 1 file changed, 2 insertions(+), 53 deletions(-)

diff --git a/quote.c b/quote.c
index 91122..64ff3 100644
--- a/quote.c
+++ b/quote.c
@@ -312,75 +312,24 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
fputc(terminator, fp);
 }
 
-static const char *path_relative(const char *in, int len,
-struct strbuf *sb, const char *prefix,
-int prefix_len);
-
 void write_name_quoted_relative(const char *name, size_t len,
const char *prefix, size_t prefix_len,
FILE *fp, int terminator)
 {
struct strbuf sb = STRBUF_INIT;
 
-   name = path_relative(name, len, sb, prefix, prefix_len);
+   name = relative_path(name, prefix, sb);
write_name_quoted(name, fp, terminator);
 
strbuf_release(sb);
 }
 
-/*
- * Give path as relative to prefix.
- *
- * The strbuf may or may not be used, so do not assume it contains the
- * returned path.
- */
-static const char *path_relative(const char *in, int len,
-struct strbuf *sb, const char *prefix,
-int prefix_len)
-{
-   int off, i;
-
-   if (len  0)
-   len = strlen(in);
-   if (prefix_len  0) {
-   if (prefix)
-   prefix_len = strlen(prefix);
-   else
-   prefix_len = 0;
-   }
-
-   off = 0;
-   i = 0;
-   while (i  prefix_len  i  len  prefix[i] == in[i]) {
-   if (prefix[i] == '/')
-   off = i + 1;
-   i++;
-   }
-   in += off;
-   len -= off;
-
-   if (i = prefix_len)
-   return in;
-
-   strbuf_reset(sb);
-   strbuf_grow(sb, len);
-
-   while (i  prefix_len) {
-   if (prefix[i] == '/')
-   strbuf_addstr(sb, ../);
-   i++;
-   }
-   strbuf_add(sb, in, len);
-
-   return sb-buf;
-}
-
 /* quote path as relative to the given prefix */
 char *quote_path_relative(const char *in, int len,
  struct strbuf *out, const char *prefix)
 {
struct strbuf sb = STRBUF_INIT;
-   const char *rel = path_relative(in, len, sb, prefix, -1);
+   const char *rel = relative_path(in, prefix, sb);
strbuf_reset(out);
quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
strbuf_release(sb);
-- 
1.8.3.1.756.g41beab0

--
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 v14 03/16] quote.c: remove path_relative, use relative_path instead

2013-06-24 Thread Junio C Hamano 
Jiang Xin worldhello@gmail.com writes:

 Since there is an enhanced version of relative_path() in path.c,
 remove duplicate counterpart path_relative() in quote.c.

There is no nice comparison chart before and after like you had in
patch 02/16?

  void write_name_quoted_relative(const char *name, size_t len,
   const char *prefix, size_t prefix_len,
   FILE *fp, int terminator)
  {
   struct strbuf sb = STRBUF_INIT;
  
 - name = path_relative(name, len, sb, prefix, prefix_len);
 + name = relative_path(name, prefix, sb);

Are we sure nobody calls prefix_len pointing into the middle of
string, not at the end of prefix?  This is unsafe for such a
caller, and to make sure we catch them, we should remove the
now-unused prefix_len parameter from this function.

   write_name_quoted(name, fp, terminator);
  
   strbuf_release(sb);
  }
  
 -/*
 - * Give path as relative to prefix.
 - *
 - * The strbuf may or may not be used, so do not assume it contains the
 - * returned path.
 - */
 -static const char *path_relative(const char *in, int len,
 -  struct strbuf *sb, const char *prefix,
 -  int prefix_len)
 -{
 - int off, i;
 -
 - if (len  0)
 - len = strlen(in);
 - if (prefix_len  0) {
 - if (prefix)
 - prefix_len = strlen(prefix);
 - else
 - prefix_len = 0;
 - }
 -
 - off = 0;
 - i = 0;
 - while (i  prefix_len  i  len  prefix[i] == in[i]) {
 - if (prefix[i] == '/')
 - off = i + 1;
 - i++;
 - }
 - in += off;
 - len -= off;
 -
 - if (i = prefix_len)
 - return in;
 -
 - strbuf_reset(sb);
 - strbuf_grow(sb, len);
 -
 - while (i  prefix_len) {
 - if (prefix[i] == '/')
 - strbuf_addstr(sb, ../);
 - i++;
 - }
 - strbuf_add(sb, in, len);
 -
 - return sb-buf;
 -}
 -
  /* quote path as relative to the given prefix */
  char *quote_path_relative(const char *in, int len,
 struct strbuf *out, const char *prefix)
  {
   struct strbuf sb = STRBUF_INIT;
 - const char *rel = path_relative(in, len, sb, prefix, -1);
 + const char *rel = relative_path(in, prefix, sb);
   strbuf_reset(out);
   quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
   strbuf_release(sb);
--
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