threads / patch / 11111

patchquote_path: fix collapsing of relative paths

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

## tl;dr

4 messages between Dec 3, 2007 and Dec 3, 2007. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Jeff King· Dec 3, 2007, 05:30 UTC · lore

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(-)
Show changes to 2 files +3 −2

t/t7502-status.sh, wt-status.c

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
Junio C Hamano· Dec 3, 2007, 05:56 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 51 quoted lines
> 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(-)
Show changes to wt-status.c +6 −5
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) {
Jeff King· Dec 3, 2007, 06:05 UTC · re: Junio C Hamano · lore

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

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. :)

Show 8 quoted lines
> -	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
Johannes Schindelin· Dec 3, 2007, 12:32 UTC · re: Jeff King · lore

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

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

← back to recent threads