Re: [PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.
On Thu, Jul 10, 2008 at 07:14:18AM +0000, Jeff King wrote:
Show 46 quoted lines
> On Wed, Jul 09, 2008 at 11:38:34PM +0200, Pierre Habouzit wrote:
>
> > It seems we're using handle_revision_opt the same way each time, have a
> > wrapper around it that does the 9-liner we copy each time instead.
> >
> > handle_revision_opt can be static in the module for now, it's always
> > possible to make it public again if needed.
>
> I have been on the road, and I finally got the chance to read through
> your whole parseopt/blame refactoring. I think it looks good overall, as
> do these patches.
>
> I have one comment, though I am not sure it is worth implementing.
>
> I was happy to see this refactoring, which I think improves readability:
>
> > - n = handle_revision_opt(&revs, ctx.argc, ctx.argv,
> > - &ctx.cpidx, ctx.out);
> > - if (n <= 0) {
> > - error("unknown option `%s'", ctx.argv[0]);
> > - usage_with_options(blame_opt_usage, options);
> > - }
> > - ctx.argv += n;
> > - ctx.argc -= n;
> > + parse_revision_opt(&revs, &ctx, options, blame_opt_usage);
>
> but it also seems like the top bit of that for loop is boilerplate, too:
>
> > for (;;) {
> > switch (parse_options_step(&ctx, options, shortlog_usage)) {
> > case PARSE_OPT_HELP:
> > exit(129);
> > case PARSE_OPT_DONE:
> > goto parse_done;
> > }
>
> AFAICT, the main reason for not folding this into your refactored
> function is that after the parse_options_step, but before we handle the
> revision arg to parse_revision_opt, there needs to be an opportunity for
> the caller to intercept and do something based on revision opts (like
> blame does with --reverse):
>
> if (!strcmp(ctx.argv[0], "--reverse")) {
> ctx.argv[0] = "--children";
> reverse = 1;
> } The other thing I would like to do is remove the exit(129) and replace
it with proper documentation for the rev-list options, and this will
depend upon the fact that we parse revisions or something else inside
the loop. Of course this is boilerplate, but well, I wouldn't like to
hide it and prevent people from thinking they can hook other things in
there.
And I'm not very keen on adding more options to parse-options like you
propose, our endgame is to get rid of parse_revision_opt in this form
and have a full parse-opt thing from top to bottom. the "--reverse" hack
could be done really differently, because we really know what
"--children" does and we could directly do what the revision option
parser does. But oh well... For now I'm more interested to see more
commands migrated thanks to this facility, and see what we can refactor
to get rid of the old parsers at once.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
Pierre Habouzit <madcoder@debian.org>