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

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

[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

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

[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

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

[Jeff King]

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

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

[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

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

[Junio C Hamano]

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?

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

[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

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

[Junio C Hamano]

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.