git status -s -v: no override

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

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

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?

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

Jeff King wrote:

quoted

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

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 [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
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

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".

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 [off-list ref]

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

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 [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
Acked-by: Jeff King <redacted>
---
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