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

From: Johannes Sixt <j6t@kdbg.org>
Date: Sep 19, 2021, 09:00 UTC
Message-ID: <8dfd0683-e019-a45c-30a4-d49ea627e36d@kdbg.org>
In-Reply-To: <20210919015729.98323-3-davvid@gmail.com>

Am 19.09.21 um 03:57 schrieb David Aguilar:

Show 5 quoted lines

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

Show 12 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]).

-- Hannes