Re: [PATCH] Make builtin-tag.c use parse_options.

From: Pierre Habouzit <madcoder@debian.org>
Date: Nov 12, 2007, 14:52 UTC
Message-ID: <20071112145252.GA343@artemis.corp>
In-Reply-To: <1b46aba20711120509l104792ebo4ea9a51c710510f3@mail.gmail.com>

On Mon, Nov 12, 2007 at 01:09:37PM +0000, Carlos Rica wrote:

Show 22 quoted lines

> 2007/11/10, Junio C Hamano <gitster@pobox.com>:
> > Carlos Rica <jasampler@gmail.com> writes:
> >
> > > Also, this removes those tests ensuring that repeated
> > > -m options don't allocate memory more than once, because now
> > > this is done after parsing options, using the last one
> > > when more are given. The same for -F.
> >
> > The reason for this change is...?  Is this because it is
> > cumbersome to detect and refuse multiple -m options using the
> > parseopt API?  If so, the API may be what needs to be fixed.
> > Taking the last one and discarding earlier ones feels to me an
> > arbitrary choice.
> 
> You can do many things with repeated options.
> Here in git-tag we considered two different ways to manage them:
> Concatenating values for the option and/or refusing more than one.
> I found that current option-parser can do both from the client
> using callbacks, as Pierre shows me, so I think it is the right way to do it.
> 
> Pierre, by default, I think that the parser should print an error
> when more than one option of the same type is given,

  I beg to differ. It makes sense for OPTION_STRING options, but not for
other. Though you cannot always detect that.

Also note that:
(1) repeating options was already silent in many git commands, so it's
    not really a regression ;
(2) for many commands it actually make sense to allow repeating (for
    _BOOLEAN e.g.). And I'd argue that for OPTION_BIT it also makes
    sense as well.

Show 9 quoted lines

> in order to report it to the command-line user, but make this
> behaviour optional for the programmer.  Specifically, I thought in
> this last option:
> 
> enum parse_opt_option_flags {
> 	PARSE_OPT_OPTARG   = 1,
> 	PARSE_OPT_NOARG    = 2,
> 	PARSE_OPT_ALLOWREP = 4
> };

  To do that you need to keep a list of the triggered commands to do
that, there is no way to achieve that reliably right now. As taking the
last one and discarding the other is the usual way for option parsers I
never saw this as a big issue.

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