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

Hi Junio,

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

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.

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