Re: [PATCH v2 1/3] path: use forward slashes in XDG config on Windows

[Junio C Hamano <gitster@pobox.com>]

Delilah Ashley Wu <delilahwu@linux.microsoft.com> writes:

> From: Delilah Ashley Wu <delilahwu@microsoft.com>
>
> Git prefers forward slashes as directory separators across all
> platforms. On Windows, the backslash is the native directory separator,
> but all Windows versions supported by Git also accept the forward slash
> in all but rare circumstances. Our tests expect forward slashes. Git
> displays relative paths with forward slashes. Forward slashes are more
> convenient to use in shell scripts.
>
> For these reasons, we enforced forward slashes in `interpolate_path()`
> in 5ca6b7bb47b (config --show-origin: report paths with forward slashes,
> 2016-03-23). However, other code paths may construct paths containing
> backslashes. For example, `config --show-origin` prints the XDG config
> path with mixed slashes on Windows:
>
>     $ git config --list --show-origin
>     file:C:/Program Files/Git/etc/gitconfig         system.foo=bar
>     file:"C:\\Users\\delilah/.config/git/config"    xdg.foo=bar
>     file:C:/Users/delilah/.gitconfig                home.foo=bar
>     file:.git/config                                local.foo=bar
>
> These mixed slashes occur because the `$HOME` and `$XDG_CONFIG_HOME`
> environment variables usually contain backslashes on Windows, and
> `xdg_config_home_for()` interpolates them into templates that use
> hardcoded forward slashes.
>
> Since callers of `xdg_config_home_for()` handle mixed slashes correctly,
> it is reasonable to assume that they can handle paths with only forward
> slashes. Let's enforce forward slashes in `xdg_config_home_for()` by
> using `convert_slashes()` on Windows.
>
> Also, there are no tests for the XDG path with `--show-origin`. Add a
> test for slash conversion and a confidence check for the default path.

Is this "force forwared slashes to Windows users" a required part of
XDG/HOME global fix?  If not, please leave it out of the topic.

Even if it is a good idea to always force forward slashes to Windows
users (I have no strong opinions on the topic), and if it is very
unlikely to break existing Windows users (I do not have any clue if
that would be the case or not, as I do not do Windows), we would
want to make sure if we can get the same effect without sprinkling
"#ifdef" in the platform agnostic part of the codebase like "path.c"
file.

Where would the slash in "ret" that is passed to convert_slashes()
function come from?  If they come from environment variables like
XDG_CONFIG_HOME and HOME, that is end-user's preference and we have
no business forcing them which forms of slashes to use.  Does it
come from "subdir" or "filename" parameters?  It might be the job
for the callers to standardize slashes in the value they send in,
but as far as I can see, these do not have anything other than
hardcoded constants that use no slashes (most of them) or one
forward slash ("systemd/user").

Again, I do not see it explained why this change has to be part of
this series in the proposed log message, so...?

> Signed-off-by: Delilah Ashley Wu <delilahwu@microsoft.com>
> ---
>  path.c            | 16 ++++++++++------
>  t/t1300-config.sh | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/path.c b/path.c
> index c3a709a928..f17595fd1b 100644
> --- a/path.c
> +++ b/path.c
> @@ -1544,19 +1544,23 @@ int looks_like_command_line_option(const char *str)
>  
>  char *xdg_config_home_for(const char *subdir, const char *filename)
>  {
> +	char *ret;
>  	const char *home, *config_home;
>  
>  	assert(subdir);
>  	assert(filename);
>  	config_home = getenv("XDG_CONFIG_HOME");
>  	if (config_home && *config_home)
> -		return mkpathdup("%s/%s/%s", config_home, subdir, filename);
> -
> -	home = getenv("HOME");
> -	if (home)
> -		return mkpathdup("%s/.config/%s/%s", home, subdir, filename);
> +		ret = mkpathdup("%s/%s/%s", config_home, subdir, filename);
> +	else if ((home = getenv("HOME")))
> +		ret = mkpathdup("%s/.config/%s/%s", home, subdir, filename);
> +	else
> +		return NULL;
>  
> -	return NULL;
> +#ifdef GIT_WINDOWS_NATIVE
> +	convert_slashes(ret);
> +#endif
> +	return ret;
>  }
>  
>  char *xdg_config_home(const char *filename)
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index e3f8064889..329407a73d 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -2350,6 +2350,38 @@ test_expect_success '--show-origin with --default' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'set up xdg config --show-origin tests' '
> +	mkdir -p "$HOME"/.config/git &&
> +	cat >"$HOME"/.config/git/config <<-EOF
> +	[xdg]
> +		config = true
> +	EOF
> +'
> +
> +test_expect_success MINGW '--show-origin converts backslashes in xdg path to forward slashes on Windows' '
> +	backslash_home="$(echo "$HOME" | tr / \\\\)" &&
> +	echo "file:$HOME/.config/git/config	true" >expect &&
> +
> +	(
> +		sane_unset XDG_CONFIG_HOME &&
> +		HOME="$backslash_home" git config ${mode_get} --show-origin xdg.config >actual
> +	) &&
> +	test_cmp expect actual &&
> +
> +	XDG_CONFIG_HOME="$backslash_home\\.config" git config ${mode_get} --show-origin xdg.config >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '--show-origin with default xdg path' '
> +	echo "file:$HOME/.config/git/config	true" >expect &&
> +	git config ${mode_get} --show-origin xdg.config >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'clean up xdg config --show-origin tests' '
> +	rm -rf "$HOME"/.config/git
> +'
> +
>  test_expect_success '--show-scope with --list' '
>  	cat >expect <<-EOF &&
>  	global	user.global=true