From: Junio C Hamano Date: Thu, 30 Sep 2021 22:17:22 GMT Subject: Re: [PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function Message-ID: In-Reply-To: <20210930170146.61489-4-davvid@gmail.com> David Aguilar 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 and write to it", i.e. a variant of write_file() but inside the tree specified by the extra 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 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.