Re: [PATCH] revisions: refactor init_revisions and setup_revisions.

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 4229 bytes --]

On Mon, Mar 10, 2008 at 07:25:25AM +0000, Junio C Hamano wrote:
> 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.

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

> > @@ -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@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]