Re: [PATCH v5 2/3] difftool: create a tmpdir path without repeated slashes

[David Aguilar <davvid@gmail.com>]

On Fri, Sep 24, 2021 at 3:43 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Wed, Sep 22 2021, David Aguilar wrote:
>
> > @@ -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.


> 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