From 3ab7e2f70b7b220e2ad46b667892d3f4ce311485 Mon Sep 17 00:00:00 2001 From: Rajiv Sharma Date: Tue, 7 May 2024 03:11:41 -0700 Subject: [PATCH] Fix for git pull with tags in Mononoke GRit server Summary: As seen in the previous diff, when the request from the client includes tag Object Ids in the `WANTS` section, we just consider the commit pointed to by the tag and completely ignore the tag itself. As a result, the output tag stream sent as part of the server response does not include the requested tags. The changes in this diff ensure that every tag that matches either of the following constraint gets sent in the response to the client: - If a tag points to any commit that is part of the response, then that tag gets sent - If a tag is explicitly requested by the client through the `WANTS` section, then that tag gets sent Reviewed By: mitrandir77 Differential Revision: D56980436 fbshipit-source-id: 5d92dff784e212ec71b6b78c9f50e0877731c8c1 --- eden/mononoke/git/protocol/src/generator.rs | 36 ++++++++++++------- .../test-mononoke-git-server-pull-with-tag.t | 7 ++-- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/eden/mononoke/git/protocol/src/generator.rs b/eden/mononoke/git/protocol/src/generator.rs index 188e2e30e7..325f13eed9 100644 --- a/eden/mononoke/git/protocol/src/generator.rs +++ b/eden/mononoke/git/protocol/src/generator.rs @@ -265,14 +265,14 @@ fn to_commit_stream(commits: Vec) -> BoxStream<'static, Result, -) -> Result> { +) -> Result<(Vec, Arc>)> { if git_shas.is_empty() { - return Ok(vec![]); + return Ok((vec![], Arc::new(FxHashSet::default()))); } // Fetch the names of the tags corresponding to the tag object represented by the input object ids let tag_names = repo @@ -285,7 +285,8 @@ async fn tagged_commits( .collect::>(); let tag_names = Arc::new(tag_names); // Fetch the commits pointed to by those tags - repo.bookmarks_cache() + let commits = repo + .bookmarks_cache() .list( ctx, &BookmarkPrefix::new(TAGS_PREFIX)?, @@ -304,16 +305,18 @@ async fn tagged_commits( } }) .collect::>() - }) + })?; + Ok((commits, tag_names)) } /// Fetch the corresponding bonsai commits for the input Git object ids. If the object id doesn't -/// correspond to a bonsai commit, try to resolve it to a tag and then fetch the bonsai commit +/// correspond to a bonsai commit, try to resolve it to a tag and then fetch the bonsai commit and +/// return it along with the tag name async fn git_shas_to_bonsais( ctx: &CoreContext, repo: &impl Repo, oids: impl Iterator>, -) -> Result> { +) -> Result<(Vec, Arc>)> { let shas = oids .map(|oid| GitSha1::from_object_id(oid.as_ref())) .collect::>>() @@ -335,11 +338,11 @@ async fn git_shas_to_bonsais( .into_iter() .filter(|&sha| !entries.iter().any(|entry| entry.git_sha1 == sha)) .collect::>(); - let mut commits_from_tags = tagged_commits(ctx, repo, tag_shas) + let (mut commits_from_tags, tag_names) = tagged_commits(ctx, repo, tag_shas) .await .context("Error while resolving annotated tags to their commits")?; commits_from_tags.extend(entries.into_iter().map(|entry| entry.bcs_id)); - anyhow::Ok(commits_from_tags) + Ok((commits_from_tags, tag_names)) } /// Fetch the Bonsai Git Mappings for the given bonsais @@ -882,6 +885,7 @@ async fn tags_packfile_stream<'a>( repo: &'a impl Repo, requested_commits: Vec, packfile_item_inclusion: PackfileItemInclusion, + requested_tag_names: Arc>, concurrency: usize, ) -> Result<(BoxStream<'a, Result>, usize)> { let requested_commits: Arc> = @@ -909,14 +913,18 @@ async fn tags_packfile_stream<'a>( .collect::>() }) .context("Error in getting tags pointing to input set of commits")?; - // Fetch entries corresponding to annotated tags in the repo + // Fetch entries corresponding to annotated tags in the repo or with names + // that match the requested tag names let tag_entries = repo .bonsai_tag_mapping() .get_all_entries() .await .context("Error in getting tags during fetch")? .into_iter() - .filter(|entry| required_tag_names.contains(&entry.tag_name)) + .filter(|entry| { + required_tag_names.contains(&entry.tag_name) + || requested_tag_names.contains(&entry.tag_name) + }) .collect::>(); let tags_count = tag_entries.len(); let tag_stream = @@ -1079,10 +1087,11 @@ pub async fn fetch_response<'a>( .git_delta_manifest_version; let ctx = Arc::new(ctx); // Convert the base commits and head commits, which are represented as Git hashes, into Bonsai hashes - let bases = git_shas_to_bonsais(&ctx, repo, request.bases.iter()) + // If the input contains tag object Ids, fetch the corresponding tag names + let (bases, _) = git_shas_to_bonsais(&ctx, repo, request.bases.iter()) .await .context("Error converting base Git commits to Bonsai duing fetch")?; - let heads = git_shas_to_bonsais(&ctx, repo, request.heads.iter()) + let (heads, requested_tag_names) = git_shas_to_bonsais(&ctx, repo, request.heads.iter()) .await .context("Error converting head Git commits to Bonsai during fetch")?; // Get the stream of commits between the bases and heads @@ -1139,6 +1148,7 @@ pub async fn fetch_response<'a>( repo, target_commits, packfile_item_inclusion, + requested_tag_names, request.concurrency.tags, ) .await diff --git a/eden/mononoke/tests/integration/test-mononoke-git-server-pull-with-tag.t b/eden/mononoke/tests/integration/test-mononoke-git-server-pull-with-tag.t index e8db76c41a..17e126feaa 100644 --- a/eden/mononoke/tests/integration/test-mononoke-git-server-pull-with-tag.t +++ b/eden/mononoke/tests/integration/test-mononoke-git-server-pull-with-tag.t @@ -80,7 +80,6 @@ # Wait for the warm bookmark cache to catch up with the latest changes $ wait_for_git_bookmark_move HEAD $current_head $ quiet git_client pull - fatal: bad object 113454d6c6f11b84d16c504f75e39fca4c522f00 - error: https://*/repos/git/ro/repo.git did not send all necessary objects (glob) - - [1] +# Verify that we get the same Git repo back that we started with + $ git rev-list --objects --all | git cat-file --batch-check='%(objectname) %(objecttype) %(rest)' | sort > $TESTTMP/new_object_list + $ diff -w $TESTTMP/new_object_list $TESTTMP/object_list