threads / discuss / 50431

`git status -u no` suppresses modified files too.

Subject: `git status -u no` suppresses modified files too.

## tl;dr

5 messages between Feb 8, 2019 and Mar 3, 2019.

replies: 4people: 4as markdown or json

Rusty Russell· Feb 8, 2019, 01:48 UTC · lore

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 <file>..." to update what will be committed)
          (use "git checkout -- <file>..." to discard changes in working directory)
        
        	modified:   tools/repro-build.sh
        
        Untracked files:
          (use "git add <file>..." 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· Feb 8, 2019, 02:48 UTC · re: Rusty Russell · lore

Re: `git status -u no` suppresses modified files too.

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 <command> -- <path>...' 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· Feb 23, 2019, 04:05 UTC · re: Jeff King · lore

Re: `git status -u no` suppresses modified files too.

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> 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.

Show 61 quoted lines
> 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 <command> -- <path>...' 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· Feb 25, 2019, 08:59 UTC · re: Rusty Russell · lore

Re: `git status -u no` suppresses modified files too.

On Mon, Feb 25, 2019 at 12:37 PM Rusty Russell <rusty@rustcorp.com.au> wrote:
Show 30 quoted lines
>
> Jeff King <peff@peff.net> 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.

Show 67 quoted lines
> 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 <command> -- <path>...' 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· Mar 3, 2019, 01:22 UTC · re: Jeff King · lore

Re: `git status -u no` suppresses modified files too.

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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 <command> -- <path>...' 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?

Show 39 quoted lines
> ---
> 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);

← back to recent threads