diff contrib/chg/chg.c @ 50353:c2a1f8668606 stable

chg: set CHGHG before connecting to command server

cf4d2f31 (!523) changed chg to set `CHGHG` itself when spawning a new command
server, in order to ensure that the path to the `hg` executable would be checked
during server validation. (This is useful when chg is built with `HGPATHREL`).

However, that change broke chg because it failed to set `CHGHG` before trying
to connect to an existing command server. This means that if `CHGHG` is not
present in the environment, chg will always spawn a new command server, entirely
negating the point of chg.

This breakage wasn't initially caught because of the difficulty of writing
automated tests with the `HGPATHREL` feature enabled, which meant the change
was only tested manually to make sure that it fixed the problem with `HGPATHREL`
that prompted the change.

In practice, this functionality is only really useful when chg is built with
`HGPATHREL`, so I considered wrapping it in an `#ifdef` to preserve the old
behavior by default. However, this makes it hard to write tests since one would
have to explicitly set `HGPATHREL=1` when running `run-tests.py` (which is why
the original change lacked tests). It would be great if there were a way of
testing features that are gated behind conditional compilation.

--- a/contrib/chg/chg.c	Mon Apr 24 10:30:08 2023 -0400
+++ b/contrib/chg/chg.c	Thu Apr 20 09:23:58 2023 -0400
@@ -234,18 +234,12 @@
 			hgcmd = "hg";
 #endif
 	}
-	/* Set $CHGHG to the path to the seleted hg executable if it wasn't
-	 * already set. This has the effect of ensuring that a new command
-	 * server will be spawned if the existing command server is running from
-	 * an executable at a different path. */
-	if (setenv("CHGHG", hgcmd, 1) != 0)
-		abortmsgerrno("failed to setenv");
 	return hgcmd;
 }
 
-static void execcmdserver(const char *hgcmd, const struct cmdserveropts *opts)
+static void execcmdserver(const struct cmdserveropts *opts)
 {
-
+	const char *hgcmd = gethgcmd();
 	const char *baseargv[] = {
 	    hgcmd,     "serve",     "--no-profile",     "--cmdserver",
 	    "chgunix", "--address", opts->initsockname, "--daemon-postexec",
@@ -382,16 +376,11 @@
 
 	debugmsg("start cmdserver at %s", opts->initsockname);
 
-	/* Get the path to the hg executable before we fork because this
-	 * function might update the environment, and we want this to be
-	 * reflected in both the parent and child processes. */
-	const char *hgcmd = gethgcmd();
-
 	pid_t pid = fork();
 	if (pid < 0)
 		abortmsg("failed to fork cmdserver process");
 	if (pid == 0) {
-		execcmdserver(hgcmd, opts);
+		execcmdserver(opts);
 	} else {
 		hgc = retryconnectcmdserver(opts, pid);
 	}
@@ -525,6 +514,16 @@
 		}
 	}
 
+	/* Set $CHGHG to the path of the hg executable we intend to use. This
+	 * is a no-op if $CHGHG was expliclty specified, but otherwise this
+	 * ensures that we will spawn a new command server if we connect to an
+	 * existing one running from a different executable. This should only
+	 * only be needed when chg is built with HGPATHREL since otherwise the
+	 * hg executable used when CHGHG is absent should be deterministic.
+	 * */
+	if (setenv("CHGHG", gethgcmd(), 1) != 0)
+		abortmsgerrno("failed to setenv");
+
 	hgclient_t *hgc;
 	size_t retry = 0;
 	while (1) {