Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

git-compat-util: move convert_slashes from compat/mingw.h and rename it #1699

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

git-compat-util: move convert_slashes from compat/mingw.h and rename it #1699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0157b1d
git-compat-util: migrate `convert_slashes()` from compat/mingw.h
mohit-marathe Mar 18, 2024
8a730b6
test-lib: replace repeated code logic with an existing helper
mohit-marathe Mar 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions abspath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)
strbuf_reset(resolved);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Mohit Marathe via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Mohit Marathe <mohitmarathe@proton.me>
>
> This patch migrates the `convert_slashes` function to `git-compat-
> util.h` and renames it to `change_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.

> diff --git a/abspath.c b/abspath.c
> index 1202cde23db..ea35e2c05ce 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -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.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 7c2a6538e5a..3db90c09295 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -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
> diff --git a/path.c b/path.c
> index 8bb223c92c9..cd7c88ffa0d 100644
> --- a/path.c
> +++ b/path.c
> @@ -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.

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);
}
Expand Down Expand Up @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions compat/mingw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) */
Expand Down
6 changes: 0 additions & 6 deletions compat/mingw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions git-compat-util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion path.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
9 changes: 3 additions & 6 deletions t/unit-tests/test-lib.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,7 @@ static const char *make_relative(const char *location)
prefix_len = len - needle_len;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Mohit Marathe via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Mohit Marathe <mohitmarathe@proton.me>
>
> 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.

> @@ -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;
>  }

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Josh Steadmon wrote (reply to this):

On 2024.03.18 12:47, Mohit Marathe via GitGitGadget wrote:
> From: Mohit Marathe <mohitmarathe@proton.me>
> 
> This patch replaces the code, that changes the path separators,
> with the already existing function `change_path_separators()`
> 
> Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
> ---
>  t/unit-tests/test-lib.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/t/unit-tests/test-lib.c b/t/unit-tests/test-lib.c
> index 66d6980ffbe..b0e26263046 100644
> --- a/t/unit-tests/test-lib.c
> +++ b/t/unit-tests/test-lib.c
> @@ -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);
?

>  			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);

And here, why not:
change_path_separators(buf)
?

>  	return buf;
>  }
>  
> -- 
> gitgitgadget
>

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;
Expand Down Expand Up @@ -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;
}

Expand Down
Loading