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

From: David Aguilar <davvid@gmail.com>
Date: Sep 19, 2021, 18:13 UTC
Message-ID: <CAJDDKr6AM0YZ-5GmXHz=2VbRKOusjQ-OAP2ng8GpBDb0x1pPRg@mail.gmail.com>
In-Reply-To: <8dfd0683-e019-a45c-30a4-d49ea627e36d@kdbg.org>

On Sun, Sep 19, 2021 at 2:00 AM Johannes Sixt <j6t@kdbg.org> wrote:

Show 13 quoted lines

>
> Am 19.09.21 um 03:57 schrieb David Aguilar:
> > 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.

Show 14 quoted lines

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