[PATCH 0/3] diff topic tweaks

[PATCH 0/3] diff topic tweaks

[Wincent Colaiuta]

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

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

[Wincent Colaiuta]

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

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

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

[Wincent Colaiuta]

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

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

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

[Wincent Colaiuta]

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

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

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

[Junio C Hamano]

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.

> @@ -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.

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

[Wincent Colaiuta]

El 16/12/2007, a las 20:43, Junio C Hamano escribió:

> 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

Re: git.c option parsing

[Junio C Hamano]

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

Re: git.c option parsing

[Wincent Colaiuta]

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

Re: git.c option parsing

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

On Mon, Dec 17, 2007 at 09:01:56AM +0000, Wincent Colaiuta wrote:
> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: git.c option parsing

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1946 bytes --]

On Mon, Dec 17, 2007 at 09:20:13AM +0000, Pierre Habouzit wrote:
>   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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

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

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


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Johannes Schindelin]

Hi,

On Mon, 17 Dec 2007, Pierre Habouzit wrote:

> 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

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

[Wincent Colaiuta]

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?

Cheers,
Wincent

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

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.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2208 bytes --]

Hi,

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.
> 
>   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

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

[Wincent Colaiuta]

El 17/12/2007, a las 12:50, Johannes Schindelin escribió:

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

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

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

[Johannes Schindelin]

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.

Ciao,
Dscho

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

[Wincent Colaiuta]

El 17/12/2007, a las 13:26, Johannes Schindelin escribió:

> 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

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

[Johannes Schindelin]

Hi,

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.

Ciao,
Dscho

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

[Wincent Colaiuta]

El 17/12/2007, a las 13:55, Johannes Schindelin escribió:

> 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

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

[Johannes Schindelin]

Hi,

On Mon, 17 Dec 2007, Wincent Colaiuta wrote:

> 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.

> 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

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

[Wincent Colaiuta]

El 17/12/2007, a las 14:30, Johannes Schindelin escribió:

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

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

[Johannes Sixt]

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.

-- Hannes

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

On Mon, Dec 17, 2007 at 03:13:56PM +0000, Johannes Sixt wrote:
> 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]