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

From: Jeff King <peff@peff.net>
Date: Jul 10, 2008, 07:14 UTC
Message-ID: <20080710071418.GD3195@sigill.intra.peff.net>
In-Reply-To: <1215639514-1612-2-git-send-email-madcoder@debian.org>

On Wed, Jul 09, 2008 at 11:38:34PM +0200, Pierre Habouzit wrote:

Show 5 quoted lines

> 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:

Show 9 quoted lines

> -		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:

Show 7 quoted lines

>  	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