threads / patch / 11289

patch, 3 partsTest interaction between diff --check and --exit-code

Subject: [PATCH 3/3] Test interaction between diff --check and --exit-code

## tl;dr

24 messages between Dec 14, 2007 and Dec 17, 2007. Diffs are folded; open one to read it.

replies: 23people: 5as markdown or json

Wincent Colaiuta· Dec 14, 2007, 11:23 UTC · lore

[PATCH 0/3] diff topic tweaks

I've eyeballed and done some testing of the changes that Junio pushed out to "next" and they all look good to me.

Some minor adjustments:
[1/3] Revert changes and extend diff option documentation

Seeing as Junio's adjustments changed the behaviour, the documentation needs to be changed as well.

[2/3] Use shorter error messages for whitespace problems
Cosmetic only.
[3/3] Test interaction between diff --check and --exit-code
Just to make sure it does what we think it does (and it does).

Cheers, Wincent

Wincent Colaiuta· Dec 14, 2007, 11:23 UTC · re: Wincent Colaiuta · lore

[PATCH 1/3] Revert changes and extend diff option documentation

Revert the part of 62c6489 which says that --check affects the exit code of "git diff"; as of da31b35 this is only true if you pass --exit-code as well or disable the pager with --no-pager or GIT_PAGER=cat.

Instead extend the discussion of the --exit-code switch.

Also, instead of hard-coding "what whitespace errors are", mention that their classification can be controlled via config and per-path attributes.

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 Documentation/diff-options.txt |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)
Show changes to Documentation/diff-options.txt +7 −6
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9ecc1d7..54207f0 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -92,10 +92,10 @@ endif::git-format-patch[]
 	file gives the default to do so.
 
 --check::
-	Warn if changes introduce trailing whitespace
-	or an indent that uses a space before a tab. Exits with
-	non-zero status if problems are found. Not compatible with
-	--exit-code.
+	Warn if changes introduce whitespace problems (such as
+	trailing whitespace). Configuration and per-path attributes
+	control what git classifies as a whitespace problem (see
+	gitlink:git-config[1] and gitlink:gitattributes[5]).
 
 --full-index::
 	Instead of the first handful characters, show full
@@ -197,8 +197,9 @@ endif::git-format-patch[]
 
 --exit-code::
 	Make the program exit with codes similar to diff(1).
-	That is, it exits with 1 if there were differences and
-	0 means no differences.
+	That is, it exits with 0 if there were no differences
+	and 1 if there were. If --check is used and the
+	differences introduce whitespace problems exits with 3.
 
 --quiet::
 	Disable all output of the program. Implies --exit-code.

-- 
1.5.4.rc0.1099.g76fa0-dirty
Wincent Colaiuta· Dec 14, 2007, 11:23 UTC · re: Wincent Colaiuta · lore

[PATCH 2/3] Use shorter error messages for whitespace problems

The initial version of the whitespace_error_string() function took the messages from builtin-apply.c rather than the shorter messages from diff.c.

This commit addresses Junio's concern that these messages might be too long (now that we can emit multiple warnings per line).

Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 t/t4015-diff-whitespace.sh |    2 +-
 ws.c                       |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 2 files +4 −4

t/t4015-diff-whitespace.sh, ws.c

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 05ef78b..9bff8f5 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -121,7 +121,7 @@ test_expect_success 'check mixed spaces and tabs in indent' '
 
 	# This is indented with SP HT SP.
 	echo " 	 foo();" > x &&
-	git diff --check | grep "Space in indent is followed by a tab"
+	git diff --check | grep "space before tab in indent"
 
 '
 
diff --git a/ws.c b/ws.c
index d7d1522..46cbdd6 100644
--- a/ws.c
+++ b/ws.c
@@ -101,16 +101,16 @@ char *whitespace_error_string(unsigned ws)
 	struct strbuf err;
 	strbuf_init(&err, 0);
 	if (ws & WS_TRAILING_SPACE)
-		strbuf_addstr(&err, "Adds trailing whitespace");
+		strbuf_addstr(&err, "trailing whitespace");
 	if (ws & WS_SPACE_BEFORE_TAB) {
 		if (err.len)
 			strbuf_addstr(&err, ", ");
-		strbuf_addstr(&err, "Space in indent is followed by a tab");
+		strbuf_addstr(&err, "space before tab in indent");
 	}
 	if (ws & WS_INDENT_WITH_NON_TAB) {
 		if (err.len)
 			strbuf_addstr(&err, ", ");
-		strbuf_addstr(&err, "Indent more than 8 places with spaces");
+		strbuf_addstr(&err, "indent with spaces");
 	}
 	return strbuf_detach(&err, NULL);
 }

-- 
1.5.4.rc0.1099.g76fa0-dirty
Wincent Colaiuta· Dec 14, 2007, 11:23 UTC · re: Wincent Colaiuta · lore
Make sure that it works as advertised in the man page.
Signed-off-by: Wincent Colaiuta <win@wincent.com>
---
 t/t4017-diff-retval.sh |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)
Show changes to t/t4017-diff-retval.sh +29 −0
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index 6873190..dc0b712 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -76,4 +76,33 @@ test_expect_success 'git diff-index --cached HEAD' '
 	}
 '
 
+test_expect_success '--check --exit-code returns 0 for no difference' '
+
+	git diff --check --exit-code
+
+'
+
+test_expect_success '--check --exit-code returns 1 for a clean difference' '
+
+	echo "good" > a &&
+	git diff --check --exit-code
+	test $? = 1
+
+'
+
+test_expect_success '--check --exit-code returns 3 for a dirty difference' '
+
+	echo "bad   " >> a &&
+	git diff --check --exit-code
+	test $? = 3
+
+'
+
+test_expect_success '--check with --no-pager returns 2 for dirty difference' '
+
+	git --no-pager diff --check
+	test $? = 2
+
+'
+
 test_done

-- 
1.5.4.rc0.1099.g76fa0-dirty
Junio C Hamano· Dec 16, 2007, 19:43 UTC · re: Wincent Colaiuta · lore

Re: [PATCH 1/3] Revert changes and extend diff option documentation

Wincent Colaiuta <win@wincent.com> writes:
Show 16 quoted lines
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 9ecc1d7..54207f0 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -92,10 +92,10 @@ endif::git-format-patch[]
>  	file gives the default to do so.
>  
>  --check::
> -	Warn if changes introduce trailing whitespace
> -	or an indent that uses a space before a tab. Exits with
> -	non-zero status if problems are found. Not compatible with
> -	--exit-code.
> +	Warn if changes introduce whitespace problems (such as
> +	trailing whitespace). Configuration and per-path attributes
> +	control what git classifies as a whitespace problem (see
> +	gitlink:git-config[1] and gitlink:gitattributes[5]).

This is not quite right, is it? The command still exits with exit code. It is just that the calling process does not see it if you let it spawn the pager.

Show 9 quoted lines
> @@ -197,8 +197,9 @@ endif::git-format-patch[]
>  
>  --exit-code::
>  	Make the program exit with codes similar to diff(1).
> -	That is, it exits with 1 if there were differences and
> -	0 means no differences.
> +	That is, it exits with 0 if there were no differences
> +	and 1 if there were. If --check is used and the
> +	differences introduce whitespace problems exits with 3.
This side is correct.
Wincent Colaiuta· Dec 17, 2007, 08:27 UTC · re: Junio C Hamano · lore

git.c option parsing (was: [PATCH 1/3] Revert changes and extend diff option documentation)

El 16/12/2007, a las 20:43, Junio C Hamano escribió:
Show 25 quoted lines
> Wincent Colaiuta <win@wincent.com> writes:
>
>> diff --git a/Documentation/diff-options.txt b/Documentation/diff- 
>> options.txt
>> index 9ecc1d7..54207f0 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -92,10 +92,10 @@ endif::git-format-patch[]
>> 	file gives the default to do so.
>>
>> --check::
>> -	Warn if changes introduce trailing whitespace
>> -	or an indent that uses a space before a tab. Exits with
>> -	non-zero status if problems are found. Not compatible with
>> -	--exit-code.
>> +	Warn if changes introduce whitespace problems (such as
>> +	trailing whitespace). Configuration and per-path attributes
>> +	control what git classifies as a whitespace problem (see
>> +	gitlink:git-config[1] and gitlink:gitattributes[5]).
>
> This is not quite right, is it?  The command still exits with exit  
> code.
> It is just that the calling process does not see it if you let it  
> spawn
> the pager.

Yes, I know, but I thought that in the interests of brevity it would be best not to clutter up the man page with that kind of detail. If a user is interested in the exit code they will probably do a search of the man page for "exit" and find the --exit-code section, which gives them the answer they want. Adding a line like "Also, if you pass --no- pager will exit with a non-zero exit code" doesn't really add much other than clutter IMO.

Incidentally, and this is really tangential, I find the whole "--no- pager" UI pretty horrendous, along with all the other switches parsed by git.c. Basically, understanding these switches requires the user to know internal implementation details that he/she should most definitely not have to know.

For example, "git --no-pager diff --check" works as expected but "git diff --no-pager --check" carps with "error: invalid option: --no- pager". To understand why this is so, the user needs to know too many things which should be internal implementation details:

- that "git" is just a wrapper tool that chooses a low level command  
to run and runs it
- that there are some parameters that affect the way all the other  
commands run and should be passed to "git" only, prior to the  
"subcommand" part
- that those parameters can't be passed after the "subcommand"  
specification

And seeing as we are actively encouraging users to always use the "git diff" form rather than "git-diff", in a sense we are actively encourage them to think of Git as a single tool rather than a collection of commands, it seems like an ugly wart that we then have to teach them that some parameters must go *between* the "git" and the "diff" but not after. And of course, if you always use the "git-diff" then there's no way to use the --no-pager switch at all; you instead have to use an environment variable.

I've been thinking about ways to iron out these wrinkles and unify things. Here's my thinking about a possible cause of action:

1. Factor out the code in git.c which handles the common arguments  
into separate functions; at this stage it's just a refactoring and the  
behaviour is unchanged, but those separate functions will later be  
used from the builtins.
2. Teach the builtin commands to handle those common arguments by  
using the common functions. For example, we teach "git diff" to  
understand the --no-pager option by leveraging the functions we just  
factored out in the previous step.
3. Now, when git.c sees an argument that would normally go before a  
"subcommand" it no longer needs to handle it directly itself but can  
instead pass it along to the subcommand. In other words, where it  
formerly saw "git --no-pager diff args..." and handled the --no-pager  
part itself, it can instead just pass "--no-pager args..." to the  
builtin diff. This gives us backward compatibility with the old  
format, but the new format (user types "git diff --no-pager args...")  
will work also.
4. Update the docs: make a common-options.txt page which is included  
in all other manual pages either listing the options explicitly or  
directing users to the git(7) manpage (which should probably become  
git(1) if we are talking about Git as a single tool) to learn about  
them.

Benefits of this approach: we'd have a consistent UI which didn't require too much knowledge of the internals of Git, and all of the following would work:

git diff --no-pager args... git-diff --no-pager args... git --no-pager diff args...

Looking at the options currently parsed by git.c, I think most of them would be very straightforward to support in the way described above:

--paginate --no-pager --git-dir --work-tree --bare

Existing special cases:

--help (calls "git help foo") --version

Problematic:

-p (short form of --paginate: seeing as -p is already used to mean other things by other commands, I think this should be deprecated as a synonym for --paginate)

Tricky:

--exec-path (this is the only one which git.c *must* handle before passing control to the "subcommand", so it actually has to look ahead past the "subcommand" part in order to see it and act upon it. Basically it would just have to look at all the arguments up to but not including a "--" pathspec marker checking to see if --exec-path is supplied)

Of course, the above plan will only work for builtins, not for scripts. An additional step would be needed to enable scripts to handle these options; perhaps teaching "git rev-parse" something...

Cheers, Wincent

Junio C Hamano· Dec 17, 2007, 08:48 UTC · re: Wincent Colaiuta · lore

Re: git.c option parsing

Wincent Colaiuta <win@wincent.com> writes:
> Of course, the above plan will only work for builtins, not for  
> scripts. An additional step would be needed to enable scripts to  
> handle these options; perhaps teaching "git rev-parse" something...

As long as special options stay special and we make a rule not to allow any subcommand to assign its own meaning to them, the git wrapper can lookahead and reorder, when seeing a command line:

	git scripted-command --special
into
	git --special scripted-command

And that approach would work well for built-ins as well, I would imagine.

There is one minor detail, though. There could be an option-parameter that is literally --special. E.g.

	git grep -e --no-paginate
should not be reordered to
	git --no-paginate grep -e
Wincent Colaiuta· Dec 17, 2007, 09:01 UTC · re: Junio C Hamano · lore

Re: git.c option parsing

El 17/12/2007, a las 9:48, Junio C Hamano escribió:
Show 19 quoted lines
> Wincent Colaiuta <win@wincent.com> writes:
>
>> Of course, the above plan will only work for builtins, not for
>> scripts. An additional step would be needed to enable scripts to
>> handle these options; perhaps teaching "git rev-parse" something...
>
> As long as special options stay special and we make a rule not to  
> allow
> any subcommand to assign its own meaning to them, the git wrapper can
> lookahead and reorder, when seeing a command line:
>
> 	git scripted-command --special
>
> into
>
> 	git --special scripted-command
>
> And that approach would work well for built-ins as well, I would
> imagine.

Yes, and it would be simpler to implement also. The only downside is that without all the other proposed changes things like "git-dashed -- special" wouldn't work; only teaching the builtins to actually handle the special options would work in that case. And in the interests of consistency I think it's pretty important that "git subcommand -- special" and "git-subcommand --special" both work the same as the user would (reasonably) expect them to...

Show 8 quoted lines
> There is one minor detail, though.  There could be an option-parameter
> that is literally --special.  E.g.
>
> 	git grep -e --no-paginate
>
> should not be reordered to
>
> 	git --no-paginate grep -e

Yes, that's definitely one that would need to be treated as a special case.

Cheers, Wincent

Pierre Habouzit· Dec 17, 2007, 09:20 UTC · re: Wincent Colaiuta · lore

Re: git.c option parsing

On Mon, Dec 17, 2007 at 09:01:56AM +0000, Wincent Colaiuta wrote:
Show 28 quoted lines
> El 17/12/2007, a las 9:48, Junio C Hamano escribió:
> 
> >Wincent Colaiuta <win@wincent.com> writes:
> >
> >>Of course, the above plan will only work for builtins, not for
> >>scripts. An additional step would be needed to enable scripts to
> >>handle these options; perhaps teaching "git rev-parse" something...
> >
> >As long as special options stay special and we make a rule not to allow
> >any subcommand to assign its own meaning to them, the git wrapper can
> >lookahead and reorder, when seeing a command line:
> >
> >	git scripted-command --special
> >
> >into
> >
> >	git --special scripted-command
> >
> >And that approach would work well for built-ins as well, I would
> >imagine.
> 
> Yes, and it would be simpler to implement also. The only downside is that 
> without all the other proposed changes things like "git-dashed --special" 
> wouldn't work; only teaching the builtins to actually handle the special 
> options would work in that case. And in the interests of consistency I 
> think it's pretty important that "git subcommand --special" and 
> "git-subcommand --special" both work the same as the user would 
> (reasonably) expect them to...
  There is a simple way to do that, that wouldn't conflict with the git
grep -e one, that would be to define an array of "Special" flags, in a
macro, and have every builtin using parseopt adding that macro at the
end.
  Then in git.c, you would have to scan for the command name when called
through `git --opt1 --opt2 foo`, and pass it to parseopt with as argc
the position of `foo` in the argument array. Parseopt will trust you for
it, and if it returns a non zero count, you have to barf, then you just
need to work on the rest of the array. That would mean in pseudo code
that this would work like:
    // ...
    /* when in `git --opt1 --opt2 foo -a -b -c` mode: */
    int cmd_pos = git_find_builtin_command_name(argc, argv);
    int count = parse_options(cmd_pos, argv, git_generic_options,
	"git [special-options] cmd [options]", 0);
    if (count)
	die("unknown git command: `%s`", argv[0]);
    argv += cmd_pos;
    argc -= cmd_pos;
    /* here we simulate an argv of {"foo", "-a", "-b", "-c"} */
  Of course this only works on builtins that do support parseopt other
ones will need the massage you describe. For them the sole thing to
change would be to replace the OPT_END() with a GIT_SPECIAL_OPTS() or
sth alike.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Pierre Habouzit· Dec 17, 2007, 09:26 UTC · re: Pierre Habouzit · lore

Re: git.c option parsing

On Mon, Dec 17, 2007 at 09:20:13AM +0000, Pierre Habouzit wrote:
Show 27 quoted lines
>   There is a simple way to do that, that wouldn't conflict with the git
> grep -e one, that would be to define an array of "Special" flags, in a
> macro, and have every builtin using parseopt adding that macro at the
> end.
> 
>   Then in git.c, you would have to scan for the command name when called
> through `git --opt1 --opt2 foo`, and pass it to parseopt with as argc
> the position of `foo` in the argument array. Parseopt will trust you for
> it, and if it returns a non zero count, you have to barf, then you just
> need to work on the rest of the array. That would mean in pseudo code
> that this would work like:
> 
>     // ...
>     /* when in `git --opt1 --opt2 foo -a -b -c` mode: */
>     int cmd_pos = git_find_builtin_command_name(argc, argv);
>     int count = parse_options(cmd_pos, argv, git_generic_options,
> 	"git [special-options] cmd [options]", 0);
>     if (count)
> 	die("unknown git command: `%s`", argv[0]);
>     argv += cmd_pos;
>     argc -= cmd_pos;
>     /* here we simulate an argv of {"foo", "-a", "-b", "-c"} */
> 
>   Of course this only works on builtins that do support parseopt other
> ones will need the massage you describe. For them the sole thing to
> change would be to replace the OPT_END() with a GIT_SPECIAL_OPTS() or
> sth alike.
  Oh and I forgot to say that I find this is a brilliant idea, because
there is quite a _lot_ of options that are very pervasive: --quiet,
--verbose, … are pretty obvious candidates, but things like:
`--abbrev=12` does make a lot of sense to me, and it would factor quite
a lot of option structures :) Okay `git --abbrev=12 status` probably
makes no real sense, but who cares ?
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Pierre Habouzit· Dec 17, 2007, 09:50 UTC · re: Pierre Habouzit · lore

[PATCH] Have a flag to stop the option parsing at the first argument.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
    >     // ...
    >     /* when in `git --opt1 --opt2 foo -a -b -c` mode: */
    >     int cmd_pos = git_find_builtin_command_name(argc, argv);
    >     int count = parse_options(cmd_pos, argv, git_generic_options,
    > 	"git [special-options] cmd [options]", 0);
    >     if (count)
    > 	die("unknown git command: `%s`", argv[0]);
    >     argv += cmd_pos;
    >     argc -= cmd_pos;
    >     /* here we simulate an argv of {"foo", "-a", "-b", "-c"} */
    Or even simpler, with the following specifically tailored patch you
    can directly write:
    argc = parse_options(argc, argv, git_generic_options,
	"git [generic-options] <command> [cmd-options]",
	PARSE_OPT_STOP_AT_ARG);
    and then {argc, argv} will exactly be the NULL-terminated array
    starting with the builtin command. Kind of nice :)
 parse-options.c |    2 ++
 parse-options.h |    1 +
 2 files changed, 3 insertions(+), 0 deletions(-)
Show changes to 2 files +3 −0

parse-options.c, parse-options.h

diff --git a/parse-options.c b/parse-options.c
index 7a08a0c..4f5c55e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -229,6 +229,8 @@ int parse_options(int argc, const char **argv, const struct option *options,
 		const char *arg = args.argv[0];
 
 		if (*arg != '-' || !arg[1]) {
+			if (flags & PARSE_OPT_STOP_AT_ARG)
+				break;
 			argv[j++] = args.argv[0];
 			continue;
 		}
diff --git a/parse-options.h b/parse-options.h
index 102ac31..7c636b9 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -18,6 +18,7 @@ enum parse_opt_type {
 
 enum parse_opt_flags {
 	PARSE_OPT_KEEP_DASHDASH = 1,
+	PARSE_OPT_STOP_AT_ARG   = 2,
 };
 
 enum parse_opt_option_flags {

-- 
1.5.4.rc0.1147.gfd22d
Johannes Schindelin· Dec 17, 2007, 10:34 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

Hi,
On Mon, 17 Dec 2007, Pierre Habouzit wrote:
Show 25 quoted lines
> diff --git a/parse-options.c b/parse-options.c
> index 7a08a0c..4f5c55e 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -229,6 +229,8 @@ int parse_options(int argc, const char **argv, const struct option *options,
>  		const char *arg = args.argv[0];
>  
>  		if (*arg != '-' || !arg[1]) {
> +			if (flags & PARSE_OPT_STOP_AT_ARG)
> +				break;
>  			argv[j++] = args.argv[0];
>  			continue;
>  		}
> diff --git a/parse-options.h b/parse-options.h
> index 102ac31..7c636b9 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -18,6 +18,7 @@ enum parse_opt_type {
>  
>  enum parse_opt_flags {
>  	PARSE_OPT_KEEP_DASHDASH = 1,
> +	PARSE_OPT_STOP_AT_ARG   = 2,
>  };
>  
>  enum parse_opt_option_flags {
Funny.  I already posted this:
http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=504f763a28b3109fce258b36f9e94e7c54be6f3d

Ciao, Dscho

Wincent Colaiuta· Dec 17, 2007, 11:10 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

El 17/12/2007, a las 10:50, Pierre Habouzit escribió:
Show 22 quoted lines
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> ---
>>    // ...
>>    /* when in `git --opt1 --opt2 foo -a -b -c` mode: */
>>    int cmd_pos = git_find_builtin_command_name(argc, argv);
>>    int count = parse_options(cmd_pos, argv, git_generic_options,
>> 	"git [special-options] cmd [options]", 0);
>>    if (count)
>> 	die("unknown git command: `%s`", argv[0]);
>>    argv += cmd_pos;
>>    argc -= cmd_pos;
>>    /* here we simulate an argv of {"foo", "-a", "-b", "-c"} */
>
>    Or even simpler, with the following specifically tailored patch you
>    can directly write:
>
>    argc = parse_options(argc, argv, git_generic_options,
> 	"git [generic-options] <command> [cmd-options]",
> 	PARSE_OPT_STOP_AT_ARG);
>
>    and then {argc, argv} will exactly be the NULL-terminated array
>    starting with the builtin command. Kind of nice :)

Indeed, nice ideas. I think all this will lead to a nice UI improvement post-1.5.4.

About the only thing that I think would merit action *prior* to 1.5.4 is marking the "-p" switch to git (synonym for --paginate) as deprecated, see as it clashes with other commands' uses of that switch ("git log -p" for example). Are there any other conflicting specials that a currently parsed in git.c?

Cheers, Wincent

Pierre Habouzit· Dec 17, 2007, 11:47 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

On Mon, Dec 17, 2007 at 11:10:00AM +0000, Wincent Colaiuta wrote:
Show 33 quoted lines
> El 17/12/2007, a las 10:50, Pierre Habouzit escribió:
> 
> >Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> >---
> >>   // ...
> >>   /* when in `git --opt1 --opt2 foo -a -b -c` mode: */
> >>   int cmd_pos = git_find_builtin_command_name(argc, argv);
> >>   int count = parse_options(cmd_pos, argv, git_generic_options,
> >>	"git [special-options] cmd [options]", 0);
> >>   if (count)
> >>	die("unknown git command: `%s`", argv[0]);
> >>   argv += cmd_pos;
> >>   argc -= cmd_pos;
> >>   /* here we simulate an argv of {"foo", "-a", "-b", "-c"} */
> >
> >   Or even simpler, with the following specifically tailored patch you
> >   can directly write:
> >
> >   argc = parse_options(argc, argv, git_generic_options,
> >	"git [generic-options] <command> [cmd-options]",
> >	PARSE_OPT_STOP_AT_ARG);
> >
> >   and then {argc, argv} will exactly be the NULL-terminated array
> >   starting with the builtin command. Kind of nice :)
> 
> Indeed, nice ideas. I think all this will lead to a nice UI improvement 
> post-1.5.4.
> 
> About the only thing that I think would merit action *prior* to 1.5.4 is 
> marking the "-p" switch to git (synonym for --paginate) as deprecated, 
> see as it clashes with other commands' uses of that switch ("git log -p" 
> for example). Are there any other conflicting specials that a currently 
> parsed in git.c?
  You don't need to, and I'd see that as a regression. With my proposal,
there isn't any kind of need that git commands do not clash with git
ones. The parse-option mechanism will properly hide options that are
masked this way, dscho wrote the patch for that.
  git -p log -p ...

just makes sense to me. CVS or SVN e.g. (don't hit me !) have the same kind of "issues", and I never found that weird.

  In fact I see this the other way around: git status -p that is in fact
the same as git -p status, is the conveniency, git -p status is the
canonical form.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Johannes Schindelin· Dec 17, 2007, 11:50 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

Hi,
On Mon, 17 Dec 2007, Pierre Habouzit wrote:
Show 48 quoted lines
> On Mon, Dec 17, 2007 at 11:10:00AM +0000, Wincent Colaiuta wrote:
> > El 17/12/2007, a las 10:50, Pierre Habouzit escribió:
> > 
> > >Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > >---
> > >>   // ...
> > >>   /* when in `git --opt1 --opt2 foo -a -b -c` mode: */
> > >>   int cmd_pos = git_find_builtin_command_name(argc, argv);
> > >>   int count = parse_options(cmd_pos, argv, git_generic_options,
> > >>	"git [special-options] cmd [options]", 0);
> > >>   if (count)
> > >>	die("unknown git command: `%s`", argv[0]);
> > >>   argv += cmd_pos;
> > >>   argc -= cmd_pos;
> > >>   /* here we simulate an argv of {"foo", "-a", "-b", "-c"} */
> > >
> > >   Or even simpler, with the following specifically tailored patch you
> > >   can directly write:
> > >
> > >   argc = parse_options(argc, argv, git_generic_options,
> > >	"git [generic-options] <command> [cmd-options]",
> > >	PARSE_OPT_STOP_AT_ARG);
> > >
> > >   and then {argc, argv} will exactly be the NULL-terminated array
> > >   starting with the builtin command. Kind of nice :)
> > 
> > Indeed, nice ideas. I think all this will lead to a nice UI improvement 
> > post-1.5.4.
> > 
> > About the only thing that I think would merit action *prior* to 1.5.4 is 
> > marking the "-p" switch to git (synonym for --paginate) as deprecated, 
> > see as it clashes with other commands' uses of that switch ("git log -p" 
> > for example). Are there any other conflicting specials that a currently 
> > parsed in git.c?
> 
>   You don't need to, and I'd see that as a regression. With my proposal,
> there isn't any kind of need that git commands do not clash with git
> ones. The parse-option mechanism will properly hide options that are
> masked this way, dscho wrote the patch for that.
> 
>   git -p log -p ...
> 
> just makes sense to me. CVS or SVN e.g. (don't hit me !) have the same
> kind of "issues", and I never found that weird.
> 
>   In fact I see this the other way around: git status -p that is in fact
> the same as git -p status, is the conveniency, git -p status is the
> canonical form.
I would even go further: "git status -p" looks utterly wrong to me.

Ciao, Dscho

Wincent Colaiuta· Dec 17, 2007, 12:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

El 17/12/2007, a las 12:50, Johannes Schindelin escribió:
Show 54 quoted lines
> On Mon, 17 Dec 2007, Pierre Habouzit wrote:
>
>> On Mon, Dec 17, 2007 at 11:10:00AM +0000, Wincent Colaiuta wrote:
>>> El 17/12/2007, a las 10:50, Pierre Habouzit escribió:
>>>
>>>> Signed-off-by: Pierre Habouzit <madcoder@debian.org>
>>>> ---
>>>>>  // ...
>>>>>  /* when in `git --opt1 --opt2 foo -a -b -c` mode: */
>>>>>  int cmd_pos = git_find_builtin_command_name(argc, argv);
>>>>>  int count = parse_options(cmd_pos, argv, git_generic_options,
>>>>> 	"git [special-options] cmd [options]", 0);
>>>>>  if (count)
>>>>> 	die("unknown git command: `%s`", argv[0]);
>>>>>  argv += cmd_pos;
>>>>>  argc -= cmd_pos;
>>>>>  /* here we simulate an argv of {"foo", "-a", "-b", "-c"} */
>>>>
>>>>  Or even simpler, with the following specifically tailored patch  
>>>> you
>>>>  can directly write:
>>>>
>>>>  argc = parse_options(argc, argv, git_generic_options,
>>>> 	"git [generic-options] <command> [cmd-options]",
>>>> 	PARSE_OPT_STOP_AT_ARG);
>>>>
>>>>  and then {argc, argv} will exactly be the NULL-terminated array
>>>>  starting with the builtin command. Kind of nice :)
>>>
>>> Indeed, nice ideas. I think all this will lead to a nice UI  
>>> improvement
>>> post-1.5.4.
>>>
>>> About the only thing that I think would merit action *prior* to  
>>> 1.5.4 is
>>> marking the "-p" switch to git (synonym for --paginate) as  
>>> deprecated,
>>> see as it clashes with other commands' uses of that switch ("git  
>>> log -p"
>>> for example). Are there any other conflicting specials that a  
>>> currently
>>> parsed in git.c?
>>
>>  You don't need to, and I'd see that as a regression. With my  
>> proposal,
>> there isn't any kind of need that git commands do not clash with git
>> ones. The parse-option mechanism will properly hide options that are
>> masked this way, dscho wrote the patch for that.
>>
>>  git -p log -p ...
>>
>> just makes sense to me. CVS or SVN e.g. (don't hit me !) have the  
>> same
>> kind of "issues", and I never found that weird.

Yes, we know what it does because we know that "git ... log ..." is actually two commands and each one handles one of the -p switches, but it is much easier to present git as a single tool to the newcomer (and I guess I don't need to argue that case here seeing as the decision has already been taken long ago to talk using dashless forms), and it is much easier to explain to a newcomer something like:

git log --paginate -p
Than:
git -p log -p

(Incidentally, who actually uses "git -p log -p"? Doesn't everybody have a pager set-up by default?)

Show 6 quoted lines
>>  In fact I see this the other way around: git status -p that is in  
>> fact
>> the same as git -p status, is the conveniency, git -p status is the
>> canonical form.
>
> I would even go further: "git status -p" looks utterly wrong to me.

This is exactly the kind of insider knowledge that will baffle newcomers. The canonical form is canonical only because it relies on an historical (and continuing) implementation detail, but we shouldn't expect newcomers to have to learn such details in order to know where to stick their command line arguments; they already have enough things to learn about, I think.

But it doesn't really matter. The proposed changes allow old-timers to continue putting their special options between the "git" and the "command". If you don't want to deprecate the -p special because of the confusion it might cause, I think we should at least not give it a very prominent place in the documentation, nor use it any examples.

Cheers, Wincent

Johannes Schindelin· Dec 17, 2007, 12:26 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

Hi,
On Mon, 17 Dec 2007, Wincent Colaiuta wrote:
Show 12 quoted lines
> Yes, we know what it does because we know that "git ... log ..." is 
> actually two commands and each one handles one of the -p switches, but 
> it is much easier to present git as a single tool to the newcomer (and I 
> guess I don't need to argue that case here seeing as the decision has 
> already been taken long ago to talk using dashless forms), and it is 
> much easier to explain to a newcomer something like:
> 
> git log --paginate -p
> 
> Than:
> 
> git -p log -p
How about
	git log -p
Hmm?

Fact is: you make the tool easier by having sane defaults. Not by moving around command line options. The option "-p" for git is an option that holds for _all_ subcommands. That's why it belongs _before_ the subcommand.

Show 5 quoted lines
> But it doesn't really matter. The proposed changes allow old-timers to 
> continue putting their special options between the "git" and the 
> "command". If you don't want to deprecate the -p special because of the 
> confusion it might cause, I think we should at least not give it a very 
> prominent place in the documentation, nor use it any examples.

I think it is wrong to go out of our way to support "git status -p" as a synonym to "git -p status". I simply do not believe that newcomers are not intelligent enough to understand that "git -p <subcommand>" means that the output goes into their pager.

Ciao, Dscho

Wincent Colaiuta· Dec 17, 2007, 12:40 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

El 17/12/2007, a las 13:26, Johannes Schindelin escribió:
Show 48 quoted lines
> Hi,
>
> On Mon, 17 Dec 2007, Wincent Colaiuta wrote:
>
>> Yes, we know what it does because we know that "git ... log ..." is
>> actually two commands and each one handles one of the -p switches,  
>> but
>> it is much easier to present git as a single tool to the newcomer  
>> (and I
>> guess I don't need to argue that case here seeing as the decision has
>> already been taken long ago to talk using dashless forms), and it is
>> much easier to explain to a newcomer something like:
>>
>> git log --paginate -p
>>
>> Than:
>>
>> git -p log -p
>
> How about
>
> 	git log -p
>
> Hmm?
>
> Fact is: you make the tool easier by having sane defaults.  Not by  
> moving
> around command line options.  The option "-p" for git is an option  
> that
> holds for _all_ subcommands.  That's why it belongs _before_ the
> subcommand.
>
>> But it doesn't really matter. The proposed changes allow old-timers  
>> to
>> continue putting their special options between the "git" and the
>> "command". If you don't want to deprecate the -p special because of  
>> the
>> confusion it might cause, I think we should at least not give it a  
>> very
>> prominent place in the documentation, nor use it any examples.
>
> I think it is wrong to go out of our way to support "git status -p"  
> as a
> synonym to "git -p status".  I simply do not believe that newcomers  
> are
> not intelligent enough to understand that "git -p <subcommand>"  
> means that
> the output goes into their pager.

But the point is, of all the special options, -p is the *only* that can't unambiguously go after the subcommand.

I'm arguing that the world would be a simpler place, friendlier to newcomers if *all* git commands looked like "git subcommand opts...", and at the moment -p is the only obstacle to making this so. And just in case it's necessary to restate this, I am not proposing removing support for the git specials subcommand opts..." form; I'm just trying to make it so that we don't have to advertise it so prominently. This seems to be the natural complement to the move to dashless forms.

Cheers, Wincent

Johannes Schindelin· Dec 17, 2007, 12:55 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

Hi,
On Mon, 17 Dec 2007, Wincent Colaiuta wrote:
Show 9 quoted lines
> El 17/12/2007, a las 13:26, Johannes Schindelin escribi?:
> 
> > I think it is wrong to go out of our way to support "git status -p" as 
> > a synonym to "git -p status".  I simply do not believe that newcomers 
> > are not intelligent enough to understand that "git -p <subcommand>" 
> > means that the output goes into their pager.
> 
> But the point is, of all the special options, -p is the *only* that 
> can't unambiguously go after the subcommand.

It should not be put after the subcommand. That's my point. Exactly because it is -- even conceptually -- no subcommand option.

CVS has many shortcomings, but one lesson here is that people had no problems with "cvs -z3 update -d -P". See, the "-z3" is an option that has nothing to do with the subcommand.

Exactly the same situation here. I never had any problems explaining why "-p" goes before the subcommand here. Never.

Ciao, Dscho

Wincent Colaiuta· Dec 17, 2007, 13:12 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

El 17/12/2007, a las 13:55, Johannes Schindelin escribió:
Show 25 quoted lines
> On Mon, 17 Dec 2007, Wincent Colaiuta wrote:
>
>> El 17/12/2007, a las 13:26, Johannes Schindelin escribi?:
>>
>>> I think it is wrong to go out of our way to support "git status - 
>>> p" as
>>> a synonym to "git -p status".  I simply do not believe that  
>>> newcomers
>>> are not intelligent enough to understand that "git -p <subcommand>"
>>> means that the output goes into their pager.
>>
>> But the point is, of all the special options, -p is the *only* that
>> can't unambiguously go after the subcommand.
>
> It should not be put after the subcommand.  That's my point.  Exactly
> because it is -- even conceptually -- no subcommand option.
>
> CVS has many shortcomings, but one lesson here is that people had no
> problems with "cvs -z3 update -d -P".  See, the "-z3" is an option  
> that
> has nothing to do with the subcommand.
>
> Exactly the same situation here.  I never had any problems  
> explaining why
> "-p" goes before the subcommand here.  Never.

Would be even better if there was nothing to explain and it all just worked, which is what I'm proposing here. As I said, -p is the only special option which clashes with any non-special uses.

But leaving -p aside, will you oppose any patches that make it possible for people to write stuff like:

git init --bare

Personally, I think this is an obvious usability improvement worth striving for. Given that "git --bare init" will continue to work under what I'm proposing, I really can't see any worthwhile argument against it. Because we're talking about a UI improvement for newcomers at no cost to old timers.

Cheers, Wincent

Johannes Schindelin· Dec 17, 2007, 13:30 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

Hi,
On Mon, 17 Dec 2007, Wincent Colaiuta wrote:
Show 8 quoted lines
> El 17/12/2007, a las 13:55, Johannes Schindelin escribi?:
> 
> > I never had any problems explaining why "-p" goes before the 
> > subcommand here.  Never.
> 
> Would be even better if there was nothing to explain and it all just 
> worked, which is what I'm proposing here. As I said, -p is the only 
> special option which clashes with any non-special uses.
Even in the best of circumstances, you have to teach people a little.
Show 10 quoted lines
> But leaving -p aside, will you oppose any patches that make it possible 
> for people to write stuff like:
> 
> git init --bare
> 
> Personally, I think this is an obvious usability improvement worth 
> striving for. Given that "git --bare init" will continue to work under 
> what I'm proposing, I really can't see any worthwhile argument against 
> it. Because we're talking about a UI improvement for newcomers at no 
> cost to old timers.

My reasoning is like as for "-p". "--bare" is not special to "init". It makes git not guess, but work on the current directory as a bare repository.

In my experience, it is easier to give people a clear-cut distinction between different concepts. Then way fewer surprises will hassle them later.

Ciao, Dscho

Wincent Colaiuta· Dec 17, 2007, 13:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

El 17/12/2007, a las 14:30, Johannes Schindelin escribió:
Show 18 quoted lines
>> But leaving -p aside, will you oppose any patches that make it  
>> possible
>> for people to write stuff like:
>>
>> git init --bare
>>
>> Personally, I think this is an obvious usability improvement worth
>> striving for. Given that "git --bare init" will continue to work  
>> under
>> what I'm proposing, I really can't see any worthwhile argument  
>> against
>> it. Because we're talking about a UI improvement for newcomers at no
>> cost to old timers.
>
> My reasoning is like as for "-p".  "--bare" is not special to  
> "init".  It
> makes git not guess, but work on the current directory as a bare
> repository.

Yes, I know what --bare does. It seems to me your argument is as follows:

- "git subcommand" is actually "git" (which does setup) followed by  
"git-subcommand" (which performs an action)[*]
- the newcomer should be taught this, and know which options go  
between "git" and "subcommand" and which options go after
- even though there is a straightforward way to have "git subcommand  
specials args" work without breaking compatibility, you oppose it on  
the grounds that users should know how Git works
- the fact that CVS also has some options that go before the  
subcommand makes it OK for git to do the same
- even though we could have a better UI that didn't require any of  
this explanation at all, we should keep the UI we have seeing as it is  
easy to explain
And possibly some combination of these two as well:
- requiring arguments to be inserted between "git" and "subcommand"  
doesn't undermine the effort to make git appear like a single tool to  
users
- the effort to move to dashless forms has nothing to do with  
presenting git as a single tool to users

Well, I basically disagree with you on most of these points; all but the first one(*), really. I think it's fairly clear what both you and I think on this and I'm not very interested in debating it further; I am more interested in hearing other people's points on this to get a feeling for whether there is a consensus (or lack thereof).

Cheers, Wincent

Johannes Sixt· Dec 17, 2007, 15:13 UTC · re: Wincent Colaiuta · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

Wincent Colaiuta schrieb:
Show 10 quoted lines
> But leaving -p aside, will you oppose any patches that make it possible
> for people to write stuff like:
> 
> git init --bare
> 
> Personally, I think this is an obvious usability improvement worth
> striving for. Given that "git --bare init" will continue to work under
> what I'm proposing, I really can't see any worthwhile argument against
> it. Because we're talking about a UI improvement for newcomers at no
> cost to old timers.

Your point. I hate to have to think hard each time whether it's "git --bare init" or "git init --bare" and "git clone --bare" or "git --bare clone" and wouldn't mind if I no longer needed to.

-- Hannes
Pierre Habouzit· Dec 17, 2007, 16:29 UTC · re: Johannes Sixt · lore

Re: [PATCH] Have a flag to stop the option parsing at the first argument.

On Mon, Dec 17, 2007 at 03:13:56PM +0000, Johannes Sixt wrote:
Show 15 quoted lines
> Wincent Colaiuta schrieb:
> > But leaving -p aside, will you oppose any patches that make it possible
> > for people to write stuff like:
> > 
> > git init --bare
> > 
> > Personally, I think this is an obvious usability improvement worth
> > striving for. Given that "git --bare init" will continue to work under
> > what I'm proposing, I really can't see any worthwhile argument against
> > it. Because we're talking about a UI improvement for newcomers at no
> > cost to old timers.
> 
> Your point. I hate to have to think hard each time whether it's "git --bare
> init" or "git init --bare" and "git clone --bare" or "git --bare clone" and
> wouldn't mind if I no longer needed to.
Seconded.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

← back to recent threads