# [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z 7 messages from 2011-05-26 to 2011-05-26. Participants: Brandon Casey, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/27474 ## Brandon Casey, 2011-05-26 20:43 Subject: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Message-ID: URL: https://gitlist.dev/e/vdMu20HxiNaWL_IogfamtfZCd6xRyKanIqTSw0k4f2SrbLfUYHDzKT2EGkwdFRlaLzX-5vGGtDCZRGu84HrJzw%40cipher.nrlssc.navy.mil ``` From: Brandon Casey When 'git status' is supplied the -z switch, and no output format has been selected, it is supposed to use the --porcelain format. This does not happen. Instead, the standard long format is used. Add a test to demonstrate this failure. Signed-off-by: Brandon Casey --- t/t7508-status.sh | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/t/t7508-status.sh b/t/t7508-status.sh index cd6e2c5..9601258 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -533,6 +533,13 @@ test_expect_success 'status --porcelain ignores -b' ' ' +test_expect_failure 'status -z implies porcelain' ' + echo " M dir1/modifiedQA dir2/addedQ?? dir1/untrackedQ?? dir2/modifiedQ?? dir2/untrackedQ?? expectQ?? outputQ?? untrackedQ" | + q_to_nul | tr -d "\\012" >expect && + git status -z >output && + test_cmp expect output +' + cat >expect <<\EOF # On branch master # Changes to be committed: -- 1.7.4.4 ``` ## Brandon Casey, 2011-05-26 20:43 Subject: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing Message-ID: URL: https://gitlist.dev/e/vdMu20HxiNaWL_IogfamtflLupHrDNZVtQH5MK7obj8IjaOAuDtj-EwLzysKLoeGXYfBRtJqF-hgEQF5bfRzsQ%40cipher.nrlssc.navy.mil In-Reply-To: ``` From: Brandon Casey 'git status' should use --porcelain output format when -z is given. It was not doing so since the _effect_ of using -z, namely that null_termination would be set, was being checked _before_ option parsing was performed. So, move the check so that it is performed after option parsing. Signed-off-by: Brandon Casey --- builtin/commit.c | 7 ++++--- t/t7508-status.sh | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 5286432..e1af9b1 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1207,9 +1207,6 @@ int cmd_status(int argc, const char **argv, const char *prefix) if (argc == 2 && !strcmp(argv[1], "-h")) usage_with_options(builtin_status_usage, builtin_status_options); - if (null_termination && status_format == STATUS_FORMAT_LONG) - status_format = STATUS_FORMAT_PORCELAIN; - wt_status_prepare(&s); gitmodules_config(); git_config(git_status_config, &s); @@ -1217,6 +1214,10 @@ int cmd_status(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, builtin_status_options, builtin_status_usage, 0); + + if (null_termination && status_format == STATUS_FORMAT_LONG) + status_format = STATUS_FORMAT_PORCELAIN; + handle_untracked_files_arg(&s); if (show_ignored_in_status) s.show_ignored_files = 1; diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 9601258..dc2cc33 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -533,7 +533,7 @@ test_expect_success 'status --porcelain ignores -b' ' ' -test_expect_failure 'status -z implies porcelain' ' +test_expect_success 'status -z implies porcelain' ' echo " M dir1/modifiedQA dir2/addedQ?? dir1/untrackedQ?? dir2/modifiedQ?? dir2/untrackedQ?? expectQ?? outputQ?? untrackedQ" | q_to_nul | tr -d "\\012" >expect && git status -z >output && -- 1.7.4.4 ``` ## Jeff King, 2011-05-26 21:17 Subject: Re: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing Message-ID: <20110526211723.GE31340@sigill.intra.peff.net> URL: https://gitlist.dev/e/20110526211723.GE31340%40sigill.intra.peff.net In-Reply-To: ``` On Thu, May 26, 2011 at 01:43:21PM -0700, Brandon Casey wrote: > 'git status' should use --porcelain output format when -z is given. > It was not doing so since the _effect_ of using -z, namely that > null_termination would be set, was being checked _before_ option parsing > was performed. > > So, move the check so that it is performed after option parsing. Wow, that's an embarrassing bug. I was about to own up to it, but it actually looks like it is Junio's bug from 173e6c8 (git stat -s: short status output, 2009-08-04). Of course, I _twice_ modified the conditional afterwards and failed to notice that it was doing absolutely nothing. So I'm not sure which one of us deserves the brown paper bag. :) Your fix looks obviously correct. Acked-by: Jeff King -Peff ``` ## Brandon Casey, 2011-05-26 21:34 Subject: Re: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing Message-ID: URL: https://gitlist.dev/e/zC-f1fI9ZM3f29xvafX60e8cV8ocMeVmk23oEjhHXxWYhonKbDLDGw%40cipher.nrlssc.navy.mil In-Reply-To: <20110526211723.GE31340@sigill.intra.peff.net> ``` On 05/26/2011 04:17 PM, Jeff King wrote: > On Thu, May 26, 2011 at 01:43:21PM -0700, Brandon Casey wrote: > >> 'git status' should use --porcelain output format when -z is given. >> It was not doing so since the _effect_ of using -z, namely that >> null_termination would be set, was being checked _before_ option parsing >> was performed. >> >> So, move the check so that it is performed after option parsing. > > Wow, that's an embarrassing bug. I was about to own up to it, but it > actually looks like it is Junio's bug from 173e6c8 (git stat -s: short > status output, 2009-08-04). Of course, I _twice_ modified the > conditional afterwards and failed to notice that it was doing absolutely > nothing. So I'm not sure which one of us deserves the brown paper bag. :) :) Well, apparently no one uses -z with git-status anyway (or they only do so in addition to --porcelain). Otherwise it would have been fixed a long time ago. -Brandon ``` ## Junio C Hamano, 2011-05-26 21:45 Subject: Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Message-ID: <7v4o4hw2zm.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4o4hw2zm.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Brandon Casey writes: > From: Brandon Casey > > When 'git status' is supplied the -z switch, and no output format has been > selected, it is supposed to use the --porcelain format. This does not > happen. Instead, the standard long format is used. Add a test to > demonstrate this failure. > > Signed-off-by: Brandon Casey I didn't even know we attempted to default to porcelain when -z is given, even though it is a logical thing to do in the sense that nobody sane would want to read a NUL-terminated human readable format. I'll rewrite the test to avoid hardcoded and context dependent test vector, like this: test_expect_failure 'status -z implies porcelain' ' git status --porcelain | perl -pe "s/\012/\000/g" >expect git status -z >output && test_cmp expect output ' as the only thing you are interested in is the two output to match identically modulo the record termination. By the way, don't we however also want to make sure -z does not kick in automatically when other options like "short" or "normal" is given? ``` ## Brandon Casey, 2011-05-26 22:51 Subject: Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Message-ID: URL: https://gitlist.dev/e/oxsohkXWsdC5MeWOTaXZMEPAVeHglxeXSUXEWJ6xA_rmLcm2iqjtpg%40cipher.nrlssc.navy.mil In-Reply-To: <7v4o4hw2zm.fsf@alter.siamese.dyndns.org> ``` On 05/26/2011 04:45 PM, Junio C Hamano wrote: > Brandon Casey writes: > >> From: Brandon Casey >> >> When 'git status' is supplied the -z switch, and no output format has been >> selected, it is supposed to use the --porcelain format. This does not >> happen. Instead, the standard long format is used. Add a test to >> demonstrate this failure. >> >> Signed-off-by: Brandon Casey > > I didn't even know we attempted to default to porcelain when -z is given, > even though it is a logical thing to do in the sense that nobody sane > would want to read a NUL-terminated human readable format. The behavior is derived from your original behavior where -z was supposed to imply --short (see 173e6c8). You even documented it: 9e4b7ab :) Later, Jeff added --porcelain (6f15787), and updated the documentation: -z:: - Terminate entries with NUL, instead of LF. This implies `-s` - (short status) output format. + Terminate entries with NUL, instead of LF. This implies + the `--porcelain` output format if no other format is given. > I'll rewrite the test to avoid hardcoded and context dependent > test vector, like this: > > test_expect_failure 'status -z implies porcelain' ' > git status --porcelain | > perl -pe "s/\012/\000/g" >expect Missing '&&' here. > git status -z >output && > test_cmp expect output > ' > > as the only thing you are interested in is the two output to match > identically modulo the record termination. Thanks. > By the way, don't we however also want to make sure -z does not kick in > automatically when other options like "short" or "normal" is given? It seems it was intended to be able to do git status -z --short and since -z implies --porcelain if --short is not given, it is not possible to produce "normal" long format and NUL termination at the same time. So, building on your modified test above, we could add something like: test_expect_failure 'status -z with -s works correctly' ' git status -s | perl -pe "s/\012/\000/g" >expect && git status -s -z >output && test_cmp expect output ' Hopefully, the existing tests for status, without -z, will notice if NUL termination kicks in when it is not supposed to. -Brandon ``` ## Junio C Hamano, 2011-05-26 23:31 Subject: Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Message-ID: <7vzkm9t4x3.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzkm9t4x3.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Brandon Casey writes: >> By the way, don't we however also want to make sure -z does not kick in >> automatically when other options like "short" or "normal" is given? Heh, the above obviously does not make sense. What I meant was "if you have -z but also --short or something other than --porcelain, -z should not cause the command to fall back to --porcelain". > It seems it was intended to be able to do > > git status -z --short Yes, you are of course right; in this case we shouldn't do either normal nor porcelain; we should do NUL-terminated short output. ```