Am 19.09.21 um 21:28 schrieb David Aguilar: > On Sun, Sep 19, 2021 at 11:46 AM Johannes Sixt <j6t@xxxxxxxx> 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. > > """ With the justification that the path is user-visible you have my full consent to this patch. It shows a careful eye for the detail. -- Hannes
