Re: [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes
From: David Aguilar <hidden>
Date: 2021-09-30 17:05:51
On Fri, Sep 24, 2021 at 3:43 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
On Wed, Sep 22 2021, David Aguilar wrote:quoted
@@ -580,7 +574,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, flags = 0; } else setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1); - rc = run_command_v_opt(helper_argv, flags); + ret = run_command_v_opt(helper_argv, flags); /* TODO: audit for interaction with sparse-index. */ ensure_full_index(&wtindex);Just on this part: There was already a logic error in the pre-image where we'd return error() return values up to cmd_main() et al. This means that (on POSIX) we return 255, not 1, per the C standard doing this is "implementation defined". I think you want the "rc" variable back, and either in another commit or in some other cleanup this on top or ahead of this patch:
Good catch. I submitted an incremental follow-up patch that addresses this issue.
quoted hunk ↗ jump to hunk
diff --git a/builtin/difftool.c b/builtin/difftool.c index a867c49067c..6605d17e6bc 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c@@ -345,6 +345,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL }; struct hashmap wt_modified, tmp_modified; int indices_loaded = 0; + int rc; workdir = get_git_work_tree();@@ -574,7 +575,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, flags = 0; } else setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1); - ret = run_command_v_opt(helper_argv, flags); + rc = run_command_v_opt(helper_argv, flags); /* TODO: audit for interaction with sparse-index. */ ensure_full_index(&wtindex);@@ -660,7 +661,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, strbuf_release(&tmpdir); UNLEAK(working_tree_dups); - return ret; + return ret < 0 ? 1 : rc; }
"return ret < 0 ? 1 : rc;" seems rather subtle. In the submitted patch we're able to keep this as "return ret;" by never assigning -1 in the first place. My rationale is that it's better to not have to apply this fixup after the fact. Thanks for reviewing. -- David