revision API design, was Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

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

Hi Junio,

On Tue, 30 May 2017, Junio C Hamano wrote:

> We do not want these [revision API] 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.

Just to make sure we are on the same level: you want the argc & argv to be
the free-form API of setup_revisions(), rather than code setting fields
in struct rev_info whose names can be verified at compile time, and whose
names also suggest their intended use.

I am still flabberghasted that any seasoned software developer sincerely
would suggest this.

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

This is a very eloquent description of a problem with the API.

The correct suggestion would be to fix the API, of course. Not to declare
an interface intended for command-line parsing the internal API.

Maybe the introduction of `pretty_given` was a strong hint at a design
flaw to begin with, pointing to the fact that user_format is a singleton
(yes, really, you can't have two different user_formats in the same Git
process, what were we thinking)?

Ciao,
Dscho