Jeff King <peff@xxxxxxxx> writes: > On Mon, Jun 19, 2017 at 11:45:50AM +0200, Johannes Schindelin wrote: > >> The reason for this suggestion is that one of the revision machinery's >> implementation details is an ugly little semi-secret: the pretty-printing >> machinery uses a global state, and that is why we need the "pretty_given" >> flag in the first place. > > I think that's mis-stating Junio's complaint. The point is not the > pretty_given flag itself, which we know about and can work around. The > point is that we don't know what other similar problems we have or will > have due to future changes in the revision code. > > In other words, there are two APIs: the one where C code manipulates > rev_info directly, and the one where revision.c responds to string > arguments. From a maintenance perspective, it is easy for somebody make > a change that works for the latter but not the former. There are (1) the API for users of revision traversal machinery and (2) the implementation detail of the API. Of course, the code that actually parses the plumbing and end-user supplied set of options needs to manipulate rev_info directly, but we should avoid direct manipulation when we can to future-proof the codebase. My main complaint is that Johannes's "compiler can catch mistakes" is true only for the most trivial kind of mistakes. The current implementation detail for reacting to --format="<string>" given by the revision traversal machinery happens to be to set three things in rev_info structure: set verbose_header and pretty_given to 1 and then call get_commit_format() on the format string. Dscho is correct to say that the compiler can catch a mistake that misspells, say verbose_header as verbos_header. The compiler would not catch an equivalent mistake to misspell the option as --formta="<string>", so from that point of view, his argument may seem to make sense. But the compiler would not help catching a copy-and-paste mistake to do only two out of the three things needed (e.g. forgetting to set pretty_given). If the code relies on setup_revisions() to react to options just the way "rev-list" plumbing does, such a mistake cannot happen in the first place---there is no need to worry about "catching". As you clarified correctly, the writer of the code that _uses_ the machinery, like the one in sequencer_make_script(), cannot anticipate how the internal implementation of reacting to the features will change, and more importantly, it should not have to know how it will change. By using the setup_revisions() API that uses the exactly the same parser as plumbing command "git rev-list" does, the forward compatibility is guaranteed. Copying and pasting the current internal implementation detail will break that. > I do agree that the lack of compile-time safety for obvious mistakes > like "--pertty" is a downside, though. On the other hand, there are > strong run-time checks there, so the tests would catch it. True. After setup_revisions() returns, if there are unrecognized options (e.g. misspelt "--pertty"), that can be used as the indication of a programming error, as the new code is not even parsing arbitrary set of options given by the end-user.
