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) {
--- a/tests/test-chg.t Mon Apr 24 10:30:08 2023 -0400
+++ b/tests/test-chg.t Thu Apr 20 09:23:58 2023 -0400
@@ -553,3 +553,20 @@
$ filteredchg log -r . --no-profile
$ filteredchg log -r .
Sample count: * (glob)
+
+chg setting CHGHG itself
+------------------------
+
+If CHGHG is not set, chg will set it before spawning the command server.
+ $ hg --kill-chg-daemon
+ $ HG=$CHGHG CHGHG= CHGDEBUG= hg debugshell -c \
+ > 'ui.write(b"CHGHG=%s\n" % ui.environ.get(b"CHGHG"))' 2>&1 \
+ > | egrep 'CHGHG|start'
+ chg: debug: * start cmdserver at * (glob)
+ CHGHG=/*/install/bin/hg (glob)
+
+Running the same command a second time shouldn't spawn a new command server.
+ $ HG=$CHGHG CHGHG= CHGDEBUG= hg debugshell -c \
+ > 'ui.write(b"CHGHG=%s\n" % ui.environ.get(b"CHGHG"))' 2>&1 \
+ > | egrep 'CHGHG|start'
+ CHGHG=/*/install/bin/hg (glob)