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

[Junio C Hamano <gitster@pobox.com>]

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