Re: [PATCH] revisions: refactor handle_revision_opt into parse_revision_opt.

[Jeff King <peff@peff.net>]

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;
	}

But I wonder if it would be a suitable alternative to just add
"--reverse" in this case to the blame options, but with an option flag
for "parse me, but also pass me along to the next parser" (which would
be added). Then we could do our thing in a callback.

Of course, in this case, we do something a bit tricky by actually
_rewriting_ the argument to "--children". So we would have to have
support for callbacks rewriting arguments, or it would have to manually
do what "--children" should do. So perhaps it isn't worth the trouble.
This particular boilerplate is at least not very error-prone.

So food for thought, mainly, I suppose. Apologies if you already thought
of this and I missed the discussion. I think I am up to date on my
back-reading of the git list, but it is easy to lose some threads. :)

-Peff