{"thread":{"id":"5372","subject":"[PATCH] Refactoring tracing code in \"git.c\" and \"exec_cmd.c\".","startedAt":"2006-08-24T05:45:47Z","lastAt":"2006-08-27T19:38:54Z","messageCount":3,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"25831","messageId":"20060824074547.a8fa0005.chriscool@tuxfamily.org","threadId":"5372","inReplyTo":null,"subject":"[PATCH] Refactoring tracing code in \"git.c\" and \"exec_cmd.c\".","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-08-24T05:45:47Z","receivedAt":"2006-08-24T05:45:47Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Some new helper functions in \"quote.c\" are used for this.\nThe goal is also to get near the point where we can use\none write(2) call to trace in any open file descriptor.\nThis is why we put the trace string into one buffer.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n exec_cmd.c |   10 ++------\n git.c      |   20 ++++-----------\n quote.c    |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n quote.h    |    3 ++\n 4 files changed, 88 insertions(+), 23 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex e30936d..5688e0b 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -98,14 +98,8 @@ int execv_git_cmd(const char **argv)\n \t\targv[0] = git_command;\n \n \t\tif (getenv(\"GIT_TRACE\")) {\n-\t\t\tconst char **p = argv;\n-\t\t\tfputs(\"trace: exec:\", stderr);\n-\t\t\twhile (*p) {\n-\t\t\t\tfputc(' ', stderr);\n-\t\t\t\tsq_quote_print(stderr, *p);\n-\t\t\t\t++p;\n-\t\t\t}\n-\t\t\tputc('\\n', stderr);\n+\t\t\tsq_quote_argv_printf(stderr, argv, -1,\n+\t\t\t\t\t     \"trace: exec: ARGV\\n\");\n \t\t\tfflush(stderr);\n \t\t}\n \ndiff --git a/git.c b/git.c\nindex 930998b..d13b841 100644\n--- a/git.c\n+++ b/git.c\n@@ -180,14 +180,9 @@ static int handle_alias(int *argcp, cons\n \t\t\tdie(\"recursive alias: %s\", alias_command);\n \n \t\tif (getenv(\"GIT_TRACE\")) {\n-\t\t\tint i;\n-\t\t\tfprintf(stderr, \"trace: alias expansion: %s =>\",\n-\t\t\t\talias_command);\n-\t\t\tfor (i = 0; i < count; ++i) {\n-\t\t\t\tfputc(' ', stderr);\n-\t\t\t\tsq_quote_print(stderr, new_argv[i]);\n-\t\t\t}\n-\t\t\tfputc('\\n', stderr);\n+\t\t\tsq_quote_argv_printf(stderr, new_argv, count,\n+\t\t\t\t\t     \"trace: alias expansion: %s => ARGV\\n\",\n+\t\t\t\t\t     alias_command);\n \t\t\tfflush(stderr);\n \t\t}\n \n@@ -292,13 +287,8 @@ static void handle_internal_command(int \n \t\tif (p->option & USE_PAGER)\n \t\t\tsetup_pager();\n \t\tif (getenv(\"GIT_TRACE\")) {\n-\t\t\tint i;\n-\t\t\tfprintf(stderr, \"trace: built-in: git\");\n-\t\t\tfor (i = 0; i < argc; ++i) {\n-\t\t\t\tfputc(' ', stderr);\n-\t\t\t\tsq_quote_print(stderr, argv[i]);\n-\t\t\t}\n-\t\t\tputc('\\n', stderr);\n+\t\t\tsq_quote_argv_printf(stderr, argv, argc,\n+\t\t\t\t\t     \"trace: built-in: git ARGV\\n\");\n \t\t\tfflush(stderr);\n \t\t}\n \ndiff --git a/quote.c b/quote.c\nindex e220dcc..84d0b7b 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -74,6 +74,84 @@ char *sq_quote(const char *src)\n \treturn buf;\n }\n \n+char *sq_quote_argv(const char** argv, int count)\n+{\n+\tchar *buf, *to;\n+\tint i;\n+\tsize_t len;\n+\n+\t/* Count argv if needed. */\n+\tif (count < 0) {\n+\t\tchar **p = (char **)argv;\n+\t\tcount = 0;\n+\t\twhile (*p++) count++;\n+\t}\n+\n+\t/* Get destination buffer length. */\n+\tlen = count ? count : 1;\n+\tfor (i = 0; i < count; ++i)\n+\t\tlen += sq_quote_buf(NULL, 0, argv[i]);\n+\n+\t/* Alloc destination buffer. */\n+\tto = buf = xmalloc(len);\n+\n+\t/* Copy into destination buffer. */\n+\tfor (i = 0; i < count; ++i) {\n+\t\tif (i) *to++ = ' ';\n+\t\tto += sq_quote_buf(to, len, argv[i]);\n+\t}\n+\n+\tif (!count)\n+\t\t*buf = 0;\n+\t\n+\treturn buf;\n+}\n+\n+/* Return a newly allocated copy of \"format\" where the\n+ * first occurence of \"old\" has been replaced by \"new\". */\n+static char *str_subst(const char *format, const char *old, const char *new)\n+{\n+\t/* Get destination buffer length. */\n+\tsize_t fmt_len = strlen(format);\n+\tsize_t old_len = strlen(old);\n+\tsize_t new_len = strlen(new);\n+\tsize_t len = fmt_len - old_len + new_len + 1;\n+\n+\t/* Get the first occurence of old. */ \n+\tchar *pos = strstr(format, old);\n+\tif (!pos)\n+\t\treturn strcpy(xmalloc(fmt_len + 1), format);\n+\tsize_t start_len = pos - format;\n+\n+\t/* Alloc destination buffer. */\n+\tchar *result = xmalloc(len);\n+\n+\t/* Copy into destination buffer. */\n+\tstrncpy(result, format, start_len);\n+\tstrncpy(result + start_len, new, new_len);\n+\tstrcpy(result + start_len + new_len,\n+\t       format + start_len + old_len);\n+\n+\treturn result;\n+}\n+\n+void sq_quote_argv_printf(FILE* out, const char **argv, int count,\n+\t\t\t   const char *format, ...)\n+{\n+\t/* Replace the string \"ARGV\" in format with the quoted arg values. */\n+\tchar *argv_str = sq_quote_argv(argv, count);\n+\tchar *new_format = str_subst(format, \"ARGV\", argv_str);\n+\n+\t/* Print into \"out\" using the new format. */\n+\tva_list rest;\n+\tva_start(rest, format);\n+\tvfprintf(out, new_format, rest);\n+\tva_end(rest);\n+\n+\tfree(new_format);\n+\tfree(argv_str);\t\n+}\n+\n char *sq_dequote(char *arg)\n {\n \tchar *dst = arg;\ndiff --git a/quote.h b/quote.h\nindex fc5481e..2bbf196 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -31,6 +31,9 @@ #include <stdio.h>\n extern char *sq_quote(const char *src);\n extern void sq_quote_print(FILE *stream, const char *src);\n extern size_t sq_quote_buf(char *dst, size_t n, const char *src);\n+extern char *sq_quote_argv(const char** argv, int count);\n+extern void sq_quote_argv_printf(FILE* out, const char **argv, int count,\n+\t\t\t\t const char *format, ...);\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\n-- \n1.4.2.gc35c-dirty\n"},{"id":"25975","messageId":"7vmz9qybs5.fsf@assigned-by-dhcp.cox.net","threadId":"5372","inReplyTo":"20060824074547.a8fa0005.chriscool@tuxfamily.org","subject":"Re: [PATCH] Refactoring tracing code in \"git.c\" and \"exec_cmd.c\".","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-27T05:42:50Z","receivedAt":"2006-08-27T05:42:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> Some new helper functions in \"quote.c\" are used for this.\n> The goal is also to get near the point where we can use\n> one write(2) call to trace in any open file descriptor.\n> This is why we put the trace string into one buffer.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n\nI really liked the fact (not necessarily the way, though) this\nshortens the callers.\n\n> diff --git a/quote.c b/quote.c\n> index e220dcc..84d0b7b 100644\n> --- a/quote.c\n> +++ b/quote.c\n> @@ -74,6 +74,84 @@ char *sq_quote(const char *src)\n>  \treturn buf;\n>  }\n>  \n> +char *sq_quote_argv(const char** argv, int count)\n> +{\n> +\tchar *buf, *to;\n> +\tint i;\n> +\tsize_t len;\n> +\n> +\t/* Count argv if needed. */\n> +\tif (count < 0) {\n> +\t\tchar **p = (char **)argv;\n> +\t\tcount = 0;\n> +\t\twhile (*p++) count++;\n> +\t}\n\nWouldn't this be easier to read?\n\n\tif (count < 0)\n        \tfor (count = 0; argv[count]; count++)\n                \t; /* just counting */\n\n\n> +\t/* Get destination buffer length. */\n> +\tlen = count ? count : 1;\n\nThis confused me quite a bit.  Wouldn't it be simpler to special\ncase the count==0 case and return xcalloc(1,1) here (this would\nallow you to lose \"if (!count)\" later as well)?\n\n> +\t/* Copy into destination buffer. */\n> +\tfor (i = 0; i < count; ++i) {\n> +\t\tif (i) *to++ = ' ';\n\n(style)\n\tif (i)\n        \t*to++ = ' ';\n\n> +\t\tto += sq_quote_buf(to, len, argv[i]);\n> +\t}\n> +\n> +\tif (!count)\n> +\t\t*buf = 0;\n> +\t\n> +\treturn buf;\n> +}\n\n> +/* Return a newly allocated copy of \"format\" where the\n> + * first occurence of \"old\" has been replaced by \"new\". */\n> +static char *str_subst(const char *format, const char *old, const char *new)\n> +{\n\nI do not think there is anything wrong with this function\nper-se, but...\n\n> +void sq_quote_argv_printf(FILE* out, const char **argv, int count,\n> +\t\t\t   const char *format, ...)\n> +{\n> +\t/* Replace the string \"ARGV\" in format with the quoted arg values. */\n> +\tchar *argv_str = sq_quote_argv(argv, count);\n> +\tchar *new_format = str_subst(format, \"ARGV\", argv_str);\n> +\n> +\t/* Print into \"out\" using the new format. */\n> +\tva_list rest;\n> +\tva_start(rest, format);\n> +\tvfprintf(out, new_format, rest);\n> +\tva_end(rest);\n\nthis feels wrong.  What happens when the original argv had\na per-cent in it?\n"},{"id":"25997","messageId":"200608272138.54834.chriscool@tuxfamily.org","threadId":"5372","inReplyTo":"7vmz9qybs5.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Refactoring tracing code in \"git.c\" and \"exec_cmd.c\".","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-08-27T19:38:54Z","receivedAt":"2006-08-27T19:38:54Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le dimanche 27 août 2006 07:42, Junio C Hamano a écrit :\n> > +char *sq_quote_argv(const char** argv, int count)\n> > +{\n> > +\tchar *buf, *to;\n> > +\tint i;\n> > +\tsize_t len;\n> > +\n> > +\t/* Count argv if needed. */\n> > +\tif (count < 0) {\n> > +\t\tchar **p = (char **)argv;\n> > +\t\tcount = 0;\n> > +\t\twhile (*p++) count++;\n> > +\t}\n>\n> Wouldn't this be easier to read?\n>\n> \tif (count < 0)\n>         \tfor (count = 0; argv[count]; count++)\n>                 \t; /* just counting */\n\nYes, it looks better.\n\n> > +\t/* Get destination buffer length. */\n> > +\tlen = count ? count : 1;\n>\n> This confused me quite a bit.  Wouldn't it be simpler to special\n> case the count==0 case and return xcalloc(1,1) here (this would\n> allow you to lose \"if (!count)\" later as well)?\n>\n> > +\t/* Copy into destination buffer. */\n> > +\tfor (i = 0; i < count; ++i) {\n> > +\t\tif (i) *to++ = ' ';\n>\n> (style)\n> \tif (i)\n>         \t*to++ = ' ';\n\nOk, I will take care of this.\n\n> > +\t\tto += sq_quote_buf(to, len, argv[i]);\n> > +\t}\n> > +\n> > +\tif (!count)\n> > +\t\t*buf = 0;\n> > +\n> > +\treturn buf;\n> > +}\n> >\n> > +/* Return a newly allocated copy of \"format\" where the\n> > + * first occurence of \"old\" has been replaced by \"new\". */\n> > +static char *str_subst(const char *format, const char *old, const char\n> > *new) +{\n>\n> I do not think there is anything wrong with this function\n> per-se, but...\n>\n> > +void sq_quote_argv_printf(FILE* out, const char **argv, int count,\n> > +\t\t\t   const char *format, ...)\n> > +{\n> > +\t/* Replace the string \"ARGV\" in format with the quoted arg values. */\n> > +\tchar *argv_str = sq_quote_argv(argv, count);\n> > +\tchar *new_format = str_subst(format, \"ARGV\", argv_str);\n> > +\n> > +\t/* Print into \"out\" using the new format. */\n> > +\tva_list rest;\n> > +\tva_start(rest, format);\n> > +\tvfprintf(out, new_format, rest);\n> > +\tva_end(rest);\n>\n> this feels wrong.  What happens when the original argv had\n> a per-cent in it?\n\nYou are right, I will rework this.\n\nThanks,\nChristian. \n"}]}