# [PATCH 1/8] Use a clearer style to issue commands to remote helpers 3 messages from 2009-08-09 to 2009-08-09. Participants: Daniel Barkalow, Johannes Schindelin. Thread: https://gitlist.dev/t/20499 ## Daniel Barkalow, 2009-08-09 19:28 Subject: [PATCH 1/8] Use a clearer style to issue commands to remote helpers Message-ID: URL: https://gitlist.dev/e/alpine.LNX.2.00.0908091526350.27553%40iabervon.org ``` 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 --- 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(-) 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, 2009-08-09 20:17 Subject: Re: [PATCH 1/8] Use a clearer style to issue commands to remote helpers Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0908092211030.8306%40pacific.mpi-cbg.de In-Reply-To: ``` Hi, On Sun, 9 Aug 2009, Daniel Barkalow wrote: > 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 > --- 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, 2009-08-09 21:14 Subject: Re: [PATCH 1/8] Use a clearer style to issue commands to remote helpers Message-ID: URL: https://gitlist.dev/e/alpine.LNX.2.00.0908091617310.27553%40iabervon.org In-Reply-To: ``` On Sun, 9 Aug 2009, Johannes Schindelin wrote: > > + 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* ```