Patchwork [1,of,3,V2] chg: fallback to original hg for some unsupported commands or flags

login
register
mail settings
Submitter Jun Wu
Date Feb. 26, 2016, 2:20 p.m.
Message ID <ecefdf920cc8b8b983e0.1456496425@x1c>
Download mbox | patch
Permalink /patch/13411/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Feb. 26, 2016, 2:20 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1456496279 0
#      Fri Feb 26 14:17:59 2016 +0000
# Node ID ecefdf920cc8b8b983e06da8e82b8db0f67b85de
# Parent  824ccfaf08a238b50f179b39ceebf303c798031a
chg: fallback to original hg for some unsupported commands or flags

There are some known unsupported commands or flags for chg, such as hg serve -d
and hg foo --time. This patch detects these situations and transparently fall
back to the original hg. So the users won't bother remembering what chg can and
cannot do by themselves.

The current detection is not 100% accurate since we do not have an equivalent
command line parser in C. But it tries not to cause false positives that
prevents people from using chg for legit cases. In the future we may want to
implement a more accurate "unsupported" check server-side.
Yuya Nishihara - Feb. 27, 2016, 7:37 a.m.
On Fri, 26 Feb 2016 14:20:25 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456496279 0
> #      Fri Feb 26 14:17:59 2016 +0000
> # Node ID ecefdf920cc8b8b983e06da8e82b8db0f67b85de
> # Parent  824ccfaf08a238b50f179b39ceebf303c798031a
> chg: fallback to original hg for some unsupported commands or flags

Queued modified version to the clowncopter, thanks.

> The current detection is not 100% accurate since we do not have an equivalent
> command line parser in C. But it tries not to cause false positives that
> prevents people from using chg for legit cases. In the future we may want to
> implement a more accurate "unsupported" check server-side.

I want to make them just work instead.

> +/*
> + * Test whether the command is unsupported or not. This is not designed to
> + * cover all cases. But it's fast, does not depend on the server and does
> + * not return false positives.
> + */
> +static int isunsupported(int argc, const char *argv[])
> +{
> +	enum {
> +		SERVE = 1,
> +		DAEMON = 2,
> +		SERVEDAEMON = SERVE | DAEMON,
> +		TIME = 4,
> +	};
> +	unsigned int state = 0;
> +	int i;
> +	for (i = 0; i < argc; ++i) {
> +		if (!strcmp(argv[i], "--"))
> +			break;
> +		if (i == 0 && strcmp("serve", argv[i]) == 0)
> +			state |= SERVE;

This can't catch "hg --xxx serve -d", but that's okay bacause this function
tries to avoid false positive.

> +static void execoriginalhg(const char *argv[])
> +{
> +	debugmsg("execute original hg");
> +	if (execvp(gethgcmd(), (char **)argv) < 0)
> +		abortmsg("failed to exec original hg (errno = %d)", errno);
> +}

Confirmed that the C standard says argv is NULL-terminated.

I'm not sure if we should set argv[0] to hgcmd. And it can exec chg infinitely
as it isn't guarded by CHGINTERNALMARK.

>  int main(int argc, const char *argv[], const char *envp[])
>  {
>  	if (getenv("CHGDEBUG"))
>  		enabledebugmsg();
>  
> +	if (isunsupported(argc, argv))
> +		execoriginalhg(argv);

Fixed as isunsupported(argc - 1, argv + 1).
Jun Wu - Feb. 27, 2016, 5:34 p.m.
On 02/27/2016 07:37 AM, Yuya Nishihara wrote:
> I want to make them just work instead.

The problem here is you don't know all the flags and how many values
should be after each of them.

> I'm not sure if we should set argv[0] to hgcmd. And it can exec chg
> infinitely as it isn't guarded by CHGINTERNALMARK.

I think it only affects argv[0] and hg does not use it so it's safe.

> Fixed as isunsupported(argc - 1, argv + 1).

Oops. I should be more careful. Thanks!
Yuya Nishihara - Feb. 28, 2016, 3:26 a.m.
On Sat, 27 Feb 2016 17:34:21 +0000, Jun Wu wrote:
> On 02/27/2016 07:37 AM, Yuya Nishihara wrote:
> > I want to make them just work instead.
> 
> The problem here is you don't know all the flags and how many values
> should be after each of them.

I meant I want to fix "--time" and "serve --daemon" to do the right thing
in command-server session.
Yuya Nishihara - Feb. 28, 2016, 10:43 a.m.
On Sun, 28 Feb 2016 12:26:54 +0900, Yuya Nishihara wrote:
> On Sat, 27 Feb 2016 17:34:21 +0000, Jun Wu wrote:
> > On 02/27/2016 07:37 AM, Yuya Nishihara wrote:
> > > I want to make them just work instead.
> > 
> > The problem here is you don't know all the flags and how many values
> > should be after each of them.
> 
> I meant I want to fix "--time" and "serve --daemon" to do the right thing
> in command-server session.

Ok, I managed to get them working. Fixing "--time" is trivial, I can send the
patches. "serve --daemon" isn't, I need to refactor _dispatch(), clean up a
mess and fix several bugs in my patches.
Jun Wu - Feb. 28, 2016, 11:57 a.m.
On 02/28/2016 03:26 AM, Yuya Nishihara wrote:
> I meant I want to fix "--time" and "serve --daemon" to do the right
> thing in command-server session.

Wow. That's great to hear! I thought about that but haven't taken action
yet.

Patch

diff --git a/contrib/chg/chg.c b/contrib/chg/chg.c
--- a/contrib/chg/chg.c
+++ b/contrib/chg/chg.c
@@ -448,11 +448,51 @@ 
 	abortmsg("failed to prepare pager (errno = %d)", errno);
 }
 
+/*
+ * Test whether the command is unsupported or not. This is not designed to
+ * cover all cases. But it's fast, does not depend on the server and does
+ * not return false positives.
+ */
+static int isunsupported(int argc, const char *argv[])
+{
+	enum {
+		SERVE = 1,
+		DAEMON = 2,
+		SERVEDAEMON = SERVE | DAEMON,
+		TIME = 4,
+	};
+	unsigned int state = 0;
+	int i;
+	for (i = 0; i < argc; ++i) {
+		if (!strcmp(argv[i], "--"))
+			break;
+		if (i == 0 && strcmp("serve", argv[i]) == 0)
+			state |= SERVE;
+		else if (strcmp("-d", argv[i]) == 0 ||
+			 strcmp("--daemon", argv[i]) == 0)
+			state |= DAEMON;
+		else if (strcmp("--time", argv[i]) == 0)
+			state |= TIME;
+	}
+	return (state & TIME) == TIME ||
+	       (state & SERVEDAEMON) == SERVEDAEMON;
+}
+
+static void execoriginalhg(const char *argv[])
+{
+	debugmsg("execute original hg");
+	if (execvp(gethgcmd(), (char **)argv) < 0)
+		abortmsg("failed to exec original hg (errno = %d)", errno);
+}
+
 int main(int argc, const char *argv[], const char *envp[])
 {
 	if (getenv("CHGDEBUG"))
 		enabledebugmsg();
 
+	if (isunsupported(argc, argv))
+		execoriginalhg(argv);
+
 	struct cmdserveropts opts;
 	initcmdserveropts(&opts);
 	setcmdserveropts(&opts);