threads / patch / 60898

patchunit-tests: do show relative file paths on non-Windows, too

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

## tl;dr

9 messages between Feb 11, 2024 and Feb 13, 2024. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Junio C Hamano· Feb 11, 2024, 08:57 UTC · lore

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(-)
Show changes to t/unit-tests/test-lib.c +47 −13
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
Phillip Wood· Feb 11, 2024, 11:03 UTC · re: Junio C Hamano · lore

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

Hi Junio
On 11/02/2024 08:57, Junio C Hamano wrote:
Show 21 quoted lines
> 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.

Show 10 quoted lines
> +		/*
> +		 * 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
Junio C Hamano· Feb 11, 2024, 15:58 UTC · re: Phillip Wood · lore

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

Show 7 quoted lines
>
> 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(-)
Show changes to t/unit-tests/test-lib.c +47 −14
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
Phillip Wood· Feb 12, 2024, 10:44 UTC · re: Junio C Hamano · lore

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

Hi Junio
On 11/02/2024 15:58, Junio C Hamano wrote:
Show 11 quoted lines
>>
>> 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
Show 176 quoted lines
> ------- >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)
>   {
Junio C Hamano· Feb 12, 2024, 22:41 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 6 quoted lines
>> 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.

Show 166 quoted lines
> 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)
>>   {
Phillip Wood· Feb 13, 2024, 10:55 UTC · re: Junio C Hamano · lore

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

On 12/02/2024 22:41, Junio C Hamano wrote:
Show 14 quoted lines
> 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
Show 166 quoted lines
>> 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)
>>>    {
Junio C Hamano· Feb 13, 2024, 17:10 UTC · re: Phillip Wood · lore

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

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.
Johannes Schindelin· Feb 13, 2024, 19:58 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Sun, 11 Feb 2024, Junio C Hamano wrote:
Show 49 quoted lines
> 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

Show 53 quoted lines
> +			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
>
>
>
Junio C Hamano· Feb 13, 2024, 20:48 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 14 quoted lines
>> +		/*
>> +		 * 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.

← back to recent threads