{"thread":{"id":"47139","subject":"RE: Bug - Status - Space in Filename","startedAt":"2017-11-08T17:28:13Z","lastAt":"2017-11-10T18:13:12Z","messageCount":6,"participants":["Joseph Strauss","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"332085","messageId":"655aaa9d2abf4be1b6ade0574d88c999@EXMBX01B.bhphotovideo.local","threadId":"47139","inReplyTo":null,"subject":"RE: Bug - Status - Space in Filename","fromName":"Joseph Strauss","fromEmail":"josephst@bhphoto.com","sentAt":"2017-11-08T17:27:24Z","receivedAt":"2017-11-08T17:28:13Z","isPatch":false,"sender":{"key":"josephst@bhphoto.com","avatar":null},"body":"I believe I have found a bug in the way git status -s lists filenames.\n\nAccording to the documentation:\n  The fields (including the ->) are separated from each other by a single space. If a filename contains whitespace or other nonprintable characters,   that field will be quoted in the manner of a C string literal: surrounded by ASCII double quote (34) characters, and with interior special characters backslash-escaped.\n\nWhile this is true in most situations, it does not seem to apply to merge conflicts. When a file has merge conflicts I am getting the following:\n $ git status -s\n UU some/path/with space/in/the/name\n M  \"another/path/with space/in/the/name \"\n\nI found the same problem for the following versions:\n. git version 2.15.0.windows.1\n. git version 2.10.0\n\n\n\nJoseph Kalman Strauss\nLifecycle Management Engineer\nB&H Photo\n212-239-7500 x2212\njosephst@bhphoto.com\n\n"},{"id":"332105","messageId":"xmqqvaikjfoj.fsf@gitster.mtv.corp.google.com","threadId":"47139","inReplyTo":"655aaa9d2abf4be1b6ade0574d88c999@EXMBX01B.bhphotovideo.local","subject":"Re: Bug - Status - Space in Filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-09T03:26:20Z","receivedAt":"2017-11-09T03:26:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joseph Strauss <josephst@bhphoto.com> writes:\n\n> I believe I have found a bug in the way git status -s lists filenames.\n>\n> According to the documentation:\n>   The fields (including the ->) are separated from each other by a single space. If a filename contains whitespace or other nonprintable characters,   that field will be quoted in the manner of a C string literal: surrounded by ASCII double quote (34) characters, and with interior special characters backslash-escaped.\n>\n> While this is true in most situations, it does not seem to apply to merge conflicts. When a file has merge conflicts I am getting the following:\n>  $ git status -s\n>  UU some/path/with space/in/the/name\n>  M  \"another/path/with space/in/the/name \"\n>\n> I found the same problem for the following versions:\n> . git version 2.15.0.windows.1\n> . git version 2.10.0\n\nThanks.\n\nwt_shortstatus_status() has this code:\n\n\tif (s->null_termination) {\n\t\tfprintf(stdout, \"%s%c\", it->string, 0);\n\t\tif (d->head_path)\n\t\t\tfprintf(stdout, \"%s%c\", d->head_path, 0);\n\t} else {\n\t\tstruct strbuf onebuf = STRBUF_INIT;\n\t\tconst char *one;\n\t\tif (d->head_path) {\n\t\t\tone = quote_path(d->head_path, s->prefix, &onebuf);\n\t\t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n\t\t\t\tputchar('\"');\n\t\t\t\tstrbuf_addch(&onebuf, '\"');\n\t\t\t\tone = onebuf.buf;\n\t\t\t}\n\t\t\tprintf(\"%s -> \", one);\n\t\t\tstrbuf_release(&onebuf);\n\t\t}\n\t\tone = quote_path(it->string, s->prefix, &onebuf);\n\t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n\t\t\tputchar('\"');\n\t\t\tstrbuf_addch(&onebuf, '\"');\n\t\t\tone = onebuf.buf;\n\t\t}\n\t\tprintf(\"%s\\n\", one);\n\t\tstrbuf_release(&onebuf);\n\t}\n\nBut the corresponding part in wt_shortstatus_unmerged() reads like\nthis:\n\n\tif (s->null_termination) {\n\t\tfprintf(stdout, \" %s%c\", it->string, 0);\n\t} else {\n\t\tstruct strbuf onebuf = STRBUF_INIT;\n\t\tconst char *one;\n\t\tone = quote_path(it->string, s->prefix, &onebuf);\n\t\tprintf(\" %s\\n\", one);\n\t\tstrbuf_release(&onebuf);\n\t}\n\nThe difference in d->head_path part is dealing about renames, which\nis irrelevant for showing unmerged paths, but the key difference is\nthat the _unmerged() forgets to add the enclosing \"\" around the\nresult of quote_path().\n\nIt seems that wt_shortstatus_other() shares the same issue.  Perhaps\nthis will \"fix\" it?\n\nHaving said all that, the whole \"quote path using c-style\" was\ndesigned very deliberately to treat SP (but not other kind of\nwhitespaces like HT) as something we do *not* have to quote (and\nmore importantly, do not *want* to quote, as pathnames with SP in it\nare quite common for those who are used to \"My Documents/\" etc.), so\nit is arguably that what is broken and incorrect is the extra\nquoting with dq done in wt_shortstatus_status().  It of course is\ntoo late to change the rules this late in the game, though.\n\nPerhaps a better approach is to refactor the extra quoting I moved\nto this emit_with_optional_dq() helper into quote_path() which\ncurrently is just aliased to quote_path_relative().  It also is\npossible that we may want to extend the \"no_dq\" parameter that is\ngiven to quote_c_style() helper from a boolean to a set of flag\nbits, and allow callers to request \"I want SP added to the set of\nbytes that triggers the quoting\".\n\n\n wt-status.c | 38 ++++++++++++++++++++++----------------\n 1 file changed, 22 insertions(+), 16 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex bedef256ce..472d53d596 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1671,6 +1671,20 @@ static void wt_longstatus_print(struct wt_status *s)\n \t\twt_longstatus_print_stash_summary(s);\n }\n \n+static const char *emit_with_optional_dq(const char *in, const char *prefix, \n+\t\t\t\t\t  struct strbuf *out)\n+{\n+\tconst char *one;\n+\n+\tone = quote_path(in, prefix, out);\n+\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n+\t\tputchar('\"');\n+\t\tstrbuf_addch(out, '\"');\n+\t\tone = out->buf;\n+\t}\n+\treturn one;\n+}\n+\n static void wt_shortstatus_unmerged(struct string_list_item *it,\n \t\t\t   struct wt_status *s)\n {\n@@ -1692,8 +1706,9 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,\n \t} else {\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n-\t\tone = quote_path(it->string, s->prefix, &onebuf);\n-\t\tprintf(\" %s\\n\", one);\n+\t\tputchar(' ');\n+\t\tone = emit_with_optional_dq(it->string, s->prefix, &onebuf);\n+\t\tprintf(\"%s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n }\n@@ -1720,21 +1735,11 @@ static void wt_shortstatus_status(struct string_list_item *it,\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n \t\tif (d->head_path) {\n-\t\t\tone = quote_path(d->head_path, s->prefix, &onebuf);\n-\t\t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n-\t\t\t\tputchar('\"');\n-\t\t\t\tstrbuf_addch(&onebuf, '\"');\n-\t\t\t\tone = onebuf.buf;\n-\t\t\t}\n+\t\t\tone = emit_with_optional_dq(d->head_path, s->prefix, &onebuf);\n \t\t\tprintf(\"%s -> \", one);\n \t\t\tstrbuf_release(&onebuf);\n \t\t}\n-\t\tone = quote_path(it->string, s->prefix, &onebuf);\n-\t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n-\t\t\tputchar('\"');\n-\t\t\tstrbuf_addch(&onebuf, '\"');\n-\t\t\tone = onebuf.buf;\n-\t\t}\n+\t\tone = emit_with_optional_dq(it->string, s->prefix, &onebuf);\n \t\tprintf(\"%s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n@@ -1748,9 +1753,10 @@ static void wt_shortstatus_other(struct string_list_item *it,\n \t} else {\n \t\tstruct strbuf onebuf = STRBUF_INIT;\n \t\tconst char *one;\n-\t\tone = quote_path(it->string, s->prefix, &onebuf);\n \t\tcolor_fprintf(s->fp, color(WT_STATUS_UNTRACKED, s), \"%s\", sign);\n-\t\tprintf(\" %s\\n\", one);\n+\t\tputchar(' ');\n+\t\tone = emit_with_optional_dq(it->string, s->prefix, &onebuf);\n+\t\tprintf(\"%s\\n\", one);\n \t\tstrbuf_release(&onebuf);\n \t}\n }\n"},{"id":"332116","messageId":"20171109132939.3v2z6sf22b4tnwpq@sigill.intra.peff.net","threadId":"47139","inReplyTo":"xmqqvaikjfoj.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug - Status - Space in Filename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-09T13:29:39Z","receivedAt":"2017-11-09T13:29:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2017 at 12:26:20PM +0900, Junio C Hamano wrote:\n\n> The difference in d->head_path part is dealing about renames, which\n> is irrelevant for showing unmerged paths, but the key difference is\n> that the _unmerged() forgets to add the enclosing \"\" around the\n> result of quote_path().\n> \n> It seems that wt_shortstatus_other() shares the same issue.  Perhaps\n> this will \"fix\" it?\n> \n> Having said all that, the whole \"quote path using c-style\" was\n> designed very deliberately to treat SP (but not other kind of\n> whitespaces like HT) as something we do *not* have to quote (and\n> more importantly, do not *want* to quote, as pathnames with SP in it\n> are quite common for those who are used to \"My Documents/\" etc.), so\n> it is arguably that what is broken and incorrect is the extra\n> quoting with dq done in wt_shortstatus_status().  It of course is\n> too late to change the rules this late in the game, though.\n\nYeah, I think the original sin is using \" -> \" in the --porcelain output\n(which just got it from --short). That should have been a HT all along\nto match the rest of the diff code. The --porcelain=v2 format gets this\nright.\n\n> Perhaps a better approach is to refactor the extra quoting I moved\n> to this emit_with_optional_dq() helper into quote_path() which\n> currently is just aliased to quote_path_relative().  It also is\n> possible that we may want to extend the \"no_dq\" parameter that is\n> given to quote_c_style() helper from a boolean to a set of flag\n> bits, and allow callers to request \"I want SP added to the set of\n> bytes that triggers the quoting\".\n\nYeah, I had the same thought upon digging into the code.\n\nThat said, if this is the only place that has this funny quoting, it may\nnot be worth polluting the rest of the code with the idea that quoting\nspaces is a good thing to do.\n\n> diff --git a/wt-status.c b/wt-status.c\n> index bedef256ce..472d53d596 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1671,6 +1671,20 @@ static void wt_longstatus_print(struct wt_status *s)\n>  \t\twt_longstatus_print_stash_summary(s);\n>  }\n>  \n> +static const char *emit_with_optional_dq(const char *in, const char *prefix, \n> +\t\t\t\t\t  struct strbuf *out)\n> +{\n> +\tconst char *one;\n> +\n> +\tone = quote_path(in, prefix, out);\n> +\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n> +\t\tputchar('\"');\n> +\t\tstrbuf_addch(out, '\"');\n> +\t\tone = out->buf;\n> +\t}\n> +\treturn one;\n> +}\n\nThis puts part of its output to stdout, and the other part into a\nstrbuf. That works for these callers, but maybe just emitting the whole\nthing to stdout would be less confusing (and I suspect would clean up\nthe callers a bit, who would not have to worry about the strbuf at all).\n\n-Peff\n"},{"id":"332138","messageId":"xmqqvaijhs5b.fsf@gitster.mtv.corp.google.com","threadId":"47139","inReplyTo":"20171109132939.3v2z6sf22b4tnwpq@sigill.intra.peff.net","subject":"Re: Bug - Status - Space in Filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-10T00:52:16Z","receivedAt":"2017-11-10T00:52:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, I think the original sin is using \" -> \" in the --porcelain output\n> (which just got it from --short). That should have been a HT all along\n> to match the rest of the diff code. The --porcelain=v2 format gets this\n> right.\n\nSo we at lesat did something right, which is a consolation.\n\n>> Perhaps a better approach is to refactor the extra quoting I moved\n>> to this emit_with_optional_dq() helper into quote_path() which\n>> currently is just aliased to quote_path_relative().  It also is\n>> possible that we may want to extend the \"no_dq\" parameter that is\n>> given to quote_c_style() helper from a boolean to a set of flag\n>> bits, and allow callers to request \"I want SP added to the set of\n>> bytes that triggers the quoting\".\n>\n> Yeah, I had the same thought upon digging into the code.\n>\n> That said, if this is the only place that has this funny quoting, it may\n> not be worth polluting the rest of the code with the idea that quoting\n> spaces is a good thing to do.\n\nSounds sane.  We can probably use a helper like this:\n\nstatic char *quote_path_with_sp(const char *in, const char *prefix, struct strbuf *out)\n{\n\tconst char dq = '\"';\n\n\tquote_path(in, prefix, out);\n\tif (out->buf[0] != dq && strchr(out->buf, ' ') != NULL) {\n\t\tstrbuf_insert(out, 0, &dq, 1);\n\t\tstrbuf_addch(out, dq);\n\t}\n\treturn out->buf;\n}\n\nwhich allows the current users like shortstatus_status() to become a\nlot shorter.\n"},{"id":"332153","messageId":"20171110095808.igydpwweyceu6qcf@sigill.intra.peff.net","threadId":"47139","inReplyTo":"xmqqvaijhs5b.fsf@gitster.mtv.corp.google.com","subject":"Re: Bug - Status - Space in Filename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-10T09:58:09Z","receivedAt":"2017-11-10T09:58:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2017 at 09:52:16AM +0900, Junio C Hamano wrote:\n\n> > That said, if this is the only place that has this funny quoting, it may\n> > not be worth polluting the rest of the code with the idea that quoting\n> > spaces is a good thing to do.\n> \n> Sounds sane.  We can probably use a helper like this:\n> \n> static char *quote_path_with_sp(const char *in, const char *prefix, struct strbuf *out)\n> {\n> \tconst char dq = '\"';\n> \n> \tquote_path(in, prefix, out);\n> \tif (out->buf[0] != dq && strchr(out->buf, ' ') != NULL) {\n> \t\tstrbuf_insert(out, 0, &dq, 1);\n> \t\tstrbuf_addch(out, dq);\n> \t}\n> \treturn out->buf;\n> }\n> \n> which allows the current users like shortstatus_status() to become a\n> lot shorter.\n\nAre there callers who don't just print the result? If not, we could just\nalways emit. That's slightly more efficient since it drops the expensive\nstrbuf_insert (though there are already so many copies going on in\nquote_path_relative that it hardly matters). But it also drops the need\nfor the caller to know about the strbuf at all.\n\nLike:\ndiff --git a/wt-status.c b/wt-status.c\nindex 937a87bbd5..4f4706a6e2 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1703,6 +1703,18 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,\n \t}\n }\n \n+static void emit_path(const char *in, const char *prefix)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tquote_path(in, prefix, &buf);\n+\tif (buf.buf[0] != '\"' && strchr(buf.buf, ' ') != NULL) {\n+\t\tputchar('\"');\n+\t\tstrbuf_addch(&buf, '\"');\n+\t}\n+\tfwrite(buf.buf, 1, buf.len, stdout);\n+\tstrbuf_release(&buf);\n+}\n+\n static void wt_shortstatus_status(struct string_list_item *it,\n \t\t\t struct wt_status *s)\n {\n@@ -1722,26 +1734,12 @@ static void wt_shortstatus_status(struct string_list_item *it,\n \t\tif (d->head_path)\n \t\t\tfprintf(stdout, \"%s%c\", d->head_path, 0);\n \t} else {\n-\t\tstruct strbuf onebuf = STRBUF_INIT;\n-\t\tconst char *one;\n \t\tif (d->head_path) {\n-\t\t\tone = quote_path(d->head_path, s->prefix, &onebuf);\n-\t\t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n-\t\t\t\tputchar('\"');\n-\t\t\t\tstrbuf_addch(&onebuf, '\"');\n-\t\t\t\tone = onebuf.buf;\n-\t\t\t}\n-\t\t\tprintf(\"%s -> \", one);\n-\t\t\tstrbuf_release(&onebuf);\n+\t\t\temit_path(d->head_path, s->prefix);\n+\t\t\tprintf(\" -> \");\n \t\t}\n-\t\tone = quote_path(it->string, s->prefix, &onebuf);\n-\t\tif (*one != '\"' && strchr(one, ' ') != NULL) {\n-\t\t\tputchar('\"');\n-\t\t\tstrbuf_addch(&onebuf, '\"');\n-\t\t\tone = onebuf.buf;\n-\t\t}\n-\t\tprintf(\"%s\\n\", one);\n-\t\tstrbuf_release(&onebuf);\n+\t\temit_path(it->string, s->prefix);\n+\t\tputchar('\\n');\n \t}\n }\n \n\nThough really I am fine with any solution that puts this pattern into a\nhelper function rather than repeating it inline.\n\n-Peff\n"},{"id":"332202","messageId":"xmqqfu9mf1e5.fsf@gitster.mtv.corp.google.com","threadId":"47139","inReplyTo":"20171110095808.igydpwweyceu6qcf@sigill.intra.peff.net","subject":"Re: Bug - Status - Space in Filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-10T18:13:06Z","receivedAt":"2017-11-10T18:13:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Are there callers who don't just print the result? If not, we could just\n> always emit. That's slightly more efficient since it drops the expensive\n> strbuf_insert (though there are already so many copies going on in\n> quote_path_relative that it hardly matters). But it also drops the need\n> for the caller to know about the strbuf at all.\n\nI am fine by that, too.  Consider that I'm still suffering from the\ntrauma caused by some patches (not in this area but others) that\nchanged output we have long been directly emiting to stdio to\ninstead capture to strings ;-)\n\nThis might also be a good bite-sized material for the weekend thing\n;-)\n"}]}