- 
                Notifications
    You must be signed in to change notification settings 
- Fork 718
fix: assure we convert worktree data to Git before writing it (#5558) #5564
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.
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
Conversation
| The latest updates on your projects. Learn more about Vercel for Git ↗︎ 
 | 
| @Byron is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. | 
277ec1f    to
    4645ad2      
    Compare
  
    | let mut file_for_git = | ||
| pipeline.convert_to_git(std::fs::File::open(&file_path)?, path, &index)?; | ||
| buf.clear(); | ||
| std::io::copy(&mut file_for_git, &mut buf)?; | ||
| let blob_id = self.blob(&buf)?; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to copy the data into the buffer and not just directly pass it to self.blob?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file_for_git isn't a buffer, it's something that implements std::io::Read, while blob() takes &[u8].
This is the version of the code that is easiest to write, but also slowest as two out of three cases already have a buffer underneath which could be passed directly.
I will adjust it to be the most efficient possible.
That way, if a buffer is already present, we just write that directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good to me 👍
Fixes #5558
Tasks
Notes for the reviewer
git2::Repository::blob_writer(path)which automatically applies the 'worktree-to-git' filtering as well. That would lead to simpler code, at least, but will have disadvantages in correctness as it doesn't support external filters, nor will it pick up all the configuration.gitoxidemakes these the default for good measure - right now it prefers performance as that option involves launchinggitto have it tell us where its installation configuration is located. In future, I can see that shift as more features will only work correctly in with this option enabled.