Patchwork [1,of,3] chg: extract gethgcmd logic to a function

login
register
mail settings
Submitter Jun Wu
Date Feb. 24, 2016, 2:31 p.m.
Message ID <fb9caea246de70223b26.1456324283@x1c>
Download mbox | patch
Permalink /patch/13352/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Feb. 24, 2016, 2:31 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1456323840 0
#      Wed Feb 24 14:24:00 2016 +0000
# Node ID fb9caea246de70223b26fe94ffcd1f6b32ba49a6
# Parent  1d3998abd58ad32bd17381e14a530ff7ff175d48
chg: extract gethgcmd logic to a function

gethgcmd is to get original hg (not chg) binary name. This patch extracts
the logic from execcmdserver to make it available for the following patch.
Sean Farley - Feb. 24, 2016, 6:24 p.m.
Jun Wu <quark@fb.com> writes:

> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456323840 0
> #      Wed Feb 24 14:24:00 2016 +0000
> # Node ID fb9caea246de70223b26fe94ffcd1f6b32ba49a6
> # Parent  1d3998abd58ad32bd17381e14a530ff7ff175d48
> chg: extract gethgcmd logic to a function
>
> gethgcmd is to get original hg (not chg) binary name. This patch extracts
> the logic from execcmdserver to make it available for the following patch.
>
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -190,13 +190,22 @@
>  	opts->lockfd = -1;
>  }
>  
> +static const char *gethgcmd(void)
> +{
> +	static const char *hgcmd;

Did you mean to change this to static?
Jun Wu - Feb. 24, 2016, 6:38 p.m.
On 02/24/2016 06:24 PM, Sean Farley wrote:
> Did you mean to change this to static?

I don't quite get it but do you mean the following change?

  static const char *gethgcmd(void)
  {
-  static const char *hgcmd;
+  const char *hgcmd;

"hgcmd" is static intensionally. So we can skip getenv() (expensive)
when calling gethgcmd() the 2nd time. It's like "@util.cachefunc".
Sean Farley - Feb. 24, 2016, 6:51 p.m.
Jun Wu <quark@fb.com> writes:

> On 02/24/2016 06:24 PM, Sean Farley wrote:
>> Did you mean to change this to static?
>
> I don't quite get it but do you mean the following change?
>
>   static const char *gethgcmd(void)
>   {
> -  static const char *hgcmd;
> +  const char *hgcmd;
>
> "hgcmd" is static intensionally. So we can skip getenv() (expensive)
> when calling gethgcmd() the 2nd time. It's like "@util.cachefunc".

Righto, I got that part. I just wanted to make sure it was intentional
:-) These patches are fine by me, then.
Yuya Nishihara - Feb. 25, 2016, 1:15 p.m.
On Wed, 24 Feb 2016 14:31:23 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456323840 0
> #      Wed Feb 24 14:24:00 2016 +0000
> # Node ID fb9caea246de70223b26fe94ffcd1f6b32ba49a6
> # Parent  1d3998abd58ad32bd17381e14a530ff7ff175d48
> chg: extract gethgcmd logic to a function
> 
> gethgcmd is to get original hg (not chg) binary name. This patch extracts
> the logic from execcmdserver to make it available for the following patch.
> 
> diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
> --- a/contrib/chg/chg.c
> +++ b/contrib/chg/chg.c
> @@ -190,13 +190,22 @@
>  	opts->lockfd = -1;
>  }
>  
> +static const char *gethgcmd(void)
> +{
> +	static const char *hgcmd;

I've added = NULL. I know static variables are placed in .bss section, but
the whole initialization rule is hard to remember.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -190,13 +190,22 @@ 
 	opts->lockfd = -1;
 }
 
+static const char *gethgcmd(void)
+{
+	static const char *hgcmd;
+	if (!hgcmd) {
+		hgcmd = getenv("CHGHG");
+		if (!hgcmd || hgcmd[0] == '\0')
+			hgcmd = getenv("HG");
+		if (!hgcmd || hgcmd[0] == '\0')
+			hgcmd = "hg";
+	}
+	return hgcmd;
+}
+
 static void execcmdserver(const struct cmdserveropts *opts)
 {
-	const char *hgcmd = getenv("CHGHG");
-	if (!hgcmd || hgcmd[0] == '\0')
-		hgcmd = getenv("HG");
-	if (!hgcmd || hgcmd[0] == '\0')
-		hgcmd = "hg";
+	const char *hgcmd = gethgcmd();
 
 	const char *baseargv[] = {
 		hgcmd,