Re: [PATCH 3/4] difftool: use a strbuf to create the tmpdir path

On Sun, Sep 19, 2021 at 2:00 AM Johannes Sixt [off-list ref] wrote:

Am 19.09.21 um 03:57 schrieb David Aguilar:

quoted

Use a strbuf to create the buffer used for the dir-diff tmpdir.
Strip trailing slashes "/" from the value read from TMPDIR to avoid
double-slashes in the calculated paths.

Add a unit test to ensure that double-slashes are not present.

I wonder why it is necessary to strip trailing slashes? You even go so
far as to add a test case, but then bury the change in a commit with a
title that is about a completely different topic.

So, which one of the two changes is the "while at it, do that, too" change?

A better title might be:

    difftool: use a strbuf to generate a tmpdir path without double-slashes

The double-slashes are the point. This patch is a minor cleanup that I
found on the path towards fixing the data loss bug in patch 4.

Thanks for the heads-up about the confusion ~ I'll reword in the next
submission to make this point clearer.

Ævar (cc'd) also asked why we have a patch that deletes the temporary
repositories used by the tests. It sounds like it's best to leave
those in place so the next submission will also drop that patch and
will adjust patch 4/4 (now 3/3) so that it also does not remove the
temporary repo used by its new test.

quoted

@@ -360,11 +359,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,

      /* Setup temp directories */
      tmp = getenv("TMPDIR");
-     xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
-     if (!mkdtemp(tmpdir))
-             return error("could not create '%s'", tmpdir);
-     strbuf_addf(&ldir, "%s/left/", tmpdir);
-     strbuf_addf(&rdir, "%s/right/", tmpdir);
+     strbuf_add_absolute_path(&tmpdir, tmp ? tmp : "/tmp");
+     /* Remove trailing slashes when $TMPDIR ends in '/'. */
+     while (tmpdir.len > 0 && tmpdir.buf[tmpdir.len - 1] == '/') {

This should most likely be is_dir_sep(tmpdir.buf[tmpdir.len - 1]).

Indeed. Peff also suggested strbuf_strip_trailing_dir_sep(&tmpdir)
which is what we have in patch v2. That uses is_dir_sep().

This commit will be reworded, patch 1/4 "t7800-difftool: cleanup
temporary repositories" will be dropped, and the final patch will be
adjusted to not cleanup its temporary test repository.

I'll resend all 3 patches later with these suggestions as "v4".

cheers,
-- 
David