Skip to content

don't fail when computing diff on partial clones #1093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 24, 2023
Merged

don't fail when computing diff on partial clones #1093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
617c8c3
Skip commit when computing diff if blob missing
o2sh Jun 24, 2023
4692211
add integration test
o2sh Jun 24, 2023
06142d2
Assure that `changes()` don't track rewrites which accesses blobs; ma…
Byron Jun 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/info/git/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ fn compute_diff_with_parent(
.into_tree()
.changes()?
.track_path()
.track_rewrites(None)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this line is removed, the exact error is reproduced that this PR is fixing.

.for_each_to_obtain_tree(&commit.tree()?, |change| {
let is_file_change = match change.event {
Event::Addition { entry_mode, .. } | Event::Modification { entry_mode, .. } => {
Expand Down
31 changes: 31 additions & 0 deletions tests/fixtures/make_partial_repo.sh
View file
Open in desktop
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce the bug with this script. It seems like the blobs are still fetched for previous commits even with --filter=blob:none. @Byron @spenserblack Any idea how I could setup a repository to get the following error

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an issue with the configuration that is fixed - by trying the lines by hand in the respective fixtures/generated-do-not-edit folder I observed a warning which led to tracking down the option to enable filtering when git upload-pack is invoked.r

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#!/bin/bash
set -eu -o pipefail

mkdir base
(cd base
git init -q
git checkout -b main
touch code.rs
git add code.rs
git commit -q -m c1
echo hello >> code.rs
git add code.rs
git commit -q -m c2
echo world >> code.rs
git add code.rs
git commit -q -m c3
echo something >> code.rs
git add code.rs
git commit -q -m c4
echo more >> code.rs
git mv code.rs renamed.rs
echo change >> renamed.rs
git commit -q -am c5

git config uploadpack.allowfilter true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes --filter=blob:none work, otherwise we see a warning when cloning, saying that the option is ignored as the server didn't support it.

)

git clone --filter=blob:none file://$PWD/base partial
(cd partial
git config diff.renames true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to be explicit about this option as it is the one that matters. If unset, it would still default to true though, but I thought it's nice to see all options that are important here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea! Yeah, I think it's a good philosophy that tests' setup code describes what's being tested, even if it's unnecessarily explicit.

)
20 changes: 19 additions & 1 deletion tests/repo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,19 @@ use onefetch::cli::{CliOptions, InfoCliOptions, TextForamttingCliOptions};
use onefetch::info::{build_info, get_work_dir};

fn repo(name: &str) -> Result<Repository> {
let name = name.to_string();
let repo_path = gix_testtools::scripted_fixture_read_only(name).unwrap();
let safe_repo = ThreadSafeRepository::open_opts(repo_path, open::Options::isolated())?;
Ok(safe_repo.to_thread_local())
}

pub fn named_repo(fixture: &str, name: &str) -> Result<Repository> {
let repo_path = gix_testtools::scripted_fixture_read_only(fixture)
.unwrap()
.join(name);
let safe_repo = ThreadSafeRepository::open_opts(repo_path, open::Options::isolated())?;
Ok(safe_repo.to_thread_local())
}

#[test]
fn test_bare_repo() -> Result<()> {
let repo = repo("bare_repo.sh")?;
Expand Down Expand Up @@ -65,3 +72,14 @@ fn test_repo_without_remote() -> Result<()> {

Ok(())
}

#[test]
fn test_partial_repo() -> Result<()> {
let repo = named_repo("make_partial_repo.sh", "partial")?;
let config: CliOptions = CliOptions {
input: repo.path().to_path_buf(),
..Default::default()
};
let _info = build_info(&config).expect("no error");
Ok(())
}