Hi Jonathan, On Mon, 12 Oct 2020, Jonathan Nieder wrote: > Johannes Schindelin wrote: > > > The `merge_bases_many()` function has only 33 lines of code, partially > > duplicating `get_reachable_subset()`. Yet, it had a bug in it for two > > years that was not found. > > > > How much worse will the situation be with your 431 lines of code. > > > > Even more so when you consider the fact that you intend to shove the same > > duplication down libgit2's throat. It's "triplicating" code. > > Careful: you seem to be making a bunch of assumptions here (for > example, around Han-Wen having some intent around shoving things down > libgit2's throat), and you seem to be focusing on the person instead > of the contribution. > > Would you mind restating your point in a way that is a little easier > to process, for example by focusing on the effect that this patch > would have on you as a Git developer? I really hope that Han-Wen did not take this as a personal attack. In case he did, I'll gladly try to re-phrase my point: It does not matter how much code is duplicated. The fact that we run into bugs even in unintentionally duplicated code is reason enough to not lightly accept this design decision as being set in stone. Which is why I, you, Emily and Josh pointed that out, and it is the same reason why Peff and Junio had made the same point in the past. I was quite unpleasantly surprised to see that all of those objections seemed to be insufficient, hence my rather forceful reply. As to the libgit2 angle: libgit2 also has `strbuf` (which it calls `git_buf` IIRC), and I am certain that it also has those other helper functions à la `put_be32()`. So yes, the triplication I mentioned is a very real, undesirable feature of this patch series. To be honest, I would much rather spend my time reviewing patches that added `reftable` support using as much of libgit.a's functionality as possible rather than repeating the same point over and over again: that it makes very little sense _not_ to use that functionality. Of course, at this stage I understand that my feedback is not very welcome, so I will try to refrain from commenting on this patch series further (I do reserve the right to send patches that fix CI failures, just like I've done quite a few times up to this point). Ciao, Dscho
