Hi Junio,
On Thu, 27 Apr 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schinde...@gmx.de> writes:
>
> > On Wed, 26 Apr 2017, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <johannes.schinde...@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
