Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html