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

[David Aguilar <davvid@gmail.com>]

On Sun, Sep 19, 2021 at 11:46 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> 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