{"thread":{"id":"27474","subject":"[PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z","startedAt":"2011-05-26T20:43:20Z","lastAt":"2011-05-26T23:31:36Z","messageCount":7,"participants":["Brandon Casey","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"168821","messageId":"vdMu20HxiNaWL_IogfamtfZCd6xRyKanIqTSw0k4f2SrbLfUYHDzKT2EGkwdFRlaLzX-5vGGtDCZRGu84HrJzw@cipher.nrlssc.navy.mil","threadId":"27474","inReplyTo":null,"subject":"[PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2011-05-26T20:43:20Z","receivedAt":"2011-05-26T20:43:20Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey \n\nWhen 'git status' is supplied the -z switch, and no output format has been\nselected, it is supposed to use the --porcelain format. This does not\nhappen. Instead, the standard long format is used. Add a test to\ndemonstrate this failure.\n\nSigned-off-by: Brandon Casey \n---\n t/t7508-status.sh | 7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex cd6e2c5..9601258 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -533,6 +533,13 @@ test_expect_success 'status --porcelain ignores -b' '\n \n '\n \n+test_expect_failure 'status -z implies porcelain' '\n+\techo \" M dir1/modifiedQA dir2/addedQ?? dir1/untrackedQ?? dir2/modifiedQ?? dir2/untrackedQ?? expectQ?? outputQ?? untrackedQ\" |\n+\t\tq_to_nul | tr -d \"\\\\012\" >expect &&\n+\tgit status -z >output &&\n+\ttest_cmp expect output\n+'\n+\n cat >expect <<\\EOF\n # On branch master\n # Changes to be committed:\n-- \n1.7.4.4\n"},{"id":"168822","messageId":"vdMu20HxiNaWL_IogfamtflLupHrDNZVtQH5MK7obj8IjaOAuDtj-EwLzysKLoeGXYfBRtJqF-hgEQF5bfRzsQ@cipher.nrlssc.navy.mil","threadId":"27474","inReplyTo":"vdMu20HxiNaWL_IogfamtfZCd6xRyKanIqTSw0k4f2SrbLfUYHDzKT2EGkwdFRlaLzX-5vGGtDCZRGu84HrJzw@cipher.nrlssc.navy.mil","subject":"[PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2011-05-26T20:43:21Z","receivedAt":"2011-05-26T20:43:21Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey \n\n'git status' should use --porcelain output format when -z is given.\nIt was not doing so since the _effect_ of using -z, namely that\nnull_termination would be set, was being checked _before_ option parsing\nwas performed.\n\nSo, move the check so that it is performed after option parsing.\n\nSigned-off-by: Brandon Casey \n---\n builtin/commit.c | 7 ++++---\n t/t7508-status.sh | 2 +-\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5286432..e1af9b1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1207,9 +1207,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_status_usage, builtin_status_options);\n \n-\tif (null_termination && status_format == STATUS_FORMAT_LONG)\n-\t\tstatus_format = STATUS_FORMAT_PORCELAIN;\n-\n \twt_status_prepare(&s);\n \tgitmodules_config();\n \tgit_config(git_status_config, &s);\n@@ -1217,6 +1214,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_status_options,\n \t\t\t builtin_status_usage, 0);\n+\n+\tif (null_termination && status_format == STATUS_FORMAT_LONG)\n+\t\tstatus_format = STATUS_FORMAT_PORCELAIN;\n+\n \thandle_untracked_files_arg(&s);\n \tif (show_ignored_in_status)\n \t\ts.show_ignored_files = 1;\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 9601258..dc2cc33 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -533,7 +533,7 @@ test_expect_success 'status --porcelain ignores -b' '\n \n '\n \n-test_expect_failure 'status -z implies porcelain' '\n+test_expect_success 'status -z implies porcelain' '\n \techo \" M dir1/modifiedQA dir2/addedQ?? dir1/untrackedQ?? dir2/modifiedQ?? dir2/untrackedQ?? expectQ?? outputQ?? untrackedQ\" |\n \t\tq_to_nul | tr -d \"\\\\012\" >expect &&\n \tgit status -z >output &&\n-- \n1.7.4.4\n"},{"id":"168826","messageId":"20110526211723.GE31340@sigill.intra.peff.net","threadId":"27474","inReplyTo":"vdMu20HxiNaWL_IogfamtflLupHrDNZVtQH5MK7obj8IjaOAuDtj-EwLzysKLoeGXYfBRtJqF-hgEQF5bfRzsQ@cipher.nrlssc.navy.mil","subject":"Re: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T21:17:23Z","receivedAt":"2011-05-26T21:17:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2011 at 01:43:21PM -0700, Brandon Casey wrote:\n\n> 'git status' should use --porcelain output format when -z is given.\n> It was not doing so since the _effect_ of using -z, namely that\n> null_termination would be set, was being checked _before_ option parsing\n> was performed.\n> \n> So, move the check so that it is performed after option parsing.\n\nWow, that's an embarrassing bug. I was about to own up to it, but it\nactually looks like it is Junio's bug from 173e6c8 (git stat -s: short\nstatus output, 2009-08-04). Of course, I _twice_ modified the\nconditional afterwards and failed to notice that it was doing absolutely\nnothing. So I'm not sure which one of us deserves the brown paper bag. :)\n\nYour fix looks obviously correct.\n\nAcked-by: Jeff King \n\n-Peff\n"},{"id":"168831","messageId":"zC-f1fI9ZM3f29xvafX60e8cV8ocMeVmk23oEjhHXxWYhonKbDLDGw@cipher.nrlssc.navy.mil","threadId":"27474","inReplyTo":"20110526211723.GE31340@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2011-05-26T21:34:50Z","receivedAt":"2011-05-26T21:34:50Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On 05/26/2011 04:17 PM, Jeff King wrote:\n> On Thu, May 26, 2011 at 01:43:21PM -0700, Brandon Casey wrote:\n> \n>> 'git status' should use --porcelain output format when -z is given.\n>> It was not doing so since the _effect_ of using -z, namely that\n>> null_termination would be set, was being checked _before_ option parsing\n>> was performed.\n>>\n>> So, move the check so that it is performed after option parsing.\n> \n> Wow, that's an embarrassing bug. I was about to own up to it, but it\n> actually looks like it is Junio's bug from 173e6c8 (git stat -s: short\n> status output, 2009-08-04). Of course, I _twice_ modified the\n> conditional afterwards and failed to notice that it was doing absolutely\n> nothing. So I'm not sure which one of us deserves the brown paper bag. :)\n\n:) Well, apparently no one uses -z with git-status anyway (or they only\ndo so in addition to --porcelain). Otherwise it would have been fixed\na long time ago.\n\n-Brandon\n"},{"id":"168832","messageId":"7v4o4hw2zm.fsf@alter.siamese.dyndns.org","threadId":"27474","inReplyTo":"vdMu20HxiNaWL_IogfamtfZCd6xRyKanIqTSw0k4f2SrbLfUYHDzKT2EGkwdFRlaLzX-5vGGtDCZRGu84HrJzw@cipher.nrlssc.navy.mil","subject":"Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T21:45:01Z","receivedAt":"2011-05-26T21:45:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> From: Brandon Casey \n>\n> When 'git status' is supplied the -z switch, and no output format has been\n> selected, it is supposed to use the --porcelain format. This does not\n> happen. Instead, the standard long format is used. Add a test to\n> demonstrate this failure.\n>\n> Signed-off-by: Brandon Casey \n\nI didn't even know we attempted to default to porcelain when -z is given,\neven though it is a logical thing to do in the sense that nobody sane\nwould want to read a NUL-terminated human readable format.\n\nI'll rewrite the test to avoid hardcoded and context dependent\ntest vector, like this:\n\n test_expect_failure 'status -z implies porcelain' '\n git status --porcelain |\n perl -pe \"s/\\012/\\000/g\" >expect\n git status -z >output &&\n test_cmp expect output\n '\n\nas the only thing you are interested in is the two output to match\nidentically modulo the record termination.\n\nBy the way, don't we however also want to make sure -z does not kick in\nautomatically when other options like \"short\" or \"normal\" is given?\n"},{"id":"168845","messageId":"oxsohkXWsdC5MeWOTaXZMEPAVeHglxeXSUXEWJ6xA_rmLcm2iqjtpg@cipher.nrlssc.navy.mil","threadId":"27474","inReplyTo":"7v4o4hw2zm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2011-05-26T22:51:26Z","receivedAt":"2011-05-26T22:51:26Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On 05/26/2011 04:45 PM, Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> From: Brandon Casey \n>>\n>> When 'git status' is supplied the -z switch, and no output format has been\n>> selected, it is supposed to use the --porcelain format. This does not\n>> happen. Instead, the standard long format is used. Add a test to\n>> demonstrate this failure.\n>>\n>> Signed-off-by: Brandon Casey \n> \n> I didn't even know we attempted to default to porcelain when -z is given,\n> even though it is a logical thing to do in the sense that nobody sane\n> would want to read a NUL-terminated human readable format.\n\nThe behavior is derived from your original behavior where -z was\nsupposed to imply --short (see 173e6c8). You even documented\nit: 9e4b7ab :) Later, Jeff added --porcelain (6f15787), and\nupdated the documentation:\n\n -z::\n - Terminate entries with NUL, instead of LF. This implies `-s`\n - (short status) output format.\n + Terminate entries with NUL, instead of LF. This implies\n + the `--porcelain` output format if no other format is given.\n\n> I'll rewrite the test to avoid hardcoded and context dependent\n> test vector, like this:\n> \n> test_expect_failure 'status -z implies porcelain' '\n> git status --porcelain |\n> perl -pe \"s/\\012/\\000/g\" >expect\n\nMissing '&&' here.\n\n> git status -z >output &&\n> test_cmp expect output\n> '\n> \n> as the only thing you are interested in is the two output to match\n> identically modulo the record termination.\n\nThanks.\n\n> By the way, don't we however also want to make sure -z does not kick in\n> automatically when other options like \"short\" or \"normal\" is given?\n\nIt seems it was intended to be able to do\n\n git status -z --short\n\nand since -z implies --porcelain if --short is not given, it is not\npossible to produce \"normal\" long format and NUL termination at\nthe same time.\n\nSo, building on your modified test above, we could add something like:\n\n test_expect_failure 'status -z with -s works correctly' '\n git status -s |\n perl -pe \"s/\\012/\\000/g\" >expect &&\n git status -s -z >output &&\n test_cmp expect output\n '\n\nHopefully, the existing tests for status, without -z, will notice\nif NUL termination kicks in when it is not supposed to.\n\n-Brandon\n"},{"id":"168846","messageId":"7vzkm9t4x3.fsf@alter.siamese.dyndns.org","threadId":"27474","inReplyTo":"oxsohkXWsdC5MeWOTaXZMEPAVeHglxeXSUXEWJ6xA_rmLcm2iqjtpg@cipher.nrlssc.navy.mil","subject":"Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T23:31:36Z","receivedAt":"2011-05-26T23:31:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n>> By the way, don't we however also want to make sure -z does not kick in\n>> automatically when other options like \"short\" or \"normal\" is given?\n\nHeh, the above obviously does not make sense. What I meant was \"if you\nhave -z but also --short or something other than --porcelain, -z should\nnot cause the command to fall back to --porcelain\".\n\n> It seems it was intended to be able to do\n>\n> git status -z --short\n\nYes, you are of course right; in this case we shouldn't do either normal\nnor porcelain; we should do NUL-terminated short output.\n"}]}