Re: [PATCH 2/2] entry.c: convert write_entry to use strbuf
On Fri, Oct 25, 2013 at 2:49 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Thu, Oct 24, 2013 at 12:52 AM, Junio C Hamano wrote: >>> Nguyễn Thái Ngọc Duy writes: >>> The strcpy call in open_output_fd() implies that the output buffer must be at least 25 chars long. >>> >>> Hmph, where does that 25 come from? >>> >>> [snipped] >> >> Much better. Thanks. > > So where does that 25 come from? > > We strcpy ".merge_link_XX" or ".merge_file_XX" into path[] > and run mkstemp() on it, and these templates are 18 bytes long, so I > am puzzled. > > Is 25 "just a small random number that is surely longer than these > templates--did not bother to count how long the templates are"? Yes. I was too lazy to subtract precisely the column number from between the quotes, so I just made sure the number is large enough to cover the columns.. > That's fine by me; I am just trying to make sure I am not missing > anything that turns these templates into a longer filename. -- Duy -- 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 2/2] entry.c: convert write_entry to use strbuf
Duy Nguyen writes: > On Thu, Oct 24, 2013 at 12:52 AM, Junio C Hamano wrote: >> Nguyễn Thái Ngọc Duy writes: >> >>> The strcpy call in open_output_fd() implies that the output buffer >>> must be at least 25 chars long. >> >> Hmph, where does that 25 come from? >> >> [snipped] > > Much better. Thanks. So where does that 25 come from? We strcpy ".merge_link_XX" or ".merge_file_XX" into path[] and run mkstemp() on it, and these templates are 18 bytes long, so I am puzzled. Is 25 "just a small random number that is surely longer than these templates--did not bother to count how long the templates are"? That's fine by me; I am just trying to make sure I am not missing anything that turns these templates into a longer filename. Thanks. -- 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 2/2] entry.c: convert write_entry to use strbuf
On Thu, Oct 24, 2013 at 12:52 AM, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy writes: > >> The strcpy call in open_output_fd() implies that the output buffer >> must be at least 25 chars long. > > Hmph, where does that 25 come from? > > [snipped] Much better. Thanks. -- Duy -- 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 2/2] entry.c: convert write_entry to use strbuf
Nguyễn Thái Ngọc Duy writes:
> The strcpy call in open_output_fd() implies that the output buffer
> must be at least 25 chars long.
Hmph, where does that 25 come from?
> And it's true. The only caller that
> can trigger that code is checkout-index, which has the buffer of
> PATH_MAX chars (and any systems that have PATH_MAX shorter than 25
> chars are just insane).
>
> But in order to say that, one has to walk through a dozen of
> functions. Just convert it to strbuf to avoid the constraint and
> confusion.
Wouldn't it be far clearer to document what is going on especially
around the topath parameter to checkout_entry(), than to introduce
unnecessary strbuf overhead?
At first glance, it might appear that the caller of checkout_entry()
can specify to which path the contents are written out, but in
reality topath[] is to point at the buffer to store the temporary
path generated by the lower guts of write_entry(). It is unclear in
the original code and that is worth an in-code comment.
And when describing that API requirement, we would need to say how
big a buffer the caller must allocate for topath[] in the comment.
That size does not have to be platform-dependent PATH_MAX.
Something like this?
builtin/checkout-index.c | 2 +-
cache.h | 1 +
entry.c | 8
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index b1feda7..4ed6b23 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -14,7 +14,7 @@
static int line_termination = '\n';
static int checkout_stage; /* default to checkout stage0 */
static int to_tempfile;
-static char topath[4][PATH_MAX + 1];
+static char topath[4][TEMPORARY_FILENAME_LENGTH + 1];
static struct checkout state;
diff --git a/cache.h b/cache.h
index 85b544f..3118b7f 100644
--- a/cache.h
+++ b/cache.h
@@ -975,6 +975,7 @@ struct checkout {
refresh_cache:1;
};
+#define TEMPORARY_FILENAME_LENGTH 25
extern int checkout_entry(struct cache_entry *ce, const struct checkout
*state, char *topath);
struct cache_def {
diff --git a/entry.c b/entry.c
index d955af5..2df4ee1 100644
--- a/entry.c
+++ b/entry.c
@@ -234,6 +234,14 @@ static int check_path(const char *path, int len, struct
stat *st, int skiplen)
return lstat(path, st);
}
+/*
+ * Write the contents from ce out to the working tree.
+ *
+ * When topath[] is not NULL, instead of writing to the working tree
+ * file named by ce, a temporary file is created by this function and
+ * its name is returned in topath[], which must be able to hold at
+ * least TEMPORARY_FILENAME_LENGTH bytes long.
+ */
int checkout_entry(struct cache_entry *ce,
const struct checkout *state, char *topath)
{
--
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 2/2] entry.c: convert write_entry to use strbuf
The strcpy call in open_output_fd() implies that the output buffer
must be at least 25 chars long. And it's true. The only caller that
can trigger that code is checkout-index, which has the buffer of
PATH_MAX chars (and any systems that have PATH_MAX shorter than 25
chars are just insane).
But in order to say that, one has to walk through a dozen of
functions. Just convert it to strbuf to avoid the constraint and
confusion.
Although my original motivation was simpler than that. I just wanted
to change "char *path" to "const char *path" in checkout_entry() to
make sure no funny business regarding "path" in that function.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/checkout-index.c | 19 ---
cache.h | 2 +-
entry.c | 29 -
3 files changed, 29 insertions(+), 21 deletions(-)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 69e167b..6d88c0c 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -14,7 +14,12 @@
static int line_termination = '\n';
static int checkout_stage; /* default to checkout stage0 */
static int to_tempfile;
-static char topath[4][PATH_MAX + 1];
+static struct strbuf topath[4] = {
+ STRBUF_INIT,
+ STRBUF_INIT,
+ STRBUF_INIT,
+ STRBUF_INIT
+};
static struct checkout state;
@@ -26,19 +31,19 @@ static void write_tempfile_record(const char *name, int
prefix_length)
for (i = 1; i < 4; i++) {
if (i > 1)
putchar(' ');
- if (topath[i][0])
- fputs(topath[i], stdout);
+ if (topath[i].len)
+ fputs(topath[i].buf, stdout);
else
putchar('.');
}
} else
- fputs(topath[checkout_stage], stdout);
+ fputs(topath[checkout_stage].buf, stdout);
putchar('\t');
write_name_quoted(name + prefix_length, stdout, line_termination);
for (i = 0; i < 4; i++) {
- topath[i][0] = 0;
+ strbuf_reset(&topath[i]);
}
}
@@ -65,7 +70,7 @@ static int checkout_file(const char *name, int prefix_length)
continue;
did_checkout = 1;
if (checkout_entry(ce, &state,
- to_tempfile ? topath[ce_stage(ce)] : NULL) < 0)
+ to_tempfile ? &topath[ce_stage(ce)] : NULL) < 0)
errs++;
}
@@ -109,7 +114,7 @@ static void checkout_all(const char *prefix, int
prefix_length)
write_tempfile_record(last_ce->name,
prefix_length);
}
if (checkout_entry(ce, &state,
- to_tempfile ? topath[ce_stage(ce)] : NULL) < 0)
+ to_tempfile ? &topath[ce_stage(ce)] : NULL) < 0)
errs++;
last_ce = ce;
}
diff --git a/cache.h b/cache.h
index 51d6602..276182f 100644
--- a/cache.h
+++ b/cache.h
@@ -962,7 +962,7 @@ struct checkout {
refresh_cache:1;
};
-extern int checkout_entry(struct cache_entry *ce, const struct checkout
*state, char *topath);
+extern int checkout_entry(struct cache_entry *ce, const struct checkout
*state, struct strbuf *topath);
struct cache_def {
char path[PATH_MAX + 1];
diff --git a/entry.c b/entry.c
index d955af5..a76942d 100644
--- a/entry.c
+++ b/entry.c
@@ -92,15 +92,15 @@ static void *read_blob_entry(const struct cache_entry *ce,
unsigned long *size)
return NULL;
}
-static int open_output_fd(char *path, const struct cache_entry *ce, int
to_tempfile)
+static int open_output_fd(struct strbuf *path, const struct cache_entry *ce,
int to_tempfile)
{
int symlink = (ce->ce_mode & S_IFMT) != S_IFREG;
if (to_tempfile) {
- strcpy(path, symlink
- ? ".merge_link_XX" : ".merge_file_XX");
- return mkstemp(path);
+ strbuf_reset(path);
+ strbuf_addstr(path, symlink ? ".merge_link_XX" :
".merge_file_XX");
+ return mkstemp(path->buf);
} else {
- return create_file(path, !symlink ? ce->ce_mode : 0666);
+ return create_file(path->buf, !symlink ? ce->ce_mode : 0666);
}
}
@@ -115,7 +115,7 @@ static int fstat_output(int fd, const struct checkout
*state, struct stat *st)
return 0;
}
-static int streaming_write_entry(const struct cache_entry *ce, char *path,
+static int streaming_write_entry(const struct cache_entry *ce, struct strbuf
*path,
struct stream_filter *filter,
const struct checkout *state, int to_tempfile,
int *fstat_done, struct stat *statbuf)
@@ -132,12 +132,12 @@ static in
