[PATCH 0/4] Git for Windows fixes in preparation for 2.8.0

[PATCH 0/4] Git for Windows fixes in preparation for 2.8.0

[Johannes Schindelin]

The t1300 and t9115 tests regressed on Windows. These patches fix that.


Johannes Schindelin (4):
  config --show-origin: report paths with forward slashes
  Make t1300-repo-config resilient to being run via 'sh -x'
  t1300: fix the new --show-origin tests on Windows
  mingw: skip some tests in t9115 due to file name issues

 compat/mingw.h                           |  6 ++++++
 path.c                                   |  3 +++
 t/t1300-repo-config.sh                   | 23 ++++++++++++++---------
 t/t9115-git-svn-dcommit-funky-renames.sh |  4 ++--
 4 files changed, 25 insertions(+), 11 deletions(-)

-- 
2.7.4.windows.1

[PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Schindelin]

On Windows, the backslash is the native directory separator, but all
supported Windows versions also accept the forward slash in most
circumstances.

Our tests expect forward slashes.

Relative paths are generated by Git using forward slashes.

So let's try to be consistent and use forward slashes in the $HOME part
of the paths reported by `git config --show-origin`, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.h | 6 ++++++
 path.c         | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/compat/mingw.h b/compat/mingw.h
index 8c5bf50..c008694 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -396,6 +396,12 @@ static inline char *mingw_find_last_dir_sep(const char *path)
 			ret = (char *)path;
 	return ret;
 }
+static inline void convert_slashes(char *path)
+{
+	for (; *path; path++)
+		if (*path == '\\')
+			*path = '/';
+}
 #define find_last_dir_sep mingw_find_last_dir_sep
 int mingw_offset_1st_component(const char *path);
 #define offset_1st_component mingw_offset_1st_component
diff --git a/path.c b/path.c
index 8b7e168..969b494 100644
--- a/path.c
+++ b/path.c
@@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
 			if (!home)
 				goto return_null;
 			strbuf_addstr(&user_path, home);
+#ifdef GIT_WINDOWS_NATIVE
+			convert_slashes(user_path.buf);
+#endif
 		} else {
 			struct passwd *pw = getpw_str(username, username_len);
 			if (!pw)
-- 
2.7.4.windows.1

[PATCH 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

[Johannes Schindelin]

One of this developer's primary tools to diagnose broken regression
tests is to run the test script using 'sh -x t... -i -v' to find out
*which* call *actually* demonstrates the symptom.

Hence it is pretty counterproductive if the test script behaves
differently when being run via 'sh -x', in particular when using
test_cmp or test_i18ncmp on redirected stderr.

So let's use grep instead of test_cmp/test_i18ncmp to verify that stderr
looks as expected.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1300-repo-config.sh | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 8867ce1..0236fe2 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -699,17 +699,13 @@ test_expect_success 'invalid unit' '
 	echo 1auto >expect &&
 	git config aninvalid.unit >actual &&
 	test_cmp expect actual &&
-	cat >expect <<-\EOF &&
-	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in file .git/config: invalid unit
-	EOF
 	test_must_fail git config --int --get aninvalid.unit 2>actual &&
-	test_i18ncmp expect actual
+	grep "^fatal: bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit$" actual
 '
 
 test_expect_success 'invalid stdin config' '
-	echo "fatal: bad config line 1 in standard input " >expect &&
 	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
-	test_cmp expect output
+	grep "^fatal: bad config line 1 in standard input $" output
 '
 
 cat > expect << EOF
-- 
2.7.4.windows.1

[PATCH 3/4] t1300: fix the new --show-origin tests on Windows

[Johannes Schindelin]

On Windows, we have that funny situation where the test script can refer
to POSIX paths because it runs in a shell that uses a POSIX emulation
layer ("MSYS2 runtime"). Yet, git.exe does *not* understand POSIX paths
at all but only pure Windows paths.

So let's just convert the POSIX paths to Windows paths before passing
them on to Git, using `pwd` (which is already modified on Windows to
output Windows paths).

While fixing the new tests on Windows, we also have to exclude the tests
that want to write a file with a name that is illegal on Windows
(unfortunately, there is more than one test trying to make use of that
file).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1300-repo-config.sh | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 0236fe2..18eb769 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1232,6 +1232,15 @@ test_expect_success 'set up --show-origin tests' '
 	EOF
 '
 
+if test_have_prereq MINGW
+then
+	# convert to Windows paths
+	HOME="$(pwd)"
+	INCLUDE_DIR="$HOME/include"
+	export HOME INCLUDE_DIR
+	git config -f .gitconfig include.path "$INCLUDE_DIR/absolute.include"
+fi
+
 test_expect_success '--show-origin with --list' '
 	cat >expect <<-EOF &&
 		file:$HOME/.gitconfig	user.global=true
@@ -1304,7 +1313,7 @@ test_expect_success 'set up custom config file' '
 	EOF
 '
 
-test_expect_success '--show-origin escape special file name characters' '
+test_expect_success !MINGW '--show-origin escape special file name characters' '
 	cat >expect <<-\EOF &&
 		file:"file\" (dq) and spaces.conf"	user.custom=true
 	EOF
@@ -1333,7 +1342,7 @@ test_expect_success '--show-origin stdin with file include' '
 	test_cmp expect output
 '
 
-test_expect_success '--show-origin blob' '
+test_expect_success !MINGW '--show-origin blob' '
 	cat >expect <<-\EOF &&
 		blob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
 	EOF
@@ -1342,7 +1351,7 @@ test_expect_success '--show-origin blob' '
 	test_cmp expect output
 '
 
-test_expect_success '--show-origin blob ref' '
+test_expect_success !MINGW '--show-origin blob ref' '
 	cat >expect <<-\EOF &&
 		blob:"master:file\" (dq) and spaces.conf"	user.custom=true
 	EOF
-- 
2.7.4.windows.1

[PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Johannes Schindelin]

These two tests wanted to write file names which are incompatible with
Windows' file naming rules.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t9115-git-svn-dcommit-funky-renames.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh
index 0990f8d..864395e 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -93,7 +93,7 @@ test_expect_success 'git svn rebase works inside a fresh-cloned repository' '
 # > to special UNICODE characters in the range 0xf000 to 0xf0ff (the
 # > "Private use area") when creating or accessing files.
 prepare_a_utf8_locale
-test_expect_success UTF8 'svn.pathnameencoding=cp932 new file on dcommit' '
+test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on dcommit' '
 	LC_ALL=$a_utf8_locale &&
 	export LC_ALL &&
 	neq=$(printf "\201\202") &&
@@ -105,7 +105,7 @@ test_expect_success UTF8 'svn.pathnameencoding=cp932 new file on dcommit' '
 '
 
 # See the comment on the above test for setting of LC_ALL.
-test_expect_success 'svn.pathnameencoding=cp932 rename on dcommit' '
+test_expect_success !MINGW 'svn.pathnameencoding=cp932 rename on dcommit' '
 	LC_ALL=$a_utf8_locale &&
 	export LC_ALL &&
 	inf=$(printf "\201\207") &&
-- 
2.7.4.windows.1

Re: [PATCH 0/4] Git for Windows fixes in preparation for 2.8.0

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> The t1300 and t9115 tests regressed on Windows. These patches fix that.

Thanks, it might be probably too late for 2.8.0 final to do
back-and-forth reviews, but lets see how it goes.


>
>
> Johannes Schindelin (4):
>   config --show-origin: report paths with forward slashes
>   Make t1300-repo-config resilient to being run via 'sh -x'
>   t1300: fix the new --show-origin tests on Windows
>   mingw: skip some tests in t9115 due to file name issues
>
>  compat/mingw.h                           |  6 ++++++
>  path.c                                   |  3 +++
>  t/t1300-repo-config.sh                   | 23 ++++++++++++++---------
>  t/t9115-git-svn-dcommit-funky-renames.sh |  4 ++--
>  4 files changed, 25 insertions(+), 11 deletions(-)

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> On Windows, the backslash is the native directory separator, but
> all supported Windows versions also accept the forward slash in
> most circumstances.
>
> Our tests expect forward slashes.
>
> Relative paths are generated by Git using forward slashes.

... and the paths tracked by Git (in the index) use slashes.

> So let's try to be consistent and use forward slashes in the $HOME
> part of the paths reported by `git config --show-origin`, too.

OK, sounds sensible.

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  compat/mingw.h | 6 ++++++
>  path.c         | 3 +++
>  2 files changed, 9 insertions(+)
>
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 8c5bf50..c008694 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -396,6 +396,12 @@ static inline char *mingw_find_last_dir_sep(const char *path)
>  			ret = (char *)path;
>  	return ret;
>  }
> +static inline void convert_slashes(char *path)
> +{
> +	for (; *path; path++)
> +		if (*path == '\\')
> +			*path = '/';
> +}
>  #define find_last_dir_sep mingw_find_last_dir_sep
>  int mingw_offset_1st_component(const char *path);
>  #define offset_1st_component mingw_offset_1st_component
> diff --git a/path.c b/path.c
> index 8b7e168..969b494 100644
> --- a/path.c
> +++ b/path.c
> @@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
>  			if (!home)
>  				goto return_null;
>  			strbuf_addstr(&user_path, home);
> +#ifdef GIT_WINDOWS_NATIVE
> +			convert_slashes(user_path.buf);
> +#endif

Hmm, I wonder if we want to do this at a bit lower level, e.g.

    1. have a fallback for other platforms in git-compat-util.h

    #ifndef get_HOME
    #define get_HOME() getenv("HOME")
    #endif

    2. have the one that does convert-slashes for mingw

    static inline const char *mingw_getenv_HOME(void) {
    	... do convert-slashes on the result of getenv("HOME");
    }
    #define get_HOME() mingw_getenv_HOME()

    3. Instead of the above patch to path.c, change the line before
       the precontext with s/getenv("HOME")/get_HOME()/

Or even lower, inside mingw_getenv() and catch variables that deal
with paths (not just HOME but PATH, TMP, TMPDIR, etc.)

>  		} else {
>  			struct passwd *pw = getpw_str(username, username_len);
>  			if (!pw)

Re: [PATCH 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

[Jonathan Nieder]

Johannes Schindelin wrote:

> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -699,17 +699,13 @@ test_expect_success 'invalid unit' '
>  	echo 1auto >expect &&
>  	git config aninvalid.unit >actual &&
>  	test_cmp expect actual &&
> -	cat >expect <<-\EOF &&
> -	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in file .git/config: invalid unit
> -	EOF
>  	test_must_fail git config --int --get aninvalid.unit 2>actual &&
> -	test_i18ncmp expect actual
> +	grep "^fatal: bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit$" actual

Would test_i18ngrep work?

>  '
>  
>  test_expect_success 'invalid stdin config' '
> -	echo "fatal: bad config line 1 in standard input " >expect &&
>  	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
> -	test_cmp expect output
> +	grep "^fatal: bad config line 1 in standard input $" output

This test is very strange.  Why do we care that it starts with
"fatal:" as opposed to error?  Why are we testing for an extra space at
the end of the line?

I would expect something like

	test_i18ngrep 'line 1 in standard input' output

to be more useful for testing the useful part of the error message while
remaining resilient against error message changes.

Thanks,
Jonathan

Re: [PATCH 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> One of this developer's primary tools to diagnose broken regression
> tests is to run the test script using 'sh -x t... -i -v' to find out
> *which* call *actually* demonstrates the symptom.
>
> Hence it is pretty counterproductive if the test script behaves
> differently when being run via 'sh -x', in particular when using
> test_cmp or test_i18ncmp on redirected stderr.
>
> So let's use grep instead of test_cmp/test_i18ncmp to verify that stderr
> looks as expected.

In the modern world, I would probably described the problem as
"tXXXX -i -v -x", though, not "sh -x tXXXX", but they both exhibit
the same symptom.

I wonder if "tXXXX -i -v -x" can be made not to contaminate the
standard error stream of the test, but that would be a larger change
we probably would not have time for 2.8 final anyway.

Will queue.  Thanks.

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t1300-repo-config.sh | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 8867ce1..0236fe2 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -699,17 +699,13 @@ test_expect_success 'invalid unit' '
>  	echo 1auto >expect &&
>  	git config aninvalid.unit >actual &&
>  	test_cmp expect actual &&
> -	cat >expect <<-\EOF &&
> -	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in file .git/config: invalid unit
> -	EOF
>  	test_must_fail git config --int --get aninvalid.unit 2>actual &&
> -	test_i18ncmp expect actual
> +	grep "^fatal: bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit$" actual
>  '
>  
>  test_expect_success 'invalid stdin config' '
> -	echo "fatal: bad config line 1 in standard input " >expect &&
>  	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
> -	test_cmp expect output
> +	grep "^fatal: bad config line 1 in standard input $" output
>  '
>  
>  cat > expect << EOF

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Jonathan Nieder]

Johannes Schindelin wrote:

> These two tests wanted to write file names which are incompatible with
> Windows' file naming rules.

Makes sense.  Ideally these would use the FUNNYNAMES prereq which
would be factored out as a lazy prereq from t3600, t4135, and t9903.
But using !MINGW is simpler and works now.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 3/4] t1300: fix the new --show-origin tests on Windows

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> On Windows, we have that funny situation where the test script can refer
> to POSIX paths because it runs in a shell that uses a POSIX emulation
> layer ("MSYS2 runtime"). Yet, git.exe does *not* understand POSIX paths
> at all but only pure Windows paths.
>
> So let's just convert the POSIX paths to Windows paths before passing
> them on to Git, using `pwd` (which is already modified on Windows to
> output Windows paths).
>
> While fixing the new tests on Windows, we also have to exclude the tests
> that want to write a file with a name that is illegal on Windows
> (unfortunately, there is more than one test trying to make use of that
> file).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t1300-repo-config.sh | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 0236fe2..18eb769 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1232,6 +1232,15 @@ test_expect_success 'set up --show-origin tests' '
>  	EOF
>  '
>  
> +if test_have_prereq MINGW
> +then
> +	# convert to Windows paths
> +	HOME="$(pwd)"

We override HOME in t/test-lib.sh; shouldn't this be done there?

> +	INCLUDE_DIR="$HOME/include"

I am puzzled. 'set up --show-origin tests' do say INCLUDE_DIR is
"$HOME/include" already, so why is this needed?

> +	export HOME INCLUDE_DIR

Existing tests use $INCLUDE_DIR (and $HOME) as shell variables just
fine without exporting.  Why do these need to be exported only on
MINGW?

> +	git config -f .gitconfig include.path "$INCLUDE_DIR/absolute.include"
> +fi

Perhaps if you adjust HOME before 'set up --show-origin tests' test,
most (or all) of the above become unnecessary?

The changes below that skip tests that relies on pathnames that
cannot be used on Windows makes sense, though.

>  test_expect_success '--show-origin with --list' '
>  	cat >expect <<-EOF &&
>  		file:$HOME/.gitconfig	user.global=true
> @@ -1304,7 +1313,7 @@ test_expect_success 'set up custom config file' '
>  	EOF
>  '
>  
> -test_expect_success '--show-origin escape special file name characters' '
> +test_expect_success !MINGW '--show-origin escape special file name characters' '
>  	cat >expect <<-\EOF &&
>  		file:"file\" (dq) and spaces.conf"	user.custom=true
>  	EOF
> @@ -1333,7 +1342,7 @@ test_expect_success '--show-origin stdin with file include' '
>  	test_cmp expect output
>  '
>  
> -test_expect_success '--show-origin blob' '
> +test_expect_success !MINGW '--show-origin blob' '
>  	cat >expect <<-\EOF &&
>  		blob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
>  	EOF
> @@ -1342,7 +1351,7 @@ test_expect_success '--show-origin blob' '
>  	test_cmp expect output
>  '
>  
> -test_expect_success '--show-origin blob ref' '
> +test_expect_success !MINGW '--show-origin blob ref' '
>  	cat >expect <<-\EOF &&
>  		blob:"master:file\" (dq) and spaces.conf"	user.custom=true
>  	EOF

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Torsten Bögershausen]

On 2016-03-22 18.43, Johannes Schindelin wrote:
> These two tests wanted to write file names which are incompatible with
> Windows' file naming rules.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Is there a chance to squeeze in a precondition for HFS under Mac OS ?
> -test_expect_success UTF8 'svn.pathnameencoding=cp932 new file on dcommit' '
> +test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on dcommit' '
+test_expect_success UTF8,!MINGW,!UTF8_NFD_TO_NFC

>  	LC_ALL=$a_utf8_locale &&
>  	export LC_ALL &&
>  	neq=$(printf "\201\202") &&
> @@ -105,7 +105,7 @@ test_expect_success UTF8 'svn.pathnameencoding=cp932 new file on dcommit' '
>  '
>  
>  # See the comment on the above test for setting of LC_ALL.
> -test_expect_success 'svn.pathnameencoding=cp932 rename on dcommit' '
> +test_expect_success !MINGW 'svn.pathnameencoding=cp932 rename on dcommit' '
And the same protection for HFS here.

Question: Why do we need a UTF8 protection in #11, but not in #12 ?



I just tested t9115 under german cygwin, passes.

http://article.gmane.org/gmane.comp.version-control.git/288827

Re: [PATCH 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Johannes Schindelin wrote:
>
>> --- a/t/t1300-repo-config.sh
>> +++ b/t/t1300-repo-config.sh
>> @@ -699,17 +699,13 @@ test_expect_success 'invalid unit' '
>>  	echo 1auto >expect &&
>>  	git config aninvalid.unit >actual &&
>>  	test_cmp expect actual &&
>> -	cat >expect <<-\EOF &&
>> -	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in file .git/config: invalid unit
>> -	EOF
>>  	test_must_fail git config --int --get aninvalid.unit 2>actual &&
>> -	test_i18ncmp expect actual
>> +	grep "^fatal: bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit$" actual
>
> Would test_i18ngrep work?
>
>>  '
>>  
>>  test_expect_success 'invalid stdin config' '
>> -	echo "fatal: bad config line 1 in standard input " >expect &&
>>  	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
>> -	test_cmp expect output
>> +	grep "^fatal: bad config line 1 in standard input $" output
>
> This test is very strange.  Why do we care that it starts with
> "fatal:" as opposed to error?  Why are we testing for an extra space at
> the end of the line?
>
> I would expect something like
>
> 	test_i18ngrep 'line 1 in standard input' output
>
> to be more useful for testing the useful part of the error message while
> remaining resilient against error message changes.

Both sounds sensible.  Should we squash this in, then?

 t/t1300-repo-config.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 0236fe2..dca27a3 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -700,12 +700,12 @@ test_expect_success 'invalid unit' '
 	git config aninvalid.unit >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config --int --get aninvalid.unit 2>actual &&
-	grep "^fatal: bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit$" actual
+	test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
 '
 
 test_expect_success 'invalid stdin config' '
 	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
-	grep "^fatal: bad config line 1 in standard input $" output
+	test_i18ngrep "bad config line 1 in standard input" output
 '
 
 cat > expect << EOF

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 2016-03-22 18.43, Johannes Schindelin wrote:
>> These two tests wanted to write file names which are incompatible with
>> Windows' file naming rules.
>> 
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Is there a chance to squeeze in a precondition for HFS under Mac OS ?

So you want this squashed into it?

 t/t9115-git-svn-dcommit-funky-renames.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh
index 864395e..a87d3d3 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -93,7 +93,7 @@ test_expect_success 'git svn rebase works inside a fresh-cloned repository' '
 # > to special UNICODE characters in the range 0xf000 to 0xf0ff (the
 # > "Private use area") when creating or accessing files.
 prepare_a_utf8_locale
-test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on dcommit' '
+test_expect_success UTF8,!MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 new file on dcommit' '
 	LC_ALL=$a_utf8_locale &&
 	export LC_ALL &&
 	neq=$(printf "\201\202") &&
@@ -105,7 +105,7 @@ test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on dcommit'
 '
 
 # See the comment on the above test for setting of LC_ALL.
-test_expect_success !MINGW 'svn.pathnameencoding=cp932 rename on dcommit' '
+test_expect_success !MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 rename on dcommit' '
 	LC_ALL=$a_utf8_locale &&
 	export LC_ALL &&
 	inf=$(printf "\201\207") &&

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Torsten Bögershausen]

On 2016-03-22 21.44, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> On 2016-03-22 18.43, Johannes Schindelin wrote:
>>> These two tests wanted to write file names which are incompatible with
>>> Windows' file naming rules.
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> Is there a chance to squeeze in a precondition for HFS under Mac OS ?
> 
> So you want this squashed into it?
Yes, please.
> 
>  t/t9115-git-svn-dcommit-funky-renames.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh
> index 864395e..a87d3d3 100755
> --- a/t/t9115-git-svn-dcommit-funky-renames.sh
> +++ b/t/t9115-git-svn-dcommit-funky-renames.sh
> @@ -93,7 +93,7 @@ test_expect_success 'git svn rebase works inside a fresh-cloned repository' '
>  # > to special UNICODE characters in the range 0xf000 to 0xf0ff (the
>  # > "Private use area") when creating or accessing files.
>  prepare_a_utf8_locale
> -test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on dcommit' '
> +test_expect_success UTF8,!MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 new file on dcommit' '
>  	LC_ALL=$a_utf8_locale &&
>  	export LC_ALL &&
>  	neq=$(printf "\201\202") &&
> @@ -105,7 +105,7 @@ test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on dcommit'
>  '
>  
>  # See the comment on the above test for setting of LC_ALL.
> -test_expect_success !MINGW 'svn.pathnameencoding=cp932 rename on dcommit' '
> +test_expect_success !MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 rename on dcommit' '
>  	LC_ALL=$a_utf8_locale &&
>  	export LC_ALL &&
>  	inf=$(printf "\201\207") &&

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> On 2016-03-22 21.44, Junio C Hamano wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>> 
>>> On 2016-03-22 18.43, Johannes Schindelin wrote:
>>>> These two tests wanted to write file names which are incompatible with
>>>> Windows' file naming rules.
>>>>
>>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> Is there a chance to squeeze in a precondition for HFS under Mac OS ?
>> 
>> So you want this squashed into it?
> Yes, please.

Dscho, I queued two out of these four, with a proposed fix-up patch
for each of them, on 'pu'; but I won't squash them together myself
without hearing from you as I do not test mingw or macosx.

Thanks.

Re: [PATCH 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

[Jonathan Nieder]

Junio C Hamano wrote:

> Both sounds sensible.  Should we squash this in, then?
>
>  t/t1300-repo-config.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Torsten Bögershausen]

On 2016-03-22 23.57, Junio C Hamano wrote:
> Dscho, I queued two out of these four, with a proposed fix-up patch
> for each of them, on 'pu'; but I won't squash them together myself
> without hearing from you as I do not test mingw or macosx.
> 
> Thanks.

the queued t9115 passes under Mac OS X (#11 and #12 are skipped as expected)

----------------
Beside that, do we want to amend the commit message like this:

Author: Johannes Schindelin <johannes.schindelin@gmx.de>
Date:   Tue Mar 22 18:43:00 2016 +0100

    skip some tests in t9115 due to file name issues

    These two tests wanted to write file names which work under Linux or
    CYGWIN, but are incompatible with file naming rules under mingw or HFS.

    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Torsten Bögershausen <tboegi@web.de>
    Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>
----------------

http://thread.gmane.org/gmane.comp.version-control.git/285776/focus=287717

Re: [PATCH 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

[Johannes Schindelin]

Hi,

On Tue, 22 Mar 2016, Jonathan Nieder wrote:

> Junio C Hamano wrote:
> 
> > Both sounds sensible.  Should we squash this in, then?
> >
> >  t/t1300-repo-config.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Sure, makes sense. That late in the 2.8.0 game I tried to err on the more
faithful side, hence I kept testing for "fatal:" but if you two are fine
with dropping it...

Ciao,
Dscho

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Schindelin]

Hi Junio,

On Tue, 22 Mar 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > On Windows, the backslash is the native directory separator, but
> > all supported Windows versions also accept the forward slash in
> > most circumstances.
> >
> > Our tests expect forward slashes.
> >
> > Relative paths are generated by Git using forward slashes.
> 
> ... and the paths tracked by Git (in the index) use slashes.
> 
> > So let's try to be consistent and use forward slashes in the $HOME
> > part of the paths reported by `git config --show-origin`, too.
> 
> OK, sounds sensible.
> 
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  compat/mingw.h | 6 ++++++
> >  path.c         | 3 +++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/compat/mingw.h b/compat/mingw.h
> > index 8c5bf50..c008694 100644
> > --- a/compat/mingw.h
> > +++ b/compat/mingw.h
> > @@ -396,6 +396,12 @@ static inline char *mingw_find_last_dir_sep(const char *path)
> >  			ret = (char *)path;
> >  	return ret;
> >  }
> > +static inline void convert_slashes(char *path)
> > +{
> > +	for (; *path; path++)
> > +		if (*path == '\\')
> > +			*path = '/';
> > +}
> >  #define find_last_dir_sep mingw_find_last_dir_sep
> >  int mingw_offset_1st_component(const char *path);
> >  #define offset_1st_component mingw_offset_1st_component
> > diff --git a/path.c b/path.c
> > index 8b7e168..969b494 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
> >  			if (!home)
> >  				goto return_null;
> >  			strbuf_addstr(&user_path, home);
> > +#ifdef GIT_WINDOWS_NATIVE
> > +			convert_slashes(user_path.buf);
> > +#endif
> 
> Hmm, I wonder if we want to do this at a bit lower level,

Well, I tried to be careful. There *are* circumstamces when backslashes
are required (CreateProcess() comes to mind), so I wanted to have this
conversion as much only in the user-visible output as possible.

> e.g.
> 
>     1. have a fallback for other platforms in git-compat-util.h
> 
>     #ifndef get_HOME
>     #define get_HOME() getenv("HOME")
>     #endif

Once upon a time, I had something like that (without the ugly uppercase,
to account for the fact that Windows has no single HOME setting):

	http://thread.gmane.org/gmane.comp.version-control.msysgit/20339

>     2. have the one that does convert-slashes for mingw
> 
>     static inline const char *mingw_getenv_HOME(void) {
>     	... do convert-slashes on the result of getenv("HOME");
>     }
>     #define get_HOME() mingw_getenv_HOME()

We could do that much easier now:

-- snip --
diff --git a/compat/mingw.c b/compat/mingw.c
index 54c82ec..96022b7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2117,6 +2117,12 @@ static void setup_windows_environment()
 				*tmp = '/';
 	}
 
+	tmp = getenv("HOME");
+	if (tmp)
+		for (; *tmp; tmp++)
+			if (*tmp == '\\')
+				*tmp = '/';
+
 	/* simulate TERM to enable auto-color (see color.c) */
 	if (!getenv("TERM"))
 		setenv("TERM", "cygwin", 1);
-- snap --

But again, I am uncomfortable to do this wholesale without having the time
to do anything quickly about unintended side effects.


>     3. Instead of the above patch to path.c, change the line before
>        the precontext with s/getenv("HOME")/get_HOME()/
> 
> Or even lower, inside mingw_getenv() and catch variables that deal
> with paths (not just HOME but PATH, TMP, TMPDIR, etc.)

That outright scares me. Don't do that.

:-)

Again, just to make sure my point is heard: there *are* circumstances when
the Win32 API expects backslashes and cannot handle forward ones. I would
not dare to claim to be an expert in that matter, so I would rather want
to err on the side of converting backslashes to forward slashes *only*
when really needed.

Ciao,
Dscho

Re: [PATCH 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

[Johannes Schindelin]

Hi Junio,

On Tue, 22 Mar 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > One of this developer's primary tools to diagnose broken regression
> > tests is to run the test script using 'sh -x t... -i -v' to find out
> > *which* call *actually* demonstrates the symptom.
> >
> > Hence it is pretty counterproductive if the test script behaves
> > differently when being run via 'sh -x', in particular when using
> > test_cmp or test_i18ncmp on redirected stderr.
> >
> > So let's use grep instead of test_cmp/test_i18ncmp to verify that stderr
> > looks as expected.
> 
> In the modern world, I would probably described the problem as
> "tXXXX -i -v -x", though, not "sh -x tXXXX", but they both exhibit
> the same symptom.

Thanks for pointing me to -i -v -x. The introduction of the RHS -x somehow
slipped by me.

> I wonder if "tXXXX -i -v -x" can be made not to contaminate the
> standard error stream of the test, but that would be a larger change
> we probably would not have time for 2.8 final anyway.

Agree, both on "we want to have this" and "after 2.8".

Ciao,
Dscho

Re: [PATCH 3/4] t1300: fix the new --show-origin tests on Windows

[Johannes Schindelin]

Hi Junio,

On Tue, 22 Mar 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > On Windows, we have that funny situation where the test script can refer
> > to POSIX paths because it runs in a shell that uses a POSIX emulation
> > layer ("MSYS2 runtime"). Yet, git.exe does *not* understand POSIX paths
> > at all but only pure Windows paths.
> >
> > So let's just convert the POSIX paths to Windows paths before passing
> > them on to Git, using `pwd` (which is already modified on Windows to
> > output Windows paths).
> >
> > While fixing the new tests on Windows, we also have to exclude the tests
> > that want to write a file with a name that is illegal on Windows
> > (unfortunately, there is more than one test trying to make use of that
> > file).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/t1300-repo-config.sh | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > index 0236fe2..18eb769 100755
> > --- a/t/t1300-repo-config.sh
> > +++ b/t/t1300-repo-config.sh
> > @@ -1232,6 +1232,15 @@ test_expect_success 'set up --show-origin tests' '
> >  	EOF
> >  '
> >  
> > +if test_have_prereq MINGW
> > +then
> > +	# convert to Windows paths
> > +	HOME="$(pwd)"
> 
> We override HOME in t/test-lib.sh; shouldn't this be done there?

We override it with $PWD.

Remember, on Windows we have this funny situation where the shell,
Perl, and the Unix tools used in scripting, know about POSIX paths, but
little else. Most notably git.exe does *not* understand them [*1*].

The difference between $PWD and $(pwd) is, you guessed it, POSIX path vs
Windows path, respectively. And since *some* of our tests verify
shell/Perl scripts' correct behavior, we *want* $HOME to be a POSIX path,
at least some of the time.

> > +	INCLUDE_DIR="$HOME/include"
> 
> I am puzzled. 'set up --show-origin tests' do say INCLUDE_DIR is
> "$HOME/include" already, so why is this needed?
> 
> > +	export HOME INCLUDE_DIR
> 
> Existing tests use $INCLUDE_DIR (and $HOME) as shell variables just
> fine without exporting.  Why do these need to be exported only on
> MINGW?

Habit. The export is actually not needed at all, you are totally correct.

> > +	git config -f .gitconfig include.path "$INCLUDE_DIR/absolute.include"
> > +fi
> 
> Perhaps if you adjust HOME before 'set up --show-origin tests' test,
> most (or all) of the above become unnecessary?

It did not even occur to me, thanks for that suggestion. It works
perfectly. Will send out v2 in a moment.

Ciao,
Dscho

Footnote [*1*]: we do have this hack, system_path(), that can turn "POSIX
paths" into Windows paths. However, it actually turns paths relative to
the prefix (as in "/usr/") into Windows paths, and the prefix is
determined at runtime, from the location of git.exe. When the test suite
runs, the location of git.exe is most definitely *not* related to any
sensible prefix, therefore we simply cannot expect git.exe to handle POSIX
paths correctly in the test suite.

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

Hi Junio & Torsten,

On Wed, 23 Mar 2016, Torsten Bögershausen wrote:

> On 2016-03-22 23.57, Junio C Hamano wrote:
> > Dscho, I queued two out of these four, with a proposed fix-up patch
> > for each of them, on 'pu'; but I won't squash them together myself
> > without hearing from you as I do not test mingw or macosx.

The suggested changes will be squashed into v2.

> Beside that, do we want to amend the commit message like this:
> 
> Author: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date:   Tue Mar 22 18:43:00 2016 +0100
> 
>     skip some tests in t9115 due to file name issues
> 
>     These two tests wanted to write file names which work under Linux or
>     CYGWIN, but are incompatible with file naming rules under mingw or HFS.
> 
>     Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>     Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>     Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks, I used a slightly different version, as I had crafted it before
reading this mail already.

Ciao,
Dscho

[PATCH v2 0/4] Git for Windows fixes in preparation for 2.8.0

[Johannes Schindelin]

The t1300 and t9115 tests regressed on Windows. These patches fix that.


Johannes Schindelin (4):
  config --show-origin: report paths with forward slashes
  Make t1300-repo-config resilient to being run via 'sh -x'
  t1300: fix the new --show-origin tests on Windows
  mingw: skip some tests in t9115 due to file name issues

 compat/mingw.h                           |  6 ++++++
 path.c                                   |  3 +++
 t/t1300-repo-config.sh                   | 18 +++++++++---------
 t/t9115-git-svn-dcommit-funky-renames.sh |  4 ++--
 4 files changed, 20 insertions(+), 11 deletions(-)

Interdiff vs v1:

 diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
 index 18eb769..a37ebb2 100755
 --- a/t/t1300-repo-config.sh
 +++ b/t/t1300-repo-config.sh
 @@ -700,12 +700,12 @@ test_expect_success 'invalid unit' '
  	git config aninvalid.unit >actual &&
  	test_cmp expect actual &&
  	test_must_fail git config --int --get aninvalid.unit 2>actual &&
 -	grep "^fatal: bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit$" actual
 +	test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
  '
  
  test_expect_success 'invalid stdin config' '
  	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
 -	grep "^fatal: bad config line 1 in standard input $" output
 +	test_i18ngrep "bad config line 1 in standard input" output
  '
  
  cat > expect << EOF
 @@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
  	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
  '
  
 +! test_have_prereq MINGW ||
 +HOME="$(pwd)" # convert to Windows path
 +
  test_expect_success 'set up --show-origin tests' '
  	INCLUDE_DIR="$HOME/include" &&
  	mkdir -p "$INCLUDE_DIR" &&
 @@ -1232,14 +1235,6 @@ test_expect_success 'set up --show-origin tests' '
  	EOF
  '
  
 -if test_have_prereq MINGW
 -then
 -	# convert to Windows paths
 -	HOME="$(pwd)"
 -	INCLUDE_DIR="$HOME/include"
 -	export HOME INCLUDE_DIR
 -	git config -f .gitconfig include.path "$INCLUDE_DIR/absolute.include"
 -fi
  
  test_expect_success '--show-origin with --list' '
  	cat >expect <<-EOF &&
 diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh
 index 864395e..a87d3d3 100755
 --- a/t/t9115-git-svn-dcommit-funky-renames.sh
 +++ b/t/t9115-git-svn-dcommit-funky-renames.sh
 @@ -93,7 +93,7 @@ test_expect_success 'git svn rebase works inside a fresh-cloned repository' '
  # > to special UNICODE characters in the range 0xf000 to 0xf0ff (the
  # > "Private use area") when creating or accessing files.
  prepare_a_utf8_locale
 -test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on dcommit' '
 +test_expect_success UTF8,!MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 new file on dcommit' '
  	LC_ALL=$a_utf8_locale &&
  	export LC_ALL &&
  	neq=$(printf "\201\202") &&
 @@ -105,7 +105,7 @@ test_expect_success UTF8,!MINGW 'svn.pathnameencoding=cp932 new file on dcommit'
  '
  
  # See the comment on the above test for setting of LC_ALL.
 -test_expect_success !MINGW 'svn.pathnameencoding=cp932 rename on dcommit' '
 +test_expect_success !MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 rename on dcommit' '
  	LC_ALL=$a_utf8_locale &&
  	export LC_ALL &&
  	inf=$(printf "\201\207") &&

-- 
2.7.4.windows.1

[PATCH v2 1/4] config --show-origin: report paths with forward slashes

[Johannes Schindelin]

On Windows, the backslash is the native directory separator, but all
supported Windows versions also accept the forward slash in most
circumstances.

Our tests expect forward slashes.

Relative paths are generated by Git using forward slashes.

So let's try to be consistent and use forward slashes in the $HOME part
of the paths reported by `git config --show-origin`, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/mingw.h | 6 ++++++
 path.c         | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/compat/mingw.h b/compat/mingw.h
index 8c5bf50..c008694 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -396,6 +396,12 @@ static inline char *mingw_find_last_dir_sep(const char *path)
 			ret = (char *)path;
 	return ret;
 }
+static inline void convert_slashes(char *path)
+{
+	for (; *path; path++)
+		if (*path == '\\')
+			*path = '/';
+}
 #define find_last_dir_sep mingw_find_last_dir_sep
 int mingw_offset_1st_component(const char *path);
 #define offset_1st_component mingw_offset_1st_component
diff --git a/path.c b/path.c
index 8b7e168..969b494 100644
--- a/path.c
+++ b/path.c
@@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
 			if (!home)
 				goto return_null;
 			strbuf_addstr(&user_path, home);
+#ifdef GIT_WINDOWS_NATIVE
+			convert_slashes(user_path.buf);
+#endif
 		} else {
 			struct passwd *pw = getpw_str(username, username_len);
 			if (!pw)
-- 
2.7.4.windows.1

[PATCH v2 2/4] Make t1300-repo-config resilient to being run via 'sh -x'

[Johannes Schindelin]

One of this developer's primary tools to diagnose broken regression
tests is to run the test script using 'sh -x t... -i -v' to find out
*which* call *actually* demonstrates the symptom.

Hence it is pretty counterproductive if the test script behaves
differently when being run via 'sh -x', in particular when using
test_cmp or test_i18ncmp on redirected stderr.

So let's use test_i18ngrep (as suggested by Jonathan Nieder) instead of
test_cmp/test_i18ncmp to verify that stderr looks as expected.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1300-repo-config.sh | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 8867ce1..dca27a3 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -699,17 +699,13 @@ test_expect_success 'invalid unit' '
 	echo 1auto >expect &&
 	git config aninvalid.unit >actual &&
 	test_cmp expect actual &&
-	cat >expect <<-\EOF &&
-	fatal: bad numeric config value '\''1auto'\'' for '\''aninvalid.unit'\'' in file .git/config: invalid unit
-	EOF
 	test_must_fail git config --int --get aninvalid.unit 2>actual &&
-	test_i18ncmp expect actual
+	test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
 '
 
 test_expect_success 'invalid stdin config' '
-	echo "fatal: bad config line 1 in standard input " >expect &&
 	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
-	test_cmp expect output
+	test_i18ngrep "bad config line 1 in standard input" output
 '
 
 cat > expect << EOF
-- 
2.7.4.windows.1

[PATCH v2 3/4] t1300: fix the new --show-origin tests on Windows

[Johannes Schindelin]

On Windows, we have that funny situation where the test script can refer
to POSIX paths because it runs in a shell that uses a POSIX emulation
layer ("MSYS2 runtime"). Yet, git.exe does *not* understand POSIX paths
at all but only pure Windows paths.

So let's just convert the POSIX paths to Windows paths before passing
them on to Git, using `pwd` (which is already modified on Windows to
output Windows paths).

While fixing the new tests on Windows, we also have to exclude the tests
that want to write a file with a name that is illegal on Windows
(unfortunately, there is more than one test trying to make use of that
file).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1300-repo-config.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index dca27a3..a37ebb2 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
+! test_have_prereq MINGW ||
+HOME="$(pwd)" # convert to Windows path
+
 test_expect_success 'set up --show-origin tests' '
 	INCLUDE_DIR="$HOME/include" &&
 	mkdir -p "$INCLUDE_DIR" &&
@@ -1232,6 +1235,7 @@ test_expect_success 'set up --show-origin tests' '
 	EOF
 '
 
+
 test_expect_success '--show-origin with --list' '
 	cat >expect <<-EOF &&
 		file:$HOME/.gitconfig	user.global=true
@@ -1304,7 +1308,7 @@ test_expect_success 'set up custom config file' '
 	EOF
 '
 
-test_expect_success '--show-origin escape special file name characters' '
+test_expect_success !MINGW '--show-origin escape special file name characters' '
 	cat >expect <<-\EOF &&
 		file:"file\" (dq) and spaces.conf"	user.custom=true
 	EOF
@@ -1333,7 +1337,7 @@ test_expect_success '--show-origin stdin with file include' '
 	test_cmp expect output
 '
 
-test_expect_success '--show-origin blob' '
+test_expect_success !MINGW '--show-origin blob' '
 	cat >expect <<-\EOF &&
 		blob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
 	EOF
@@ -1342,7 +1346,7 @@ test_expect_success '--show-origin blob' '
 	test_cmp expect output
 '
 
-test_expect_success '--show-origin blob ref' '
+test_expect_success !MINGW '--show-origin blob ref' '
 	cat >expect <<-\EOF &&
 		blob:"master:file\" (dq) and spaces.conf"	user.custom=true
 	EOF
-- 
2.7.4.windows.1

[PATCH v2 4/4] mingw: skip some tests in t9115 due to file name issues

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

These two tests wanted to write file names which are incompatible with
Windows' file naming rules (even if they pass using Cygwin due to
Cygwin's magic path mangling).

While at it, skip the same tests also on MacOSX/HFS, as pointed out by
Torsten Bögershausen.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t9115-git-svn-dcommit-funky-renames.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh b/t/t9115-git-svn-dcommit-funky-renames.sh
index 0990f8d..a87d3d3 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -93,7 +93,7 @@ test_expect_success 'git svn rebase works inside a fresh-cloned repository' '
 # > to special UNICODE characters in the range 0xf000 to 0xf0ff (the
 # > "Private use area") when creating or accessing files.
 prepare_a_utf8_locale
-test_expect_success UTF8 'svn.pathnameencoding=cp932 new file on dcommit' '
+test_expect_success UTF8,!MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 new file on dcommit' '
 	LC_ALL=$a_utf8_locale &&
 	export LC_ALL &&
 	neq=$(printf "\201\202") &&
@@ -105,7 +105,7 @@ test_expect_success UTF8 'svn.pathnameencoding=cp932 new file on dcommit' '
 '
 
 # See the comment on the above test for setting of LC_ALL.
-test_expect_success 'svn.pathnameencoding=cp932 rename on dcommit' '
+test_expect_success !MINGW,!UTF8_NFD_TO_NFC 'svn.pathnameencoding=cp932 rename on dcommit' '
 	LC_ALL=$a_utf8_locale &&
 	export LC_ALL &&
 	inf=$(printf "\201\207") &&
-- 
2.7.4.windows.1

Re: [PATCH v2 3/4] t1300: fix the new --show-origin tests on Windows

[Lars Schneider]

On 23 Mar 2016, at 11:55, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

> On Windows, we have that funny situation where the test script can refer
> to POSIX paths because it runs in a shell that uses a POSIX emulation
> layer ("MSYS2 runtime"). Yet, git.exe does *not* understand POSIX paths
> at all but only pure Windows paths.
> 
> So let's just convert the POSIX paths to Windows paths before passing
> them on to Git, using `pwd` (which is already modified on Windows to
> output Windows paths).
> 
> While fixing the new tests on Windows, we also have to exclude the tests
> that want to write a file with a name that is illegal on Windows
> (unfortunately, there is more than one test trying to make use of that
> file).

Thanks for these Windows fixes! After the 2.8 release I will try to post 
a patch that uses a different filename where possible.

Cheers,
Lars

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> t/t1300-repo-config.sh | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index dca27a3..a37ebb2 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
> 	  "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
> '
> 
> +! test_have_prereq MINGW ||
> +HOME="$(pwd)" # convert to Windows path
> +
> test_expect_success 'set up --show-origin tests' '
> 	INCLUDE_DIR="$HOME/include" &&
> 	mkdir -p "$INCLUDE_DIR" &&
> @@ -1232,6 +1235,7 @@ test_expect_success 'set up --show-origin tests' '
> 	EOF
> '
> 
> +
> test_expect_success '--show-origin with --list' '
> 	cat >expect <<-EOF &&
> 		file:$HOME/.gitconfig	user.global=true
> @@ -1304,7 +1308,7 @@ test_expect_success 'set up custom config file' '
> 	EOF
> '
> 
> -test_expect_success '--show-origin escape special file name characters' '
> +test_expect_success !MINGW '--show-origin escape special file name characters' '
> 	cat >expect <<-\EOF &&
> 		file:"file\" (dq) and spaces.conf"	user.custom=true
> 	EOF
> @@ -1333,7 +1337,7 @@ test_expect_success '--show-origin stdin with file include' '
> 	test_cmp expect output
> '
> 
> -test_expect_success '--show-origin blob' '
> +test_expect_success !MINGW '--show-origin blob' '
> 	cat >expect <<-\EOF &&
> 		blob:a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08	user.custom=true
> 	EOF
> @@ -1342,7 +1346,7 @@ test_expect_success '--show-origin blob' '
> 	test_cmp expect output
> '
> 
> -test_expect_success '--show-origin blob ref' '
> +test_expect_success !MINGW '--show-origin blob ref' '
> 	cat >expect <<-\EOF &&
> 		blob:"master:file\" (dq) and spaces.conf"	user.custom=true
> 	EOF
> -- 
> 2.7.4.windows.1
> 
> 

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio & Torsten,
>
> On Wed, 23 Mar 2016, Torsten Bögershausen wrote:
>
>> On 2016-03-22 23.57, Junio C Hamano wrote:
>> > Dscho, I queued two out of these four, with a proposed fix-up patch
>> > for each of them, on 'pu'; but I won't squash them together myself
>> > without hearing from you as I do not test mingw or macosx.
>
> The suggested changes will be squashed into v2.
>
>> Beside that, do we want to amend the commit message like this:
>> 
>> Author: Johannes Schindelin <johannes.schindelin@gmx.de>
>> Date:   Tue Mar 22 18:43:00 2016 +0100
>> 
>>     skip some tests in t9115 due to file name issues
>> 
>>     These two tests wanted to write file names which work under Linux or
>>     CYGWIN, but are incompatible with file naming rules under mingw or HFS.
>> 
>>     Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>     Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>>     Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Thanks, I used a slightly different version, as I had crafted it before
> reading this mail already.

Thanks; Torsten, sorry but could you do another round of check, please?

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > diff --git a/path.c b/path.c
>> > index 8b7e168..969b494 100644
>> > --- a/path.c
>> > +++ b/path.c
>> > @@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
>> >  			if (!home)
>> >  				goto return_null;
>> >  			strbuf_addstr(&user_path, home);
>> > +#ifdef GIT_WINDOWS_NATIVE
>> > +			convert_slashes(user_path.buf);
>> > +#endif
>> 
>> Hmm, I wonder if we want to do this at a bit lower level,
>
> Well, I tried to be careful. There *are* circumstamces when backslashes
> are required (CreateProcess() comes to mind), so I wanted to have this
> conversion as much only in the user-visible output as possible.

I was able to guess that it would be the reason, and I was willing
to accept this as a short-term workaround.

As you are very well aware, the usual pattern we use is to implement
a higher level function (e.g. expand_user_path() in this case) in
terms of helpers that offer abstraction of implementation details
that may be platform specific (e.g. getenv() may be implemented
differently on Windows).  An "#ifdef" in otherwise platform agnostic
codepath like this one is a sign that the code is not well thought
out to find the right abstraction to use to follow that pattern.

I was mostly reacting to that "#ifdef" and thinking aloud what the
right abstraction is appropriate.  As a short-term workaround, the
above would have to do.

And no, I do not think convert_slashes() that becomes no-op on
non-windows platforms is the right abstraction.

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Torsten Bögershausen]

On 2016-03-23 16.56, Junio C Hamano wrote:

>> Thanks, I used a slightly different version, as I had crafted it before
>> reading this mail already.
> 
> Thanks; Torsten, sorry but could you do another round of check, please?
Sure :-)

Re: [PATCH 4/4] mingw: skip some tests in t9115 due to file name issues

[Torsten Bögershausen]

> 
> Thanks; Torsten, sorry but could you do another round of check, please?
Thanks, v2 tested OK under Mac OS and Linux

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Sixt]

Am 22.03.2016 um 18:42 schrieb Johannes Schindelin:
> On Windows, the backslash is the native directory separator, but all
> supported Windows versions also accept the forward slash in most
> circumstances.
> 
> Our tests expect forward slashes.
> 
> Relative paths are generated by Git using forward slashes.
> 
> So let's try to be consistent and use forward slashes in the $HOME part
> of the paths reported by `git config --show-origin`, too.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/mingw.h | 6 ++++++
>   path.c         | 3 +++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 8c5bf50..c008694 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -396,6 +396,12 @@ static inline char *mingw_find_last_dir_sep(const char *path)
>   			ret = (char *)path;
>   	return ret;
>   }
> +static inline void convert_slashes(char *path)
> +{
> +	for (; *path; path++)
> +		if (*path == '\\')
> +			*path = '/';
> +}
>   #define find_last_dir_sep mingw_find_last_dir_sep
>   int mingw_offset_1st_component(const char *path);
>   #define offset_1st_component mingw_offset_1st_component
> diff --git a/path.c b/path.c
> index 8b7e168..969b494 100644
> --- a/path.c
> +++ b/path.c
> @@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
>   			if (!home)
>   				goto return_null;
>   			strbuf_addstr(&user_path, home);
> +#ifdef GIT_WINDOWS_NATIVE
> +			convert_slashes(user_path.buf);
> +#endif
>   		} else {
>   			struct passwd *pw = getpw_str(username, username_len);
>   			if (!pw)
> 

This change is not warranted for the following reasons:

- It is only there to satisfy the --show-origin tests, but not
  for the benefit of the users.

- The use of $HOME in those tests is just incidental, but not necessary.

- There is no reason to change only paths depending on $HOME,
  but no other paths imported through the environment.

I see no advantage for the users of --show-origin that they now
see C:/foo/bar instead of C:\foo\bar. (For this reason, I'm not
suggesting to change all paths imported from the environment, just
the contrary, to leave them all alone).

A change like this whould have been preferable:

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 6767da8..4c96044 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1209,7 +1209,7 @@ test_expect_success POSIXPERM,PERL 'preserves existing permissions' '
 HOME="$(pwd)" # convert to Windows path
 
 test_expect_success 'set up --show-origin tests' '
-	INCLUDE_DIR="$HOME/include" &&
+	INCLUDE_DIR="$(pwd)/include" &&
 	mkdir -p "$INCLUDE_DIR" &&
 	cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
 		[user]
@@ -1219,7 +1219,7 @@ test_expect_success 'set up --show-origin tests' '
 		[user]
 			relative = include
 	EOF
-	cat >"$HOME"/.gitconfig <<-EOF &&
+	cat >"$(pwd)"/.gitconfig <<-EOF &&
 		[user]
 			global = true
 			override = global
@@ -1237,9 +1237,9 @@ test_expect_success 'set up --show-origin tests' '
 
 test_expect_success '--show-origin with --list' '
 	cat >expect <<-EOF &&
-		file:$HOME/.gitconfig	user.global=true
-		file:$HOME/.gitconfig	user.override=global
-		file:$HOME/.gitconfig	include.path=$INCLUDE_DIR/absolute.include
+		file:$(pwd)/.gitconfig	user.global=true
+		file:$(pwd)/.gitconfig	user.override=global
+		file:$(pwd)/.gitconfig	include.path=$INCLUDE_DIR/absolute.include
 		file:$INCLUDE_DIR/absolute.include	user.absolute=include
 		file:.git/config	user.local=true
 		file:.git/config	user.override=local
@@ -1253,9 +1253,9 @@ test_expect_success '--show-origin with --list' '
 
 test_expect_success '--show-origin with --list --null' '
 	cat >expect <<-EOF &&
-		file:$HOME/.gitconfigQuser.global
-		trueQfile:$HOME/.gitconfigQuser.override
-		globalQfile:$HOME/.gitconfigQinclude.path
+		file:$(pwd)/.gitconfigQuser.global
+		trueQfile:$(pwd)/.gitconfigQuser.override
+		globalQfile:$(pwd)/.gitconfigQinclude.path
 		$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
 		includeQfile:.git/configQuser.local
 		trueQfile:.git/configQuser.override
@@ -1284,7 +1284,7 @@ test_expect_success '--show-origin with single file' '
 
 test_expect_success '--show-origin with --get-regexp' '
 	cat >expect <<-EOF &&
-		file:$HOME/.gitconfig	user.global true
+		file:$(pwd)/.gitconfig	user.global true
 		file:.git/config	user.local true
 	EOF
 	git config --show-origin --get-regexp "user\.[g|l].*" >output &&

But it seems I'm late in the game. Can't I take a break for a week
without something odd happening to the Windows port...?

-- Hannes

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Schindelin]

Hi Hannes,

On Mon, 28 Mar 2016, Johannes Sixt wrote:

> A change like this whould have been preferable:
> [...]

The problem with your patch is that it does not account for backslashes in
paths resulting in quoting. I am afraid that your patch will most likely
*not* let the tests pass in Git for Windows SDK, while my patch does.

Ciao,
Dscho

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Sixt]

Am 28.03.2016 um 17:14 schrieb Johannes Schindelin:
> Hi Hannes,
> 
> On Mon, 28 Mar 2016, Johannes Sixt wrote:
> 
>> A change like this whould have been preferable:
>> [...]
> 
> The problem with your patch is that it does not account for backslashes in
> paths resulting in quoting. I am afraid that your patch will most likely
> *not* let the tests pass in Git for Windows SDK, while my patch does.

It does pass. The reason is that pwd -W generates forward slashes.

This part of your 45bf3297 (t1300: fix the new --show-origin tests on
Windows)

@@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing per
          "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
 '
 
+! test_have_prereq MINGW ||
+HOME="$(pwd)" # convert to Windows path
+
 test_expect_success 'set up --show-origin tests' '
        INCLUDE_DIR="$HOME/include" &&
        mkdir -p "$INCLUDE_DIR" &&

is actually a much more concise version of my proposed patch,
although the result still misuses $HOME where it does not have
to. In fact, if I revert 5ca6b7bb (config --show-origin: report
paths with forward slashes), the tests still pass. But since it
does not make a difference save for a few microseconds more or
less during startup, it is not worth the churn at this point.

-- Hannes

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> This part of your 45bf3297 (t1300: fix the new --show-origin tests on
> Windows)
>
> @@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing per
>           "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
>  '
>  
> +! test_have_prereq MINGW ||
> +HOME="$(pwd)" # convert to Windows path
> +
>  test_expect_success 'set up --show-origin tests' '
>         INCLUDE_DIR="$HOME/include" &&
>         mkdir -p "$INCLUDE_DIR" &&
>
> is actually a much more concise version of my proposed patch,
> although the result still misuses $HOME where it does not have
> to. In fact, if I revert 5ca6b7bb (config --show-origin: report
> paths with forward slashes), the tests still pass. But since it
> does not make a difference save for a few microseconds more or
> less during startup, it is not worth the churn at this point.

Well, from the point of view of codebase cleanliness, if we can do
without 5ca6b7bb4, we would be much better off in the longer term,
so I would say it would be wonderful if we can safely revert it.

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Sixt]

Am 29.03.2016 um 21:18 schrieb Johannes Sixt:
> Am 28.03.2016 um 17:14 schrieb Johannes Schindelin:
>> The problem with your patch is that it does not account for backslashes in
>> paths resulting in quoting. I am afraid that your patch will most likely
>> *not* let the tests pass in Git for Windows SDK, while my patch does.
>
> It does pass. The reason is that pwd -W generates forward slashes.

It just occurred to me that we might be observing a difference in 
behavior of pwd -W between the modern MSYS2 bash and the old MSYS1 bash 
that I am using. If that is the case the tests won't pass under MSYS2 
without the change made by 5ca6b7bb (config --show-origin: report paths 
with forward slashes).

-- Hannes

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Sixt]

Am 30.03.2016 um 07:52 schrieb Johannes Sixt:
> Am 29.03.2016 um 21:18 schrieb Johannes Sixt:
>> It does pass. The reason is that pwd -W generates forward slashes.
>
> It just occurred to me that we might be observing a difference in
> behavior of pwd -W between the modern MSYS2 bash and the old MSYS1 bash
> that I am using.

No that isn't it. Both versions produce forward slashes. Puzzled...

-- Hannes

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Sixt]

Am 29.03.2016 um 22:05 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> This part of your 45bf3297 (t1300: fix the new --show-origin tests on
>> Windows)
>>
>> @@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing per
>>            "die q(badrename) if ((stat(q(.git/config)))[2] & 07777) != 0600"
>>   '
>>   
>> +! test_have_prereq MINGW ||
>> +HOME="$(pwd)" # convert to Windows path
>> +
>>   test_expect_success 'set up --show-origin tests' '
>>          INCLUDE_DIR="$HOME/include" &&
>>          mkdir -p "$INCLUDE_DIR" &&
>>
>> is actually a much more concise version of my proposed patch,
>> although the result still misuses $HOME where it does not have
>> to. In fact, if I revert 5ca6b7bb (config --show-origin: report
>> paths with forward slashes), the tests still pass. But since it
>> does not make a difference save for a few microseconds more or
>> less during startup, it is not worth the churn at this point.
> 
> Well, from the point of view of codebase cleanliness, if we can do
> without 5ca6b7bb4, we would be much better off in the longer term,
> so I would say it would be wonderful if we can safely revert it.

Although I am convinced that the change is not necessary for
correctness, I can buy the justification that we should produce forward
slashes for consistency. There are a number of occasions where we
present paths to the user, and we do show forward-slashes in all cases
that I found. We should keep the commit.

But then let's do this:

---- 8< ----
Subject: [PATCH] Windows: shorten code by re-using convert_slashes()

Make a few more spots more readable by using the recently introduced,
Windows-specific helper.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 abspath.c      | 5 +----
 compat/mingw.c | 9 ++-------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/abspath.c b/abspath.c
index 5edb4e7..2825de8 100644
--- a/abspath.c
+++ b/abspath.c
@@ -167,7 +167,6 @@ const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 	strbuf_add(&path, pfx, pfx_len);
 	strbuf_addstr(&path, arg);
 #else
-	char *p;
 	/* don't add prefix to absolute paths, but still replace '\' by '/' */
 	strbuf_reset(&path);
 	if (is_absolute_path(arg))
@@ -175,9 +174,7 @@ const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 	else if (pfx_len)
 		strbuf_add(&path, pfx, pfx_len);
 	strbuf_addstr(&path, arg);
-	for (p = path.buf + pfx_len; *p; p++)
-		if (*p == '\\')
-			*p = '/';
+	convert_slashes(path.buf + pfx_len);
 #endif
 	return path.buf;
 }
diff --git a/compat/mingw.c b/compat/mingw.c
index 54c82ec..0413d5c 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -763,15 +763,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
 
 char *mingw_getcwd(char *pointer, int len)
 {
-	int i;
 	wchar_t wpointer[MAX_PATH];
 	if (!_wgetcwd(wpointer, ARRAY_SIZE(wpointer)))
 		return NULL;
 	if (xwcstoutf(pointer, wpointer, len) < 0)
 		return NULL;
-	for (i = 0; pointer[i]; i++)
-		if (pointer[i] == '\\')
-			pointer[i] = '/';
+	convert_slashes(pointer);
 	return pointer;
 }
 
@@ -2112,9 +2109,7 @@ static void setup_windows_environment()
 		 * executable (by not mistaking the dir separators
 		 * for escape characters).
 		 */
-		for (; *tmp; tmp++)
-			if (*tmp == '\\')
-				*tmp = '/';
+		convert_slashes(tmp);
 	}
 
 	/* simulate TERM to enable auto-color (see color.c) */
-- 
2.7.0.118.g90056ae

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Although I am convinced that the change is not necessary for
> correctness, I can buy the justification that we should produce forward
> slashes for consistency. There are a number of occasions where we
> present paths to the user, and we do show forward-slashes in all cases
> that I found. We should keep the commit.
>
> But then let's do this:

Sounds like a plan; even though I am mildly against adding more
platform specific #ifdef to files outside compat/, this patch does
not.

Dscho?

>
> ---- 8< ----
> Subject: [PATCH] Windows: shorten code by re-using convert_slashes()
>
> Make a few more spots more readable by using the recently introduced,
> Windows-specific helper.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  abspath.c      | 5 +----
>  compat/mingw.c | 9 ++-------
>  2 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index 5edb4e7..2825de8 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -167,7 +167,6 @@ const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>  	strbuf_add(&path, pfx, pfx_len);
>  	strbuf_addstr(&path, arg);
>  #else
> -	char *p;
>  	/* don't add prefix to absolute paths, but still replace '\' by '/' */
>  	strbuf_reset(&path);
>  	if (is_absolute_path(arg))
> @@ -175,9 +174,7 @@ const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>  	else if (pfx_len)
>  		strbuf_add(&path, pfx, pfx_len);
>  	strbuf_addstr(&path, arg);
> -	for (p = path.buf + pfx_len; *p; p++)
> -		if (*p == '\\')
> -			*p = '/';
> +	convert_slashes(path.buf + pfx_len);
>  #endif
>  	return path.buf;
>  }
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 54c82ec..0413d5c 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -763,15 +763,12 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
>  
>  char *mingw_getcwd(char *pointer, int len)
>  {
> -	int i;
>  	wchar_t wpointer[MAX_PATH];
>  	if (!_wgetcwd(wpointer, ARRAY_SIZE(wpointer)))
>  		return NULL;
>  	if (xwcstoutf(pointer, wpointer, len) < 0)
>  		return NULL;
> -	for (i = 0; pointer[i]; i++)
> -		if (pointer[i] == '\\')
> -			pointer[i] = '/';
> +	convert_slashes(pointer);
>  	return pointer;
>  }
>  
> @@ -2112,9 +2109,7 @@ static void setup_windows_environment()
>  		 * executable (by not mistaking the dir separators
>  		 * for escape characters).
>  		 */
> -		for (; *tmp; tmp++)
> -			if (*tmp == '\\')
> -				*tmp = '/';
> +		convert_slashes(tmp);
>  	}
>  
>  	/* simulate TERM to enable auto-color (see color.c) */

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Schindelin]

Hi Junio,

On Mon, 4 Apr 2016, Junio C Hamano wrote:

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Although I am convinced that the change is not necessary for
> > correctness, I can buy the justification that we should produce forward
> > slashes for consistency. There are a number of occasions where we
> > present paths to the user, and we do show forward-slashes in all cases
> > that I found. We should keep the commit.
> >
> > But then let's do this:
> 
> Sounds like a plan; even though I am mildly against adding more
> platform specific #ifdef to files outside compat/, this patch does
> not.
> 
> Dscho?

Fine by me!
Dscho