David Aguilar [off-list ref] writes:
This is cleanup refactoring that Junio suggested when
5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)
touched this area of the code.
Not really what I would want to take credit for ;-)
+static void write_entry(const char *path, const char *content,
+ struct strbuf *buf, size_t len)
+{
+ if (!*content)
+ return;
I am not sure "this function is unable to write an empty file" is a
limitation we want to give to an otherwise more or less generic
helper function that may be useful in this file (it probably is not
very useful outside difftool, as what add_path() does seems quite
specific to it).
Also, is "write entry" a good name for this helper? The fact that
the contents came from entry->$side is lost inside this callee. It
looks more like "create a file for <path> and write <content> to it",
i.e. a variant of write_file() but inside the tree specified by the
extra <buf, len> pair. So perhaps
write_file_in_directory(buf, len, path, content);
to match how the write_file() takes its parameters? While
write_file() takes a single pathname with the payload, this thing
takes three parameters <buf, len, path> to specify to which
"file-in-directory" the payload is written.
+ add_path(buf, len, path);
+ ensure_leading_directories(buf->buf);
+ unlink(buf->buf);
+ write_file(buf->buf, "%s", content);
+}
Other than these two minor points, this looks good to me.
Thanks.
On Thu, Sep 30, 2021 at 3:17 PM Junio C Hamano [off-list ref] wrote:
David Aguilar [off-list ref] writes:
quoted
This is cleanup refactoring that Junio suggested when
5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)
touched this area of the code.
Not really what I would want to take credit for ;-)
Likewise, even I don't like to take credit for my scrappy patches sometimes ;-)
I'll reword this to avoid mentioning the review context.
quoted
+static void write_entry(const char *path, const char *content,
+ struct strbuf *buf, size_t len)
+{
+ if (!*content)
+ return;
I am not sure "this function is unable to write an empty file" is a
limitation we want to give to an otherwise more or less generic
helper function that may be useful in this file (it probably is not
very useful outside difftool, as what add_path() does seems quite
specific to it).
Good point, I'll move the conditional checks into the call sites
rather than having it in the helper. It'll read a little more clearly that
way as well.
Also, is "write entry" a good name for this helper? The fact that
the contents came from entry->$side is lost inside this callee. It
looks more like "create a file for <path> and write <content> to it",
i.e. a variant of write_file() but inside the tree specified by the
extra <buf, len> pair. So perhaps
write_file_in_directory(buf, len, path, content);
to match how the write_file() takes its parameters? While
write_file() takes a single pathname with the payload, this thing
takes three parameters <buf, len, path> to specify to which
"file-in-directory" the payload is written.
quoted
+ add_path(buf, len, path);
+ ensure_leading_directories(buf->buf);
+ unlink(buf->buf);
+ write_file(buf->buf, "%s", content);
+}
Other than these two minor points, this looks good to me.
write_file_in_directory() is much clearer. I'll adjust the signature
in the next version.
Thanks for the review. I'll wait to hear back about 3/5 before sending v7.
--
David