Re: [PATCH v4 2/3] difftool: use a strbuf to create a tmpdir path without repeated slashes

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Sep 19, 2021 at 4:38 PM David Aguilar <davvid@gmail.com> wrote:
> difftool: use a strbuf to create a tmpdir path without repeated slashes
>
> The paths generated by difftool are passed to user-facing diff tools.
> Using paths with repeated slashes in them is a cosmetic blemish that
> is exposed to users and can be avoided.
>
> Use a strbuf to create the buffer used for the dir-diff tmpdir.
> Strip trailing slashes from the value read from TMPDIR to avoid
> repeated slashes in the generated paths.

Mentioning strbuf in the commit message misleads the reviewer into
thinking that it somehow merits extra attention and close scrutiny. In
fact, the opposite is true: strbuf is just an implementation detail;
there is no reason to mention it in the commit message at all. The
commit message's emphasis on strbuf distracts the reader from the real
purpose of this change, which is to fix a cosmetic issue (and maybe a
real issue on Windows in which double-slash can have significance?).
So, perhaps:

    difftool: fold out repeated slashes from TMPDIR

    Paths generated by difftool are passed to user-facing diff tools.
    Supplying paths with repeated slashes is a cosmetic blemish that
    is exposed to users and can be avoided. Therefore, strip trailing
    slashes from the value of TMPDIR to avoid repeated slashes in the
    generated paths.

> Add a unit test to ensure that repeated slashes are not present.

Unless there is something unusual or tricky about the new test that
requires extra explanation in the commit message, there is little
reason to mention that you're adding a new test, so I'd probably drop
this line, as well. After all, the patch easily speaks for itself, and
a reviewer can see at a glance that you're adding a new test.

> diff --git a/builtin/difftool.c b/builtin/difftool.c
> @@ -252,11 +252,10 @@ static void changed_files(struct hashmap *result, const char *index_path,
> -static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
> +static NORETURN void exit_cleanup(struct strbuf *buf, int exit_code)
>  {
> -       struct strbuf buf = STRBUF_INIT;
> -       strbuf_addstr(&buf, tmpdir);
> -       remove_dir_recursively(&buf, 0);
> +       remove_dir_recursively(buf, 0);
> +       strbuf_release(buf);
>         if (exit_code)
>                 warning(_("failed: %d"), exit_code);
>         exit(exit_code);

It feels wrong to be releasing the caller-supplied strbuf; this change
makes it harder to reason about ownership. Normally, the entity which
allocates a resource should be the one to release it. More on this
below...

> @@ -333,11 +332,11 @@ static int checkout_path(unsigned mode, struct object_id *oid,
> +       struct strbuf tmpdir = STRBUF_INIT;
> +       strbuf_add_absolute_path(&tmpdir, tmp ? tmp : "/tmp");
> +       strbuf_trim_trailing_dir_sep(&tmpdir);
> +       strbuf_addstr(&tmpdir, "/git-difftool.XXXXXX");
> +       if (!mkdtemp(tmpdir.buf))
> +               return error("could not create '%s'", tmpdir.buf);

Leaking the `tmpdir` strbuf here. You'd want:

    if (!mkdtemp(tmpdir.buf)) {
        error("could not create '%s'", tmpdir.buf);
        strbuf_release(&tmpdir);
        return -1;
    }

> @@ -644,11 +645,11 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>         if (err) {
> -               warning(_("temporary files exist in '%s'."), tmpdir);
> +               warning(_("temporary files exist in '%s'."), tmpdir.buf);
>                 warning(_("you may want to cleanup or recover these."));
>                 exit(1);
>         } else
> -               exit_cleanup(tmpdir, rc);
> +               exit_cleanup(&tmpdir, rc);

... [continued from above]

Both branches in this conditional terminate the program either
directly by `exit(1)` or indirectly through `exit_cleanup(...)`. Yet
only the `exit_cleanup(...)` branch releases the strbuf (because you
updated `exit_cleanup()` above to do so); the other branch just leaks
the strbuf. This is inconsistent.

Since we're exiting anyhow, and since `exit_cleanup()` was already
leaking its own strbuf even before this patch, and since it feels
somewhat dirty to have `exit_cleanup()` responsible for releasing a
resource it didn't allocate, it may make sense just to maintain the
status-quo and just leak the strbuf before exiting. That is, don't
make any changes to `exit_cleanup()`, and let both of these branches
leak the strbuf.

On the other hand, if you really do want to release the strbuf, then
it would be more consistent for both branches in this conditional to
do so, not just one. That is, add a strbuf_release() to the `then`
arm.

>  finish:
>         if (fp)
> @@ -660,6 +661,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>         strbuf_release(&rdir);
>         strbuf_release(&wtdir);
>         strbuf_release(&buf);
> +       strbuf_release(&tmpdir);

Correctly releasing the strbuf. Good.