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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 20, 2021, 05:40 UTC
Message-ID: <CAPig+cTBfP5_czsPiALcF3tODJmNfXvNkTjqVFRbHCS535d-ng@mail.gmail.com>
In-Reply-To: <20210919203832.91207-3-davvid@gmail.com>

On Sun, Sep 19, 2021 at 4:38 PM David Aguilar <davvid@gmail.com> wrote:

Show 9 quoted lines

> 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.

Show 13 quoted lines

> 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...

Show 7 quoted lines

> @@ -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;
    }

Show 9 quoted lines

> @@ -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.

Show 7 quoted lines

>  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.