# `git status -u no` suppresses modified files too. 5 messages from 2019-02-08 to 2019-03-03. Participants: Rusty Russell, Jeff King, Duy Nguyen, Junio C Hamano. Thread: https://gitlist.dev/t/50431 ## Rusty Russell, 2019-02-08 01:48 Subject: `git status -u no` suppresses modified files too. Message-ID: <87y36rcanq.fsf@rustcorp.com.au> URL: https://gitlist.dev/e/87y36rcanq.fsf%40rustcorp.com.au ``` This broke my "is this clean?" sanity check and very much violates the man page description. (I am now using `git diff --name-only` instead at Joel's suggestion.) $ git status On branch guilt/repro Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) modified: tools/repro-build.sh Untracked files: (use "git add ..." to include in what will be committed) .cache/ ANALYSIS NOTES SHA256SUMS SHA256SUMS.asc badpeer.json base channels.out.xz no changes added to commit (use "git add" and/or "git commit -a") $ git status -u no On branch guilt/repro nothing to commit, working tree clean $ git --version git version 2.19.1 $ Cheers, Rusty. ``` ## Jeff King, 2019-02-08 02:48 Subject: Re: `git status -u no` suppresses modified files too. Message-ID: <20190208024800.GA11392@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190208024800.GA11392%40sigill.intra.peff.net In-Reply-To: <87y36rcanq.fsf@rustcorp.com.au> ``` On Fri, Feb 08, 2019 at 12:18:57PM +1030, Rusty Russell wrote: > This broke my "is this clean?" sanity check and very much violates > the man page description. Wow, this one had me scratching my head for a minute. What you're describing here: > $ git status -u no > On branch guilt/repro ...seems horribly broken, and the behavior goes back to the beginnings of "-u". So I wondered how we would not have noticed for so long. But what is going on is quite subtle. The "-u" option takes an optional argument, and so must be used in its "stuck" form. I.e., git status -uno does do what you expect. We cannot allow the separated form here, because that would conflict with another actual option, like: git status -u --porcelain So why does it behave as it does? We accept "no" as its own pathspec argument, and thus we limit the status to that path. And that's why there's "nothing to commit"; there's nothing in that pathspec. This is a pretty horrible UI trap. Most of the time with pathspecs we require them to be on the right-hand side of a "--" unless the paths actually exist in the filesystem. But then, in most of those cases we're making sure they're not ambiguous between revisions and paths. So maybe it's overkill here. I dunno. But the patch below certainly makes what's going on in your case less subtle: $ git status -u no fatal: no: no such path in the working tree. Use 'git -- ...' to specify paths that do not exist locally. You do still have to figure out why it wasn't stuck to "-u", though. --- diff --git a/builtin/commit.c b/builtin/commit.c index 2986553d5f..7177d7d82f 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1300,6 +1300,16 @@ static int git_status_config(const char *k, const char *v, void *cb) return git_diff_ui_config(k, v, NULL); } +static void verify_pathspec(const char *prefix, const char **argv) +{ + while (*argv) { + const char *arg = *argv++; + if (!strcmp(arg, "--")) + return; + verify_filename(prefix, arg, 0); + } +} + int cmd_status(int argc, const char **argv, const char *prefix) { static int no_renames = -1; @@ -1351,7 +1361,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) status_init_config(&s, git_status_config); argc = parse_options(argc, argv, prefix, builtin_status_options, - builtin_status_usage, 0); + builtin_status_usage, PARSE_OPT_KEEP_DASHDASH); finalize_colopts(&s.colopts, -1); finalize_deferred_config(&s); @@ -1362,6 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) s.show_untracked_files == SHOW_NO_UNTRACKED_FILES) die(_("Unsupported combination of ignored and untracked-files arguments")); + verify_pathspec(prefix, argv); parse_pathspec(&s.pathspec, 0, PATHSPEC_PREFER_FULL, prefix, argv); ``` ## Rusty Russell, 2019-02-23 04:05 Subject: Re: `git status -u no` suppresses modified files too. Message-ID: <87va1btaic.fsf@rustcorp.com.au> URL: https://gitlist.dev/e/87va1btaic.fsf%40rustcorp.com.au In-Reply-To: <20190208024800.GA11392@sigill.intra.peff.net> ``` Jeff King writes: > On Fri, Feb 08, 2019 at 12:18:57PM +1030, Rusty Russell wrote: > >> This broke my "is this clean?" sanity check and very much violates >> the man page description. > > Wow, this one had me scratching my head for a minute. What you're > describing here: > >> $ git status -u no >> On branch guilt/repro > > ...seems horribly broken, and the behavior goes back to the beginnings > of "-u". So I wondered how we would not have noticed for so long. > > But what is going on is quite subtle. The "-u" option takes an optional > argument, and so must be used in its "stuck" form. I.e., > > git status -uno Urgh, manual page even *says* this, and I missed it: It is optional: it defaults to all, and if specified, it must be stuck to the option (e.g. -uno, but not -u no). Note that optional arguments are fundamentally broken like this: you should *never* add them to your programs. If -u suddently wanted an argument, a new option should have been added. But despite that "well, I wouldn't start from here!" advice, I think your patch seems really helpful. Thanks for clue contribution! Rusty. > does do what you expect. We cannot allow the separated form here, > because that would conflict with another actual option, like: > > git status -u --porcelain > > So why does it behave as it does? We accept "no" as its own pathspec > argument, and thus we limit the status to that path. And that's why > there's "nothing to commit"; there's nothing in that pathspec. > > This is a pretty horrible UI trap. Most of the time with pathspecs we > require them to be on the right-hand side of a "--" unless the paths > actually exist in the filesystem. But then, in most of those cases we're > making sure they're not ambiguous between revisions and paths. So maybe > it's overkill here. I dunno. But the patch below certainly makes what's > going on in your case less subtle: > > $ git status -u no > fatal: no: no such path in the working tree. > Use 'git -- ...' to specify paths that do not exist locally. > > You do still have to figure out why it wasn't stuck to "-u", though. > > --- > diff --git a/builtin/commit.c b/builtin/commit.c > index 2986553d5f..7177d7d82f 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1300,6 +1300,16 @@ static int git_status_config(const char *k, const char *v, void *cb) > return git_diff_ui_config(k, v, NULL); > } > > +static void verify_pathspec(const char *prefix, const char **argv) > +{ > + while (*argv) { > + const char *arg = *argv++; > + if (!strcmp(arg, "--")) > + return; > + verify_filename(prefix, arg, 0); > + } > +} > + > int cmd_status(int argc, const char **argv, const char *prefix) > { > static int no_renames = -1; > @@ -1351,7 +1361,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) > status_init_config(&s, git_status_config); > argc = parse_options(argc, argv, prefix, > builtin_status_options, > - builtin_status_usage, 0); > + builtin_status_usage, PARSE_OPT_KEEP_DASHDASH); > finalize_colopts(&s.colopts, -1); > finalize_deferred_config(&s); > > @@ -1362,6 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) > s.show_untracked_files == SHOW_NO_UNTRACKED_FILES) > die(_("Unsupported combination of ignored and untracked-files arguments")); > > + verify_pathspec(prefix, argv); > parse_pathspec(&s.pathspec, 0, > PATHSPEC_PREFER_FULL, > prefix, argv); ``` ## Duy Nguyen, 2019-02-25 08:59 Subject: Re: `git status -u no` suppresses modified files too. Message-ID: URL: https://gitlist.dev/e/CACsJy8DjHrCSXsOpH-pm5n-KUEm4BhJmhWYxZEWygmTEj1ykLA%40mail.gmail.com In-Reply-To: <87va1btaic.fsf@rustcorp.com.au> ``` On Mon, Feb 25, 2019 at 12:37 PM Rusty Russell wrote: > > Jeff King writes: > > On Fri, Feb 08, 2019 at 12:18:57PM +1030, Rusty Russell wrote: > > > >> This broke my "is this clean?" sanity check and very much violates > >> the man page description. > > > > Wow, this one had me scratching my head for a minute. What you're > > describing here: > > > >> $ git status -u no > >> On branch guilt/repro > > > > ...seems horribly broken, and the behavior goes back to the beginnings > > of "-u". So I wondered how we would not have noticed for so long. > > > > But what is going on is quite subtle. The "-u" option takes an optional > > argument, and so must be used in its "stuck" form. I.e., > > > > git status -uno > > Urgh, manual page even *says* this, and I missed it: > > It is optional: it > defaults to all, and if specified, it must be stuck to the option (e.g. -uno, but not -u > no). > > Note that optional arguments are fundamentally broken like this: you > should *never* add them to your programs. If -u suddently wanted an > argument, a new option should have been added. This -uno thing is bugging me too and I've seen a couple more options like this. Maybe we can start moving to -u=no or something like that. It's still broken if we accept -= as a valid option (I don't think we do as of now), but at least it does not look as misleading as -uno, which reads -u -n -o by everybody else who has not read all the man pages. > But despite that "well, I wouldn't start from here!" advice, I think > your patch seems really helpful. > > Thanks for clue contribution! > Rusty. > > > does do what you expect. We cannot allow the separated form here, > > because that would conflict with another actual option, like: > > > > git status -u --porcelain > > > > So why does it behave as it does? We accept "no" as its own pathspec > > argument, and thus we limit the status to that path. And that's why > > there's "nothing to commit"; there's nothing in that pathspec. > > > > This is a pretty horrible UI trap. Most of the time with pathspecs we > > require them to be on the right-hand side of a "--" unless the paths > > actually exist in the filesystem. But then, in most of those cases we're > > making sure they're not ambiguous between revisions and paths. So maybe > > it's overkill here. I dunno. But the patch below certainly makes what's > > going on in your case less subtle: > > > > $ git status -u no > > fatal: no: no such path in the working tree. > > Use 'git -- ...' to specify paths that do not exist locally. > > > > You do still have to figure out why it wasn't stuck to "-u", though. > > > > --- > > diff --git a/builtin/commit.c b/builtin/commit.c > > index 2986553d5f..7177d7d82f 100644 > > --- a/builtin/commit.c > > +++ b/builtin/commit.c > > @@ -1300,6 +1300,16 @@ static int git_status_config(const char *k, const char *v, void *cb) > > return git_diff_ui_config(k, v, NULL); > > } > > > > +static void verify_pathspec(const char *prefix, const char **argv) > > +{ > > + while (*argv) { > > + const char *arg = *argv++; > > + if (!strcmp(arg, "--")) > > + return; > > + verify_filename(prefix, arg, 0); > > + } > > +} > > + > > int cmd_status(int argc, const char **argv, const char *prefix) > > { > > static int no_renames = -1; > > @@ -1351,7 +1361,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) > > status_init_config(&s, git_status_config); > > argc = parse_options(argc, argv, prefix, > > builtin_status_options, > > - builtin_status_usage, 0); > > + builtin_status_usage, PARSE_OPT_KEEP_DASHDASH); > > finalize_colopts(&s.colopts, -1); > > finalize_deferred_config(&s); > > > > @@ -1362,6 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) > > s.show_untracked_files == SHOW_NO_UNTRACKED_FILES) > > die(_("Unsupported combination of ignored and untracked-files arguments")); > > > > + verify_pathspec(prefix, argv); > > parse_pathspec(&s.pathspec, 0, > > PATHSPEC_PREFER_FULL, > > prefix, argv); -- Duy ``` ## Junio C Hamano, 2019-03-03 01:22 Subject: Re: `git status -u no` suppresses modified files too. Message-ID: URL: https://gitlist.dev/e/xmqqfts44ur0.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190208024800.GA11392@sigill.intra.peff.net> ``` Jeff King writes: > This is a pretty horrible UI trap. Most of the time with pathspecs we > require them to be on the right-hand side of a "--" unless the paths > actually exist in the filesystem. But then, in most of those cases we're > making sure they're not ambiguous between revisions and paths. So maybe > it's overkill here. I dunno. But the patch below certainly makes what's > going on in your case less subtle: > > $ git status -u no > fatal: no: no such path in the working tree. > Use 'git -- ...' to specify paths that do not exist locally. Yeah. It was a mistake that we gave a short-and-sweet "-u" too hastily to a relatively useless form that does not take any argument and ended up needing to resort to the optional-arg trick. > You do still have to figure out why it wasn't stuck to "-u", though. Sorry, but I am not sure what you mean by this comment. Your illustration patch lets you say "no, 'no' is not a pathspec" with git status -u no -- and "I want the unadorned -u and am asking about stuff in the 'no' subdirectory" with git status -u -- no but in the former, it would not make 'no' an arg to '-u' by saying "'no' is not a pathspec", would it? > --- > diff --git a/builtin/commit.c b/builtin/commit.c > index 2986553d5f..7177d7d82f 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1300,6 +1300,16 @@ static int git_status_config(const char *k, const char *v, void *cb) > return git_diff_ui_config(k, v, NULL); > } > > +static void verify_pathspec(const char *prefix, const char **argv) > +{ > + while (*argv) { > + const char *arg = *argv++; > + if (!strcmp(arg, "--")) > + return; > + verify_filename(prefix, arg, 0); > + } > +} > + > int cmd_status(int argc, const char **argv, const char *prefix) > { > static int no_renames = -1; > @@ -1351,7 +1361,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) > status_init_config(&s, git_status_config); > argc = parse_options(argc, argv, prefix, > builtin_status_options, > - builtin_status_usage, 0); > + builtin_status_usage, PARSE_OPT_KEEP_DASHDASH); > finalize_colopts(&s.colopts, -1); > finalize_deferred_config(&s); > > @@ -1362,6 +1372,7 @@ int cmd_status(int argc, const char **argv, const char *prefix) > s.show_untracked_files == SHOW_NO_UNTRACKED_FILES) > die(_("Unsupported combination of ignored and untracked-files arguments")); > > + verify_pathspec(prefix, argv); > parse_pathspec(&s.pathspec, 0, > PATHSPEC_PREFER_FULL, > prefix, argv); ```