git status -s -v: no override

git status -s -v: no override

[Jacek Masiulaniec]

Hello git@,

Small git-status argument processing issue:

[jacekm@localhost test]$ git init
Initialized empty Git repository in /private/tmp/test/.git/
[jacekm@localhost test]$ git status -v
# On branch master
#
# Initial commit
#
nothing to commit (create/copy files and use "git add" to track)
[jacekm@localhost test]$ git status -s
[jacekm@localhost test]$ git status -v -s
[jacekm@localhost test]$ git status -s -v
[jacekm@localhost test]$

Things look consistent until the last command: -v does not override -s,
which is unexpected given that -s does override -v.

Jacek

Re: git status -s -v: no override

[Jeff King]

On Fri, Apr 15, 2011 at 10:34:55PM +0100, Jacek Masiulaniec wrote:

> Small git-status argument processing issue:
> 
> [jacekm@localhost test]$ git init
> Initialized empty Git repository in /private/tmp/test/.git/
> [jacekm@localhost test]$ git status -v
> # On branch master
> #
> # Initial commit
> #
> nothing to commit (create/copy files and use "git add" to track)
> [jacekm@localhost test]$ git status -s
> [jacekm@localhost test]$ git status -v -s
> [jacekm@localhost test]$ git status -s -v
> [jacekm@localhost test]$
> 
> Things look consistent until the last command: -v does not override -s,
> which is unexpected given that -s does override -v.

Sort of. I think you are expecting "-v" to mean "use the long output
format", but it doesn't. Instead, "-v" actually indicates that we should
show a diff along with the usual output (in your case, the diff is
empty).

There is no option that means "counteract -s or --porcelain seen earlier
on the command line and use the default long format", which I think is
what you want.

-Peff

Re: git status -s -v: no override

[Jonathan Nieder]

Jeff King wrote:

> There is no option that means "counteract -s or --porcelain seen earlier
> on the command line and use the default long format", which I think is
> what you want.

Doesn't "git status $opts --no-short --no-porcelain" work?

Re: git status -s -v: no override

[Jeff King]

On Fri, Apr 15, 2011 at 07:45:45PM -0500, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > There is no option that means "counteract -s or --porcelain seen earlier
> > on the command line and use the default long format", which I think is
> > what you want.
> 
> Doesn't "git status $opts --no-short --no-porcelain" work?

Hmm. That does work (with either option, or both), but it is somewhat of
an accident. There is an enum specifying the format the user wants. We
hand it to parse-options for those options, telling it that the value is
an int.  Parse-options will treat --no-foo on an int as setting it to 0.

The enumeration list does not have explicit integer values, but does
happen to have the constant for the long format first, which in ANSI C
guarantees it to be 0.

So yeah, it works, but I will admit to being surprised by it. And
certainly as a user, I wouldn't have thought of that.

I think it probably should have been a tristate like:

 --format={long,short,porcelain}

all along, though I don't know that it is a particularly big deal. We
can still do that if we want, and just keep --short and --porcelain as
aliases.

-Peff

[PATCH] status: store format option as an int

[Jonathan Nieder]

It is unsafe to pass a pointer to a value of enumerated type to
OPT_SET_INT (as v1.7.0-rc0~137^2~14, 2009-0905) does, since it might
have the wrong alignment or width (C99 only says "Each enumerated type
shall be compatible with char, a signed integer type, or an unsigned
integer type.  The choice of type is implementation-defined, but shall
be capable of representing the values of all the members of the
enumeration.)

Probably this didn't come up in practice because by default GCC uses
an 'int' to represent small enums unless passed -fshort-enums (except
on certain architectures where -fshort-enums is the default).

Noticed-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:

> Hmm. That does work (with either option, or both), but it is somewhat of
> an accident. There is an enum specifying the format the user wants. We
> hand it to parse-options for those options, telling it that the value is
> an int.

Yikes.  That's an accident waiting to happen.  How about this, to start?

 builtin/commit.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6e32166..b28848d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -94,11 +94,12 @@ static const char *only_include_assumed;
 static struct strbuf message;
 
 static int null_termination;
-static enum {
-	STATUS_FORMAT_LONG,
+enum status_format {
+	STATUS_FORMAT_LONG = 0,
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN
-} status_format = STATUS_FORMAT_LONG;
+};
+static int status_format;
 static int status_show_branch;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
-- 
1.7.5.rc2

Re: [PATCH] status: store format option as an int

[Jonathan Nieder]

Jonathan Nieder wrote:

> It is unsafe to pass a pointer to a value of enumerated type to
> OPT_SET_INT (as v1.7.0-rc0~137^2~14, 2009-0905) does, since it might

Agh, proofreading fail.  For the confused, this is supposed to read as
"(as v1.7.0-rc0~137^2~14, status: refactor format option parsing,
2009-09-05) does".

Re: [PATCH] status: store format option as an int

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Nieder wrote:
>
>> It is unsafe to pass a pointer to a value of enumerated type to
>> OPT_SET_INT (as v1.7.0-rc0~137^2~14, 2009-0905) does, since it might
>
> Agh, proofreading fail.  For the confused, this is supposed to read as
> "(as v1.7.0-rc0~137^2~14, status: refactor format option parsing,
> 2009-09-05) does".

Still ECANTPARSE.  Perhaps the "does" at the end should be inside the same
parentheses pair as "as v1.7.0-..."?

Re: [PATCH] status: store format option as an int

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Jonathan Nieder wrote:

>> Agh, proofreading fail.  For the confused, this is supposed to read as
>> "(as v1.7.0-rc0~137^2~14, status: refactor format option parsing,
>> 2009-09-05) does".
>
> Still ECANTPARSE.  Perhaps the "does" at the end should be inside the same
> parentheses pair as "as v1.7.0-..."?

Yes.  Sorry for the noise.

Re: [PATCH] status: store format option as an int

[Jeff King]

On Sat, Apr 16, 2011 at 12:27:04AM -0500, Jonathan Nieder wrote:

> It is unsafe to pass a pointer to a value of enumerated type to
> OPT_SET_INT (as v1.7.0-rc0~137^2~14, 2009-0905) does, since it might
> have the wrong alignment or width (C99 only says "Each enumerated type
> shall be compatible with char, a signed integer type, or an unsigned
> integer type.  The choice of type is implementation-defined, but shall
> be capable of representing the values of all the members of the
> enumeration.)
> 
> Probably this didn't come up in practice because by default GCC uses
> an 'int' to represent small enums unless passed -fshort-enums (except
> on certain architectures where -fshort-enums is the default).
> 
> Noticed-by: Jeff King <peff@peff.net>

If by "noticed by" you mean "mentioned but was completely unaware of the
significance of what he was saying", then yes. :)

Now that you mention it, though, I was reminded that we had run across
something similar before. And I think it was this:

  http://article.gmane.org/gmane.comp.version-control.git/144858

Your fix looks sane. I don't think we can do anything more clever on the
parse-options side.

-Peff

[PATCH resend] status: store format option as an int

[Jonathan Nieder]

It is unsafe to pass a pointer to a value of enumerated type to
OPT_SET_INT (as v1.7.0-rc0~137^2~14, 2009-09-05, does), since it might
have the wrong alignment or width.  C99 only says "Each enumerated
type shall be compatible with char, a signed integer type, or an
unsigned integer type.  The choice of type is implementation-defined,
but shall be capable of representing the values of all the members of
the enumeration."

Probably this hasn't come up in practice because GCC uses an 'int' to
represent small enums unless passed -fshort-enums, except on certain
architectures where -fshort-enums is the default.

Noticed-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jeff King <peff@peff.net>
---
I last sent this about a month ago and it seemed ok.  The changes
since last time are to the commit message:

 - hopefully it parses as English now
 - adding Jeff's ack

Thanks again, both.

 builtin/commit.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 411d5e4..64808aa 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -108,11 +108,12 @@ static const char *only_include_assumed;
 static struct strbuf message;
 
 static int null_termination;
-static enum {
-	STATUS_FORMAT_LONG,
+enum status_format {
+	STATUS_FORMAT_LONG = 0,
 	STATUS_FORMAT_SHORT,
 	STATUS_FORMAT_PORCELAIN
-} status_format = STATUS_FORMAT_LONG;
+};
+static int status_format;
 static int status_show_branch;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
-- 
1.7.5.1