Re: [PATCH v2 3/5] diffcore-rename: enable limiting rename detection to relevant destinations

From: Elijah Newren <newren@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"Taylor Blau" <me@ttaylorr.com>, "René Scharfe" <l.s.r@web.de>
Subject: Re: [PATCH v2 3/5] diffcore-rename: enable limiting rename detection to relevant destinations
Date: Thu, 3 Jun 2021 07:13:08 -0700	[thread overview]
Message-ID: <CABPp-BHaGEKYKCbwJxYgk7mY_mx6h9dSReRn=PXEpd0xCzqsdg@mail.gmail.com> (raw)
In-Reply-To: <1016e500-8169-7bb3-8428-b5ea35d19630@gmail.com>

On Thu, Jun 3, 2021 at 5:54 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 6/1/2021 10:58 AM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Our former optimizations focused on limiting rename detection to a
> > pre-specified set of relevant sources.  This was because the merge logic
> > only had a way of knowing which sources were relevant.  However, other
> > callers of rename detection might benefit from being able to limit
> > rename detection to a known set of relevant destinations.  In
> > particular, a properly implemented `git log --follow` might benefit from
> > such an ability.
>
> I would be interested in seeing such an improvement. It could also
> help the batch-download of missing blobs in a partial clone situation
> because it would only need the "deletes" portion of the diff, since we
> only care about one "add" (we'd need that blob, too, of course).

Oh, good point.  And in order to work well with the new prefetching
improvements (which will be the next series submitted), I should
really rename remove_unneeded_paths_from_src() to
remove_unneeded_paths() and add a relevant_destinations parameter and
use it, instead of having the strset_contains() check in
diffcore_rename_extended.

> > Since the code to implement such limiting is very similar to what we've
> > already done, just implement it now even though we do not yet have any
> > callers making use of this ability.
>
> However, I'm not sure that this change is warranted without such an
> integration. Perhaps keep this patch here on the list as a reference
> for anyone who wants to do the `git log --follow` speedup? This person
> can include "you in the future".
>
> I'm just worried that without a consumer, this code has no automated
> validation and can either be buggy now or become buggy in the future
> before someone starts trying to use the logic.

Fair enough; I'll pull it out.