Message ID | 20241217-pks-use-the-repository-conversion-v1-0-0dba48bcc239@pks.im (mailing list archive) |
---|---|
State | New |
Headers | show |
On Tue, Dec 17, 2024 at 07:43:47AM +0100, Patrick Steinhardt wrote: > Hi, > > this small patch series performs some refactorings to stop using > `the_repository` in several subsystems. There wasn't really any > criterium for which subsystems I picked, except that all of them have > been trivial to convert. > > In this patch series I'm merely bubbling up `the_repository` one more > layer even though some calling contexts already have a repository > available. For the sake of triviality I decided not to handle these > cases though and instead let a future patch series worry about them. > Actually, I am excited to see that we remove the global variable "the_repository" in some subsystems because I have seen every patch with "<subsystem>: stop using `the_repository`". By this, we make the problem smaller, which is good. I have read through all the patches, which looks to me. Thanks, Jialuo
On Tue, Dec 17, 2024 at 08:45:54PM +0800, shejialuo wrote: > On Tue, Dec 17, 2024 at 07:43:47AM +0100, Patrick Steinhardt wrote: > > Hi, > > > > this small patch series performs some refactorings to stop using > > `the_repository` in several subsystems. There wasn't really any > > criterium for which subsystems I picked, except that all of them have > > been trivial to convert. > > > > In this patch series I'm merely bubbling up `the_repository` one more > > layer even though some calling contexts already have a repository > > available. For the sake of triviality I decided not to handle these > > cases though and instead let a future patch series worry about them. > > > > Actually, I am excited to see that we remove the global variable > "the_repository" in some subsystems because I have seen every patch with > "<subsystem>: stop using `the_repository`". > > By this, we make the problem smaller, which is good. I have read through > all the patches, which looks to me. Thanks for your review! Patrick
Patrick Steinhardt <ps@pks.im> writes: > Hi, > > this small patch series performs some refactorings to stop using > `the_repository` in several subsystems. There wasn't really any > criterium for which subsystems I picked, except that all of them have > been trivial to convert. > > In this patch series I'm merely bubbling up `the_repository` one more > layer even though some calling contexts already have a repository > available. For the sake of triviality I decided not to handle these > cases though and instead let a future patch series worry about them. > > This series is built on v2.48.0-rc0 with ps/build-sign-compare at > e03d2a9ccb (t/helper: don't depend on implicit wraparound, 2024-12-06) > merged into it. There's a single merge conflict with 'seen' that can be > solved like this: I went through half of the patches a week ago, but got back to reading through the series today. The approach here is to simply bubble up the usage of `the_repository` to upper layers and use `the_repository` there. The alternative approach would be to try and resolve the dependency on the upper layers and not use `the_repository`. This approach seems much safer. The patches look good to me. Thanks!
Karthik Nayak <karthik.188@gmail.com> writes: > I went through half of the patches a week ago, but got back to reading > through the series today. I've also been reading through the whole series today, and the changes are trivial and look good to me. > The approach here is to simply bubble up the usage of `the_repository` > to upper layers and use `the_repository` there. The alternative approach > would be to try and resolve the dependency on the upper layers and not > use `the_repository`. This approach seems much safer. The patches look > good to me. I took me a while to get into the mindset of taking this approach, but after chatting with Patrick I've changed my mind and agree with this approach. The goal of this series is to eliminate the use of `the_repository` in the mentioned subsystems. Simply bubbling up the use of that variable to the callers of those subsystems is very trivial and safe to do. -- Toon
Toon Claes <toon@iotcl.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> I went through half of the patches a week ago, but got back to reading >> through the series today. > > I've also been reading through the whole series today, and the changes > are trivial and look good to me. > >> The approach here is to simply bubble up the usage of `the_repository` >> to upper layers and use `the_repository` there. The alternative approach >> would be to try and resolve the dependency on the upper layers and not >> use `the_repository`. This approach seems much safer. The patches look >> good to me. > > I took me a while to get into the mindset of taking this approach, but > after chatting with Patrick I've changed my mind and agree with this > approach. The goal of this series is to eliminate the use of > `the_repository` in the mentioned subsystems. Simply bubbling up the use > of that variable to the callers of those subsystems is very trivial and > safe to do. Yup. That way, the conversion would be bug-to-bug compatible, which is much better than a rewrite that improves some parts while by mistake breaks the existing code. Thanks, all. Let's mark it for 'next'.
diff --cc pack-bitmap.c index 48e3b99efb,7d8f910588..0000000000 --- a/pack-bitmap.c