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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Thu, 27 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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.

In that case, I would strongly advise to consider redesigning the API. It
is just no good to ask for a change in stringent code that would delay
compile errors to runtime errors, that's just poor form.

And if the API allows settings that do something unintentional without at
least a runtime warning, the API is no good.

Ciao,
Dscho