| Message ID | xmqqle7r9enn.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f66286364f7875f9b3e3ac63733dd98e396fb7b8 |
| Headers | show |
| Series | [v2] unit-tests: do show relative file paths on non-Windows, too | expand |
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) > {
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) >> {
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) >>> {
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.
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 > > >
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.
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) {