~radicle-link/dev

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
12 3

[PATCH radicle-link] lnk-identities: optionally use initial branch on git clone

Details
Message ID
<20220825173051.3734-1-slackcoder@server.ky>
DKIM signature
missing
Download raw message
Patch: +30 -9
Better follow git's command line behavior by setting one local branch, the
projects default head, on clone.  It is assumed to be the project
identities 'default_branch' field.

Implement the new clone argument as optional to keep backward
compatibility.
---
 cli/lnk-identities/src/git.rs                 |  7 +++++
 cli/lnk-identities/src/git/checkout.rs        | 30 ++++++++++++++-----
 .../t/src/tests/git/checkout.rs               |  2 +-
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/cli/lnk-identities/src/git.rs b/cli/lnk-identities/src/git.rs
index c9ed5e90..f2dac322 100644
--- a/cli/lnk-identities/src/git.rs
+++ b/cli/lnk-identities/src/git.rs
@@ -188,6 +188,7 @@ pub fn clone<F>(
    path: &Path,
    storage: F,
    mut remote: Remote<LocalUrl>,
    init_branch: Option<&str>,
) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
where
    F: CanOpenStorage + 'static,
@@ -201,6 +202,12 @@ where
        let branch: git_ext::RefLike = OneLevel::from(reference).into();
        let branch = branch.strip_prefix(remote.name.clone())?;
        let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
        if let Some(init_branch) = init_branch {
            if branch.as_str() != init_branch {
                continue;
            }
        }

        let _remote_branch = repo.reference(
            reflike!("refs/remotes")
                .join(remote.name.clone())
diff --git a/cli/lnk-identities/src/git/checkout.rs b/cli/lnk-identities/src/git/checkout.rs
index b85163dd..bcbbfa2c 100644
--- a/cli/lnk-identities/src/git/checkout.rs
+++ b/cli/lnk-identities/src/git/checkout.rs
@@ -139,17 +139,21 @@ where
pub struct Local {
    url: LocalUrl,
    path: PathBuf,
    default_branch: OneLevel,
}

impl Local {
    pub fn new<I>(identity: &I, path: PathBuf) -> Self
    pub fn new<I>(identity: &I, path: PathBuf) -> Result<Self, Error>
    where
        I: HasName + HasUrn,
        I: HasBranch + HasName + HasUrn,
    {
        Self {
            url: LocalUrl::from(identity.urn()),
        let urn = identity.urn();
        let default_branch = identity.branch_or_die(urn.clone())?;
        Ok(Self {
            url: LocalUrl::from(urn),
            path,
        }
            default_branch,
        })
    }

    fn checkout<F>(self, open_storage: F) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
@@ -164,7 +168,12 @@ impl Local {
                force: Force::True,
            },
        );
        Ok(git::clone(&self.path, open_storage, rad)?)
        Ok(git::clone(
            &self.path,
            open_storage,
            rad,
            Some(&self.default_branch),
        )?)
    }
}

@@ -209,7 +218,12 @@ impl Peer {
            force: Force::True,
        }]);

        let (repo, _) = git::clone(&self.path, open_storage.clone(), remote)?;
        let (repo, _) = git::clone(
            &self.path,
            open_storage.clone(),
            remote,
            Some(&self.default_branch),
        )?;

        // Create a rad remote and push the default branch so we can set it as the
        // upstream.
@@ -248,7 +262,7 @@ where
{
    let path = path.resolve(identity.name());
    Ok(match remote {
        None => Either::Left(Local::new(identity, path)),
        None => Either::Left(Local::new(identity, path)?),
        Some(remote) => Either::Right(Peer::new(identity, remote, path)?),
    })
}
diff --git a/cli/lnk-identities/t/src/tests/git/checkout.rs b/cli/lnk-identities/t/src/tests/git/checkout.rs
index 5177eb38..1ca7d8fb 100644
--- a/cli/lnk-identities/t/src/tests/git/checkout.rs
+++ b/cli/lnk-identities/t/src/tests/git/checkout.rs
@@ -47,7 +47,7 @@ fn local_checkout() -> anyhow::Result<()> {
        signer: signer.into(),
    };

    let local = Local::new(&proj.project, temp.path().to_path_buf());
    let local = Local::new(&proj.project, temp.path().to_path_buf())?;
    let repo = checkout(
        &paths,
        settings,
-- 
2.35.3

[radicle-link/patches/nixos-latest.yml] build failed

builds.sr.ht <builds@sr.ht>
Details
Message ID
<CMFAVYUL29G3.3JE0440EAF0LF@cirno>
In-Reply-To
<20220825173051.3734-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
radicle-link/patches/nixos-latest.yml: FAILED in 22m33s

[lnk-identities: optionally use initial branch on git clone][0] from [Slack Coder][1]

[0]: https://lists.sr.ht/~radicle-link/dev/patches/34923
[1]: slackcoder@server.ky

✗ #831519 FAILED radicle-link/patches/nixos-latest.yml https://builds.sr.ht/~radicle-link/job/831519
Details
Message ID
<CMFWPPOWC3Y6.1LP5TMQTYVAVC@haptop>
In-Reply-To
<20220825173051.3734-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
On Thu Aug 25, 2022 at 6:30 PM IST, Slack Coder wrote:
> Better follow git's command line behavior by setting one local branch, the
> projects default head, on clone.  It is assumed to be the project
> identities 'default_branch' field.
>
> Implement the new clone argument as optional to keep backward
> compatibility.
> ---
>  cli/lnk-identities/src/git.rs                 |  7 +++++
>  cli/lnk-identities/src/git/checkout.rs        | 30 ++++++++++++++-----
>  .../t/src/tests/git/checkout.rs               |  2 +-
>  3 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/cli/lnk-identities/src/git.rs b/cli/lnk-identities/src/git.rs
> index c9ed5e90..f2dac322 100644
> --- a/cli/lnk-identities/src/git.rs
> +++ b/cli/lnk-identities/src/git.rs
> @@ -188,6 +188,7 @@ pub fn clone<F>(
>      path: &Path,
>      storage: F,
>      mut remote: Remote<LocalUrl>,
> +    init_branch: Option<&str>,
>  ) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
>  where
>      F: CanOpenStorage + 'static,
> @@ -201,6 +202,12 @@ where
>          let branch: git_ext::RefLike = OneLevel::from(reference).into();
>          let branch = branch.strip_prefix(remote.name.clone())?;
>          let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
> +        if let Some(init_branch) = init_branch {
> +            if branch.as_str() != init_branch {
> +                continue;
> +            }
> +        }
> +

I'm wondering if instead of matching all the time, we do one fetch and
one match, following some pseudo code like:

    match init_branch {
       // same as old logic
       None => loop refs -> create ref
       // we don't use a for loop and instead just search through for
       // the one we're looking for
       Some(init) => refs.find(init) -> create ref
    }

>          let _remote_branch = repo.reference(
>              reflike!("refs/remotes")
>                  .join(remote.name.clone())
> diff --git a/cli/lnk-identities/src/git/checkout.rs b/cli/lnk-identities/src/git/checkout.rs
> index b85163dd..bcbbfa2c 100644
> --- a/cli/lnk-identities/src/git/checkout.rs
> +++ b/cli/lnk-identities/src/git/checkout.rs
> @@ -139,17 +139,21 @@ where
>  pub struct Local {
>      url: LocalUrl,
>      path: PathBuf,
> +    default_branch: OneLevel,

I'm wondering, since the `init_branch` is optional could this made
`Option<OneLevel>`? That we don't need to use `branch_or_die` and the
code stays compatible with a `Project` that doesn't define a
`default_branch` and also a `Person`.

>  }
>  
>  impl Local {
> -    pub fn new<I>(identity: &I, path: PathBuf) -> Self
> +    pub fn new<I>(identity: &I, path: PathBuf) -> Result<Self, Error>
>      where
> -        I: HasName + HasUrn,
> +        I: HasBranch + HasName + HasUrn,
>      {
> -        Self {
> -            url: LocalUrl::from(identity.urn()),
> +        let urn = identity.urn();
> +        let default_branch = identity.branch_or_die(urn.clone())?;
> +        Ok(Self {
> +            url: LocalUrl::from(urn),
>              path,
> -        }
> +            default_branch,
> +        })
>      }
>  
>      fn checkout<F>(self, open_storage: F) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
> @@ -164,7 +168,12 @@ impl Local {
>                  force: Force::True,
>              },
>          );
> -        Ok(git::clone(&self.path, open_storage, rad)?)
> +        Ok(git::clone(
> +            &self.path,
> +            open_storage,
> +            rad,
> +            Some(&self.default_branch),
> +        )?)
>      }
>  }
>  
> @@ -209,7 +218,12 @@ impl Peer {
>              force: Force::True,
>          }]);
>  
> -        let (repo, _) = git::clone(&self.path, open_storage.clone(), remote)?;
> +        let (repo, _) = git::clone(
> +            &self.path,
> +            open_storage.clone(),
> +            remote,
> +            Some(&self.default_branch),
> +        )?;
>  
>          // Create a rad remote and push the default branch so we can set it as the
>          // upstream.
> @@ -248,7 +262,7 @@ where
>  {
>      let path = path.resolve(identity.name());
>      Ok(match remote {
> -        None => Either::Left(Local::new(identity, path)),
> +        None => Either::Left(Local::new(identity, path)?),
>          Some(remote) => Either::Right(Peer::new(identity, remote, path)?),
>      })
>  }
> diff --git a/cli/lnk-identities/t/src/tests/git/checkout.rs b/cli/lnk-identities/t/src/tests/git/checkout.rs
> index 5177eb38..1ca7d8fb 100644
> --- a/cli/lnk-identities/t/src/tests/git/checkout.rs
> +++ b/cli/lnk-identities/t/src/tests/git/checkout.rs
> @@ -47,7 +47,7 @@ fn local_checkout() -> anyhow::Result<()> {
>          signer: signer.into(),
>      };
>  
> -    let local = Local::new(&proj.project, temp.path().to_path_buf());
> +    let local = Local::new(&proj.project, temp.path().to_path_buf())?;
>      let repo = checkout(
>          &paths,
>          settings,
> -- 
> 2.35.3
Details
Message ID
<5a4ac07f-2c69-5b0e-cace-82a4bedf6639@server.ky>
In-Reply-To
<CMFWPPOWC3Y6.1LP5TMQTYVAVC@haptop> (view parent)
DKIM signature
missing
Download raw message
On 8/26/22 06:00, Fintan Halpenny wrote:
>> Better follow git's command line behavior by setting one local branch, the
>> projects default head, on clone.  It is assumed to be the project
>> identities 'default_branch' field.
>>
>> Implement the new clone argument as optional to keep backward
>> compatibility.
>> ---
>>   cli/lnk-identities/src/git.rs                 |  7 +++++
>>   cli/lnk-identities/src/git/checkout.rs        | 30 ++++++++++++++-----
>>   .../t/src/tests/git/checkout.rs               |  2 +-
>>   3 files changed, 30 insertions(+), 9 deletions(-)
>>
>> diff --git a/cli/lnk-identities/src/git.rs b/cli/lnk-identities/src/git.rs
>> index c9ed5e90..f2dac322 100644
>> --- a/cli/lnk-identities/src/git.rs
>> +++ b/cli/lnk-identities/src/git.rs
>> @@ -188,6 +188,7 @@ pub fn clone<F>(
>>       path: &Path,
>>       storage: F,
>>       mut remote: Remote<LocalUrl>,
>> +    init_branch: Option<&str>,
>>   ) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
>>   where
>>       F: CanOpenStorage + 'static,
>> @@ -201,6 +202,12 @@ where
>>           let branch: git_ext::RefLike = OneLevel::from(reference).into();
>>           let branch = branch.strip_prefix(remote.name.clone())?;
>>           let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
>> +        if let Some(init_branch) = init_branch {
>> +            if branch.as_str() != init_branch {
>> +                continue;
>> +            }
>> +        }
>> +
> I'm wondering if instead of matching all the time, we do one fetch and
> one match, following some pseudo code like:
> 
>      match init_branch {
>         // same as old logic
>         None => loop refs -> create ref
>         // we don't use a for loop and instead just search through for
>         // the one we're looking for
>         Some(init) => refs.find(init) -> create ref
>      }
> 
It would involve moving each the branch ref parsing and the reference 
creation to their own function to keep the code dry.  I'm happy either 
way, good to go ahead with this?

[PATCH radicle-link v2 0/1] Reduce local branches from git::clone()

Details
Message ID
<20220901164653.12695-1-slackcoder@server.ky>
In-Reply-To
<20220825173051.3734-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
Version two for reducing local branch creation for git clone.

Slack Coder (1):
  lnk-identities: optionally use initial branch on git clone

 cli/lnk-identities/src/git.rs                 | 74 +++++++++++++------
 cli/lnk-identities/src/git/checkout.rs        | 30 ++++++--
 .../t/src/tests/git/checkout.rs               |  2 +-
 3 files changed, 75 insertions(+), 31 deletions(-)

--
2.35.3

[PATCH radicle-link v2 1/1] lnk-identities: optionally use initial branch on git clone

Details
Message ID
<20220901164653.12695-2-slackcoder@server.ky>
In-Reply-To
<20220825173051.3734-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
Patch: +75 -31
Better follow git's command line behavior by setting one local branch, the
projects default head, on clone.  It is assumed to be the project
identities 'default_branch' field.

Implement the new clone argument as optional to keep backward
compatibility.
---
 cli/lnk-identities/src/git.rs                 | 74 +++++++++++++------
 cli/lnk-identities/src/git/checkout.rs        | 30 ++++++--
 .../t/src/tests/git/checkout.rs               |  2 +-
 3 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/cli/lnk-identities/src/git.rs b/cli/lnk-identities/src/git.rs
index c9ed5e90..48e167b8 100644
--- a/cli/lnk-identities/src/git.rs
+++ b/cli/lnk-identities/src/git.rs
@@ -16,14 +16,11 @@ use librad::{
        },
        types::{
            remote::{LocalFetchspec, LocalPushspec, Remote},
            Fetchspec,
            Force,
            Refspec,
            Fetchspec, Force, Refspec,
        },
    },
    git_ext::{self, OneLevel, Qualified},
    reflike,
    refspec_pattern,
    reflike, refspec_pattern,
    std_ext::result::ResultExt as _,
};

@@ -49,6 +46,9 @@ pub enum Error {

    #[error(transparent)]
    Git(#[from] git2::Error),

    #[error("branch does not exist")]
    BranchNotExist,
}

/// Equips a repository with a rad remote for the given id. If the directory at
@@ -178,6 +178,26 @@ pub fn set_upstream<Url>(
    Ok(())
}

fn create_local_ref(
    repo: &git2::Repository,
    branch: git_ext::RefLike,
    oid: git2::Oid,
    remote: &Remote<LocalUrl>,
    msg: &str,
) -> Result<(), Error> {
    let _remote_branch = repo.reference(
        reflike!("refs/remotes")
            .join(remote.name.clone())
            .join(branch.clone())
            .as_str(),
        oid,
        true,
        msg,
    )?;
    let _local_branch = repo.reference(Qualified::from(branch).as_str(), oid, true, msg);
    Ok(())
}

/// Clone a git repository to the `path` location, based off of the `remote`
/// provided.
///
@@ -188,29 +208,39 @@ pub fn clone<F>(
    path: &Path,
    storage: F,
    mut remote: Remote<LocalUrl>,
    init_branch: Option<&str>,
) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
where
    F: CanOpenStorage + 'static,
{
    let repo = git2::Repository::init(path)?;
    remote.save(&repo)?;
    for (reference, oid) in remote.fetch(storage, &repo, LocalFetchspec::Configured)? {
        let msg = format!("Fetched `{}->{}`", reference, oid);
        tracing::debug!("{}", msg);

        let branch: git_ext::RefLike = OneLevel::from(reference).into();
        let branch = branch.strip_prefix(remote.name.clone())?;
        let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
        let _remote_branch = repo.reference(
            reflike!("refs/remotes")
                .join(remote.name.clone())
                .join(branch.clone())
                .as_str(),
            oid,
            true,
            &msg,
        )?;
        let _local_branch = repo.reference(Qualified::from(branch).as_str(), oid, true, &msg);

    let _branches: Vec<_> = remote
        .fetch(storage, &repo, LocalFetchspec::Configured)?
        .map(|(_ref, oid)| {
            let msg = format!("Fetched `{}->{}`", _ref, oid);
            tracing::debug!("{}", msg);

            let branch: git_ext::RefLike = OneLevel::from(_ref).into();
            let branch = branch.strip_prefix(remote.name.clone()).unwrap();
            let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
            (branch, oid, msg)
        })
        .collect();
    match init_branch {
        Some(init_branch) => {
            let (branch, oid, msg) = _branches
                .iter()
                .find(|(branch, _, _)| branch.as_str() == init_branch)
                .ok_or(Error::BranchNotExist)?;
            create_local_ref(&repo, branch.clone(), *oid, &remote, msg)?;
        },
        None => {
            for (branch, oid, msg) in _branches {
                create_local_ref(&repo, branch, oid, &remote, &msg)?
            }
        },
    }

    Ok((repo, remote))
diff --git a/cli/lnk-identities/src/git/checkout.rs b/cli/lnk-identities/src/git/checkout.rs
index b85163dd..bcbbfa2c 100644
--- a/cli/lnk-identities/src/git/checkout.rs
+++ b/cli/lnk-identities/src/git/checkout.rs
@@ -139,17 +139,21 @@ where
pub struct Local {
    url: LocalUrl,
    path: PathBuf,
    default_branch: OneLevel,
}

impl Local {
    pub fn new<I>(identity: &I, path: PathBuf) -> Self
    pub fn new<I>(identity: &I, path: PathBuf) -> Result<Self, Error>
    where
        I: HasName + HasUrn,
        I: HasBranch + HasName + HasUrn,
    {
        Self {
            url: LocalUrl::from(identity.urn()),
        let urn = identity.urn();
        let default_branch = identity.branch_or_die(urn.clone())?;
        Ok(Self {
            url: LocalUrl::from(urn),
            path,
        }
            default_branch,
        })
    }

    fn checkout<F>(self, open_storage: F) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
@@ -164,7 +168,12 @@ impl Local {
                force: Force::True,
            },
        );
        Ok(git::clone(&self.path, open_storage, rad)?)
        Ok(git::clone(
            &self.path,
            open_storage,
            rad,
            Some(&self.default_branch),
        )?)
    }
}

@@ -209,7 +218,12 @@ impl Peer {
            force: Force::True,
        }]);

        let (repo, _) = git::clone(&self.path, open_storage.clone(), remote)?;
        let (repo, _) = git::clone(
            &self.path,
            open_storage.clone(),
            remote,
            Some(&self.default_branch),
        )?;

        // Create a rad remote and push the default branch so we can set it as the
        // upstream.
@@ -248,7 +262,7 @@ where
{
    let path = path.resolve(identity.name());
    Ok(match remote {
        None => Either::Left(Local::new(identity, path)),
        None => Either::Left(Local::new(identity, path)?),
        Some(remote) => Either::Right(Peer::new(identity, remote, path)?),
    })
}
diff --git a/cli/lnk-identities/t/src/tests/git/checkout.rs b/cli/lnk-identities/t/src/tests/git/checkout.rs
index 5177eb38..1ca7d8fb 100644
--- a/cli/lnk-identities/t/src/tests/git/checkout.rs
+++ b/cli/lnk-identities/t/src/tests/git/checkout.rs
@@ -47,7 +47,7 @@ fn local_checkout() -> anyhow::Result<()> {
        signer: signer.into(),
    };

    let local = Local::new(&proj.project, temp.path().to_path_buf());
    let local = Local::new(&proj.project, temp.path().to_path_buf())?;
    let repo = checkout(
        &paths,
        settings,
-- 
2.35.3

Re: [PATCH radicle-link v2 1/1] lnk-identities: optionally use initial branch on git clone

Details
Message ID
<CML8QLERES94.1STKPO275ON0Q@haptop>
In-Reply-To
<20220901164653.12695-2-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
You need to run `cargo fmt` over `git.rs` :)

On Thu Sep 1, 2022 at 5:46 PM IST, Slack Coder wrote:
> Better follow git's command line behavior by setting one local branch, the
> projects default head, on clone.  It is assumed to be the project
> identities 'default_branch' field.
>
> Implement the new clone argument as optional to keep backward
> compatibility.
> ---
>  cli/lnk-identities/src/git.rs                 | 74 +++++++++++++------
>  cli/lnk-identities/src/git/checkout.rs        | 30 ++++++--
>  .../t/src/tests/git/checkout.rs               |  2 +-
>  3 files changed, 75 insertions(+), 31 deletions(-)
>
> diff --git a/cli/lnk-identities/src/git.rs b/cli/lnk-identities/src/git.rs
> index c9ed5e90..48e167b8 100644
> --- a/cli/lnk-identities/src/git.rs
> +++ b/cli/lnk-identities/src/git.rs
> @@ -16,14 +16,11 @@ use librad::{
>          },
>          types::{
>              remote::{LocalFetchspec, LocalPushspec, Remote},
> -            Fetchspec,
> -            Force,
> -            Refspec,
> +            Fetchspec, Force, Refspec,
>          },
>      },
>      git_ext::{self, OneLevel, Qualified},
> -    reflike,
> -    refspec_pattern,
> +    reflike, refspec_pattern,
>      std_ext::result::ResultExt as _,
>  };
>  
> @@ -49,6 +46,9 @@ pub enum Error {
>  
>      #[error(transparent)]
>      Git(#[from] git2::Error),
> +
> +    #[error("branch does not exist")]
> +    BranchNotExist,
>  }
>  
>  /// Equips a repository with a rad remote for the given id. If the directory at
> @@ -178,6 +178,26 @@ pub fn set_upstream<Url>(
>      Ok(())
>  }
>  
> +fn create_local_ref(
> +    repo: &git2::Repository,
> +    branch: git_ext::RefLike,
> +    oid: git2::Oid,
> +    remote: &Remote<LocalUrl>,
> +    msg: &str,
> +) -> Result<(), Error> {
> +    let _remote_branch = repo.reference(
> +        reflike!("refs/remotes")
> +            .join(remote.name.clone())
> +            .join(branch.clone())
> +            .as_str(),
> +        oid,
> +        true,
> +        msg,
> +    )?;
> +    let _local_branch = repo.reference(Qualified::from(branch).as_str(), oid, true, msg);
> +    Ok(())
> +}
> +
>  /// Clone a git repository to the `path` location, based off of the `remote`
>  /// provided.
>  ///
> @@ -188,29 +208,39 @@ pub fn clone<F>(
>      path: &Path,
>      storage: F,
>      mut remote: Remote<LocalUrl>,
> +    init_branch: Option<&str>,

Can you change this to be `git_ext::RefLike` instead, please?

>  ) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
>  where
>      F: CanOpenStorage + 'static,
>  {
>      let repo = git2::Repository::init(path)?;
>      remote.save(&repo)?;
> -    for (reference, oid) in remote.fetch(storage, &repo, LocalFetchspec::Configured)? {
> -        let msg = format!("Fetched `{}->{}`", reference, oid);
> -        tracing::debug!("{}", msg);
> -
> -        let branch: git_ext::RefLike = OneLevel::from(reference).into();
> -        let branch = branch.strip_prefix(remote.name.clone())?;
> -        let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
> -        let _remote_branch = repo.reference(
> -            reflike!("refs/remotes")
> -                .join(remote.name.clone())
> -                .join(branch.clone())
> -                .as_str(),
> -            oid,
> -            true,
> -            &msg,
> -        )?;
> -        let _local_branch = repo.reference(Qualified::from(branch).as_str(), oid, true, &msg);
> +
> +    let _branches: Vec<_> = remote

The underscore is misleading here. It generally means that a variable
isn't used, but in fact it is used below.

> +        .fetch(storage, &repo, LocalFetchspec::Configured)?
> +        .map(|(_ref, oid)| {
> +            let msg = format!("Fetched `{}->{}`", _ref, oid);
> +            tracing::debug!("{}", msg);
> +
> +            let branch: git_ext::RefLike = OneLevel::from(_ref).into();
> +            let branch = branch.strip_prefix(remote.name.clone()).unwrap();
> +            let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
> +            (branch, oid, msg)
> +        })
> +        .collect();
> +    match init_branch {
> +        Some(init_branch) => {
> +            let (branch, oid, msg) = _branches
> +                .iter()
> +                .find(|(branch, _, _)| branch.as_str() == init_branch)
> +                .ok_or(Error::BranchNotExist)?;

This error might cause some confusion for users. Could we provide:
a) The context of the missing branch, ie. we were trying to clone with
a speicific initial branch.
b) The actual branch we were looking for.

> +            create_local_ref(&repo, branch.clone(), *oid, &remote, msg)?;
> +        },
> +        None => {
> +            for (branch, oid, msg) in _branches {
> +                create_local_ref(&repo, branch, oid, &remote, &msg)?
> +            }
> +        },
>      }
>  
>      Ok((repo, remote))
> diff --git a/cli/lnk-identities/src/git/checkout.rs b/cli/lnk-identities/src/git/checkout.rs
> index b85163dd..bcbbfa2c 100644
> --- a/cli/lnk-identities/src/git/checkout.rs
> +++ b/cli/lnk-identities/src/git/checkout.rs
> @@ -139,17 +139,21 @@ where
>  pub struct Local {
>      url: LocalUrl,
>      path: PathBuf,
> +    default_branch: OneLevel,

I believe I asked for this to be optional in the last patch?

>  }
>  
>  impl Local {
> -    pub fn new<I>(identity: &I, path: PathBuf) -> Self
> +    pub fn new<I>(identity: &I, path: PathBuf) -> Result<Self, Error>
>      where
> -        I: HasName + HasUrn,
> +        I: HasBranch + HasName + HasUrn,
>      {
> -        Self {
> -            url: LocalUrl::from(identity.urn()),
> +        let urn = identity.urn();
> +        let default_branch = identity.branch_or_die(urn.clone())?;

The above would make this not need to use `branch_or_die`.

> +        Ok(Self {
> +            url: LocalUrl::from(urn),
>              path,
> -        }
> +            default_branch,
> +        })
>      }
>  
>      fn checkout<F>(self, open_storage: F) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
> @@ -164,7 +168,12 @@ impl Local {
>                  force: Force::True,
>              },
>          );
> -        Ok(git::clone(&self.path, open_storage, rad)?)
> +        Ok(git::clone(
> +            &self.path,
> +            open_storage,
> +            rad,
> +            Some(&self.default_branch),
> +        )?)
>      }
>  }
>  
> @@ -209,7 +218,12 @@ impl Peer {
>              force: Force::True,
>          }]);
>  
> -        let (repo, _) = git::clone(&self.path, open_storage.clone(), remote)?;
> +        let (repo, _) = git::clone(
> +            &self.path,
> +            open_storage.clone(),
> +            remote,
> +            Some(&self.default_branch),
> +        )?;
>  
>          // Create a rad remote and push the default branch so we can set it as the
>          // upstream.
> @@ -248,7 +262,7 @@ where
>  {
>      let path = path.resolve(identity.name());
>      Ok(match remote {
> -        None => Either::Left(Local::new(identity, path)),
> +        None => Either::Left(Local::new(identity, path)?),
>          Some(remote) => Either::Right(Peer::new(identity, remote, path)?),
>      })
>  }
> diff --git a/cli/lnk-identities/t/src/tests/git/checkout.rs b/cli/lnk-identities/t/src/tests/git/checkout.rs
> index 5177eb38..1ca7d8fb 100644
> --- a/cli/lnk-identities/t/src/tests/git/checkout.rs
> +++ b/cli/lnk-identities/t/src/tests/git/checkout.rs
> @@ -47,7 +47,7 @@ fn local_checkout() -> anyhow::Result<()> {
>          signer: signer.into(),
>      };
>  
> -    let local = Local::new(&proj.project, temp.path().to_path_buf());
> +    let local = Local::new(&proj.project, temp.path().to_path_buf())?;
>      let repo = checkout(
>          &paths,
>          settings,
> -- 
> 2.35.3

[PATCH radicle-link v3 0/1] lnk-identities: optionally use initial branch on git clone

Details
Message ID
<20220902154622.26289-1-slackcoder@server.ky>
In-Reply-To
<20220825173051.3734-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
The latest patch set addresses all issues from the feedback.

 - remove bad leading underscores '_' in some variable names
 - Use OneLevel and RefLike
 - make Local.default_branch optional
 - correct source code format

Slack Coder (1):
  lnk-identities: allow initial branch on git clone

 cli/lnk-identities/src/git.rs                 | 67 ++++++++++++++-----
 cli/lnk-identities/src/git/checkout.rs        | 30 ++++++---
 .../t/src/tests/git/checkout.rs               |  2 +-
 3 files changed, 73 insertions(+), 26 deletions(-)

--
2.35.3

[PATCH radicle-link v3 1/1] lnk-identities: allow initial branch on git clone

Details
Message ID
<20220902154622.26289-2-slackcoder@server.ky>
In-Reply-To
<20220825173051.3734-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
Patch: +73 -26
Better follow git's command line behavior by setting one local branch, the
projects default head, on clone.  It is assumed to be the project
identities 'default_branch' field.

Implement the new clone argument as optional to keep backward
compatibility.
---
 cli/lnk-identities/src/git.rs                 | 67 ++++++++++++++-----
 cli/lnk-identities/src/git/checkout.rs        | 30 ++++++---
 .../t/src/tests/git/checkout.rs               |  2 +-
 3 files changed, 73 insertions(+), 26 deletions(-)

diff --git a/cli/lnk-identities/src/git.rs b/cli/lnk-identities/src/git.rs
index c9ed5e90..c31fefc3 100644
--- a/cli/lnk-identities/src/git.rs
+++ b/cli/lnk-identities/src/git.rs
@@ -49,6 +49,9 @@ pub enum Error {

    #[error(transparent)]
    Git(#[from] git2::Error),

    #[error("failed to clone project with initial branch `{0}`, the branch is missing")]
    MissingInitialBranch(git_ext::RefLike),
}

/// Equips a repository with a rad remote for the given id. If the directory at
@@ -178,6 +181,26 @@ pub fn set_upstream<Url>(
    Ok(())
}

fn create_local_ref(
    repo: &git2::Repository,
    branch: git_ext::RefLike,
    oid: git2::Oid,
    remote: &Remote<LocalUrl>,
    msg: &str,
) -> Result<(), Error> {
    let _remote_branch = repo.reference(
        reflike!("refs/remotes")
            .join(remote.name.clone())
            .join(branch.clone())
            .as_str(),
        oid,
        true,
        msg,
    )?;
    let _local_branch = repo.reference(Qualified::from(branch).as_str(), oid, true, msg);
    Ok(())
}

/// Clone a git repository to the `path` location, based off of the `remote`
/// provided.
///
@@ -188,29 +211,39 @@ pub fn clone<F>(
    path: &Path,
    storage: F,
    mut remote: Remote<LocalUrl>,
    init_branch: Option<&git_ext::RefLike>,
) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
where
    F: CanOpenStorage + 'static,
{
    let repo = git2::Repository::init(path)?;
    remote.save(&repo)?;
    for (reference, oid) in remote.fetch(storage, &repo, LocalFetchspec::Configured)? {
        let msg = format!("Fetched `{}->{}`", reference, oid);
        tracing::debug!("{}", msg);

        let branch: git_ext::RefLike = OneLevel::from(reference).into();
        let branch = branch.strip_prefix(remote.name.clone())?;
        let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
        let _remote_branch = repo.reference(
            reflike!("refs/remotes")
                .join(remote.name.clone())
                .join(branch.clone())
                .as_str(),
            oid,
            true,
            &msg,
        )?;
        let _local_branch = repo.reference(Qualified::from(branch).as_str(), oid, true, &msg);

    let branches: Vec<_> = remote
        .fetch(storage, &repo, LocalFetchspec::Configured)?
        .map(|(reference, oid)| {
            let msg = format!("Fetched `{}->{}`", reference, oid);
            tracing::debug!("{}", msg);

            let branch: git_ext::RefLike = OneLevel::from(reference).into();
            let branch = branch.strip_prefix(remote.name.clone()).unwrap();
            let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
            (branch, oid, msg)
        })
        .collect();
    match init_branch {
        Some(init_branch) => {
            let (branch, oid, msg) = branches
                .iter()
                .find(|(branch, _, _)| branch == init_branch)
                .ok_or(Error::MissingInitialBranch(init_branch.clone()))?;
            create_local_ref(&repo, branch.clone(), *oid, &remote, msg)?;
        },
        None => {
            for (branch, oid, msg) in branches {
                create_local_ref(&repo, branch, oid, &remote, &msg)?
            }
        },
    }

    Ok((repo, remote))
diff --git a/cli/lnk-identities/src/git/checkout.rs b/cli/lnk-identities/src/git/checkout.rs
index b85163dd..5f9e86e2 100644
--- a/cli/lnk-identities/src/git/checkout.rs
+++ b/cli/lnk-identities/src/git/checkout.rs
@@ -139,17 +139,21 @@ where
pub struct Local {
    url: LocalUrl,
    path: PathBuf,
    default_branch: Option<OneLevel>,
}

impl Local {
    pub fn new<I>(identity: &I, path: PathBuf) -> Self
    pub fn new<I>(identity: &I, path: PathBuf) -> Result<Self, Error>
    where
        I: HasName + HasUrn,
        I: HasBranch + HasName + HasUrn,
    {
        Self {
            url: LocalUrl::from(identity.urn()),
        let urn = identity.urn();
        let default_branch = identity.branch();
        Ok(Self {
            url: LocalUrl::from(urn),
            path,
        }
            default_branch,
        })
    }

    fn checkout<F>(self, open_storage: F) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
@@ -164,7 +168,12 @@ impl Local {
                force: Force::True,
            },
        );
        Ok(git::clone(&self.path, open_storage, rad)?)
        Ok(git::clone(
            &self.path,
            open_storage,
            rad,
            self.default_branch.map(|v| RefLike::from(v)).as_ref(),
        )?)
    }
}

@@ -209,7 +218,12 @@ impl Peer {
            force: Force::True,
        }]);

        let (repo, _) = git::clone(&self.path, open_storage.clone(), remote)?;
        let (repo, _) = git::clone(
            &self.path,
            open_storage.clone(),
            remote,
            Some(&RefLike::from(self.default_branch.clone())),
        )?;

        // Create a rad remote and push the default branch so we can set it as the
        // upstream.
@@ -248,7 +262,7 @@ where
{
    let path = path.resolve(identity.name());
    Ok(match remote {
        None => Either::Left(Local::new(identity, path)),
        None => Either::Left(Local::new(identity, path)?),
        Some(remote) => Either::Right(Peer::new(identity, remote, path)?),
    })
}
diff --git a/cli/lnk-identities/t/src/tests/git/checkout.rs b/cli/lnk-identities/t/src/tests/git/checkout.rs
index 5177eb38..1ca7d8fb 100644
--- a/cli/lnk-identities/t/src/tests/git/checkout.rs
+++ b/cli/lnk-identities/t/src/tests/git/checkout.rs
@@ -47,7 +47,7 @@ fn local_checkout() -> anyhow::Result<()> {
        signer: signer.into(),
    };

    let local = Local::new(&proj.project, temp.path().to_path_buf());
    let local = Local::new(&proj.project, temp.path().to_path_buf())?;
    let repo = checkout(
        &paths,
        settings,
-- 
2.35.3

Re: [PATCH radicle-link v3 0/1] lnk-identities: optionally use initial branch on git clone

Details
Message ID
<CMOAZEOLT7MR.1J8ZBNK7FNKEW@haptop>
In-Reply-To
<20220902154622.26289-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
Looks good, I had to make a couple of changes to appease clippy and
also noticed that a `Result` was not needed anymore. I amended those
locally.

One last thing I need from you is your sign-off, ie. name and email,
per our DCO policy[0].

[0]: https://github.com/radicle-dev/radicle-link/blob/master/CONTRIBUTING.md#certificate-of-origin

[PATCH radicle-link v4 0/1] lnk-identities: optionally use initial branch on git clone

Details
Message ID
<20220905124539.22240-1-slackcoder@server.ky>
In-Reply-To
<20220825173051.3734-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
Amend commits with sign-off

Slack Coder (1):
  lnk-identities: allow initial branch on git clone

 cli/lnk-identities/src/git.rs                 | 67 ++++++++++++++-----
 cli/lnk-identities/src/git/checkout.rs        | 30 ++++++---
 .../t/src/tests/git/checkout.rs               |  2 +-
 3 files changed, 73 insertions(+), 26 deletions(-)

--
2.35.3

[PATCH radicle-link v4 1/1] lnk-identities: allow initial branch on git clone

Details
Message ID
<20220905124539.22240-2-slackcoder@server.ky>
In-Reply-To
<20220825173051.3734-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
Patch: +73 -26
Better follow git's command line behavior by setting one local branch, the
projects default head, on clone.  It is assumed to be the project
identities 'default_branch' field.

Implement the new clone argument as optional to keep backward
compatibility.

Signed-off-by: Slack Coder <slackcoder@server.ky>
---
 cli/lnk-identities/src/git.rs                 | 67 ++++++++++++++-----
 cli/lnk-identities/src/git/checkout.rs        | 30 ++++++---
 .../t/src/tests/git/checkout.rs               |  2 +-
 3 files changed, 73 insertions(+), 26 deletions(-)

diff --git a/cli/lnk-identities/src/git.rs b/cli/lnk-identities/src/git.rs
index c9ed5e90..c31fefc3 100644
--- a/cli/lnk-identities/src/git.rs
+++ b/cli/lnk-identities/src/git.rs
@@ -49,6 +49,9 @@ pub enum Error {

    #[error(transparent)]
    Git(#[from] git2::Error),

    #[error("failed to clone project with initial branch `{0}`, the branch is missing")]
    MissingInitialBranch(git_ext::RefLike),
}

/// Equips a repository with a rad remote for the given id. If the directory at
@@ -178,6 +181,26 @@ pub fn set_upstream<Url>(
    Ok(())
}

fn create_local_ref(
    repo: &git2::Repository,
    branch: git_ext::RefLike,
    oid: git2::Oid,
    remote: &Remote<LocalUrl>,
    msg: &str,
) -> Result<(), Error> {
    let _remote_branch = repo.reference(
        reflike!("refs/remotes")
            .join(remote.name.clone())
            .join(branch.clone())
            .as_str(),
        oid,
        true,
        msg,
    )?;
    let _local_branch = repo.reference(Qualified::from(branch).as_str(), oid, true, msg);
    Ok(())
}

/// Clone a git repository to the `path` location, based off of the `remote`
/// provided.
///
@@ -188,29 +211,39 @@ pub fn clone<F>(
    path: &Path,
    storage: F,
    mut remote: Remote<LocalUrl>,
    init_branch: Option<&git_ext::RefLike>,
) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
where
    F: CanOpenStorage + 'static,
{
    let repo = git2::Repository::init(path)?;
    remote.save(&repo)?;
    for (reference, oid) in remote.fetch(storage, &repo, LocalFetchspec::Configured)? {
        let msg = format!("Fetched `{}->{}`", reference, oid);
        tracing::debug!("{}", msg);

        let branch: git_ext::RefLike = OneLevel::from(reference).into();
        let branch = branch.strip_prefix(remote.name.clone())?;
        let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
        let _remote_branch = repo.reference(
            reflike!("refs/remotes")
                .join(remote.name.clone())
                .join(branch.clone())
                .as_str(),
            oid,
            true,
            &msg,
        )?;
        let _local_branch = repo.reference(Qualified::from(branch).as_str(), oid, true, &msg);

    let branches: Vec<_> = remote
        .fetch(storage, &repo, LocalFetchspec::Configured)?
        .map(|(reference, oid)| {
            let msg = format!("Fetched `{}->{}`", reference, oid);
            tracing::debug!("{}", msg);

            let branch: git_ext::RefLike = OneLevel::from(reference).into();
            let branch = branch.strip_prefix(remote.name.clone()).unwrap();
            let branch = branch.strip_prefix(reflike!("heads")).unwrap_or(branch);
            (branch, oid, msg)
        })
        .collect();
    match init_branch {
        Some(init_branch) => {
            let (branch, oid, msg) = branches
                .iter()
                .find(|(branch, _, _)| branch == init_branch)
                .ok_or(Error::MissingInitialBranch(init_branch.clone()))?;
            create_local_ref(&repo, branch.clone(), *oid, &remote, msg)?;
        },
        None => {
            for (branch, oid, msg) in branches {
                create_local_ref(&repo, branch, oid, &remote, &msg)?
            }
        },
    }

    Ok((repo, remote))
diff --git a/cli/lnk-identities/src/git/checkout.rs b/cli/lnk-identities/src/git/checkout.rs
index b85163dd..5f9e86e2 100644
--- a/cli/lnk-identities/src/git/checkout.rs
+++ b/cli/lnk-identities/src/git/checkout.rs
@@ -139,17 +139,21 @@ where
pub struct Local {
    url: LocalUrl,
    path: PathBuf,
    default_branch: Option<OneLevel>,
}

impl Local {
    pub fn new<I>(identity: &I, path: PathBuf) -> Self
    pub fn new<I>(identity: &I, path: PathBuf) -> Result<Self, Error>
    where
        I: HasName + HasUrn,
        I: HasBranch + HasName + HasUrn,
    {
        Self {
            url: LocalUrl::from(identity.urn()),
        let urn = identity.urn();
        let default_branch = identity.branch();
        Ok(Self {
            url: LocalUrl::from(urn),
            path,
        }
            default_branch,
        })
    }

    fn checkout<F>(self, open_storage: F) -> Result<(git2::Repository, Remote<LocalUrl>), Error>
@@ -164,7 +168,12 @@ impl Local {
                force: Force::True,
            },
        );
        Ok(git::clone(&self.path, open_storage, rad)?)
        Ok(git::clone(
            &self.path,
            open_storage,
            rad,
            self.default_branch.map(|v| RefLike::from(v)).as_ref(),
        )?)
    }
}

@@ -209,7 +218,12 @@ impl Peer {
            force: Force::True,
        }]);

        let (repo, _) = git::clone(&self.path, open_storage.clone(), remote)?;
        let (repo, _) = git::clone(
            &self.path,
            open_storage.clone(),
            remote,
            Some(&RefLike::from(self.default_branch.clone())),
        )?;

        // Create a rad remote and push the default branch so we can set it as the
        // upstream.
@@ -248,7 +262,7 @@ where
{
    let path = path.resolve(identity.name());
    Ok(match remote {
        None => Either::Left(Local::new(identity, path)),
        None => Either::Left(Local::new(identity, path)?),
        Some(remote) => Either::Right(Peer::new(identity, remote, path)?),
    })
}
diff --git a/cli/lnk-identities/t/src/tests/git/checkout.rs b/cli/lnk-identities/t/src/tests/git/checkout.rs
index 5177eb38..1ca7d8fb 100644
--- a/cli/lnk-identities/t/src/tests/git/checkout.rs
+++ b/cli/lnk-identities/t/src/tests/git/checkout.rs
@@ -47,7 +47,7 @@ fn local_checkout() -> anyhow::Result<()> {
        signer: signer.into(),
    };

    let local = Local::new(&proj.project, temp.path().to_path_buf());
    let local = Local::new(&proj.project, temp.path().to_path_buf())?;
    let repo = checkout(
        &paths,
        settings,
-- 
2.35.3

Re: [PATCH radicle-link v4 0/1] lnk-identities: optionally use initial branch on git clone

Details
Message ID
<CMOHHTZOL6H2.316KPW0VECG6V@haptop>
In-Reply-To
<20220905124539.22240-1-slackcoder@server.ky> (view parent)
DKIM signature
missing
Download raw message
Thanks! Merged :)

Master-at: 904250f533a6482ebfa4bfb7e3660a4982f07ed8
Reply to thread Export thread (mbox)