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

[Junio C Hamano <gitster@pobox.com>]

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.

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

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