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

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