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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 12, 2020, 19:01 UTC
Message-ID: <nycvar.QRO.7.76.6.2010122058100.50@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqeem3qttu.fsf@gitster.c.googlers.com>

Hi Junio,

On Mon, 12 Oct 2020, Junio C Hamano wrote:

Show 18 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> FWIW, the duplication is really tiny: according to
> >>
> >>  $ wc $(grep -l REFTABLE_STANDALONE *[ch])
> >>
> >> it's just 431 lines of code.
> >
> > 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.
>
> It does not affect the current discussion, but what you are giving
> is a revisionist view of the world.  The latter function came MUCH
> later to do a bit more than the former.  The bug was caused by the
> fact that those that added the latter neglected the responsibility
> of maintaining the former to the same degree when new feature like
> commit-graph were added to the latter.

You're right, I mixed up the direction: by introducing new code, the old code became under-tested and a bug creeped in that was not detected for two years.

> The root cause was that the latter one did not share the code with
> the former one when it was introduced.  That does make it appear
> similar to the situation we have at hand with duplicated utility
> functions.

Indeed, and even if this is just one concrete example, experience (and teachers and instructors and mentors) repeat the lesson that code duplication should be avoided because it _does_ cause problems.

Thanks for setting the record straight, Dscho

Show 11 quoted lines

>
> > 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.
> >
> > So I find the argument you made above quite unconvincing.
> >
> > Ciao,
> > Dscho
>