{"thread":{"id":"50431","subject":"`git status -u no` suppresses modified files too.","startedAt":"2019-02-08T01:49:08Z","lastAt":"2019-03-03T01:22:32Z","messageCount":5,"participants":["Rusty Russell","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"368767","messageId":"87y36rcanq.fsf@rustcorp.com.au","threadId":"50431","inReplyTo":null,"subject":"`git status -u no` suppresses modified files too.","fromName":"Rusty Russell","fromEmail":"rusty@rustcorp.com.au","sentAt":"2019-02-08T01:48:57Z","receivedAt":"2019-02-08T01:49:08Z","isPatch":false,"sender":{"key":"rusty@rustcorp.com.au","avatar":"https://gravatar.com/avatar/6b532b78e53abc4701523f6225607caa34fc0b5264600ce23bb5e3f537209fc6?d=mp&s=160"},"body":"This broke my \"is this clean?\" sanity check and very much violates\nthe man page description.\n\n(I am now using `git diff --name-only` instead at Joel's suggestion.)\n\n $ git status\n On branch guilt/repro\n Changes not staged for commit:\n (use \"git add ...\" to update what will be committed)\n (use \"git checkout -- ...\" to discard changes in working directory)\n \n \tmodified: tools/repro-build.sh\n \n Untracked files:\n (use \"git add ...\" to include in what will be committed)\n \n \t.cache/\n \tANALYSIS\n \tNOTES\n \tSHA256SUMS\n \tSHA256SUMS.asc\n \tbadpeer.json\n \tbase\n \tchannels.out.xz\n \n no changes added to commit (use \"git add\" and/or \"git commit -a\")\n $ git status -u no\n On branch guilt/repro\n nothing to commit, working tree clean\n $ git --version\n git version 2.19.1\n $\n\nCheers,\nRusty.\n"},{"id":"368768","messageId":"20190208024800.GA11392@sigill.intra.peff.net","threadId":"50431","inReplyTo":"87y36rcanq.fsf@rustcorp.com.au","subject":"Re: `git status -u no` suppresses modified files too.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-08T02:48:01Z","receivedAt":"2019-02-08T02:48:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 08, 2019 at 12:18:57PM +1030, Rusty Russell wrote:\n\n> This broke my \"is this clean?\" sanity check and very much violates\n> the man page description.\n\nWow, this one had me scratching my head for a minute. What you're\ndescribing here:\n\n> $ git status -u no\n> On branch guilt/repro\n\n...seems horribly broken, and the behavior goes back to the beginnings\nof \"-u\". So I wondered how we would not have noticed for so long.\n\nBut what is going on is quite subtle. The \"-u\" option takes an optional\nargument, and so must be used in its \"stuck\" form. I.e.,\n\n git status -uno\n\ndoes do what you expect. We cannot allow the separated form here,\nbecause that would conflict with another actual option, like:\n\n git status -u --porcelain\n\nSo why does it behave as it does? We accept \"no\" as its own pathspec\nargument, and thus we limit the status to that path. And that's why\nthere's \"nothing to commit\"; there's nothing in that pathspec.\n\nThis is a pretty horrible UI trap. Most of the time with pathspecs we\nrequire them to be on the right-hand side of a \"--\" unless the paths\nactually exist in the filesystem. But then, in most of those cases we're\nmaking sure they're not ambiguous between revisions and paths. So maybe\nit's overkill here. I dunno. But the patch below certainly makes what's\ngoing on in your case less subtle:\n\n $ git status -u no\n fatal: no: no such path in the working tree.\n Use 'git -- ...' to specify paths that do not exist locally.\n\nYou do still have to figure out why it wasn't stuck to \"-u\", though.\n\n---\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2986553d5f..7177d7d82f 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1300,6 +1300,16 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \treturn git_diff_ui_config(k, v, NULL);\n }\n \n+static void verify_pathspec(const char *prefix, const char **argv)\n+{\n+\twhile (*argv) {\n+\t\tconst char *arg = *argv++;\n+\t\tif (!strcmp(arg, \"--\"))\n+\t\t\treturn;\n+\t\tverify_filename(prefix, arg, 0);\n+\t}\n+}\n+\n int cmd_status(int argc, const char **argv, const char *prefix)\n {\n \tstatic int no_renames = -1;\n@@ -1351,7 +1361,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tstatus_init_config(&s, git_status_config);\n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_status_options,\n-\t\t\t builtin_status_usage, 0);\n+\t\t\t builtin_status_usage, PARSE_OPT_KEEP_DASHDASH);\n \tfinalize_colopts(&s.colopts, -1);\n \tfinalize_deferred_config(&s);\n \n@@ -1362,6 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t s.show_untracked_files == SHOW_NO_UNTRACKED_FILES)\n \t\tdie(_(\"Unsupported combination of ignored and untracked-files arguments\"));\n \n+\tverify_pathspec(prefix, argv);\n \tparse_pathspec(&s.pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL,\n \t\t prefix, argv);\n"},{"id":"370104","messageId":"87va1btaic.fsf@rustcorp.com.au","threadId":"50431","inReplyTo":"20190208024800.GA11392@sigill.intra.peff.net","subject":"Re: `git status -u no` suppresses modified files too.","fromName":"Rusty Russell","fromEmail":"rusty@rustcorp.com.au","sentAt":"2019-02-23T04:05:31Z","receivedAt":"2019-02-25T05:36:41Z","isPatch":false,"sender":{"key":"rusty@rustcorp.com.au","avatar":"https://gravatar.com/avatar/6b532b78e53abc4701523f6225607caa34fc0b5264600ce23bb5e3f537209fc6?d=mp&s=160"},"body":"Jeff King writes:\n> On Fri, Feb 08, 2019 at 12:18:57PM +1030, Rusty Russell wrote:\n>\n>> This broke my \"is this clean?\" sanity check and very much violates\n>> the man page description.\n>\n> Wow, this one had me scratching my head for a minute. What you're\n> describing here:\n>\n>> $ git status -u no\n>> On branch guilt/repro\n>\n> ...seems horribly broken, and the behavior goes back to the beginnings\n> of \"-u\". So I wondered how we would not have noticed for so long.\n>\n> But what is going on is quite subtle. The \"-u\" option takes an optional\n> argument, and so must be used in its \"stuck\" form. I.e.,\n>\n> git status -uno\n\nUrgh, manual page even *says* this, and I missed it:\n\n It is optional: it\n defaults to all, and if specified, it must be stuck to the option (e.g. -uno, but not -u\n no).\n\nNote that optional arguments are fundamentally broken like this: you\nshould *never* add them to your programs. If -u suddently wanted an\nargument, a new option should have been added.\n\nBut despite that \"well, I wouldn't start from here!\" advice, I think\nyour patch seems really helpful.\n\nThanks for clue contribution! \nRusty.\n\n> does do what you expect. We cannot allow the separated form here,\n> because that would conflict with another actual option, like:\n>\n> git status -u --porcelain\n>\n> So why does it behave as it does? We accept \"no\" as its own pathspec\n> argument, and thus we limit the status to that path. And that's why\n> there's \"nothing to commit\"; there's nothing in that pathspec.\n>\n> This is a pretty horrible UI trap. Most of the time with pathspecs we\n> require them to be on the right-hand side of a \"--\" unless the paths\n> actually exist in the filesystem. But then, in most of those cases we're\n> making sure they're not ambiguous between revisions and paths. So maybe\n> it's overkill here. I dunno. But the patch below certainly makes what's\n> going on in your case less subtle:\n>\n> $ git status -u no\n> fatal: no: no such path in the working tree.\n> Use 'git -- ...' to specify paths that do not exist locally.\n>\n> You do still have to figure out why it wasn't stuck to \"-u\", though.\n>\n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2986553d5f..7177d7d82f 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1300,6 +1300,16 @@ static int git_status_config(const char *k, const char *v, void *cb)\n> \treturn git_diff_ui_config(k, v, NULL);\n> }\n> \n> +static void verify_pathspec(const char *prefix, const char **argv)\n> +{\n> +\twhile (*argv) {\n> +\t\tconst char *arg = *argv++;\n> +\t\tif (!strcmp(arg, \"--\"))\n> +\t\t\treturn;\n> +\t\tverify_filename(prefix, arg, 0);\n> +\t}\n> +}\n> +\n> int cmd_status(int argc, const char **argv, const char *prefix)\n> {\n> \tstatic int no_renames = -1;\n> @@ -1351,7 +1361,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> \tstatus_init_config(&s, git_status_config);\n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_status_options,\n> -\t\t\t builtin_status_usage, 0);\n> +\t\t\t builtin_status_usage, PARSE_OPT_KEEP_DASHDASH);\n> \tfinalize_colopts(&s.colopts, -1);\n> \tfinalize_deferred_config(&s);\n> \n> @@ -1362,6 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> \t s.show_untracked_files == SHOW_NO_UNTRACKED_FILES)\n> \t\tdie(_(\"Unsupported combination of ignored and untracked-files arguments\"));\n> \n> +\tverify_pathspec(prefix, argv);\n> \tparse_pathspec(&s.pathspec, 0,\n> \t\t PATHSPEC_PREFER_FULL,\n> \t\t prefix, argv);\n"},{"id":"370108","messageId":"CACsJy8DjHrCSXsOpH-pm5n-KUEm4BhJmhWYxZEWygmTEj1ykLA@mail.gmail.com","threadId":"50431","inReplyTo":"87va1btaic.fsf@rustcorp.com.au","subject":"Re: `git status -u no` suppresses modified files too.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-25T08:59:04Z","receivedAt":"2019-02-25T08:59:33Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Feb 25, 2019 at 12:37 PM Rusty Russell wrote:\n>\n> Jeff King writes:\n> > On Fri, Feb 08, 2019 at 12:18:57PM +1030, Rusty Russell wrote:\n> >\n> >> This broke my \"is this clean?\" sanity check and very much violates\n> >> the man page description.\n> >\n> > Wow, this one had me scratching my head for a minute. What you're\n> > describing here:\n> >\n> >> $ git status -u no\n> >> On branch guilt/repro\n> >\n> > ...seems horribly broken, and the behavior goes back to the beginnings\n> > of \"-u\". So I wondered how we would not have noticed for so long.\n> >\n> > But what is going on is quite subtle. The \"-u\" option takes an optional\n> > argument, and so must be used in its \"stuck\" form. I.e.,\n> >\n> > git status -uno\n>\n> Urgh, manual page even *says* this, and I missed it:\n>\n> It is optional: it\n> defaults to all, and if specified, it must be stuck to the option (e.g. -uno, but not -u\n> no).\n>\n> Note that optional arguments are fundamentally broken like this: you\n> should *never* add them to your programs. If -u suddently wanted an\n> argument, a new option should have been added.\n\nThis -uno thing is bugging me too and I've seen a couple more options\nlike this. Maybe we can start moving to -u=no or something like that.\nIt's still broken if we accept -= as a valid option (I don't think we\ndo as of now), but at least it does not look as misleading as -uno,\nwhich reads -u -n -o by everybody else who has not read all the man\npages.\n\n> But despite that \"well, I wouldn't start from here!\" advice, I think\n> your patch seems really helpful.\n>\n> Thanks for clue contribution!\n> Rusty.\n>\n> > does do what you expect. We cannot allow the separated form here,\n> > because that would conflict with another actual option, like:\n> >\n> > git status -u --porcelain\n> >\n> > So why does it behave as it does? We accept \"no\" as its own pathspec\n> > argument, and thus we limit the status to that path. And that's why\n> > there's \"nothing to commit\"; there's nothing in that pathspec.\n> >\n> > This is a pretty horrible UI trap. Most of the time with pathspecs we\n> > require them to be on the right-hand side of a \"--\" unless the paths\n> > actually exist in the filesystem. But then, in most of those cases we're\n> > making sure they're not ambiguous between revisions and paths. So maybe\n> > it's overkill here. I dunno. But the patch below certainly makes what's\n> > going on in your case less subtle:\n> >\n> > $ git status -u no\n> > fatal: no: no such path in the working tree.\n> > Use 'git -- ...' to specify paths that do not exist locally.\n> >\n> > You do still have to figure out why it wasn't stuck to \"-u\", though.\n> >\n> > ---\n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 2986553d5f..7177d7d82f 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -1300,6 +1300,16 @@ static int git_status_config(const char *k, const char *v, void *cb)\n> > return git_diff_ui_config(k, v, NULL);\n> > }\n> >\n> > +static void verify_pathspec(const char *prefix, const char **argv)\n> > +{\n> > + while (*argv) {\n> > + const char *arg = *argv++;\n> > + if (!strcmp(arg, \"--\"))\n> > + return;\n> > + verify_filename(prefix, arg, 0);\n> > + }\n> > +}\n> > +\n> > int cmd_status(int argc, const char **argv, const char *prefix)\n> > {\n> > static int no_renames = -1;\n> > @@ -1351,7 +1361,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> > status_init_config(&s, git_status_config);\n> > argc = parse_options(argc, argv, prefix,\n> > builtin_status_options,\n> > - builtin_status_usage, 0);\n> > + builtin_status_usage, PARSE_OPT_KEEP_DASHDASH);\n> > finalize_colopts(&s.colopts, -1);\n> > finalize_deferred_config(&s);\n> >\n> > @@ -1362,6 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> > s.show_untracked_files == SHOW_NO_UNTRACKED_FILES)\n> > die(_(\"Unsupported combination of ignored and untracked-files arguments\"));\n> >\n> > + verify_pathspec(prefix, argv);\n> > parse_pathspec(&s.pathspec, 0,\n> > PATHSPEC_PREFER_FULL,\n> > prefix, argv);\n\n\n\n-- \nDuy\n"},{"id":"370490","messageId":"xmqqfts44ur0.fsf@gitster-ct.c.googlers.com","threadId":"50431","inReplyTo":"20190208024800.GA11392@sigill.intra.peff.net","subject":"Re: `git status -u no` suppresses modified files too.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-03T01:22:27Z","receivedAt":"2019-03-03T01:22:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is a pretty horrible UI trap. Most of the time with pathspecs we\n> require them to be on the right-hand side of a \"--\" unless the paths\n> actually exist in the filesystem. But then, in most of those cases we're\n> making sure they're not ambiguous between revisions and paths. So maybe\n> it's overkill here. I dunno. But the patch below certainly makes what's\n> going on in your case less subtle:\n>\n> $ git status -u no\n> fatal: no: no such path in the working tree.\n> Use 'git -- ...' to specify paths that do not exist locally.\n\nYeah. It was a mistake that we gave a short-and-sweet \"-u\" too\nhastily to a relatively useless form that does not take any argument\nand ended up needing to resort to the optional-arg trick.\n\n> You do still have to figure out why it wasn't stuck to \"-u\", though.\n\nSorry, but I am not sure what you mean by this comment.\n\nYour illustration patch lets you say \"no, 'no' is not a pathspec\"\nwith\n\n\tgit status -u no --\n\nand \"I want the unadorned -u and am asking about stuff in the 'no'\nsubdirectory\" with\n\n\tgit status -u -- no\n\nbut in the former, it would not make 'no' an arg to '-u' by saying\n\"'no' is not a pathspec\", would it?\n\n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2986553d5f..7177d7d82f 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1300,6 +1300,16 @@ static int git_status_config(const char *k, const char *v, void *cb)\n> \treturn git_diff_ui_config(k, v, NULL);\n> }\n> \n> +static void verify_pathspec(const char *prefix, const char **argv)\n> +{\n> +\twhile (*argv) {\n> +\t\tconst char *arg = *argv++;\n> +\t\tif (!strcmp(arg, \"--\"))\n> +\t\t\treturn;\n> +\t\tverify_filename(prefix, arg, 0);\n> +\t}\n> +}\n> +\n> int cmd_status(int argc, const char **argv, const char *prefix)\n> {\n> \tstatic int no_renames = -1;\n> @@ -1351,7 +1361,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> \tstatus_init_config(&s, git_status_config);\n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_status_options,\n> -\t\t\t builtin_status_usage, 0);\n> +\t\t\t builtin_status_usage, PARSE_OPT_KEEP_DASHDASH);\n> \tfinalize_colopts(&s.colopts, -1);\n> \tfinalize_deferred_config(&s);\n> \n> @@ -1362,6 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> \t s.show_untracked_files == SHOW_NO_UNTRACKED_FILES)\n> \t\tdie(_(\"Unsupported combination of ignored and untracked-files arguments\"));\n> \n> +\tverify_pathspec(prefix, argv);\n> \tparse_pathspec(&s.pathspec, 0,\n> \t\t PATHSPEC_PREFER_FULL,\n> \t\t prefix, argv);\n"}]}