{"thread":{"id":"9947","subject":"quote/strbuf series, take 3","startedAt":"2007-09-19T22:42:09Z","lastAt":"2007-09-21T07:39:51Z","messageCount":11,"participants":["Pierre Habouzit","Junio C Hamano","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"53610","messageId":"1190241736-30449-1-git-send-email-madcoder@debian.org","threadId":"9947","inReplyTo":null,"subject":"quote/strbuf series, take 3","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-19T22:42:09Z","receivedAt":"2007-09-19T22:42:09Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"  Here is the take 3. wrt last time: the need for strbuf_addvf has been\navoided. It was not really needed for many places, and I chose ad-hoc\nsolutions for each. It has been done in a new patch (inserted at slot\n2).\n\n  Note that this patch 2/7 looks curious, especially the:\n\n  write_fd_or_whine(..., \"\\n\", 1, ...)\n\n  it is rewritten in a following patch in the series, it's just to have\nan intermediate working state. Don't really mind the bad look of it,\nplease :).\n\n\n  As a consequence, strbuf_addvf is no longer added nor used in the\nseries.\n\n  I just rebased it on the last next (with the new git-fetch).\n"},{"id":"53623","messageId":"7vwsumkll8.fsf@gitster.siamese.dyndns.org","threadId":"9947","inReplyTo":"1190241736-30449-3-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-20T04:27:31Z","receivedAt":"2007-09-20T04:27:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit <madcoder@debian.org> writes:\n\n> * drop nfasprintf.\n> * move nfvasprintf into imap-send.c back, and let it work on a 8k buffer,\n>   and die() in case of overflow. It should be enough for imap commands, if\n>   someone cares about imap-send, he's welcomed to fix it properly.\n> * replace nfvasprintf use in merge-recursive with a copy of the strbuf_addf\n>   logic, it's one place, we'll live with it.\n>   To ease the change, output_buffer string list is replaced with a strbuf ;)\n\nWhile I'd agree with all of the above,\n\n> * rework trace.c API's so that only one of the trace functions takes a\n>   vararg. It's used to format strerror()s and git command names, it should\n>   never be more than a few octets long, let it work on a 8k static buffer\n>   with vsnprintf or die loudly.\n\nand I'd agree with this in principle, there is a minor nit with\nthe implementation and use in trace.c.  E.g.\n\n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index 9b74ed2..c0f954e 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -97,7 +97,8 @@ int execv_git_cmd(const char **argv)\n>  \t\ttmp = argv[0];\n>  \t\targv[0] = git_command;\n>  \n> -\t\ttrace_argv_printf(argv, -1, \"trace: exec:\");\n> +\t\ttrace_printf(\"trace: exec:\");\n> +\t\ttrace_argv(argv, -1);\n\nThis used to be a single call into trace.c which would format a\nsingle string to write(2) out.  Now these two messages go\nthrough separate write(2) and can be broken up.  I think the\natomicity of the log/trace message was the primary reason the\noriginal had such a strange calling convention.\n"},{"id":"53624","messageId":"200709200653.26490.chriscool@tuxfamily.org","threadId":"9947","inReplyTo":"7vwsumkll8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2007-09-20T04:53:26Z","receivedAt":"2007-09-20T04:53:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le jeudi 20 septembre 2007, Junio C Hamano a écrit :\n> > diff --git a/exec_cmd.c b/exec_cmd.c\n> > index 9b74ed2..c0f954e 100644\n> > --- a/exec_cmd.c\n> > +++ b/exec_cmd.c\n> > @@ -97,7 +97,8 @@ int execv_git_cmd(const char **argv)\n> >               tmp = argv[0];\n> >               argv[0] = git_command;\n> >  \n> > -             trace_argv_printf(argv, -1, \"trace: exec:\");\n> > +             trace_printf(\"trace: exec:\");\n> > +             trace_argv(argv, -1);\n>\n> This used to be a single call into trace.c which would format a\n> single string to write(2) out.  Now these two messages go\n> through separate write(2) and can be broken up.  I think the\n> atomicity of the log/trace message was the primary reason the\n> original had such a strange calling convention.\n\nThat's right.\n\nChristian.\n"},{"id":"53632","messageId":"20070920082701.GA2053@artemis.corp","threadId":"9947","inReplyTo":"7vwsumkll8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-20T08:27:01Z","receivedAt":"2007-09-20T08:27:01Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Sep 20, 2007 at 04:27:31AM +0000, Junio C Hamano wrote:\n> Pierre Habouzit <madcoder@debian.org> writes:\n> \n> > * drop nfasprintf.\n> > * move nfvasprintf into imap-send.c back, and let it work on a 8k buffer,\n> >   and die() in case of overflow. It should be enough for imap commands, if\n> >   someone cares about imap-send, he's welcomed to fix it properly.\n> > * replace nfvasprintf use in merge-recursive with a copy of the strbuf_addf\n> >   logic, it's one place, we'll live with it.\n> >   To ease the change, output_buffer string list is replaced with a strbuf ;)\n> \n> While I'd agree with all of the above,\n> \n> > * rework trace.c API's so that only one of the trace functions takes a\n> >   vararg. It's used to format strerror()s and git command names, it should\n> >   never be more than a few octets long, let it work on a 8k static buffer\n> >   with vsnprintf or die loudly.\n> \n> and I'd agree with this in principle, there is a minor nit with\n> the implementation and use in trace.c.  E.g.\n> \n> > diff --git a/exec_cmd.c b/exec_cmd.c\n> > index 9b74ed2..c0f954e 100644\n> > --- a/exec_cmd.c\n> > +++ b/exec_cmd.c\n> > @@ -97,7 +97,8 @@ int execv_git_cmd(const char **argv)\n> >  \t\ttmp = argv[0];\n> >  \t\targv[0] = git_command;\n> >  \n> > -\t\ttrace_argv_printf(argv, -1, \"trace: exec:\");\n> > +\t\ttrace_printf(\"trace: exec:\");\n> > +\t\ttrace_argv(argv, -1);\n> \n> This used to be a single call into trace.c which would format a\n> single string to write(2) out.  Now these two messages go\n> through separate write(2) and can be broken up.  I think the\n> atomicity of the log/trace message was the primary reason the\n> original had such a strange calling convention.\n\n  Okay, given that the formats (as you can see) are always very short,\nand that it will always fit in a big enough static buffer, I'll\nreinstate this and use a vsnprintf twice then.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"53633","messageId":"20070920084311.GB2053@artemis.corp","threadId":"9947","inReplyTo":"20070920082701.GA2053@artemis.corp","subject":"[SUPERSEDES PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-20T08:43:11Z","receivedAt":"2007-09-20T08:43:11Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"* drop nfasprintf.\n* move nfvasprintf into imap-send.c back, and let it work on a 8k buffer,\n  and die() in case of overflow. It should be enough for imap commands, if\n  someone cares about imap-send, he's welcomed to fix it properly.\n* replace nfvasprintf use in merge-recursive with a copy of the strbuf_addf\n  logic, it's one place, we'll live with it.\n  To ease the change, output_buffer string list is replaced with a strbuf ;)\n* rework trace.c to call vsnprintf itself.  It's used to format strerror()s\n  and git command names, it should never be more than a few octets long, let\n  it work on a 8k static buffer with vsnprintf or die loudly.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n\n  This reinstates the trace_argv_printf API. The implementation is\nstupid, but is rewritten in a latter commit. I didn't wanted to bother\noptimizing it.\n\n\n cache.h           |    2 -\n imap-send.c       |   13 ++++++++\n merge-recursive.c |   74 ++++++++++++++++++++-----------------------\n trace.c           |   90 ++++++++++++++++-------------------------------------\n 4 files changed, 74 insertions(+), 105 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c57ccd6..b127c43 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -587,8 +587,6 @@ extern void *alloc_object_node(void);\n extern void alloc_report(void);\n \n /* trace.c */\n-extern int nfasprintf(char **str, const char *fmt, ...);\n-extern int nfvasprintf(char **str, const char *fmt, va_list va);\n extern void trace_printf(const char *format, ...);\n extern void trace_argv_printf(const char **argv, int count, const char *format, ...);\n \ndiff --git a/imap-send.c b/imap-send.c\nindex 905d097..e95cdde 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -105,6 +105,19 @@ static void free_generic_messages( message_t * );\n \n static int nfsnprintf( char *buf, int blen, const char *fmt, ... );\n \n+static int nfvasprintf(char **strp, const char *fmt, va_list ap)\n+{\n+\tint len;\n+\tchar tmp[8192];\n+\n+\tlen = vsnprintf(tmp, sizeof(tmp), fmt, ap);\n+\tif (len < 0)\n+\t\tdie(\"Fatal: Out of memory\\n\");\n+\tif (len >= sizeof(tmp))\n+\t\tdie(\"imap command overflow !\\n\");\n+\t*strp = xmemdupz(tmp, len);\n+\treturn len;\n+}\n \n static void arc4_init( void );\n static unsigned char arc4_getbyte( void );\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 14b56c2..4e27549 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -85,63 +85,57 @@ struct stage_data\n \tunsigned processed:1;\n };\n \n-struct output_buffer\n-{\n-\tstruct output_buffer *next;\n-\tchar *str;\n-};\n-\n static struct path_list current_file_set = {NULL, 0, 0, 1};\n static struct path_list current_directory_set = {NULL, 0, 0, 1};\n \n static int call_depth = 0;\n static int verbosity = 2;\n static int buffer_output = 1;\n-static struct output_buffer *output_list, *output_end;\n+static struct strbuf obuf = STRBUF_INIT;\n \n-static int show (int v)\n+static int show(int v)\n {\n \treturn (!call_depth && verbosity >= v) || verbosity >= 5;\n }\n \n-static void output(int v, const char *fmt, ...)\n+static void flush_output(void)\n {\n-\tva_list args;\n-\tva_start(args, fmt);\n-\tif (buffer_output && show(v)) {\n-\t\tstruct output_buffer *b = xmalloc(sizeof(*b));\n-\t\tnfvasprintf(&b->str, fmt, args);\n-\t\tb->next = NULL;\n-\t\tif (output_end)\n-\t\t\toutput_end->next = b;\n-\t\telse\n-\t\t\toutput_list = b;\n-\t\toutput_end = b;\n-\t} else if (show(v)) {\n-\t\tint i;\n-\t\tfor (i = call_depth; i--;)\n-\t\t\tfputs(\"  \", stdout);\n-\t\tvfprintf(stdout, fmt, args);\n-\t\tfputc('\\n', stdout);\n+\tif (obuf.len) {\n+\t\tfputs(obuf.buf, stdout);\n+\t\tstrbuf_reset(&obuf);\n \t}\n-\tva_end(args);\n }\n \n-static void flush_output(void)\n+static void output(int v, const char *fmt, ...)\n {\n-\tstruct output_buffer *b, *n;\n-\tfor (b = output_list; b; b = n) {\n-\t\tint i;\n-\t\tfor (i = call_depth; i--;)\n-\t\t\tfputs(\"  \", stdout);\n-\t\tfputs(b->str, stdout);\n-\t\tfputc('\\n', stdout);\n-\t\tn = b->next;\n-\t\tfree(b->str);\n-\t\tfree(b);\n+\tif (show(v)) {\n+\t\tint len;\n+\t\tva_list ap;\n+\n+\t\tstrbuf_grow(&obuf, call_depth);\n+\t\tmemset(obuf.buf + obuf.len, ' ', call_depth);\n+\t\tstrbuf_setlen(&obuf, obuf.len + call_depth);\n+\n+\t\tva_start(ap, fmt);\n+\t\tlen = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);\n+\t\tva_end(ap);\n+\n+\t\tif (len < 0)\n+\t\t\tlen = 0;\n+\t\tif (len > strbuf_avail(&obuf)) {\n+\t\t\tstrbuf_grow(&obuf, len);\n+\t\t\tva_start(ap, fmt);\n+\t\t\tlen = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);\n+\t\t\tva_end(ap);\n+\t\t\tif (len > strbuf_avail(&obuf)) {\n+\t\t\t\tdie(\"this should not happen, your snprintf is broken\");\n+\t\t\t}\n+\t\t}\n+\n+\t\tstrbuf_setlen(&obuf, obuf.len + len);\n+\t\tif (!buffer_output)\n+\t\t\tflush_output();\n \t}\n-\toutput_list = NULL;\n-\toutput_end = NULL;\n }\n \n static void output_commit_title(struct commit *commit)\ndiff --git a/trace.c b/trace.c\nindex 7961a27..91548a5 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -25,33 +25,6 @@\n #include \"cache.h\"\n #include \"quote.h\"\n \n-/* Stolen from \"imap-send.c\". */\n-int nfvasprintf(char **strp, const char *fmt, va_list ap)\n-{\n-\tint len;\n-\tchar tmp[1024];\n-\n-\tif ((len = vsnprintf(tmp, sizeof(tmp), fmt, ap)) < 0 ||\n-\t    !(*strp = xmalloc(len + 1)))\n-\t\tdie(\"Fatal: Out of memory\\n\");\n-\tif (len >= (int)sizeof(tmp))\n-\t\tvsprintf(*strp, fmt, ap);\n-\telse\n-\t\tmemcpy(*strp, tmp, len + 1);\n-\treturn len;\n-}\n-\n-int nfasprintf(char **str, const char *fmt, ...)\n-{\n-\tint rc;\n-\tva_list args;\n-\n-\tva_start(args, fmt);\n-\trc = nfvasprintf(str, fmt, args);\n-\tva_end(args);\n-\treturn rc;\n-}\n-\n /* Get a trace file descriptor from GIT_TRACE env variable. */\n static int get_trace_fd(int *need_close)\n {\n@@ -89,63 +62,54 @@ static int get_trace_fd(int *need_close)\n static const char err_msg[] = \"Could not trace into fd given by \"\n \t\"GIT_TRACE environment variable\";\n \n-void trace_printf(const char *format, ...)\n+void trace_printf(const char *fmt, ...)\n {\n-\tchar *trace_str;\n-\tva_list rest;\n-\tint need_close = 0;\n-\tint fd = get_trace_fd(&need_close);\n+\tchar buf[8192];\n+\tva_list ap;\n+\tint fd, len, need_close = 0;\n \n+\tfd = get_trace_fd(&need_close);\n \tif (!fd)\n \t\treturn;\n \n-\tva_start(rest, format);\n-\tnfvasprintf(&trace_str, format, rest);\n-\tva_end(rest);\n-\n-\twrite_or_whine_pipe(fd, trace_str, strlen(trace_str), err_msg);\n-\n-\tfree(trace_str);\n+\tva_start(ap, fmt);\n+\tlen = vsnprintf(buf, sizeof(buf), fmt, ap);\n+\tva_end(ap);\n+\tif (len >= sizeof(buf))\n+\t\tdie(\"unreasonnable trace length\");\n+\twrite_or_whine_pipe(fd, buf, len, err_msg);\n \n \tif (need_close)\n \t\tclose(fd);\n }\n \n-void trace_argv_printf(const char **argv, int count, const char *format, ...)\n+void trace_argv_printf(const char **argv, int count, const char *fmt, ...)\n {\n-\tchar *argv_str, *format_str, *trace_str;\n-\tsize_t argv_len, format_len, trace_len;\n-\tva_list rest;\n-\tint need_close = 0;\n-\tint fd = get_trace_fd(&need_close);\n+\tchar buf[8192];\n+\tva_list ap;\n+\tchar *argv_str;\n+\tsize_t argv_len;\n+\tint fd, len, need_close = 0;\n \n+\tfd = get_trace_fd(&need_close);\n \tif (!fd)\n \t\treturn;\n \n+\tva_start(ap, fmt);\n+\tlen = vsnprintf(buf, sizeof(buf), fmt, ap);\n+\tva_end(ap);\n+\tif (len >= sizeof(buf))\n+\t\tdie(\"unreasonnable trace length\");\n+\n \t/* Get the argv string. */\n \targv_str = sq_quote_argv(argv, count);\n \targv_len = strlen(argv_str);\n \n-\t/* Get the formated string. */\n-\tva_start(rest, format);\n-\tnfvasprintf(&format_str, format, rest);\n-\tva_end(rest);\n-\n-\t/* Allocate buffer for trace string. */\n-\tformat_len = strlen(format_str);\n-\ttrace_len = argv_len + format_len + 1; /* + 1 for \\n */\n-\ttrace_str = xmalloc(trace_len + 1);\n-\n-\t/* Copy everything into the trace string. */\n-\tstrncpy(trace_str, format_str, format_len);\n-\tstrncpy(trace_str + format_len, argv_str, argv_len);\n-\tstrcpy(trace_str + trace_len - 1, \"\\n\");\n-\n-\twrite_or_whine_pipe(fd, trace_str, trace_len, err_msg);\n+\twrite_or_whine_pipe(fd, buf, len, err_msg);\n+\twrite_or_whine_pipe(fd, argv_str, argv_len, err_msg);\n+\twrite_or_whine_pipe(fd, \"\\n\", 1, err_msg);\n \n \tfree(argv_str);\n-\tfree(format_str);\n-\tfree(trace_str);\n \n \tif (need_close)\n \t\tclose(fd);\n-- \n1.5.3.2.1036.gca8d2\n"},{"id":"53634","messageId":"20070920084441.GC2053@artemis.corp","threadId":"9947","inReplyTo":"20070920082701.GA2053@artemis.corp","subject":"[SUPERSEDES PATCH 4/7] sq_quote_argv and add_to_string rework with strbuf's.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-20T08:44:41Z","receivedAt":"2007-09-20T08:44:41Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"* sq_quote_buf is made public, and works on a strbuf.\n* sq_quote_argv also works on a strbuf.\n* make sq_quote_argv take a \"maxlen\" argument to check the buffer won't grow\n  too big.\n\nSigned-off-by: Pierre Habouzit <madcoder@debian.org>\n---\n connect.c |   21 ++++++--------\n git.c     |   16 +++-------\n quote.c   |   94 +++++++++++++++---------------------------------------------\n quote.h   |    9 ++----\n trace.c   |   25 +++++++---------\n 5 files changed, 52 insertions(+), 113 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 1653a0e..06d279e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -577,16 +577,13 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n \tif (pid < 0)\n \t\tdie(\"unable to fork\");\n \tif (!pid) {\n-\t\tchar command[MAX_CMD_LEN];\n-\t\tchar *posn = command;\n-\t\tint size = MAX_CMD_LEN;\n-\t\tint of = 0;\n+\t\tstruct strbuf cmd;\n \n-\t\tof |= add_to_string(&posn, &size, prog, 0);\n-\t\tof |= add_to_string(&posn, &size, \" \", 0);\n-\t\tof |= add_to_string(&posn, &size, path, 1);\n-\n-\t\tif (of)\n+\t\tstrbuf_init(&cmd, MAX_CMD_LEN);\n+\t\tstrbuf_addstr(&cmd, prog);\n+\t\tstrbuf_addch(&cmd, ' ');\n+\t\tsq_quote_buf(&cmd, path);\n+\t\tif (cmd.len >= MAX_CMD_LEN)\n \t\t\tdie(\"command line too long\");\n \n \t\tdup2(pipefd[1][0], 0);\n@@ -606,10 +603,10 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n \t\t\t\tssh_basename++;\n \n \t\t\tif (!port)\n-\t\t\t\texeclp(ssh, ssh_basename, host, command, NULL);\n+\t\t\t\texeclp(ssh, ssh_basename, host, cmd.buf, NULL);\n \t\t\telse\n \t\t\t\texeclp(ssh, ssh_basename, \"-p\", port, host,\n-\t\t\t\t       command, NULL);\n+\t\t\t\t       cmd.buf, NULL);\n \t\t}\n \t\telse {\n \t\t\tunsetenv(ALTERNATE_DB_ENVIRONMENT);\n@@ -618,7 +615,7 @@ pid_t git_connect(int fd[2], char *url, const char *prog, int flags)\n \t\t\tunsetenv(GIT_WORK_TREE_ENVIRONMENT);\n \t\t\tunsetenv(GRAFT_ENVIRONMENT);\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n-\t\t\texeclp(\"sh\", \"sh\", \"-c\", command, NULL);\n+\t\t\texeclp(\"sh\", \"sh\", \"-c\", cmd.buf, NULL);\n \t\t}\n \t\tdie(\"exec failed\");\n \t}\ndiff --git a/git.c b/git.c\nindex 6773c12..d7c6bca 100644\n--- a/git.c\n+++ b/git.c\n@@ -187,19 +187,13 @@ static int handle_alias(int *argcp, const char ***argv)\n \tif (alias_string) {\n \t\tif (alias_string[0] == '!') {\n \t\t\tif (*argcp > 1) {\n-\t\t\t\tint i, sz = PATH_MAX;\n-\t\t\t\tchar *s = xmalloc(sz), *new_alias = s;\n+\t\t\t\tstruct strbuf buf;\n \n-\t\t\t\tadd_to_string(&s, &sz, alias_string, 0);\n+\t\t\t\tstrbuf_init(&buf, PATH_MAX);\n+\t\t\t\tstrbuf_addstr(&buf, alias_string);\n+\t\t\t\tsq_quote_argv(&buf, (*argv) + 1, *argcp - 1, PATH_MAX);\n \t\t\t\tfree(alias_string);\n-\t\t\t\talias_string = new_alias;\n-\t\t\t\tfor (i = 1; i < *argcp &&\n-\t\t\t\t\t!add_to_string(&s, &sz, \" \", 0) &&\n-\t\t\t\t\t!add_to_string(&s, &sz, (*argv)[i], 1)\n-\t\t\t\t\t; i++)\n-\t\t\t\t\t; /* do nothing */\n-\t\t\t\tif (!sz)\n-\t\t\t\t\tdie(\"Too many or long arguments\");\n+\t\t\t\talias_string = buf.buf;\n \t\t\t}\n \t\t\ttrace_printf(\"trace: alias to shell cmd: %s => %s\\n\",\n \t\t\t\t     alias_command, alias_string + 1);\ndiff --git a/quote.c b/quote.c\nindex d88bf75..edd1a09 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -12,37 +12,31 @@\n  *  a'b      ==> a'\\''b    ==> 'a'\\''b'\n  *  a!b      ==> a'\\!'b    ==> 'a'\\!'b'\n  */\n-#undef EMIT\n-#define EMIT(x) do { if (++len < n) *bp++ = (x); } while(0)\n-\n static inline int need_bs_quote(char c)\n {\n \treturn (c == '\\'' || c == '!');\n }\n \n-static size_t sq_quote_buf(char *dst, size_t n, const char *src)\n+void sq_quote_buf(struct strbuf *dst, const char *src)\n {\n-\tchar c;\n-\tchar *bp = dst;\n-\tsize_t len = 0;\n-\n-\tEMIT('\\'');\n-\twhile ((c = *src++)) {\n-\t\tif (need_bs_quote(c)) {\n-\t\t\tEMIT('\\'');\n-\t\t\tEMIT('\\\\');\n-\t\t\tEMIT(c);\n-\t\t\tEMIT('\\'');\n-\t\t} else {\n-\t\t\tEMIT(c);\n+\tchar *to_free = NULL;\n+\n+\tif (dst->buf == src)\n+\t\tto_free = strbuf_detach(dst);\n+\n+\tstrbuf_addch(dst, '\\'');\n+\twhile (*src) {\n+\t\tsize_t len = strcspn(src, \"'\\\\\");\n+\t\tstrbuf_add(dst, src, len);\n+\t\tsrc += len;\n+\t\twhile (need_bs_quote(*src)) {\n+\t\t\tstrbuf_addstr(dst, \"'\\\\\");\n+\t\t\tstrbuf_addch(dst, *src++);\n+\t\t\tstrbuf_addch(dst, '\\'');\n \t\t}\n \t}\n-\tEMIT('\\'');\n-\n-\tif ( n )\n-\t\t*bp = 0;\n-\n-\treturn len;\n+\tstrbuf_addch(dst, '\\'');\n+\tfree(to_free);\n }\n \n void sq_quote_print(FILE *stream, const char *src)\n@@ -62,11 +56,10 @@ void sq_quote_print(FILE *stream, const char *src)\n \tfputc('\\'', stream);\n }\n \n-char *sq_quote_argv(const char** argv, int count)\n+void sq_quote_argv(struct strbuf *dst, const char** argv, int count,\n+                   size_t maxlen)\n {\n-\tchar *buf, *to;\n \tint i;\n-\tsize_t len = 0;\n \n \t/* Count argv if needed. */\n \tif (count < 0) {\n@@ -74,53 +67,14 @@ char *sq_quote_argv(const char** argv, int count)\n \t\t\t; /* just counting */\n \t}\n \n-\t/* Special case: no argv. */\n-\tif (!count)\n-\t\treturn xcalloc(1,1);\n-\n-\t/* Get destination buffer length. */\n-\tfor (i = 0; i < count; i++)\n-\t\tlen += sq_quote_buf(NULL, 0, argv[i]) + 1;\n-\n-\t/* Alloc destination buffer. */\n-\tto = buf = xmalloc(len + 1);\n-\n \t/* Copy into destination buffer. */\n+\tstrbuf_grow(dst, 32 * count);\n \tfor (i = 0; i < count; ++i) {\n-\t\t*to++ = ' ';\n-\t\tto += sq_quote_buf(to, len, argv[i]);\n+\t\tstrbuf_addch(dst, ' ');\n+\t\tsq_quote_buf(dst, argv[i]);\n+\t\tif (maxlen && dst->len > maxlen)\n+\t\t\tdie(\"Too many or long arguments\");\n \t}\n-\n-\treturn buf;\n-}\n-\n-/*\n- * Append a string to a string buffer, with or without shell quoting.\n- * Return true if the buffer overflowed.\n- */\n-int add_to_string(char **ptrp, int *sizep, const char *str, int quote)\n-{\n-\tchar *p = *ptrp;\n-\tint size = *sizep;\n-\tint oc;\n-\tint err = 0;\n-\n-\tif (quote)\n-\t\toc = sq_quote_buf(p, size, str);\n-\telse {\n-\t\toc = strlen(str);\n-\t\tmemcpy(p, str, (size <= oc) ? size - 1 : oc);\n-\t}\n-\n-\tif (size <= oc) {\n-\t\terr = 1;\n-\t\toc = size - 1;\n-\t}\n-\n-\t*ptrp += oc;\n-\t**ptrp = '\\0';\n-\t*sizep -= oc;\n-\treturn err;\n }\n \n char *sq_dequote(char *arg)\ndiff --git a/quote.h b/quote.h\nindex 8a59cc5..78e8d3e 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -29,13 +29,10 @@\n  */\n \n extern void sq_quote_print(FILE *stream, const char *src);\n-extern char *sq_quote_argv(const char** argv, int count);\n \n-/*\n- * Append a string to a string buffer, with or without shell quoting.\n- * Return true if the buffer overflowed.\n- */\n-extern int add_to_string(char **ptrp, int *sizep, const char *str, int quote);\n+extern void sq_quote_buf(struct strbuf *, const char *src);\n+extern void sq_quote_argv(struct strbuf *, const char **argv, int count,\n+                          size_t maxlen);\n \n /* This unwraps what sq_quote() produces in place, but returns\n  * NULL if the input does not look like what sq_quote would have\ndiff --git a/trace.c b/trace.c\nindex 91548a5..efc656b 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -85,32 +85,29 @@ void trace_printf(const char *fmt, ...)\n \n void trace_argv_printf(const char **argv, int count, const char *fmt, ...)\n {\n-\tchar buf[8192];\n \tva_list ap;\n-\tchar *argv_str;\n-\tsize_t argv_len;\n+\tstruct strbuf trace;\n \tint fd, len, need_close = 0;\n \n \tfd = get_trace_fd(&need_close);\n \tif (!fd)\n \t\treturn;\n \n+\tstrbuf_init(&trace, 0);\n+\tstrbuf_grow(&trace, 8192);\n+\n \tva_start(ap, fmt);\n-\tlen = vsnprintf(buf, sizeof(buf), fmt, ap);\n+\tlen = vsnprintf(trace.buf, strbuf_avail(&trace) + 1, fmt, ap);\n \tva_end(ap);\n-\tif (len >= sizeof(buf))\n+\tif (len >= strbuf_avail(&trace))\n \t\tdie(\"unreasonnable trace length\");\n+\tstrbuf_setlen(&trace, len);\n \n-\t/* Get the argv string. */\n-\targv_str = sq_quote_argv(argv, count);\n-\targv_len = strlen(argv_str);\n-\n-\twrite_or_whine_pipe(fd, buf, len, err_msg);\n-\twrite_or_whine_pipe(fd, argv_str, argv_len, err_msg);\n-\twrite_or_whine_pipe(fd, \"\\n\", 1, err_msg);\n-\n-\tfree(argv_str);\n+\tsq_quote_argv(&trace, argv, count, 0);\n+\tstrbuf_addch(&trace, '\\n');\n+\twrite_or_whine_pipe(fd, trace.buf, trace.len, err_msg);\n \n+\tstrbuf_release(&trace);\n \tif (need_close)\n \t\tclose(fd);\n }\n-- \n1.5.3.2.1036.gca8d2\n\n"},{"id":"53684","messageId":"20070920220506.GC17514@artemis.corp","threadId":"9947","inReplyTo":"1190241736-30449-8-git-send-email-madcoder@debian.org","subject":"Re: [PATCH 7/7] Avoid duplicating memory, and use xmemdupz instead of xstrdup.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-20T22:05:06Z","receivedAt":"2007-09-20T22:05:06Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On mer, sep 19, 2007 at 10:42:16 +0000, Pierre Habouzit wrote:\n> Signed-off-by: Pierre Habouzit <madcoder@debian.org>\n> ---\n>  walker.c |   23 +++++++++--------------\n>  1 files changed, 9 insertions(+), 14 deletions(-)\n> \n> diff --git a/walker.c b/walker.c\n> index 397b80d..0abdd64 100644\n> --- a/walker.c\n> +++ b/walker.c\n> @@ -213,24 +213,19 @@ int walker_targets_stdin(char ***target, const char ***write_ref)\n>  \tstruct strbuf buf;\n>  \t*target = NULL; *write_ref = NULL;\n>  \tstrbuf_init(&buf, 0);\n> -\twhile (1) {\n> -\t\tchar *rf_one = NULL;\n> -\t\tchar *tg_one;\n> -\n> -\t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF)\n> -\t\t\tbreak;\n> -\t\ttg_one = buf.buf;\n> -\t\trf_one = strchr(tg_one, '\\t');\n> -\t\tif (rf_one)\n> -\t\t\t*rf_one++ = 0;\n> +\twhile (strbuf_getline(&buf, stdin, '\\n') != EOF) {\n> +\t\tchar *rf_one = memchr(buf.buf, '\\t', buf.len);\n>  \n>  \t\tif (targets >= targets_alloc) {\n> -\t\t\ttargets_alloc = targets_alloc ? targets_alloc * 2 : 64;\n> -\t\t\t*target = xrealloc(*target, targets_alloc * sizeof(**target));\n> +\t\t\tALLOC_GROW(target, targets, targets_alloc);\n>  \t\t\t*write_ref = xrealloc(*write_ref, targets_alloc * sizeof(**write_ref));\n>  \t\t}\n> -\t\t(*target)[targets] = xstrdup(tg_one);\n> -\t\t(*write_ref)[targets] = rf_one ? xstrdup(rf_one) : NULL;\n> +\t\tif (rf_one) {\n> +\t\t\t(*write_ref)[targets] = xmemdupz(rf_one, buf.len - (rf_one - buf.buf));\n> +\t\t} else {\n\n  As someone pointed to me off-list the above should be:\n  +\t\tif (rf_one) {\n  +\t\t\t(*write_ref)[targets] = xmemdupz(rf_one + 1, buf.len - (rf_one + 1 - buf.buf));\n  +\t\t} else {\n\n  Or better:\n  +\t\tif (rf_one) {\n  +\t\t\trf_one++; /* skip \\t */\n  +\t\t\t(*write_ref)[targets] = xmemdupz(rf_one, buf.buf + buf.len - rf_one);\n  +\t\t} else {\n\n  Which is definitely more readable.\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"53697","messageId":"7vejgsk0ev.fsf@gitster.siamese.dyndns.org","threadId":"9947","inReplyTo":"20070920084311.GB2053@artemis.corp","subject":"Re: [SUPERSEDES PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-21T06:17:12Z","receivedAt":"2007-09-21T06:17:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit <madcoder@debian.org> writes:\n\n> This reinstates the trace_argv_printf API. The implementation is\n> stupid, but is rewritten in a latter commit. I didn't wanted to bother\n> optimizing it.\n> ...\n>  cache.h           |    2 -\n>  imap-send.c       |   13 ++++++++\n>  merge-recursive.c |   74 ++++++++++++++++++++-----------------------\n>  trace.c           |   90 ++++++++++++++++-------------------------------------\n>  4 files changed, 74 insertions(+), 105 deletions(-)\n> ...\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 14b56c2..4e27549 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -85,63 +85,57 @@ struct stage_data\n> +static void flush_output(void)\n>  {\n> +\tif (obuf.len) {\n> +\t\tfputs(obuf.buf, stdout);\n> +\t\tstrbuf_reset(&obuf);\n>  \t}\n>  }\n\nThis assumes obuf.buf has necessary indentations and line\nbreaks, which is sensible.  However...\n\n> +static void output(int v, const char *fmt, ...)\n>  {\n> +\tif (show(v)) {\n> +\t\tint len;\n> +\t\tva_list ap;\n\nYuck, this single if statement covers the entirety of the\nfunction.  Let's do\n\n\tif (!show(v))\n\t        return;\n\n> +\t\tstrbuf_grow(&obuf, call_depth);\n> +\t\tmemset(obuf.buf + obuf.len, ' ', call_depth);\n> +\t\tstrbuf_setlen(&obuf, obuf.len + call_depth);\n\nPer depth indentation used to be two whitespaces.\n\n> +\t\tva_start(ap, fmt);\n> +\t\tlen = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);\n> +\t\tva_end(ap);\n\nAnd you overwrite whatever used to be in the buffer, including\nthe previous buffered message and indentation you added.  Not\nnice...\n\nI'll squash this on top of yours for now.\n\n merge-recursive.c |   45 +++++++++++++++++++++++----------------------\n 1 files changed, 23 insertions(+), 22 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 4e27549..86767e6 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -108,34 +108,35 @@ static void flush_output(void)\n \n static void output(int v, const char *fmt, ...)\n {\n-\tif (show(v)) {\n-\t\tint len;\n-\t\tva_list ap;\n+\tint len;\n+\tva_list ap;\n \n-\t\tstrbuf_grow(&obuf, call_depth);\n-\t\tmemset(obuf.buf + obuf.len, ' ', call_depth);\n-\t\tstrbuf_setlen(&obuf, obuf.len + call_depth);\n+\tif (!show(v))\n+\t\treturn;\n+\n+\tstrbuf_grow(&obuf, call_depth * 2 + 2);\n+\tmemset(obuf.buf + obuf.len, ' ', call_depth * 2);\n+\tstrbuf_setlen(&obuf, obuf.len + call_depth * 2);\n+\n+\tva_start(ap, fmt);\n+\tlen = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap);\n+\tva_end(ap);\n \n+\tif (len < 0)\n+\t\tlen = 0;\n+\tif (len >= strbuf_avail(&obuf)) {\n+\t\tstrbuf_grow(&obuf, len + 2);\n \t\tva_start(ap, fmt);\n-\t\tlen = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);\n+\t\tlen = vsnprintf(obuf.buf + obuf.len, strbuf_avail(&obuf), fmt, ap);\n \t\tva_end(ap);\n-\n-\t\tif (len < 0)\n-\t\t\tlen = 0;\n-\t\tif (len > strbuf_avail(&obuf)) {\n-\t\t\tstrbuf_grow(&obuf, len);\n-\t\t\tva_start(ap, fmt);\n-\t\t\tlen = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);\n-\t\t\tva_end(ap);\n-\t\t\tif (len > strbuf_avail(&obuf)) {\n-\t\t\t\tdie(\"this should not happen, your snprintf is broken\");\n-\t\t\t}\n+\t\tif (len >= strbuf_avail(&obuf)) {\n+\t\t\tdie(\"this should not happen, your snprintf is broken\");\n \t\t}\n-\n-\t\tstrbuf_setlen(&obuf, obuf.len + len);\n-\t\tif (!buffer_output)\n-\t\t\tflush_output();\n \t}\n+\tstrbuf_setlen(&obuf, obuf.len + len);\n+\tstrbuf_add(&obuf, \"\\n\", 1);\n+\tif (!buffer_output)\n+\t\tflush_output();\n }\n \n static void output_commit_title(struct commit *commit)\n"},{"id":"53700","messageId":"20070921070215.GA5689@artemis.corp","threadId":"9947","inReplyTo":"7vejgsk0ev.fsf@gitster.siamese.dyndns.org","subject":"Re: [SUPERSEDES PATCH 2/7] nfv?asprintf are broken without va_copy, workaround them.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-21T07:02:15Z","receivedAt":"2007-09-21T07:02:15Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Fri, Sep 21, 2007 at 06:17:12AM +0000, Junio C Hamano wrote:\n> Pierre Habouzit <madcoder@debian.org> writes:\n> \n> > This reinstates the trace_argv_printf API. The implementation is\n> > stupid, but is rewritten in a latter commit. I didn't wanted to bother\n> > optimizing it.\n> > ...\n> >  cache.h           |    2 -\n> >  imap-send.c       |   13 ++++++++\n> >  merge-recursive.c |   74 ++++++++++++++++++++-----------------------\n> >  trace.c           |   90 ++++++++++++++++-------------------------------------\n> >  4 files changed, 74 insertions(+), 105 deletions(-)\n> > ...\n> > diff --git a/merge-recursive.c b/merge-recursive.c\n> > index 14b56c2..4e27549 100644\n> > --- a/merge-recursive.c\n> > +++ b/merge-recursive.c\n> > @@ -85,63 +85,57 @@ struct stage_data\n> > +static void flush_output(void)\n> >  {\n> > +\tif (obuf.len) {\n> > +\t\tfputs(obuf.buf, stdout);\n> > +\t\tstrbuf_reset(&obuf);\n> >  \t}\n> >  }\n> \n> This assumes obuf.buf has necessary indentations and line\n> breaks, which is sensible.  However...\n> \n> > +static void output(int v, const char *fmt, ...)\n> >  {\n> > +\tif (show(v)) {\n> > +\t\tint len;\n> > +\t\tva_list ap;\n> \n> Yuck, this single if statement covers the entirety of the\n> function.  Let's do\n> \n> \tif (!show(v))\n> \t        return;\n> \n> > +\t\tstrbuf_grow(&obuf, call_depth);\n> > +\t\tmemset(obuf.buf + obuf.len, ' ', call_depth);\n> > +\t\tstrbuf_setlen(&obuf, obuf.len + call_depth);\n> \n> Per depth indentation used to be two whitespaces.\n> \n> > +\t\tva_start(ap, fmt);\n> > +\t\tlen = vsnprintf(obuf.buf, strbuf_avail(&obuf) + 1, fmt, ap);\n> > +\t\tva_end(ap);\n> \n> And you overwrite whatever used to be in the buffer, including\n> the previous buffered message and indentation you added.  Not\n> nice...\n\n  ooops, I wrote it too quickly.\n\n> I'll squash this on top of yours for now.\n\n  works for me except the little remark in the end :)\n\n>  merge-recursive.c |   45 +++++++++++++++++++++++----------------------\n>  1 files changed, 23 insertions(+), 22 deletions(-)\n> \n\n> +\tstrbuf_setlen(&obuf, obuf.len + len);\n> +\tstrbuf_add(&obuf, \"\\n\", 1);\n\n  rather use strbuf_addch(&obuf, '\\n')\n\n> +\tif (!buffer_output)\n> +\t\tflush_output();\n>  }\n>  \n>  static void output_commit_title(struct commit *commit)\n> -\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"53701","messageId":"20070921070329.GB5689@artemis.corp","threadId":"9947","inReplyTo":"20070920220506.GC17514@artemis.corp","subject":"Re: [PATCH 7/7] Avoid duplicating memory, and use xmemdupz instead of xstrdup.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-21T07:03:29Z","receivedAt":"2007-09-21T07:03:29Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On jeu, sep 20, 2007 at 10:05:06 +0000, Pierre Habouzit wrote:\n> On mer, sep 19, 2007 at 10:42:16 +0000, Pierre Habouzit wrote:\n> \n>   As someone pointed to me off-list the above should be:\n>   +\t\tif (rf_one) {\n>   +\t\t\t(*write_ref)[targets] = xmemdupz(rf_one + 1, buf.len - (rf_one + 1 - buf.buf));\n>   +\t\t} else {\n> \n>   Or better:\n>   +\t\tif (rf_one) {\n>   +\t\t\trf_one++; /* skip \\t */\n>   +\t\t\t(*write_ref)[targets] = xmemdupz(rf_one, buf.buf + buf.len - rf_one);\n>   +\t\t} else {\n> \n>   Which is definitely more readable.\n\n  damn it was not the error that was reported to me, there is another\none, I'll roll a new patch, sorry :/\n\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"},{"id":"53702","messageId":"20070921073951.GB14008@artemis.corp","threadId":"9947","inReplyTo":"20070921070329.GB5689@artemis.corp","subject":"[DON'T MERGE PATCH 7/7]","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-21T07:39:51Z","receivedAt":"2007-09-21T07:39:51Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Fri, Sep 21, 2007 at 07:03:29AM +0000, Pierre Habouzit wrote:\n> On jeu, sep 20, 2007 at 10:05:06 +0000, Pierre Habouzit wrote:\n> > On mer, sep 19, 2007 at 10:42:16 +0000, Pierre Habouzit wrote:\n> > \n> >   As someone pointed to me off-list the above should be:\n> >   +\t\tif (rf_one) {\n> >   +\t\t\t(*write_ref)[targets] = xmemdupz(rf_one + 1, buf.len - (rf_one + 1 - buf.buf));\n> >   +\t\t} else {\n> > \n> >   Or better:\n> >   +\t\tif (rf_one) {\n> >   +\t\t\trf_one++; /* skip \\t */\n> >   +\t\t\t(*write_ref)[targets] = xmemdupz(rf_one, buf.buf + buf.len - rf_one);\n> >   +\t\t} else {\n> > \n> >   Which is definitely more readable.\n> \n>   damn it was not the error that was reported to me, there is another\n> one, I'll roll a new patch, sorry :/\n\n  Okay this patch is worthless, the previous version worked the same, I\ntotally missed what it did. I should not code when I'm too tired, I'm\nsorry.\n\n  Don't merge patch 7/7 it's broken.\n\n-- \n·O·  Pierre Habouzit\n··O                                                madcoder@debian.org\nOOO                                                http://www.madism.org\n"}]}