Re: [PATCH 2/2] entry.c: convert write_entry to use strbuf

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 23, 2013, 17:52 UTC
Message-ID: <xmqqeh7bri1h.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1382532907-30561-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@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?

Show 8 quoted lines

> 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.