{"thread":{"id":"61143","subject":"[PATCH 0/2] git-compat-util: move convert_slashes from compat/mingw.h and rename it","startedAt":"2024-03-18T12:47:45Z","lastAt":"2024-04-11T21:42:14Z","messageCount":6,"participants":["Mohit Marathe via GitGitGadget","Junio C Hamano","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"490859","messageId":"pull.1699.git.1710766062.gitgitgadget@gmail.com","threadId":"61143","inReplyTo":null,"subject":"[PATCH 0/2] git-compat-util: move convert_slashes from compat/mingw.h and rename it","fromName":"Mohit Marathe via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-18T12:47:40Z","receivedAt":"2024-03-18T12:47:45Z","isPatch":true,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":"Hi all,\n\nThis series aims to complete a #leftoverbits: https://lore.kernel.org/\ngit/xmqqzfw43xad.fsf@gitster.g/ by moving convert_slashes() to\ngit-compat-util.h and renaming it to change_path_separators().\n\nI appreciate your review and feedback on this patch series.\n\nBest regards, Mohit Marathe\n\nMohit Marathe (2):\n  git-compat-util: migrate `convert_slashes()` from compat/mingw.h\n  test-lib: replace repeated code logic with an existing helper\n\n abspath.c               | 4 ++--\n compat/mingw.c          | 4 ++--\n compat/mingw.h          | 6 ------\n git-compat-util.h       | 7 +++++++\n path.c                  | 2 +-\n t/unit-tests/test-lib.c | 9 +++------\n 6 files changed, 15 insertions(+), 17 deletions(-)\n\n\nbase-commit: 2953d95d402b6bff1a59c4712f4d46f1b9ea137f\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1699%2Fmohit-marathe%2Fconvert-slashes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1699/mohit-marathe/convert-slashes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1699\n-- \ngitgitgadget\n"},{"id":"490860","messageId":"0157b1deaebd5de4734d35f96f22a8da752e09bd.1710766062.git.gitgitgadget@gmail.com","threadId":"61143","inReplyTo":"pull.1699.git.1710766062.gitgitgadget@gmail.com","subject":"[PATCH 1/2] git-compat-util: migrate `convert_slashes()` from compat/mingw.h","fromName":"Mohit Marathe via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-18T12:47:41Z","receivedAt":"2024-03-18T12:47:46Z","isPatch":true,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":"From: Mohit Marathe <mohitmarathe@proton.me>\n\nThis patch migrates the `convert_slashes` function to `git-compat-\nutil.h` and renames it to `change_path_separators`.\n\nSigned-off-by: Mohit Marathe <mohitmarathe@proton.me>\n---\n abspath.c         | 4 ++--\n compat/mingw.c    | 4 ++--\n compat/mingw.h    | 6 ------\n git-compat-util.h | 7 +++++++\n path.c            | 2 +-\n 5 files changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 1202cde23db..ea35e2c05ce 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -58,7 +58,7 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)\n \tstrbuf_reset(resolved);\n \tstrbuf_add(resolved, remaining->buf, offset);\n #ifdef GIT_WINDOWS_NATIVE\n-\tconvert_slashes(resolved->buf);\n+\tchange_path_separators(resolved->buf);\n #endif\n \tstrbuf_remove(remaining, 0, offset);\n }\n@@ -278,7 +278,7 @@ char *prefix_filename(const char *pfx, const char *arg)\n \n \tstrbuf_addstr(&path, arg);\n #ifdef GIT_WINDOWS_NATIVE\n-\tconvert_slashes(path.buf + pfx_len);\n+\tchange_path_separators(path.buf + pfx_len);\n #endif\n \treturn strbuf_detach(&path, NULL);\n }\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 320fb99a90e..f7c1a009563 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1170,7 +1170,7 @@ char *mingw_getcwd(char *pointer, int len)\n \t}\n \tif (xwcstoutf(pointer, wpointer, len) < 0)\n \t\treturn NULL;\n-\tconvert_slashes(pointer);\n+\tchange_path_separators(pointer);\n \treturn pointer;\n }\n \n@@ -2636,7 +2636,7 @@ static void setup_windows_environment(void)\n \t\t * executable (by not mistaking the dir separators\n \t\t * for escape characters).\n \t\t */\n-\t\tconvert_slashes(tmp);\n+\t\tchange_path_separators(tmp);\n \t}\n \n \t/* simulate TERM to enable auto-color (see color.c) */\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 6aec50e4124..f5ca4adc194 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -448,12 +448,6 @@ HANDLE winansi_get_osfhandle(int fd);\n  * git specific compatibility\n  */\n \n-static inline void convert_slashes(char *path)\n-{\n-\tfor (; *path; path++)\n-\t\tif (*path == '\\\\')\n-\t\t\t*path = '/';\n-}\n #define PATH_SEP ';'\n char *mingw_query_user_email(void);\n #define query_user_email mingw_query_user_email\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 7c2a6538e5a..3db90c09295 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -1309,6 +1309,13 @@ static inline int strtol_i(char const *s, int base, int *result)\n \treturn 0;\n }\n \n+static inline void change_path_separators(char *path)\n+{\n+\tfor (; *path; path++)\n+\t\tif (*path == '\\\\')\n+\t\t\t*path = '/';\n+}\n+\n void git_stable_qsort(void *base, size_t nmemb, size_t size,\n \t\t      int(*compar)(const void *, const void *));\n #ifdef INTERNAL_QSORT\ndiff --git a/path.c b/path.c\nindex 8bb223c92c9..cd7c88ffa0d 100644\n--- a/path.c\n+++ b/path.c\n@@ -758,7 +758,7 @@ char *interpolate_path(const char *path, int real_home)\n \t\t\telse\n \t\t\t\tstrbuf_addstr(&user_path, home);\n #ifdef GIT_WINDOWS_NATIVE\n-\t\t\tconvert_slashes(user_path.buf);\n+\t\t\tchange_path_separators(user_path.buf);\n #endif\n \t\t} else {\n \t\t\tstruct passwd *pw = getpw_str(username, username_len);\n-- \ngitgitgadget\n\n"},{"id":"490861","messageId":"8a730b6ebba4111aaad4e8fc938c52f6e7e54480.1710766062.git.gitgitgadget@gmail.com","threadId":"61143","inReplyTo":"pull.1699.git.1710766062.gitgitgadget@gmail.com","subject":"[PATCH 2/2] test-lib: replace repeated code logic with an existing helper","fromName":"Mohit Marathe via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-18T12:47:42Z","receivedAt":"2024-03-18T12:47:47Z","isPatch":true,"sender":{"key":"mohitmarathe@proton.me","avatar":"https://gravatar.com/avatar/3bf18f96e6d36cef960077143b9880e8b22468d960da18bf0b34c4e87d4df7e9?d=mp&s=160"},"body":"From: Mohit Marathe <mohitmarathe@proton.me>\n\nThis patch replaces the code, that changes the path separators,\nwith the already existing function `change_path_separators()`\n\nSigned-off-by: Mohit Marathe <mohitmarathe@proton.me>\n---\n t/unit-tests/test-lib.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c\nindex 66d6980ffbe..b0e26263046 100644\n--- a/t/unit-tests/test-lib.c\n+++ b/t/unit-tests/test-lib.c\n@@ -52,9 +52,7 @@ static const char *make_relative(const char *location)\n \t\tprefix_len = len - needle_len;\n \t\tif (prefix[prefix_len + 1] == '/') {\n \t\t\t/* Oh, we're not Windows */\n-\t\t\tfor (size_t i = 0; i < needle_len; i++)\n-\t\t\t\tif (needle[i] == '\\\\')\n-\t\t\t\t\tneedle[i] = '/';\n+\t\t\tchange_path_separators(&needle[0]);\n \t\t\tneed_bs_to_fs = 0;\n \t\t} else {\n \t\t\tneed_bs_to_fs = 1;\n@@ -88,9 +86,8 @@ static const char *make_relative(const char *location)\n \n \t/* convert backslashes to forward slashes */\n \tstrlcpy(buf, location + prefix_len, sizeof(buf));\n-\tfor (p = buf; *p; p++)\n-\t\tif (*p == '\\\\')\n-\t\t\t*p = '/';\n+\tp = buf;\n+\tchange_path_separators(p);\n \treturn buf;\n }\n \n-- \ngitgitgadget\n"},{"id":"490901","messageId":"xmqqh6h3dwyp.fsf@gitster.g","threadId":"61143","inReplyTo":"0157b1deaebd5de4734d35f96f22a8da752e09bd.1710766062.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] git-compat-util: migrate `convert_slashes()` from compat/mingw.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-18T21:57:50Z","receivedAt":"2024-03-18T21:57:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Mohit Marathe via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Mohit Marathe <mohitmarathe@proton.me>\n>\n> This patch migrates the `convert_slashes` function to `git-compat-\n> util.h` and renames it to `change_path_separators`.\n\nThat is something a reader can tell from looking at what the patch\ndoes.  What they cannot read from the diff is why the author of the\npatch thought it was a good idea and that is what we want to see\nhere.\n\n> diff --git a/abspath.c b/abspath.c\n> index 1202cde23db..ea35e2c05ce 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -58,7 +58,7 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)\n>  \tstrbuf_reset(resolved);\n>  \tstrbuf_add(resolved, remaining->buf, offset);\n>  #ifdef GIT_WINDOWS_NATIVE\n> -\tconvert_slashes(resolved->buf);\n> +\tchange_path_separators(resolved->buf);\n>  #endif\n\nIn the context of \"#ifdef GIT_WINDOWS_NATIVE..#endif\" conditional\ncompilation, the name convert_slashes() may have made some sense,\ni.e. \"on this system, we get a path with the kind of slash that is\nnot suitable to be used in Git, so we correct it to the other kind\nof slash\".  But neither it, or change_path_separators(), as a name\nof public function makes much sense.  The direction of the change\nis totally unclear.\n\n\"normalize\" may be a verb that has some connotation which direction\nthe conversion is going, but still, without knowing why this change\nis being made (not the name change, but the part that makes it public),\nI cannot offer much better candidates for its name.\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 7c2a6538e5a..3db90c09295 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -1309,6 +1309,13 @@ static inline int strtol_i(char const *s, int base, int *result)\n>  \treturn 0;\n>  }\n>  \n> +static inline void change_path_separators(char *path)\n> +{\n> +\tfor (; *path; path++)\n> +\t\tif (*path == '\\\\')\n> +\t\t\t*path = '/';\n> +}\n> +\n>  void git_stable_qsort(void *base, size_t nmemb, size_t size,\n>  \t\t      int(*compar)(const void *, const void *));\n>  #ifdef INTERNAL_QSORT\n> diff --git a/path.c b/path.c\n> index 8bb223c92c9..cd7c88ffa0d 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -758,7 +758,7 @@ char *interpolate_path(const char *path, int real_home)\n>  \t\t\telse\n>  \t\t\t\tstrbuf_addstr(&user_path, home);\n>  #ifdef GIT_WINDOWS_NATIVE\n> -\t\t\tconvert_slashes(user_path.buf);\n> +\t\t\tchange_path_separators(user_path.buf);\n>  #endif\n>  \t\t} else {\n>  \t\t\tstruct passwd *pw = getpw_str(username, username_len);\n\nIf the idea were that \"here we know that the path came from reading\nfilesystem and on platforms that use backslashes as path separators,\nwe need to normalize them into slashes before the path is usable in\nGit\", then I might understand if a change were more like:\n\n * Introduce normalize_path_separators_in_place(char *) that is a\n   NOOP by default, but does the backslash->slash conversion ONLY on\n   platforms that require it (i.e. Windows).\n\n * Lose \"#ifdef GIT_WINDOWS_NATIVE\" and corresponding \"#endif\", and\n   call that normalize thing unconditionally, which gets optimized\n   away on most systems other then where it is required.\n\nThat way, the affected .c files would become somewhat easier to\nfollow without ugly conditional compilation.\n\nBut with the proposed patch lacking any explanation why the author\nthought it was a good idea, the above is just my wild guess.\n"},{"id":"490902","messageId":"xmqq7chzdwwu.fsf@gitster.g","threadId":"61143","inReplyTo":"8a730b6ebba4111aaad4e8fc938c52f6e7e54480.1710766062.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] test-lib: replace repeated code logic with an existing helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-18T21:58:57Z","receivedAt":"2024-03-18T21:59:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Mohit Marathe via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Mohit Marathe <mohitmarathe@proton.me>\n>\n> This patch replaces the code, that changes the path separators,\n> with the already existing function `change_path_separators()`\n\nAside from that the name change_path_separators() needs to be\nupdated, using it here means that the helper cannot become a NO-OP\non platforms where we do not have to change backslashes we read from\nthe system to forward slashes.  So if we really wanted to use it\nhere, and update the other existing callers in the main code to help\nthem lose the ugly \"#if WINDOWS\" conditional compilation, we would\nneed two helper functions, perhaps one that is used here that is\nidentical to the current convert_slashes(), and the other one used\nto clean-up the callsites in [1/2] to also remove the conditional\ncompilation.\n\nEven if we were to make it public, I am not sure if compat-util is\nthe right place, though.  It is not a kitchen sink.\n\nIn any case, perhaps we want to do something like this in a header,\n...\n\n\tstatic inline void bs_to_forward_slash_in_place(char *path)\n\t{\n\t\t...\n\t}\n\t#ifdef GIT_WINDOWS_NATIVE\n\t#define normalize_slashes_in_place(path) bs_to_forward_slash_in_place(path)\n\t#else\n\t#define normalize_slashes_in_place(path) do { ; /* nothing */ } while (0)\n\t#endif\n\n... and then use the \"normalize\" one to lose \"#ifdef\" in the callers\n[1/2] touches, while \"bs_to_forward_slash\" one here.\n\nI am not convinced if it is worth doing only for this single test,\nthough.\n\n> @@ -88,9 +86,8 @@ static const char *make_relative(const char *location)\n>  \n>  \t/* convert backslashes to forward slashes */\n>  \tstrlcpy(buf, location + prefix_len, sizeof(buf));\n> -\tfor (p = buf; *p; p++)\n> -\t\tif (*p == '\\\\')\n> -\t\t\t*p = '/';\n> +\tp = buf;\n> +\tchange_path_separators(p);\n>  \treturn buf;\n>  }\n"},{"id":"492809","messageId":"ZhhZMOWnrni94WTD@google.com","threadId":"61143","inReplyTo":"8a730b6ebba4111aaad4e8fc938c52f6e7e54480.1710766062.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] test-lib: replace repeated code logic with an existing helper","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-11T21:42:08Z","receivedAt":"2024-04-11T21:42:14Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.18 12:47, Mohit Marathe via GitGitGadget wrote:\n> From: Mohit Marathe <mohitmarathe@proton.me>\n> \n> This patch replaces the code, that changes the path separators,\n> with the already existing function `change_path_separators()`\n> \n> Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>\n> ---\n>  t/unit-tests/test-lib.c | 9 +++------\n>  1 file changed, 3 insertions(+), 6 deletions(-)\n> \n> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c\n> index 66d6980ffbe..b0e26263046 100644\n> --- a/t/unit-tests/test-lib.c\n> +++ b/t/unit-tests/test-lib.c\n> @@ -52,9 +52,7 @@ static const char *make_relative(const char *location)\n>  \t\tprefix_len = len - needle_len;\n>  \t\tif (prefix[prefix_len + 1] == '/') {\n>  \t\t\t/* Oh, we're not Windows */\n> -\t\t\tfor (size_t i = 0; i < needle_len; i++)\n> -\t\t\t\tif (needle[i] == '\\\\')\n> -\t\t\t\t\tneedle[i] = '/';\n> +\t\t\tchange_path_separators(&needle[0]);\n\nWhy not just:\nchange_path_separators(needle);\n?\n\n>  \t\t\tneed_bs_to_fs = 0;\n>  \t\t} else {\n>  \t\t\tneed_bs_to_fs = 1;\n> @@ -88,9 +86,8 @@ static const char *make_relative(const char *location)\n>  \n>  \t/* convert backslashes to forward slashes */\n>  \tstrlcpy(buf, location + prefix_len, sizeof(buf));\n> -\tfor (p = buf; *p; p++)\n> -\t\tif (*p == '\\\\')\n> -\t\t\t*p = '/';\n> +\tp = buf;\n> +\tchange_path_separators(p);\n\nAnd here, why not:\nchange_path_separators(buf)\n?\n\n>  \treturn buf;\n>  }\n>  \n> -- \n> gitgitgadget\n> \n"}]}