git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* `git status -u no` suppresses modified files too.
@ 2019-02-08  1:48 Rusty Russell
  2019-02-08  2:48 ` Jeff King
  0 siblings, 1 reply; 5+ messages in thread
From: Rusty Russell @ 2019-02-08  1:48 UTC (permalink / raw)
  To: git; +Cc: Joel Stanley

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: `git status -u no` suppresses modified files too.
  2019-02-08  1:48 `git status -u no` suppresses modified files too Rusty Russell
@ 2019-02-08  2:48 ` Jeff King
  2019-02-23  4:05   ` Rusty Russell
  2019-03-03  1:22   ` Junio C Hamano
  0 siblings, 2 replies; 5+ messages in thread
From: Jeff King @ 2019-02-08  2:48 UTC (permalink / raw)
  To: Rusty Russell; +Cc: git, Joel Stanley

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: `git status -u no` suppresses modified files too.
  2019-02-08  2:48 ` Jeff King
@ 2019-02-23  4:05   ` Rusty Russell
  2019-02-25  8:59     ` Duy Nguyen
  2019-03-03  1:22   ` Junio C Hamano
  1 sibling, 1 reply; 5+ messages in thread
From: Rusty Russell @ 2019-02-23  4:05 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Joel Stanley

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: `git status -u no` suppresses modified files too.
  2019-02-23  4:05   ` Rusty Russell
@ 2019-02-25  8:59     ` Duy Nguyen
  0 siblings, 0 replies; 5+ messages in thread
From: Duy Nguyen @ 2019-02-25  8:59 UTC (permalink / raw)
  To: Rusty Russell; +Cc: Jeff King, Git Mailing List, Joel Stanley

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: `git status -u no` suppresses modified files too.
  2019-02-08  2:48 ` Jeff King
  2019-02-23  4:05   ` Rusty Russell
@ 2019-03-03  1:22   ` Junio C Hamano
  1 sibling, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2019-03-03  1:22 UTC (permalink / raw)
  To: Jeff King; +Cc: Rusty Russell, git, Joel Stanley

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-03-03  1:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-08  1:48 `git status -u no` suppresses modified files too Rusty Russell
2019-02-08  2:48 ` Jeff King
2019-02-23  4:05   ` Rusty Russell
2019-02-25  8:59     ` Duy Nguyen
2019-03-03  1:22   ` Junio C Hamano

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).