{"thread":{"id":"11111","subject":"[PATCH] quote_path: fix collapsing of relative paths","startedAt":"2007-12-03T05:30:01Z","lastAt":"2007-12-03T12:32:00Z","messageCount":4,"participants":["Jeff King","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"61725","messageId":"20071203053001.GA12696@coredump.intra.peff.net","threadId":"11111","inReplyTo":null,"subject":"[PATCH] quote_path: fix collapsing of relative paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-03T05:30:01Z","receivedAt":"2007-12-03T05:30:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The code tries to collapse identical leading components\nbetween the prefix and the path. So if we're in \"dir1\", the\npath \"dir1/file\" should become just \"file\". However, we were\nending up with \"../dir1/file\". The included test expected\nthe wrong output.\n\nBecause the \"len\" parameter to quote_path can be passed in\nas -1 to indicate a NUL-terminated string, we have to\nconsider that possibility in our loop conditional (but no\nadditional checks are necessary, since we already check that\nprefix[off] and in[off] are identical, and that prefix[off]\nis not NUL.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis behavior in git-status had been bugging me, and when I went to fix\nit, I was surprised to find code already there to do it. :) Dscho,\nplease confirm that the test is in fact in error, and that I've read the\nintent of your code correctly.\n\n t/t7502-status.sh |    2 +-\n wt-status.c       |    3 ++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t7502-status.sh b/t/t7502-status.sh\nindex 269b334..d6ae69d 100755\n--- a/t/t7502-status.sh\n+++ b/t/t7502-status.sh\n@@ -68,7 +68,7 @@ cat > expect << \\EOF\n # Changed but not updated:\n #   (use \"git add <file>...\" to update what will be committed)\n #\n-#\tmodified:   ../dir1/modified\n+#\tmodified:   modified\n #\n # Untracked files:\n #   (use \"git add <file>...\" to include in what will be committed)\ndiff --git a/wt-status.c b/wt-status.c\nindex e77120d..09666ec 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -90,7 +90,8 @@ static char *quote_path(const char *in, int len,\n \n \tif (prefix) {\n \t\tint off = 0;\n-\t\twhile (prefix[off] && off < len && prefix[off] == in[off])\n+\t\twhile (prefix[off] && (len < 0 || off < len)\n+\t\t\t\t&& prefix[off] == in[off])\n \t\t\tif (prefix[off] == '/') {\n \t\t\t\tprefix += off + 1;\n \t\t\t\tin += off + 1;\n-- \n1.5.3.7.2070.g88cf2-dirty\n"},{"id":"61732","messageId":"7vabosqq1l.fsf@gitster.siamese.dyndns.org","threadId":"11111","inReplyTo":"20071203053001.GA12696@coredump.intra.peff.net","subject":"Re: [PATCH] quote_path: fix collapsing of relative paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-03T05:56:54Z","receivedAt":"2007-12-03T05:56:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The code tries to collapse identical leading components\n> between the prefix and the path. So if we're in \"dir1\", the\n> path \"dir1/file\" should become just \"file\". However, we were\n> ending up with \"../dir1/file\". The included test expected\n> the wrong output.\n>\n> Because the \"len\" parameter to quote_path can be passed in\n> as -1 to indicate a NUL-terminated string, we have to\n> consider that possibility in our loop conditional (but no\n> additional checks are necessary, since we already check that\n> prefix[off] and in[off] are identical, and that prefix[off]\n> is not NUL.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> This behavior in git-status had been bugging me, and when I went to fix\n> it, I was surprised to find code already there to do it. :) Dscho,\n> please confirm that the test is in fact in error, and that I've read the\n> intent of your code correctly.\n>\n>  t/t7502-status.sh |    2 +-\n>  wt-status.c       |    3 ++-\n>  2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t7502-status.sh b/t/t7502-status.sh\n> index 269b334..d6ae69d 100755\n> --- a/t/t7502-status.sh\n> +++ b/t/t7502-status.sh\n> @@ -68,7 +68,7 @@ cat > expect << \\EOF\n>  # Changed but not updated:\n>  #   (use \"git add <file>...\" to update what will be committed)\n>  #\n> -#\tmodified:   ../dir1/modified\n> +#\tmodified:   modified\n>  #\n>  # Untracked files:\n>  #   (use \"git add <file>...\" to include in what will be committed)\n> diff --git a/wt-status.c b/wt-status.c\n> index e77120d..09666ec 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -90,7 +90,8 @@ static char *quote_path(const char *in, int len,\n>  \n>  \tif (prefix) {\n>  \t\tint off = 0;\n> -\t\twhile (prefix[off] && off < len && prefix[off] == in[off])\n> +\t\twhile (prefix[off] && (len < 0 || off < len)\n> +\t\t\t\t&& prefix[off] == in[off])\n>  \t\t\tif (prefix[off] == '/') {\n>  \t\t\t\tprefix += off + 1;\n>  \t\t\t\tin += off + 1;\n\nSomehow I feel it would be simpler and less error prone if you atually\ncount to set len to the length if you got negative.\n\nThe part that follows the patch, there is a line that subtracts a small\nnumber (off+1) from len --- while it won't have a wraparound issue, it\nfeels a bit ugly to rely on the \"magic -1\" value to stay \"magic\nnegative\" if small positive integers are subtracted from it.\n\nAlso the reason the updated condition to the while loop does not have to\ncheck NUL termination upon negative len is because both (prefix[off] !=\nNUL) and (prefix[off] == in[off]) are checked there, which some may find\nsubtle.\n\nSo perhaps this is easier to read?\n\n---\n wt-status.c |   11 ++++++-----\n 1 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 0e0439f..52ab41c 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -82,12 +82,13 @@ static void wt_status_print_trailer(struct wt_status *s)\n }\n \n static char *quote_path(const char *in, int len,\n-\t\tstruct strbuf *out, const char *prefix)\n+\t\t\tstruct strbuf *out, const char *prefix)\n {\n-\tif (len > 0)\n-\t\tstrbuf_grow(out, len);\n-\tstrbuf_setlen(out, 0);\n+\tif (len < 0)\n+\t\tlen = strlen(in);\n \n+\tstrbuf_grow(out, len);\n+\tstrbuf_setlen(out, 0);\n \tif (prefix) {\n \t\tint off = 0;\n \t\twhile (prefix[off] && off < len && prefix[off] == in[off])\n@@ -104,7 +105,7 @@ static char *quote_path(const char *in, int len,\n \t\t\t\tstrbuf_addstr(out, \"../\");\n \t}\n \n-\tfor (; (len < 0 && *in) || len > 0; in++, len--) {\n+\tfor ( ; len > 0; in++, len--) {\n \t\tint ch = *in;\n \n \t\tswitch (ch) {\n"},{"id":"61733","messageId":"20071203060557.GA13011@coredump.intra.peff.net","threadId":"11111","inReplyTo":"7vabosqq1l.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] quote_path: fix collapsing of relative paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-03T06:05:57Z","receivedAt":"2007-12-03T06:05:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 02, 2007 at 09:56:54PM -0800, Junio C Hamano wrote:\n\n> Somehow I feel it would be simpler and less error prone if you atually\n> count to set len to the length if you got negative.\n> [...]\n> So perhaps this is easier to read?\n\nYes, I actually wrote the exact same patch but discarded it in favor of\nwhat I sent, because I wanted a minimal change (and I assumed it had\nbeen implemented that way to avoid the extra strlen call). But that is\njust premature micro-optimization, and I think the version you posted is\nmuch less subtle.\n\n> The part that follows the patch, there is a line that subtracts a small\n> number (off+1) from len --- while it won't have a wraparound issue, it\n> feels a bit ugly to rely on the \"magic -1\" value to stay \"magic\n> negative\" if small positive integers are subtracted from it.\n\nI hadn't considered that, but another good reason to just set len in the\nfirst place.\n\n> Also the reason the updated condition to the while loop does not have to\n> check NUL termination upon negative len is because both (prefix[off] !=\n> NUL) and (prefix[off] == in[off]) are checked there, which some may find\n> subtle.\n\nYes, I found it subtle, too, which is why I documented it in the commit\nmessage. :)\n\n> -\tif (len > 0)\n> -\t\tstrbuf_grow(out, len);\n> -\tstrbuf_setlen(out, 0);\n> +\tif (len < 0)\n> +\t\tlen = strlen(in);\n>  \n> +\tstrbuf_grow(out, len);\n> +\tstrbuf_setlen(out, 0);\n\nI wonder if this 'grow' is really all that useful, since we are now\ngoing to overallocate in some cases. Perhaps we should just let strbuf\ndo its job and grow as necessary. But it likely doesn't matter either\nway.\n\n-Peff\n"},{"id":"61778","messageId":"Pine.LNX.4.64.0712031231440.27959@racer.site","threadId":"11111","inReplyTo":"20071203053001.GA12696@coredump.intra.peff.net","subject":"Re: [PATCH] quote_path: fix collapsing of relative paths","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-03T12:32:00Z","receivedAt":"2007-12-03T12:32:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 3 Dec 2007, Jeff King wrote:\n\n> This behavior in git-status had been bugging me, and when I went to fix \n> it, I was surprised to find code already there to do it. :) Dscho, \n> please confirm that the test is in fact in error, and that I've read the \n> intent of your code correctly.\n\nYes, you did.\n\nCiao,\nDscho\n"}]}