On Mon, Mar 10, 2008 at 07:25:25AM +0000, Junio C Hamano wrote: > Pierre Habouzit <madcoder@xxxxxxxxxx> 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. > - 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. > > Signed-off-by: Pierre Habouzit <madcoder@xxxxxxxxxx> > > --- > > > > 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 :) > > @@ -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. > 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@xxxxxxxxxx OOO http://www.madism.org
Attachment:
pgpZGzvKqq8Zp.pgp
Description: PGP signature
