[PATCH (!) 0/2] Fix serious regressions in latest master

[PATCH (!) 0/2] Fix serious regressions in latest master

[Ramkumar Ramachandra]

Hi,

It turns out that status.short and status.branch introduce two very
serious regressions.  This series fixes them.

Thanks.

Ramkumar Ramachandra (2):
  status: really ignore config with --porcelain
  commit: make it work with status.short

 builtin/commit.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
1.8.3.1.550.gd96f26e.dirty

[PATCH 1/2] status: really ignore config with --porcelain

[Ramkumar Ramachandra]

1a22bd3 (Merge branch 'jg/status-config', 2013-06-23) introduced a
serious regression in --porcelain by introducing the configuration
variables status.short and status.branch.  Contrary to its description,
the output of

  $ git status --porcelain

now depends on the configuration variables status.short and
status.branch.  As a result, callers that expect parsable output to be
returned are broken.  For instance, in a repository with submodules with
status.branch and status.short set,

  $ git status

always reports all submodules as containing modified content, even if
they are clean.

One solution to the problem is to turn off s->show_branch in
wt_porcelain_print() just like we turn off other s->* variables, but
that would break callers of --porcelain --branch (in fact, there is such
a caller in t/t7508-status.sh).  Besides, we never said that --porcelain
cannot be combined with other options.  The larger problem is that the
config parser and command-line option parser set the same variables,
making it impossible to determine who set them.

The correct solution is therefore to skip the config parser completely
when --porcelain is given.  Unfortunately, to determine that --porcelain
is given, we have to run the command-line option parser.  Running the
command-line option parser before the config parser is undesirable, as
configuration variables would override options on the command-line.  As
a fair compromise, check that argv[1] is equal to the string
"--porcelain" and skip the config parser in this case.  It is a
compromise, because we expect --porcelain to be specified as the first
argument to status.

On a related note, the command-line parser is not very robust.

  $ git status --short --long
  $ git status --long --long
  $ git status --porcelain --long
  $ git status --long --porcelain
  $ git status --porcelain --short
  $ git status --short --porcelain

all return different outputs.  This bug is left as an exercise for
future contributors to fix.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/commit.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b589ce0..896f002 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1193,7 +1193,12 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	wt_status_prepare(&s);
 	gitmodules_config();
-	git_config(git_status_config, &s);
+
+	if (argc > 1 && !strcmp(argv[1], "--porcelain"))
+		; /* Do not read user configuration */
+	else
+		git_config(git_status_config, &s);
+
 	determine_whence(&s);
 	argc = parse_options(argc, argv, prefix,
 			     builtin_status_options,
-- 
1.8.3.1.550.gd96f26e.dirty

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Contrary to its description, the output of
>
>   $ git status --porcelain
>
> now depends on the configuration variables status.short and
> status.branch.

Ouch, indeed :-(.

> The correct solution is therefore to skip the config parser completely
> when --porcelain is given.  Unfortunately, to determine that --porcelain
> is given, we have to run the command-line option parser.  Running the
> command-line option parser before the config parser is undesirable, as
> configuration variables would override options on the command-line.  As
> a fair compromise, check that argv[1] is equal to the string
> "--porcelain" and skip the config parser in this case.

I really don't like this. If we go for a solution looking explicitely at
argv[], we should at least iterate over it (also not satisfactory
because --porcelain could be the argument of another switch).

I think it's possible to have an actually robust solution, either

* running the CLI parser after, if --porcelain is given, reset the
  effect of the variables. Not very clean because we'd have to reset all
  the variables to their default, and there is a risk of forgetting one.

* Or, running the CLI parser before, but with different variables to
  specify what the command-line says and what will actually be done,
  with something like

  actual_short = 0;
  switch (command_line_short) {
  case yes:
          actual_short = 1;
          break;
  case no:
          actual_short = 0;
          break;
  case unset: /* nothing */
  }
  switch (config_short) {
  // same
  }

> ---
>  builtin/commit.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

No time to contribute one now myself, but this would really deserve a
test.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Ramkumar Ramachandra]

Matthieu Moy wrote:
> [...]

Before we begin, let me say that this series is written with the
emergency-fix mindset, and targets maint.  I didn't spend time
thinking about how to do it best or even add tests.

>> The correct solution is therefore to skip the config parser completely
>> when --porcelain is given.  Unfortunately, to determine that --porcelain
>> is given, we have to run the command-line option parser.  Running the
>> command-line option parser before the config parser is undesirable, as
>> configuration variables would override options on the command-line.  As
>> a fair compromise, check that argv[1] is equal to the string
>> "--porcelain" and skip the config parser in this case.
>
> I really don't like this. If we go for a solution looking explicitely at
> argv[], we should at least iterate over it (also not satisfactory
> because --porcelain could be the argument of another switch).

Yep, that's the compromise.

> I think it's possible to have an actually robust solution, either
>
> * running the CLI parser after, if --porcelain is given, reset the
>   effect of the variables. Not very clean because we'd have to reset all
>   the variables to their default, and there is a risk of forgetting one.

Since it's impossible to determine what effect the CLI parser had on
various variables (some of which are static global), I'm against this
approach.

> * Or, running the CLI parser before, but with different variables to
>   specify what the command-line says and what will actually be done,
>   with something like

Basically, having the CLI parser and the config parser flip two
different sets of variables, so we can discriminate who set what.
What annoys me is that this is the first instance of such a
requirement.

The approach I'm currently tilting towards is extending the
parse-options API to allow parsing one special option early.  I would
argue that this is a good feature that we should have asked for when
we saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup',
2010-12-10).  What do you think?

>>  builtin/commit.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> No time to contribute one now myself, but this would really deserve a
> test.

Yeah, will do as a follow-up.  I'm interested in guarding against such
breakages too :)

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Matthieu Moy]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Matthieu Moy wrote:
>> [...]
>
> Before we begin, let me say that this series is written with the
> emergency-fix mindset, and targets maint.

maint shouldn't be necessary since the breakage hasn't been released. At
worse, we can revert the bad commits now and re-implement them properly
later.

> I didn't spend time thinking about how to do it best or even add
> tests.

No problem.

>> * running the CLI parser after, if --porcelain is given, reset the
>>   effect of the variables. Not very clean because we'd have to reset all
>>   the variables to their default, and there is a risk of forgetting one.
>
> Since it's impossible to determine what effect the CLI parser had on
> various variables (some of which are static global), I'm against this
> approach.

I think you meant "what effect the config parser had". If you meant the
CLI parser, then the guilty commits did not change anything wrt that.

>
>> * Or, running the CLI parser before, but with different variables to
>>   specify what the command-line says and what will actually be done,
>>   with something like
>
> Basically, having the CLI parser and the config parser flip two
> different sets of variables, so we can discriminate who set what.
> What annoys me is that this is the first instance of such a
> requirement.

I don't think it's the first instance, but I can't remember precise
examples.

> The approach I'm currently tilting towards is extending the
> parse-options API to allow parsing one special option early.  I would
> argue that this is a good feature that we should have asked for when
> we saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup',
> 2010-12-10).  What do you think?

That's an option too, yes. But probably not easy to implement :-(.

>>>  builtin/commit.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> No time to contribute one now myself, but this would really deserve a
>> test.
>
> Yeah, will do as a follow-up.  I'm interested in guarding against such
> breakages too :)

Should look like this:

diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 498332c..423e8c4 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1378,6 +1378,11 @@ test_expect_success '"status.branch=true" weaker than "--no-branch"' '
        test_cmp expected_nobranch actual
 '
 
+test_expect_success '"status.branch=true" weaker than "--porcelain"' '
+       git -c status.branch=true status --porcelain >actual &&
+       test_cmp expected_nobranch actual
+'
+
 test_expect_success '"status.branch=false" same as "--no-branch"' '
        git -c status.branch=false status -s >actual &&
        test_cmp expected_nobranch actual


-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> Basically, having the CLI parser and the config parser flip two
>> different sets of variables, so we can discriminate who set what.
>> What annoys me is that this is the first instance of such a
>> requirement.
>
> I don't think it's the first instance, but I can't remember precise
> examples.

"First read config, override with command line" is what we always
do.  One recent workaround with selective exception I can think of
offhand is in diff config parser 6c374008 (diff_opt: track whether
flags have been set explicitly, 2013-05-10), but I am fairly sure
there are others.  An older example is how show_notes_given is used
in the revision traversal machinery to conditionally set show_notes.

>> The approach I'm currently tilting towards is extending the
>> parse-options API to allow parsing one special option early.  I would
>> argue that this is a good feature that we should have asked for when
>> we saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup',
>> 2010-12-10).  What do you think?
>
> That's an option too, yes. But probably not easy to implement :-(.

Isn't it essentially your second option (running the CLI parser
before once, then read config, and then run the CLI parser for
real)?

In any case, I am still not convinced yet that status.short is a
real problem if --porcelain readers trip with "## branchname"
output.  Isn't it that the readers are broken and need fixing?
They should pick out what they care about and ignore the rest.

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>>> Basically, having the CLI parser and the config parser flip two
>>> different sets of variables, so we can discriminate who set what.
>>> What annoys me is that this is the first instance of such a
>>> requirement.
>>
>> I don't think it's the first instance, but I can't remember precise
>> examples.
>
> "First read config, override with command line" is what we always
> do.  One recent workaround with selective exception I can think of
> offhand is in diff config parser 6c374008 (diff_opt: track whether
> flags have been set explicitly, 2013-05-10), but I am fairly sure
> there are others.

That was the one I had in mind.

>>> The approach I'm currently tilting towards is extending the
>>> parse-options API to allow parsing one special option early.  I would
>>> argue that this is a good feature that we should have asked for when
>>> we saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup',
>>> 2010-12-10).  What do you think?
>>
>> That's an option too, yes. But probably not easy to implement :-(.
>
> Isn't it essentially your second option (running the CLI parser
> before once, then read config, and then run the CLI parser for
> real)?

Not really. The first run should be a kind of dry-run, except for the
--porcelain part.

> In any case, I am still not convinced yet that status.short is a
> real problem if --porcelain readers trip with "## branchname"
> output.  Isn't it that the readers are broken and need fixing?

Before introducing status.short, scripts could call "git status
--porcelain" and get some output. They had no way to know whether
something would be added in the future. Now, they can run the same
command and get a different output. To me, that's exactly what we're
trying to avoid in plumbing.

The configuration file here is really meant for the user, not for
scripts. Scripts that want the branch information can use --branch.
Scripts that do not have absolutely nothing to gain in getting this
extra output (only extra parser complexity).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Scripts that want the branch information can use --branch.
> Scripts that do not have absolutely nothing to gain in getting this
> extra output (only extra parser complexity).

OK, I could buy that.

I personally have always _assumed_ that the way the "--porcelain"
output was meant to evolve was to make sure scripts are robust to
ignore what it does not care about, so that we allow people to add
more types of information without breaking the syntax, but script
writers have been assuming otherwise and there is no clearly written
rule in the documentation, so it is too late to enforce it now.

That means after we add more and more other options like --branch
that enrich the output in several years, scripts that want to use
the enriched data need to pass tons of options to get what they
want, which is not very nice, but at least it won't break the status
quo, and "ask for only what you use" is a good principle for scripts
anyway.

Thanks for a sanity check.

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> That means after we add more and more other options like --branch
> that enrich the output in several years, scripts that want to use
> the enriched data need to pass tons of options to get what they
> want, which is not very nice,

Right, but accepting status.branch would not even solve that. Script
writers should not assume that status.branch is set. So, the --branch is
still required.

Actually, that would even be worse, as a script writer who's used to
status.branch being set would not even notice that --branch is missing.
So, he'd still need it, but would have greater chance to miss it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> "First read config, override with command line" is what we always
>> do.  One recent workaround with selective exception I can think of
>> offhand is in diff config parser 6c374008 (diff_opt: track whether
>> flags have been set explicitly, 2013-05-10), but I am fairly sure
>> there are others.
>
> That was the one I had in mind.

If we want to continue that tried-and-proven approach as a
short-term fix, a patch may look like this.

We use "unspecified" value to initialize the main variables
(status_format global and s->show_branch), and also prepare
"from-config" counterpart variables.  After we read both, we figure
out which one should be used, while resetting the main variables to
their default if neither mechanism touched them.

 builtin/commit.c  | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
 t/t7508-status.sh |  5 +++++
 wt-status.c       |  1 +
 3 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 0da944f..a535eb2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -111,12 +111,14 @@ static int show_ignored_in_status;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
-static enum {
+static enum status_format {
 	STATUS_FORMAT_NONE = 0,
 	STATUS_FORMAT_LONG,
 	STATUS_FORMAT_SHORT,
-	STATUS_FORMAT_PORCELAIN
-} status_format;
+	STATUS_FORMAT_PORCELAIN,
+
+	STATUS_FORMAT_UNSPECIFIED
+} status_format = STATUS_FORMAT_UNSPECIFIED;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
@@ -457,6 +459,9 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	case STATUS_FORMAT_PORCELAIN:
 		wt_porcelain_print(s);
 		break;
+	case STATUS_FORMAT_UNSPECIFIED:
+		die("BUG: finalize_deferred_config() should have been called");
+		break;
 	case STATUS_FORMAT_NONE:
 	case STATUS_FORMAT_LONG:
 		wt_status_print(s);
@@ -958,6 +963,33 @@ static const char *read_commit_message(const char *name)
 	return logmsg_reencode(commit, NULL, out_enc);
 }
 
+/*
+ * Enumerate what needs to be propagated when --porcelain
+ * is not in effect here.
+ */
+static struct status_deferred_config {
+	enum status_format status_format;
+	int show_branch;
+} status_deferred_config = {
+	STATUS_FORMAT_UNSPECIFIED,
+	-1 /* unspecified */
+};
+
+static void finalize_deferred_config(struct wt_status *s)
+{
+	int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN);
+
+	if (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)
+		status_format = status_deferred_config.status_format;
+	if (status_format == STATUS_FORMAT_UNSPECIFIED)
+		status_format = STATUS_FORMAT_NONE;
+
+	if (use_deferred_config && s->show_branch < 0)
+		s->show_branch = status_deferred_config.show_branch;
+	if (s->show_branch < 0)
+		s->show_branch = 0;
+}
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const struct option *options,
 				      const char * const usage[],
@@ -968,6 +1000,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	int f = 0;
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
+	finalize_deferred_config(s);
 
 	if (force_author && !strchr(force_author, '>'))
 		force_author = find_author_by_nickname(force_author);
@@ -1112,13 +1145,13 @@ static int git_status_config(const char *k, const char *v, void *cb)
 	}
 	if (!strcmp(k, "status.short")) {
 		if (git_config_bool(k, v))
-			status_format = STATUS_FORMAT_SHORT;
+			status_deferred_config.status_format = STATUS_FORMAT_SHORT;
 		else
-			status_format = STATUS_FORMAT_NONE;
+			status_deferred_config.status_format = STATUS_FORMAT_NONE;
 		return 0;
 	}
 	if (!strcmp(k, "status.branch")) {
-		s->show_branch = git_config_bool(k, v);
+		status_deferred_config.show_branch = git_config_bool(k, v);
 		return 0;
 	}
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
@@ -1163,8 +1196,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		OPT__VERBOSE(&verbose, N_("be verbose")),
 		OPT_SET_INT('s', "short", &status_format,
 			    N_("show status concisely"), STATUS_FORMAT_SHORT),
-		OPT_BOOLEAN('b', "branch", &s.show_branch,
-			    N_("show branch information")),
+		OPT_BOOL('b', "branch", &s.show_branch,
+			 N_("show branch information")),
 		OPT_SET_INT(0, "porcelain", &status_format,
 			    N_("machine-readable output"),
 			    STATUS_FORMAT_PORCELAIN),
@@ -1197,6 +1230,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			     builtin_status_options,
 			     builtin_status_usage, 0);
 	finalize_colopts(&s.colopts, -1);
+	finalize_deferred_config(&s);
 
 	if (s.null_termination) {
 		if (status_format == STATUS_FORMAT_NONE)
@@ -1232,6 +1266,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	case STATUS_FORMAT_PORCELAIN:
 		wt_porcelain_print(&s);
 		break;
+	case STATUS_FORMAT_UNSPECIFIED:
+		die("BUG: finalize_deferred_config() should have been called");
+		break;
 	case STATUS_FORMAT_NONE:
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
@@ -1400,7 +1437,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN(0, "dry-run", &dry_run, N_("show what would be committed")),
 		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
 			    STATUS_FORMAT_SHORT),
-		OPT_BOOLEAN(0, "branch", &s.show_branch, N_("show branch information")),
+		OPT_BOOL(0, "branch", &s.show_branch, N_("show branch information")),
 		OPT_SET_INT(0, "porcelain", &status_format,
 			    N_("machine-readable output"), STATUS_FORMAT_PORCELAIN),
 		OPT_SET_INT(0, "long", &status_format,
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 498332c..ac3d0fe 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1378,6 +1378,11 @@ test_expect_success '"status.branch=true" weaker than "--no-branch"' '
 	test_cmp expected_nobranch actual
 '
 
+test_expect_success '"status.branch=true" weaker than "--porcelain"' '
+       git -c status.branch=true status --porcelain >actual &&
+       test_cmp expected_nobranch actual
+'
+
 test_expect_success '"status.branch=false" same as "--no-branch"' '
 	git -c status.branch=false status -s >actual &&
 	test_cmp expected_nobranch actual
diff --git a/wt-status.c b/wt-status.c
index bf84a86..6778755 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -127,6 +127,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->change.strdup_strings = 1;
 	s->untracked.strdup_strings = 1;
 	s->ignored.strdup_strings = 1;
+	s->show_branch = -1;  /* unspecified */
 }
 
 static void wt_status_print_unmerged_header(struct wt_status *s)

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If we want to continue that tried-and-proven approach as a
> short-term fix, a patch may look like this.

I don't like this special-casing for show_branch at all.  What is the
problem with skipping branch configuration altogether and going
straight to diff-ui configuration, like I suggested earlier?

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> If we want to continue that tried-and-proven approach as a
>> short-term fix, a patch may look like this.
>
> I don't like this special-casing for show_branch at all.

The patch does not special-case show_branch at all, in that I would
expect that people over time will want to add more and more
configuration not to type short and sweet command line options.  The
primary value of the patch is to illustrate how we would ignore the
ones we do not want when we want to still call git_status_config().
It lays a groundwork for doing so, using one single variable we
happen to know about today as an example.

> What is the
> problem with skipping branch configuration altogether and going
> straight to diff-ui configuration, like I suggested earlier?

Why diff-*UI*-config, not diff_basic or even default_core?

I actually find that alternative approach a lot nicer, and I wish
the world were that simple.

One thing that complicates the matter is that "--porcelain" output
is affected by "status.showuntrackedfiles" setting, whose parser
dates only back to June 2008, while --porcelain came much later in
Sep 2009.  So we have close to four years worth of user scripts that
read from --porcelain and expects no untracked paths shown in a
repository with that configration set.  If we care about not
breaking people's scripts, we need to somehow arrange the option to
keep honoring status.showuntrackedfiles setting, which may mean that
we cannot just say "let's bypass git_status_config() and be done
with it".

So...

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> We use "unspecified" value to initialize the main variables
> (status_format global and s->show_branch), and also prepare
> "from-config" counterpart variables.  After we read both, we figure
> out which one should be used, while resetting the main variables to
> their default if neither mechanism touched them.
>
>  builtin/commit.c  | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  t/t7508-status.sh |  5 +++++
>  wt-status.c       |  1 +
>  3 files changed, 52 insertions(+), 9 deletions(-)

At least that needs this on top to deal with the late defaulting of
"-z".

We know we do not want deferred configuration values when the
command line says "--porcelain" or "-z".  And after handling the
"-z" to set status_format, we apply config only if we want deferred
configuration and command line left them unspecified.  And finally
we apply the default if both config and command line left them
unspecified.

 builtin/commit.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a535eb2..6f8cb04 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -977,7 +977,16 @@ static struct status_deferred_config {
 
 static void finalize_deferred_config(struct wt_status *s)
 {
-	int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN);
+	int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
+				   !s->null_termination);
+
+	if (s->null_termination) {
+		if (status_format == STATUS_FORMAT_NONE ||
+		    status_format == STATUS_FORMAT_UNSPECIFIED)
+			status_format = STATUS_FORMAT_PORCELAIN;
+		else if (status_format == STATUS_FORMAT_LONG)
+			die(_("--long and -z are incompatible"));
+	}
 
 	if (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)
 		status_format = status_deferred_config.status_format;
@@ -1085,12 +1094,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (all && argc > 0)
 		die(_("Paths with -a does not make sense."));
 
-	if (s->null_termination) {
-		if (status_format == STATUS_FORMAT_NONE)
-			status_format = STATUS_FORMAT_PORCELAIN;
-		else if (status_format == STATUS_FORMAT_LONG)
-			die(_("--long and -z are incompatible"));
-	}
 	if (status_format != STATUS_FORMAT_NONE)
 		dry_run = 1;
 
@@ -1232,13 +1235,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	finalize_colopts(&s.colopts, -1);
 	finalize_deferred_config(&s);
 
-	if (s.null_termination) {
-		if (status_format == STATUS_FORMAT_NONE)
-			status_format = STATUS_FORMAT_PORCELAIN;
-		else if (status_format == STATUS_FORMAT_LONG)
-			die(_("--long and -z are incompatible"));
-	}
-
 	handle_untracked_files_arg(&s);
 	if (show_ignored_in_status)
 		s.show_ignored_files = 1;

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Jeff King]

On Mon, Jun 24, 2013 at 11:16:56AM -0700, Junio C Hamano wrote:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 0da944f..a535eb2 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -111,12 +111,14 @@ static int show_ignored_in_status;
>  static const char *only_include_assumed;
>  static struct strbuf message = STRBUF_INIT;
>  
> -static enum {
> +static enum status_format {
>  	STATUS_FORMAT_NONE = 0,
>  	STATUS_FORMAT_LONG,
>  	STATUS_FORMAT_SHORT,
> -	STATUS_FORMAT_PORCELAIN
> -} status_format;
> +	STATUS_FORMAT_PORCELAIN,
> +
> +	STATUS_FORMAT_UNSPECIFIED
> +} status_format = STATUS_FORMAT_UNSPECIFIED;

I'm not sure why you need to add UNSPECIFIED here; the point of NONE is
that no format had yet been specified.

It looks like the only difference is that finalize_deferred_config
converts unspecified into NONE. But I think you can just use NONE for
both cases, and drop this hunk:

>  static int opt_parse_m(const struct option *opt, const char *arg, int unset)
>  {
> @@ -457,6 +459,9 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
>  	case STATUS_FORMAT_PORCELAIN:
>  		wt_porcelain_print(s);
>  		break;
> +	case STATUS_FORMAT_UNSPECIFIED:
> +		die("BUG: finalize_deferred_config() should have been called");
> +		break;
>  	case STATUS_FORMAT_NONE:
>  	case STATUS_FORMAT_LONG:
>  		wt_status_print(s);

You lose the assertion that finalize_deferred_config has been called,
but I think the resulting code would be simpler, as it drops this
die("BUG") state entirely. Am I missing something?

-Peff

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You lose the assertion that finalize_deferred_config has been called,
> but I think the resulting code would be simpler, as it drops this
> die("BUG") state entirely. Am I missing something?

Probably not.  Depending on "-z", NONE is sometimes converted to
PORCELAIN and sometimes left as-is.  I originally wanted to keep the
"unspecified" state as long as possible so that this deferred config
logic and the "-z" default logic can be kept separate.

The final patch ended up folding that "-z" default logic into the
same function, so it probably is saner to remove UNSPECIFIED.

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> You lose the assertion that finalize_deferred_config has been called,
>> but I think the resulting code would be simpler, as it drops this
>> die("BUG") state entirely. Am I missing something?
>
> Probably not.  Depending on "-z", NONE is sometimes converted to
> PORCELAIN and sometimes left as-is.  I originally wanted to keep the
> "unspecified" state as long as possible so that this deferred config
> logic and the "-z" default logic can be kept separate.
>
> The final patch ended up folding that "-z" default logic into the
> same function, so it probably is saner to remove UNSPECIFIED.

Actually, the code needs to be able to differentiate between

	git status --no-short
        git status

the former telling us explicitly to defeat status.short while the
latter telling us to use whatever random value we happen to have in
the configuration.  Initializing the variable to UNSPECIFIED is one
way to achieve that, as the former will explicitly set it to NONE
while the latter will leave it UNSPECIFIED when the command line
parsing finishes.

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Jeff King]

On Fri, Jun 28, 2013 at 10:37:26AM -0700, Junio C Hamano wrote:

> > The final patch ended up folding that "-z" default logic into the
> > same function, so it probably is saner to remove UNSPECIFIED.
> 
> Actually, the code needs to be able to differentiate between
> 
> 	git status --no-short
>         git status
> 
> the former telling us explicitly to defeat status.short while the
> latter telling us to use whatever random value we happen to have in
> the configuration.  Initializing the variable to UNSPECIFIED is one
> way to achieve that, as the former will explicitly set it to NONE
> while the latter will leave it UNSPECIFIED when the command line
> parsing finishes.

Hmm. I would have thought --no-short would just set it to LONG. That is,
we are no longer NONE at that point, as the user has told us something
on the command line. So we are whatever --no-short is, which is LONG.

But I guess that would wreck

  git status --no-short -z

which currently defaults to porcelain. Which, to be honest, seems a
little crazy to me, but I guess there is no reason to break it.

I am just trying to prevent the future maintenance confusion where a
reader of the code says "Huh? What is the difference between NONE and
UNSPECIFIED?"

-Peff

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm. I would have thought --no-short would just set it to LONG. That is,
> we are no longer NONE at that point, as the user has told us something
> on the command line. So we are whatever --no-short is, which is LONG.
>
> But I guess that would wreck
>
>   git status --no-short -z
>
> which currently defaults to porcelain. Which, to be honest, seems a
> little crazy to me, but I guess there is no reason to break it.
>
> I am just trying to prevent the future maintenance confusion where a
> reader of the code says "Huh? What is the difference between NONE and
> UNSPECIFIED?"

Yeah, I share your sentiment, but I did not think of a better way to
do this without unnecessarily changing behaviour.

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> In any case, I am still not convinced yet that status.short is a
> real problem if --porcelain readers trip with "## branchname"
> output.  Isn't it that the readers are broken and need fixing?

If you're going to read the configuration and then scramble to reset
it in wt_porcelain_print(), why did you read the configuration in the
first place?  Should column.branch, status.submodulesummary,
status.showuntrackedfiles affect the --porcelain output too?  Isn't
your expectation of my parser unreasonable?

I will argue that --porcelain should skip the branch config, and drop
down one level lower directly: to the diff-ui configuration.

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> I really don't like this. If we go for a solution looking explicitely at
> argv[], we should at least iterate over it (also not satisfactory
> because --porcelain could be the argument of another switch).

Ram, thanks for a report.

I won't comment on what is the correct way to see if "--porcelain"
is given by the caller before I have enough time to think about it,
but we did read configurattion even when "--porcelain"is given
before the topic was merged, and I think it was done for a good
reason.

Configuration related to the output format (like color=always) must
be ignored under "--porcelain", but if we do not read core-ish
configuration variables (e.g. core.crlf) that affect the logic to
list what is changed what is not, we would not give the right
result, no?

So checking "--porcelain" option and skipping configuration may not
be a solution but merely trading one regression with another.

For now, I'll revert the merge and see if people can come up with a
reasonable way forward.  My knee-jerk reaction is that, because the
"--porcelain" output was designed to be extensible and scripts
reading from it is expected to ignore what it does not understand,
if the setting of status.branch is a problem, the reading side is
buggy and needs to be fixed.

Do we have in-core reader that does not behave well when one or both
of these configuration variables are set (perhaps something related
to submodule?)???

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> My knee-jerk reaction is that, because the
> "--porcelain" output was designed to be extensible and scripts
> reading from it is expected to ignore what it does not understand,
> if the setting of status.branch is a problem, the reading side is
> buggy and needs to be fixed.

It is extensible in the sense that the caller can provide more
command-line options to get more output (i.e. say --branch --porcelain),
but providing different results for the same call because of the
configuration file is broken IMHO.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/2] status: really ignore config with --porcelain

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Configuration related to the output format (like color=always) must
> be ignored under "--porcelain", but if we do not read core-ish
> configuration variables (e.g. core.crlf) that affect the logic to
> list what is changed what is not, we would not give the right
> result, no?

Ah, ofcourse.  My stupidity.

>  My knee-jerk reaction is that, because the
> "--porcelain" output was designed to be extensible and scripts
> reading from it is expected to ignore what it does not understand,
> if the setting of status.branch is a problem, the reading side is
> buggy and needs to be fixed.

Are you 100% sure about this?

--porcelain::
	Give the output in an easy-to-parse format for scripts.
	This is similar to the short output, but will remain stable
	across Git versions and regardless of user configuration. See
	below for details.

status.branch is an exception, no doubt: it just adds one line clearly
prefixed with "##" to the beginning, and any script can ignore it easily.

> Do we have in-core reader that does not behave well when one or both
> of these configuration variables are set (perhaps something related
> to submodule?)???

Yeah, the submodule.c parser is very naively written (see
submodule.c:737), and I can fix it.  But the bigger question still
remains: if we have such a non-robust parser in git.git itself,
wouldn't you expect external parsers to be even worse?

[PATCH 2/2] commit: make it work with status.short

[Ramkumar Ramachandra]

50e4f75 (status: introduce status.short to enable --short by default,
2013-06-11) introduced a regression in git commit; it is now impossible
to commit with status.short set.

This happens because commit internally runs run_status() to set
s->commitable and determine whether or not there is something to
commit.  The problem arises from the fact that only STATUS_FORMAT_NONE
(or STATUS_FORMAT_LONG) is equipped to set s->commitable.
7c9f7038 (commit: support alternate status formats, 2009-09-05) clearly
states that --short and --porcelain imply --dry-run and are therefore
only intended for display purposes.

The bigger problem is that it is impossible to differentiate between a
status_format set by the command-line option parser versus that set by
the config parser.  So these two are exactly equivalent:

  $ git -c status.short=true commit
  $ git commit --short

To alleviate this problem, clear status_format as soon as the config
parser has finished its work to nullify the effect of parsing
status.short, and get commit to work again.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/commit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 896f002..dc5ed7d 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1448,6 +1448,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	wt_status_prepare(&s);
 	gitmodules_config();
 	git_config(git_commit_config, &s);
+	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
 	determine_whence(&s);
 	s.colopts = 0;
 
-- 
1.8.3.1.550.gd96f26e.dirty

Re: [PATCH 2/2] commit: make it work with status.short

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 50e4f75 (status: introduce status.short to enable --short by default,
> 2013-06-11) introduced a regression in git commit; it is now impossible
> to commit with status.short set.
>
> This happens because commit internally runs run_status() to set
> s->commitable and determine whether or not there is something to
> commit.  The problem arises from the fact that only STATUS_FORMAT_NONE
> (or STATUS_FORMAT_LONG) is equipped to set s->commitable.
> 7c9f7038 (commit: support alternate status formats, 2009-09-05) clearly
> states that --short and --porcelain imply --dry-run and are therefore
> only intended for display purposes.
>
> The bigger problem is that it is impossible to differentiate between a
> status_format set by the command-line option parser versus that set by
> the config parser.  So these two are exactly equivalent:
>
>   $ git -c status.short=true commit
>   $ git commit --short

Thanks for a report.  I think the analysis above is correct, and if
we want to ignore status.short but still want to honor --short from
the command line, your patch is a way to go.

As I said in the other message, I'll revert the merge of the topic
branch from 'master' for now, and queue this on top of the topic so
that we will have the preventive fix when the topic is in a better
shape for the other possible breakage on the "--branch" thing.

Having said that, even before the merge of that status.short
(e.g. v1.8.3), "git commit --short" did not work and that was
deliberate only because "git commit --dry-run" has long been an
equivalent for "git status", "--short/-z/--porcelain" were options
for "status", and therefore these options were made to imply
"--dry-run".

I have to wonder if that is a sane thing in the first place, now
that it has been quite a while since "git status" has become
different from "git commit --dry-run".

That is, "git commit --short/--porcelain/-z" has these three
possibilities:

 - work (ignoring these options);

 - work (showing the template in some kind of "short" format); or

 - error out (clearly indicating that we did *not* make a commit).

and what we currently do is closest to the last (but we do not say
we did not create a commit).  In the longer term for Git 2.0, we may
want to change it to do one of the former two.

Re: [PATCH 2/2] commit: make it work with status.short

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> As I said in the other message, I'll revert the merge of the topic
> branch from 'master' for now, and queue this on top of the topic so
> that we will have the preventive fix when the topic is in a better
> shape for the other possible breakage on the "--branch" thing.

Thanks, a quick revert is probably the best way to go.

> That is, "git commit --short/--porcelain/-z" has these three
> possibilities:
>
>  - work (ignoring these options);
>
>  - work (showing the template in some kind of "short" format); or
>
>  - error out (clearly indicating that we did *not* make a commit).

Actually, I had a different idea: to make short (and porcelain, by
extension) set s->commitable correctly.  From a quick analysis, it
shouldn't be a hard problem.  Then, we can make

  $ git commit

display short-form status when there is nothing to commit.