Re: [RFH] convert shortlog to use parse_options

Pierre Habouzit [off-list ref] writes:

In fact we have kind of the issue for every single optional argument out
there:

$ git describe --abbrev HEAD
error: option `abbrev' expects a numerical value
[...]

  *ouch*

So I believe that with optional arguments we must change the way we do
things, and that we _must_ enforce the argument to be sticked in that
case.

I think "Must" is a bit too strong an expression.

	git describe --abbrev 7 HEAD
        git describe --abbrev HEAD
        git describe --abbrev=HEAD
	git describe --abbrev=7 HEAD
	git describe --abbrev

The --abbrev parser in this case could be asked with this question: "You
are on the command line.  There is a token after you.  Is it your
parameter?".

Among the above cases, the third one through the last one will get slightly
different questions.  The third and fourth ones get "You are given this
parameter and it must be yours", and the last one gets "You are on the
command line, and were not given any parameter."

The parser can do one of these things:

 * Inspect the token, if exists, and see if it is appropriate for it.

   * If not

     - if it is optional, then take the default value, and answer "I
       handled myself Ok, but that HEAD is not mine";

     - if it "must be yours" (the third case), barf.

   * If so

     - Use that given value and answer "I handled myself Ok, and that
       parameter 7 is mine";  this includes the fourth case as well.

And this does not have to be callback for common types like integers.

On Thu, Dec 13, 2007 at 05:40:23PM +0000, Junio C Hamano wrote:

Pierre Habouzit [off-list ref] writes:

quoted

In fact we have kind of the issue for every single optional argument out
there:

$ git describe --abbrev HEAD
error: option `abbrev' expects a numerical value
[...]

  *ouch*

So I believe that with optional arguments we must change the way we do
things, and that we _must_ enforce the argument to be sticked in that
case.

I think "Must" is a bit too strong an expression.

	git describe --abbrev 7 HEAD
        git describe --abbrev HEAD
        git describe --abbrev=HEAD
	git describe --abbrev=7 HEAD
	git describe --abbrev

The --abbrev parser in this case could be asked with this question: "You
are on the command line.  There is a token after you.  Is it your
parameter?".

  I thought of that, but it's really convoluted and can definitely lead
to very subtle issues. The number of git commands with optional
arguments is quite low, mostly due to legacy, I don't expect _new_
commands to take optional arguments. I don't really like the ambiguity
it creates, and in some cases you just won't be able to disambiguate at
all. Here it looks nice because --abbrev takes an integer argument, and
it's likely that no branch nor reference names will be only made of
digits. Though for commands taking an optional string[0] argument this is
way more fishy.

  *I* (and it's my opinion, maybe other don't see it that way) see the
parse-option module as a convenience given to people using the CLI UI in
an interactive shell. So it tries to achieve a good balance between
brevity and error detection. Here I think it's quite error prone and
gives almost no help to the user: if there is a gain to type git repack
-afd vs. git repack -a -f -d, I see no real gain in --abbrev 10 vs.
--abbrev=10.


  [0] OTOH I'm not sure there will ever be optional arguments that
      aren't integers in git, but I may be wrong.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

On Thu, Dec 13, 2007 at 06:03:47PM +0000, Pierre Habouzit wrote:

On Thu, Dec 13, 2007 at 05:40:23PM +0000, Junio C Hamano wrote:

quoted

Pierre Habouzit [off-list ref] writes:

quoted

In fact we have kind of the issue for every single optional argument out
there:

$ git describe --abbrev HEAD
error: option `abbrev' expects a numerical value
[...]

  *ouch*

So I believe that with optional arguments we must change the way we do
things, and that we _must_ enforce the argument to be sticked in that
case.

I think "Must" is a bit too strong an expression.

	git describe --abbrev 7 HEAD
        git describe --abbrev HEAD
        git describe --abbrev=HEAD
	git describe --abbrev=7 HEAD
	git describe --abbrev

The --abbrev parser in this case could be asked with this question: "You
are on the command line.  There is a token after you.  Is it your
parameter?".

The other issue is that when you had --abbrev=foo or --abbrev foo, the
first one has to generate an error, whereas the second one should just
say "foo" is not for me. The point being that the callback is not really
aware of how the argument got assigned to it.

That means that we will have to pass more flags to callabacks, making it
less easy to write them. Again, I'm not sure that the coding hassle is
worth of the small gain.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

On Thu, 2007-12-13 at 19:03 +0100, Pierre Habouzit wrote:

On Thu, Dec 13, 2007 at 05:40:23PM +0000, Junio C Hamano wrote:

quoted

Pierre Habouzit [off-list ref] writes:

quoted

In fact we have kind of the issue for every single optional argument out
there:

$ git describe --abbrev HEAD
error: option `abbrev' expects a numerical value
[...]

  *ouch*

So I believe that with optional arguments we must change the way we do
things, and that we _must_ enforce the argument to be sticked in that
case.

I think "Must" is a bit too strong an expression.

	git describe --abbrev 7 HEAD
        git describe --abbrev HEAD
        git describe --abbrev=HEAD
	git describe --abbrev=7 HEAD
	git describe --abbrev

The --abbrev parser in this case could be asked with this question: "You
are on the command line.  There is a token after you.  Is it your
parameter?".

  I thought of that, but it's really convoluted and can definitely lead
to very subtle issues. The number of git commands with optional
arguments is quite low, mostly due to legacy, I don't expect _new_
commands to take optional arguments. I don't really like the ambiguity
it creates, and in some cases you just won't be able to disambiguate at
all. Here it looks nice because --abbrev takes an integer argument, and
it's likely that no branch nor reference names will be only made of
digits. Though for commands taking an optional string[0] argument this is
way more fishy.

My 

On Thu, 2007-12-13 at 13:28 -0500, Kristian Høgsberg wrote:

On Thu, 2007-12-13 at 19:03 +0100, Pierre Habouzit wrote:

quoted

On Thu, Dec 13, 2007 at 05:40:23PM +0000, Junio C Hamano wrote:

quoted

Pierre Habouzit [off-list ref] writes:

quoted

In fact we have kind of the issue for every single optional argument out
there:

$ git describe --abbrev HEAD
error: option `abbrev' expects a numerical value
[...]

  *ouch*

So I believe that with optional arguments we must change the way we do
things, and that we _must_ enforce the argument to be sticked in that
case.

I think "Must" is a bit too strong an expression.

	git describe --abbrev 7 HEAD
        git describe --abbrev HEAD
        git describe --abbrev=HEAD
	git describe --abbrev=7 HEAD
	git describe --abbrev

The --abbrev parser in this case could be asked with this question: "You
are on the command line.  There is a token after you.  Is it your
parameter?".

  I thought of that, but it's really convoluted and can definitely lead
to very subtle issues. The number of git commands with optional
arguments is quite low, mostly due to legacy, I don't expect _new_
commands to take optional arguments. I don't really like the ambiguity
it creates, and in some cases you just won't be able to disambiguate at
all. Here it looks nice because --abbrev takes an integer argument, and
it's likely that no branch nor reference names will be only made of
digits. Though for commands taking an optional string[0] argument this is
way more fishy.

My 

Oops, sorry about that.  I just wanted to say we shouldn't jump through
all these hoops to make the option parser support every type of option
there ever was in the git command line ui.  A lot of these were probably
decided somewhat arbitrarily by whoever implemented the command.
Instead it's an opportunity to retroactively enforce some consistency
and predictability to the various option-styles that have been
hand-rolled over time in different git commands.

Kristian

On Thu, Dec 13, 2007 at 01:47:36PM -0500, Kristian Høgsberg wrote:

Oops, sorry about that.  I just wanted to say we shouldn't jump through
all these hoops to make the option parser support every type of option
there ever was in the git command line ui.  A lot of these were probably
decided somewhat arbitrarily by whoever implemented the command.
Instead it's an opportunity to retroactively enforce some consistency
and predictability to the various option-styles that have been
hand-rolled over time in different git commands.

I agree. I am already a little bit uncomfortable with the "--abbrev 10"
won't work but "--foo 10" will, because it requires that the user
remember which arguments are optional and which are required. But
switching it to "--abbrev 10 works, but --abbrev $foo does not, unless
of course $foo is an integer, in which case you must use --abbrev=$foo"
is just a little bit too DWIM. E.g., if you are scripting, it's just one
more source of error (if I have $foo, how must I write --abbrev $foo for
it to ensure that I _don't_ trigger the optional argument?).

-Peff