Re: [PATCH] revisions: refactor init_revisions and setup_revisions.
On Mon, Mar 10, 2008 at 07:25:25AM +0000, Junio C Hamano wrote:
Show 28 quoted lines
> Pierre Habouzit <madcoder@debian.org> writes:
>
> struct rev_info gains two new fields:
>
> * .def to store --default argument;
> * .show_merge 1-bit field.
>
> The refactor consists of the following steps:
>
> * init_revisions now takes the --default argument to initialize .def
> properly, instead of getting it through setup_revisions.
>
> * setup_revisions has been split in two:
>
> - parse_revisions that does (almost) only argument parsing, to be more
> like what parse-options can do, and
>
> - setup_revisions that does the rest.
>
> Many places had no arguments to pass to setup_revisions, and those don't
> use parse_revisions at all.
>
> Conceptually, I like the split up into three, which are:
>
> - initializes the structure (init_revisions);
>
> - parses the user arguments (parse_revisions -- the naming could probably
> be improved, though);
Well, the goal is to replace that bit with a parse_options
structure/macro at some point, so the naming isn't really important.
Show 8 quoted lines
> - prepares the derived/derivable fields for the real work
> (setup_revisions).
>
> Was there a particular reason you moved default to init_revisions() and
> not kept it with setup_revisions()? All the callers of cmd_log_init()
> needs to say "I want to default to HEAD" because of it, instead of letting
> cmd_log_init() say it at only one place. Also the caller needs to decide
> upfront before calling init_revisions() what the default should be.
Yes, because the revision parsing has side effects on that default
parameter (the --default command line switch).
We could of course have a .def member in the struct rev_info, and use
the one passed to setup_revisions then if it's still NULL, but it
doesn't really makes sense to me, and I don't really see a problem with
saying at init time that you'll default to "HEAD". Though if you really
dislike it that much, I squash a patch that does that on top of it.
Show 16 quoted lines
> > Signed-off-by: Pierre Habouzit <madcoder@debian.org>
> > ---
> >
> > This is a required patch for revisions parse-optification that splits
> > the logic of revision arguments parsing in parsing, and post-processing.
> >
> > The aim is to replace parsing with parse-opt at some point.
> >
> > The final version (using parse-options) will probably need more rework
> > in revisions.[hc], but I'd like to avoid rebasing this patch over and
> > over, and I'd be glad if it's merged now, as it's not _that_
> > intrusive.
>
> > I've been using a git with this patch for a week without issues.
>
> .... which is a nice assuring comment ;-)
Well it has been even more since :)
Show 10 quoted lines
> > @@ -892,8 +893,7 @@ static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token
> > opt->regflags = REG_NEWLINE;
> > revs->grep_filter = opt;
> > }
> > - append_grep_pattern(revs->grep_filter, ptn,
> > - "command line", 0, what);
> > + append_grep_pattern(revs->grep_filter, ptn, "command line", 0, what);
> > }
>
> Made me go "huh?".
Huh yeah, sorry, that's totally spurious indeed.
Show 8 quoted lines
> But everything else looked pretty mechanical conversion, and in that sense
> it is not that intrusive.
>
> Applying this to 'master' and then merging 'pu' shows that there are a few
> topics that are cooking that would conflict with this change. Merging
> 'next' seems to go cleanly (I haven't checked the result), so it is not
> too bad for me to carrry this at this moment, if we were not this close to
> the rc freeze. I dunno.
Well I can wait longer, I'd just like to see it merged in a not too
far future, because I have to check for new places that would need
conversions at each reabase :) (though the init/setup_revisions
functions changed their number of arguments so I _think_ I should miss
none each time but …).
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
Pierre Habouzit <madcoder@debian.org>