On Tue, Feb 20, 2018 at 11:00 AM, Brandon Williams <bmwill@xxxxxxxxxx> wrote: > On 02/20, Stefan Beller wrote: >> On Fri, Feb 16, 2018 at 2:34 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: >> > Hi, >> > >> > Stefan Beller wrote: >> > >> >> v2: >> >> * duplicated the 'ignore_env' bit into the object store as well >> >> * the #define trick no longer works as we do not have a "the_objectstore" global, >> >> which means there is just one patch per function that is converted. >> >> As this follows the same structure of the previous series, I am still confident >> >> there is no hidden dependencies to globals outside the object store in these >> >> converted functions. >> >> * rebased on top of current master, resolving the merge conflicts. >> >> I think I used the list.h APIs right, but please double check. >> > >> > For what it's worth, I think I prefer v1. I put some comments on why >> > on patch 0 of v1 and would be interested in your thoughts on them >> > (e.g. as a reply to that). I also think that even if we want to >> > switch to a style that passes around object_store separately from >> > repository, it is easier to do the migration in two steps: first get >> > rid of hidden dependencies on the_repository, then do the (simpler) >> > automatic migration from >> > >> > f(the_repository) >> > >> > to >> > >> > f(the_repository->object_store) >> > >> > *afterwards*. >> > >> > Thoughts? >> >> I would prefer to not spend more time on these conversions. >> If Duy and you would come to a conclusion to either pick this >> or the previous version I would be happy. >> >> I do not see the benefit in splitting up the series even further and >> do this multistep f(repo) -> f(object store), as the cost in potential >> merge conflicts is too high. Note that brian just sent another object >> id conversion series, also touching sha1_file.c, which I am sure will >> produce merge conflicts for Junio. >> >> For the next part 2 and onwards I'd be happy to take either this >> strategy or Duys strategy as requested. > > I think Jonathan is trying to point out that converting to f(repo) maybe > easier than converting to f(repo->object_store) upfront I agree. > which would make > it easier to write the patches (which most of them are already written) true, but for this series we also have the conversion to f(object_store) written already. > and to review because you can use the #define trick to make some sort of > guarantees. That is true, so it would be a tradeoff between reviewers and maintainers time? > After we have successfully completed the migration to f(repo), then we > can revisit the subsystems which want to have a clearer abstraction > layer and make the jump to f(repo->object_store). I would think we can take this series as-is and then move on with making f(repo) abstractions, eventually moving to f(specialized-subsystem) as those patches are not written yet (neither direct conversions, nor the repo trick; the patches I already have need adaption which takes enough time on its own.) > > -- > Brandon Williams
