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

Johannes Schindelin [off-list ref] writes:

Hi Junio,

On Mon, 29 May 2017, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

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. [off-list ref]

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.

Hi Junio,

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

Johannes Schindelin [off-list ref] writes:

quoted

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.  

In other words, you want me to put my name to sloppy code.

Sorry, not interested,
Dscho

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