Re: [PATCH v3 1/9] rebase -i: generate the script via rebase--helper

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

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

> Hi Junio,
>
> On Wed, 26 Apr 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> 
>> > diff --git a/sequencer.c b/sequencer.c
>> > index 77afecaebf0..e858a976279 100644
>> > --- a/sequencer.c
>> > +++ b/sequencer.c
>> > @@ -2388,3 +2388,48 @@ 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 = xstrdup("%s");
>> > +	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;
>> > +
>> > +	revs.pretty_given = 1;
>> > +	git_config_get_string("rebase.instructionFormat", &format);
>> > +	get_commit_format(format, &revs);
>> > +	free(format);
>> > +	pp.fmt = revs.commit_format;
>> > +	pp.output_encoding = get_log_output_encoding();
>> 
>> All of the above feels like inviting unnecessary future breakages by
>> knowing too much about the implementation the current version of
>> revision.c happens to use.
>
> You mean that the `--reverse` option gets translated into the `reverse`
> bit, and the other settings?

Yes.  The "pretty_given" trick is one example that the underlying
implementation can change over time.  If you wrote this patch before
66b2ed09 ("Fix "log" family not to be too agressive about showing
notes", 2010-01-20) happened, you wouldn't have known to flip this
bit on to emulate the command line parsing of "--pretty" and
friends, and you would have required the author of that change to
know that you have this cut & pasted duplicated code here when the
commit is primarily about updating revision.c

So I am very serious when I say that this is adding an unnecessary
maintenance burden.