Re: [PATCH 2/2] entry.c: convert write_entry to use strbuf
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:04
Nguyễn Thái Ngọc Duy [off-list ref] 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) {
--
--
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.
You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en
---
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.