[parse-options] proposal for merge, take 1

[parse-options] proposal for merge, take 1

[Pierre Habouzit]

  Hi, thanks to the tremendous help I received from Dscho to make its
API and internals even better, I believe that my parseopt branch is
ready for inclusion.

  There is only _one_ small backward incompatibility (if other ones,
then it's not done on purpose and should be fixed) in git-show-ref: it
had a -h short option, synonym of --head. I made this -H because -h is
automatically diverted to "show usage" now, and it's counter-intuitive
anyways. I rebased -i my series so that this patch comes last if people
think it's a bad idea, so that the rest can be still be merged.

  Note that patch 7/25 has a small discussion in it.

  The series comes in 2 parts: the parse-options construction, split
into small pieces to introduce each feature. And the migration of a few
builtins. Diffstats and shortlogs follow.

  The branch is available from git://git.madism.org/git.git in the topic
branch ph/parseopt. I just rebased it onto the last "next" spearce
produced.

  Thanks to Alex Riesen, Johannes Schindelin, Jonas Fonseca and Kristian
Høgsberg for their help, patches and advices.

==============================================================================
Part 1
------------------------------------------------------------------------------

  Alex Riesen (1):
      Rework make_usage to print the usage message immediately

  Johannes Schindelin (2):
      Add tests for parse-options.c
      parse-options: Allow abbreviated options when unambiguous

  Pierre Habouzit (5):
      Add a simple option parser.
      parse-options: be able to generate usages automatically
      Add shortcuts for very often used options.
      parse-options: make some arguments optional, add callbacks.
      parse-options: allow callbacks to take no arguments at all.

  .gitignore               |    1 +
  Makefile                 |    6 +-
  parse-options.c          |  328 ++++++++++++++++++++++++++++++++++++++++++++++
  parse-options.h          |   70 ++++++++++
  t/t0040-parse-options.sh |   92 +++++++++++++
  test-parse-options.c     |   35 +++++
  6 files changed, 529 insertions(+), 3 deletions(-)


==============================================================================
Part 2
------------------------------------------------------------------------------

  Jonas Fonseca (1):
      Update manpages to reflect new short and long option aliases

  Kristian Høgsberg (1):
      Port builtin-add.c to use the new option parser.

  Pierre Habouzit (16):
      Make builtin-rm.c use parse_options.
      Make builtin-mv.c use parse-options
      Make builtin-branch.c use parse_options.
      Make builtin-describe.c use parse_options
      Make builtin-fetch.c use parse_options.
      Make builtin-revert.c use parse_options.
      Make builtin-update-ref.c use parse_options
      Make builtin-symbolic-ref.c use parse_options.
      Make builtin-http-fetch.c use parse_options.
      Make builtin-for-each-ref.c use parse-opts.
      Make builtin-fsck.c use parse_options.
      Make builtin-count-objects.c use parse_options.
      Make builtin-name-rev.c use parse_options.
      Make builtin-pack-refs.c use parse_options.
      Make builtin-show-ref.c use parse_options [small backward incompatibility].
      Make builtin-pack-objects.c use parse_options.

  Documentation/git-add.txt          |    4 +-
  Documentation/git-branch.txt       |    2 +-
  Documentation/git-mv.txt           |    2 +-
  Documentation/git-rm.txt           |    4 +-
  Documentation/git-show-ref.txt     |    4 +-
  Documentation/git-symbolic-ref.txt |    2 +-
  builtin-add.c                      |   70 +++------
  builtin-branch.c                   |  147 +++++++------------
  builtin-count-objects.c            |   32 ++--
  builtin-describe.c                 |   70 ++++-----
  builtin-fetch.c                    |  146 ++++++------------
  builtin-for-each-ref.c             |  138 ++++++++----------
  builtin-fsck.c                     |   80 +++-------
  builtin-http-fetch.c               |   65 ++++-----
  builtin-mv.c                       |   84 +++++------
  builtin-name-rev.c                 |   64 +++-----
  builtin-pack-objects.c             |  294 +++++++++++++++++-------------------
  builtin-pack-refs.c                |   47 +++----
  builtin-revert.c                   |   67 ++++-----
  builtin-rm.c                       |   54 +++----
  builtin-show-ref.c                 |  127 ++++++----------
  builtin-symbolic-ref.c             |   52 +++----
  builtin-update-ref.c               |   65 +++-----
  23 files changed, 654 insertions(+), 966 deletions(-)

Re: [parse-options] proposal for merge, take 1

[Pierre Habouzit]

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

On mar, oct 16, 2007 at 08:16:31 +0000, Pierre Habouzit wrote:
>   The branch is available from git://git.madism.org/git.git in the topic
> branch ph/parseopt. I just rebased it onto the last "next" spearce
> produced.

>       Make builtin-pack-objects.c use parse_options.

  ERRR DON'T TAKE 25/25, it's WIP and is broken atm, format-patch again
hasn't DWIM. It is not pushed to git://git.madism.org/git.git btw.

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

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

Re: [parse-options] proposal for merge, take 1

[Pierre Habouzit]

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

On mar, oct 16, 2007 at 08:20:10 +0000, Pierre Habouzit wrote:
> On mar, oct 16, 2007 at 08:16:31 +0000, Pierre Habouzit wrote:
> >   The branch is available from git://git.madism.org/git.git in the topic
> > branch ph/parseopt. I just rebased it onto the last "next" spearce
> > produced.
> 
> >       Make builtin-pack-objects.c use parse_options.
> 
>   ERRR DON'T TAKE 25/25, it's WIP and is broken atm, format-patch again
> hasn't DWIM. It is not pushed to git://git.madism.org/git.git btw.

okay I'm wrong, sorry for the alarm, I just generated the --stat and shortlog
too broadly, but 25/25 is definitely OK.

Proper stats for part2 are:

  Jonas Fonseca (1):
      Update manpages to reflect new short and long option aliases

  Kristian Høgsberg (1):
      Port builtin-add.c to use the new option parser.

  Pierre Habouzit (15):
      Make builtin-rm.c use parse_options.
      Make builtin-mv.c use parse-options
      Make builtin-branch.c use parse_options.
      Make builtin-describe.c use parse_options
      Make builtin-fetch.c use parse_options.
      Make builtin-revert.c use parse_options.
      Make builtin-update-ref.c use parse_options
      Make builtin-symbolic-ref.c use parse_options.
      Make builtin-http-fetch.c use parse_options.
      Make builtin-for-each-ref.c use parse-opts.
      Make builtin-fsck.c use parse_options.
      Make builtin-count-objects.c use parse_options.
      Make builtin-name-rev.c use parse_options.
      Make builtin-pack-refs.c use parse_options.
      Make builtin-show-ref.c use parse_options [small backward incompatibility].

    Documentation/git-add.txt          |    4 +-
    Documentation/git-branch.txt       |    2 +-
    Documentation/git-mv.txt           |    2 +-
    Documentation/git-rm.txt           |    4 +-
    Documentation/git-show-ref.txt     |    4 +-
    Documentation/git-symbolic-ref.txt |    2 +-
    builtin-add.c                      |   70 ++++++-----------
    builtin-branch.c                   |  147 +++++++++++++-----------------------
    builtin-count-objects.c            |   32 ++++----
    builtin-describe.c                 |   70 +++++++----------
    builtin-fetch.c                    |  146 ++++++++++++------------------------
    builtin-for-each-ref.c             |  138 +++++++++++++++-------------------
    builtin-fsck.c                     |   80 ++++++-------------
    builtin-http-fetch.c               |   65 +++++++---------
    builtin-mv.c                       |   84 +++++++++------------
    builtin-name-rev.c                 |   64 ++++++----------
    builtin-pack-refs.c                |   47 +++++-------
    builtin-revert.c                   |   67 +++++++---------
    builtin-rm.c                       |   54 ++++++--------
    builtin-show-ref.c                 |  127 +++++++++++--------------------
    builtin-symbolic-ref.c             |   52 +++++--------
    builtin-update-ref.c               |   65 ++++++----------
    22 files changed, 515 insertions(+), 811 deletions(-)

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

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

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Pierre Habouzit]

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

This bit is to allow to aggregate options with arguments together when
the argument is numeric.

    +#if 0
    +		/* can be used to understand -A1B1 like -A1 -B1 */
    +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
    +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
    +			return 0;
    +		}
    +#endif

I'm not a huge fan, but people may like it. Feel free to keep the
chunk, drop it, or enable it to your liking.


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

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

Re: [PATCH 09/25] Port builtin-add.c to use the new option parser.

[Michael Witten]

On 16 Oct 2007, at 4:39:42 AM, Pierre Habouzit wrote:

> +	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files  
> that git already knows about"),

"update only files in the current directory that git already knows  
about"

Re: [PATCH 09/25] Port builtin-add.c to use the new option parser.

[Michael Witten]

On 16 Oct 2007, at 4:55:41 AM, Michael Witten wrote:

> On 16 Oct 2007, at 4:39:42 AM, Pierre Habouzit wrote:
>
>> +	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only  
>> files that git already knows about"),
>
> "update only files in the current directory that git already knows  
> about"

Better grammar:

"update only files that git already knows about in the current  
directory"

Re: [PATCH 09/25] Port builtin-add.c to use the new option parser.

[Johannes Schindelin]

Hi,

On Tue, 16 Oct 2007, Michael Witten wrote:

> On 16 Oct 2007, at 4:39:42 AM, Pierre Habouzit wrote:
> 
> > +	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files
> > that git already knows about"),
> 
> "update only files in the current directory that git already knows about"

"update tracked files"

Ciao,
Dscho

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Johannes Schindelin]

Hi,

On Tue, 16 Oct 2007, Pierre Habouzit wrote:

> This bit is to allow to aggregate options with arguments together when
> the argument is numeric.
> 
>     +#if 0
>     +		/* can be used to understand -A1B1 like -A1 -B1 */
>     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
>     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
>     +			return 0;
>     +		}
>     +#endif
> 
> I'm not a huge fan, but people may like it. Feel free to keep the
> chunk, drop it, or enable it to your liking.

FWIW I like it.  It allows me to aggregate options such as -M30 with other 
short options.

Ciao,
Dscho

Re: [PATCH 09/25] Port builtin-add.c to use the new option parser.

[Michael Witten]

On 16 Oct 2007, at 9:17:29 AM, Johannes Schindelin wrote:

> Hi,
>
> On Tue, 16 Oct 2007, Michael Witten wrote:
>
>> On 16 Oct 2007, at 4:39:42 AM, Pierre Habouzit wrote:
>>
>>> +	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files
>>> that git already knows about"),
>>
>> "update only files that git already knows about in the current  
>> directory"
>
> "update tracked files"

"update tracked files in ."

;-)


Consider the description for git-commit's -a:

> Tell the command to automatically stage files that have been
> modified and deleted, but new files you have not told git about
> are not affected.

However, that's not what -u does.

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[René Scharfe]

Pierre Habouzit schrieb:
> This bit is to allow to aggregate options with arguments together when
> the argument is numeric.
> 
>     +#if 0
>     +		/* can be used to understand -A1B1 like -A1 -B1 */
>     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
>     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
>     +			return 0;
>     +		}
>     +#endif

I don't like it, it complicates number options with unit suffixes (e.g.
--windows-memory of git-pack-objects).

René

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Johannes Schindelin]

Hi,

On Tue, 16 Oct 2007, Ren? Scharfe wrote:

> Pierre Habouzit schrieb:
> > This bit is to allow to aggregate options with arguments together when
> > the argument is numeric.
> > 
> >     +#if 0
> >     +		/* can be used to understand -A1B1 like -A1 -B1 */
> >     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
> >     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
> >     +			return 0;
> >     +		}
> >     +#endif
> 
> I don't like it, it complicates number options with unit suffixes (e.g.
> --windows-memory of git-pack-objects).

Why?  It only means that you cannot say -W10mxabc instead of -W10m xabc.  

Remember: this is a special case for OPT_INTEGER.  Nothing to do with 
OPT_SIZE, which you'd probably implement as a callback.

Ciao,
Dscho

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Nicolas Pitre]

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

On Tue, 16 Oct 2007, René Scharfe wrote:

> Pierre Habouzit schrieb:
> > This bit is to allow to aggregate options with arguments together when
> > the argument is numeric.
> > 
> >     +#if 0
> >     +		/* can be used to understand -A1B1 like -A1 -B1 */
> >     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
> >     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
> >     +			return 0;
> >     +		}
> >     +#endif
> 
> I don't like it, it complicates number options with unit suffixes (e.g.
> --windows-memory of git-pack-objects).

This is my opinion too.


Nicolas

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Pierre Habouzit]

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

On Tue, Oct 16, 2007 at 04:38:36PM +0000, René Scharfe wrote:
> Pierre Habouzit schrieb:
> > This bit is to allow to aggregate options with arguments together when
> > the argument is numeric.
> > 
> >     +#if 0
> >     +		/* can be used to understand -A1B1 like -A1 -B1 */
> >     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
> >     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
> >     +			return 0;
> >     +		}
> >     +#endif
> 
> I don't like it, it complicates number options with unit suffixes (e.g.
> --windows-memory of git-pack-objects).

  Oh yeah, you're right, well you example is not an issue, but indeed
you pointed out a real probable issue:

  With that chunk, if an option that takes now an integer, becomes an
option with a suffix, we would then break backward compatibility. I'm
not sure I'm clear, but if you had a -S option (for size) that for now
gets sizes in kilooctet, git foo -S1000. Then if we decide that it's
worth understanting M/G/.. suffixes for this option, we would make the
type of the option be a CALLBACK using git_parse_ulong. This would mean
that with such a change:

before:
  git foo -S1000a would mean git foo -S1000 -a

After:
  git foo -S1000a would be rejected because 'a' isn't a valid size
  suffix.

  This of course become worse if we take git foo -S1000k where the
breakage would be silent.


  This is a very strong argument _against_ this chunk IMO.

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

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

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Pierre Habouzit]

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

On Tue, Oct 16, 2007 at 04:44:44PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 16 Oct 2007, Ren? Scharfe wrote:
> 
> > Pierre Habouzit schrieb:
> > > This bit is to allow to aggregate options with arguments together when
> > > the argument is numeric.
> > > 
> > >     +#if 0
> > >     +		/* can be used to understand -A1B1 like -A1 -B1 */
> > >     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
> > >     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
> > >     +			return 0;
> > >     +		}
> > >     +#endif
> > 
> > I don't like it, it complicates number options with unit suffixes (e.g.
> > --windows-memory of git-pack-objects).
> 
> Why?  It only means that you cannot say -W10mxabc instead of -W10m xabc.  
> 
> Remember: this is a special case for OPT_INTEGER.  Nothing to do with 
> OPT_SIZE, which you'd probably implement as a callback.

  Yeah but the point is that you can't migrate an option currently being
an integer to an OPT_SIZE because of that (see my other mail). Meaning
that once an argument is of type OPT_INTEGER you can't change it's type
in the future _AT ALL_ without breaking backward compatibility badly.
I'd say it's a rather sucky design.

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

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

Re: [PATCH 09/25] Port builtin-add.c to use the new option parser.

[Pierre Habouzit]

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

On mar, oct 16, 2007 at 08:55:41 +0000, Michael Witten wrote:
> 
> On 16 Oct 2007, at 4:39:42 AM, Pierre Habouzit wrote:
> 
> >+	OPT_BOOLEAN('u', NULL, &take_worktree_changes, "update only files that 
> >git already knows about"),
> 
> "update only files in the current directory that git already knows about"

  As a general rule, as I'm not a native speaker, I'd be glad if someone
went through the diffs and made it better english. I'm not sure what the
more efficient way to merge those corrections is, as I'm not able to see
if it's any better or not. So the someone mergint it (Shawn ?) may have
to go through every OPTION_* and check the helps strings, and --amend
fixes on top of the patches if needed. It looks like the best way to me.

  Alternatively, a native speaker could propose a 26th patch that fixes
all of the bad strings on top of this series. But again, I won't do
that, not because of lazyness, just because I can't decide :)

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

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

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[René Scharfe]

Johannes Schindelin schrieb:
> Hi,
> 
> On Tue, 16 Oct 2007, Ren? Scharfe wrote:
> 
>> Pierre Habouzit schrieb:
>>> This bit is to allow to aggregate options with arguments together when
>>> the argument is numeric.
>>>
>>>     +#if 0
>>>     +		/* can be used to understand -A1B1 like -A1 -B1 */
>>>     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
>>>     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
>>>     +			return 0;
>>>     +		}
>>>     +#endif
>> I don't like it, it complicates number options with unit suffixes (e.g.
>> --windows-memory of git-pack-objects).
> 
> Why?  It only means that you cannot say -W10mxabc instead of -W10m xabc.  
> 
> Remember: this is a special case for OPT_INTEGER.  Nothing to do with 
> OPT_SIZE, which you'd probably implement as a callback.

You mean I need to take a look at the actual patch to get a bit more
context? ;-)  Now that I did, I retract my comment.

Pierre, FYI: I didn't see your patches coming through the NNTP gateway
of gmane.org, which is my way of reading this list.  Its web interface
doesn't show them, either, so it's probably not caused by my news reader.

René

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Nicolas Pitre]

On Tue, 16 Oct 2007, Pierre Habouzit wrote:

> On Tue, Oct 16, 2007 at 04:44:44PM +0000, Johannes Schindelin wrote:
> > Hi,
> > 
> > On Tue, 16 Oct 2007, Ren? Scharfe wrote:
> > 
> > > Pierre Habouzit schrieb:
> > > > This bit is to allow to aggregate options with arguments together when
> > > > the argument is numeric.
> > > > 
> > > >     +#if 0
> > > >     +		/* can be used to understand -A1B1 like -A1 -B1 */
> > > >     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
> > > >     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
> > > >     +			return 0;
> > > >     +		}
> > > >     +#endif
> > > 
> > > I don't like it, it complicates number options with unit suffixes (e.g.
> > > --windows-memory of git-pack-objects).
> > 
> > Why?  It only means that you cannot say -W10mxabc instead of -W10m xabc.  
> > 
> > Remember: this is a special case for OPT_INTEGER.  Nothing to do with 
> > OPT_SIZE, which you'd probably implement as a callback.
> 
>   Yeah but the point is that you can't migrate an option currently being
> an integer to an OPT_SIZE because of that (see my other mail). Meaning
> that once an argument is of type OPT_INTEGER you can't change it's type
> in the future _AT ALL_ without breaking backward compatibility badly.
> I'd say it's a rather sucky design.

And what's the point of supporting so criptic arguments?
It doesn't have to go that far.


Nicolas

Re: [PATCH 04/25] Rework make_usage to print the usage message immediately

[Alex Riesen]

Pierre Habouzit, Tue, Oct 16, 2007 10:39:37 +0200:
> From: Alex Riesen <raa.lkml@gmail.com>
> 
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
>  parse-options.c |   48 ++++++++++++++++++++++--------------------------
>  1 files changed, 22 insertions(+), 26 deletions(-)

Got it three times: you put git@kernel.org into To: and Cc:

Why stderr, BTW? For instance, the output from "git help" is on
stdout. To be fair, I don't know why it is stdout there either.

Re: [PATCH 04/25] Rework make_usage to print the usage message immediately

[Pierre Habouzit]

On Tue, Oct 16, 2007 at 05:56:50PM +0000, Alex Riesen wrote:
> Pierre Habouzit, Tue, Oct 16, 2007 10:39:37 +0200:
> > From: Alex Riesen <raa.lkml@gmail.com>
> > 
> > Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> > ---
> >  parse-options.c |   48 ++++++++++++++++++++++--------------------------
> >  1 files changed, 22 insertions(+), 26 deletions(-)
> 
> Got it three times: you put git@kernel.org into To: and Cc:

  sorry, but it's not the reason, I sent it three time, there was a
problem with Shawn O. Pierce, it wasn't escaped properly and VGER ate
the mails because of that.

> Why stderr, BTW? For instance, the output from "git help" is on
> stdout. To be fair, I don't know why it is stdout there either.
  because it's what usage() does already ?
> 

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

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Shawn O. Pearce]

Pierre Habouzit <madcoder@debian.org> wrote:
> On Tue, Oct 16, 2007 at 04:38:36PM +0000, René Scharfe wrote:
> > Pierre Habouzit schrieb:
> > > This bit is to allow to aggregate options with arguments together when
> > > the argument is numeric.
> > > 
> > >     +#if 0
> > >     +		/* can be used to understand -A1B1 like -A1 -B1 */
> > >     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
> > >     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
> > >     +			return 0;
> > >     +		}
> > >     +#endif
> > 
> > I don't like it, it complicates number options with unit suffixes (e.g.
> > --windows-memory of git-pack-objects).
...
>   This is a very strong argument _against_ this chunk IMO.

Since everyone (including myself) is apparently strongly against this
hunk I removed it when I cherry-picked this series from Pierre into
my tree.  The series will be in my pu tonight, but minus this hunk.

-- 
Shawn.

Re: [PATCH 01/25] Add a simple option parser.

[Shawn O. Pearce]

Pierre Habouzit <madcoder@debian.org> wrote:
> The option parser takes argc, argv, an array of struct option
> and a usage string. ...

OK, I've chewed down some version of this series.  ;-)

To be more specific I fetched ph/parseopt (11b83dc4da) from your
tree at git://git.madism.org/git.git and split it apart somewhat.
All of the patches were rebased onto my most recent master but I
also yanked the two that impacted the builtin-fetch series out and
layered them over a merge of db/fetch-pack and my version of your
ph/parseopt series.

Why?  Well I want to keep our options open about which series
graduates to master first.  Although builtin-fetch has been cooking
for a while there's been a number of issues with that code.
There exists a (perhaps small) chance that ph/parseopt will be
ready before db/fetch-pack.

Currently ph/parseopt is in pu.  Tomorrow I'll look at the usage
strings in more depth and see if any improvements can be easily
made.  I already made one suggested by Dscho in builtin-add.

-- 
Shawn.

Re: [PATCH 01/25] Add a simple option parser.

[Pierre Habouzit]

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

On Wed, Oct 17, 2007 at 07:24:19AM +0000, Shawn O. Pearce wrote:
> Pierre Habouzit <madcoder@debian.org> wrote:
> > The option parser takes argc, argv, an array of struct option
> > and a usage string. ...
> 
> OK, I've chewed down some version of this series.  ;-)
> 
> To be more specific I fetched ph/parseopt (11b83dc4da) from your
> tree at git://git.madism.org/git.git and split it apart somewhat.
> All of the patches were rebased onto my most recent master but I
> also yanked the two that impacted the builtin-fetch series out and
> layered them over a merge of db/fetch-pack and my version of your
> ph/parseopt series.
> 
> Why?  Well I want to keep our options open about which series
> graduates to master first.  Although builtin-fetch has been cooking
> for a while there's been a number of issues with that code.
> There exists a (perhaps small) chance that ph/parseopt will be
> ready before db/fetch-pack.

  Yes, this makes sense, otoh the migration of the fetch commands are
really independant so you can put those appart, even if I end up needing
to rewrite them it's not an issue.

> Currently ph/parseopt is in pu.  Tomorrow I'll look at the usage
> strings in more depth and see if any improvements can be easily
> made.  I already made one suggested by Dscho in builtin-add.

  wonderful, thanks.

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

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

Re: [PATCH 07/25] parse-options: make some arguments optional, add callbacks.

[Johannes Schindelin]

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

Hi,

On Wed, 17 Oct 2007, Shawn O. Pearce wrote:

> Pierre Habouzit <madcoder@debian.org> wrote:
> > On Tue, Oct 16, 2007 at 04:38:36PM +0000, René Scharfe wrote:
> > > Pierre Habouzit schrieb:
> > > > This bit is to allow to aggregate options with arguments together when
> > > > the argument is numeric.
> > > > 
> > > >     +#if 0
> > > >     +		/* can be used to understand -A1B1 like -A1 -B1 */
> > > >     +		if (flag & OPT_SHORT && opt->opt && isdigit(*opt->opt)) {
> > > >     +			*(int *)opt->value = strtol(opt->opt, (char **)&opt->opt, 10);
> > > >     +			return 0;
> > > >     +		}
> > > >     +#endif
> > > 
> > > I don't like it, it complicates number options with unit suffixes (e.g.
> > > --windows-memory of git-pack-objects).
> ...
> >   This is a very strong argument _against_ this chunk IMO.
> 
> Since everyone (including myself)

... except for me ...

> is apparently strongly against this hunk I removed it when I 
> cherry-picked this series from Pierre into my tree.  The series will be 
> in my pu tonight, but minus this hunk.

I can live without this hunk.

Ciao,
Dscho

Re: [PATCH 04/25] Rework make_usage to print the usage message immediately

[Alex Riesen]

Pierre Habouzit, Wed, Oct 17, 2007 00:15:55 +0200:
> > Why stderr, BTW? For instance, the output from "git help" is on
> > stdout. To be fair, I don't know why it is stdout there either.
>   because it's what usage() does already ?

fair enough.