{"thread":{"id":"56615","subject":"[PATCH v6 1/5] difftool: create a tmpdir path without repeated slashes","startedAt":"2021-09-30T17:01:55Z","lastAt":"2021-10-01T00:12:27Z","messageCount":11,"participants":["David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":5},"messages":[{"id":"437556","messageId":"20210930170146.61489-1-davvid@gmail.com","threadId":"56615","inReplyTo":null,"subject":"[PATCH v6 1/5] difftool: create a tmpdir path without repeated slashes","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T17:01:42Z","receivedAt":"2021-09-30T17:01:55Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The paths generated by difftool are passed to user-facing diff tools.\nUsing paths with repeated slashes in them is a cosmetic blemish that\nis exposed to users and can be avoided.\n\nUse a strbuf to create the buffer used for the dir-diff tmpdir.\nStrip trailing slashes from the value read from TMPDIR to avoid\nrepeated slashes in the generated paths.\n\nAdjust the error handling to avoid leaking strbufs.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis patch is a resend based off of \"next\" so that this series can be\ntracked together.\n\nThis patch is unchanged since the v5 was submitted, but there are\nnew related patches that proceed it.\n\n builtin/difftool.c  | 48 ++++++++++++++++++++++-----------------------\n t/t7800-difftool.sh |  7 +++++++\n 2 files changed, 30 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 210da03908..9d2dfd3361 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -252,16 +252,6 @@ static void changed_files(struct hashmap *result, const char *index_path,\n \tstrbuf_release(&buf);\n }\n \n-static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstrbuf_addstr(&buf, tmpdir);\n-\tremove_dir_recursively(&buf, 0);\n-\tif (exit_code)\n-\t\twarning(_(\"failed: %d\"), exit_code);\n-\texit(exit_code);\n-}\n-\n static int ensure_leading_directories(char *path)\n {\n \tswitch (safe_create_leading_directories(path)) {\n@@ -333,16 +323,16 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tstruct child_process *child)\n {\n-\tchar tmpdir[PATH_MAX];\n \tstruct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n \tstruct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;\n \tstruct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;\n \tstruct strbuf wtdir = STRBUF_INIT;\n-\tchar *lbase_dir, *rbase_dir;\n+\tstruct strbuf tmpdir = STRBUF_INIT;\n+\tchar *lbase_dir = NULL, *rbase_dir = NULL;\n \tsize_t ldir_len, rdir_len, wtdir_len;\n \tconst char *workdir, *tmp;\n \tint ret = 0, i;\n-\tFILE *fp;\n+\tFILE *fp = NULL;\n \tstruct hashmap working_tree_dups = HASHMAP_INIT(working_tree_entry_cmp,\n \t\t\t\t\t\t\tNULL);\n \tstruct hashmap submodules = HASHMAP_INIT(pair_cmp, NULL);\n@@ -351,7 +341,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct pair_entry *entry;\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n-\tint rc, flags = RUN_GIT_CMD, err = 0;\n+\tint flags = RUN_GIT_CMD, err = 0;\n \tconst char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n \tstruct hashmap wt_modified, tmp_modified;\n \tint indices_loaded = 0;\n@@ -360,11 +350,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \n \t/* Setup temp directories */\n \ttmp = getenv(\"TMPDIR\");\n-\txsnprintf(tmpdir, sizeof(tmpdir), \"%s/git-difftool.XXXXXX\", tmp ? tmp : \"/tmp\");\n-\tif (!mkdtemp(tmpdir))\n-\t\treturn error(\"could not create '%s'\", tmpdir);\n-\tstrbuf_addf(&ldir, \"%s/left/\", tmpdir);\n-\tstrbuf_addf(&rdir, \"%s/right/\", tmpdir);\n+\tstrbuf_add_absolute_path(&tmpdir, tmp ? tmp : \"/tmp\");\n+\tstrbuf_trim_trailing_dir_sep(&tmpdir);\n+\tstrbuf_addstr(&tmpdir, \"/git-difftool.XXXXXX\");\n+\tif (!mkdtemp(tmpdir.buf)) {\n+\t\tret = error(\"could not create '%s'\", tmpdir.buf);\n+\t\tgoto finish;\n+\t}\n+\tstrbuf_addf(&ldir, \"%s/left/\", tmpdir.buf);\n+\tstrbuf_addf(&rdir, \"%s/right/\", tmpdir.buf);\n \tstrbuf_addstr(&wtdir, workdir);\n \tif (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))\n \t\tstrbuf_addch(&wtdir, '/');\n@@ -580,7 +574,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tflags = 0;\n \t} else\n \t\tsetenv(\"GIT_DIFFTOOL_DIRDIFF\", \"true\", 1);\n-\trc = run_command_v_opt(helper_argv, flags);\n+\tret = run_command_v_opt(helper_argv, flags);\n \n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(&wtindex);\n@@ -614,7 +608,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tif (!indices_loaded) {\n \t\t\tstruct lock_file lock = LOCK_INIT;\n \t\t\tstrbuf_reset(&buf);\n-\t\t\tstrbuf_addf(&buf, \"%s/wtindex\", tmpdir);\n+\t\t\tstrbuf_addf(&buf, \"%s/wtindex\", tmpdir.buf);\n \t\t\tif (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\n \t\t\t    write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {\n \t\t\t\tret = error(\"could not write %s\", buf.buf);\n@@ -644,11 +638,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t}\n \n \tif (err) {\n-\t\twarning(_(\"temporary files exist in '%s'.\"), tmpdir);\n+\t\twarning(_(\"temporary files exist in '%s'.\"), tmpdir.buf);\n \t\twarning(_(\"you may want to cleanup or recover these.\"));\n-\t\texit(1);\n-\t} else\n-\t\texit_cleanup(tmpdir, rc);\n+\t\tret = 1;\n+\t} else {\n+\t\tremove_dir_recursively(&tmpdir, 0);\n+\t\tif (ret)\n+\t\t\twarning(_(\"failed: %d\"), ret);\n+\t}\n \n finish:\n \tif (fp)\n@@ -660,6 +657,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstrbuf_release(&rdir);\n \tstrbuf_release(&wtdir);\n \tstrbuf_release(&buf);\n+\tstrbuf_release(&tmpdir);\n \n \treturn ret;\n }\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 528e0dabf0..096456292c 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -453,6 +453,13 @@ run_dir_diff_test 'difftool --dir-diff' '\n \tgrep \"^file$\" output\n '\n \n+run_dir_diff_test 'difftool --dir-diff avoids repeated slashes in TMPDIR' '\n+\tTMPDIR=\"${TMPDIR:-/tmp}////\" \\\n+\t\tgit difftool --dir-diff $symlinks --extcmd echo branch >output &&\n+\tgrep -v // output >actual &&\n+\ttest_line_count = 1 actual\n+'\n+\n run_dir_diff_test 'difftool --dir-diff ignores --prompt' '\n \tgit difftool --dir-diff $symlinks --prompt --extcmd ls branch >output &&\n \tgrep \"^sub$\" output &&\n-- \n2.33.0.887.g8db6ae3373\n\n"},{"id":"437557","messageId":"20210930170146.61489-2-davvid@gmail.com","threadId":"56615","inReplyTo":"20210930170146.61489-1-davvid@gmail.com","subject":"[PATCH v6 2/5] difftool: add a missing space to the run_dir_diff() comments","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T17:01:43Z","receivedAt":"2021-09-30T17:01:56Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Signed-off-by: David Aguilar <davvid@gmail.com>\n---\nTrivial tweak. Unchanged but resubmitted for ease of application.\n\n builtin/difftool.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 9d2dfd3361..fdaaa86bff 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -542,7 +542,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t}\n \n \t/*\n-\t * Symbolic links require special treatment.The standard \"git diff\"\n+\t * Symbolic links require special treatment. The standard \"git diff\"\n \t * shows only the link itself, not the contents of the link target.\n \t * This loop replicates that behavior.\n \t */\n-- \n2.33.0.887.g8db6ae3373\n\n"},{"id":"437558","messageId":"20210930170146.61489-3-davvid@gmail.com","threadId":"56615","inReplyTo":"20210930170146.61489-1-davvid@gmail.com","subject":"[PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T17:01:44Z","receivedAt":"2021-09-30T17:01:56Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"difftool was forwarding the -1 result from error() to cmd_main(), which\nis implementation-defined since it is outside of the 0-255 range\nspecified by POSIX for program exit codes.\n\nStop assigning the result of error() to `ret`. Assign a value of 1\nwhenever internal errors are detected instead.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis patch addresses the note from Ævar about returning -1 to cmd_main().\n\n builtin/difftool.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex fdaaa86bff..e419bd3cd1 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -447,7 +447,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \n \t\tif (lmode && status != 'C') {\n \t\t\tif (checkout_path(lmode, &loid, src_path, &lstate)) {\n-\t\t\t\tret = error(\"could not write '%s'\", src_path);\n+\t\t\t\tret = 1;\n+\t\t\t\terror(\"could not write '%s'\", src_path);\n \t\t\t\tgoto finish;\n \t\t\t}\n \t\t}\n@@ -468,8 +469,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tif (!use_wt_file(workdir, dst_path, &roid)) {\n \t\t\t\tif (checkout_path(rmode, &roid, dst_path,\n \t\t\t\t\t\t  &rstate)) {\n-\t\t\t\t\tret = error(\"could not write '%s'\",\n-\t\t\t\t\t\t    dst_path);\n+\t\t\t\t\tret = 1;\n+\t\t\t\t\terror(\"could not write '%s'\", dst_path);\n \t\t\t\t\tgoto finish;\n \t\t\t\t}\n \t\t\t} else if (!is_null_oid(&roid)) {\n@@ -487,15 +488,16 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \n \t\t\t\tadd_path(&rdir, rdir_len, dst_path);\n \t\t\t\tif (ensure_leading_directories(rdir.buf)) {\n-\t\t\t\t\tret = error(\"could not create \"\n-\t\t\t\t\t\t    \"directory for '%s'\",\n-\t\t\t\t\t\t    dst_path);\n+\t\t\t\t\tret = 1;\n+\t\t\t\t\terror(\"could not create directory for '%s'\",\n+\t\t\t\t\t\tdst_path);\n \t\t\t\t\tgoto finish;\n \t\t\t\t}\n \t\t\t\tadd_path(&wtdir, wtdir_len, dst_path);\n \t\t\t\tif (symlinks) {\n \t\t\t\t\tif (symlink(wtdir.buf, rdir.buf)) {\n-\t\t\t\t\t\tret = error_errno(\"could not symlink '%s' to '%s'\", wtdir.buf, rdir.buf);\n+\t\t\t\t\t\tret = 1;\n+\t\t\t\t\t\terror_errno(\"could not symlink '%s' to '%s'\", wtdir.buf, rdir.buf);\n \t\t\t\t\t\tgoto finish;\n \t\t\t\t\t}\n \t\t\t\t} else {\n@@ -504,7 +506,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\t\t\t\tst.st_mode = 0644;\n \t\t\t\t\tif (copy_file(rdir.buf, wtdir.buf,\n \t\t\t\t\t\t      st.st_mode)) {\n-\t\t\t\t\t\tret = error(\"could not copy '%s' to '%s'\", wtdir.buf, rdir.buf);\n+\t\t\t\t\t\tret = 1;\n+\t\t\t\t\t\terror(\"could not copy '%s' to '%s'\", wtdir.buf, rdir.buf);\n \t\t\t\t\t\tgoto finish;\n \t\t\t\t\t}\n \t\t\t\t}\n@@ -515,7 +518,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tfclose(fp);\n \tfp = NULL;\n \tif (finish_command(child)) {\n-\t\tret = error(\"error occurred running diff --raw\");\n+\t\tret = 1;\n+\t\terror(\"error occurred running diff --raw\");\n \t\tgoto finish;\n \t}\n \n-- \n2.33.0.887.g8db6ae3373\n\n"},{"id":"437559","messageId":"20210930170146.61489-4-davvid@gmail.com","threadId":"56615","inReplyTo":"20210930170146.61489-1-davvid@gmail.com","subject":"[PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T17:01:45Z","receivedAt":"2021-09-30T17:01:57Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Add a write_entry() helper function to handle the unlinking and writing\nof the dir-diff submodule and symlink stand-in files.\n\nUse write_entry() inside of the hashmap loops to eliminate duplicate\ncode and to safeguard the submodules hashmap loop against the\nsymlink-chasing behavior that 5bafb3576a (difftool: fix symlink-file\nwriting in dir-diff mode, 2021-09-22) addressed.\n\nThe submodules loop should not strictly require the unlink() call that\nthis is introducing to them, but it does not necessarily hurt them\neither beyond the cost of the extra unlink().\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis is cleanup refactoring that Junio suggested when\n5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)\ntouched this area of the code.\n\n builtin/difftool.c | 38 ++++++++++++++++----------------------\n 1 file changed, 16 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex e419bd3cd1..bbb8b399c2 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -320,6 +320,17 @@ static int checkout_path(unsigned mode, struct object_id *oid,\n \treturn ret;\n }\n \n+static void write_entry(const char *path, const char *content,\n+\t\t\tstruct strbuf *buf, size_t len)\n+{\n+\tif (!*content)\n+\t\treturn;\n+\tadd_path(buf, len, path);\n+\tensure_leading_directories(buf->buf);\n+\tunlink(buf->buf);\n+\twrite_file(buf->buf, \"%s\", content);\n+}\n+\n static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\tstruct child_process *child)\n {\n@@ -533,16 +544,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t */\n \thashmap_for_each_entry(&submodules, &iter, entry,\n \t\t\t\tentry /* member name */) {\n-\t\tif (*entry->left) {\n-\t\t\tadd_path(&ldir, ldir_len, entry->path);\n-\t\t\tensure_leading_directories(ldir.buf);\n-\t\t\twrite_file(ldir.buf, \"%s\", entry->left);\n-\t\t}\n-\t\tif (*entry->right) {\n-\t\t\tadd_path(&rdir, rdir_len, entry->path);\n-\t\t\tensure_leading_directories(rdir.buf);\n-\t\t\twrite_file(rdir.buf, \"%s\", entry->right);\n-\t\t}\n+\t\twrite_entry(entry->path, entry->left, &ldir, ldir_len);\n+\t\twrite_entry(entry->path, entry->right, &rdir, rdir_len);\n \t}\n \n \t/*\n@@ -552,18 +555,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t */\n \thashmap_for_each_entry(&symlinks2, &iter, entry,\n \t\t\t\tentry /* member name */) {\n-\t\tif (*entry->left) {\n-\t\t\tadd_path(&ldir, ldir_len, entry->path);\n-\t\t\tensure_leading_directories(ldir.buf);\n-\t\t\tunlink(ldir.buf);\n-\t\t\twrite_file(ldir.buf, \"%s\", entry->left);\n-\t\t}\n-\t\tif (*entry->right) {\n-\t\t\tadd_path(&rdir, rdir_len, entry->path);\n-\t\t\tensure_leading_directories(rdir.buf);\n-\t\t\tunlink(rdir.buf);\n-\t\t\twrite_file(rdir.buf, \"%s\", entry->right);\n-\t\t}\n+\n+\t\twrite_entry(entry->path, entry->left, &ldir, ldir_len);\n+\t\twrite_entry(entry->path, entry->right, &rdir, rdir_len);\n \t}\n \n \tstrbuf_release(&buf);\n-- \n2.33.0.887.g8db6ae3373\n\n"},{"id":"437560","messageId":"20210930170146.61489-5-davvid@gmail.com","threadId":"56615","inReplyTo":"20210930170146.61489-1-davvid@gmail.com","subject":"[PATCH v6 5/5] difftool: remove an unnecessary call to strbuf_release()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T17:01:46Z","receivedAt":"2021-09-30T17:02:00Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"The `buf` strbuf is reused again later in the same function, so there\nis no benefit to calling strbuf_release(). The subsequent usage is\nalready using strbuf_reset() to reset the buffer, so releasing it\nearly is only going to lead to a wasteful reallocation.\n\nRemove the early call to strbuf_release(). The same strbuf is already\ncleaned up in the \"finish:\" section so nothing is leaked, either.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nThis is a small cleanup that I noticed while working on the new patches.\nThere are no previous versions of this patch.\n\n builtin/difftool.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex bbb8b399c2..859ccc6db3 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -560,8 +560,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\twrite_entry(entry->path, entry->right, &rdir, rdir_len);\n \t}\n \n-\tstrbuf_release(&buf);\n-\n \tstrbuf_setlen(&ldir, ldir_len);\n \thelper_argv[1] = ldir.buf;\n \tstrbuf_setlen(&rdir, rdir_len);\n-- \n2.33.0.887.g8db6ae3373\n\n"},{"id":"437581","messageId":"xmqqee95agbg.fsf@gitster.g","threadId":"56615","inReplyTo":"20210930170146.61489-1-davvid@gmail.com","subject":"Re: [PATCH v6 1/5] difftool: create a tmpdir path without repeated slashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-30T21:45:07Z","receivedAt":"2021-09-30T21:45:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> The paths generated by difftool are passed to user-facing diff tools.\n> Using paths with repeated slashes in them is a cosmetic blemish that\n> is exposed to users and can be avoided.\n>\n> Use a strbuf to create the buffer used for the dir-diff tmpdir.\n> Strip trailing slashes from the value read from TMPDIR to avoid\n> repeated slashes in the generated paths.\n>\n> Adjust the error handling to avoid leaking strbufs.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n> This patch is a resend based off of \"next\" so that this series can be\n> tracked together.\n>\n> This patch is unchanged since the v5 was submitted, but there are\n> new related patches that proceed it.\n\nOK.  I confirmed that this is unchanged from the first step of v5.\n"},{"id":"437582","messageId":"xmqq7dexafch.fsf@gitster.g","threadId":"56615","inReplyTo":"20210930170146.61489-3-davvid@gmail.com","subject":"Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-30T22:06:06Z","receivedAt":"2021-09-30T22:06:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> difftool was forwarding the -1 result from error() to cmd_main(), which\n> is implementation-defined since it is outside of the 0-255 range\n> specified by POSIX for program exit codes.\n>\n> Stop assigning the result of error() to `ret`. Assign a value of 1\n> whenever internal errors are detected instead.\n\nMany existing codepaths take advantage of error() returning -1 and I\ndo not see anything is wrong to keep that \"negative is error\"\nconvention for the value of \"ret\" variable.  Most lines in this\npatch however split that \"ret = error(...)\" pattern into two\nstatements and I do not see a very good reason for it.\n\nWorse yet, after applying this patch, there still are at least three\nassignments to \"ret\" that can give it a negative value:\n\n\tif (!mkdtemp(tmpdir.buf)) {\n\t\tret = error(\"could not create '%s'\", tmpdir.buf);\n\t\tgoto finish;\n\t}\n\n\tret = run_command_v_opt(helper_argv, flags);\n\n\tstrbuf_addf(&buf, \"%s/wtindex\", tmpdir.buf);\n\tif (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\n\t    write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {\n\t\tret = error(\"could not write %s\", buf.buf);\n\t\tgoto finish;\n\t}\n\nAmong them, the return value from run_command_v_opt() eventually\ncome from wait_or_whine(), I think, so it may be generic -1 or\nit may be WEXITSTATUS() of the child process.\n\nBut I am not sure if this partcular caller cares.  It is not\nprepared to handle -1 and positive return from run_command_v_opt()\nany differently.  So I think a single\n\n-\treturn ret;\n+\treturn !!ret;\n\nat the end would be much easier to reason about and maintain.\n"},{"id":"437583","messageId":"xmqq35plaetp.fsf@gitster.g","threadId":"56615","inReplyTo":"20210930170146.61489-4-davvid@gmail.com","subject":"Re: [PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-30T22:17:22Z","receivedAt":"2021-09-30T22:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> This is cleanup refactoring that Junio suggested when\n> 5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)\n> touched this area of the code.\n\nNot really what I would want to take credit for  ;-)\n\n> +static void write_entry(const char *path, const char *content,\n> +\t\t\tstruct strbuf *buf, size_t len)\n> +{\n> +\tif (!*content)\n> +\t\treturn;\n\nI am not sure \"this function is unable to write an empty file\" is a\nlimitation we want to give to an otherwise more or less generic\nhelper function that may be useful in this file (it probably is not\nvery useful outside difftool, as what add_path() does seems quite\nspecific to it).\n\nAlso, is \"write entry\" a good name for this helper?  The fact that\nthe contents came from entry->$side is lost inside this callee.  It\nlooks more like \"create a file for <path> and write <content> to it\",\ni.e. a variant of write_file() but inside the tree specified by the\nextra <buf, len> pair.  So perhaps \n\n\twrite_file_in_directory(buf, len, path, content);\n\nto match how the write_file() takes its parameters?  While\nwrite_file() takes a single pathname with the payload, this thing\ntakes three parameters <buf, len, path> to specify to which\n\"file-in-directory\" the payload is written.\n\n> +\tadd_path(buf, len, path);\n> +\tensure_leading_directories(buf->buf);\n> +\tunlink(buf->buf);\n> +\twrite_file(buf->buf, \"%s\", content);\n> +}\n\nOther than these two minor points, this looks good to me.\n\nThanks.\n"},{"id":"437586","messageId":"CAJDDKr41qzbUudAHgokRARK-kEG4araFwJ0UhLmJ4Pca=xFZRA@mail.gmail.com","threadId":"56615","inReplyTo":"xmqq7dexafch.fsf@gitster.g","subject":"Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T23:25:10Z","receivedAt":"2021-09-30T23:25:50Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Sep 30, 2021 at 3:06 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> David Aguilar <davvid@gmail.com> writes:\n>\n> > difftool was forwarding the -1 result from error() to cmd_main(), which\n> > is implementation-defined since it is outside of the 0-255 range\n> > specified by POSIX for program exit codes.\n> >\n> > Stop assigning the result of error() to `ret`. Assign a value of 1\n> > whenever internal errors are detected instead.\n>\n> Many existing codepaths take advantage of error() returning -1 and I\n> do not see anything is wrong to keep that \"negative is error\"\n> convention for the value of \"ret\" variable.  Most lines in this\n> patch however split that \"ret = error(...)\" pattern into two\n> statements and I do not see a very good reason for it.\n>\n> Worse yet, after applying this patch, there still are at least three\n> assignments to \"ret\" that can give it a negative value:\n\nIndeed.\n\n>\n>         if (!mkdtemp(tmpdir.buf)) {\n>                 ret = error(\"could not create '%s'\", tmpdir.buf);\n>                 goto finish;\n>         }\n>\n>         ret = run_command_v_opt(helper_argv, flags);\n>\n>         strbuf_addf(&buf, \"%s/wtindex\", tmpdir.buf);\n>         if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||\n>             write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {\n>                 ret = error(\"could not write %s\", buf.buf);\n>                 goto finish;\n>         }\n>\n> Among them, the return value from run_command_v_opt() eventually\n> come from wait_or_whine(), I think, so it may be generic -1 or\n> it may be WEXITSTATUS() of the child process.\n>\n> But I am not sure if this particular caller cares.  It is not\n\nThe property I was trying to maintain is that we would forward the result\nfrom the child process in most situations, so we should try and forward\nthe result from run_command_v_opt() whenever possible.\n\nBut for the others we would have to add an \"ret = 1\" there,\nand that doesn't seem worth it since it's too hard to maintain.\n\n\n\n> prepared to handle -1 and positive return from run_command_v_opt()\n> any differently.  So I think a single\n>\n> -       return ret;\n> +       return !!ret;\n>\n> at the end would be much easier to reason about and maintain.\n\nHmm I don't think we can use \"return !!ret\".\n\nIn C this does a bool cast so we lose the positive value from the\nunderlying diff tool when the value is quantized to 0/1 via !!ret.\n\nThat suggests that Ævar's sug is better...\n\n    return (ret < 0) ? 1 : ret;\n\nIf that sounds good I can send a replacement series that squashes this into the\nrepeated-symlinks patch. It doesn't seem like we'll need a separate\npatch for this.\n\n--\nDavid\n"},{"id":"437587","messageId":"CAJDDKr78WcoWjeZqz3c_bdt3s0RRg8Hx9-wC1VFmpy2yPbpbqA@mail.gmail.com","threadId":"56615","inReplyTo":"xmqq35plaetp.fsf@gitster.g","subject":"Re: [PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2021-09-30T23:34:50Z","receivedAt":"2021-09-30T23:35:30Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Sep 30, 2021 at 3:17 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> David Aguilar <davvid@gmail.com> writes:\n>\n> > This is cleanup refactoring that Junio suggested when\n> > 5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)\n> > touched this area of the code.\n>\n> Not really what I would want to take credit for  ;-)\n\nLikewise, even I don't like to take credit for my scrappy patches sometimes ;-)\n\nI'll reword this to avoid mentioning the review context.\n\n\n> > +static void write_entry(const char *path, const char *content,\n> > +                     struct strbuf *buf, size_t len)\n> > +{\n> > +     if (!*content)\n> > +             return;\n>\n> I am not sure \"this function is unable to write an empty file\" is a\n> limitation we want to give to an otherwise more or less generic\n> helper function that may be useful in this file (it probably is not\n> very useful outside difftool, as what add_path() does seems quite\n> specific to it).\n\n\nGood point, I'll move the conditional checks into the call sites\nrather than having it in the helper. It'll read a little more clearly that\nway as well.\n\n\n> Also, is \"write entry\" a good name for this helper?  The fact that\n> the contents came from entry->$side is lost inside this callee.  It\n> looks more like \"create a file for <path> and write <content> to it\",\n> i.e. a variant of write_file() but inside the tree specified by the\n> extra <buf, len> pair.  So perhaps\n>\n>         write_file_in_directory(buf, len, path, content);\n>\n> to match how the write_file() takes its parameters?  While\n> write_file() takes a single pathname with the payload, this thing\n> takes three parameters <buf, len, path> to specify to which\n> \"file-in-directory\" the payload is written.\n>\n> > +     add_path(buf, len, path);\n> > +     ensure_leading_directories(buf->buf);\n> > +     unlink(buf->buf);\n> > +     write_file(buf->buf, \"%s\", content);\n> > +}\n>\n> Other than these two minor points, this looks good to me.\n\nwrite_file_in_directory() is much clearer. I'll adjust the signature\nin the next version.\n\nThanks for the review. I'll wait to hear back about 3/5 before sending v7.\n-- \nDavid\n"},{"id":"437589","messageId":"xmqqmtnt8uxl.fsf@gitster.g","threadId":"56615","inReplyTo":"CAJDDKr41qzbUudAHgokRARK-kEG4araFwJ0UhLmJ4Pca=xFZRA@mail.gmail.com","subject":"Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-01T00:12:22Z","receivedAt":"2021-10-01T00:12:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n>> Among them, the return value from run_command_v_opt() eventually\n>> come from wait_or_whine(), I think, so it may be generic -1 or\n>> it may be WEXITSTATUS() of the child process.\n>>\n>> But I am not sure if this particular caller cares.  It is not\n>\n> The property I was trying to maintain is that we would forward the result\n> from the child process in most situations, so we should try and forward\n> the result from run_command_v_opt() whenever possible.\n\nOh, if it were the case, then I agree that !!ret would not be\nsufficient.\n\nI just had an impression that this particular caller of the\nrun_command_v_opt() did not care, as it did not special case -1\nreturned from the function as different from what came from\nWEXITSTATUS().\n\n> That suggests that Ævar's sug is better...\n>\n>     return (ret < 0) ? 1 : ret;\n\nYup, that would work, as long as exit status 1 from the external\ndiff tool is not all that special (in which case, we cannot tell\nit from 1 that came from ret = error()).\n\nThanks.\n"}]}