{"thread":{"id":"34586","subject":"[regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","startedAt":"2013-08-02T06:40:03Z","lastAt":"2013-08-03T07:18:42Z","messageCount":11,"participants":["Jonathan Nieder","Jeff King","Joey Hess","Brandon Casey","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"224444","messageId":"20130802064003.GB3013@elie.Belkin","threadId":"34586","inReplyTo":"20130801201842.GA16809@kitenet.net","subject":"[regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-02T06:40:03Z","receivedAt":"2013-08-02T06:40:03Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Joey,\n\nJoey Hess wrote[1]:\n\n> Commit c334b87b30c1464a1ab563fe1fb8de5eaf0e5bac caused a reversion in\n> git-cat-file --batch. \n>\n> With an older version:\n>\n> joey@gnu:~/tmp/rrr>git cat-file --batch\n> :file name\n> e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 blob 0\n>\n> With the new version:\n>\n> joey@wren:~/tmp/r>git cat-file --batch\n> :file name\n> :file missing\n>\n> This has broken git-annex's support for operating on files/directories\n> containing whitespace. I cannot see a way to query such a filename using\n> the new interface.\n\nOh dear.  Luckily you caught this before the final 1.8.4 release.  I\nwonder if we should just revert c334b87b (cat-file: split --batch\ninput lines on whitespace, 2013-07-11) for now.\n\nThanks,\nJonathan\n\n[1] http://bugs.debian.org/718517\n"},{"id":"224447","messageId":"20130802105402.GA25697@sigill.intra.peff.net","threadId":"34586","inReplyTo":"20130802064003.GB3013@elie.Belkin","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-02T10:54:02Z","receivedAt":"2013-08-02T10:54:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 01, 2013 at 11:40:03PM -0700, Jonathan Nieder wrote:\n\n> > Commit c334b87b30c1464a1ab563fe1fb8de5eaf0e5bac caused a reversion in\n> > git-cat-file --batch.\n> >\n> > With an older version:\n> >\n> > joey@gnu:~/tmp/rrr>git cat-file --batch\n> > :file name\n> > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 blob 0\n> >\n> > With the new version:\n> >\n> > joey@wren:~/tmp/r>git cat-file --batch\n> > :file name\n> > :file missing\n> [...]\n> Oh dear.  Luckily you caught this before the final 1.8.4 release.  I\n> wonder if we should just revert c334b87b (cat-file: split --batch\n> input lines on whitespace, 2013-07-11) for now.\n\nUgh. Yeah, the incorrect assumption from the commit message of c334b87b\nis \"Object names cannot contain spaces...\". Refs cannot, but filename\nspecifiers after a colon can.\n\nWe need to revert that commit before the release. It can either be\nreplaced with:\n\n  1. A \"--split\" (or similar) option to use the behavior only when\n     desired.\n\n  2. Enabling splitting only when %(rest) is used in the output format.\n\nAnd I suppose it is too late in the cycle for either of those to go into\nv1.8.4. That's a shame, but I think losing that particular patch does\nnot affect the rest of the series, so we are OK to ship without it.\n\nThanks Joey for a timely bug report.\n\n-Peff\n"},{"id":"224448","messageId":"20130802115906.GA9183@sigill.intra.peff.net","threadId":"34586","inReplyTo":"20130802105402.GA25697@sigill.intra.peff.net","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-02T11:59:07Z","receivedAt":"2013-08-02T11:59:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 02, 2013 at 03:54:02AM -0700, Jeff King wrote:\n\n> We need to revert that commit before the release. It can either be\n> replaced with:\n> \n>   1. A \"--split\" (or similar) option to use the behavior only when\n>      desired.\n> \n>   2. Enabling splitting only when %(rest) is used in the output format.\n\nOf the two, I think the latter is more sensible; the former is\nunnecessarily placing the burden on the user to match \"--split\" with\ntheir use of \"%(rest)\". The second is pointless without the first.\n\nA patch to implement (2) is below.\n\nBy the way, Joey, I am not sure how safe \"git cat-file --batch-check\" is\nfor arbitrary filenames. In particular, I don't know how it would react\nto a filename with an embedded newline (and I do not think it will undo\nquoting). Certainly that does not excuse this regression; even if what\nyou are doing is not 100% reliable, it is good enough in sane situations\nand we should not be breaking it. But you may want to double-check the\nbehavior of your scripts in such a case, and we may need to add a \"-z\"\nto support it reliably.\n\nThe \"rev-list --objects\" output may contain such paths, of course, but\nthey will be quoted, and \"%(rest)\" does not care (it is not trying to\ninterpret the paths, but will reliably relay the quoted bits to the\noutput).\n\n-- >8 --\nSubject: [PATCH] cat-file: only split on whitespace when %(rest) is used\n\nCommit c334b87 recently taught `cat-file --batch-check` to\nsplit input lines on whitespace, and stash everything after\nthe first token into the %(rest) output format element. That\ncommit claims:\n\n   Object names cannot contain spaces, so any input with\n   spaces would have resulted in a \"missing\" line.\n\nBut that is not correct. Refs, object sha1s, and various\npeeling suffixes cannot contain spaces, but some object\nnames can. In particular:\n\n  1. Tree paths like \"[<tree>]:path with whitespace\"\n\n  2. Reflog specifications like \"@{2 days ago}\"\n\n  3. Commit searches like \"rev^{/grep me}\" or \":/grep me\"\n\nTo remain backwards compatible, we cannot split on\nwhitespace by default. This patch teaches cat-file to only\ndo the splitting when \"%(rest)\" is used by the output\nformat. Since that element did not exist at all until\nc334b87, old scripts cannot be affected.\n\nThe existence of object names with spaces does mean that you\ncannot reliably do:\n\n  echo \":path with space and other data\" |\n  git cat-file --batch-check=\"%(objectname) %(rest)\"\n\nas it would split the path and feed only \":path\" to\nget_sha1. But that command is nonsensical. If you wanted to\nsee \"and other data\" in \"%(rest)\", git cannot possibly know\nwhere the filename ends and the \"rest\" begins.\n\nIt might be more robust to have something like \"-z\" to\nseparate the input elements. But this patch is still a\nreasonable step before having that.  It makes the easy cases\neasy; people who do not care about %(rest) do not have to\nconsider it, and the %(rest) code handles the spaces and\nnewlines of \"rev-list --objects\" correctly.\n\nHard cases remain hard but possible (if you might get\nwhitespace in your input, you do not get to use %(rest) and\nmust split and join the output yourself using more flexible\ntools). And most importantly, it does not preclude us from\nhaving different splitting rules later if a \"-z\" (or\nsimilar) option is added.  So we can make the hard\ncases easier later, if we choose to.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git-cat-file.txt | 16 ++++++++--------\n builtin/cat-file.c             | 31 +++++++++++++++++++++----------\n t/t1006-cat-file.sh            |  8 ++++++++\n 3 files changed, 37 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 3ddec0b..21cffe2 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -86,12 +86,9 @@ If `--batch` or `--batch-check` is given, `cat-file` will read objects\n ------------\n \n If `--batch` or `--batch-check` is given, `cat-file` will read objects\n-from stdin, one per line, and print information about them.\n-\n-Each line is split at the first whitespace boundary. All characters\n-before that whitespace are considered as a whole object name, and are\n-parsed as if given to linkgit:git-rev-parse[1]. Characters after that\n-whitespace can be accessed using the `%(rest)` atom (see below).\n+from stdin, one per line, and print information about them. By default,\n+the whole line is considered as an object, as if it were fed to\n+linkgit:git-rev-parse[1].\n \n You can specify the information shown for each object by using a custom\n `<format>`. The `<format>` is copied literally to stdout for each\n@@ -113,8 +110,11 @@ newline. The available atoms are:\n \tnote about on-disk sizes in the `CAVEATS` section below.\n \n `rest`::\n-\tThe text (if any) found after the first run of whitespace on the\n-\tinput line (i.e., the \"rest\" of the line).\n+\tIf this atom is used in the output string, input lines are split\n+\tat the first whitespace boundary. All characters before that\n+\twhitespace are considered to be the object name; characters\n+\tafter that first run of whitespace (i.e., the \"rest\" of the\n+\tline) are output in place of the `%(rest)` atom.\n \n If no format is specified, the default format is `%(objectname)\n %(objecttype) %(objectsize)`.\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 163ce6c..07b4818 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -128,6 +128,13 @@ struct expand_data {\n \tint mark_query;\n \n \t/*\n+\t * Whether to split the input on whitespace before feeding it to\n+\t * get_sha1; this is decided during the mark_query phase based on\n+\t * whether we have a %(rest) token in our format.\n+\t */\n+\tint split_on_whitespace;\n+\n+\t/*\n \t * After a mark_query run, this object_info is set up to be\n \t * passed to sha1_object_info_extended. It will point to the data\n \t * elements above, so you can retrieve the response from there.\n@@ -165,7 +172,9 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\telse\n \t\t\tstrbuf_addf(sb, \"%lu\", data->disk_size);\n \t} else if (is_atom(\"rest\", atom, len)) {\n-\t\tif (!data->mark_query && data->rest)\n+\t\tif (data->mark_query)\n+\t\t\tdata->split_on_whitespace = 1;\n+\t\telse if (data->rest)\n \t\t\tstrbuf_addstr(sb, data->rest);\n \t} else\n \t\tdie(\"unknown format element: %.*s\", len, atom);\n@@ -280,16 +289,18 @@ static int batch_objects(struct batch_options *opt)\n \t\tchar *p;\n \t\tint error;\n \n-\t\t/*\n-\t\t * Split at first whitespace, tying off the beginning of the\n-\t\t * string and saving the remainder (or NULL) in data.rest.\n-\t\t */\n-\t\tp = strpbrk(buf.buf, \" \\t\");\n-\t\tif (p) {\n-\t\t\twhile (*p && strchr(\" \\t\", *p))\n-\t\t\t\t*p++ = '\\0';\n+\t\tif (data.split_on_whitespace) {\n+\t\t\t/*\n+\t\t\t * Split at first whitespace, tying off the beginning of the\n+\t\t\t * string and saving the remainder (or NULL) in data.rest.\n+\t\t\t */\n+\t\t\tp = strpbrk(buf.buf, \" \\t\");\n+\t\t\tif (p) {\n+\t\t\t\twhile (*p && strchr(\" \\t\", *p))\n+\t\t\t\t\t*p++ = '\\0';\n+\t\t\t}\n+\t\t\tdata.rest = p;\n \t\t}\n-\t\tdata.rest = p;\n \n \t\terror = batch_one_object(buf.buf, opt, &data);\n \t\tif (error)\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex d499d02..a420742 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -98,6 +98,14 @@ run_tests 'blob' $hello_sha1 $hello_size \"$hello_content\" \"$hello_content\"\n \n run_tests 'blob' $hello_sha1 $hello_size \"$hello_content\" \"$hello_content\"\n \n+test_expect_success '--batch-check without %(rest) considers whole line' '\n+\techo \"$hello_sha1 blob $hello_size\" >expect &&\n+\tgit update-index --add --cacheinfo 100644 $hello_sha1 \"white space\" &&\n+\ttest_when_finished \"git update-index --remove \\\"white space\\\"\" &&\n+\techo \":white space\" | git cat-file --batch-check >actual &&\n+\ttest_cmp expect actual\n+'\n+\n tree_sha1=$(git write-tree)\n tree_size=33\n tree_pretty_content=\"100644 blob $hello_sha1\thello\"\n-- \n1.8.4.rc0.3.g042a762\n"},{"id":"224452","messageId":"20130802152713.GA23548@gnu.kitenet.net","threadId":"34586","inReplyTo":"20130802115906.GA9183@sigill.intra.peff.net","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2013-08-02T15:27:13Z","receivedAt":"2013-08-02T15:27:13Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Jeff King wrote:\n> By the way, Joey, I am not sure how safe \"git cat-file --batch-check\" is\n> for arbitrary filenames. In particular, I don't know how it would react\n> to a filename with an embedded newline (and I do not think it will undo\n> quoting). Certainly that does not excuse this regression; even if what\n> you are doing is not 100% reliable, it is good enough in sane situations\n> and we should not be breaking it. But you may want to double-check the\n> behavior of your scripts in such a case, and we may need to add a \"-z\"\n> to support it reliably.\n\nYes, I would prefer to have a -z mode. I think my code otherwise handles\nnewlines.\n\nThanks for the quick fix. I agree that only enabling the behavior with\n%{rest} makes sense.\n\n-- \nsee shy jo\n"},{"id":"224453","messageId":"CA+sFfMdeGCfWbXfv7YqZi4zZj6RhDaugVgHJNu_Fhmx35wi=8Q@mail.gmail.com","threadId":"34586","inReplyTo":"20130802152713.GA23548@gnu.kitenet.net","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-08-02T16:14:45Z","receivedAt":"2013-08-02T16:14:45Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Fri, Aug 2, 2013 at 8:27 AM, Joey Hess <joey@kitenet.net> wrote:\n> Jeff King wrote:\n>> By the way, Joey, I am not sure how safe \"git cat-file --batch-check\" is\n>> for arbitrary filenames. In particular, I don't know how it would react\n>> to a filename with an embedded newline (and I do not think it will undo\n>> quoting). Certainly that does not excuse this regression; even if what\n>> you are doing is not 100% reliable, it is good enough in sane situations\n>> and we should not be breaking it. But you may want to double-check the\n>> behavior of your scripts in such a case, and we may need to add a \"-z\"\n>> to support it reliably.\n>\n> Yes, I would prefer to have a -z mode. I think my code otherwise handles\n> newlines.\n>\n> Thanks for the quick fix. I agree that only enabling the behavior with\n> %{rest} makes sense.\n>\n> --\n> see shy jo\n\n/methinks we've identified a gap in our test coverage.  Care to add a\ntest that covers the functionality that git-annex depends on?\n\n-Brandon\n"},{"id":"224455","messageId":"7vy58koxxn.fsf@alter.siamese.dyndns.org","threadId":"34586","inReplyTo":"20130802105402.GA25697@sigill.intra.peff.net","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-02T16:32:52Z","receivedAt":"2013-08-02T16:32:52Z","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> We need to revert that commit before the release. It can either be\n> replaced with:\n>\n>   1. A \"--split\" (or similar) option to use the behavior only when\n>      desired.\n>\n>   2. Enabling splitting only when %(rest) is used in the output format.\n>\n> And I suppose it is too late in the cycle for either of those to go into\n> v1.8.4. That's a shame, but I think losing that particular patch does\n> not affect the rest of the series, so we are OK to ship without it.\n>\n> Thanks Joey for a timely bug report.\n\nThanks.  Will do this to jk/cat-file-batch-optim topic and merge it\nto 'master' for now.\n\n-- >8 --\nSubject: [PATCH] Revert \"cat-file: split --batch input lines on whitespace\"\n\nThis reverts commit c334b87b30c1464a1ab563fe1fb8de5eaf0e5bac; the\nupdate assumed that people only used the command to read from\n\"rev-list --objects\" output, whose lines begin with a 40-hex object\nname followed by a whitespace, but it turns out that scripts feed\nrandom extended SHA-1 expressions (e.g. \"HEAD:$pathname\") in which\na whitespace has to be kept.\n---\n Documentation/git-cat-file.txt | 10 ++--------\n builtin/cat-file.c             | 20 +-------------------\n t/t1006-cat-file.sh            |  7 -------\n 3 files changed, 3 insertions(+), 34 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 3ddec0b..10fbc6a 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -88,10 +88,8 @@ BATCH OUTPUT\n If `--batch` or `--batch-check` is given, `cat-file` will read objects\n from stdin, one per line, and print information about them.\n \n-Each line is split at the first whitespace boundary. All characters\n-before that whitespace are considered as a whole object name, and are\n-parsed as if given to linkgit:git-rev-parse[1]. Characters after that\n-whitespace can be accessed using the `%(rest)` atom (see below).\n+Each line is considered as a whole object name, and is parsed as if\n+given to linkgit:git-rev-parse[1].\n \n You can specify the information shown for each object by using a custom\n `<format>`. The `<format>` is copied literally to stdout for each\n@@ -112,10 +110,6 @@ newline. The available atoms are:\n \tThe size, in bytes, that the object takes up on disk. See the\n \tnote about on-disk sizes in the `CAVEATS` section below.\n \n-`rest`::\n-\tThe text (if any) found after the first run of whitespace on the\n-\tinput line (i.e., the \"rest\" of the line).\n-\n If no format is specified, the default format is `%(objectname)\n %(objecttype) %(objectsize)`.\n \ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 163ce6c..4253460 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -119,7 +119,6 @@ struct expand_data {\n \tenum object_type type;\n \tunsigned long size;\n \tunsigned long disk_size;\n-\tconst char *rest;\n \n \t/*\n \t * If mark_query is true, we do not expand anything, but rather\n@@ -164,9 +163,6 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\t\tdata->info.disk_sizep = &data->disk_size;\n \t\telse\n \t\t\tstrbuf_addf(sb, \"%lu\", data->disk_size);\n-\t} else if (is_atom(\"rest\", atom, len)) {\n-\t\tif (!data->mark_query && data->rest)\n-\t\t\tstrbuf_addstr(sb, data->rest);\n \t} else\n \t\tdie(\"unknown format element: %.*s\", len, atom);\n }\n@@ -277,21 +273,7 @@ static int batch_objects(struct batch_options *opt)\n \twarn_on_object_refname_ambiguity = 0;\n \n \twhile (strbuf_getline(&buf, stdin, '\\n') != EOF) {\n-\t\tchar *p;\n-\t\tint error;\n-\n-\t\t/*\n-\t\t * Split at first whitespace, tying off the beginning of the\n-\t\t * string and saving the remainder (or NULL) in data.rest.\n-\t\t */\n-\t\tp = strpbrk(buf.buf, \" \\t\");\n-\t\tif (p) {\n-\t\t\twhile (*p && strchr(\" \\t\", *p))\n-\t\t\t\t*p++ = '\\0';\n-\t\t}\n-\t\tdata.rest = p;\n-\n-\t\terror = batch_one_object(buf.buf, opt, &data);\n+\t\tint error = batch_one_object(buf.buf, opt, &data);\n \t\tif (error)\n \t\t\treturn error;\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex d499d02..4e911fb 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -78,13 +78,6 @@ $content\"\n \techo $sha1 | git cat-file --batch-check=\"%(objecttype) %(objectname)\" >actual &&\n \ttest_cmp expect actual\n     '\n-\n-    test_expect_success '--batch-check with %(rest)' '\n-\techo \"$type this is some extra content\" >expect &&\n-\techo \"$sha1    this is some extra content\" |\n-\t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n-\ttest_cmp expect actual\n-    '\n }\n \n hello_content=\"Hello World\"\n-- \n1.8.4-rc1-125-g7a0ec02\n"},{"id":"224457","messageId":"7vtxj8oxin.fsf@alter.siamese.dyndns.org","threadId":"34586","inReplyTo":"20130802115906.GA9183@sigill.intra.peff.net","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-02T16:41:52Z","receivedAt":"2013-08-02T16:41:52Z","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> On Fri, Aug 02, 2013 at 03:54:02AM -0700, Jeff King wrote:\n>\n>> We need to revert that commit before the release. It can either be\n>> replaced with:\n>> \n>>   1. A \"--split\" (or similar) option to use the behavior only when\n>>      desired.\n>> \n>>   2. Enabling splitting only when %(rest) is used in the output format.\n>\n> Of the two, I think the latter is more sensible; the former is\n> unnecessarily placing the burden on the user to match \"--split\" with\n> their use of \"%(rest)\". The second is pointless without the first.\n>\n> A patch to implement (2) is below.\n\nAs I'd queue this on top of the revert, I had to wrangle it a bit to\nmake it relative, i.e. \"this resurrects what the other reverted\npatch did but in a weaker/safer form\".\n\nThis will be kept outside this cycle.  Thanks for a quick fix.\n"},{"id":"224459","messageId":"20130802172804.GB11329@sigill.intra.peff.net","threadId":"34586","inReplyTo":"7vtxj8oxin.fsf@alter.siamese.dyndns.org","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-02T17:28:04Z","receivedAt":"2013-08-02T17:28:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 02, 2013 at 09:41:52AM -0700, Junio C Hamano wrote:\n\n> > Of the two, I think the latter is more sensible; the former is\n> > unnecessarily placing the burden on the user to match \"--split\" with\n> > their use of \"%(rest)\". The second is pointless without the first.\n> >\n> > A patch to implement (2) is below.\n> \n> As I'd queue this on top of the revert, I had to wrangle it a bit to\n> make it relative, i.e. \"this resurrects what the other reverted\n> patch did but in a weaker/safer form\".\n\nYeah, sorry. After doing the patch I had the thought that maybe the\nleast invasive thing would be the fix rather than the straight revert\n(we are counting on my assertion that just reverting out part of the\nseries will be OK; I'm pretty sure that is the case, but it is not\nrisk-free, either).\n\nI didn't see the result of your wrangling in pu, but I will keep an eye\nout to double-check it (unless you did not finish, in which case I am\nhappy to do the wrangling myself).\n\n-Peff\n"},{"id":"224460","messageId":"7vmwp0osic.fsf@alter.siamese.dyndns.org","threadId":"34586","inReplyTo":"20130802172804.GB11329@sigill.intra.peff.net","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-02T18:30:03Z","receivedAt":"2013-08-02T18:30:03Z","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> On Fri, Aug 02, 2013 at 09:41:52AM -0700, Junio C Hamano wrote:\n>\n>> > Of the two, I think the latter is more sensible; the former is\n>> > unnecessarily placing the burden on the user to match \"--split\" with\n>> > their use of \"%(rest)\". The second is pointless without the first.\n>> >\n>> > A patch to implement (2) is below.\n>> \n>> As I'd queue this on top of the revert, I had to wrangle it a bit to\n>> make it relative, i.e. \"this resurrects what the other reverted\n>> patch did but in a weaker/safer form\".\n>\n> Yeah, sorry. After doing the patch I had the thought that maybe the\n> least invasive thing would be the fix rather than the straight revert\n> (we are counting on my assertion that just reverting out part of the\n> series will be OK; I'm pretty sure that is the case, but it is not\n> risk-free, either).\n>\n> I didn't see the result of your wrangling in pu, but I will keep an eye\n> out to double-check it (unless you did not finish, in which case I am\n> happy to do the wrangling myself).\n\nHere is what is on top of the revert that has been pushed out on\n'pu'.\n\nThanks.\n\n-- >8 --\nFrom: Jeff King <peff@peff.net>\nDate: Fri, 2 Aug 2013 04:59:07 -0700\nSubject: [PATCH] cat-file: only split on whitespace when %(rest) is used\n\nCommit c334b87b (cat-file: split --batch input lines on whitespace,\n2013-07-11) taught `cat-file --batch-check` to split input lines on\nthe first whitespace, and stash everything after the first token\ninto the %(rest) output format element.  It claimed:\n\n   Object names cannot contain spaces, so any input with\n   spaces would have resulted in a \"missing\" line.\n\nBut that is not correct.  Refs, object sha1s, and various peeling\nsuffixes cannot contain spaces, but some object names can. In\nparticular:\n\n  1. Tree paths like \"[<tree>]:path with whitespace\"\n\n  2. Reflog specifications like \"@{2 days ago}\"\n\n  3. Commit searches like \"rev^{/grep me}\" or \":/grep me\"\n\nTo remain backwards compatible, we cannot split on whitespace by\ndefault, hence we will ship 1.8.4 with the commit reverted.\n\nResurrect its attempt but in a weaker form; only do the splitting\nwhen \"%(rest)\" is used in the output format. Since that element did\nnot exist at all before c334b87, old scripts cannot be affected.\n\nThe existence of object names with spaces does mean that you\ncannot reliably do:\n\n  echo \":path with space and other data\" |\n  git cat-file --batch-check=\"%(objectname) %(rest)\"\n\nas it would split the path and feed only \":path\" to get_sha1. But\nthat command is nonsensical. If you wanted to see \"and other data\"\nin \"%(rest)\", git cannot possibly know where the filename ends and\nthe \"rest\" begins.\n\nIt might be more robust to have something like \"-z\" to separate the\ninput elements. But this patch is still a reasonable step before\nhaving that.  It makes the easy cases easy; people who do not care\nabout %(rest) do not have to consider it, and the %(rest) code\nhandles the spaces and newlines of \"rev-list --objects\" correctly.\n\nHard cases remain hard but possible (if you might get whitespace in\nyour input, you do not get to use %(rest) and must split and join\nthe output yourself using more flexible tools). And most\nimportantly, it does not preclude us from having different splitting\nrules later if a \"-z\" (or similar) option is added.  So we can make\nthe hard cases easier later, if we choose to.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/git-cat-file.txt | 14 ++++++++++----\n builtin/cat-file.c             | 31 ++++++++++++++++++++++++++++++-\n t/t1006-cat-file.sh            | 15 +++++++++++++++\n 3 files changed, 55 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 10fbc6a..21cffe2 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -86,10 +86,9 @@ BATCH OUTPUT\n ------------\n \n If `--batch` or `--batch-check` is given, `cat-file` will read objects\n-from stdin, one per line, and print information about them.\n-\n-Each line is considered as a whole object name, and is parsed as if\n-given to linkgit:git-rev-parse[1].\n+from stdin, one per line, and print information about them. By default,\n+the whole line is considered as an object, as if it were fed to\n+linkgit:git-rev-parse[1].\n \n You can specify the information shown for each object by using a custom\n `<format>`. The `<format>` is copied literally to stdout for each\n@@ -110,6 +109,13 @@ newline. The available atoms are:\n \tThe size, in bytes, that the object takes up on disk. See the\n \tnote about on-disk sizes in the `CAVEATS` section below.\n \n+`rest`::\n+\tIf this atom is used in the output string, input lines are split\n+\tat the first whitespace boundary. All characters before that\n+\twhitespace are considered to be the object name; characters\n+\tafter that first run of whitespace (i.e., the \"rest\" of the\n+\tline) are output in place of the `%(rest)` atom.\n+\n If no format is specified, the default format is `%(objectname)\n %(objecttype) %(objectsize)`.\n \ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 4253460..07b4818 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -119,6 +119,7 @@ struct expand_data {\n \tenum object_type type;\n \tunsigned long size;\n \tunsigned long disk_size;\n+\tconst char *rest;\n \n \t/*\n \t * If mark_query is true, we do not expand anything, but rather\n@@ -127,6 +128,13 @@ struct expand_data {\n \tint mark_query;\n \n \t/*\n+\t * Whether to split the input on whitespace before feeding it to\n+\t * get_sha1; this is decided during the mark_query phase based on\n+\t * whether we have a %(rest) token in our format.\n+\t */\n+\tint split_on_whitespace;\n+\n+\t/*\n \t * After a mark_query run, this object_info is set up to be\n \t * passed to sha1_object_info_extended. It will point to the data\n \t * elements above, so you can retrieve the response from there.\n@@ -163,6 +171,11 @@ static void expand_atom(struct strbuf *sb, const char *atom, int len,\n \t\t\tdata->info.disk_sizep = &data->disk_size;\n \t\telse\n \t\t\tstrbuf_addf(sb, \"%lu\", data->disk_size);\n+\t} else if (is_atom(\"rest\", atom, len)) {\n+\t\tif (data->mark_query)\n+\t\t\tdata->split_on_whitespace = 1;\n+\t\telse if (data->rest)\n+\t\t\tstrbuf_addstr(sb, data->rest);\n \t} else\n \t\tdie(\"unknown format element: %.*s\", len, atom);\n }\n@@ -273,7 +286,23 @@ static int batch_objects(struct batch_options *opt)\n \twarn_on_object_refname_ambiguity = 0;\n \n \twhile (strbuf_getline(&buf, stdin, '\\n') != EOF) {\n-\t\tint error = batch_one_object(buf.buf, opt, &data);\n+\t\tchar *p;\n+\t\tint error;\n+\n+\t\tif (data.split_on_whitespace) {\n+\t\t\t/*\n+\t\t\t * Split at first whitespace, tying off the beginning of the\n+\t\t\t * string and saving the remainder (or NULL) in data.rest.\n+\t\t\t */\n+\t\t\tp = strpbrk(buf.buf, \" \\t\");\n+\t\t\tif (p) {\n+\t\t\t\twhile (*p && strchr(\" \\t\", *p))\n+\t\t\t\t\t*p++ = '\\0';\n+\t\t\t}\n+\t\t\tdata.rest = p;\n+\t\t}\n+\n+\t\terror = batch_one_object(buf.buf, opt, &data);\n \t\tif (error)\n \t\t\treturn error;\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 4e911fb..a420742 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -78,6 +78,13 @@ $content\"\n \techo $sha1 | git cat-file --batch-check=\"%(objecttype) %(objectname)\" >actual &&\n \ttest_cmp expect actual\n     '\n+\n+    test_expect_success '--batch-check with %(rest)' '\n+\techo \"$type this is some extra content\" >expect &&\n+\techo \"$sha1    this is some extra content\" |\n+\t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n+\ttest_cmp expect actual\n+    '\n }\n \n hello_content=\"Hello World\"\n@@ -91,6 +98,14 @@ test_expect_success \"setup\" '\n \n run_tests 'blob' $hello_sha1 $hello_size \"$hello_content\" \"$hello_content\"\n \n+test_expect_success '--batch-check without %(rest) considers whole line' '\n+\techo \"$hello_sha1 blob $hello_size\" >expect &&\n+\tgit update-index --add --cacheinfo 100644 $hello_sha1 \"white space\" &&\n+\ttest_when_finished \"git update-index --remove \\\"white space\\\"\" &&\n+\techo \":white space\" | git cat-file --batch-check >actual &&\n+\ttest_cmp expect actual\n+'\n+\n tree_sha1=$(git write-tree)\n tree_size=33\n tree_pretty_content=\"100644 blob $hello_sha1\thello\"\n-- \n1.8.4-rc1-129-g1f3472b\n"},{"id":"224463","messageId":"20130802200529.GA2963@elie.Belkin","threadId":"34586","inReplyTo":"7vmwp0osic.fsf@alter.siamese.dyndns.org","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-02T20:05:29Z","receivedAt":"2013-08-02T20:05:29Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Here is what is on top of the revert that has been pushed out on\n> 'pu'.\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n[...]\n> To remain backwards compatible, we cannot split on whitespace by\n> default, hence we will ship 1.8.4 with the commit reverted.\n[...]\n> It might be more robust to have something like \"-z\" to separate the\n> input elements. But this patch is still a reasonable step before\n> having that.  It makes the easy cases easy; people who do not care\n> about %(rest) do not have to consider it, and the %(rest) code\n> handles the spaces and newlines of \"rev-list --objects\" correctly.\n\nAnother idea for the future might be to start rejecting refnames\nstarting with a double-quote '\"', which would make it safe to treat a\nleading quote-mark as the start of a C-style quoted string.  But\ncurrently that would technically be a breaking change, making \"-z\"\nmore useful in the meantime.\n\nI think several commands already don't deal well with filenames with\nnewlines.  I hope POSIX forbids them (with some suitable migration\nplan) soonish and even wouldn't mind if git were taught to refuse to\ntrack them.\n\nThanks,\nJonathan\n"},{"id":"224495","messageId":"20130803071842.GB26894@sigill.intra.peff.net","threadId":"34586","inReplyTo":"7vmwp0osic.fsf@alter.siamese.dyndns.org","subject":"Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-03T07:18:42Z","receivedAt":"2013-08-03T07:18:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 02, 2013 at 11:30:03AM -0700, Junio C Hamano wrote:\n\n> > I didn't see the result of your wrangling in pu, but I will keep an eye\n> > out to double-check it (unless you did not finish, in which case I am\n> > happy to do the wrangling myself).\n> \n> Here is what is on top of the revert that has been pushed out on\n> 'pu'.\n\nThanks, that looks good to me.\n\nWe may want to also squash in the patch below, which puts the pointer\nvariable in the most-local block and re-wraps the newly indented comment\nfor line length. Neither introduced by your adaptation, but they became\nmore obvious to me when seen on top of the revert.\n\n-Peff\n\n---\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 07b4818..41afaa5 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -286,15 +286,15 @@ static int batch_objects(struct batch_options *opt)\n \twarn_on_object_refname_ambiguity = 0;\n \n \twhile (strbuf_getline(&buf, stdin, '\\n') != EOF) {\n-\t\tchar *p;\n \t\tint error;\n \n \t\tif (data.split_on_whitespace) {\n \t\t\t/*\n-\t\t\t * Split at first whitespace, tying off the beginning of the\n-\t\t\t * string and saving the remainder (or NULL) in data.rest.\n+\t\t\t * Split at first whitespace, tying off the beginning\n+\t\t\t * of the string and saving the remainder (or NULL) in\n+\t\t\t * data.rest.\n \t\t\t */\n-\t\t\tp = strpbrk(buf.buf, \" \\t\");\n+\t\t\tchar *p = strpbrk(buf.buf, \" \\t\");\n \t\t\tif (p) {\n \t\t\t\twhile (*p && strchr(\" \\t\", *p))\n \t\t\t\t\t*p++ = '\\0';\n"}]}