* [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z @ 2011-05-26 20:43 Brandon Casey 2011-05-26 20:43 ` [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing Brandon Casey 2011-05-26 21:45 ` [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: Brandon Casey @ 2011-05-26 20:43 UTC (permalink / raw) To: gitster; +Cc: peff, git, Brandon Casey From: Brandon Casey <drafnel@gmail.com> 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 <casey@nrlssc.navy.mil> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing 2011-05-26 20:43 [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Brandon Casey @ 2011-05-26 20:43 ` Brandon Casey 2011-05-26 21:17 ` Jeff King 2011-05-26 21:45 ` [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: Brandon Casey @ 2011-05-26 20:43 UTC (permalink / raw) To: gitster; +Cc: peff, git, Brandon Casey From: Brandon Casey <drafnel@gmail.com> '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 <casey@nrlssc.navy.mil> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing 2011-05-26 20:43 ` [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing Brandon Casey @ 2011-05-26 21:17 ` Jeff King 2011-05-26 21:34 ` Brandon Casey 0 siblings, 1 reply; 7+ messages in thread From: Jeff King @ 2011-05-26 21:17 UTC (permalink / raw) To: Brandon Casey; +Cc: gitster, git, Brandon Casey 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@peff.net> -Peff ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing 2011-05-26 21:17 ` Jeff King @ 2011-05-26 21:34 ` Brandon Casey 0 siblings, 0 replies; 7+ messages in thread From: Brandon Casey @ 2011-05-26 21:34 UTC (permalink / raw) To: Jeff King; +Cc: gitster, git, Brandon Casey 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z 2011-05-26 20:43 [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Brandon Casey 2011-05-26 20:43 ` [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing Brandon Casey @ 2011-05-26 21:45 ` Junio C Hamano 2011-05-26 22:51 ` Brandon Casey 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2011-05-26 21:45 UTC (permalink / raw) To: Brandon Casey; +Cc: peff, git, Brandon Casey Brandon Casey <casey@nrlssc.navy.mil> writes: > From: Brandon Casey <drafnel@gmail.com> > > 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 <casey@nrlssc.navy.mil> 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z 2011-05-26 21:45 ` [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Junio C Hamano @ 2011-05-26 22:51 ` Brandon Casey 2011-05-26 23:31 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Brandon Casey @ 2011-05-26 22:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: peff, git, Brandon Casey On 05/26/2011 04:45 PM, Junio C Hamano wrote: > Brandon Casey <casey@nrlssc.navy.mil> writes: > >> From: Brandon Casey <drafnel@gmail.com> >> >> 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 <casey@nrlssc.navy.mil> > > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z 2011-05-26 22:51 ` Brandon Casey @ 2011-05-26 23:31 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2011-05-26 23:31 UTC (permalink / raw) To: Brandon Casey; +Cc: peff, git, Brandon Casey Brandon Casey <casey@nrlssc.navy.mil> 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-05-26 23:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-05-26 20:43 [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Brandon Casey 2011-05-26 20:43 ` [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing Brandon Casey 2011-05-26 21:17 ` Jeff King 2011-05-26 21:34 ` Brandon Casey 2011-05-26 21:45 ` [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z Junio C Hamano 2011-05-26 22:51 ` Brandon Casey 2011-05-26 23:31 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.