{"thread":{"id":"34264","subject":"[PATCH (!) 0/2] Fix serious regressions in latest master","startedAt":"2013-06-24T12:45:10Z","lastAt":"2013-06-28T20:15:09Z","messageCount":25,"participants":["Ramkumar Ramachandra","Matthieu Moy","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"221788","messageId":"1372077912-18625-1-git-send-email-artagnon@gmail.com","threadId":"34264","inReplyTo":null,"subject":"[PATCH (!) 0/2] Fix serious regressions in latest master","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T12:45:10Z","receivedAt":"2013-06-24T12:45:10Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nIt turns out that status.short and status.branch introduce two very\nserious regressions.  This series fixes them.\n\nThanks.\n\nRamkumar Ramachandra (2):\n  status: really ignore config with --porcelain\n  commit: make it work with status.short\n\n builtin/commit.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\n-- \n1.8.3.1.550.gd96f26e.dirty\n"},{"id":"221789","messageId":"1372077912-18625-2-git-send-email-artagnon@gmail.com","threadId":"34264","inReplyTo":"1372077912-18625-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/2] status: really ignore config with --porcelain","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T12:45:11Z","receivedAt":"2013-06-24T12:45:11Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"1a22bd3 (Merge branch 'jg/status-config', 2013-06-23) introduced a\nserious regression in --porcelain by introducing the configuration\nvariables status.short and status.branch.  Contrary to its description,\nthe output of\n\n  $ git status --porcelain\n\nnow depends on the configuration variables status.short and\nstatus.branch.  As a result, callers that expect parsable output to be\nreturned are broken.  For instance, in a repository with submodules with\nstatus.branch and status.short set,\n\n  $ git status\n\nalways reports all submodules as containing modified content, even if\nthey are clean.\n\nOne solution to the problem is to turn off s->show_branch in\nwt_porcelain_print() just like we turn off other s->* variables, but\nthat would break callers of --porcelain --branch (in fact, there is such\na caller in t/t7508-status.sh).  Besides, we never said that --porcelain\ncannot be combined with other options.  The larger problem is that the\nconfig parser and command-line option parser set the same variables,\nmaking it impossible to determine who set them.\n\nThe correct solution is therefore to skip the config parser completely\nwhen --porcelain is given.  Unfortunately, to determine that --porcelain\nis given, we have to run the command-line option parser.  Running the\ncommand-line option parser before the config parser is undesirable, as\nconfiguration variables would override options on the command-line.  As\na fair compromise, check that argv[1] is equal to the string\n\"--porcelain\" and skip the config parser in this case.  It is a\ncompromise, because we expect --porcelain to be specified as the first\nargument to status.\n\nOn a related note, the command-line parser is not very robust.\n\n  $ git status --short --long\n  $ git status --long --long\n  $ git status --porcelain --long\n  $ git status --long --porcelain\n  $ git status --porcelain --short\n  $ git status --short --porcelain\n\nall return different outputs.  This bug is left as an exercise for\nfuture contributors to fix.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/commit.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex b589ce0..896f002 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1193,7 +1193,12 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \n \twt_status_prepare(&s);\n \tgitmodules_config();\n-\tgit_config(git_status_config, &s);\n+\n+\tif (argc > 1 && !strcmp(argv[1], \"--porcelain\"))\n+\t\t; /* Do not read user configuration */\n+\telse\n+\t\tgit_config(git_status_config, &s);\n+\n \tdetermine_whence(&s);\n \targc = parse_options(argc, argv, prefix,\n \t\t\t     builtin_status_options,\n-- \n1.8.3.1.550.gd96f26e.dirty\n"},{"id":"221790","messageId":"1372077912-18625-3-git-send-email-artagnon@gmail.com","threadId":"34264","inReplyTo":"1372077912-18625-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/2] commit: make it work with status.short","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T12:45:12Z","receivedAt":"2013-06-24T12:45:12Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"50e4f75 (status: introduce status.short to enable --short by default,\n2013-06-11) introduced a regression in git commit; it is now impossible\nto commit with status.short set.\n\nThis happens because commit internally runs run_status() to set\ns->commitable and determine whether or not there is something to\ncommit.  The problem arises from the fact that only STATUS_FORMAT_NONE\n(or STATUS_FORMAT_LONG) is equipped to set s->commitable.\n7c9f7038 (commit: support alternate status formats, 2009-09-05) clearly\nstates that --short and --porcelain imply --dry-run and are therefore\nonly intended for display purposes.\n\nThe bigger problem is that it is impossible to differentiate between a\nstatus_format set by the command-line option parser versus that set by\nthe config parser.  So these two are exactly equivalent:\n\n  $ git -c status.short=true commit\n  $ git commit --short\n\nTo alleviate this problem, clear status_format as soon as the config\nparser has finished its work to nullify the effect of parsing\nstatus.short, and get commit to work again.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n builtin/commit.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 896f002..dc5ed7d 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1448,6 +1448,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \twt_status_prepare(&s);\n \tgitmodules_config();\n \tgit_config(git_commit_config, &s);\n+\tstatus_format = STATUS_FORMAT_NONE; /* Ignore status.short */\n \tdetermine_whence(&s);\n \ts.colopts = 0;\n \n-- \n1.8.3.1.550.gd96f26e.dirty\n"},{"id":"221792","messageId":"vpqhagnwraj.fsf@anie.imag.fr","threadId":"34264","inReplyTo":"1372077912-18625-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-24T13:51:16Z","receivedAt":"2013-06-24T13:51:16Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Contrary to its description, the output of\n>\n>   $ git status --porcelain\n>\n> now depends on the configuration variables status.short and\n> status.branch.\n\nOuch, indeed :-(.\n\n> The correct solution is therefore to skip the config parser completely\n> when --porcelain is given.  Unfortunately, to determine that --porcelain\n> is given, we have to run the command-line option parser.  Running the\n> command-line option parser before the config parser is undesirable, as\n> configuration variables would override options on the command-line.  As\n> a fair compromise, check that argv[1] is equal to the string\n> \"--porcelain\" and skip the config parser in this case.\n\nI really don't like this. If we go for a solution looking explicitely at\nargv[], we should at least iterate over it (also not satisfactory\nbecause --porcelain could be the argument of another switch).\n\nI think it's possible to have an actually robust solution, either\n\n* running the CLI parser after, if --porcelain is given, reset the\n  effect of the variables. Not very clean because we'd have to reset all\n  the variables to their default, and there is a risk of forgetting one.\n\n* Or, running the CLI parser before, but with different variables to\n  specify what the command-line says and what will actually be done,\n  with something like\n\n  actual_short = 0;\n  switch (command_line_short) {\n  case yes:\n          actual_short = 1;\n          break;\n  case no:\n          actual_short = 0;\n          break;\n  case unset: /* nothing */\n  }\n  switch (config_short) {\n  // same\n  }\n\n> ---\n>  builtin/commit.c | 7 ++++++-\n>  1 file changed, 6 insertions(+), 1 deletion(-)\n\nNo time to contribute one now myself, but this would really deserve a\ntest.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"221793","messageId":"CALkWK0=F_i95S+53eZmOAJtA+jG=jvi5-sDc3BgW3rNQo=n3Ng@mail.gmail.com","threadId":"34264","inReplyTo":"vpqhagnwraj.fsf@anie.imag.fr","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T14:05:39Z","receivedAt":"2013-06-24T14:05:39Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Matthieu Moy wrote:\n> [...]\n\nBefore we begin, let me say that this series is written with the\nemergency-fix mindset, and targets maint.  I didn't spend time\nthinking about how to do it best or even add tests.\n\n>> The correct solution is therefore to skip the config parser completely\n>> when --porcelain is given.  Unfortunately, to determine that --porcelain\n>> is given, we have to run the command-line option parser.  Running the\n>> command-line option parser before the config parser is undesirable, as\n>> configuration variables would override options on the command-line.  As\n>> a fair compromise, check that argv[1] is equal to the string\n>> \"--porcelain\" and skip the config parser in this case.\n>\n> I really don't like this. If we go for a solution looking explicitely at\n> argv[], we should at least iterate over it (also not satisfactory\n> because --porcelain could be the argument of another switch).\n\nYep, that's the compromise.\n\n> I think it's possible to have an actually robust solution, either\n>\n> * running the CLI parser after, if --porcelain is given, reset the\n>   effect of the variables. Not very clean because we'd have to reset all\n>   the variables to their default, and there is a risk of forgetting one.\n\nSince it's impossible to determine what effect the CLI parser had on\nvarious variables (some of which are static global), I'm against this\napproach.\n\n> * Or, running the CLI parser before, but with different variables to\n>   specify what the command-line says and what will actually be done,\n>   with something like\n\nBasically, having the CLI parser and the config parser flip two\ndifferent sets of variables, so we can discriminate who set what.\nWhat annoys me is that this is the first instance of such a\nrequirement.\n\nThe approach I'm currently tilting towards is extending the\nparse-options API to allow parsing one special option early.  I would\nargue that this is a good feature that we should have asked for when\nwe saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup',\n2010-12-10).  What do you think?\n\n>>  builtin/commit.c | 7 ++++++-\n>>  1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> No time to contribute one now myself, but this would really deserve a\n> test.\n\nYeah, will do as a follow-up.  I'm interested in guarding against such\nbreakages too :)\n"},{"id":"221799","messageId":"vpqhagnv9xq.fsf@anie.imag.fr","threadId":"34264","inReplyTo":"CALkWK0=F_i95S+53eZmOAJtA+jG=jvi5-sDc3BgW3rNQo=n3Ng@mail.gmail.com","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-24T14:51:29Z","receivedAt":"2013-06-24T14:51:29Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Matthieu Moy wrote:\n>> [...]\n>\n> Before we begin, let me say that this series is written with the\n> emergency-fix mindset, and targets maint.\n\nmaint shouldn't be necessary since the breakage hasn't been released. At\nworse, we can revert the bad commits now and re-implement them properly\nlater.\n\n> I didn't spend time thinking about how to do it best or even add\n> tests.\n\nNo problem.\n\n>> * running the CLI parser after, if --porcelain is given, reset the\n>>   effect of the variables. Not very clean because we'd have to reset all\n>>   the variables to their default, and there is a risk of forgetting one.\n>\n> Since it's impossible to determine what effect the CLI parser had on\n> various variables (some of which are static global), I'm against this\n> approach.\n\nI think you meant \"what effect the config parser had\". If you meant the\nCLI parser, then the guilty commits did not change anything wrt that.\n\n>\n>> * Or, running the CLI parser before, but with different variables to\n>>   specify what the command-line says and what will actually be done,\n>>   with something like\n>\n> Basically, having the CLI parser and the config parser flip two\n> different sets of variables, so we can discriminate who set what.\n> What annoys me is that this is the first instance of such a\n> requirement.\n\nI don't think it's the first instance, but I can't remember precise\nexamples.\n\n> The approach I'm currently tilting towards is extending the\n> parse-options API to allow parsing one special option early.  I would\n> argue that this is a good feature that we should have asked for when\n> we saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup',\n> 2010-12-10).  What do you think?\n\nThat's an option too, yes. But probably not easy to implement :-(.\n\n>>>  builtin/commit.c | 7 ++++++-\n>>>  1 file changed, 6 insertions(+), 1 deletion(-)\n>>\n>> No time to contribute one now myself, but this would really deserve a\n>> test.\n>\n> Yeah, will do as a follow-up.  I'm interested in guarding against such\n> breakages too :)\n\nShould look like this:\n\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 498332c..423e8c4 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1378,6 +1378,11 @@ test_expect_success '\"status.branch=true\" weaker than \"--no-branch\"' '\n        test_cmp expected_nobranch actual\n '\n \n+test_expect_success '\"status.branch=true\" weaker than \"--porcelain\"' '\n+       git -c status.branch=true status --porcelain >actual &&\n+       test_cmp expected_nobranch actual\n+'\n+\n test_expect_success '\"status.branch=false\" same as \"--no-branch\"' '\n        git -c status.branch=false status -s >actual &&\n        test_cmp expected_nobranch actual\n\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"221798","messageId":"7v8v1zd0du.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"vpqhagnwraj.fsf@anie.imag.fr","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T14:55:09Z","receivedAt":"2013-06-24T14:55:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> I really don't like this. If we go for a solution looking explicitely at\n> argv[], we should at least iterate over it (also not satisfactory\n> because --porcelain could be the argument of another switch).\n\nRam, thanks for a report.\n\nI won't comment on what is the correct way to see if \"--porcelain\"\nis given by the caller before I have enough time to think about it,\nbut we did read configurattion even when \"--porcelain\"is given\nbefore the topic was merged, and I think it was done for a good\nreason.\n\nConfiguration related to the output format (like color=always) must\nbe ignored under \"--porcelain\", but if we do not read core-ish\nconfiguration variables (e.g. core.crlf) that affect the logic to\nlist what is changed what is not, we would not give the right\nresult, no?\n\nSo checking \"--porcelain\" option and skipping configuration may not\nbe a solution but merely trading one regression with another.\n\nFor now, I'll revert the merge and see if people can come up with a\nreasonable way forward.  My knee-jerk reaction is that, because the\n\"--porcelain\" output was designed to be extensible and scripts\nreading from it is expected to ignore what it does not understand,\nif the setting of status.branch is a problem, the reading side is\nbuggy and needs to be fixed.\n\nDo we have in-core reader that does not behave well when one or both\nof these configuration variables are set (perhaps something related\nto submodule?)???\n"},{"id":"221800","messageId":"vpqa9mfv9bs.fsf@anie.imag.fr","threadId":"34264","inReplyTo":"7v8v1zd0du.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-24T15:04:39Z","receivedAt":"2013-06-24T15:04:39Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> My knee-jerk reaction is that, because the\n> \"--porcelain\" output was designed to be extensible and scripts\n> reading from it is expected to ignore what it does not understand,\n> if the setting of status.branch is a problem, the reading side is\n> buggy and needs to be fixed.\n\nIt is extensible in the sense that the caller can provide more\ncommand-line options to get more output (i.e. say --branch --porcelain),\nbut providing different results for the same call because of the\nconfiguration file is broken IMHO.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"221801","messageId":"7v38s7czc9.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"1372077912-18625-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/2] commit: make it work with status.short","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T15:17:42Z","receivedAt":"2013-06-24T15:17:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> 50e4f75 (status: introduce status.short to enable --short by default,\n> 2013-06-11) introduced a regression in git commit; it is now impossible\n> to commit with status.short set.\n>\n> This happens because commit internally runs run_status() to set\n> s->commitable and determine whether or not there is something to\n> commit.  The problem arises from the fact that only STATUS_FORMAT_NONE\n> (or STATUS_FORMAT_LONG) is equipped to set s->commitable.\n> 7c9f7038 (commit: support alternate status formats, 2009-09-05) clearly\n> states that --short and --porcelain imply --dry-run and are therefore\n> only intended for display purposes.\n>\n> The bigger problem is that it is impossible to differentiate between a\n> status_format set by the command-line option parser versus that set by\n> the config parser.  So these two are exactly equivalent:\n>\n>   $ git -c status.short=true commit\n>   $ git commit --short\n\nThanks for a report.  I think the analysis above is correct, and if\nwe want to ignore status.short but still want to honor --short from\nthe command line, your patch is a way to go.\n\nAs I said in the other message, I'll revert the merge of the topic\nbranch from 'master' for now, and queue this on top of the topic so\nthat we will have the preventive fix when the topic is in a better\nshape for the other possible breakage on the \"--branch\" thing.\n\nHaving said that, even before the merge of that status.short\n(e.g. v1.8.3), \"git commit --short\" did not work and that was\ndeliberate only because \"git commit --dry-run\" has long been an\nequivalent for \"git status\", \"--short/-z/--porcelain\" were options\nfor \"status\", and therefore these options were made to imply\n\"--dry-run\".\n\nI have to wonder if that is a sane thing in the first place, now\nthat it has been quite a while since \"git status\" has become\ndifferent from \"git commit --dry-run\".\n\nThat is, \"git commit --short/--porcelain/-z\" has these three\npossibilities:\n\n - work (ignoring these options);\n\n - work (showing the template in some kind of \"short\" format); or\n\n - error out (clearly indicating that we did *not* make a commit).\n\nand what we currently do is closest to the last (but we do not say\nwe did not create a commit).  In the longer term for Git 2.0, we may\nwant to change it to do one of the former two.\n"},{"id":"221820","messageId":"CALkWK0=2cJZDZTirZqYrfc9yTWaX8x=2G2xC_fhW53+GDDZ+BQ@mail.gmail.com","threadId":"34264","inReplyTo":"7v38s7czc9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] commit: make it work with status.short","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T15:39:41Z","receivedAt":"2013-06-24T15:39:41Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> As I said in the other message, I'll revert the merge of the topic\n> branch from 'master' for now, and queue this on top of the topic so\n> that we will have the preventive fix when the topic is in a better\n> shape for the other possible breakage on the \"--branch\" thing.\n\nThanks, a quick revert is probably the best way to go.\n\n> That is, \"git commit --short/--porcelain/-z\" has these three\n> possibilities:\n>\n>  - work (ignoring these options);\n>\n>  - work (showing the template in some kind of \"short\" format); or\n>\n>  - error out (clearly indicating that we did *not* make a commit).\n\nActually, I had a different idea: to make short (and porcelain, by\nextension) set s->commitable correctly.  From a quick analysis, it\nshouldn't be a hard problem.  Then, we can make\n\n  $ git commit\n\ndisplay short-form status when there is nothing to commit.\n"},{"id":"221822","messageId":"CALkWK0kNFLn+XV2Ug3a1CtSkErCpfWZVO51z8xSNjD++uhHf2A@mail.gmail.com","threadId":"34264","inReplyTo":"7v8v1zd0du.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T15:50:40Z","receivedAt":"2013-06-24T15:50:40Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Configuration related to the output format (like color=always) must\n> be ignored under \"--porcelain\", but if we do not read core-ish\n> configuration variables (e.g. core.crlf) that affect the logic to\n> list what is changed what is not, we would not give the right\n> result, no?\n\nAh, ofcourse.  My stupidity.\n\n>  My knee-jerk reaction is that, because the\n> \"--porcelain\" output was designed to be extensible and scripts\n> reading from it is expected to ignore what it does not understand,\n> if the setting of status.branch is a problem, the reading side is\n> buggy and needs to be fixed.\n\nAre you 100% sure about this?\n\n--porcelain::\n\tGive the output in an easy-to-parse format for scripts.\n\tThis is similar to the short output, but will remain stable\n\tacross Git versions and regardless of user configuration. See\n\tbelow for details.\n\nstatus.branch is an exception, no doubt: it just adds one line clearly\nprefixed with \"##\" to the beginning, and any script can ignore it easily.\n\n> Do we have in-core reader that does not behave well when one or both\n> of these configuration variables are set (perhaps something related\n> to submodule?)???\n\nYeah, the submodule.c parser is very naively written (see\nsubmodule.c:737), and I can fix it.  But the bigger question still\nremains: if we have such a non-robust parser in git.git itself,\nwouldn't you expect external parsers to be even worse?\n"},{"id":"221828","messageId":"7vk3ljbh5r.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"vpqhagnv9xq.fsf@anie.imag.fr","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T16:35:44Z","receivedAt":"2013-06-24T16:35:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>> Basically, having the CLI parser and the config parser flip two\n>> different sets of variables, so we can discriminate who set what.\n>> What annoys me is that this is the first instance of such a\n>> requirement.\n>\n> I don't think it's the first instance, but I can't remember precise\n> examples.\n\n\"First read config, override with command line\" is what we always\ndo.  One recent workaround with selective exception I can think of\noffhand is in diff config parser 6c374008 (diff_opt: track whether\nflags have been set explicitly, 2013-05-10), but I am fairly sure\nthere are others.  An older example is how show_notes_given is used\nin the revision traversal machinery to conditionally set show_notes.\n\n>> The approach I'm currently tilting towards is extending the\n>> parse-options API to allow parsing one special option early.  I would\n>> argue that this is a good feature that we should have asked for when\n>> we saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup',\n>> 2010-12-10).  What do you think?\n>\n> That's an option too, yes. But probably not easy to implement :-(.\n\nIsn't it essentially your second option (running the CLI parser\nbefore once, then read config, and then run the CLI parser for\nreal)?\n\nIn any case, I am still not convinced yet that status.short is a\nreal problem if --porcelain readers trip with \"## branchname\"\noutput.  Isn't it that the readers are broken and need fixing?\nThey should pick out what they care about and ignore the rest.\n"},{"id":"221849","messageId":"vpq7ghjtpv1.fsf@anie.imag.fr","threadId":"34264","inReplyTo":"7vk3ljbh5r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-24T16:50:26Z","receivedAt":"2013-06-24T16:50:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>\n>>> Basically, having the CLI parser and the config parser flip two\n>>> different sets of variables, so we can discriminate who set what.\n>>> What annoys me is that this is the first instance of such a\n>>> requirement.\n>>\n>> I don't think it's the first instance, but I can't remember precise\n>> examples.\n>\n> \"First read config, override with command line\" is what we always\n> do.  One recent workaround with selective exception I can think of\n> offhand is in diff config parser 6c374008 (diff_opt: track whether\n> flags have been set explicitly, 2013-05-10), but I am fairly sure\n> there are others.\n\nThat was the one I had in mind.\n\n>>> The approach I'm currently tilting towards is extending the\n>>> parse-options API to allow parsing one special option early.  I would\n>>> argue that this is a good feature that we should have asked for when\n>>> we saw 6758af89e (Merge branch 'jn/git-cmd-h-bypass-setup',\n>>> 2010-12-10).  What do you think?\n>>\n>> That's an option too, yes. But probably not easy to implement :-(.\n>\n> Isn't it essentially your second option (running the CLI parser\n> before once, then read config, and then run the CLI parser for\n> real)?\n\nNot really. The first run should be a kind of dry-run, except for the\n--porcelain part.\n\n> In any case, I am still not convinced yet that status.short is a\n> real problem if --porcelain readers trip with \"## branchname\"\n> output.  Isn't it that the readers are broken and need fixing?\n\nBefore introducing status.short, scripts could call \"git status\n--porcelain\" and get some output. They had no way to know whether\nsomething would be added in the future. Now, they can run the same\ncommand and get a different output. To me, that's exactly what we're\ntrying to avoid in plumbing.\n\nThe configuration file here is really meant for the user, not for\nscripts. Scripts that want the branch information can use --branch.\nScripts that do not have absolutely nothing to gain in getting this\nextra output (only extra parser complexity).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"221851","messageId":"CALkWK0m5Q--G+KuNBoGdXphiBPnhcvMF2Jz+y4Rt0W2zx3m2KQ@mail.gmail.com","threadId":"34264","inReplyTo":"7vk3ljbh5r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T16:53:30Z","receivedAt":"2013-06-24T16:53:30Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> In any case, I am still not convinced yet that status.short is a\n> real problem if --porcelain readers trip with \"## branchname\"\n> output.  Isn't it that the readers are broken and need fixing?\n\nIf you're going to read the configuration and then scramble to reset\nit in wt_porcelain_print(), why did you read the configuration in the\nfirst place?  Should column.branch, status.submodulesummary,\nstatus.showuntrackedfiles affect the --porcelain output too?  Isn't\nyour expectation of my parser unreasonable?\n\nI will argue that --porcelain should skip the branch config, and drop\ndown one level lower directly: to the diff-ui configuration.\n"},{"id":"221858","messageId":"7v1u7rbf9v.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"vpq7ghjtpv1.fsf@anie.imag.fr","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T17:16:28Z","receivedAt":"2013-06-24T17:16:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Scripts that want the branch information can use --branch.\n> Scripts that do not have absolutely nothing to gain in getting this\n> extra output (only extra parser complexity).\n\nOK, I could buy that.\n\nI personally have always _assumed_ that the way the \"--porcelain\"\noutput was meant to evolve was to make sure scripts are robust to\nignore what it does not care about, so that we allow people to add\nmore types of information without breaking the syntax, but script\nwriters have been assuming otherwise and there is no clearly written\nrule in the documentation, so it is too late to enforce it now.\n\nThat means after we add more and more other options like --branch\nthat enrich the output in several years, scripts that want to use\nthe enriched data need to pass tons of options to get what they\nwant, which is not very nice, but at least it won't break the status\nquo, and \"ask for only what you use\" is a good principle for scripts\nanyway.\n\nThanks for a sanity check.\n"},{"id":"221860","messageId":"vpqobavs9um.fsf@anie.imag.fr","threadId":"34264","inReplyTo":"7v1u7rbf9v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-24T17:21:37Z","receivedAt":"2013-06-24T17:21:37Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> That means after we add more and more other options like --branch\n> that enrich the output in several years, scripts that want to use\n> the enriched data need to pass tons of options to get what they\n> want, which is not very nice,\n\nRight, but accepting status.branch would not even solve that. Script\nwriters should not assume that status.branch is set. So, the --branch is\nstill required.\n\nActually, that would even be worse, as a script writer who's used to\nstatus.branch being set would not even notice that --branch is missing.\nSo, he'd still need it, but would have greater chance to miss it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"221867","messageId":"7vk3lj9xwn.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"vpq7ghjtpv1.fsf@anie.imag.fr","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T18:16:56Z","receivedAt":"2013-06-24T18:16:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>> \"First read config, override with command line\" is what we always\n>> do.  One recent workaround with selective exception I can think of\n>> offhand is in diff config parser 6c374008 (diff_opt: track whether\n>> flags have been set explicitly, 2013-05-10), but I am fairly sure\n>> there are others.\n>\n> That was the one I had in mind.\n\nIf we want to continue that tried-and-proven approach as a\nshort-term fix, a patch may look like this.\n\nWe use \"unspecified\" value to initialize the main variables\n(status_format global and s->show_branch), and also prepare\n\"from-config\" counterpart variables.  After we read both, we figure\nout which one should be used, while resetting the main variables to\ntheir default if neither mechanism touched them.\n\n builtin/commit.c  | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------\n t/t7508-status.sh |  5 +++++\n wt-status.c       |  1 +\n 3 files changed, 52 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0da944f..a535eb2 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -111,12 +111,14 @@ static int show_ignored_in_status;\n static const char *only_include_assumed;\n static struct strbuf message = STRBUF_INIT;\n \n-static enum {\n+static enum status_format {\n \tSTATUS_FORMAT_NONE = 0,\n \tSTATUS_FORMAT_LONG,\n \tSTATUS_FORMAT_SHORT,\n-\tSTATUS_FORMAT_PORCELAIN\n-} status_format;\n+\tSTATUS_FORMAT_PORCELAIN,\n+\n+\tSTATUS_FORMAT_UNSPECIFIED\n+} status_format = STATUS_FORMAT_UNSPECIFIED;\n \n static int opt_parse_m(const struct option *opt, const char *arg, int unset)\n {\n@@ -457,6 +459,9 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int\n \tcase STATUS_FORMAT_PORCELAIN:\n \t\twt_porcelain_print(s);\n \t\tbreak;\n+\tcase STATUS_FORMAT_UNSPECIFIED:\n+\t\tdie(\"BUG: finalize_deferred_config() should have been called\");\n+\t\tbreak;\n \tcase STATUS_FORMAT_NONE:\n \tcase STATUS_FORMAT_LONG:\n \t\twt_status_print(s);\n@@ -958,6 +963,33 @@ static const char *read_commit_message(const char *name)\n \treturn logmsg_reencode(commit, NULL, out_enc);\n }\n \n+/*\n+ * Enumerate what needs to be propagated when --porcelain\n+ * is not in effect here.\n+ */\n+static struct status_deferred_config {\n+\tenum status_format status_format;\n+\tint show_branch;\n+} status_deferred_config = {\n+\tSTATUS_FORMAT_UNSPECIFIED,\n+\t-1 /* unspecified */\n+};\n+\n+static void finalize_deferred_config(struct wt_status *s)\n+{\n+\tint use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN);\n+\n+\tif (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)\n+\t\tstatus_format = status_deferred_config.status_format;\n+\tif (status_format == STATUS_FORMAT_UNSPECIFIED)\n+\t\tstatus_format = STATUS_FORMAT_NONE;\n+\n+\tif (use_deferred_config && s->show_branch < 0)\n+\t\ts->show_branch = status_deferred_config.show_branch;\n+\tif (s->show_branch < 0)\n+\t\ts->show_branch = 0;\n+}\n+\n static int parse_and_validate_options(int argc, const char *argv[],\n \t\t\t\t      const struct option *options,\n \t\t\t\t      const char * const usage[],\n@@ -968,6 +1000,7 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tint f = 0;\n \n \targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\tfinalize_deferred_config(s);\n \n \tif (force_author && !strchr(force_author, '>'))\n \t\tforce_author = find_author_by_nickname(force_author);\n@@ -1112,13 +1145,13 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t}\n \tif (!strcmp(k, \"status.short\")) {\n \t\tif (git_config_bool(k, v))\n-\t\t\tstatus_format = STATUS_FORMAT_SHORT;\n+\t\t\tstatus_deferred_config.status_format = STATUS_FORMAT_SHORT;\n \t\telse\n-\t\t\tstatus_format = STATUS_FORMAT_NONE;\n+\t\t\tstatus_deferred_config.status_format = STATUS_FORMAT_NONE;\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"status.branch\")) {\n-\t\ts->show_branch = git_config_bool(k, v);\n+\t\tstatus_deferred_config.show_branch = git_config_bool(k, v);\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"status.color\") || !strcmp(k, \"color.status\")) {\n@@ -1163,8 +1196,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n \t\tOPT_SET_INT('s', \"short\", &status_format,\n \t\t\t    N_(\"show status concisely\"), STATUS_FORMAT_SHORT),\n-\t\tOPT_BOOLEAN('b', \"branch\", &s.show_branch,\n-\t\t\t    N_(\"show branch information\")),\n+\t\tOPT_BOOL('b', \"branch\", &s.show_branch,\n+\t\t\t N_(\"show branch information\")),\n \t\tOPT_SET_INT(0, \"porcelain\", &status_format,\n \t\t\t    N_(\"machine-readable output\"),\n \t\t\t    STATUS_FORMAT_PORCELAIN),\n@@ -1197,6 +1230,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t\t     builtin_status_options,\n \t\t\t     builtin_status_usage, 0);\n \tfinalize_colopts(&s.colopts, -1);\n+\tfinalize_deferred_config(&s);\n \n \tif (s.null_termination) {\n \t\tif (status_format == STATUS_FORMAT_NONE)\n@@ -1232,6 +1266,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tcase STATUS_FORMAT_PORCELAIN:\n \t\twt_porcelain_print(&s);\n \t\tbreak;\n+\tcase STATUS_FORMAT_UNSPECIFIED:\n+\t\tdie(\"BUG: finalize_deferred_config() should have been called\");\n+\t\tbreak;\n \tcase STATUS_FORMAT_NONE:\n \tcase STATUS_FORMAT_LONG:\n \t\ts.verbose = verbose;\n@@ -1400,7 +1437,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOLEAN(0, \"dry-run\", &dry_run, N_(\"show what would be committed\")),\n \t\tOPT_SET_INT(0, \"short\", &status_format, N_(\"show status concisely\"),\n \t\t\t    STATUS_FORMAT_SHORT),\n-\t\tOPT_BOOLEAN(0, \"branch\", &s.show_branch, N_(\"show branch information\")),\n+\t\tOPT_BOOL(0, \"branch\", &s.show_branch, N_(\"show branch information\")),\n \t\tOPT_SET_INT(0, \"porcelain\", &status_format,\n \t\t\t    N_(\"machine-readable output\"), STATUS_FORMAT_PORCELAIN),\n \t\tOPT_SET_INT(0, \"long\", &status_format,\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 498332c..ac3d0fe 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -1378,6 +1378,11 @@ test_expect_success '\"status.branch=true\" weaker than \"--no-branch\"' '\n \ttest_cmp expected_nobranch actual\n '\n \n+test_expect_success '\"status.branch=true\" weaker than \"--porcelain\"' '\n+       git -c status.branch=true status --porcelain >actual &&\n+       test_cmp expected_nobranch actual\n+'\n+\n test_expect_success '\"status.branch=false\" same as \"--no-branch\"' '\n \tgit -c status.branch=false status -s >actual &&\n \ttest_cmp expected_nobranch actual\ndiff --git a/wt-status.c b/wt-status.c\nindex bf84a86..6778755 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -127,6 +127,7 @@ void wt_status_prepare(struct wt_status *s)\n \ts->change.strdup_strings = 1;\n \ts->untracked.strdup_strings = 1;\n \ts->ignored.strdup_strings = 1;\n+\ts->show_branch = -1;  /* unspecified */\n }\n \n static void wt_status_print_unmerged_header(struct wt_status *s)\n"},{"id":"221886","messageId":"CALkWK0ngNrxT1MrdhXyz8ViN9an1XQsRD=DnxXYXqhDRmGO8EA@mail.gmail.com","threadId":"34264","inReplyTo":"7vk3lj9xwn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-06-24T19:30:23Z","receivedAt":"2013-06-24T19:30:23Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> If we want to continue that tried-and-proven approach as a\n> short-term fix, a patch may look like this.\n\nI don't like this special-casing for show_branch at all.  What is the\nproblem with skipping branch configuration altogether and going\nstraight to diff-ui configuration, like I suggested earlier?\n"},{"id":"221888","messageId":"7vvc5370ha.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"7vk3lj9xwn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T19:49:37Z","receivedAt":"2013-06-24T19:49:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> We use \"unspecified\" value to initialize the main variables\n> (status_format global and s->show_branch), and also prepare\n> \"from-config\" counterpart variables.  After we read both, we figure\n> out which one should be used, while resetting the main variables to\n> their default if neither mechanism touched them.\n>\n>  builtin/commit.c  | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------\n>  t/t7508-status.sh |  5 +++++\n>  wt-status.c       |  1 +\n>  3 files changed, 52 insertions(+), 9 deletions(-)\n\nAt least that needs this on top to deal with the late defaulting of\n\"-z\".\n\nWe know we do not want deferred configuration values when the\ncommand line says \"--porcelain\" or \"-z\".  And after handling the\n\"-z\" to set status_format, we apply config only if we want deferred\nconfiguration and command line left them unspecified.  And finally\nwe apply the default if both config and command line left them\nunspecified.\n\n builtin/commit.c | 24 ++++++++++--------------\n 1 file changed, 10 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex a535eb2..6f8cb04 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -977,7 +977,16 @@ static struct status_deferred_config {\n \n static void finalize_deferred_config(struct wt_status *s)\n {\n-\tint use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN);\n+\tint use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&\n+\t\t\t\t   !s->null_termination);\n+\n+\tif (s->null_termination) {\n+\t\tif (status_format == STATUS_FORMAT_NONE ||\n+\t\t    status_format == STATUS_FORMAT_UNSPECIFIED)\n+\t\t\tstatus_format = STATUS_FORMAT_PORCELAIN;\n+\t\telse if (status_format == STATUS_FORMAT_LONG)\n+\t\t\tdie(_(\"--long and -z are incompatible\"));\n+\t}\n \n \tif (use_deferred_config && status_format == STATUS_FORMAT_UNSPECIFIED)\n \t\tstatus_format = status_deferred_config.status_format;\n@@ -1085,12 +1094,6 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \tif (all && argc > 0)\n \t\tdie(_(\"Paths with -a does not make sense.\"));\n \n-\tif (s->null_termination) {\n-\t\tif (status_format == STATUS_FORMAT_NONE)\n-\t\t\tstatus_format = STATUS_FORMAT_PORCELAIN;\n-\t\telse if (status_format == STATUS_FORMAT_LONG)\n-\t\t\tdie(_(\"--long and -z are incompatible\"));\n-\t}\n \tif (status_format != STATUS_FORMAT_NONE)\n \t\tdry_run = 1;\n \n@@ -1232,13 +1235,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tfinalize_colopts(&s.colopts, -1);\n \tfinalize_deferred_config(&s);\n \n-\tif (s.null_termination) {\n-\t\tif (status_format == STATUS_FORMAT_NONE)\n-\t\t\tstatus_format = STATUS_FORMAT_PORCELAIN;\n-\t\telse if (status_format == STATUS_FORMAT_LONG)\n-\t\t\tdie(_(\"--long and -z are incompatible\"));\n-\t}\n-\n \thandle_untracked_files_arg(&s);\n \tif (show_ignored_in_status)\n \t\ts.show_ignored_files = 1;\n"},{"id":"221892","messageId":"7v61x36tba.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"CALkWK0ngNrxT1MrdhXyz8ViN9an1XQsRD=DnxXYXqhDRmGO8EA@mail.gmail.com","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T22:24:25Z","receivedAt":"2013-06-24T22:24:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Junio C Hamano wrote:\n>> If we want to continue that tried-and-proven approach as a\n>> short-term fix, a patch may look like this.\n>\n> I don't like this special-casing for show_branch at all.\n\nThe patch does not special-case show_branch at all, in that I would\nexpect that people over time will want to add more and more\nconfiguration not to type short and sweet command line options.  The\nprimary value of the patch is to illustrate how we would ignore the\nones we do not want when we want to still call git_status_config().\nIt lays a groundwork for doing so, using one single variable we\nhappen to know about today as an example.\n\n> What is the\n> problem with skipping branch configuration altogether and going\n> straight to diff-ui configuration, like I suggested earlier?\n\nWhy diff-*UI*-config, not diff_basic or even default_core?\n\nI actually find that alternative approach a lot nicer, and I wish\nthe world were that simple.\n\nOne thing that complicates the matter is that \"--porcelain\" output\nis affected by \"status.showuntrackedfiles\" setting, whose parser\ndates only back to June 2008, while --porcelain came much later in\nSep 2009.  So we have close to four years worth of user scripts that\nread from --porcelain and expects no untracked paths shown in a\nrepository with that configration set.  If we care about not\nbreaking people's scripts, we need to somehow arrange the option to\nkeep honoring status.showuntrackedfiles setting, which may mean that\nwe cannot just say \"let's bypass git_status_config() and be done\nwith it\".\n\nSo...\n"},{"id":"222148","messageId":"20130628014011.GA620@sigill.intra.peff.net","threadId":"34264","inReplyTo":"7vk3lj9xwn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-28T01:40:11Z","receivedAt":"2013-06-28T01:40:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 24, 2013 at 11:16:56AM -0700, Junio C Hamano wrote:\n\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 0da944f..a535eb2 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -111,12 +111,14 @@ static int show_ignored_in_status;\n>  static const char *only_include_assumed;\n>  static struct strbuf message = STRBUF_INIT;\n>  \n> -static enum {\n> +static enum status_format {\n>  \tSTATUS_FORMAT_NONE = 0,\n>  \tSTATUS_FORMAT_LONG,\n>  \tSTATUS_FORMAT_SHORT,\n> -\tSTATUS_FORMAT_PORCELAIN\n> -} status_format;\n> +\tSTATUS_FORMAT_PORCELAIN,\n> +\n> +\tSTATUS_FORMAT_UNSPECIFIED\n> +} status_format = STATUS_FORMAT_UNSPECIFIED;\n\nI'm not sure why you need to add UNSPECIFIED here; the point of NONE is\nthat no format had yet been specified.\n\nIt looks like the only difference is that finalize_deferred_config\nconverts unspecified into NONE. But I think you can just use NONE for\nboth cases, and drop this hunk:\n\n>  static int opt_parse_m(const struct option *opt, const char *arg, int unset)\n>  {\n> @@ -457,6 +459,9 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int\n>  \tcase STATUS_FORMAT_PORCELAIN:\n>  \t\twt_porcelain_print(s);\n>  \t\tbreak;\n> +\tcase STATUS_FORMAT_UNSPECIFIED:\n> +\t\tdie(\"BUG: finalize_deferred_config() should have been called\");\n> +\t\tbreak;\n>  \tcase STATUS_FORMAT_NONE:\n>  \tcase STATUS_FORMAT_LONG:\n>  \t\twt_status_print(s);\n\nYou lose the assertion that finalize_deferred_config has been called,\nbut I think the resulting code would be simpler, as it drops this\ndie(\"BUG\") state entirely. Am I missing something?\n\n-Peff\n"},{"id":"222151","messageId":"7v61wyrikr.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"20130628014011.GA620@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-28T03:59:48Z","receivedAt":"2013-06-28T03:59:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> You lose the assertion that finalize_deferred_config has been called,\n> but I think the resulting code would be simpler, as it drops this\n> die(\"BUG\") state entirely. Am I missing something?\n\nProbably not.  Depending on \"-z\", NONE is sometimes converted to\nPORCELAIN and sometimes left as-is.  I originally wanted to keep the\n\"unspecified\" state as long as possible so that this deferred config\nlogic and the \"-z\" default logic can be kept separate.\n\nThe final patch ended up folding that \"-z\" default logic into the\nsame function, so it probably is saner to remove UNSPECIFIED.\n"},{"id":"222196","messageId":"7vhagip25l.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"7v61wyrikr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-28T17:37:26Z","receivedAt":"2013-06-28T17:37:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> You lose the assertion that finalize_deferred_config has been called,\n>> but I think the resulting code would be simpler, as it drops this\n>> die(\"BUG\") state entirely. Am I missing something?\n>\n> Probably not.  Depending on \"-z\", NONE is sometimes converted to\n> PORCELAIN and sometimes left as-is.  I originally wanted to keep the\n> \"unspecified\" state as long as possible so that this deferred config\n> logic and the \"-z\" default logic can be kept separate.\n>\n> The final patch ended up folding that \"-z\" default logic into the\n> same function, so it probably is saner to remove UNSPECIFIED.\n\nActually, the code needs to be able to differentiate between\n\n\tgit status --no-short\n        git status\n\nthe former telling us explicitly to defeat status.short while the\nlatter telling us to use whatever random value we happen to have in\nthe configuration.  Initializing the variable to UNSPECIFIED is one\nway to achieve that, as the former will explicitly set it to NONE\nwhile the latter will leave it UNSPECIFIED when the command line\nparsing finishes.\n"},{"id":"222204","messageId":"20130628193104.GA12625@sigill.intra.peff.net","threadId":"34264","inReplyTo":"7vhagip25l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-28T19:31:04Z","receivedAt":"2013-06-28T19:31:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 28, 2013 at 10:37:26AM -0700, Junio C Hamano wrote:\n\n> > The final patch ended up folding that \"-z\" default logic into the\n> > same function, so it probably is saner to remove UNSPECIFIED.\n> \n> Actually, the code needs to be able to differentiate between\n> \n> \tgit status --no-short\n>         git status\n> \n> the former telling us explicitly to defeat status.short while the\n> latter telling us to use whatever random value we happen to have in\n> the configuration.  Initializing the variable to UNSPECIFIED is one\n> way to achieve that, as the former will explicitly set it to NONE\n> while the latter will leave it UNSPECIFIED when the command line\n> parsing finishes.\n\nHmm. I would have thought --no-short would just set it to LONG. That is,\nwe are no longer NONE at that point, as the user has told us something\non the command line. So we are whatever --no-short is, which is LONG.\n\nBut I guess that would wreck\n\n  git status --no-short -z\n\nwhich currently defaults to porcelain. Which, to be honest, seems a\nlittle crazy to me, but I guess there is no reason to break it.\n\nI am just trying to prevent the future maintenance confusion where a\nreader of the code says \"Huh? What is the difference between NONE and\nUNSPECIFIED?\"\n\n-Peff\n"},{"id":"222207","messageId":"7vzjuam1pu.fsf@alter.siamese.dyndns.org","threadId":"34264","inReplyTo":"20130628193104.GA12625@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] status: really ignore config with --porcelain","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-28T20:15:09Z","receivedAt":"2013-06-28T20:15:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Hmm. I would have thought --no-short would just set it to LONG. That is,\n> we are no longer NONE at that point, as the user has told us something\n> on the command line. So we are whatever --no-short is, which is LONG.\n>\n> But I guess that would wreck\n>\n>   git status --no-short -z\n>\n> which currently defaults to porcelain. Which, to be honest, seems a\n> little crazy to me, but I guess there is no reason to break it.\n>\n> I am just trying to prevent the future maintenance confusion where a\n> reader of the code says \"Huh? What is the difference between NONE and\n> UNSPECIFIED?\"\n\nYeah, I share your sentiment, but I did not think of a better way to\ndo this without unnecessarily changing behaviour.\n"}]}