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

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