git-compat-util: move convert_slashes from compat/mingw.h and rename it
cover git-compat-util: move convert_slashes from compat/mingw.h and rename it
Hi all,
This series aims to complete a #leftoverbits: https://lore.kernel.org/
git/[email protected]/ by moving convert_slashes() to git-compat-util.h and renaming it to change_path_separators().
I appreciate your review and feedback on this patch series.
Best regards, Mohit Marathe
Mohit Marathe (2):
git-compat-util: migrate convert_slashes() from compat/mingw.h
test-lib: replace repeated code logic with an existing helper
abspath.c | 4 ++-- compat/mingw.c | 4 ++-- compat/mingw.h | 6 ------ git-compat-util.h | 7 +++++++ path.c | 2 +- t/unit-tests/test-lib.c | 9 +++------ 6 files changed, 15 insertions(+), 17 deletions(-)
base-commit: 2953d95d402b6bff1a59c4712f4d46f1b9ea137f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1699%2Fmohit-marathe%2Fconvert-slashes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1699/mohit-marathe/convert-slashes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1699
Signature · 1 line
gitgitgadget
PATCH 1/2 git-compat-util: migrate `convert_slashes()` from compat/mingw.h
From: Mohit Marathe <[email protected]>
This patch migrates the convert_slashes function to git-compat- util.h and renames it to change_path_separators.
--- abspath.c | 4 ++-- compat/mingw.c | 4 ++-- compat/mingw.h | 6 ------ git-compat-util.h | 7 +++++++ path.c | 2 +- 5 files changed, 12 insertions(+), 11 deletions(-)
abspath.c +2 -2
@@ -58,7 +58,7 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)strbuf_reset (resolved);strbuf_add (resolved, remaining->buf, offset);#ifdef GIT_WINDOWS_NATIVE -convert_slashes (resolved->buf); +change_path_separators (resolved->buf);#endif strbuf_remove (remaining,0 , offset); }
@@ -278,7 +278,7 @@ char *prefix_filename(const char *pfx, const char *arg)strbuf_addstr (&path, arg);#ifdef GIT_WINDOWS_NATIVE -convert_slashes (path.buf + pfx_len); +change_path_separators (path.buf + pfx_len);#endif return strbuf_detach (&path,NULL ); }
compat/mingw.c +2 -2
@@ -1170,7 +1170,7 @@ char *mingw_getcwd(char *pointer, int len) }if (xwcstoutf (pointer, wpointer, len) <0 )return NULL ; -convert_slashes (pointer); +change_path_separators (pointer);return pointer; }
@@ -2636,7 +2636,7 @@ static void setup_windows_environment(void)* executable (by not mistaking the dir separators * for escape characters). */ -convert_slashes (tmp); +change_path_separators (tmp); }/* simulate TERM to enable auto-color (see color.c) */
compat/mingw.h +0 -6
@@ -448,12 +448,6 @@ HANDLE winansi_get_osfhandle(int fd);* git specific compatibility */ -static inline void convert_slashes (char *path) -{ -for (; *path; path++) -if (*path =='\\' ) - *path ='/' ; -}#define PATH_SEP ';'char *mingw_query_user_email (void );#define query_user_email mingw_query_user_email
git-compat-util.h +7 -0
@@ -1309,6 +1309,13 @@ static inline int strtol_i(char const *s, int base, int *result)return 0 ; } +static inline void change_path_separators (char *path) +{ +for (; *path; path++) +if (*path =='\\' ) + *path ='/' ; +} +void git_stable_qsort (void *base,size_t nmemb,size_t size,int (*compar)(const void *,const void *));#ifdef INTERNAL_QSORT
path.c +1 -1
@@ -758,7 +758,7 @@ char *interpolate_path(const char *path, int real_home)else strbuf_addstr (&user_path, home);#ifdef GIT_WINDOWS_NATIVE -convert_slashes (user_path.buf); +change_path_separators (user_path.buf);#endif }else {struct passwd *pw =getpw_str (username, username_len);
Signature · 1 line
gitgitgadget
Re: [PATCH 1/2] git-compat-util: migrate `convert_slashes()` from compat/mingw.h
Quoted 4 lines · Mohit Marathe via GitGitGadget
From: Mohit Marathe <[email protected]>
This patch migrates the
convert_slashesfunction togit-compat- util.hand renames it tochange_path_separators.
That is something a reader can tell from looking at what the patch does. What they cannot read from the diff is why the author of the patch thought it was a good idea and that is what we want to see here.
abspath.c static void get_root_part(struct strbuf *resolved, struct strbuf *remaining) · line 62
Quoted 11 lines
abspath.c +1
-1@@ -58,7 +58,7 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)strbuf_reset (resolved);strbuf_add (resolved, remaining->buf, offset);#ifdef GIT_WINDOWS_NATIVE -convert_slashes (resolved->buf); +change_path_separators (resolved->buf);#endif
In the context of "#ifdef GIT_WINDOWS_NATIVE..#endif" conditional compilation, the name convert_slashes() may have made some sense, i.e. "on this system, we get a path with the kind of slash that is not suitable to be used in Git, so we correct it to the other kind of slash". But neither it, or change_path_separators(), as a name of public function makes much sense. The direction of the change is totally unclear.
"normalize" may be a verb that has some connotation which direction the conversion is going, but still, without knowing why this change is being made (not the name change, but the part that makes it public), I cannot offer much better candidates for its name.
Quoted 23 lines
git-compat-util.h +7
-0@@ -1309,6 +1309,13 @@ static inline int strtol_i(char const *s, int base, int *result)return 0 ; } +static inline void change_path_separators (char *path) +{ +for (; *path; path++) +if (*path =='\\' ) + *path ='/' ; +} +void git_stable_qsort (void *base,size_t nmemb,size_t size,int (*compar)(const void *,const void *));#ifdef INTERNAL_QSORT path.c +0
-0
path.c char *interpolate_path(const char *path, int real_home) · line 764
Quoted 8 lines
path.c +1
-1@@ -758,7 +758,7 @@ char *interpolate_path(const char *path, int real_home)else strbuf_addstr (&user_path, home);#ifdef GIT_WINDOWS_NATIVE -convert_slashes (user_path.buf); +change_path_separators (user_path.buf);#endif }else {struct passwd *pw =getpw_str (username, username_len);
If the idea were that "here we know that the path came from reading filesystem and on platforms that use backslashes as path separators, we need to normalize them into slashes before the path is usable in Git", then I might understand if a change were more like:
* Introduce normalize_path_separators_in_place(char *) that is a NOOP by default, but does the backslash->slash conversion ONLY on platforms that require it (i.e. Windows).
* Lose "#ifdef GIT_WINDOWS_NATIVE" and corresponding "#endif", and call that normalize thing unconditionally, which gets optimized away on most systems other then where it is required.
That way, the affected .c files would become somewhat easier to follow without ugly conditional compilation.
But with the proposed patch lacking any explanation why the author thought it was a good idea, the above is just my wild guess.
PATCH 2/2 test-lib: replace repeated code logic with an existing helper
In reply to Mohit Marathe via GitGitGadgetFrom: Mohit Marathe <[email protected]>
This patch replaces the code, that changes the path separators, with the already existing function change_path_separators()
--- t/unit-tests/test-lib.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
t/unit-tests/test-lib.c +3 -6
@@ -52,9 +52,7 @@ static const char *make_relative(const char *location) 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] ='/' ; +change_path_separators (&needle[0 ]); need_bs_to_fs =0 ; }else { need_bs_to_fs =1 ;
@@ -88,9 +86,8 @@ static const char *make_relative(const char *location)/* convert backslashes to forward slashes */ strlcpy (buf, location + prefix_len,sizeof (buf)); -for (p = buf; *p; p++) -if (*p =='\\' ) - *p ='/' ; + p = buf; +change_path_separators (p);return buf; }
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] test-lib: replace repeated code logic with an existing helper
Quoted 4 lines · Mohit Marathe via GitGitGadget
From: Mohit Marathe <[email protected]>
This patch replaces the code, that changes the path separators, with the already existing function
change_path_separators()
Aside from that the name change_path_separators() needs to be updated, using it here means that the helper cannot become a NO-OP on platforms where we do not have to change backslashes we read from the system to forward slashes. So if we really wanted to use it here, and update the other existing callers in the main code to help them lose the ugly "#if WINDOWS" conditional compilation, we would need two helper functions, perhaps one that is used here that is identical to the current convert_slashes(), and the other one used to clean-up the callsites in [1/2] to also remove the conditional compilation.
Even if we were to make it public, I am not sure if compat-util is the right place, though. It is not a kitchen sink.
In any case, perhaps we want to do something like this in a header,
...
static inline void bs_to_forward_slash_in_place(char *path)
{
...
}
#ifdef GIT_WINDOWS_NATIVE
#define normalize_slashes_in_place(path) bs_to_forward_slash_in_place(path)
#else
#define normalize_slashes_in_place(path) do { ; /* nothing */ } while (0)
#endif... and then use the "normalize" one to lose "#ifdef" in the callers [1/2] touches, while "bs_to_forward_slash" one here.
I am not convinced if it is worth doing only for this single test, though.
Quoted 11 lines
t/unit-tests/test-lib.c +2
-3@@ -88,9 +86,8 @@ static const char *make_relative(const char *location)/* convert backslashes to forward slashes */ strlcpy (buf, location + prefix_len,sizeof (buf)); -for (p = buf; *p; p++) -if (*p =='\\' ) - *p ='/' ; + p = buf; +change_path_separators (p);return buf; }
Re: [PATCH 2/2] test-lib: replace repeated code logic with an existing helper
In reply to Mohit Marathe via GitGitGadgetQuoted 15 lines · Mohit Marathe via GitGitGadget
From: Mohit Marathe <[email protected]>
This patch replaces the code, that changes the path separators, with the already existing function
change_path_separators()Signed-off-by Mohit Marathe <[email protected]>--- t/unit-tests/test-lib.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)t/unit-tests/test-lib.c +0
-0
t/unit-tests/test-lib.c static const char *make_relative(const char *location) · line 55
Quoted 7 lines
t/unit-tests/test-lib.c +1
-3@@ -52,9 +52,7 @@ static const char *make_relative(const char *location) 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] ='/' ; +change_path_separators (&needle[0 ]);
Why not just:change_path_separators(needle);
?
t/unit-tests/test-lib.c static const char *make_relative(const char *location) · line 90
Quoted 12 lines
t/unit-tests/test-lib.c +2
-3need_bs_to_fs =0 ; }else { need_bs_to_fs =1 ;@@ -88,9 +86,8 @@ static const char *make_relative(const char *location)/* convert backslashes to forward slashes */ strlcpy (buf, location + prefix_len,sizeof (buf)); -for (p = buf; *p; p++) -if (*p =='\\' ) - *p ='/' ; + p = buf; +change_path_separators (p);
And here, why not:change_path_separators(buf)
?
Quoted 6 lines
t/unit-tests/test-lib.c +0
-0return buf; }Signature · 1 line
gitgitgadget