fetch: add fetch.haveRefs config to force "haves" advertisement#2075
Open
derrickstolee wants to merge 3 commits intogitgitgadget:masterfrom
Open
fetch: add fetch.haveRefs config to force "haves" advertisement#2075derrickstolee wants to merge 3 commits intogitgitgadget:masterfrom
derrickstolee wants to merge 3 commits intogitgitgadget:masterfrom
Conversation
In a future change, I will add a test to t5516-fetch-push.sh before this 'fetch follows tags by default' test. However, this test is flaky depending on its numerical order! I'm not exactly sure why the test order matters so much, but 3f763dd (fetch: set remote/HEAD if it does not exist, 2024-11-22) updated this test changing 'sort -k 3' to 'sort -k 4' without explanation. The '-k' parameter is caring abouit which column to sort by. The file that is being sorted only has three columns, the third of which is the full ref name (refs/tags/..., refs/heads/...) which is how we want to sort things. Go back to 'sort -k 3' to prevent this flakiness. Signed-off-by: Derrick Stolee <stolee@gmail.com>
In some large repos and strange commit history data shapes, it can be very expensive to not advertise certain references. For example, a parallel 'release' branch that occasionally merges the 'main' branch and augments the content with release metadata could create a long commit history that is not reachable from any other user development branches. If the client fails to advertise its copy of the 'release' branch, then it could end up redownloading that entire history in the range 'main..origin/release' instead of only 'release..origin/release'. Add a new multi-valued config key 'fetch.haveRefs' that forces certain references to be advertised during negotiation. This change focuses on safely parsing and storing those strings in the repo_settings struct. Some things that I needed to be careful about: the repo_config_get_string_multi() method fills the given list with a pointer to a list that is managed by the config subsystem. Holding a pointer to that list is not safe, so it must be copied before moving on to other config reads. Since we have a deep copy of these values, we need to free them as the settings are cleared. Signed-off-by: Derrick Stolee <stolee@gmail.com>
The previous change documented and loaded the fetch.haveRefs multi-valued config, but did not change any behavior. Now, let's update the behavior for fetch negotiation. This is a config-only option right now, unlike the --negotiation-tips command-line options. For this reason, the values are read on-demand from the repo_settings struct instead of passed down via options. If we wish to reflect this option in the command line as well, then that plumbing would be easy to refactor. One interesting aspect is how this relates to --negotiation-tips arguments. The --negotation-tips arguments _restricts_ the set of haves while the fetch.haveRefs configs _increases_ the set of haves. I chose to have these options done in that order. This helps create tests by enforcing that the negotiation strategy restricts the set of haves before the config adds them back in. This provides some simulation of the intended use case where the haves were restricted due to an overwhelming number of refs instead of by --negotiation-tips. The set of fetch.haveRefs are output entirely before considering a batch for flushing. This should hopefully not be a problem as users _should_ select a small core set of references. While these are output, we add them to an oidset to avoid duplicates. During my development, I found that if I try to increment the counters while outputting these ref tips, then I could get into an infinite loop where the client and server go back and forth forever. This happened when the fetch.haveRefs included the entire set of references that could be selected, which is covered in the tests. Signed-off-by: Derrick Stolee <stolee@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TBD