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

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

[Rusty Russell]

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.

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

[Jeff King]

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);

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

[Rusty Russell]

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.

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);

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

[Duy Nguyen]

On Mon, Feb 25, 2019 at 12:37 PM Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> 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.

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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 <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?

> ---
> 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);