Add bindings for git email create #847
Conversation
f0rget-the-sad
force-pushed
the
email
branch
2 times, most recently
from
9dd96c5 to
1a64fbd
Compare
src/diff.rs
Outdated
| /// | ||
| /// Matches the format created by `git format-patch` | ||
| #[doc(hidden)] | ||
| #[deprecated(note = "refactored to `email_from_diff` to match upstream")] |
There was a problem hiding this comment.
I think this naming should be Email::from_diff perhaps?
libgit2-sys/lib.rs
Outdated
| ancestor: *const git_oid, | ||
| ) -> c_int; | ||
|
|
||
| #[deprecated(note = "refactored to `email_from_diff` to match upstream")] |
There was a problem hiding this comment.
I think it's ok to skip the deprecation here since that's more for the API layer rather than the sys layer
src/email.rs
Outdated
| diff_opts: ptr::read(diff_opts.raw()), | ||
| diff_find_opts: ptr::read(DiffFindOptions::new().raw()), |
There was a problem hiding this comment.
Instead of using unsafe code like ptr::read here I think this builder should probably embedd the builders for the other options? That way all the options can continue to be configurable
src/email.rs
Outdated
| patch_count, | ||
| Binding::raw(commit_id), | ||
| summary.into_c_string()?.as_ptr(), | ||
| body.into_c_string()?.as_ptr(), |
There was a problem hiding this comment.
While I think this may technically work I think it would be better to extract this to a local variable above to explicitly prevent the owned CString from being dropped too soon.
f0rget-the-sad
force-pushed
the
email
branch
2 times, most recently
from
6e9ac89 to
db76e5a
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks! Two more small things but otherwise seems good to go to me.
src/email.rs
Outdated
| /// A structure to represent patch in mbox format for sending via email | ||
| pub struct Email { | ||
| /// Buffer to store the e-mail patch in | ||
| pub buf: Buf, |
There was a problem hiding this comment.
Instead of making this a pub field could public accessors be provided for this? (sorry missed this earlier)
Ideally byte slices would be returned as well instead of &Buf too
src/email.rs
Outdated
| /// Options to use when creating diffs | ||
| pub diff_options: DiffOptions, | ||
| /// Options for finding similarities within diffs | ||
| pub diff_find_options: DiffFindOptions, |
There was a problem hiding this comment.
Instead of making these pub could method accessors be provided?
Add bindings for `git_email_create_from_diff()` and `git_email_create_from_commit()`. Deprecate `git_diff_format_email()` to reflect upstream changes.
2 participants
Add bindings for
git_email_create_from_diff()andgit_email_create_from_commit(). Deprecategit_diff_format_email()toreflect upstream changes.
git_diff_format_email was refactored in upsrtream.
It's also possible to impl git_email_create_from_diff/commit directly for Diff/Commit structs, and do not introduce
Emailstruct, but I think current approach is better for future extension(e.g. there is plan to intorducegit_email_create_from_commitsthat produces a series of emails).