More Better Worktree Support #603

foriequal0 · 2020-08-05T06:30:30Z

#404 has been draft for a while. Since @mkeeler said 'Feel free to take it over'(comment), I've appended some commits after it.
I've intentionally not cleaned commits to preserve history and intensions. I'll clean up the history as it get reviewed.

Aetf

Only noticed this after me spending two days writing my worktree bindings...

src/worktree.rs

src/call.rs

kamalmarhubi · 2020-10-27T02:30:40Z

@foriequal0 could you add a rough todo list of what needs to happen for this to be done? I might have some time to put in, and as a heavy worktree user I'd love this to make its way into git2-rs based tools. :-)

The systest changes were necessary due to the actual field name used in the C library being “ref” and not being able to name the field the same in the Rust struct because “ref” is a reserved keyword.

The type it aliases, git_indexer_progress, is still checked but this prevents spurious warnings about using deprecated fields.

foriequal0 · 2020-10-27T02:47:34Z

I rebased it to resolve merge conflict.

@kamalmarhubi I forgot this while waiting for a review. I think the feature is done. After finishing the review, I have to squash some commits before merging.
Can someone review this?

e.g. branch uses branches, find_branch, branch to list, get, create branches.

https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

kamalmarhubi · 2020-10-27T21:05:50Z

ping @joshtriplett or @alexcrichton as you've both recently merged prs

alexcrichton

Looking good, thanks for picking this up!

alexcrichton · 2020-10-30T20:02:12Z

src/repo.rs

@@ -162,6 +163,19 @@ impl Repository {
        }
    }

+    /// Attempt to open an already-existing repository from a worktree.
+    pub fn open_from_worktree(worktree: &Worktree) -> Result<Repository, Error> {
+        crate::init();


I think this can be skipped since the presence of Worktree implies it's already been called.

alexcrichton · 2020-10-30T20:04:44Z

systest/build.rs

@@ -41,5 +43,6 @@ fn main() {
    cfg.skip_roundtrip(|t| t == "git_clone_options" || t == "git_submodule_update_options");

    cfg.skip_type(|t| t == "__enum_ty");
+    cfg.skip_type(|t| t == "git_transfer_progress");


How come this is skipped?

I didn't write this commit. I think @mkeeler wanted to turn off deprecation warning according to the commit message, but it doesn't complaint on my computer, and I think it is not reponsibility of this PR. I'll remove it.

I think this may still want to get removed?

systest/build.rs

alexcrichton · 2020-10-30T20:05:33Z

src/test.rs

@@ -17,6 +18,118 @@ macro_rules! t {
    };
 }

+// `repo_test! will
+macro_rules! repo_test {


I may be missing something, but how come a macro for this was added? Could the tests be written just against the right function?

I asume that @mjkeeler wanted to make sure that same code should have same functionalities even if underlying repositories are different (normal repo, normal repo's worktree, bare repo, bare repo's worktree). I've finished his implementation, but I still think it is the libgit2's responsibility since git2-rs is just a binding. Should I remove this?

I think it's good to keep some tests in this repository, but whether or not it's done with a macro is up to you. I personally prefer to avoid macros where possible.

alexcrichton · 2020-10-30T20:05:57Z

src/test.rs

+    (tds, worktree_repo)
+}
+
+pub fn repo_init_typical_worktree() -> (TempDirs, Repository) {


Since these functions seem related to worktree-testing, perhaps they could be scoped to the worktree tests submodule?

alexcrichton · 2020-10-30T20:06:54Z

src/worktree.rs

+    /// filesystem and that the metadata is correct
+    pub fn validate(&self) -> Result<(), Error> {
+        unsafe {
+            call::c_try(raw::git_worktree_validate(call::convert(&self.raw)))?;


Could this use the try_call! macro instead of the call module directly?

(same below)

alexcrichton · 2020-10-30T20:08:11Z

src/worktree.rs

+                    let v = buf.to_vec();
+                    Ok(WorktreeLockStatus::Locked(match v.len() {
+                        0 => None,
+                        _ => String::from_utf8(v).ok(),


Could this use .unwrap() since it's expected to always be utf-8?

alexcrichton · 2020-10-30T20:09:10Z

src/worktree.rs

+
+    /// Opens the repository from the worktree
+    pub fn to_repository(&self) -> Result<Repository, Error> {
+        Repository::open_from_worktree(self)


I personally prefer to stick close to the original git2 API where possible, so could this and the other convenience alias be removed?

alexcrichton · 2020-10-30T20:10:05Z

src/worktree.rs

+        );
+
+        opts.lock = self.lock as c_int;
+        opts.reference = crate::call::convert(&self.reference.as_ref().map(|o| o.raw()));


Instead of the convert function could this do the conversion manually?

Also I think it might be good to store the git_worktree_add_options directly in this structure.

alexcrichton · 2020-10-30T20:10:31Z

src/worktree.rs

+    /// This method is unsafe as the returned value may have pointers to the
+    /// interior of this structure
+    pub unsafe fn raw(&self) -> raw::git_worktree_prune_options {
+        let mut opts = mem::zeroed();


Could this opts be stored directly in the structure?

alexcrichton · 2020-11-04T16:20:12Z

src/branch.rs

@@ -153,8 +153,7 @@ impl<'repo> Drop for Branches<'repo> {
 mod tests {
    use crate::BranchType;

-    #[test]
-    fn smoke() {
+    repo_test!(smoke, (Typical, TypicalWorktree, BareWorktree), |_| {


I think we probably don't need to update old tests like this

I'll revert old tests~~, but keep worktree tests. I'll move repo_test macro to worktree.rs~~ and worktree tests. Let smoke tests be simple smoke tests.

alexcrichton · 2020-11-04T16:21:31Z

src/test.rs

@@ -17,6 +18,118 @@ macro_rules! t {
    };
 }

+// `repo_test! will
+macro_rules! repo_test {


I think it's good to keep some tests in this repository, but whether or not it's done with a macro is up to you. I personally prefer to avoid macros where possible.

src/worktree.rs

systest/build.rs

kamalmarhubi · 2020-11-14T23:13:29Z

Looks like review comments were all addressed, is that right @foriequal0?

alexcrichton

Just a few more small comments!

alexcrichton · 2020-11-16T15:47:34Z

src/worktree.rs

+
+// It is the current belief that a `Worktree` can be sent among threads, or
+// even shared among threads in a mutex
+unsafe impl Send for Worktree {}


Oh sorry I think I missed this earlier, but are you sure of this? Is this documented in libgit2?

This is the best I could find:

Unless otherwise specified, libgit2 objects cannot be safely accessed by multiple threads simultaneously.
[...]
Use an object from a single thread at a time. Most data structures do not guard against concurrent access themselves. This is because they are rarely used in isolation and it makes more sense to synchronize access via a larger lock or similar mechanism.

—https://github.com/libgit2/libgit2/blob/master/docs/threading.md

I think this can be read as they are Send but not Sync?

Looks like Repository has been Send for a while: 51f1376

And the comment this was modeled after was added around the same time: 2b0f5cb#diff-8a44ff48a251d03d58e6d3c73e8664553e2aa7b2a925641dd6c3d2f81c0d2e11

(Both changes by you, almost six years ago!)

The original version of that file was clearer on this point:

You may safely use any libgit2 object from any thread, though there may be issues depending on the cryptographic libraries libgit2 or its dependencies link to (more on this later).

—libgit2/libgit2@15bea02#diff-bb59cfb2bcd0e444b33e25dc3f1e127e095d72379150cc82d408afa63b9903bf

Yeah as @joshtriplett pointed out below libgit2's policy isn't enough to dictate whether things are Send or Sync in Rust. You'll need to devle into the internals and audit what's happening to see if it's safe. I think I did that for git_repository a long time ago, but if this wants to stay then it should happen for git_worktree as well. It's always ok to drop this unsafe impl for now though and add it back later if it works.

You're right. I didn't notice that. I'll remove it.

src/worktree.rs

alexcrichton · 2020-11-16T15:49:02Z

systest/build.rs

@@ -41,5 +43,6 @@ fn main() {
    cfg.skip_roundtrip(|t| t == "git_clone_options" || t == "git_submodule_update_options");

    cfg.skip_type(|t| t == "__enum_ty");
+    cfg.skip_type(|t| t == "git_transfer_progress");


I think this may still want to get removed?

joshtriplett · 2020-11-16T19:03:36Z

On Mon, Nov 16, 2020 at 10:30:06AM -0800, Kamal Marhubi wrote: @kamalmarhubi commented on this pull request. > +use std::path::Path; +use std::ptr; +use std::str; +use std::{marker, mem}; + +/// An owned git worktree +/// +/// This structure corresponds to a `git_worktree` in libgit2. +// +pub struct Worktree { + raw: *mut raw::git_worktree, +} + +// It is the current belief that a `Worktree` can be sent among threads, or +// even shared among threads in a mutex +unsafe impl Send for Worktree {} This is the best I could find: > Unless otherwise specified, libgit2 objects cannot be safely accessed by multiple threads simultaneously. > [...] > Use an object from a single thread at a time. Most data structures do not guard against concurrent access themselves. This is because they are rarely used in isolation and it makes more sense to synchronize access via a larger lock or similar mechanism. —https://github.com/libgit2/libgit2/blob/master/docs/threading.md I think this can be read as they are Send but not Sync?

Not necessarily. `Send` would mean you can share a read-only reference to them across threads. That would in turn mean you can call functions that only need a read-only reference. But in some cases, libgit2's structures contain internal mutability (on the C side), which means even "read-only" methods are not safe to call from multiple threads. Marking any libgit2 structure `Send` means carefully checking for any such internal mutability, and having high confidence that no new internal mutability will be introduced in the future.

foriequal0 · 2020-11-18T01:38:12Z

Thanks for the review! I think you can squash merge if it is okay or I should rebase commits before merging them.

alexcrichton · 2020-11-18T15:23:08Z

OK everything looks great to me, thanks again! And no worries, I use the squash-and-merge button so that happens automatically.

joshtriplett · 2021-01-25T20:55:04Z

@foriequal0 To the best of your knowledge, does this fully supersede #404 , or is there additional work in that PR that needs updating and merging?

foriequal0 · 2021-01-26T04:06:35Z

@joshtriplett I think there is no additional work for this :)

Aetf suggested changes Aug 25, 2020

View reviewed changes

src/worktree.rs Outdated Show resolved Hide resolved

src/worktree.rs Outdated Show resolved Hide resolved

src/worktree.rs Outdated Show resolved Hide resolved

src/worktree.rs Outdated Show resolved Hide resolved

src/call.rs Outdated Show resolved Hide resolved

mkeeler added 3 commits October 27, 2020 11:45

Intial support for working with worktrees

dfd5fc9

The systest changes were necessary due to the actual field name used in the C library being “ref” and not being able to name the field the same in the Rust struct because “ref” is a reserved keyword.

Ignore the deprecated git_transfer_progress type alias in systest

8fca1ee

The type it aliases, git_indexer_progress, is still checked but this prevents spurious warnings about using deprecated fields.

First round of experimental multi-repo testing

714beb6

foriequal0 added 14 commits October 27, 2020 12:16

Fix macro hygene error in repo_test

9e89b47

Match worktree version const types with other version consts

a4be5a9

Match worktree naming conventions with other entities

fbd13a1

e.g. branch uses branches, find_branch, branch to list, get, create branches.

Move conversions to under its namespaces and match naming convention

c55c334

https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

Match WorktreeAddOptions style with other options

8a79cdd

Use ptr::null_mut() rather than 0

daba2aa

Take Option<> where API may take NULL as opts

b8ab3a3

Create TempDirs to hold multiple TempDirs

8db7070

Implement multi-repo testing

6d31b71

Use repo.workdir() to get working directory

8e17a98

Remove unnecessary and dangerous conversion for Cstring into *mut c_char

b51bf5c

Fix comments

7c820bc

Remove println

42cd516

Make Worktree::is_prunable return Result

a81f3ad

alexcrichton reviewed Oct 30, 2020

View reviewed changes

foriequal0 added 6 commits November 1, 2020 02:48

fixup! Remove init

fab731c

fixup! use try_call!

966c34d

fixup! use unwrap since it's expected to always be utf-8

491efd4

fixup! Remove conversion aliases

7ba981c

fixup! Store raw git_worktree_add_options in WorktreeAddOptions

3327e21

fixup! Store raw git_worktree_prune_options in WorktreePruneOptions

79d4459

foriequal0 requested a review from alexcrichton October 31, 2020 20:01

alexcrichton reviewed Nov 4, 2020

View reviewed changes

fixup! Remove repo_test! macros

dd529ae

foriequal0 requested a review from alexcrichton November 5, 2020 03:05

fixup! Make sure Reference outlive WorktreeAddOptions

f73d6a2

alexcrichton reviewed Nov 16, 2020

View reviewed changes

foriequal0 added 2 commits November 17, 2020 11:38

fixup! Remove unnecessary unsafe impl Send

858a446

fixup! Don't skip git_transfer_progress

d0418f7

foriequal0 requested a review from alexcrichton November 17, 2020 02:43

alexcrichton merged commit 2ba1852 into rust-lang:master Nov 18, 2020

joshtriplett mentioned this pull request Jan 25, 2021

Better Worktree Support #404

Closed

foriequal0 deleted the worktrees branch January 26, 2021 04:05

More Better Worktree Support #603

More Better Worktree Support #603

Conversation

foriequal0 commented Aug 5, 2020

Aetf left a comment

Choose a reason for hiding this comment

kamalmarhubi commented Oct 27, 2020

foriequal0 commented Oct 27, 2020 • edited Loading

kamalmarhubi commented Oct 27, 2020

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

foriequal0 Oct 31, 2020 • edited Loading

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

foriequal0 Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalmarhubi commented Nov 14, 2020

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalmarhubi Nov 16, 2020 • edited Loading

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

joshtriplett commented Nov 16, 2020 via email

foriequal0 commented Nov 18, 2020

alexcrichton commented Nov 18, 2020

joshtriplett commented Jan 25, 2021

foriequal0 commented Jan 26, 2021

foriequal0 commented Oct 27, 2020 •

edited

Loading

foriequal0 Oct 31, 2020 •

edited

Loading

foriequal0 Nov 5, 2020 •

edited

Loading

kamalmarhubi Nov 16, 2020 •

edited

Loading