[PATCH] quote_path: fix collapsing of relative paths

[PATCH] quote_path: fix collapsing of relative paths

[Jeff King]

The code tries to collapse identical leading components
between the prefix and the path. So if we're in "dir1", the
path "dir1/file" should become just "file". However, we were
ending up with "../dir1/file". The included test expected
the wrong output.

Because the "len" parameter to quote_path can be passed in
as -1 to indicate a NUL-terminated string, we have to
consider that possibility in our loop conditional (but no
additional checks are necessary, since we already check that
prefix[off] and in[off] are identical, and that prefix[off]
is not NUL.

Signed-off-by: Jeff King <peff@peff.net>
---
This behavior in git-status had been bugging me, and when I went to fix
it, I was surprised to find code already there to do it. :) Dscho,
please confirm that the test is in fact in error, and that I've read the
intent of your code correctly.

 t/t7502-status.sh |    2 +-
 wt-status.c       |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7502-status.sh b/t/t7502-status.sh
index 269b334..d6ae69d 100755
--- a/t/t7502-status.sh
+++ b/t/t7502-status.sh
@@ -68,7 +68,7 @@ cat > expect << \EOF
 # Changed but not updated:
 #   (use "git add <file>..." to update what will be committed)
 #
-#	modified:   ../dir1/modified
+#	modified:   modified
 #
 # Untracked files:
 #   (use "git add <file>..." to include in what will be committed)
diff --git a/wt-status.c b/wt-status.c
index e77120d..09666ec 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -90,7 +90,8 @@ static char *quote_path(const char *in, int len,
 
 	if (prefix) {
 		int off = 0;
-		while (prefix[off] && off < len && prefix[off] == in[off])
+		while (prefix[off] && (len < 0 || off < len)
+				&& prefix[off] == in[off])
 			if (prefix[off] == '/') {
 				prefix += off + 1;
 				in += off + 1;
-- 
1.5.3.7.2070.g88cf2-dirty

Re: [PATCH] quote_path: fix collapsing of relative paths

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The code tries to collapse identical leading components
> between the prefix and the path. So if we're in "dir1", the
> path "dir1/file" should become just "file". However, we were
> ending up with "../dir1/file". The included test expected
> the wrong output.
>
> Because the "len" parameter to quote_path can be passed in
> as -1 to indicate a NUL-terminated string, we have to
> consider that possibility in our loop conditional (but no
> additional checks are necessary, since we already check that
> prefix[off] and in[off] are identical, and that prefix[off]
> is not NUL.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This behavior in git-status had been bugging me, and when I went to fix
> it, I was surprised to find code already there to do it. :) Dscho,
> please confirm that the test is in fact in error, and that I've read the
> intent of your code correctly.
>
>  t/t7502-status.sh |    2 +-
>  wt-status.c       |    3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7502-status.sh b/t/t7502-status.sh
> index 269b334..d6ae69d 100755
> --- a/t/t7502-status.sh
> +++ b/t/t7502-status.sh
> @@ -68,7 +68,7 @@ cat > expect << \EOF
>  # Changed but not updated:
>  #   (use "git add <file>..." to update what will be committed)
>  #
> -#	modified:   ../dir1/modified
> +#	modified:   modified
>  #
>  # Untracked files:
>  #   (use "git add <file>..." to include in what will be committed)
> diff --git a/wt-status.c b/wt-status.c
> index e77120d..09666ec 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -90,7 +90,8 @@ static char *quote_path(const char *in, int len,
>  
>  	if (prefix) {
>  		int off = 0;
> -		while (prefix[off] && off < len && prefix[off] == in[off])
> +		while (prefix[off] && (len < 0 || off < len)
> +				&& prefix[off] == in[off])
>  			if (prefix[off] == '/') {
>  				prefix += off + 1;
>  				in += off + 1;

Somehow I feel it would be simpler and less error prone if you atually
count to set len to the length if you got negative.

The part that follows the patch, there is a line that subtracts a small
number (off+1) from len --- while it won't have a wraparound issue, it
feels a bit ugly to rely on the "magic -1" value to stay "magic
negative" if small positive integers are subtracted from it.

Also the reason the updated condition to the while loop does not have to
check NUL termination upon negative len is because both (prefix[off] !=
NUL) and (prefix[off] == in[off]) are checked there, which some may find
subtle.

So perhaps this is easier to read?

---
 wt-status.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 0e0439f..52ab41c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -82,12 +82,13 @@ static void wt_status_print_trailer(struct wt_status *s)
 }
 
 static char *quote_path(const char *in, int len,
-		struct strbuf *out, const char *prefix)
+			struct strbuf *out, const char *prefix)
 {
-	if (len > 0)
-		strbuf_grow(out, len);
-	strbuf_setlen(out, 0);
+	if (len < 0)
+		len = strlen(in);
 
+	strbuf_grow(out, len);
+	strbuf_setlen(out, 0);
 	if (prefix) {
 		int off = 0;
 		while (prefix[off] && off < len && prefix[off] == in[off])
@@ -104,7 +105,7 @@ static char *quote_path(const char *in, int len,
 				strbuf_addstr(out, "../");
 	}
 
-	for (; (len < 0 && *in) || len > 0; in++, len--) {
+	for ( ; len > 0; in++, len--) {
 		int ch = *in;
 
 		switch (ch) {

Re: [PATCH] quote_path: fix collapsing of relative paths

[Jeff King]

On Sun, Dec 02, 2007 at 09:56:54PM -0800, Junio C Hamano wrote:

> Somehow I feel it would be simpler and less error prone if you atually
> count to set len to the length if you got negative.
> [...]
> So perhaps this is easier to read?

Yes, I actually wrote the exact same patch but discarded it in favor of
what I sent, because I wanted a minimal change (and I assumed it had
been implemented that way to avoid the extra strlen call). But that is
just premature micro-optimization, and I think the version you posted is
much less subtle.

> The part that follows the patch, there is a line that subtracts a small
> number (off+1) from len --- while it won't have a wraparound issue, it
> feels a bit ugly to rely on the "magic -1" value to stay "magic
> negative" if small positive integers are subtracted from it.

I hadn't considered that, but another good reason to just set len in the
first place.

> Also the reason the updated condition to the while loop does not have to
> check NUL termination upon negative len is because both (prefix[off] !=
> NUL) and (prefix[off] == in[off]) are checked there, which some may find
> subtle.

Yes, I found it subtle, too, which is why I documented it in the commit
message. :)

> -	if (len > 0)
> -		strbuf_grow(out, len);
> -	strbuf_setlen(out, 0);
> +	if (len < 0)
> +		len = strlen(in);
>  
> +	strbuf_grow(out, len);
> +	strbuf_setlen(out, 0);

I wonder if this 'grow' is really all that useful, since we are now
going to overallocate in some cases. Perhaps we should just let strbuf
do its job and grow as necessary. But it likely doesn't matter either
way.

-Peff

Re: [PATCH] quote_path: fix collapsing of relative paths

[Johannes Schindelin]

Hi,

On Mon, 3 Dec 2007, Jeff King wrote:

> This behavior in git-status had been bugging me, and when I went to fix 
> it, I was surprised to find code already there to do it. :) Dscho, 
> please confirm that the test is in fact in error, and that I've read the 
> intent of your code correctly.

Yes, you did.

Ciao,
Dscho