Patchwork [08,of,10] chg: implement validate in hgclient

login
register
mail settings
Submitter Jun Wu
Date March 2, 2016, 10:44 a.m.
Message ID <801df1c31d0c49a19fe3.1456915450@x1c>
Download mbox | patch
Permalink /patch/13531/
State Superseded
Commit a5c773acb01877c12b19a27f508251b334e562d0
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - March 2, 2016, 10:44 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1456913788 0
#      Wed Mar 02 10:16:28 2016 +0000
# Node ID 801df1c31d0c49a19fe3dca0025b393dcc292599
# Parent  7a0eaf66c464e1fcab9b8470811de0d88ef754ff
chg: implement validate in hgclient

This patch implements the corresponding validate method in hgclient.
It will return action strings as is but will not take any real action.
timeless - March 2, 2016, 2:42 p.m.
Jun Wu <quark@fb.com> wrote:
> + * Send command line arguments to let the server decide whether it has the
> + * up-to-date config, extensions and can handle our request.

I can't tell what this means.

you wouldn't normally write `the up-to-date` it would normally be `an
up-to-date` (the implies it would be unique in a certain way, but
presumably there can be a number of concurrent chgservers each with
respectfully up-to-date configurations).

But, I also don't understand `extensions` after it. You might want to
replace `config,` with `config and`, and then `and can` with `and thus
can` (you can choose another word to replace thus, the key is that the
first and is to join a short item list, the second is to join parts of
the sentence).

> + * @return - NULL, the server passed or does not support validation.

`passed` is an odd word, in context, I'd expect it to mean "the server
expressed no interest, i.e. it passed (skipped) the request". To mean
the other form of "passed", I think you want "server validation" or
"server configuration validation" or something.

> + *         - a list of strings, which are the instructions returned by the
> + *           server, the list (not the strings) must be freed by the caller.
> + *           the last string is guaranteed NULL.

guaranteed to be NULL

> +       /* the server returns '\0' if it passed validation */

validation passed (? -- I'm less certain about this form)
Jun Wu - March 2, 2016, 3:08 p.m.
On 03/02/2016 02:42 PM, timeless wrote:
> Jun Wu <quark@fb.com> wrote:
>> + * Send command line arguments to let the server decide whether it has the
>> + * up-to-date config, extensions and can handle our request.
>
> I can't tell what this means.
>
> you wouldn't normally write `the up-to-date` it would normally be `an
> up-to-date` (the implies it would be unique in a certain way, but
> presumably there can be a number of concurrent chgservers each with
> respectfully up-to-date configurations).
>
> But, I also don't understand `extensions` after it. You might want to
> replace `config,` with `config and`, and then `and can` with `and thus
> can` (you can choose another word to replace thus, the key is that the
> first and is to join a short item list, the second is to join parts of
> the sentence).

Thanks for your comments (and other replies to the series).
How about changing to this:

/*!
  * Send command line arguments to let the server load the repo config and check
  * whether it can process our request directly or not.
  * Make sure hgc_setenv is called before calling this.
  *
  * @return - NULL, the server believes it can handle our request, or does not
  *           support "validate" command.
  *         - a list of strings, the server cannot handle our request and it
  *           sent instructions telling us how to fix the issue. See
  *           chgserver.py for possible instruction formats.
  *           the list (not the strings) must be freed by the caller.
  *           the last string is guaranteed to be NULL.
  */

I now think it's hard to explain how the server will do the actual check, so
it may be better to just skip the confusing words "config", "extensions".

>> +       /* the server returns '\0' if it passed validation */
>
> validation passed (? -- I'm less certain about this form)

How about:

/* the server returns '\0' if it can handle our request */
timeless - March 2, 2016, 3:43 p.m.
Jun Wu <quark@fb.com> wrote:
> How about changing to this:
>  *           sent instructions telling us how to fix the issue. See

I'm not a huge fan of `how to fix the issue`, otherwise it's great.

> /* the server returns '\0' if it can handle our request */

sure
Yuya Nishihara - March 3, 2016, 4:07 p.m.
On Wed, 2 Mar 2016 10:44:10 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1456913788 0
> #      Wed Mar 02 10:16:28 2016 +0000
> # Node ID 801df1c31d0c49a19fe3dca0025b393dcc292599
> # Parent  7a0eaf66c464e1fcab9b8470811de0d88ef754ff
> chg: implement validate in hgclient
> 
> This patch implements the corresponding validate method in hgclient.
> It will return action strings as is but will not take any real action.
> 
> diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
> --- a/contrib/chg/hgclient.c
> +++ b/contrib/chg/hgclient.c
> @@ -34,6 +34,7 @@
>  	CAP_GETPAGER = 0x0400,
>  	CAP_SETENV = 0x0800,
>  	CAP_SETUMASK = 0x1000,
> +	CAP_VALIDATE = 0x2000,
>  };
>  
>  typedef struct {
> @@ -49,6 +50,7 @@
>  	{"getpager", CAP_GETPAGER},
>  	{"setenv", CAP_SETENV},
>  	{"setumask", CAP_SETUMASK},
> +	{"validate", CAP_VALIDATE},
>  	{NULL, 0},  /* terminator */
>  };
>  
> @@ -463,6 +465,52 @@
>  }
>  
>  /*!
> + * Send command line arguments to let the server decide whether it has the
> + * up-to-date config, extensions and can handle our request.
> + * @return - NULL, the server passed or does not support validation.
> + *         - a list of strings, which are the instructions returned by the
> + *           server, the list (not the strings) must be freed by the caller.
> + *           the last string is guaranteed NULL.
> + */
> +const char **hgc_validate(hgclient_t *hgc, const char *const args[],
> +			  size_t argsize)

I'm not a fan of "must be freed by the caller" design. Since we know the
list is small, it could be a destination argument and return the actual length.

> +{
> +	assert(hgc);
> +	if (!(hgc->capflags & CAP_VALIDATE))
> +		return NULL;
> +
> +	packcmdargs(&hgc->ctx, args, argsize);
> +	writeblockrequest(hgc, "validate");
> +	readchannel(hgc);
> +
> +	/* the server returns '\0' if it passed validation */
> +	if (hgc->ctx.datasize <= 1)
> +		return NULL;
> +
> +	/* a list of '\0' terminated strings */
> +	size_t n = 1, i;
> +	for (i = 0; i < hgc->ctx.datasize; ++i)
> +		n += (hgc->ctx.data[i] == '\0');
> +
> +	n *= sizeof(char **);
> +	const char **insts = (const char **)mallocx(n);
> +	memset(insts, 0, n);
> +
> +	int waszero = 1;
> +	n = 0;
> +	for (i = 0; i < hgc->ctx.datasize; ++i) {
> +		if (hgc->ctx.data[i] == '\0') {
> +			waszero = 1;
> +		} else {
> +			if (waszero)
> +				insts[n++] = hgc->ctx.data + i;
> +			waszero = 0;
> +		}
> +	}

It would overflow if the data isn't NULL-terminated.
memchr() might be useful.

Patch

diff --git a/contrib/chg/hgclient.c b/contrib/chg/hgclient.c
--- a/contrib/chg/hgclient.c
+++ b/contrib/chg/hgclient.c
@@ -34,6 +34,7 @@ 
 	CAP_GETPAGER = 0x0400,
 	CAP_SETENV = 0x0800,
 	CAP_SETUMASK = 0x1000,
+	CAP_VALIDATE = 0x2000,
 };
 
 typedef struct {
@@ -49,6 +50,7 @@ 
 	{"getpager", CAP_GETPAGER},
 	{"setenv", CAP_SETENV},
 	{"setumask", CAP_SETUMASK},
+	{"validate", CAP_VALIDATE},
 	{NULL, 0},  /* terminator */
 };
 
@@ -463,6 +465,52 @@ 
 }
 
 /*!
+ * Send command line arguments to let the server decide whether it has the
+ * up-to-date config, extensions and can handle our request.
+ * @return - NULL, the server passed or does not support validation.
+ *         - a list of strings, which are the instructions returned by the
+ *           server, the list (not the strings) must be freed by the caller.
+ *           the last string is guaranteed NULL.
+ */
+const char **hgc_validate(hgclient_t *hgc, const char *const args[],
+			  size_t argsize)
+{
+	assert(hgc);
+	if (!(hgc->capflags & CAP_VALIDATE))
+		return NULL;
+
+	packcmdargs(&hgc->ctx, args, argsize);
+	writeblockrequest(hgc, "validate");
+	readchannel(hgc);
+
+	/* the server returns '\0' if it passed validation */
+	if (hgc->ctx.datasize <= 1)
+		return NULL;
+
+	/* a list of '\0' terminated strings */
+	size_t n = 1, i;
+	for (i = 0; i < hgc->ctx.datasize; ++i)
+		n += (hgc->ctx.data[i] == '\0');
+
+	n *= sizeof(char **);
+	const char **insts = (const char **)mallocx(n);
+	memset(insts, 0, n);
+
+	int waszero = 1;
+	n = 0;
+	for (i = 0; i < hgc->ctx.datasize; ++i) {
+		if (hgc->ctx.data[i] == '\0') {
+			waszero = 1;
+		} else {
+			if (waszero)
+				insts[n++] = hgc->ctx.data + i;
+			waszero = 0;
+		}
+	}
+	return insts;
+}
+
+/*!
  * Execute the specified Mercurial command
  *
  * @return result code
diff --git a/contrib/chg/hgclient.h b/contrib/chg/hgclient.h
--- a/contrib/chg/hgclient.h
+++ b/contrib/chg/hgclient.h
@@ -25,5 +25,7 @@ 
 const char *hgc_getpager(hgclient_t *hgc, const char *const args[],
 			 size_t argsize);
 void hgc_setenv(hgclient_t *hgc, const char *const envp[]);
+const char **hgc_validate(hgclient_t *hgc, const char *const args[],
+			  size_t argsize);
 
 #endif  /* HGCLIENT_H_ */