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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 23, 2016, 16:34 UTC
Message-ID: <xmqqpoulcfv3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1603230821420.4690@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 17 quoted lines

>> > 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.

I was able to guess that it would be the reason, and I was willing to accept this as a short-term workaround.

As you are very well aware, the usual pattern we use is to implement a higher level function (e.g. expand_user_path() in this case) in terms of helpers that offer abstraction of implementation details that may be platform specific (e.g. getenv() may be implemented differently on Windows). An "#ifdef" in otherwise platform agnostic codepath like this one is a sign that the code is not well thought out to find the right abstraction to use to follow that pattern.

I was mostly reacting to that "#ifdef" and thinking aloud what the right abstraction is appropriate. As a short-term workaround, the above would have to do.

And no, I do not think convert_slashes() that becomes no-op on non-windows platforms is the right abstraction.