Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 22 Mar 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > On Windows, the backslash is the native directory separator, but
> > all supported Windows versions also accept the forward slash in
> > most circumstances.
> >
> > Our tests expect forward slashes.
> >
> > Relative paths are generated by Git using forward slashes.
> 
> ... and the paths tracked by Git (in the index) use slashes.
> 
> > So let's try to be consistent and use forward slashes in the $HOME
> > part of the paths reported by `git config --show-origin`, too.
> 
> OK, sounds sensible.
> 
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  compat/mingw.h | 6 ++++++
> >  path.c         | 3 +++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/compat/mingw.h b/compat/mingw.h
> > index 8c5bf50..c008694 100644
> > --- a/compat/mingw.h
> > +++ b/compat/mingw.h
> > @@ -396,6 +396,12 @@ static inline char *mingw_find_last_dir_sep(const char *path)
> >  			ret = (char *)path;
> >  	return ret;
> >  }
> > +static inline void convert_slashes(char *path)
> > +{
> > +	for (; *path; path++)
> > +		if (*path == '\\')
> > +			*path = '/';
> > +}
> >  #define find_last_dir_sep mingw_find_last_dir_sep
> >  int mingw_offset_1st_component(const char *path);
> >  #define offset_1st_component mingw_offset_1st_component
> > diff --git a/path.c b/path.c
> > index 8b7e168..969b494 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -584,6 +584,9 @@ char *expand_user_path(const char *path)
> >  			if (!home)
> >  				goto return_null;
> >  			strbuf_addstr(&user_path, home);
> > +#ifdef GIT_WINDOWS_NATIVE
> > +			convert_slashes(user_path.buf);
> > +#endif
> 
> Hmm, I wonder if we want to do this at a bit lower level,

Well, I tried to be careful. There *are* circumstamces when backslashes
are required (CreateProcess() comes to mind), so I wanted to have this
conversion as much only in the user-visible output as possible.

> e.g.
> 
>     1. have a fallback for other platforms in git-compat-util.h
> 
>     #ifndef get_HOME
>     #define get_HOME() getenv("HOME")
>     #endif

Once upon a time, I had something like that (without the ugly uppercase,
to account for the fact that Windows has no single HOME setting):

	http://thread.gmane.org/gmane.comp.version-control.msysgit/20339

>     2. have the one that does convert-slashes for mingw
> 
>     static inline const char *mingw_getenv_HOME(void) {
>     	... do convert-slashes on the result of getenv("HOME");
>     }
>     #define get_HOME() mingw_getenv_HOME()

We could do that much easier now:

-- snip --
diff --git a/compat/mingw.c b/compat/mingw.c
index 54c82ec..96022b7 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2117,6 +2117,12 @@ static void setup_windows_environment()
 				*tmp = '/';
 	}
 
+	tmp = getenv("HOME");
+	if (tmp)
+		for (; *tmp; tmp++)
+			if (*tmp == '\\')
+				*tmp = '/';
+
 	/* simulate TERM to enable auto-color (see color.c) */
 	if (!getenv("TERM"))
 		setenv("TERM", "cygwin", 1);
-- snap --

But again, I am uncomfortable to do this wholesale without having the time
to do anything quickly about unintended side effects.


>     3. Instead of the above patch to path.c, change the line before
>        the precontext with s/getenv("HOME")/get_HOME()/
> 
> Or even lower, inside mingw_getenv() and catch variables that deal
> with paths (not just HOME but PATH, TMP, TMPDIR, etc.)

That outright scares me. Don't do that.

:-)

Again, just to make sure my point is heard: there *are* circumstances when
the Win32 API expects backslashes and cannot handle forward ones. I would
not dare to claim to be an expert in that matter, so I would rather want
to err on the side of converting backslashes to forward slashes *only*
when really needed.

Ciao,
Dscho