| Message ID | 20241119-374-refactor-midx-c-and-midx-write-c-to-not-depend-on-global-state-v2-0-e2f607174efc@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Change midx.c and midx-write.c to not use global variables | expand |
On Tue, Nov 19, 2024 at 4:38 PM Karthik Nayak <karthik.188@gmail.com> wrote: > > Similar to the earlier patch series on cleaning up packfile.c and > removing usage of global variables [1], we change the midx.c and > midx-write.c files to no longer use global variables. > > This is done by the following: > - Usage of repository variable already available in existing structs. > - Passing down repository variable from other subsystems. > - Modifying all subcommands to obtain repository variable from the > command in `builtins/` and passing down the variable from there. > > The biggest change is in the first commit, wherein we modify all > subcommands to add the repository variable. Since the subcommand > definition are not often changed, it shouldn't cause too many conflicts > with in flight topics. Overall I like the way it's done. I found a few improvements that could be made to some commit messages though. Thanks.
On Wed, Nov 20, 2024 at 07:20:33PM +0100, Christian Couder wrote: > On Tue, Nov 19, 2024 at 4:38 PM Karthik Nayak <karthik.188@gmail.com> wrote: > > > > Similar to the earlier patch series on cleaning up packfile.c and > > removing usage of global variables [1], we change the midx.c and > > midx-write.c files to no longer use global variables. > > > > This is done by the following: > > - Usage of repository variable already available in existing structs. > > - Passing down repository variable from other subsystems. > > - Modifying all subcommands to obtain repository variable from the > > command in `builtins/` and passing down the variable from there. > > > > The biggest change is in the first commit, wherein we modify all > > subcommands to add the repository variable. Since the subcommand > > definition are not often changed, it shouldn't cause too many conflicts > > with in flight topics. > > Overall I like the way it's done. I found a few improvements that > could be made to some commit messages though. Same here! Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Wed, Nov 20, 2024 at 07:20:33PM +0100, Christian Couder wrote: > >> Overall I like the way it's done. I found a few improvements that >> could be made to some commit messages though. > > Same here! Thanks, all. It seems to me that we are getting fairly close to the goal post on this topic?
Karthik Nayak <karthik.188@gmail.com> writes: > Since the `packfile.c` cleanup is still in flight, this series is based > on top of master: b31fb630c0 (Merge https://github.com/j6t/git-gui, > 2024-11-11) with those patches merged in. This applies cleanly on top > of next, but conflicts with `bf/set-head-symref` in seen, the conflict > is mostly straight forward. I'll merge the topic in if it is merged into > next soon. I think set_head() part is fairly trivial. But there seems to be a much bigger conflicts with the incremental midx topic. I actually do not understand offhand why doing anything to the midx layer needs to change the calling convention of set_head() or many other things that are unrelated to what midx layer does, and that do *not* use the new parameter *anyway*. Shouldn't the "subsubcommand can inherit the repository from the builtin subcommand implementation" be split out as a separate topic, which the midx thing later builds on? Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> Since the `packfile.c` cleanup is still in flight, this series is based >> on top of master: b31fb630c0 (Merge https://github.com/j6t/git-gui, >> 2024-11-11) with those patches merged in. This applies cleanly on top >> of next, but conflicts with `bf/set-head-symref` in seen, the conflict >> is mostly straight forward. I'll merge the topic in if it is merged into >> next soon. > > I think set_head() part is fairly trivial. But there seems to be a > much bigger conflicts with the incremental midx topic. > I see that you've merged the topic to seen and the merge resolution looks ok to me, I'll also merge in `tb/incremental-midx-part-2` for my next version. > I actually do not understand offhand why doing anything to the midx > layer needs to change the calling convention of set_head() or many > other things that are unrelated to what midx layer does, and that do > *not* use the new parameter *anyway*. Shouldn't the "subsubcommand > can inherit the repository from the builtin subcommand > implementation" be split out as a separate topic, which the midx > thing later builds on? > That was just the progression of how I worked on these patches. My goal was to remove global variables, the subsubcommand can inherit just was a dependency which I bundled along, since it was a single patch. There is no reason it cannot be split, Let me go ahead and do that.