Hello everyone! It’s hard to believe how quickly summer has passed. It feels like I was writing my first blog yesterday. Since then, it has been a great journey! I feel very honoured to have been a part of Google Summer of Code with Git. I am extremely thankful to my mentor, Christian Couder from GitLab, who guided me through this project with patience and also encouragement.
So, I took the project of refactoring the global state in Git, i.e, trying to remove the global variables and placing them in their respective contexts. This is a long and ongoing project in Git which will still be continued in the organization after my project ends.
It was fun and challenging contributing to Git. The contribution workflow is quite different as compared to other organizations and the reviewing process takes quite some time. However, I got used to it quickly. This process, although slow, is very thorough. And having a good amount of active contributors, there are a lot of reviews and we get to see everyone’s perspective. Along with communicating regularly with my mentor, I really didn’t felt stressed at any time. There were some moments where it was a bit challenging but we got through it! Will talk about it more later in this blog
Here are the patch series I posted throught my GSoC and their status:
I also have a patch series currently in development which aims to remove the #define USE_THE_REPOSITORY_VARIABLE definition from some files so I will be sending it soon.
Although I enjoyed this journey, there were a few challenges which I faced. Here are some of them:
Trying to determine which global variables to remove. This was confusing because before selecting something, you don’t actually know how complex it would be to remove a specific variable. For eg, It was fairly easy for me to remove the variable core_preload_index but it was difficult to choose it because I went through a lot other global variables before this. Variables like is_bare_repository_cfg are significantly difficult to remove because there is a lot of code around it, and it’s just not easy as you have to make changes to a lot of existing logic.
Trying to determine where to place a specific global variable. There were 3 options for the most of the time: 1. Placing it in struct repository. 2. Placing it in struct repo_settings. 3. Localizing the variable by reading it there itself (mostly when it’s used very sparsely). The thing with 3rd option is that it’s easy to integrate but it has a problem regarding user experience, as if the config for that variable is entered invalid, the user might experience the error mid operation and not while they’re setting the config. The other problem which happened was I placed variables around sparse checkout in the struct repo_settings initially. A lot later I received comments that this may look better if it was placed in the struct repository. So, right now, I’m working on this.
Sometimes when you’re doing some change, there’s a certain catch to that change which you know might be questioned by someone and it’s hard to address because you don’t know any other way to do that change, and when you post it to the mailing list, you get that question. And the thing with me is that I always felt very unsure to reply immediately because I thought maybe I haven’t checked thoroughly or maybe I need to give more time and think about the reply and the problem. I really love the reviews because they challenge me to think better and I also appreciate everyone in the community taking out their time to review it, not just the maintainer.
Another place where I was struggled was when there were multiple patch series in review, I felt it was difficult for me to do the context switch everytime there was a review on a particular patch and then make changes again.
One area I could have done better was posting my blogs on time. While I started off well, there were phases where I didn’t have significant updates to share, which made me procrastinate on writing.
I absolutely loved working on Git this summer, and I’ve come to really appreciate the community around it. I definitely want to continue contributing, as I’m really interested in the internals of Git and excited about bringing new ideas and features to the project. One of my most memorable moments was spotting a bug, fixing it myself, and seeing the change go through. It was incredibly motivating and made me want to keep doing more of that work.
Going forward, I plan to:
Work on reducing global state wherever possible.
I want to learn more about the internals, see what changes can I do to improve the performance or maybe add new features.
Dive deeper into the sequencer and commit-related code. As I went through that part while fixing the bug I found, I felt I could add more features there maybe?
I’m really grateful to my mentor from GitLab, Christian Couder, for guiding me throughout this journey. We used to have weekly meetings where he answered my questions and shared his insights. He also taught me how to write good commits, which is something I’ll carry forward in all my future work. His feedback and encouragement made a huge difference. I made a lot of silly mistakes in my code and he always pointed them out before I sent it to the mailing list ;)
A big thanks as well to the Git community. Every time someone took the time to review my patches or share their thoughts, it pushed me to think harder and improve my work. I especially want to thank Junio, Phillip, and Patrick for their valuable feedback and guidance. It has been amazing to be part of such a supportive and knowledgeable group of people.
And of course, thanks to Google Summer of Code for giving me this opportunity. It has been an incredible summer, and I’m really excited to keep contributing!
Thanks for reading the blog! -Ayush:)
]]>Hello peeps!
This week, I was working on a patch to remove the global variable merge_log_config. As I said last week, I created a function in config.c to replace the usage of the variable. I got reviews from my mentor Christian, who said that I should move that function to fmt-merge-msg.c and since the merge_log_config variable is used to just set the value of the shortlog_len variable in cmd_merge() and cmd_fmt_merge_msg(), I should modify the function to return the value of the shortlog_len instead. I also removed the dependency on the_repository in builtin/fmt-merge-msg.c and wrote a test to make sure that git fmt-merge-msg -h can be called outside a repository. I sent the patch series to the mailing list: builtin/fmt-merge-msg: remove dependency on global variables and ‘the_repository’.
This changes the behaviour a bit which is addressed by Junio:
One obvious behaviour change I can see can happen when you have an invalid value set to merge.summary and run the command with command line override with the “–log” option. In the current code, the config callback barfs when it notices an invalid merge.summary setting, even though it won’t be used because the valid value given via the “–log” option would override it. In the updated code, adjust_shortlog_len() would short-circuit and does not even bother reading from the configuration, so the user will not be notified of a broken configuration.
So, I think we need to get more reviews to reach a consensus that this behaviour is okay.
I also received reviews from Junio regarding calling repo_config() after parse_options() as it is a bad idea to read the command-line options first and then reading values from the configuration. This might override the options given by the user in command-line. Although we might go ahead without facing any bugs for now, future developers might face issues when trying to add new options. So he asked me to go through how repo_config() is working without a repository and also look at some patches related to it where they allowed NULL repository to pass to it.
That’s it for this week. Thank you for reading my blog!
-Ayush:)
]]>Hello peeps!
So this week I sent the v6 of environment: remove sparse-checkout related global variables which I had prepared last week. I received comments from Junio saying that he did not mean to say that I should minimize the calls to prepare_repo_settings(), but what he meant was that we have calls to it all over the place, and we should try to centralize it. He asked me to explain why I decided to store these variables in the struct repo_settings and not the struct repository, to which Derrick replied saying that even he thinks that these variables should be a part of the struct repository because they are a core part of the repository. He also told me that the intention of the struct repo_settings was to store optional one-off feature flags, and if we’re trying to store core settings in it, then we should call prepare_repo_settings() when initializing the repository.
Here’s the message by Derrick:
I think that the core issue here (and probably causing the issues that were seen in the user-facing issues) is that the repo settings struct was intended as a place to fill config for some one-off “feature flags” and not to replace core functionality for a repo.
There are two ways to change the approach here to fix the problem of needing prepare_repo_settings() everywhere:
With the idea that these sparse-checkout variables are critical to the functionality of the repo, they should move into the repository struct itself and be initialized along with all other values there. This changes the patches (and my follow-up series) significantly, but mechanically.
If we are going to change the intention of the repo settings struct to move from “optional one-off feature flags” to “important information about the core behavior of a repo” then we should prepare_repo_settings() when initializing the repository struct.
My preference is (1). The only argument for (2) that I can think of is that it is sometimes helpful to share only the settings for a repo without sharing the whole repo. But that seems like a weak reason right now.
I also started working on a new topic which is to remove the global variable merge_log_config. So I tried creating a function in config.c which returns the value of the setting merge.log or merge.summary and using this function in builtin/fmt-merge-msg.c and merge.c replacing the use of the global variable.
My patch series: commit: improve behaviour of core.commentChar=auto for comments in commit messages, also got merged to master!
Thank you for reading my blog!
-Ayush:)
]]>Hello folks!
So last week, I had sent an RFC Post to have a discussion on what my approach should be for the commit: avoid scanning trailing comments when ‘core.commentChar’ is “auto” patch. It had two approaches mentioned, one which was ignoring the auto keyword for comment character when there are conflict comments. Other one was just resetting the comment_line_str variable to “#” when commentChar is set or parsed as auto. The post sadly did not get any comments. But it’s fine. Phillip had sent a patch about deprecating the auto keyword from Git, so then I discussed about it in my patch series with him and he was leaning strongly towards the latter approach.
So I made a patch which was now v4 and sent it to the mailing list. I did not realise it at first but the changes in the patch were the same as what Phillip once told me. This was pointed out to me by Junio, so I said that I do think that Phillip should actually get the primary credit for this patch and I gave credit to him in the commit message and I got his signed-offtrailer as well. So, this was sent as v5 and Phillip will rebase his patch series on top of mine and this will get merged to next!
I was also preparing a v6 of the environment: move access to “core.sparsecheckout” into repo_settings patch. I tried to minimize calls to prepare_repo_settings() as much as I can in this version. My mentor Christian from GitLab also helped me in identifying a call which I had forgotten to remove. I have yet to receive any reviews on this version.
Other than that, builtin/prune: remove dependency on global variables and ‘the_repository’ got merged to master too!
Thanks again to the Git community for reviewing and helping me and also my mentor for guiding me.
That’s it for this time. Will see you next week!
-Ayush:)
]]>This week, I tried to remove the global variable is_bare_repository_cfg, but some of the tests kept failing on it. I gave it some time and tried fixing it but I couldn’t solve the issue, then I thought I should first focus on the 3 patches I had.
So as I sent a v2 of builtin/prune: remove dependency on global variables and ‘the_repository’ patch in week 4, I got reviews from Junio and Patrick regarding the usage of repo when the repo is NULL and suggesed a test should be added. So Usman recommended that we should have the test in t/t1517-outside-repo.sh. So I decided to keep the test in the file usman mentoined. So I made the changes and sent the v3, where Patrck said the changes were good.
For the commit: avoid scanning trailing comments when ‘core.commentChar’ is “auto” patch, I sent an RFC Post on the mailing list between the two options I had on how to proceed. I did not receive any replied on it this week though.
For the environment: remove sparse-checkout related global variables](https://lore.kernel.org/git/cover.1750157825.git.ayu.chandekar@gmail.com/) patch, I got a reminder from Junio saying that do we want to take this patch series forward or do we wanna postpone it. I had a draft message ready to say that I didn’t add a lot of functions, I actually replaced a few and few of them were removed too. So I told him the following. He also reminded me that I still didn’t respond to Phillip’s commen on that path series. So I replied with the approach that is storing them inside repo settings and also apologized for the delayed response.
Apart from that, college is starting in a week, so I’ve been busy running errands and getting everything ready to head back.
I’m grateful to my mentor, Christian from GitLab, for his excellent guidance, and to the reviewers of my patches in the organization for their patience and valuable feedback.
As I talked about in week 2, I posted a v4 of environment: remove sparse-checkout related global variables. In this week, I got comments from Junio that calls to prepare_repo_settings() are missing in some code paths, so I had to prepare a new patch to make sure that prepare_repo_settings() is called before accessing our settings.sparse_checkout and settings.sparse_checkout_cone settings.
While I was working on comment line variables as I mentioned in the prevoius blog, I stumbled upon a bug where it showed incorrect comment character when core.commentString=auto and we try to rebase. So, I worked on fixing this issue and sent a patch: commit: avoid scanning trailing comments when ‘core.commentChar’ is “auto”.
The patch preload-index: remove dependency on global variables and ‘the_repository’ also got merged to master.
A lot of my time this week also went in doing some of my resume verification work for college.
That was pretty much what I did in week-3.
Week 4 was a packed week because of lots of patches which were there on my table:
prepare_repo_settings() has been called before accessing the settings, I sent a v5 of the patch. I then got reviews from Junio that the calls to prepare_repo_settings() might be too deep in the code unlike the ones in cmd_foo() functions. This might cause performance issues if it’s called every now and then. So, currently, I am working on this.comment_line_str is set to # when core.commentString is set to auto.Overall, I think it’s going a bit slow due to multiple patches under review at the same time, which makes it kind of difficult to context-switch and also try to come up with a new patch. But will do better this time!
Thanks a lot to Christian from GitLab for mentoring me with patches, especially helping me write good commit messages:)
See you soon! -Ayush:)
]]>prepare_repo_settings() (in the previous version I called prepare_repo_settings() everytime in getter and setter function) and just access the member directly afterwards. So, I removed getter/setter functions. And while doing so, I realized the other sparse-checkout-related global variables(core_sparse_checkout_cone & sparse_expect_files_outside_of_patterns) could be cleaned up as well. So I made two new patches and sent to the mailing list. This patch series also removes the definition #define USE_THE_REPOSITORY_VARIABLE from the files builtin/backfill.c and sparse-index.c.core_preload_index and localize it in the function which calls it. Also, it removes the definition #define USE_THE_REPOSITORY_VARIABLE from the file preload-index.c. Junio said that he will merge this to next.I hope to get some reviews on the patch I mentioned above and this patch which I sent in the first week.
Next week, I plan to work on refactoring the comment_line_str global variable, as mentioned in my last blog post. I also hope to identify and clean up more files where the_repository can be removed.
As for my laptop, I think I’ll have to manage with this small ThinkPad for a while now.
-Ayush:)
]]>core_apply_sparse_checkout and shifts the setting to repo_settings under the variable sparse_checkout. I got reviews for this patch to change the name and make some changes in the getter/setter function for which I sent v2 of the patch. I also got a new review to add range-diff and also to mention and thank the reviewers in the cover-letter. So will send a v3 of the patch for that.the_repository from “builtin/prune.c”. This patch didn’t get any review yet.This week, I focused on removing the global variables comment_line_str, comment_line_str_to_free and auto_comment_line_char, aiming to replace them with a functions which uses repo_config_get_string_tmp() from config.c.
There was one specific test case which kept on failing: when core.commentchar is set to auto and we try to amend a commit message that begins with #foo. Previously, this relied on the global comment_line_str. To replace that, I needed a way to pass the computed prefix through various functions. My new function accepts a strbuf containing the commit message, analyzes it, and returns the appropriate prefix. However, in several call sites, it’s not feasible to pass the strbuf directly, which leads to incorrect prefix computation. To work around this, I attempted passing the prefix string through the relevant functions instead.
You can see my attempts on my fork. I think I’ll need to spend more time on this problem and will likely pick up another task alongside it.
Also, my laptop’s motherboard died this week. I’m currently using a temporary laptop, but I may need to purchase a new one soon, we’ll see how it goes.
-Ayush:)
]]>#define USE_THE_REPOSITORY_VARIABLE file by file by removing the_repository and other global variables.
After the meet, I began exploring different files, see previous attempts of refactoring done on it and trying to figure out why it was rejected or see other attempts which were successful and understand their approach, from the mailing list.
One of the things which I learnt is that for some commands, repository passed can be NULL as they can be called from outside the repository and we should keep that in mind while trying to refactor those commands.
I then chose few files and global variables to work with:
add-patch.c : Tackling this file to remove the definition was difficult as it had a global variable called comment_line_str. My mentor, Christian suggested me that I can house this inside a struct config_set which lies in the struct repository. So, my next goal is to try to store the global variable in that struct.blame.c : I was able to remove the definition from the file but it would be better if I try to include in a patch series where builtin/blame.c is also refactored.repository_format_precious_objects : I came across this global variable and I moved it into the struct repository. Along with this, I also tackled two files which included this variable:
builtin/prune.c : I refactored this file thus removing the defintion.builtin/repack.c : I tried to refactor this but it contained functions which were of type each_ref_fn which do not support repository and also the global function is_bare_repository(). So I decided to drop this file.
I plan to submit the patch series (excluding repack.c) to the mailing list this week.core_apply_sparse_checkout : This variable houses the core.sparsecheckout setting. I moved this setting to the struct repo_settings. I will also send a patch for this soon this week.At first, I kept on going down the rabbit hole as I traced all the code paths and hence, got confused. But after talking to my mentors, I gained some clarity and felt pretty confident with time. I thank my mentors Christian Couder (GitLab) and Ghanshyam Thakkar and also Patrick Steinhardt (GitLab) and Jialuo She for solving some of my doubts. I’m excited to continue working on this project and sharing my experience in the weeks to come. See you soon!
-Ayush:)
]]>Before getting selected I’ve read blogs of people who were once a mentee, and it inspired me a lot. So whether you’re a student hoping to apply next year, or just curious about open source, I hope this gives you some insight and inspiration.
Git currently uses a global object called the_repository, which refers to a single instance of struct repository. Many internal functions rely on this global object rather than accepting a struct repository as an explicit parameter. This design inherently assumes a single active repository, making it difficult to support multi-repository use cases and obstructing the long-term goal of libification of Git.
A key architectural limitation is that while struct repository encapsulates some repository-specific information, many important environment variables and configuration settings that logically belong to a repository are still stored as global variables, primarily in environment.c, not within the repository struct. As a result, even if multiple repositories were to exist concurrently, they would still share this global state, leading to incorrect behavior, race conditions, or subtle bugs.
This project aims to refactor Git’s environment handling by relocating global variables into more appropriate local contexts, primarily within struct repository and struct repo_settings. However, some global variables may only apply to specific subsystems. In such cases, rather than placing them in struct repository or struct repo_settings, they should be moved into a context that better reflects their scope.
The time period from 8th May to 1st June is Community Bonding period. This time is to read related documentation, get more familiar with the codebase, start engaging with the community.
These are some things which I would do in this period:
I’m incredibly excited to begin this journey and contribute to a tool which is so widely used and respected. Over the next few months, I’ll be sharing updates, learnings, challenges, and milestones as I work through my project.
-Ayush:)
]]>