On Wed, Oct 17, 2018 at 5:41 AM Derrick Stolee <stolee@xxxxxxxxx> wrote: > > 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. For those topics still in flight, I added re-defines, e.g. #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS #define get_merge_bases(r1, r2) repo_get_merge_bases(the_repository, r1, r2) #endif so the base function still keeps working, and we can cleanup multiple times, until eventually, we can get rid of the base function. > I had one high-level question: How are we testing that these "arbitrary > repository" changes are safe? I did the bare minimum in conversions in this series, such that the submodule code tests successfully. So if we'd revert some parts, the submodule tests would break. > 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. Or instead we could accelerate the long term plan of removing a hard coded the_repository and have each cmd builtin take an additional repository pointer from the init code, such that we'd bring all of Git to work on arbitrary repositories. Then the standard test suite should be okay, as there is no special case for the_repository any more. > 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. Yes, that is correct. We had a similar case with partial clone, as laid out in the cover letter for the RFC. I'll explore both the test tool approach as well as repository-fication of the code base. Thanks, Stefan
