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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:

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



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]