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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2008, 07:25 UTC
Message-ID: <7v63vvkplm.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20080304231945.GC24131@artemis.madism.org>

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

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

Show 12 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 ;-)

Show 8 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?".

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.