On 10/16/2018 7:35 PM, Stefan Beller wrote:
This series takes another approach as it doesn't change the signature of
functions, but introduces new functions that can deal with arbitrary
repositories, keeping the old function signature around using a shallow wrapper.
I think this is a good direction, and the changes look good to me.
Additionally each patch adds a semantic patch, that would port from the old to
the new function. These semantic patches are all applied in the very last patch,
but we could omit applying the last patch if it causes too many merge conflicts
and trickl in the semantic patches over time when there are no merge conflicts.
The semantic patches are a good idea. At some point in the future, we
can submit a series that applies the patches to any leftover calls and
removes the old callers. We can hopefully rely on review (and the
semantic patches warning that there is work to do) to prevent new
callers from being introduced in future topics. That doesn't count
topics that come around while this one isn't merged down.
I had one high-level question: How are we testing that these "arbitrary
repository" changes are safe? I just remember the issue we had with the
commit-graph code relying on arbitrary repositories, but then adding a
reference to the replace objects broke the code (because replace-objects
wasn't using arbitrary repos correctly, but the commit-graph was tested
with arbitrary repositories using "test-tool repository"). It would be
nice to introduce more method calls in t/helper/test-repository.c that
help us know these are safe conversions. Otherwise, we are essentially
waiting until we try to take submodule things in-process and find out
what breaks. As we discovered with the refstore, we can't just ensure
that all references to the_repository are removed.
Thanks,
-Stolee
