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

From: David Aguilar <davvid@gmail.com>
Date: Sep 19, 2021, 19:28 UTC
Message-ID: <CAJDDKr7+GnXX=sPfUvyxZhWOXCe8P_aOYtY_XdGS6t=cRNm8wA@mail.gmail.com>
In-Reply-To: <d882e3a7-d459-5063-eca7-e77e1addfed5@kdbg.org>

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

Show 19 quoted lines

>
> Am 19.09.21 um 20:13 schrieb David Aguilar:
> > 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.
>
> Thanks. My primary concern with the patch was actually that it looks
> like mere code churn that introduces an error by not using is_dir_sep().
> This is now settled.
>
> But still, without a justification why the slashes should be cleaned up,
> the patch looks like code churn. You can ignore me if it is obvious for
> others why we need the patch.

Nah, that's a good point. Thanks for clarifying. I'll hold off on resending until we've reached consensus on this patch.

The final bugfix patch is the most important one in the series so perhaps I should reorder the series so that the bugfix comes first, and these cosmetic improvements and typo fixes are the later ones in the series? That'll make it so that the bugfix is easier to backport and not entangled with these prep fixups.

I see double-slashes when using the dir-diff feature and they just look wrong to me, but "striving for perfection" is a mere subjective justification.

The double-slashes fixup is cosmetic from a technical perspective, but since the paths are exposed to the diff tools it's a cosmetic blemish that users will see front and center.

The test environment where I observed TMPDIR containing a trailing slash is macOS, so it's a fairly common setup. One justification is that we should not expose such blemishes to users -- that's what I've written below but perhaps there's a better way to express it?

What do you and others think about the proposed message below and whether or not this patch is worth applying?

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

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

"""

-- 
David