Re: [PATCH v2 0/4] First steps towards partial clone submodules

From: Elijah Newren <newren@gmail.com>
Date: Jun 9, 2021, 00:07 UTC
Message-ID: <CABPp-BF97Ej+WrgSrumz9DZpeg_wgKVhvPatiHn4GvSyraymag@mail.gmail.com>
In-Reply-To: <xmqq4ke7ordl.fsf@gitster.g>

On Tue, Jun 8, 2021 at 4:42 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Mon, Jun 7, 2021 at 5:26 PM Jonathan Tan <jonathantanmy@google.com> wrote:
> >>
> >> Thanks everyone for your reviews. I believe I've addressed all review
> >> comments, including the one from Elijah about the test failing with
> >> sha256 (which turns out to be because I didn't add a call to
> >> setup_git_directory(), which the other test helpers do).
> >
> > Thanks for fixing those up.  I spotted some minor nits/questions, but
> > nothing big.
> >
> > Looks like Junio did spot some bigger items...which raises a question
> > for me.  I have a series ...
>
> Do you mean, by "bigger items", that we may want to turn it around
> to have repo extension data to the in-core repository structure?

Yes.

Show 14 quoted lines

> > (https://lore.kernel.org/git/pull.969.git.1622856485.gitgitgadget@gmail.com/)
> > that also touches partial clones.  Our series are semantically
> > independent, but we both add a repository parameter to
> > fetch_objects().  So we both make the same change, but you also make
> > additional nearby changes, resulting in two trivial conflicts.  ...
>
> I can sort of see how the above plan would work if we are not going
> to fix the "keep only the partialclone related extension thing,
> instead of solving the larger structural problem that the current
> arrangement ignores that repository extensions are per repository".
> But wouldn't that leave us with two series with technical debt?
> Also, if Jonathan's series fixes the "bigger item", would the above
> "proceed more or less independently or rebase one on top of the
> other" plan work well without making the same fix in yours?

My series is completely independent of the partialclone extension stuff.

My series merely adds the recording of a single statistic (number of fetched objects) to the partial clone stuff; everything else is higher level diffcore-rename and merge-ort stuff.

Show 7 quoted lines

> I guess a better first step would be to stop, think and decide what
> to do with the "bigger" thing---if only to dismiss it with a firm
> declaration that we would never do such a fix and move extension
> data piecemeal to relevant subsystems, so that we'd reduce conflicts
> in the future, as I am reasonably sure that the "bigger item" will
> be tempting to fix even after the two series lands, and doing so at
> that time would be twice larger surgery.

I don't understand how you think the partialclone extension stuff is relevant to my series at all. My changes to promisor-remote.c are just a couple lines, and if he expands or rearranges his work, the amount of conflicts can't really get any bigger because there's only a few lines on my side for it to conflict with.