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

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

Hi Junio,

On Sun, 30 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > In that case, I would strongly advise to consider redesigning the API.
> 
> The API we currently have and is used by "log", "rev-list" and friends
> is to have setup_revisions() parse the av[], i.e. the textual API, and
> it is sufficient to hide from the caller the implementation detail of
> what bit rev_info structure has and which bits are flipped when reacting
> to say "--format=..."  option [*1*].

Yes, this (parsing options passed in as strings, with the very real
possibility of catching coding errors only at runtime) is the current way
the API is used.

Sometimes.

And sometimes not.

For example, in builtin/bisect.c's show_diff_tree() function, we *do* call
setup_revisions(), but with argc = 0. We set all the options beforehand to
avoid parsing, to avoid runtime-instead-of-compile-time errors.

The same holds true for builtin/merge.c's squash_message() function.

> As the implementaiton detail of which bits are flipped when reacting to
> each options is _not_ the API, we are essentially left with two choices:
> write this series to the current textual API, or invent an alternate API
> [*2*] and write this series to that new API.

You make it sound as if my goal was to imitate slavishly what that option
did that was passed to the Git command in the git-rebase--interactive.sh
script.

But that is not what my goal is.

My goal is to imitate the *intent* of the shell script. Faithfully, of
course. The fact that the shell script had no better way to access the
internal API than to call the Git command is just a red herring.

What I really want to do *is* to access the revision machinery, as bare
metal as possible, because sequencer *is bare metal, too*.

The current code fulfills that goal rather excellently.

Your suggested change to call the parser and pass plain options as plain
text really flies in the face of this goal.

Your suggested alternative is actually not necessary here, as the code
does exactly what it is supposed to do: it calls, from the internal
libgit.a, another part of libgit.a, and therefore it is totally legitimate
to use implementation details.

If my code were bleeding implementation details to the user interface, I
would agree with you that there is an issue.

But that code does not do that. To the contrary, it hides those
implementation details behind a rather simple user interface.

In the long run, I think you are correct in your fear that bits may be set
incorrectly.

The solution for that is, of course, not to rewrite the API. The solution
is to make the API less fragile.

To be explicit about the fragility in question: the API should not require
the pretty_given bit at all, but it should use an initialized
pretty_print_context within the rev_info struct as indicator that the
pretty print machinery should be used to print out commit messages.

There is something even more fragile about the current concept of parsing
--pretty: the fact that get_commit_format() sets a *file-local* variable
`user_format`, and that that variable is then used for formatting when
pretty_given = 1, is just asking for trouble.

This fragile aspect of the API simply dooms the revision API to suffer
side effects until fixed.

After writing this, I really, really, really fail even more to see why you
make such a big deal out of the pretty_given bit. It is insignificant. If
I were you, I would worry much, much, much, MUCH more about the fact that
`user_format` in pretty.c is changed implicitly by sequencer_make_script()
(not the fault of my patch, of course, but of the way get_commit_format()
operates).

Obviously this latter issue (the `user_format` side effect) is what is a
real problem later on, when we try to make rebase a true builtin, as
sequencer_make_script() will be called as part of a larger operation that
will subsequently run the rebase, and may very well use the revision
machinery to print other commit messages again, *possibly using that
`user_format` by mistake*.

Now, if your suggestion to undo the compile-time safety in favor of a
runtime error, say, in case of a speling eror (which I really would like
to avoid, as I find it a highly sloppy development style to turn compile
time errors into runtime errors for no good reason) would help avoid this
problem with the `user_format`, I would grudgingly bite my tongue,
implement what you suggested and move forward.

But it does not. All it does make the code less safe by pushing a possible
problem from the time of compilation to the time of running the code.
Meaning that problems would be found by users, when developers could have
caught them without this change.

I really wish we were on the same page that this is a really bad idea.

Ciao,
Dscho