Patchwork D8021: chg: switch to using global `environ` instead of envp from main

login
register
mail settings
Submitter phabricator
Date Jan. 28, 2020, 12:57 a.m.
Message ID <differential-rev-PHID-DREV-ztrqiieo6e7xmzxjddym-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44704/
State Superseded
Headers show

Comments

phabricator - Jan. 28, 2020, 12:57 a.m.
spectral created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  My followup patch wants to modify the environment before we call setenv on the
  chg server process (to add items), and that won't be handled properly if we use
  envp (which isn't modified by putenv).

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  contrib/chg/chg.c
  contrib/chg/hgclient.c
  contrib/chg/hgclient.h

CHANGE DETAILS




To: spectral, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 28, 2020, 11:57 a.m.
pulkit added a comment.


  I don't know about the code which this series touch, but since D7550 <https://phab.mercurial-scm.org/D7550> already made to 5.3rc, should this series be targeted for stable branch?

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers
Cc: pulkit, mercurial-devel
Yuya Nishihara - Jan. 28, 2020, 3:55 p.m.
>  {
>  	if (getenv("CHGDEBUG"))
>  		enabledebugmsg();
> @@ -429,7 +431,7 @@
>  		hgc = connectcmdserver(&opts);
>  		if (!hgc)
>  			abortmsg("cannot open hg client");
> -		hgc_setenv(hgc, envp);
> +		hgc_setenv(hgc);

I prefer passing in (const char**)environ to hgc_setenv().
phabricator - Jan. 28, 2020, 4:10 p.m.
yuja added a comment.


  > {
  >
  >   	if (getenv("CHGDEBUG"))
  >   		enabledebugmsg();
  >
  > @@ -429,7 +431,7 @@
  >
  >   		hgc = connectcmdserver(&opts);
  >   		if (!hgc)
  >   			abortmsg("cannot open hg client");
  >
  > - hgc_setenv(hgc, envp);
  >
  > +		hgc_setenv(hgc);
  
  I prefer passing in (const char**)environ to hgc_setenv().

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers
Cc: yuja, pulkit, mercurial-devel
phabricator - Jan. 28, 2020, 6:57 p.m.
spectral added a comment.


  > I prefer passing in (const char**)environ to hgc_setenv().
  
  Done.

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers
Cc: yuja, pulkit, mercurial-devel
phabricator - Jan. 28, 2020, 9:36 p.m.
martinvonz added a comment.


  Queuing based on @yuja's review.

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers
Cc: martinvonz, yuja, pulkit, mercurial-devel
phabricator - Jan. 28, 2020, 9:38 p.m.
martinvonz added a comment.


  In D8021#118342 <https://phab.mercurial-scm.org/D8021#118342>, @martinvonz wrote:
  
  > Queuing based on @yuja's review.
  
  Actually, since there's no point with this patch without the child, I'll wait until @spectral and @yuja have agreed what to do with that one first.

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers
Cc: martinvonz, yuja, pulkit, mercurial-devel

Patch

diff --git a/contrib/chg/hgclient.h b/contrib/chg/hgclient.h
--- a/contrib/chg/hgclient.h
+++ b/contrib/chg/hgclient.h
@@ -25,6 +25,6 @@ 
                           size_t argsize);
 int hgc_runcommand(hgclient_t *hgc, const char *const args[], size_t argsize);
 void hgc_attachio(hgclient_t *hgc);
-void hgc_setenv(hgclient_t *hgc, const char *const envp[]);
+void hgc_setenv(hgclient_t *hgc);
 
 #endif /* HGCLIENT_H_ */
diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
--- a/contrib/chg/hgclient.c
+++ b/contrib/chg/hgclient.c
@@ -26,6 +26,8 @@ 
 #include "procutil.h"
 #include "util.h"
 
+extern char **environ;
+
 enum { CAP_GETENCODING = 0x0001,
        CAP_RUNCOMMAND = 0x0002,
        /* cHg extension: */
@@ -644,12 +646,12 @@ 
  * @param envp  list of environment variables in "NAME=VALUE" format,
  *              terminated by NULL.
  */
-void hgc_setenv(hgclient_t *hgc, const char *const envp[])
+void hgc_setenv(hgclient_t *hgc)
 {
-	assert(hgc && envp);
+	assert(hgc && environ);
 	if (!(hgc->capflags & CAP_SETENV)) {
 		return;
 	}
-	packcmdargs(&hgc->ctx, envp, /*argsize*/ -1);
+	packcmdargs(&hgc->ctx, (const char**)environ, /*argsize*/ -1);
 	writeblockrequest(hgc, "setenv");
 }
diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -30,6 +30,8 @@ 
 #define PATH_MAX 4096
 #endif
 
+extern char **environ;
+
 struct cmdserveropts {
 	char sockname[PATH_MAX];
 	char initsockname[PATH_MAX];
@@ -394,7 +396,7 @@ 
 		abortmsgerrno("failed to exec original hg");
 }
 
-int main(int argc, const char *argv[], const char *envp[])
+int main(int argc, const char *argv[])
 {
 	if (getenv("CHGDEBUG"))
 		enabledebugmsg();
@@ -429,7 +431,7 @@ 
 		hgc = connectcmdserver(&opts);
 		if (!hgc)
 			abortmsg("cannot open hg client");
-		hgc_setenv(hgc, envp);
+		hgc_setenv(hgc);
 		const char **insts = hgc_validate(hgc, argv + 1, argc - 1);
 		int needreconnect = runinstructions(&opts, insts);
 		free(insts);