Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Mon, 29 May 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > diff --git a/sequencer.c b/sequencer.c
>> > index 130cc868e51..88819a1a2a9 100644
>> > --- a/sequencer.c
>> > +++ b/sequencer.c
>> > @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>> >  
>> >  	strbuf_release(&sob);
>> >  }
>> > +
>> > +int sequencer_make_script(int keep_empty, FILE *out,
>> > +		int argc, const char **argv)
>> > +{
>> > +	char *format = NULL;
>> > +	struct pretty_print_context pp = {0};
>> > +	struct strbuf buf = STRBUF_INIT;
>> > +	struct rev_info revs;
>> > +	struct commit *commit;
>> > +
>> > +	init_revisions(&revs, NULL);
>> > +	revs.verbose_header = 1;
>> > +	revs.max_parents = 1;
>> > +	revs.cherry_pick = 1;
>> > +	revs.limited = 1;
>> > +	revs.reverse = 1;
>> > +	revs.right_only = 1;
>> > +	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
>> > +	revs.topo_order = 1;
>> 
>> cf. <xmqq4lx5i83q.fsf@gitster.mtv.corp.google.com>
>> 
>> This is still futzing below the API implementation detail
>> unnecessarily.
>
> You still ask me to pass options in plain text that has to be parsed at
> run-time, rather than compile-time-verifiable flags.

Absolutely.  

We do not want these implementation details to code that does not
implement command line parsing.  This one is not parsing anybody's
set of options and should not be mucking with the low level
implementation details.

See 66b2ed09 ("Fix "log" family not to be too agressive about
showing notes", 2010-01-20) and poinder.  Back then, nobody outside
builtin/log.c and revision.c (these are the two primary things that
implement command line parsing; "log.c" needs access to the low
level details because it wants to establish custom default that is
applied before it reads options given by the end-user) mucked
directly with verbose_header, which allowed the addition of
"pretty_given" to be limited only to these places that actually do
the parsing.  If the above patch to sequencer.c existed before
66b2ed09, it would have required unnecessary change to tweak
"pretty_given" in there too when 66b2ed09 was done.  That is forcing
a totally unnecesary work.  And there is no reason to expect that
the kind of change 66b2ed09 made to the general command line parsing
will not happen in the future.  Adding more code like the above that
knows the implementation detail is unwarranted, when you can just
ask the existing command line parser to set them for you.