[PATCH] unit-tests: do show relative file paths on non-Windows, too

[PATCH] unit-tests: do show relative file paths on non-Windows, too

[Junio C Hamano]

There are compilers other than Visual C that want to show absolute
paths.  Generalize the helper introduced by a2c5e294 (unit-tests: do
show relative file paths, 2023-09-25) so that it can also work with
a path that uses slash as the directory separator, and becomes
almost no-op once one-time preparation finds out that we are using a
compiler that already gives relative paths.  Incidentally, this also
should do the right thing on Windows with a compiler that shows
relative paths but with backslash as the directory separator (if
such a thing exists and is used to build git).

Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Another change I made, which is not described in the proposed
   commit log message, is that we now use fspathcmp() instead of
   strcmp() to precompute the prefix length using a known needle[]
   string, to be consistent with the runtime check done for each and
   every path.

   This is a belated follow-up on <f0b804129e8a21449cbb6f346473d3570182ddfa.1695640837.git.gitgitgadget@gmail.com>

 t/unit-tests/test-lib.c | 60 ++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 7bf9dfdb95..83c9eb8c59 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -21,12 +21,11 @@ static struct {
 	.result = RESULT_NONE,
 };
 
-#ifndef _MSC_VER
-#define make_relative(location) location
-#else
 /*
  * Visual C interpolates the absolute Windows path for `__FILE__`,
  * but we want to see relative paths, as verified by t0080.
+ * There are other compilers that do the same, and are not for
+ * Windows.
  */
 #include "dir.h"
 
@@ -34,32 +33,67 @@ static const char *make_relative(const char *location)
 {
 	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
 	static size_t prefix_len;
+	static int need_bs_to_fs = -1;
 
-	if (!prefix_len) {
+	/* one-time preparation */
+	if (need_bs_to_fs < 0) {
 		size_t len = strlen(prefix);
-		const char *needle = "\\t\\unit-tests\\test-lib.c";
+		char needle[] = "t\\unit-tests\\test-lib.c";
 		size_t needle_len = strlen(needle);
 
-		if (len < needle_len || strcmp(needle, prefix + len - needle_len))
+		if (len < needle_len)
+			die("unexpected prefix '%s'", prefix);
+
+		/*
+		 * The path could be relative (t/unit-tests/test-lib.c)
+		 * or full (/home/user/git/t/unit-tests/test-lib.c).
+		 * Check the slash between "t" and "unit-tests".
+		 */
+		prefix_len = len - needle_len;
+		if (prefix[prefix_len + 1] == '/') {
+			/* Oh, we're not Windows */
+			for (size_t i = 0; i < needle_len; i++)
+				if (needle[i] == '\\')
+					needle[i] = '/';
+			need_bs_to_fs = 0;
+		} else {
+			need_bs_to_fs = 1;
+		}
+
+		/*
+		 * prefix_len == 0 if the compiler gives paths relative
+		 * to the root of the working tree.  Otherwise, we want
+		 * to see that we did find the needle[] at a directory
+		 * boundary.
+		 */
+		if (fspathcmp(needle, prefix + prefix_len) ||
+		    (prefix_len &&
+		     prefix[prefix_len - 1] != '/' &&
+		     prefix[prefix_len - 1] != '\\'))
 			die("unexpected suffix of '%s'", prefix);
 
-		/* let it end in a directory separator */
-		prefix_len = len - needle_len + 1;
 	}
 
+	/*
+	 * If our compiler gives relative paths and we do not need
+	 * to munge directory separator, we can return location as-is.
+	 */
+	if (!prefix_len && !need_bs_to_fs)
+		return location;
+
 	/* Does it not start with the expected prefix? */
 	if (fspathncmp(location, prefix, prefix_len))
 		return location;
 
 	strlcpy(buf, location + prefix_len, sizeof(buf));
 	/* convert backslashes to forward slashes */
-	for (p = buf; *p; p++)
-		if (*p == '\\')
-			*p = '/';
-
+	if (need_bs_to_fs) {
+		for (p = buf; *p; p++)
+			if (*p == '\\')
+				*p = '/';
+	}
 	return buf;
 }
-#endif
 
 static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
 {
-- 
2.44.0-rc0

Re: [PATCH] unit-tests: do show relative file paths on non-Windows, too

[Phillip Wood]

Hi Junio

On 11/02/2024 08:57, Junio C Hamano wrote:
> There are compilers other than Visual C that want to show absolute
> paths.  Generalize the helper introduced by a2c5e294 (unit-tests: do
> show relative file paths, 2023-09-25) so that it can also work with
> a path that uses slash as the directory separator, and becomes
> almost no-op once one-time preparation finds out that we are using a
> compiler that already gives relative paths.  Incidentally, this also
> should do the right thing on Windows with a compiler that shows
> relative paths but with backslash as the directory separator (if
> such a thing exists and is used to build git).
> 
> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>   * Another change I made, which is not described in the proposed
>     commit log message, is that we now use fspathcmp() instead of
>     strcmp() to precompute the prefix length using a known needle[]
>     string, to be consistent with the runtime check done for each and
>     every path.
> 
>     This is a belated follow-up on <f0b804129e8a21449cbb6f346473d3570182ddfa.1695640837.git.gitgitgadget@gmail.com>

Thanks for putting this together - I was about to sit down and write 
something similar when I saw your patch. I've left one comment below but 
I don't think it is worth a re-roll, this looks good to me.

> +		/*
> +		 * prefix_len == 0 if the compiler gives paths relative
> +		 * to the root of the working tree.  Otherwise, we want
> +		 * to see that we did find the needle[] at a directory
> +		 * boundary.
> +		 */
> +		if (fspathcmp(needle, prefix + prefix_len) ||
> +		    (prefix_len &&
> +		     prefix[prefix_len - 1] != '/' &&
> +		     prefix[prefix_len - 1] != '\\'))

We know which separator we're expecting so we could replace  the last 
two comparisons with

		prefix[prefix_len -1] != needle[1]

but as I say I'm not sure that is worth re-rolling for

Best Wishes

Phillip

[PATCH v2] unit-tests: do show relative file paths on non-Windows, too

[Junio C Hamano]

>
> We know which separator we're expecting so we could replace  the last
> two comparisons with
>
> 		prefix[prefix_len -1] != needle[1]
>
> but as I say I'm not sure that is worth re-rolling for

There is a larger clean-up opportunity to drop the need for making a
copy, which probably is worth doing, so I folded the above into this
version.

------- >8 ------------- >8 ------------- >8 ------------- >8 -------

There are compilers other than Visual C that want to show absolute
paths.  Generalize the helper introduced by a2c5e294 (unit-tests: do
show relative file paths, 2023-09-25) so that it can also work with
a path that uses slash as the directory separator, and becomes
almost no-op once one-time preparation finds out that we are using a
compiler that already gives relative paths.  Incidentally, this also
should do the right thing on Windows with a compiler that shows
relative paths but with backslash as the directory separator (if
such a thing exists and is used to build git).

Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  * I found that the diff relative to the result of applying v1 was
    easier to follow than the range-diff, so here it is.

  diff --git c/t/unit-tests/test-lib.c w/t/unit-tests/test-lib.c
  index 83c9eb8c59..66d6980ffb 100644
  --- c/t/unit-tests/test-lib.c
  +++ w/t/unit-tests/test-lib.c
  @@ -64,34 +64,33 @@ static const char *make_relative(const char *location)
   		 * prefix_len == 0 if the compiler gives paths relative
   		 * to the root of the working tree.  Otherwise, we want
   		 * to see that we did find the needle[] at a directory
  -		 * boundary.
  +		 * boundary.  Again we rely on that needle[] begins with
  +		 * "t" followed by the directory separator.
   		 */
   		if (fspathcmp(needle, prefix + prefix_len) ||
  -		    (prefix_len &&
  -		     prefix[prefix_len - 1] != '/' &&
  -		     prefix[prefix_len - 1] != '\\'))
  +		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
   			die("unexpected suffix of '%s'", prefix);
  -
   	}
   
   	/*
  -	 * If our compiler gives relative paths and we do not need
  -	 * to munge directory separator, we can return location as-is.
  +	 * Does it not start with the expected prefix?
  +	 * Return it as-is without making it worse.
   	 */
  -	if (!prefix_len && !need_bs_to_fs)
  +	if (prefix_len && fspathncmp(location, prefix, prefix_len))
   		return location;
   
  -	/* Does it not start with the expected prefix? */
  -	if (fspathncmp(location, prefix, prefix_len))
  -		return location;
  +	/*
  +	 * If we do not need to munge directory separator, we can return
  +	 * the substring at the tail of the location.
  +	 */
  +	if (!need_bs_to_fs)
  +		return location + prefix_len;
   
  -	strlcpy(buf, location + prefix_len, sizeof(buf));
   	/* convert backslashes to forward slashes */
  -	if (need_bs_to_fs) {
  -		for (p = buf; *p; p++)
  -			if (*p == '\\')
  -				*p = '/';
  -	}
  +	strlcpy(buf, location + prefix_len, sizeof(buf));
  +	for (p = buf; *p; p++)
  +		if (*p == '\\')
  +			*p = '/';
   	return buf;
   }
   

 t/unit-tests/test-lib.c | 61 +++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 14 deletions(-)

diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
index 7bf9dfdb95..66d6980ffb 100644
--- a/t/unit-tests/test-lib.c
+++ b/t/unit-tests/test-lib.c
@@ -21,12 +21,11 @@ static struct {
 	.result = RESULT_NONE,
 };
 
-#ifndef _MSC_VER
-#define make_relative(location) location
-#else
 /*
  * Visual C interpolates the absolute Windows path for `__FILE__`,
  * but we want to see relative paths, as verified by t0080.
+ * There are other compilers that do the same, and are not for
+ * Windows.
  */
 #include "dir.h"
 
@@ -34,32 +33,66 @@ static const char *make_relative(const char *location)
 {
 	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
 	static size_t prefix_len;
+	static int need_bs_to_fs = -1;
 
-	if (!prefix_len) {
+	/* one-time preparation */
+	if (need_bs_to_fs < 0) {
 		size_t len = strlen(prefix);
-		const char *needle = "\\t\\unit-tests\\test-lib.c";
+		char needle[] = "t\\unit-tests\\test-lib.c";
 		size_t needle_len = strlen(needle);
 
-		if (len < needle_len || strcmp(needle, prefix + len - needle_len))
-			die("unexpected suffix of '%s'", prefix);
+		if (len < needle_len)
+			die("unexpected prefix '%s'", prefix);
+
+		/*
+		 * The path could be relative (t/unit-tests/test-lib.c)
+		 * or full (/home/user/git/t/unit-tests/test-lib.c).
+		 * Check the slash between "t" and "unit-tests".
+		 */
+		prefix_len = len - needle_len;
+		if (prefix[prefix_len + 1] == '/') {
+			/* Oh, we're not Windows */
+			for (size_t i = 0; i < needle_len; i++)
+				if (needle[i] == '\\')
+					needle[i] = '/';
+			need_bs_to_fs = 0;
+		} else {
+			need_bs_to_fs = 1;
+		}
 
-		/* let it end in a directory separator */
-		prefix_len = len - needle_len + 1;
+		/*
+		 * prefix_len == 0 if the compiler gives paths relative
+		 * to the root of the working tree.  Otherwise, we want
+		 * to see that we did find the needle[] at a directory
+		 * boundary.  Again we rely on that needle[] begins with
+		 * "t" followed by the directory separator.
+		 */
+		if (fspathcmp(needle, prefix + prefix_len) ||
+		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
+			die("unexpected suffix of '%s'", prefix);
 	}
 
-	/* Does it not start with the expected prefix? */
-	if (fspathncmp(location, prefix, prefix_len))
+	/*
+	 * Does it not start with the expected prefix?
+	 * Return it as-is without making it worse.
+	 */
+	if (prefix_len && fspathncmp(location, prefix, prefix_len))
 		return location;
 
-	strlcpy(buf, location + prefix_len, sizeof(buf));
+	/*
+	 * If we do not need to munge directory separator, we can return
+	 * the substring at the tail of the location.
+	 */
+	if (!need_bs_to_fs)
+		return location + prefix_len;
+
 	/* convert backslashes to forward slashes */
+	strlcpy(buf, location + prefix_len, sizeof(buf));
 	for (p = buf; *p; p++)
 		if (*p == '\\')
 			*p = '/';
-
 	return buf;
 }
-#endif
 
 static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
 {
-- 
2.44.0-rc0

Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too

[Phillip Wood]

Hi Junio

On 11/02/2024 15:58, Junio C Hamano wrote:
>>
>> We know which separator we're expecting so we could replace  the last
>> two comparisons with
>>
>> 		prefix[prefix_len -1] != needle[1]
>>
>> but as I say I'm not sure that is worth re-rolling for
> 
> There is a larger clean-up opportunity to drop the need for making a
> copy, which probably is worth doing, so I folded the above into this
> version.

Ooh, that's nice. This version looks good, I found the code comments 
very helpful

Best Wishes

Phillip

> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> 
> There are compilers other than Visual C that want to show absolute
> paths.  Generalize the helper introduced by a2c5e294 (unit-tests: do
> show relative file paths, 2023-09-25) so that it can also work with
> a path that uses slash as the directory separator, and becomes
> almost no-op once one-time preparation finds out that we are using a
> compiler that already gives relative paths.  Incidentally, this also
> should do the right thing on Windows with a compiler that shows
> relative paths but with backslash as the directory separator (if
> such a thing exists and is used to build git).
> 
> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>    * I found that the diff relative to the result of applying v1 was
>      easier to follow than the range-diff, so here it is.
> 
>    diff --git c/t/unit-tests/test-lib.c w/t/unit-tests/test-lib.c
>    index 83c9eb8c59..66d6980ffb 100644
>    --- c/t/unit-tests/test-lib.c
>    +++ w/t/unit-tests/test-lib.c
>    @@ -64,34 +64,33 @@ static const char *make_relative(const char *location)
>     		 * prefix_len == 0 if the compiler gives paths relative
>     		 * to the root of the working tree.  Otherwise, we want
>     		 * to see that we did find the needle[] at a directory
>    -		 * boundary.
>    +		 * boundary.  Again we rely on that needle[] begins with
>    +		 * "t" followed by the directory separator.
>     		 */
>     		if (fspathcmp(needle, prefix + prefix_len) ||
>    -		    (prefix_len &&
>    -		     prefix[prefix_len - 1] != '/' &&
>    -		     prefix[prefix_len - 1] != '\\'))
>    +		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
>     			die("unexpected suffix of '%s'", prefix);
>    -
>     	}
>     
>     	/*
>    -	 * If our compiler gives relative paths and we do not need
>    -	 * to munge directory separator, we can return location as-is.
>    +	 * Does it not start with the expected prefix?
>    +	 * Return it as-is without making it worse.
>     	 */
>    -	if (!prefix_len && !need_bs_to_fs)
>    +	if (prefix_len && fspathncmp(location, prefix, prefix_len))
>     		return location;
>     
>    -	/* Does it not start with the expected prefix? */
>    -	if (fspathncmp(location, prefix, prefix_len))
>    -		return location;
>    +	/*
>    +	 * If we do not need to munge directory separator, we can return
>    +	 * the substring at the tail of the location.
>    +	 */
>    +	if (!need_bs_to_fs)
>    +		return location + prefix_len;
>     
>    -	strlcpy(buf, location + prefix_len, sizeof(buf));
>     	/* convert backslashes to forward slashes */
>    -	if (need_bs_to_fs) {
>    -		for (p = buf; *p; p++)
>    -			if (*p == '\\')
>    -				*p = '/';
>    -	}
>    +	strlcpy(buf, location + prefix_len, sizeof(buf));
>    +	for (p = buf; *p; p++)
>    +		if (*p == '\\')
>    +			*p = '/';
>     	return buf;
>     }
>     
> 
>   t/unit-tests/test-lib.c | 61 +++++++++++++++++++++++++++++++----------
>   1 file changed, 47 insertions(+), 14 deletions(-)
> 
> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
> index 7bf9dfdb95..66d6980ffb 100644
> --- a/t/unit-tests/test-lib.c
> +++ b/t/unit-tests/test-lib.c
> @@ -21,12 +21,11 @@ static struct {
>   	.result = RESULT_NONE,
>   };
>   
> -#ifndef _MSC_VER
> -#define make_relative(location) location
> -#else
>   /*
>    * Visual C interpolates the absolute Windows path for `__FILE__`,
>    * but we want to see relative paths, as verified by t0080.
> + * There are other compilers that do the same, and are not for
> + * Windows.
>    */
>   #include "dir.h"
>   
> @@ -34,32 +33,66 @@ static const char *make_relative(const char *location)
>   {
>   	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
>   	static size_t prefix_len;
> +	static int need_bs_to_fs = -1;
>   
> -	if (!prefix_len) {
> +	/* one-time preparation */
> +	if (need_bs_to_fs < 0) {
>   		size_t len = strlen(prefix);
> -		const char *needle = "\\t\\unit-tests\\test-lib.c";
> +		char needle[] = "t\\unit-tests\\test-lib.c";
>   		size_t needle_len = strlen(needle);
>   
> -		if (len < needle_len || strcmp(needle, prefix + len - needle_len))
> -			die("unexpected suffix of '%s'", prefix);
> +		if (len < needle_len)
> +			die("unexpected prefix '%s'", prefix);
> +
> +		/*
> +		 * The path could be relative (t/unit-tests/test-lib.c)
> +		 * or full (/home/user/git/t/unit-tests/test-lib.c).
> +		 * Check the slash between "t" and "unit-tests".
> +		 */
> +		prefix_len = len - needle_len;
> +		if (prefix[prefix_len + 1] == '/') {
> +			/* Oh, we're not Windows */
> +			for (size_t i = 0; i < needle_len; i++)
> +				if (needle[i] == '\\')
> +					needle[i] = '/';
> +			need_bs_to_fs = 0;
> +		} else {
> +			need_bs_to_fs = 1;
> +		}
>   
> -		/* let it end in a directory separator */
> -		prefix_len = len - needle_len + 1;
> +		/*
> +		 * prefix_len == 0 if the compiler gives paths relative
> +		 * to the root of the working tree.  Otherwise, we want
> +		 * to see that we did find the needle[] at a directory
> +		 * boundary.  Again we rely on that needle[] begins with
> +		 * "t" followed by the directory separator.
> +		 */
> +		if (fspathcmp(needle, prefix + prefix_len) ||
> +		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
> +			die("unexpected suffix of '%s'", prefix);
>   	}
>   
> -	/* Does it not start with the expected prefix? */
> -	if (fspathncmp(location, prefix, prefix_len))
> +	/*
> +	 * Does it not start with the expected prefix?
> +	 * Return it as-is without making it worse.
> +	 */
> +	if (prefix_len && fspathncmp(location, prefix, prefix_len))
>   		return location;
>   
> -	strlcpy(buf, location + prefix_len, sizeof(buf));
> +	/*
> +	 * If we do not need to munge directory separator, we can return
> +	 * the substring at the tail of the location.
> +	 */
> +	if (!need_bs_to_fs)
> +		return location + prefix_len;
> +
>   	/* convert backslashes to forward slashes */
> +	strlcpy(buf, location + prefix_len, sizeof(buf));
>   	for (p = buf; *p; p++)
>   		if (*p == '\\')
>   			*p = '/';
> -
>   	return buf;
>   }
> -#endif
>   
>   static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
>   {

Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> There is a larger clean-up opportunity to drop the need for making a
>> copy, which probably is worth doing, so I folded the above into this
>> version.
>
> Ooh, that's nice. This version looks good, I found the code comments
> very helpful

Thanks.

Judging from https://github.com/git/git/actions/runs/7878254534/job/21496314393#step:5:142
I do not seen to have broken Windows with this change, so let's
fast-track and merge it down to 'master' before -rc1.

> Best Wishes
>
> Phillip
>
>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>> There are compilers other than Visual C that want to show absolute
>> paths.  Generalize the helper introduced by a2c5e294 (unit-tests: do
>> show relative file paths, 2023-09-25) so that it can also work with
>> a path that uses slash as the directory separator, and becomes
>> almost no-op once one-time preparation finds out that we are using a
>> compiler that already gives relative paths.  Incidentally, this also
>> should do the right thing on Windows with a compiler that shows
>> relative paths but with backslash as the directory separator (if
>> such a thing exists and is used to build git).
>> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
>> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>    * I found that the diff relative to the result of applying v1 was
>>      easier to follow than the range-diff, so here it is.
>>    diff --git c/t/unit-tests/test-lib.c w/t/unit-tests/test-lib.c
>>    index 83c9eb8c59..66d6980ffb 100644
>>    --- c/t/unit-tests/test-lib.c
>>    +++ w/t/unit-tests/test-lib.c
>>    @@ -64,34 +64,33 @@ static const char *make_relative(const char *location)
>>     		 * prefix_len == 0 if the compiler gives paths relative
>>     		 * to the root of the working tree.  Otherwise, we want
>>     		 * to see that we did find the needle[] at a directory
>>    -		 * boundary.
>>    +		 * boundary.  Again we rely on that needle[] begins with
>>    +		 * "t" followed by the directory separator.
>>     		 */
>>     		if (fspathcmp(needle, prefix + prefix_len) ||
>>    -		    (prefix_len &&
>>    -		     prefix[prefix_len - 1] != '/' &&
>>    -		     prefix[prefix_len - 1] != '\\'))
>>    +		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
>>     			die("unexpected suffix of '%s'", prefix);
>>    -
>>     	}
>>         	/*
>>    -	 * If our compiler gives relative paths and we do not need
>>    -	 * to munge directory separator, we can return location as-is.
>>    +	 * Does it not start with the expected prefix?
>>    +	 * Return it as-is without making it worse.
>>     	 */
>>    -	if (!prefix_len && !need_bs_to_fs)
>>    +	if (prefix_len && fspathncmp(location, prefix, prefix_len))
>>     		return location;
>>        -	/* Does it not start with the expected prefix? */
>>    -	if (fspathncmp(location, prefix, prefix_len))
>>    -		return location;
>>    +	/*
>>    +	 * If we do not need to munge directory separator, we can return
>>    +	 * the substring at the tail of the location.
>>    +	 */
>>    +	if (!need_bs_to_fs)
>>    +		return location + prefix_len;
>>        -	strlcpy(buf, location + prefix_len, sizeof(buf));
>>     	/* convert backslashes to forward slashes */
>>    -	if (need_bs_to_fs) {
>>    -		for (p = buf; *p; p++)
>>    -			if (*p == '\\')
>>    -				*p = '/';
>>    -	}
>>    +	strlcpy(buf, location + prefix_len, sizeof(buf));
>>    +	for (p = buf; *p; p++)
>>    +		if (*p == '\\')
>>    +			*p = '/';
>>     	return buf;
>>     }
>>       t/unit-tests/test-lib.c | 61
>> +++++++++++++++++++++++++++++++----------
>>   1 file changed, 47 insertions(+), 14 deletions(-)
>> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
>> index 7bf9dfdb95..66d6980ffb 100644
>> --- a/t/unit-tests/test-lib.c
>> +++ b/t/unit-tests/test-lib.c
>> @@ -21,12 +21,11 @@ static struct {
>>   	.result = RESULT_NONE,
>>   };
>>   -#ifndef _MSC_VER
>> -#define make_relative(location) location
>> -#else
>>   /*
>>    * Visual C interpolates the absolute Windows path for `__FILE__`,
>>    * but we want to see relative paths, as verified by t0080.
>> + * There are other compilers that do the same, and are not for
>> + * Windows.
>>    */
>>   #include "dir.h"
>>   @@ -34,32 +33,66 @@ static const char *make_relative(const char
>> *location)
>>   {
>>   	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
>>   	static size_t prefix_len;
>> +	static int need_bs_to_fs = -1;
>>   -	if (!prefix_len) {
>> +	/* one-time preparation */
>> +	if (need_bs_to_fs < 0) {
>>   		size_t len = strlen(prefix);
>> -		const char *needle = "\\t\\unit-tests\\test-lib.c";
>> +		char needle[] = "t\\unit-tests\\test-lib.c";
>>   		size_t needle_len = strlen(needle);
>>   -		if (len < needle_len || strcmp(needle, prefix + len -
>> needle_len))
>> -			die("unexpected suffix of '%s'", prefix);
>> +		if (len < needle_len)
>> +			die("unexpected prefix '%s'", prefix);
>> +
>> +		/*
>> +		 * The path could be relative (t/unit-tests/test-lib.c)
>> +		 * or full (/home/user/git/t/unit-tests/test-lib.c).
>> +		 * Check the slash between "t" and "unit-tests".
>> +		 */
>> +		prefix_len = len - needle_len;
>> +		if (prefix[prefix_len + 1] == '/') {
>> +			/* Oh, we're not Windows */
>> +			for (size_t i = 0; i < needle_len; i++)
>> +				if (needle[i] == '\\')
>> +					needle[i] = '/';
>> +			need_bs_to_fs = 0;
>> +		} else {
>> +			need_bs_to_fs = 1;
>> +		}
>>   -		/* let it end in a directory separator */
>> -		prefix_len = len - needle_len + 1;
>> +		/*
>> +		 * prefix_len == 0 if the compiler gives paths relative
>> +		 * to the root of the working tree.  Otherwise, we want
>> +		 * to see that we did find the needle[] at a directory
>> +		 * boundary.  Again we rely on that needle[] begins with
>> +		 * "t" followed by the directory separator.
>> +		 */
>> +		if (fspathcmp(needle, prefix + prefix_len) ||
>> +		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
>> +			die("unexpected suffix of '%s'", prefix);
>>   	}
>>   -	/* Does it not start with the expected prefix? */
>> -	if (fspathncmp(location, prefix, prefix_len))
>> +	/*
>> +	 * Does it not start with the expected prefix?
>> +	 * Return it as-is without making it worse.
>> +	 */
>> +	if (prefix_len && fspathncmp(location, prefix, prefix_len))
>>   		return location;
>>   -	strlcpy(buf, location + prefix_len, sizeof(buf));
>> +	/*
>> +	 * If we do not need to munge directory separator, we can return
>> +	 * the substring at the tail of the location.
>> +	 */
>> +	if (!need_bs_to_fs)
>> +		return location + prefix_len;
>> +
>>   	/* convert backslashes to forward slashes */
>> +	strlcpy(buf, location + prefix_len, sizeof(buf));
>>   	for (p = buf; *p; p++)
>>   		if (*p == '\\')
>>   			*p = '/';
>> -
>>   	return buf;
>>   }
>> -#endif
>>     static void msg_with_prefix(const char *prefix, const char
>> *format, va_list ap)
>>   {

Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too

[Phillip Wood]

On 12/02/2024 22:41, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> There is a larger clean-up opportunity to drop the need for making a
>>> copy, which probably is worth doing, so I folded the above into this
>>> version.
>>
>> Ooh, that's nice. This version looks good, I found the code comments
>> very helpful
> 
> Thanks.
> 
> Judging from https://github.com/git/git/actions/runs/7878254534/job/21496314393#step:5:142
> I do not seen to have broken Windows with this change, so let's
> fast-track and merge it down to 'master' before -rc1.

I think it was only the MSVC that needed the paths munging which we 
don't test by default. I have tweaked our github actions to run those 
tests and they pass 
https://github.com/phillipwood/git/actions/runs/7885144920/job/21515922057#step:5:146

Best Wishes

Phillip


>> Best Wishes
>>
>> Phillip
>>
>>> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
>>> There are compilers other than Visual C that want to show absolute
>>> paths.  Generalize the helper introduced by a2c5e294 (unit-tests: do
>>> show relative file paths, 2023-09-25) so that it can also work with
>>> a path that uses slash as the directory separator, and becomes
>>> almost no-op once one-time preparation finds out that we are using a
>>> compiler that already gives relative paths.  Incidentally, this also
>>> should do the right thing on Windows with a compiler that shows
>>> relative paths but with backslash as the directory separator (if
>>> such a thing exists and is used to build git).
>>> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
>>> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>> ---
>>>     * I found that the diff relative to the result of applying v1 was
>>>       easier to follow than the range-diff, so here it is.
>>>     diff --git c/t/unit-tests/test-lib.c w/t/unit-tests/test-lib.c
>>>     index 83c9eb8c59..66d6980ffb 100644
>>>     --- c/t/unit-tests/test-lib.c
>>>     +++ w/t/unit-tests/test-lib.c
>>>     @@ -64,34 +64,33 @@ static const char *make_relative(const char *location)
>>>      		 * prefix_len == 0 if the compiler gives paths relative
>>>      		 * to the root of the working tree.  Otherwise, we want
>>>      		 * to see that we did find the needle[] at a directory
>>>     -		 * boundary.
>>>     +		 * boundary.  Again we rely on that needle[] begins with
>>>     +		 * "t" followed by the directory separator.
>>>      		 */
>>>      		if (fspathcmp(needle, prefix + prefix_len) ||
>>>     -		    (prefix_len &&
>>>     -		     prefix[prefix_len - 1] != '/' &&
>>>     -		     prefix[prefix_len - 1] != '\\'))
>>>     +		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
>>>      			die("unexpected suffix of '%s'", prefix);
>>>     -
>>>      	}
>>>          	/*
>>>     -	 * If our compiler gives relative paths and we do not need
>>>     -	 * to munge directory separator, we can return location as-is.
>>>     +	 * Does it not start with the expected prefix?
>>>     +	 * Return it as-is without making it worse.
>>>      	 */
>>>     -	if (!prefix_len && !need_bs_to_fs)
>>>     +	if (prefix_len && fspathncmp(location, prefix, prefix_len))
>>>      		return location;
>>>         -	/* Does it not start with the expected prefix? */
>>>     -	if (fspathncmp(location, prefix, prefix_len))
>>>     -		return location;
>>>     +	/*
>>>     +	 * If we do not need to munge directory separator, we can return
>>>     +	 * the substring at the tail of the location.
>>>     +	 */
>>>     +	if (!need_bs_to_fs)
>>>     +		return location + prefix_len;
>>>         -	strlcpy(buf, location + prefix_len, sizeof(buf));
>>>      	/* convert backslashes to forward slashes */
>>>     -	if (need_bs_to_fs) {
>>>     -		for (p = buf; *p; p++)
>>>     -			if (*p == '\\')
>>>     -				*p = '/';
>>>     -	}
>>>     +	strlcpy(buf, location + prefix_len, sizeof(buf));
>>>     +	for (p = buf; *p; p++)
>>>     +		if (*p == '\\')
>>>     +			*p = '/';
>>>      	return buf;
>>>      }
>>>        t/unit-tests/test-lib.c | 61
>>> +++++++++++++++++++++++++++++++----------
>>>    1 file changed, 47 insertions(+), 14 deletions(-)
>>> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
>>> index 7bf9dfdb95..66d6980ffb 100644
>>> --- a/t/unit-tests/test-lib.c
>>> +++ b/t/unit-tests/test-lib.c
>>> @@ -21,12 +21,11 @@ static struct {
>>>    	.result = RESULT_NONE,
>>>    };
>>>    -#ifndef _MSC_VER
>>> -#define make_relative(location) location
>>> -#else
>>>    /*
>>>     * Visual C interpolates the absolute Windows path for `__FILE__`,
>>>     * but we want to see relative paths, as verified by t0080.
>>> + * There are other compilers that do the same, and are not for
>>> + * Windows.
>>>     */
>>>    #include "dir.h"
>>>    @@ -34,32 +33,66 @@ static const char *make_relative(const char
>>> *location)
>>>    {
>>>    	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
>>>    	static size_t prefix_len;
>>> +	static int need_bs_to_fs = -1;
>>>    -	if (!prefix_len) {
>>> +	/* one-time preparation */
>>> +	if (need_bs_to_fs < 0) {
>>>    		size_t len = strlen(prefix);
>>> -		const char *needle = "\\t\\unit-tests\\test-lib.c";
>>> +		char needle[] = "t\\unit-tests\\test-lib.c";
>>>    		size_t needle_len = strlen(needle);
>>>    -		if (len < needle_len || strcmp(needle, prefix + len -
>>> needle_len))
>>> -			die("unexpected suffix of '%s'", prefix);
>>> +		if (len < needle_len)
>>> +			die("unexpected prefix '%s'", prefix);
>>> +
>>> +		/*
>>> +		 * The path could be relative (t/unit-tests/test-lib.c)
>>> +		 * or full (/home/user/git/t/unit-tests/test-lib.c).
>>> +		 * Check the slash between "t" and "unit-tests".
>>> +		 */
>>> +		prefix_len = len - needle_len;
>>> +		if (prefix[prefix_len + 1] == '/') {
>>> +			/* Oh, we're not Windows */
>>> +			for (size_t i = 0; i < needle_len; i++)
>>> +				if (needle[i] == '\\')
>>> +					needle[i] = '/';
>>> +			need_bs_to_fs = 0;
>>> +		} else {
>>> +			need_bs_to_fs = 1;
>>> +		}
>>>    -		/* let it end in a directory separator */
>>> -		prefix_len = len - needle_len + 1;
>>> +		/*
>>> +		 * prefix_len == 0 if the compiler gives paths relative
>>> +		 * to the root of the working tree.  Otherwise, we want
>>> +		 * to see that we did find the needle[] at a directory
>>> +		 * boundary.  Again we rely on that needle[] begins with
>>> +		 * "t" followed by the directory separator.
>>> +		 */
>>> +		if (fspathcmp(needle, prefix + prefix_len) ||
>>> +		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
>>> +			die("unexpected suffix of '%s'", prefix);
>>>    	}
>>>    -	/* Does it not start with the expected prefix? */
>>> -	if (fspathncmp(location, prefix, prefix_len))
>>> +	/*
>>> +	 * Does it not start with the expected prefix?
>>> +	 * Return it as-is without making it worse.
>>> +	 */
>>> +	if (prefix_len && fspathncmp(location, prefix, prefix_len))
>>>    		return location;
>>>    -	strlcpy(buf, location + prefix_len, sizeof(buf));
>>> +	/*
>>> +	 * If we do not need to munge directory separator, we can return
>>> +	 * the substring at the tail of the location.
>>> +	 */
>>> +	if (!need_bs_to_fs)
>>> +		return location + prefix_len;
>>> +
>>>    	/* convert backslashes to forward slashes */
>>> +	strlcpy(buf, location + prefix_len, sizeof(buf));
>>>    	for (p = buf; *p; p++)
>>>    		if (*p == '\\')
>>>    			*p = '/';
>>> -
>>>    	return buf;
>>>    }
>>> -#endif
>>>      static void msg_with_prefix(const char *prefix, const char
>>> *format, va_list ap)
>>>    {

Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I think it was only the MSVC that needed the paths munging which we
> don't test by default.

Ah, my sillies.

> I have tweaked our github actions to run those
> tests and they pass
> https://github.com/phillipwood/git/actions/runs/7885144920/job/21515922057#step:5:146

Thanks.

Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too

[Johannes Schindelin]

Hi Junio,

On Sun, 11 Feb 2024, Junio C Hamano wrote:

> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
> index 7bf9dfdb95..66d6980ffb 100644
> --- a/t/unit-tests/test-lib.c
> +++ b/t/unit-tests/test-lib.c
> @@ -21,12 +21,11 @@ static struct {
>  	.result = RESULT_NONE,
>  };
>
> -#ifndef _MSC_VER
> -#define make_relative(location) location
> -#else
>  /*
>   * Visual C interpolates the absolute Windows path for `__FILE__`,
>   * but we want to see relative paths, as verified by t0080.
> + * There are other compilers that do the same, and are not for
> + * Windows.
>   */
>  #include "dir.h"
>
> @@ -34,32 +33,66 @@ static const char *make_relative(const char *location)
>  {
>  	static char prefix[] = __FILE__, buf[PATH_MAX], *p;
>  	static size_t prefix_len;
> +	static int need_bs_to_fs = -1;
>
> -	if (!prefix_len) {
> +	/* one-time preparation */
> +	if (need_bs_to_fs < 0) {
>  		size_t len = strlen(prefix);
> -		const char *needle = "\\t\\unit-tests\\test-lib.c";
> +		char needle[] = "t\\unit-tests\\test-lib.c";
>  		size_t needle_len = strlen(needle);
>
> -		if (len < needle_len || strcmp(needle, prefix + len - needle_len))
> -			die("unexpected suffix of '%s'", prefix);
> +		if (len < needle_len)
> +			die("unexpected prefix '%s'", prefix);
> +
> +		/*
> +		 * The path could be relative (t/unit-tests/test-lib.c)
> +		 * or full (/home/user/git/t/unit-tests/test-lib.c).
> +		 * Check the slash between "t" and "unit-tests".
> +		 */
> +		prefix_len = len - needle_len;
> +		if (prefix[prefix_len + 1] == '/') {
> +			/* Oh, we're not Windows */
> +			for (size_t i = 0; i < needle_len; i++)
> +				if (needle[i] == '\\')
> +					needle[i] = '/';

This looks very similar to the `convert_slashes()` function that is
defined in `compat/mingw.h`. It might be a good opportunity to rename that
function and move it to `git-compat-util.h`, then use it here and once
more below at the end of `make_relative()`.

That's just a nit, though. The patch looks good to me.

Thanks,
Johannes

> +			need_bs_to_fs = 0;
> +		} else {
> +			need_bs_to_fs = 1;
> +		}
>
> -		/* let it end in a directory separator */
> -		prefix_len = len - needle_len + 1;
> +		/*
> +		 * prefix_len == 0 if the compiler gives paths relative
> +		 * to the root of the working tree.  Otherwise, we want
> +		 * to see that we did find the needle[] at a directory
> +		 * boundary.  Again we rely on that needle[] begins with
> +		 * "t" followed by the directory separator.
> +		 */
> +		if (fspathcmp(needle, prefix + prefix_len) ||
> +		    (prefix_len && prefix[prefix_len - 1] != needle[1]))
> +			die("unexpected suffix of '%s'", prefix);
>  	}
>
> -	/* Does it not start with the expected prefix? */
> -	if (fspathncmp(location, prefix, prefix_len))
> +	/*
> +	 * Does it not start with the expected prefix?
> +	 * Return it as-is without making it worse.
> +	 */
> +	if (prefix_len && fspathncmp(location, prefix, prefix_len))
>  		return location;
>
> -	strlcpy(buf, location + prefix_len, sizeof(buf));
> +	/*
> +	 * If we do not need to munge directory separator, we can return
> +	 * the substring at the tail of the location.
> +	 */
> +	if (!need_bs_to_fs)
> +		return location + prefix_len;
> +
>  	/* convert backslashes to forward slashes */
> +	strlcpy(buf, location + prefix_len, sizeof(buf));
>  	for (p = buf; *p; p++)
>  		if (*p == '\\')
>  			*p = '/';
> -
>  	return buf;
>  }
> -#endif
>
>  static void msg_with_prefix(const char *prefix, const char *format, va_list ap)
>  {
> --
> 2.44.0-rc0
>
>
>

Re: [PATCH v2] unit-tests: do show relative file paths on non-Windows, too

[Junio C Hamano]

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

>> +		/*
>> +		 * The path could be relative (t/unit-tests/test-lib.c)
>> +		 * or full (/home/user/git/t/unit-tests/test-lib.c).
>> +		 * Check the slash between "t" and "unit-tests".
>> +		 */
>> +		prefix_len = len - needle_len;
>> +		if (prefix[prefix_len + 1] == '/') {
>> +			/* Oh, we're not Windows */
>> +			for (size_t i = 0; i < needle_len; i++)
>> +				if (needle[i] == '\\')
>> +					needle[i] = '/';
>
> This looks very similar to the `convert_slashes()` function that is
> defined in `compat/mingw.h`.

I lifted it from your later loop in the function, but given that
many of the things that needed on Windows came from you, it is not
surprising if you have another copy there ;-)

> It might be a good opportunity to rename that
> function and move it to `git-compat-util.h`, then use it here and once
> more below at the end of `make_relative()`.

Right.  But not as part of the -rc fix.  Let's leave it as
#leftoverbits.

Thanks.