threads / discuss / 10310

[parse-options] proposal for merge, take 1

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

## tl;dr

24 messages between Oct 16, 2007 and Oct 17, 2007.

replies: 23people: 7as markdown or json

Pierre Habouzit· Oct 16, 2007, 08:16 UTC · lore
  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(-)
Pierre Habouzit· Oct 16, 2007, 08:20 UTC · re: Pierre Habouzit · lore

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

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
Pierre Habouzit· Oct 16, 2007, 08:43 UTC · re: Pierre Habouzit · lore

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

On mar, oct 16, 2007 at 08:20:10 +0000, Pierre Habouzit wrote:
Show 9 quoted lines
> 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
Pierre Habouzit· Oct 16, 2007, 08:45 UTC · lore

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

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
Johannes Schindelin· Oct 16, 2007, 13:18 UTC · re: Pierre Habouzit · lore

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

Hi,
On Tue, 16 Oct 2007, Pierre Habouzit wrote:
Show 13 quoted lines
> 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

René Scharfe· Oct 16, 2007, 16:38 UTC · re: Pierre Habouzit · lore

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

Pierre Habouzit schrieb:
Show 10 quoted lines
> 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é
Johannes Schindelin· Oct 16, 2007, 16:44 UTC · re: René Scharfe · lore

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

Hi,
On Tue, 16 Oct 2007, Ren? Scharfe wrote:
Show 14 quoted lines
> 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

Pierre Habouzit· Oct 16, 2007, 16:53 UTC · re: Johannes Schindelin · lore

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

On Tue, Oct 16, 2007 at 04:44:44PM +0000, Johannes Schindelin wrote:
Show 23 quoted lines
> 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
Nicolas Pitre· Oct 16, 2007, 17:21 UTC · re: Pierre Habouzit · lore

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

On Tue, 16 Oct 2007, Pierre Habouzit wrote:
Show 30 quoted lines
> 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
René Scharfe· Oct 16, 2007, 17:04 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 22 quoted lines
> 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é
Nicolas Pitre· Oct 16, 2007, 16:44 UTC · re: René Scharfe · lore

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

On Tue, 16 Oct 2007, René Scharfe wrote:
Show 14 quoted lines
> 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
Pierre Habouzit· Oct 16, 2007, 16:50 UTC · re: René Scharfe · lore

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

On Tue, Oct 16, 2007 at 04:38:36PM +0000, René Scharfe wrote:
Show 14 quoted lines
> 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
Shawn O. Pearce· Oct 17, 2007, 04:44 UTC · re: Pierre Habouzit · lore

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

Pierre Habouzit <madcoder@debian.org> wrote:
Show 15 quoted lines
> 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.
Johannes Schindelin· Oct 17, 2007, 18:00 UTC · re: Shawn O. Pearce · lore

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

Hi,
On Wed, 17 Oct 2007, Shawn O. Pearce wrote:
Show 20 quoted lines
> 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

Michael Witten· Oct 16, 2007, 08:55 UTC · lore

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

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"

Michael Witten· Oct 16, 2007, 09:36 UTC · re: Michael Witten · lore

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

On 16 Oct 2007, at 4:55:41 AM, Michael Witten wrote:
Show 7 quoted lines
> 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"

Johannes Schindelin· Oct 16, 2007, 13:17 UTC · re: Michael Witten · lore

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

Hi,
On Tue, 16 Oct 2007, Michael Witten wrote:
Show 6 quoted lines
> 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

Michael Witten· Oct 16, 2007, 15:36 UTC · re: Johannes Schindelin · lore

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

On 16 Oct 2007, at 9:17:29 AM, Johannes Schindelin wrote:
Show 13 quoted lines
> 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.
Pierre Habouzit· Oct 16, 2007, 16:55 UTC · re: Michael Witten · lore

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

On mar, oct 16, 2007 at 08:55:41 +0000, Michael Witten wrote:
Show 7 quoted lines
> 
> 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
Alex Riesen· Oct 16, 2007, 17:56 UTC · lore

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

Pierre Habouzit, Tue, Oct 16, 2007 10:39:37 +0200:
Show 6 quoted lines
> 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.

Pierre Habouzit· Oct 16, 2007, 22:15 UTC · re: Alex Riesen · lore

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

On Tue, Oct 16, 2007 at 05:56:50PM +0000, Alex Riesen wrote:
Show 9 quoted lines
> 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
Alex Riesen· Oct 17, 2007, 19:06 UTC · re: Pierre Habouzit · lore

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

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.
Shawn O. Pearce· Oct 17, 2007, 07:24 UTC · lore

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

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.
Pierre Habouzit· Oct 17, 2007, 07:52 UTC · re: Shawn O. Pearce · lore

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

On Wed, Oct 17, 2007 at 07:24:19AM +0000, Shawn O. Pearce wrote:
Show 18 quoted lines
> 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

← back to recent threads