Add binding for `git_message_trailers` #749

LemmingAvalanche · 2021-09-27T19:10:57Z

No description provided.

LemmingAvalanche · 2021-10-10T10:12:57Z

I don’t know how to fix the current error from the CI.

The output on my machine:

$ if [[ "stable" != "nightly" ]]; then cargo run --manifest-path systest/Cargo.toml; fi
…
  running: "cc" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-g" "-fno-omit-frame-pointer" "-m64" "-I" "<path-to-repo>/target/debug/build/libgit2-sys-b48a2ccdd54f1249/out/include" "-Wall" "-Wextra" "-Wall" "-Wextra" "-Werror" "-Wno-unused-parameter" "-Wno-type-limits" "-Wno-address-of-packed-member" "-Wno-unknown-warning-option" "-Wno-deprecated-declarations" "-o" "<path-to-repo>/target/debug/build/systest-ae23871569b4b2ee/out/all.o" "-c" "<path-to-repo>/target/debug/build/systest-ae23871569b4b2ee/out/all.c"
  cargo:warning=<path-to-repo>/target/debug/build/systest-ae23871569b4b2ee/out/all.c: In function ‘__test_field_type_git_message_trailer_array_trailers’:
  cargo:warning=<path-to-repo>/target/debug/build/systest-ae23871569b4b2ee/out/all.c:18438:28: error: return from incompatible pointer type [-Werror=incompatible-pointer-types]
  cargo:warning=                     return &b->trailers;
  cargo:warning=                            ^~~~~~~~~~~~
  cargo:warning=<path-to-repo>/target/debug/build/systest-ae23871569b4b2ee/out/all.c: At top level:
  cargo:warning=cc1: error: unrecognized command line option ‘-Wno-unknown-warning-option’ [-Werror]
  cargo:warning=cc1: error: unrecognized command line option ‘-Wno-address-of-packed-member’ [-Werror]
  cargo:warning=cc1: all warnings being treated as errors
  exit code: 1

  --- stderr
  rust version: 1.47.0


  error occurred: Command "cc" "-O0" "-ffunction-sections" "-fdata-sections" "-fPIC" "-g" "-fno-omit-frame-pointer" "-m64" "-I" "<path-to-repo>/target/debug/build/libgit2-sys-b48a2ccdd54f1249/out/include" "-Wall" "-Wextra" "-Wall" "-Wextra" "-Werror" "-Wno-unused-parameter" "-Wno-type-limits" "-Wno-address-of-packed-member" "-Wno-unknown-warning-option" "-Wno-deprecated-declarations" "-o" "<path-to-repo>/target/debug/build/systest-ae23871569b4b2ee/out/all.o" "-c" "<path-to-repo>/target/debug/build/systest-ae23871569b4b2ee/out/all.c" with args "cc" did not execute successfully (status code exit code: 1).

The C function at line 18438 in that file:

const git_message_trailer** __test_field_type_git_message_trailer_array_trailers(git_message_trailer_array* b) {
    return &b->trailers;
}

The error output from the CI is better than the one on my machine:

error: returning ‘git_message_trailer **’ {aka ‘struct <anonymous> **’} from a function with incompatible return type ‘const git_message_trailer **’ {aka ‘const struct <anonymous> **’} [-Werror=incompatible-pointer-types]

How can I provide a better type in src/libgit2-sys/lib.rs?

alexcrichton · 2021-10-11T14:46:21Z

That looks like a difference of *const and *mut in Rust I think?

Fix error from `cargo run --manifest-path systest/Cargo.toml;`. You can’t just mix and match `*mut` and `*const` like that.

LemmingAvalanche · 2021-10-11T18:28:38Z

Oh wow. Yep, that was it. Thanks!

alexcrichton

Thanks again for this! I left some comments but otherwise this is looking pretty good!

alexcrichton · 2021-10-11T20:30:54Z

src/message.rs

+/// Collection of trailer key–value pairs.
+///
+/// Use `iter()` to get access to the values.
+pub struct MessageTrailers<'pair> {


I believe that this is a uniquely owned value, so I don't think that a 'pair lifetime parameter here is necessary.

See rev. f6fd4b7

alexcrichton · 2021-10-11T20:31:27Z

src/message.rs

+    }
+}
+
+impl<'pair> Binding for MessageTrailers<'pair> {


I think this may not actually be necessary?

See rev. b0015cc

alexcrichton · 2021-10-11T20:32:12Z

src/message.rs

+/// A borrowed iterator.
+pub struct MessageTrailersIterator<'pair> {
+    trailers: &'pair MessageTrailers<'pair>,
+    counter: usize,


One thing I might recommend is to change this to Range<usize> so that way next() is easy to implement.

Never heard of, but I like it!

See rev. e94008a

alexcrichton · 2021-10-11T20:32:31Z

src/message.rs

+    }
+}
+
+struct Trailer<'pair> {


This is a one-off struct just for the method below so I think it's fine to remove this and just inline the method into the iterator implementation.

See rev. 55ea909

alexcrichton · 2021-10-11T20:33:09Z

src/message.rs

+            }
+        }
+    }
+}


Could you add implementations for size_hint and ExactSizeIterator here as well? (which would also necessitate DoubleEndedIterator, and the implementations should be relatively easy if the counter field is changed to a Range)

I implemented len() for ExactSizeIterator. The doc says that size_hint() can be derived from it. I couldn’t implement size_hint() myself.

The doc says that len() has a default implementation, but I guess just returning the value directly wouldn’t be less efficient than whatever the default impl. is.

See rev. e7b0306

Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

I love it. Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

Also change `to_str_tuple(…)` in order to share more code between two of the iterators. Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

LemmingAvalanche · 2021-10-14T20:53:37Z

What are your preferences with regards to rebase?

alexcrichton

This'll get rebased when merged so no need to worry about manual rebasing!

alexcrichton · 2021-10-15T15:01:19Z

src/message.rs

+    _marker: marker::PhantomData<c_char>,
+}
+
+impl<'pair> MessageTrailers {


Since the type doesn't have a lifetime parameter any more this shoudl move down to the iter method itself, but even on the iter method it can be elided and it can return MessageTrailersIterator<'_>

See rev e450945

alexcrichton · 2021-10-15T15:01:29Z

src/message.rs

+/// Use `iter()` to get access to the values.
+pub struct MessageTrailers {
+    raw: raw::git_message_trailer_array,
+    _marker: marker::PhantomData<c_char>,


This I don't believe is necessary any longer.

See rev a0bd026

alexcrichton · 2021-10-15T15:02:07Z

src/message.rs

+}
+
+impl<'pair> Iterator for MessageTrailersIterator<'pair> {
+    type Item = (&'pair str, &'pair str);


Could this perhaps be an iterator of &[u8] instead of &str to allow for non-utf8 messages?

I’ve gone back and forth on the UTF-8 issue. I think I ended up trying to do what message_prettify(…) does, and that function seems to assume that it gets a UTF-8 string back. It seems though that it can send in a non-UTF-8 string since it passes in a IntoCString (as does message_trailers(…)), in which case it would error out when it gets non-UTF-8 strings back (if I’ve read the code correctly).

Supporting &[u8] seems good, but what about both types? It seems inconvenient to send in a UTF-8 string to C, get back &[u8] values that you know are UTF-8 and then having to convert or assert that they are also valid as &str.

Eh I could also go either way on this. I don't think the bindings are super principled so far, some of the time it tries to give you bytes and other times it gives you strings. I don't really mind either way on this, we can always add a separate iterator in the future specifically for bytes if strings becomes a problem.

See rev 70b7ebd

alexcrichton · 2021-10-15T15:02:29Z

src/message.rs

+
+impl ExactSizeIterator for MessageTrailersIterator<'_> {
+    fn len(&self) -> usize {
+        self.range.end


This'll want to be self.range.len() since the length changes over time.

See rev 34fa7b5

alexcrichton · 2021-10-15T15:02:38Z

src/message.rs

+        self.range
+            .next()
+            .map(|index| to_str_tuple(&self.trailers, index, marker::PhantomData))
+    }


Could you add size_hint here too?

Ah okay I think I understand now. I confused myself thought that size_hint() was part of ExactSizeIterator.

See rev 16321f3

alexcrichton · 2021-10-15T15:02:55Z

src/message.rs

+    }
+}
+
+#[inline(always)]


This annotation isn't necessary

See rev 023fb92

See: #749 (comment)

Better than the default implementation. See: #749 (comment)

Support both raw bytes messages as well as normal (UTF-8) messages by making two iterators. See: #749 (comment)

(cherry picked from commit cd4c2db)

LemmingAvalanche · 2021-10-16T10:49:25Z

Something unrelated: cargo fmt wants to do this change:

diff --git a/src/util.rs b/src/util.rs
index 1c6001ddbf60..a31e63bce2c3 100644
--- a/src/util.rs
+++ b/src/util.rs
@@ -204,7 +204,7 @@ pub fn c_cmp_to_ordering(cmp: c_int) -> Ordering {
 pub fn path_to_repo_path(path: &Path) -> Result<CString, Error> {
     macro_rules! err {
         ($msg:literal, $path:expr) => {
-            return Err(Error::from_str(&format!($msg, $path.display())))
+            return Err(Error::from_str(&format!($msg, $path.display())));
         };
     }
     match path.components().next() {

But that causes one of the CI checks to fail (corrected in f35b68f and cd4c2db).

This is also what cargo fmt -- --check in the CI proposes. At least on my machine.

Are you supposed to give an option/argument to cargo fmt?

alexcrichton · 2021-10-18T15:20:27Z

This all looks great to me, thanks! No worries about the extra change, we just bow to whatever the rustfmt gods say we should do.

LemmingAvalanche and others added 2 commits October 3, 2021 13:31

Add binding for git_message_trailers

15a7d4e

cargo fmt

5575c3d

Fix pointer mismatch error

33ebdcd

Fix error from `cargo run --manifest-path systest/Cargo.toml;`. You can’t just mix and match `*mut` and `*const` like that.

alexcrichton reviewed Oct 11, 2021

View reviewed changes

LemmingAvalanche added 7 commits October 12, 2021 21:14

Remove a lot of unnecessary lifetimes

f6fd4b7

Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

Remove another unnecessary lifetime

b0015cc

Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

Use Range<usize> instead of usize

e94008a

I love it. Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

cargo fmt

e830f7d

Inline one-off struct

55ea909

Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

Implement more iterators

e7b0306

Also change `to_str_tuple(…)` in order to share more code between two of the iterators. Suggested-by: Alex Crichton <alex@alexcrichton.com> See: #749 (comment)

Undo accidental and unrelated edit

cd4c2db

alexcrichton reviewed Oct 15, 2021

View reviewed changes

LemmingAvalanche added 10 commits October 15, 2021 20:51

Less explicit lifetimes

e450945

See: #749 (comment)

Don’t need std::marker any more

a0bd026

See: #749 (comment)

Correct len(…)

34fa7b5

See: #749 (comment)

Remove unnecessary annotation

023fb92

See: #749 (comment)

Implement size_hint()

16321f3

Better than the default implementation. See: #749 (comment)

Split into “bytes” and “string” iterators

70b7ebd

Support both raw bytes messages as well as normal (UTF-8) messages by making two iterators. See: #749 (comment)

Remove more lifetimes

ed11c76

Docs

958e378

cargo fmt

2508506

Undo accidental and unrelated edit

f35b68f

(cherry picked from commit cd4c2db)

alexcrichton merged commit 4461b17 into rust-lang:master Oct 18, 2021

LemmingAvalanche deleted the message-trailers branch October 18, 2021 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add binding for `git_message_trailers` #749

Add binding for `git_message_trailers` #749

LemmingAvalanche commented Sep 27, 2021

LemmingAvalanche commented Oct 10, 2021

alexcrichton commented Oct 11, 2021

LemmingAvalanche commented Oct 11, 2021

alexcrichton left a comment

alexcrichton Oct 11, 2021

LemmingAvalanche Oct 14, 2021

alexcrichton Oct 11, 2021

LemmingAvalanche Oct 14, 2021

alexcrichton Oct 11, 2021

LemmingAvalanche Oct 14, 2021

alexcrichton Oct 11, 2021

LemmingAvalanche Oct 14, 2021

alexcrichton Oct 11, 2021

LemmingAvalanche Oct 14, 2021

LemmingAvalanche commented Oct 14, 2021

alexcrichton left a comment

alexcrichton Oct 15, 2021

LemmingAvalanche Oct 16, 2021

alexcrichton Oct 15, 2021

LemmingAvalanche Oct 16, 2021

alexcrichton Oct 15, 2021

LemmingAvalanche Oct 15, 2021

alexcrichton Oct 15, 2021

LemmingAvalanche Oct 16, 2021

alexcrichton Oct 15, 2021

LemmingAvalanche Oct 16, 2021

alexcrichton Oct 15, 2021

LemmingAvalanche Oct 16, 2021

alexcrichton Oct 15, 2021

LemmingAvalanche Oct 16, 2021

LemmingAvalanche commented Oct 16, 2021

alexcrichton commented Oct 18, 2021

Navigation Menu

Add binding for git_message_trailers #749

Add binding for git_message_trailers #749

Conversation

LemmingAvalanche commented Sep 27, 2021

LemmingAvalanche commented Oct 10, 2021

alexcrichton commented Oct 11, 2021

LemmingAvalanche commented Oct 11, 2021

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LemmingAvalanche commented Oct 14, 2021

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LemmingAvalanche commented Oct 16, 2021

alexcrichton commented Oct 18, 2021

Add binding for `git_message_trailers` #749

Add binding for `git_message_trailers` #749