| Message ID | 20210930170146.61489-4-davvid@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6,1/5] difftool: create a tmpdir path without repeated slashes | expand |
David Aguilar <davvid@gmail.com> 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 <gitster@pobox.com> wrote: > > David Aguilar <davvid@gmail.com> 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 ;-) Likewise, even I don't like to take credit for my scrappy patches sometimes ;-) I'll reword this to avoid mentioning the review context. > > +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. > > > + 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.
diff --git a/builtin/difftool.c b/builtin/difftool.c index e419bd3cd1..bbb8b399c2 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -320,6 +320,17 @@ static int checkout_path(unsigned mode, struct object_id *oid, return ret; } +static void write_entry(const char *path, const char *content, + struct strbuf *buf, size_t len) +{ + if (!*content) + return; + add_path(buf, len, path); + ensure_leading_directories(buf->buf); + unlink(buf->buf); + write_file(buf->buf, "%s", content); +} + static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, struct child_process *child) { @@ -533,16 +544,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, */ hashmap_for_each_entry(&submodules, &iter, entry, entry /* member name */) { - if (*entry->left) { - add_path(&ldir, ldir_len, entry->path); - ensure_leading_directories(ldir.buf); - write_file(ldir.buf, "%s", entry->left); - } - if (*entry->right) { - add_path(&rdir, rdir_len, entry->path); - ensure_leading_directories(rdir.buf); - write_file(rdir.buf, "%s", entry->right); - } + write_entry(entry->path, entry->left, &ldir, ldir_len); + write_entry(entry->path, entry->right, &rdir, rdir_len); } /* @@ -552,18 +555,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, */ hashmap_for_each_entry(&symlinks2, &iter, entry, entry /* member name */) { - if (*entry->left) { - add_path(&ldir, ldir_len, entry->path); - ensure_leading_directories(ldir.buf); - unlink(ldir.buf); - write_file(ldir.buf, "%s", entry->left); - } - if (*entry->right) { - add_path(&rdir, rdir_len, entry->path); - ensure_leading_directories(rdir.buf); - unlink(rdir.buf); - write_file(rdir.buf, "%s", entry->right); - } + + write_entry(entry->path, entry->left, &ldir, ldir_len); + write_entry(entry->path, entry->right, &rdir, rdir_len); } strbuf_release(&buf);
Add a write_entry() helper function to handle the unlinking and writing of the dir-diff submodule and symlink stand-in files. Use write_entry() inside of the hashmap loops to eliminate duplicate code and to safeguard the submodules hashmap loop against the symlink-chasing behavior that 5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22) addressed. The submodules loop should not strictly require the unlink() call that this is introducing to them, but it does not necessarily hurt them either beyond the cost of the extra unlink(). Signed-off-by: David Aguilar <davvid@gmail.com> --- 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. builtin/difftool.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-)