Nguyễn Thái Ngọc Duy <pclo...@gmail.com> 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

Reply via email to