Changes since v6: - avoid returning -1 to cmd_main() by adjusting the return site in "create a tmpdir path without repeated slashes". - "refactor dir-diff to write files using helper functions" was reworked to add two helper functions instead of one so that the common checks for *entry->{left,right} can be handled in a single place. - write_entry() was renamed to write_file_in_directory() and its signature was adjusted to match how write_file() takes its parameters. - write_file_in_directory() gets called from the newly added write_standin_files() helper which encompases the guts of the symlinks and submodules hashmap loops. - Comments were added describing the purpose of the helper functions. David Aguilar (4): difftool: create a tmpdir path without repeated slashes difftool: refactor dir-diff to write files using helper functions difftool: remove an unnecessary call to strbuf_release() difftool: add a missing space to the run_dir_diff() comments builtin/difftool.c | 104 ++++++++++++++++++++++---------------------- t/t7800-difftool.sh | 7 +++ 2 files changed, 60 insertions(+), 51 deletions(-) Range-diff against v6: 1: 121186ca0f ! 1: 14b5618945 difftool: create a tmpdir path without repeated slashes @@ Commit message Strip trailing slashes from the value read from TMPDIR to avoid repeated slashes in the generated paths. - Adjust the error handling to avoid leaking strbufs. + Adjust the error handling to avoid leaking strbufs and to avoid + returning -1 to cmd_main(). Signed-off-by: David Aguilar <davvid@xxxxxxxxx> @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, co strbuf_release(&buf); + strbuf_release(&tmpdir); - return ret; +- return ret; ++ return (ret < 0) ? 1 : ret; } + + static int run_file_diff(int prompt, const char *prefix, ## t/t7800-difftool.sh ## @@ t/t7800-difftool.sh: run_dir_diff_test 'difftool --dir-diff' ' 4: 8e7d54616f ! 2: 0824321eb9 difftool: refactor dir-diff to write files using a helper function @@ Metadata Author: David Aguilar <davvid@xxxxxxxxx> ## Commit message ## - difftool: refactor dir-diff to write files using a helper function + difftool: refactor dir-diff to write files using helper functions - Add a write_entry() helper function to handle the unlinking and writing + Add a helpers 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. + Use the helpers to implement the guts of the hashmap loops. + This eliminate duplicate code and safeguards 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 @@ builtin/difftool.c: static int checkout_path(unsigned mode, struct object_id *oi return ret; } -+static void write_entry(const char *path, const char *content, -+ struct strbuf *buf, size_t len) ++static void write_file_in_directory(struct strbuf *dir, size_t dir_len, ++ const char *path, const char *content) +{ -+ if (!*content) -+ return; -+ add_path(buf, len, path); -+ ensure_leading_directories(buf->buf); -+ unlink(buf->buf); -+ write_file(buf->buf, "%s", content); ++ add_path(dir, dir_len, path); ++ ensure_leading_directories(dir->buf); ++ unlink(dir->buf); ++ write_file(dir->buf, "%s", content); ++} ++ ++/* Write the file contents for the left and right sides of the difftool ++ * dir-diff representation for submodules and symlinks. Symlinks and submodules ++ * are written as regular text files so that external diff tools can diff them ++ * as text files, resulting in behavior that is analogous to to what "git diff" ++ * displays for symlink and submodule diffs. ++ */ ++static void write_standin_files(struct pair_entry *entry, ++ struct strbuf *ldir, size_t ldir_len, ++ struct strbuf *rdir, size_t rdir_len) ++{ ++ if (*entry->left) ++ write_file_in_directory(ldir, ldir_len, entry->path, entry->left); ++ if (*entry->right) ++ write_file_in_directory(rdir, rdir_len, entry->path, entry->right); +} + static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, co - 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); ++ write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len); } /* @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, co - 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); ++ write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len); } strbuf_release(&buf); 5: 8db6ae3373 ! 3: 94ad86157e difftool: remove an unnecessary call to strbuf_release() @@ Commit message ## builtin/difftool.c ## @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, - write_entry(entry->path, entry->right, &rdir, rdir_len); + write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len); } - strbuf_release(&buf); 2: 080a113917 = 4: 5b6dfe5e5c difftool: add a missing space to the run_dir_diff() comments 3: 1fbc47a58d < -: ---------- difftool: avoid returning -1 to cmd_main() from run_dir_diff() -- 2.33.0.886.g5b6dfe5e5c