-
Notifications
You must be signed in to change notification settings - Fork 413
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
WIP: Add wrappers for git_index_conflict_{add,remove,get,cleanup} #780
Open
fin-ger
wants to merge
9
commits into
rust-lang:master
Choose a base branch
from
fin-ger:add-index-conflict-fns
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
eeff9c2
Add wrappers for git_index_conflict_{add,remove,get,cleanup}
fin-ger da622f7
Run `cargo fmt`
fin-ger aa6d3fb
Add helper function to construct raw::git_index_entry's
fin-ger 3a85808
Format code, ci was failing
fin-ger 68e7cfc
Make try_raw_entries stack allocated
fin-ger 35b8804
Initialize array without dropping uninitialized contents, fixes CI
fin-ger b0ab4ee
Format code, forgot again...
fin-ger b2eb90e
Implement callback-less helper function
fin-ger b548535
Add libgit2-sys to workspace members
fin-ger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
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.
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.
The general intention of this crate is to be a lightweight binding over libgit2 itself, but this is a pretty heavyweight method. Would it be possible to avoid the movement around of all the data internally?
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.
I based this implementation on all the other methods taking a
IndexEntry
(e.g.Index::add()
). This one is particularly long because it takes threeIndexEntry
parameters. I don't know if the handling ofIndexEntry
s can be more concise, my intention was more to handle them exactly the same as in the existing methods.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.
Maybe
std::mem::transmute
can be used here, but I have no clue ifgit_index_entry
is even remotely binary compatible with anIndexEntry
especially regarding thepath: Vec<u8>
. Also, this would be incredibly easy to mess up and hard to debug 🙈Maybe someone has a better idea 😄
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.
Ah ok, well in that case can the conversion into a
git_index_entry
be refactored into a helper function instead of being duplicated?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.
Sure, will do so. Should I also refactor all conversions in other methods using
IndexEntry
?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.
I guess? Sort of depends, I'm not intimately familiar with all this code.
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.
Alright, my solution is to use a helper function which supplies pointers to
raw::git_index_entry
instances via a callback. This is needed as the construction of agit_index_entry
requires the construction of aCString
to which thegit_index_entry
points to. I went for a callback solution as it is (in my opinion) the easiest approach to make sure that theCString
is available while using the rawgit_index_entry
. The procedure is implemented intry_raw_entries()@src/index.rs:92
. The implementation is a bit messy and requires the construction of threeVec
s. Suggestions welcome!