Patchwork D8357: rust-chg: add brief comment about initial capacity of temp_sock_path()

login
register
mail settings
Submitter phabricator
Date April 2, 2020, 11:12 a.m.
Message ID <differential-rev-PHID-DREV-swxq7k3hpwoaepy2kbec-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45989/
State Superseded
Headers show

Comments

phabricator - April 2, 2020, 11:12 a.m.
yuja created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I don't know if it can be expressed as a compile-time constant, so it's
  a comment for now.
  
  About this series:
  This is quite old patches for rust-chg. I heard from Octobus people that
  there's a plan to do an experiment on merging hgcli + chg + some Rust?,
  so I decided to respin the rust-chg series.
  
  Maybe we'll rewrite the core to leverage the recent async/await functionality,
  but I want to first make my old patches in so the rust-chg can be a drop-in
  replacement for the chg of C. Compiler warnings will be removed later, and
  the codebase will be upgraded to the 2018 edition later.

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8357

AFFECTED FILES
  rust/chg/src/locator.rs

CHANGE DETAILS




To: yuja, #hg-reviewers
Cc: mercurial-devel
Raphaël Gomès - April 2, 2020, 12:17 p.m.
Maybe I'm completely off-base here, but pid length varies with the OS, 
the architecture and user preferences. On Linux 64 bits it can be 
4194304, 32768 for 32bits and 99999 for FreeBSD. We could add constants, 
but I feel like it would be a lot of work for not a lot of gain.

This patch looks good no matter what.

On 4/2/20 1:12 PM, yuja (Yuya Nishihara) wrote:
> yuja created this revision.
> Herald added a subscriber: mercurial-devel.
> Herald added a reviewer: hg-reviewers.
>
> REVISION SUMMARY
>    I don't know if it can be expressed as a compile-time constant, so it's
>    a comment for now.
>    
>    About this series:
>    This is quite old patches for rust-chg. I heard from Octobus people that
>    there's a plan to do an experiment on merging hgcli + chg + some Rust?,
>    so I decided to respin the rust-chg series.
>    
>    Maybe we'll rewrite the core to leverage the recent async/await functionality,
>    but I want to first make my old patches in so the rust-chg can be a drop-in
>    replacement for the chg of C. Compiler warnings will be removed later, and
>    the codebase will be upgraded to the 2018 edition later.
>
> REPOSITORY
>    rHG Mercurial
>
> BRANCH
>    default
>
> REVISION DETAIL
>    https://phab.mercurial-scm.org/D8357
>
> AFFECTED FILES
>    rust/chg/src/locator.rs
>
> CHANGE DETAILS
>
> diff --git a/rust/chg/src/locator.rs b/rust/chg/src/locator.rs
> --- a/rust/chg/src/locator.rs
> +++ b/rust/chg/src/locator.rs
> @@ -47,7 +47,7 @@
>       /// Temporary socket path for this client process.
>       fn temp_sock_path(&self) -> PathBuf {
>           let src = self.base_sock_path.as_os_str().as_bytes();
> -        let mut buf = Vec::with_capacity(src.len() + 6);
> +        let mut buf = Vec::with_capacity(src.len() + 6); // "{src}.{pid}".len()
>           buf.extend_from_slice(src);
>           buf.extend_from_slice(format!(".{}", self.process_id).as_bytes());
>           OsString::from_vec(buf).into()
>
>
>
> To: yuja, #hg-reviewers
> Cc: mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
phabricator - April 3, 2020, 12:18 p.m.
Alphare added a comment.
Alphare accepted this revision.


  (I'll be re-pasting all of my comments that got lost in emails for this series)
  
  Maybe I'm completely off-base here, but pid length varies with the OS, the architecture and user preferences. On Linux 64 bits it can be 4194304, 32768 for 32bits and 99999 for FreeBSD. We could add constants, but I feel like it would be a lot of work for not a lot of gain.
  
  This patch looks good no matter what.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8357/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8357

To: yuja, #hg-reviewers, Alphare
Cc: Alphare, mercurial-devel
phabricator - April 3, 2020, 12:25 p.m.
Alphare added a comment.


  This series is great.
  
  In case anyone wants to give their opinion, I have suggested to Yuya to start using `async_std` now that it's post-1.0 instead of `tokio`. It's made by `tokio` devs and is supposedly the better designed of the two as it gained hindsight from the different `tokio`/`futures` versions of the past few years.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8357/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8357

To: yuja, #hg-reviewers, Alphare
Cc: Alphare, mercurial-devel

Patch

diff --git a/rust/chg/src/locator.rs b/rust/chg/src/locator.rs
--- a/rust/chg/src/locator.rs
+++ b/rust/chg/src/locator.rs
@@ -47,7 +47,7 @@ 
     /// Temporary socket path for this client process.
     fn temp_sock_path(&self) -> PathBuf {
         let src = self.base_sock_path.as_os_str().as_bytes();
-        let mut buf = Vec::with_capacity(src.len() + 6);
+        let mut buf = Vec::with_capacity(src.len() + 6); // "{src}.{pid}".len()
         buf.extend_from_slice(src);
         buf.extend_from_slice(format!(".{}", self.process_id).as_bytes());
         OsString::from_vec(buf).into()