Re: [PATCH 11/67] trace: use strbuf for quote_crnl output
On Wed, Sep 16, 2015 at 06:31:14AM -0400, Jeff King wrote:
> On Tue, Sep 15, 2015 at 08:55:58PM -0400, Eric Sunshine wrote:
> > On Tue, Sep 15, 2015 at 11:28 AM, Jeff King wrote:
> > > static const char *quote_crnl(const char *path)
> > > {
> > > - static char new_path[PATH_MAX];
> > > + static struct strbuf new_path = STRBUF_INIT;
> > > const char *p2 = path;
> > > - char *p1 = new_path;
> >
> > It's a little sad that this leaves a variable named 'p2' when there is
> > no corresponding 'p1'. Would this deserve a cleanup patch which
> > renames 'p2' to 'p' or do we not care enough?
>
> Yeah, you're right. The original had symmetry in p1 and p2, in that it
> moved them forward together. Now that symmetry is gone, and I wonder if
> the simplest cleanup is to just drop "p2" altogether and advance the
> "path" source pointer? Like this:
Works for me. The diff is a bit noisier, but the final result feels clean.
> -- >8 --
> Subject: [PATCH] trace: use strbuf for quote_crnl output
>
> When we output GIT_TRACE_SETUP paths, we quote any
> meta-characters. But our buffer to hold the result is only
> PATH_MAX bytes, and we could double the size of the input
> path (if every character needs quoting). We could use a
> 2*PATH_MAX buffer, if we assume the input will never be more
> than PATH_MAX. But it's easier still to just switch to a
> strbuf and not worry about whether the input can exceed
> PATH_MAX or not.
>
> The original copied the "p2" pointer to "p1", advancing
> both. Since this gets rid of "p1", let's also drop "p2",
> whose name is now confusing. We can just advance the
> original "path" pointer.
>
> Signed-off-by: Jeff King
> ---
> trace.c | 23 +++
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/trace.c b/trace.c
> index 7393926..4aeea60 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -277,25 +277,24 @@ void trace_performance_fl(const char *file, int line,
> uint64_t nanos,
>
> static const char *quote_crnl(const char *path)
> {
> - static char new_path[PATH_MAX];
> - const char *p2 = path;
> - char *p1 = new_path;
> + static struct strbuf new_path = STRBUF_INIT;
>
> if (!path)
> return NULL;
>
> - while (*p2) {
> - switch (*p2) {
> - case '\\': *p1++ = '\\'; *p1++ = '\\'; break;
> - case '\n': *p1++ = '\\'; *p1++ = 'n'; break;
> - case '\r': *p1++ = '\\'; *p1++ = 'r'; break;
> + strbuf_reset(&new_path);
> +
> + while (*path) {
> + switch (*path) {
> + case '\\': strbuf_addstr(&new_path, ""); break;
> + case '\n': strbuf_addstr(&new_path, "\\n"); break;
> + case '\r': strbuf_addstr(&new_path, "\\r"); break;
> default:
> - *p1++ = *p2;
> + strbuf_addch(&new_path, *path);
> }
> - p2++;
> + path++;
> }
> - *p1 = '\0';
> - return new_path;
> + return new_path.buf;
> }
>
> /* FIXME: move prefix to startup_info struct and get rid of this arg */
> --
> 2.6.0.rc2.408.ga2926b9
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/67] trace: use strbuf for quote_crnl output
On Tue, Sep 15, 2015 at 08:55:58PM -0400, Eric Sunshine wrote:
> On Tue, Sep 15, 2015 at 11:28 AM, Jeff King wrote:
> > When we output GIT_TRACE_SETUP paths, we quote any
> > meta-characters. But our buffer to hold the result is only
> > PATH_MAX bytes, and we could double the size of the input
> > path (if every character needs quoted). We could use a
>
> s/quoted/to be &/ ...or... s/quoted/quoting/
Thanks, fixed.
> > static const char *quote_crnl(const char *path)
> > {
> > - static char new_path[PATH_MAX];
> > + static struct strbuf new_path = STRBUF_INIT;
> > const char *p2 = path;
> > - char *p1 = new_path;
>
> It's a little sad that this leaves a variable named 'p2' when there is
> no corresponding 'p1'. Would this deserve a cleanup patch which
> renames 'p2' to 'p' or do we not care enough?
Yeah, you're right. The original had symmetry in p1 and p2, in that it
moved them forward together. Now that symmetry is gone, and I wonder if
the simplest cleanup is to just drop "p2" altogether and advance the
"path" source pointer? Like this:
-- >8 --
Subject: [PATCH] trace: use strbuf for quote_crnl output
When we output GIT_TRACE_SETUP paths, we quote any
meta-characters. But our buffer to hold the result is only
PATH_MAX bytes, and we could double the size of the input
path (if every character needs quoting). We could use a
2*PATH_MAX buffer, if we assume the input will never be more
than PATH_MAX. But it's easier still to just switch to a
strbuf and not worry about whether the input can exceed
PATH_MAX or not.
The original copied the "p2" pointer to "p1", advancing
both. Since this gets rid of "p1", let's also drop "p2",
whose name is now confusing. We can just advance the
original "path" pointer.
Signed-off-by: Jeff King
---
trace.c | 23 +++
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/trace.c b/trace.c
index 7393926..4aeea60 100644
--- a/trace.c
+++ b/trace.c
@@ -277,25 +277,24 @@ void trace_performance_fl(const char *file, int line,
uint64_t nanos,
static const char *quote_crnl(const char *path)
{
- static char new_path[PATH_MAX];
- const char *p2 = path;
- char *p1 = new_path;
+ static struct strbuf new_path = STRBUF_INIT;
if (!path)
return NULL;
- while (*p2) {
- switch (*p2) {
- case '\\': *p1++ = '\\'; *p1++ = '\\'; break;
- case '\n': *p1++ = '\\'; *p1++ = 'n'; break;
- case '\r': *p1++ = '\\'; *p1++ = 'r'; break;
+ strbuf_reset(&new_path);
+
+ while (*path) {
+ switch (*path) {
+ case '\\': strbuf_addstr(&new_path, ""); break;
+ case '\n': strbuf_addstr(&new_path, "\\n"); break;
+ case '\r': strbuf_addstr(&new_path, "\\r"); break;
default:
- *p1++ = *p2;
+ strbuf_addch(&new_path, *path);
}
- p2++;
+ path++;
}
- *p1 = '\0';
- return new_path;
+ return new_path.buf;
}
/* FIXME: move prefix to startup_info struct and get rid of this arg */
--
2.6.0.rc2.408.ga2926b9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/67] trace: use strbuf for quote_crnl output
On Tue, Sep 15, 2015 at 11:28 AM, Jeff King wrote:
> When we output GIT_TRACE_SETUP paths, we quote any
> meta-characters. But our buffer to hold the result is only
> PATH_MAX bytes, and we could double the size of the input
> path (if every character needs quoted). We could use a
s/quoted/to be &/ ...or... s/quoted/quoting/
> 2*PATH_MAX buffer, if we assume the input will never be more
> than PATH_MAX. But it's easier still to just switch to a
> strbuf and not worry about whether the input can exceed
> PATH_MAX or not.
>
> Signed-off-by: Jeff King
> ---
> diff --git a/trace.c b/trace.c
> index 7393926..0c06d71 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -277,25 +277,25 @@ void trace_performance_fl(const char *file, int line,
> uint64_t nanos,
>
> static const char *quote_crnl(const char *path)
> {
> - static char new_path[PATH_MAX];
> + static struct strbuf new_path = STRBUF_INIT;
> const char *p2 = path;
> - char *p1 = new_path;
It's a little sad that this leaves a variable named 'p2' when there is
no corresponding 'p1'. Would this deserve a cleanup patch which
renames 'p2' to 'p' or do we not care enough?
> if (!path)
> return NULL;
>
> + strbuf_reset(&new_path);
> +
> while (*p2) {
> switch (*p2) {
> - case '\\': *p1++ = '\\'; *p1++ = '\\'; break;
> - case '\n': *p1++ = '\\'; *p1++ = 'n'; break;
> - case '\r': *p1++ = '\\'; *p1++ = 'r'; break;
> + case '\\': strbuf_addstr(&new_path, ""); break;
> + case '\n': strbuf_addstr(&new_path, "\\n"); break;
> + case '\r': strbuf_addstr(&new_path, "\\r"); break;
> default:
> - *p1++ = *p2;
> + strbuf_addch(&new_path, *p2);
> }
> p2++;
> }
> - *p1 = '\0';
> - return new_path;
> + return new_path.buf;
> }
>
> /* FIXME: move prefix to startup_info struct and get rid of this arg */
> --
> 2.6.0.rc2.408.ga2926b9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/67] trace: use strbuf for quote_crnl output
When we output GIT_TRACE_SETUP paths, we quote any
meta-characters. But our buffer to hold the result is only
PATH_MAX bytes, and we could double the size of the input
path (if every character needs quoted). We could use a
2*PATH_MAX buffer, if we assume the input will never be more
than PATH_MAX. But it's easier still to just switch to a
strbuf and not worry about whether the input can exceed
PATH_MAX or not.
Signed-off-by: Jeff King
---
trace.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/trace.c b/trace.c
index 7393926..0c06d71 100644
--- a/trace.c
+++ b/trace.c
@@ -277,25 +277,25 @@ void trace_performance_fl(const char *file, int line,
uint64_t nanos,
static const char *quote_crnl(const char *path)
{
- static char new_path[PATH_MAX];
+ static struct strbuf new_path = STRBUF_INIT;
const char *p2 = path;
- char *p1 = new_path;
if (!path)
return NULL;
+ strbuf_reset(&new_path);
+
while (*p2) {
switch (*p2) {
- case '\\': *p1++ = '\\'; *p1++ = '\\'; break;
- case '\n': *p1++ = '\\'; *p1++ = 'n'; break;
- case '\r': *p1++ = '\\'; *p1++ = 'r'; break;
+ case '\\': strbuf_addstr(&new_path, ""); break;
+ case '\n': strbuf_addstr(&new_path, "\\n"); break;
+ case '\r': strbuf_addstr(&new_path, "\\r"); break;
default:
- *p1++ = *p2;
+ strbuf_addch(&new_path, *p2);
}
p2++;
}
- *p1 = '\0';
- return new_path;
+ return new_path.buf;
}
/* FIXME: move prefix to startup_info struct and get rid of this arg */
--
2.6.0.rc2.408.ga2926b9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
