Re: [PATCH] convert shortlog to use parse_options

Jeff King [off-list ref] writes:

The behavior should be identical, except that we now support
option bundling (e.g., "git shortlog -nse").

Sorry, but this breaks

	git shortlog -n -s -e --no-merges v1.5.4..

On Sat, Mar 01, 2008 at 01:37:26AM -0800, Junio C Hamano wrote:

quoted

The behavior should be identical, except that we now support
option bundling (e.g., "git shortlog -nse").

Sorry, but this breaks

	git shortlog -n -s -e --no-merges v1.5.4..

I didn't test this very extensively (obviously!) since it was just a
respin of an old patch. Sorry to waste your time with something that is
so obviously broken.

I started to write a patch to give parse-options a "stop processing at
the first unknown option" flag, but it was very unsatisfactory.
Specifically:

  - the final parser has to know that it's the last, and complain about
    unrecognized options. In this case, setup_revisions would be the
    second and final parser, and it of course doesn't do this (though
    perhaps one could check the residual options from setup_revisions
    manually and barf on that)

  - the fact that we have two sets of parsed options can't be
    transparent to the user. We'd stop at "--no-merges", which means you
    can't say "git shortlog --no-merges -e", which is silly. But we
    can't possibly continue, since we don't know if "-e" is another
    option or an argument there.

And it doesn't work to do the setup_revisions first, since there is a
conflict over the "-n" option (and we can't even munge the result
afterwards, since one version takes an argument and one doesn't).

So I think using parse-options here should be put on hold until we have
the revision and diff parameters in a parseopt-understandable form. I
would think we could do something like:

#define OPT__REVISION(x) \
        OPT_BOOLEAN(0, "no-merges", &(x)->no_merges, "don't show merges"),
        OPT_BOOLEAN(0, "boundary", &(x)->boundary, "show boundary commits"),
        ...

and we could have unified options tables. I seem to recall some work
being done in this area early on in the parse-options history, but I
can't seem to find any mention of it in the list archive. Pierre, does
this ring a bell?

-Peff

On Sun, Mar 02, 2008 at 06:15:42AM +0000, Jeff King wrote:

#define OPT__REVISION(x) \
        OPT_BOOLEAN(0, "no-merges", &(x)->no_merges, "don't show merges"),
        OPT_BOOLEAN(0, "boundary", &(x)->boundary, "show boundary commits"),
        ...

and we could have unified options tables. I seem to recall some work
being done in this area early on in the parse-options history, but I
can't seem to find any mention of it in the list archive. Pierre, does
this ring a bell?

  Yes, I didn't had the time to finish that, I just started some ground
works in the diff options area, I hope I didn't lost that work, it's
probably somewhere on my public repository. Though revision parsing are
special because of --not, but I think the proper solution wrt --not and
--all in revision parsing is to ask parse-opt to "let" some options stay
as arguments, and do the final revision parsing with them kept.

  Like you may have noticed, I didn't have a lot of time for git
recently, and that's a shame :| But maybe your mail will beat me into
working on this again now that 1.5.4 is released, because I also feared
that reworking diff and revision parsing options will probably introduce
quite a few regressions, and it's rather better doing so at the
beginning of a release cycle :)

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

On Sun, Mar 02, 2008 at 08:11:40AM +0000, Pierre Habouzit wrote:

On Sun, Mar 02, 2008 at 06:15:42AM +0000, Jeff King wrote:

quoted

#define OPT__REVISION(x) \
        OPT_BOOLEAN(0, "no-merges", &(x)->no_merges, "don't show merges"),
        OPT_BOOLEAN(0, "boundary", &(x)->boundary, "show boundary commits"),
        ...

and we could have unified options tables. I seem to recall some work
being done in this area early on in the parse-options history, but I
can't seem to find any mention of it in the list archive. Pierre, does
this ring a bell?

  In fact everything is here:
http://git.madism.org/?p=~madcoder/git.git;a=shortlog;h=refs/heads/ph/parseopt

  I just resent the first patch that I sent (with a bug) not so long
time ago. The last one is the current state of work for diff opts.

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