threads / patch / 20499

patch, 8 partsUse a clearer style to issue commands to remote helpers

Subject: [PATCH 1/8] Use a clearer style to issue commands to remote helpers

## tl;dr

3 messages between Aug 9, 2009 and Aug 9, 2009. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Daniel Barkalow· Aug 9, 2009, 19:28 UTC · lore

This style is overkill for some commands, but it's worthwhile to use the same style to issue all commands, and it's useful to avoid open-coding string lengths.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---

This is really a clarity improvement for the previous series, but it's here to establish the nicer style before introducing more commands.

 transport-helper.c |   21 ++++++++++++++-------
 1 files changed, 14 insertions(+), 7 deletions(-)

Show changes to transport-helper.c +14 −7

diff --git a/transport-helper.c b/transport-helper.c
index 43fdc0a..cb255e4 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -37,7 +37,10 @@ static struct child_process *get_helper(struct transport *transport)
 		die("Unable to run helper: git %s", helper->argv[0]);
 	data->helper = helper;
 
-	write_in_full(data->helper->in, "capabilities\n", 13);
+	strbuf_addstr(&buf, "capabilities\n");
+	write_in_full(helper->in, buf.buf, buf.len);
+	strbuf_reset(&buf);
+
 	file = fdopen(helper->out, "r");
 	while (1) {
 		if (strbuf_getline(&buf, file, '\n') == EOF)
@@ -78,11 +81,12 @@ static int fetch_with_fetch(struct transport *transport,
 		const struct ref *posn = to_fetch[i];
 		if (posn->status & REF_STATUS_UPTODATE)
 			continue;
-		write_in_full(helper->in, "fetch ", 6);
-		write_in_full(helper->in, sha1_to_hex(posn->old_sha1), 40);
-		write_in_full(helper->in, " ", 1);
-		write_in_full(helper->in, posn->name, strlen(posn->name));
-		write_in_full(helper->in, "\n", 1);
+
+		strbuf_addf(&buf, "fetch %s %s\n",
+			    sha1_to_hex(posn->old_sha1), posn->name);
+		write_in_full(helper->in, buf.buf, buf.len);
+		strbuf_reset(&buf);
+
 		if (strbuf_getline(&buf, file, '\n') == EOF)
 			exit(128); /* child died, message supplied already */
 	}
@@ -119,7 +123,10 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 	FILE *file;
 
 	helper = get_helper(transport);
-	write_in_full(helper->in, "list\n", 5);
+
+	strbuf_addstr(&buf, "list\n");
+	write_in_full(helper->in, buf.buf, buf.len);
+	strbuf_reset(&buf);
 
 	file = fdopen(helper->out, "r");
 	while (1) {

-- 
1.6.4.183.g77eb9.dirty

Johannes Schindelin· Aug 9, 2009, 20:17 UTC · re: Daniel Barkalow · lore

Re: [PATCH 1/8] Use a clearer style to issue commands to remote helpers

Hi,

On Sun, 9 Aug 2009, Daniel Barkalow wrote:

Show 6 quoted lines

> This style is overkill for some commands, but it's worthwhile to use
> the same style to issue all commands, and it's useful to avoid
> open-coding string lengths.
> 
> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> ---

Thanks. I am positively surprised that you did this patch in spite of the basis already being in 'next'.

> +	strbuf_addstr(&buf, "capabilities\n");
> +	write_in_full(helper->in, buf.buf, buf.len);
> +	strbuf_reset(&buf);

Hmm. I think you use the same paradigm three times. Maybe it is time to heed the DRY principle and introduce a helper function?

Maybe something like

void fprintf_in_full(struct strbuf *buf, int fd, const char *format, ...)
{
	va_list ap;

	va_start(ap, format);
	strbuf_reset(buf);
	strbuf_vaddf(buf, format, ap);
	write_in_full(fd, buf->buf, buf->len);
	va_end(ap);
}

But I keep forgetting that there is no strbuf_vaddf() in 'next'... maybe it is high time that I forget my promise and re-submit that series?

Ciao, Dscho

Daniel Barkalow· Aug 9, 2009, 21:14 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/8] Use a clearer style to issue commands to remote helpers

On Sun, 9 Aug 2009, Johannes Schindelin wrote:

Show 22 quoted lines

> > +	strbuf_addstr(&buf, "capabilities\n");
> > +	write_in_full(helper->in, buf.buf, buf.len);
> > +	strbuf_reset(&buf);
> 
> Hmm.  I think you use the same paradigm three times.  Maybe it is time to 
> heed the DRY principle and introduce a helper function?
> 
> Maybe something like
> 
> void fprintf_in_full(struct strbuf *buf, int fd, const char *format, ...)
> {
> 	va_list ap;
> 
> 	va_start(ap, format);
> 	strbuf_reset(buf);
> 	strbuf_vaddf(buf, format, ap);
> 	write_in_full(fd, buf->buf, buf->len);
> 	va_end(ap);
> }
> 
> But I keep forgetting that there is no strbuf_vaddf() in 'next'... maybe 
> it is high time that I forget my promise and re-submit that series?

Yup, that's exactly what I started writing, until I ran into the lack of strbuf_vaddf() and remembered that there were portability issues I don't understand with copying va_lists.

	-Daniel
*This .sig left intentionally blank*