threads / patch / 27474

patch, 2 partst7508: demonstrate status's failure to use --porcelain format with -z

Subject: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z

## tl;dr

7 messages between May 26, 2011 and May 26, 2011. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Brandon Casey· May 26, 2011, 20:43 UTC · lore
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(-)
Show changes to t/t7508-status.sh +7 −0
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· May 26, 2011, 20:43 UTC · re: Brandon Casey · lore

[PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing

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(-)
Show changes to 2 files +5 −4

builtin/commit.c, t/t7508-status.sh

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· May 26, 2011, 21:17 UTC · re: Brandon Casey · lore

Re: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing

On Thu, May 26, 2011 at 01:43:21PM -0700, Brandon Casey wrote:
Show 6 quoted lines
> '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
Brandon Casey· May 26, 2011, 21:34 UTC · re: Jeff King · lore

Re: [PATCH 2/2] builtin/commit.c: set status_format _after_ option parsing

On 05/26/2011 04:17 PM, Jeff King wrote:
Show 14 quoted lines
> 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· May 26, 2011, 21:45 UTC · re: Brandon Casey · lore

Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z

Brandon Casey <casey@nrlssc.navy.mil> writes:
Show 8 quoted lines
> 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?

Brandon Casey· May 26, 2011, 22:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z

On 05/26/2011 04:45 PM, Junio C Hamano wrote:
Show 14 quoted lines
> 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.
Show 6 quoted lines
> 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.
Show 6 quoted lines
>            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· May 26, 2011, 23:31 UTC · re: Brandon Casey · lore

Re: [PATCH 1/2] t7508: demonstrate status's failure to use --porcelain format with -z

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.

← back to recent threads