Re: [PATCH v2 05/13] reftable: utility functions

Hi Junio,

On Fri, 2 Oct 2020, Junio C Hamano wrote:

You're right, sorry.

Yes, you were spot on.

I am rather strongly opposed to introducing duplicated functionality. Rather than spending time on finding half a dozen links to conversations in the past, let me try to make the case afresh:

I have a very concrete example of the undesirable consequences: the commit-graph breakage Stolee and I debugged yesterday, where a bug hid in `in_merge_bases_many()` for two years, hidden by the fact that there was _duplicate logic_ in `get_reachable_subset()`.

Pretty much _all_ of the code in `reftable/` is _very_ new, has not seen any real-world testing, and as such cannot really be trusted, in particular the code that duplicates functionality already present in libgit.a. Note that I do not claim that Han-Wen cannot be trusted: to the contrary, I trust him very much (we have a history going all the way back to Lilypond, the musical typesetter). The code, however, cannot be trusted, and needs to be reviewed and tested in practice.

The simple fact is that even the thorough review that the commit-graph patches received did not prevent the `min_generation`/`max_generation` bug in `in_merge_bases_many()` from entering the code base, and if we had not had _another_ function doing very, very similar things, that bug would most likely not have lingered for _this_ long.

Likewise for the reimplementation of the convenient functionality like `strbuf`. This kind of duplication (in the case of `struct strbuf`, quite _literally_: there are now confusingly _two_ `strbuf.h` files that even try to implement the _exact same_ API) is _prone_ to lead to stale, or long undiscovered, bugs in one of the duplicated implementations.

Which means that we're likely to introduce bugs with those new functions introduced in `reftable/`. For the reftable functionality, that has to be accepted. For functions that do the same as equivalents in libgit.a, we do not have to accept it.

In any case, duplicated functionality is a maintenance burden, and especially in this case, an unnecessary one at that: unlike xdiff or compat/regex/, the reftable functionality is co-dependent with `libgit.a`. Yes, you can implement libreftable as a stand-alone library (including duplicated functionality from libgit.a, as I pointed out), but you _cannot use it independently of Git_.

The reftable concerns _Git_ refs.

This is not a compressor that could come in handy in a messenger app, or a diff engine that could potentially help with a semantic merging tool.

It is code whose entire purpose is to manage boatloads of Git refs efficiently. Therefore, I see no convincing reason to insist on maintaining this code outside of git/git.

It would make more sense to maintain parse-options or strbuf or t/test-lib.sh outside of git/git (because at least that functionality is independent enough of Git to be potentially useful to other projects) and we don't. We maintain them inside git/git, and for good reason: it makes development and maintenance a hell of a lot easier.

And once we agree that reftable is intended to be so integral a part of Git that it should absolutely have its home inside git/git, all of that duplicated functionality (with almost totally untested implementations, at least when it comes to "battle testing in production") can totally evaporate and does not need to worry us any more.

And then we can start to spend reviewers' time on the actual code revolving around the actual reftables rather than having to bother with reviewing, say, a close, but non-identical `strbuf` that might even have "borrowed" parts of the `strbuf` code and put it under a more permissive license (I don't know, and I don't want to know, that's part of the reason why I don't want to review the reftable patches in their current form).

In other words, we can then finally start to actually review the reftable code on its merits of teaching Git a valuable new feature.

Sounds good to me.

Ciao, Dscho