| Message ID | 2287dd96cf0b9e9e250fdf92a32dcf666510e67d.1541515994.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mingw: handle absolute paths in expand_user_path() | expand |
On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > On Windows, an absolute POSIX path needs to be turned into a Windows > one. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > path.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/path.c b/path.c > index 34f0f98349..a72abf0e1f 100644 > --- a/path.c > +++ b/path.c > @@ -11,6 +11,7 @@ > #include "path.h" > #include "packfile.h" > #include "object-store.h" > +#include "exec-cmd.h" > > static int get_st_mode_bits(const char *path, int *mode) > { > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) > > if (path == NULL) > goto return_null; > +#ifdef __MINGW32__ > + if (path[0] == '/') > + return system_path(path + 1); > +#endif Hmm, this doesn't quite fit with the intended use of this function! ;-) (even on windows!) I haven't looked very deeply, but doesn't this affect all absolute paths in the config read by git_config_pathname(), along with all 'included config' files? I am pretty sure that I would not want the absolute paths in my config file(s) magically 'moved' depending on whether git has been compiled with 'runtime prefix' support or not! ATB, Ramsay Jones > if (path[0] == '~') { > const char *first_slash = strchrnul(path, '/'); > const char *username = path + 1; >
On 06/11/2018 15:54, Ramsay Jones wrote: > > > On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote: >> From: Johannes Schindelin <johannes.schindelin@gmx.de> >> >> On Windows, an absolute POSIX path needs to be turned into a Windows >> one. >> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >> --- >> path.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/path.c b/path.c >> index 34f0f98349..a72abf0e1f 100644 >> --- a/path.c >> +++ b/path.c >> @@ -11,6 +11,7 @@ >> #include "path.h" >> #include "packfile.h" >> #include "object-store.h" >> +#include "exec-cmd.h" >> >> static int get_st_mode_bits(const char *path, int *mode) >> { >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) >> >> if (path == NULL) >> goto return_null; >> +#ifdef __MINGW32__ >> + if (path[0] == '/') >> + return system_path(path + 1); >> +#endif > > Hmm, this doesn't quite fit with the intended use of this > function! ;-) (even on windows!) > > I haven't looked very deeply, but doesn't this affect all > absolute paths in the config read by git_config_pathname(), > along with all 'included config' files? > > I am pretty sure that I would not want the absolute paths > in my config file(s) magically 'moved' depending on whether > git has been compiled with 'runtime prefix' support or not! So, I hit 'send' before finishing my thought ... I can't think of a non-backwards compatible way of doing what you want. If backward compatibility wasn't an issue, then we could (maybe) have used some kind of pathname prefix like 'system:/path/relative/to/git/executable', or somesuch. ATB, Ramsay Jones
On Tue, Nov 6, 2018 at 3:55 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > On Windows, an absolute POSIX path needs to be turned into a Windows > one. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > path.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/path.c b/path.c > index 34f0f98349..a72abf0e1f 100644 > --- a/path.c > +++ b/path.c > @@ -11,6 +11,7 @@ > #include "path.h" > #include "packfile.h" > #include "object-store.h" > +#include "exec-cmd.h" > > static int get_st_mode_bits(const char *path, int *mode) > { > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) > > if (path == NULL) > goto return_null; > +#ifdef __MINGW32__ > + if (path[0] == '/') > + return system_path(path + 1); > +#endif Should this behavior be documented somewhere, maybe in config.txt? > if (path[0] == '~') { > const char *first_slash = strchrnul(path, '/'); > const char *username = path + 1; > -- > gitgitgadget
On Tue, Nov 6, 2018 at 7:15 PM Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) > >> > >> if (path == NULL) > >> goto return_null; > >> +#ifdef __MINGW32__ > >> + if (path[0] == '/') > >> + return system_path(path + 1); > >> +#endif > > > > Hmm, this doesn't quite fit with the intended use of this > > function! ;-) (even on windows!) > > > > I haven't looked very deeply, but doesn't this affect all > > absolute paths in the config read by git_config_pathname(), > > along with all 'included config' files? > > > > I am pretty sure that I would not want the absolute paths > > in my config file(s) magically 'moved' depending on whether > > git has been compiled with 'runtime prefix' support or not! > > So, I hit 'send' before finishing my thought ... > > I can't think of a non-backwards compatible way of doing > what you want. If backward compatibility wasn't an issue, > then we could (maybe) have used some kind of pathname prefix > like 'system:/path/relative/to/git/executable', or somesuch. A pseudo variable might work, like $ROOT/path/relative/to/somewhere
Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > On Windows, an absolute POSIX path needs to be turned into a Windows > one. If I were picky, I would say that in a pure Windows application there cannot be POSIX paths to begin with. Even if a path looks like a POSIX paths, i.e. it starts with a directory separator, but not with drive-letter-colon, it still has a particular meaning, namely (as far as I know) that the path is anchored at the root of the drive of the current working directory. If a user specifies such a path on Windows, and it must undergo expand_user_path(), then that is a user error, or the user knows what they are doing. I think it is wrong to interpret such a path as relative to some random other directory, like this patch seems to do. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > path.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/path.c b/path.c > index 34f0f98349..a72abf0e1f 100644 > --- a/path.c > +++ b/path.c > @@ -11,6 +11,7 @@ > #include "path.h" > #include "packfile.h" > #include "object-store.h" > +#include "exec-cmd.h" > > static int get_st_mode_bits(const char *path, int *mode) > { > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) > > if (path == NULL) > goto return_null; > +#ifdef __MINGW32__ > + if (path[0] == '/') > + return system_path(path + 1); > +#endif > if (path[0] == '~') { > const char *first_slash = strchrnul(path, '/'); > const char *username = path + 1; >
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote: >> From: Johannes Schindelin <johannes.schindelin@gmx.de> >> >> On Windows, an absolute POSIX path needs to be turned into a Windows >> one. >> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >> --- >> path.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/path.c b/path.c >> index 34f0f98349..a72abf0e1f 100644 >> --- a/path.c >> +++ b/path.c >> @@ -11,6 +11,7 @@ >> #include "path.h" >> #include "packfile.h" >> #include "object-store.h" >> +#include "exec-cmd.h" >> >> static int get_st_mode_bits(const char *path, int *mode) >> { >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) >> >> if (path == NULL) >> goto return_null; >> +#ifdef __MINGW32__ >> + if (path[0] == '/') >> + return system_path(path + 1); >> +#endif > > Hmm, this doesn't quite fit with the intended use of this > function! ;-) (even on windows!) > > I haven't looked very deeply, but doesn't this affect all > absolute paths in the config read by git_config_pathname(), > along with all 'included config' files? I think so. I have not thought things through to say if replacing a "full path in the current drive" with system_path() is a sensible thing to do in the first place, but I am getting the impression from review comments that it probably is not. > I am pretty sure that I would not want the absolute paths > in my config file(s) magically 'moved' depending on whether > git has been compiled with 'runtime prefix' support or not! In any case, the helper is about expanding ~/foo and ~who/foo to absolute paths, without touching other paths, so it is a wrong function to implement it in, even if the motivation were sensible.
Hi, On Wed, 7 Nov 2018, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > > > On 06/11/2018 14:53, Johannes Schindelin via GitGitGadget wrote: > >> From: Johannes Schindelin <johannes.schindelin@gmx.de> > >> > >> On Windows, an absolute POSIX path needs to be turned into a Windows > >> one. > >> > >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > >> --- > >> path.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/path.c b/path.c > >> index 34f0f98349..a72abf0e1f 100644 > >> --- a/path.c > >> +++ b/path.c > >> @@ -11,6 +11,7 @@ > >> #include "path.h" > >> #include "packfile.h" > >> #include "object-store.h" > >> +#include "exec-cmd.h" > >> > >> static int get_st_mode_bits(const char *path, int *mode) > >> { > >> @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) > >> > >> if (path == NULL) > >> goto return_null; > >> +#ifdef __MINGW32__ > >> + if (path[0] == '/') > >> + return system_path(path + 1); > >> +#endif > > > > Hmm, this doesn't quite fit with the intended use of this > > function! ;-) (even on windows!) > > > > I haven't looked very deeply, but doesn't this affect all > > absolute paths in the config read by git_config_pathname(), > > along with all 'included config' files? > > I think so. I have not thought things through to say if replacing a > "full path in the current drive" with system_path() is a sensible > thing to do in the first place, but I am getting the impression from > review comments that it probably is not. > > > I am pretty sure that I would not want the absolute paths > > in my config file(s) magically 'moved' depending on whether > > git has been compiled with 'runtime prefix' support or not! The cute thing is: your absolute paths would not be moved because we are talking about Windows. Therefore your absolute paths would not start with a forward slash. > In any case, the helper is about expanding ~/foo and ~who/foo to > absolute paths, without touching other paths, so it is a wrong > function to implement it in, even if the motivation were sensible. It could be renamed. In any case, for this feature we would need to expand a path that is not the final path, and this here location is the most logical place to do so. Ciao, Dscho
Hi, On Tue, 6 Nov 2018, Duy Nguyen wrote: > On Tue, Nov 6, 2018 at 3:55 PM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > On Windows, an absolute POSIX path needs to be turned into a Windows > > one. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > path.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/path.c b/path.c > > index 34f0f98349..a72abf0e1f 100644 > > --- a/path.c > > +++ b/path.c > > @@ -11,6 +11,7 @@ > > #include "path.h" > > #include "packfile.h" > > #include "object-store.h" > > +#include "exec-cmd.h" > > > > static int get_st_mode_bits(const char *path, int *mode) > > { > > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) > > > > if (path == NULL) > > goto return_null; > > +#ifdef __MINGW32__ > > + if (path[0] == '/') > > + return system_path(path + 1); > > +#endif > > Should this behavior be documented somewhere, maybe in config.txt? First we need to find a consensus how this should work. Ciao, Dscho > > > if (path[0] == '~') { > > const char *first_slash = strchrnul(path, '/'); > > const char *username = path + 1; > > -- > > gitgitgadget > -- > Duy >
Hi Hannes, On Tue, 6 Nov 2018, Johannes Sixt wrote: > Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > On Windows, an absolute POSIX path needs to be turned into a Windows > > one. > > If I were picky, I would say that in a pure Windows application there cannot > be POSIX paths to begin with. > > Even if a path looks like a POSIX paths, i.e. it starts with a directory > separator, but not with drive-letter-colon, it still has a particular meaning, > namely (as far as I know) that the path is anchored at the root of the drive > of the current working directory. > > If a user specifies such a path on Windows, and it must undergo > expand_user_path(), then that is a user error, or the user knows what they are > doing. > > I think it is wrong to interpret such a path as relative to some random other > directory, like this patch seems to do. Okay, now we know everything you find wrong with the current patch. Do you have any suggestion how to make it right? I.e. what would you suggest as a way to specify in a gitconfig in a portable Git where the certificate bundle is? Thanks, Dscho > > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > path.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/path.c b/path.c > > index 34f0f98349..a72abf0e1f 100644 > > --- a/path.c > > +++ b/path.c > > @@ -11,6 +11,7 @@ > > #include "path.h" > > #include "packfile.h" > > #include "object-store.h" > > +#include "exec-cmd.h" > > > > static int get_st_mode_bits(const char *path, int *mode) > > { > > @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) > > > > if (path == NULL) > > goto return_null; > > +#ifdef __MINGW32__ > > + if (path[0] == '/') > > + return system_path(path + 1); > > +#endif > > if (path[0] == '~') { > > const char *first_slash = strchrnul(path, '/'); > > const char *username = path + 1; > > > > >
On 07/11/2018 11:19, Johannes Schindelin wrote: [snip] >>> Hmm, this doesn't quite fit with the intended use of this >>> function! ;-) (even on windows!) >>> >>> I haven't looked very deeply, but doesn't this affect all >>> absolute paths in the config read by git_config_pathname(), >>> along with all 'included config' files? >> >> I think so. I have not thought things through to say if replacing a >> "full path in the current drive" with system_path() is a sensible >> thing to do in the first place, but I am getting the impression from >> review comments that it probably is not. >> >>> I am pretty sure that I would not want the absolute paths >>> in my config file(s) magically 'moved' depending on whether >>> git has been compiled with 'runtime prefix' support or not! > > The cute thing is: your absolute paths would not be moved because we are > talking about Windows. Therefore your absolute paths would not start with > a forward slash. Ah, sorry, I must have misunderstood a comment in your cover letter: The reason is this: something like this (make paths specified e.g. via http.sslCAInfo relative to the runtime prefix) is potentially useful also in the non-Windows context, as long as Git was built with the runtime prefix feature. ... so I thought you meant to add this code for POSIX systems as well. My mistake. :( ATB, Ramsay Jones
Am 07.11.18 um 12:23 schrieb Johannes Schindelin: > On Tue, 6 Nov 2018, Johannes Sixt wrote: > >> Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget: >> Even if a path looks like a POSIX paths, i.e. it starts with a directory >> separator, but not with drive-letter-colon, it still has a particular meaning, >> namely (as far as I know) that the path is anchored at the root of the drive >> of the current working directory. >> >> If a user specifies such a path on Windows, and it must undergo >> expand_user_path(), then that is a user error, or the user knows what they are >> doing. >> >> I think it is wrong to interpret such a path as relative to some random other >> directory, like this patch seems to do. > > Okay, now we know everything you find wrong with the current patch. Do you > have any suggestion how to make it right? I.e. what would you suggest as a > way to specify in a gitconfig in a portable Git where the certificate > bundle is? Ah, so your actual problem is quite a different one! Do I understand correctly, that you use a leading slash as an indicator to construct a path relative to system_path(). How about a "reserved" user name? For example, [http] sslcert = ~system_path/what/ever although a more unique name, perhaps with some punctuation, may be desirable. -- Hannes
On Wed, Nov 07, 2018 at 07:52:28PM +0100, Johannes Sixt wrote: > > Okay, now we know everything you find wrong with the current patch. Do you > > have any suggestion how to make it right? I.e. what would you suggest as a > > way to specify in a gitconfig in a portable Git where the certificate > > bundle is? > > Ah, so your actual problem is quite a different one! > > Do I understand correctly, that you use a leading slash as an indicator to > construct a path relative to system_path(). How about a "reserved" user > name? For example, > > [http] sslcert = ~system_path/what/ever > > although a more unique name, perhaps with some punctuation, may be > desirable. It's syntactically a bit further afield, but something like: [http] sslcert = $RUNTIME_PREFIX/what/ever would make sense to me, and is a bit less subtle than the fake user. I don't know if that would confuse people into thinking that we interpolate arbitrary environment variables, though. -Peff
Am 07.11.18 um 21:41 schrieb Jeff King: > On Wed, Nov 07, 2018 at 07:52:28PM +0100, Johannes Sixt wrote: >> Do I understand correctly, that you use a leading slash as an indicator to >> construct a path relative to system_path(). How about a "reserved" user >> name? For example, >> >> [http] sslcert = ~system_path/what/ever >> >> although a more unique name, perhaps with some punctuation, may be >> desirable. > > It's syntactically a bit further afield, but something like: > > [http] > sslcert = $RUNTIME_PREFIX/what/ever > > would make sense to me, and is a bit less subtle than the fake user. I > don't know if that would confuse people into thinking that we > interpolate arbitrary environment variables, though. The expansion of a fake user name would have to go in expand_user_path(), a fake variable name would have to be expanded elsewhere. Now, Dscho mentions in the cover letter that his patch has already seen some field testing ("has been 'in production' for a good while"). So, we have gained some confidence that the point where the substitution happens, in expand_user_path(), is suitable. Therefore, I have slight preference for a fake user. -- Hannes
On Wed, Nov 07, 2018 at 10:36:52PM +0100, Johannes Sixt wrote: > Am 07.11.18 um 21:41 schrieb Jeff King: > > On Wed, Nov 07, 2018 at 07:52:28PM +0100, Johannes Sixt wrote: > > > Do I understand correctly, that you use a leading slash as an indicator to > > > construct a path relative to system_path(). How about a "reserved" user > > > name? For example, > > > > > > [http] sslcert = ~system_path/what/ever > > > > > > although a more unique name, perhaps with some punctuation, may be > > > desirable. > > > > It's syntactically a bit further afield, but something like: > > > > [http] > > sslcert = $RUNTIME_PREFIX/what/ever > > > > would make sense to me, and is a bit less subtle than the fake user. I > > don't know if that would confuse people into thinking that we > > interpolate arbitrary environment variables, though. > > The expansion of a fake user name would have to go in expand_user_path(), a > fake variable name would have to be expanded elsewhere. Now, Dscho mentions > in the cover letter that his patch has already seen some field testing ("has > been 'in production' for a good while"). So, we have gained some confidence > that the point where the substitution happens, in expand_user_path(), is > suitable. Therefore, I have slight preference for a fake user. I don't think that necessarily needs to limit us. expand_user_path() is named that because right now it just expands "~". But there's no reason it couldn't be used for general expansion. The problem in the original patch is that it expands _even when the user has not asked us to do it_. Looking at the callers, most of them would be fine with the new expansion (the only exception is the one in daemon.c, though it manually checks for "~" already). Now I agree that the new function would probably need a better name, at which point you could easily have a function expand_path() which just wraps expand_user_path(). All that said, if we're just interested in allowing this for config, then we already have such a wrapper function: git_config_pathname(). So I don't think it's a big deal to implement it in any of these ways. It's much more important to get the syntax right, because that's user-facing and will be with us forever. -Peff
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> The cute thing is: your absolute paths would not be moved because we are >> talking about Windows. Therefore your absolute paths would not start with >> a forward slash. > > Ah, sorry, I must have misunderstood a comment in your cover letter: > > The reason is this: something like this (make paths specified e.g. via > http.sslCAInfo relative to the runtime prefix) is potentially useful > also in the non-Windows context, as long as Git was built with the > runtime prefix feature. > > ... so I thought you meant to add this code for POSIX systems as well. > > My mistake. :( Well, I do not think you should feel so bad about it, as you are not alone. It wasn't clear to me either that it was to introduce a new syntax that users would not have otherwise used before (i.e. avoids negatively affecting existing settings---because the users would have used a path that begins with a backslash if they meant "full path down from the top of the current drive") to mean a new thing (i.e. "relative to the runtime prefix") that the patch wanted to do. If that is the motivation, then it does make sense to extend this function, and possibly rename it, like this patch does, as we would want this new syntax available in anywhere we take a pathname to an untracked thing. And for such a pathname, we should be consistently using expand_user_path() *anyway* even without this new "now we know how to spell 'relative to the runtime prefix'" feature. So I agree with the patch that the issue it wants to address is worth addressing, and the function to enhance is this one. I am still unsure about the solution, though. I suspect that any build that uses runtime prefix would want access to this feature, regardless of the platform. The new syntax that is "a pathname that begins with a slash is not an absolute path but is relative to the runtime prefix" cannot avoid ambiguity with users' existing settings on platforms other than Windows, which means this feature cannot be made platform neutral in its posted form. The documentation must say "On windows, the way to spell runtime prefix relative paths is this; on macs, you must spell it differently like this." etc. Which is rather unfortunate. Perhaps we need to make an effort to find a syntax that is universally unambiguous and use it consistently across platforms? I am tempted to say "//<token>/<the remainder>" might also be such a way, even in the POSIX world, but am not brave enough to do so, as I suspect that may have a fallout in the Windows world X-<. An earlier suggestion by Duy in [*1*] to pretend as if we take $VARIABLE and define special variables might be a better direction. Are there security implications if we started allowing references to environment varibables in strings we pass expand_user_path()? If we cannot convince ourselves that it is safe to allow access to any and all environment variables, then we probably can start by specific "pseudo variables" under our control, like GIT_EXEC_PATH and GIT_INSTALL_ROOT, that do not even have to be tied to environment variables, perhaps with further restriction to allow it only at the beginning of the string, or something like that, if necessary. [References] *1* <CACsJy8DmyU_Kn4yytu_PQdpppXO8wLcuuzQ-fjwnyjA0ntB2Dw@mail.gmail.com>
Jeff King <peff@peff.net> writes: > On Wed, Nov 07, 2018 at 10:36:52PM +0100, Johannes Sixt wrote: > > All that said, if we're just interested in allowing this for config, > then we already have such a wrapper function: git_config_pathname(). > > So I don't think it's a big deal to implement it in any of these ways. > It's much more important to get the syntax right, because that's > user-facing and will be with us forever. All of us are on the same page after seeing the clarification by Dscho, it seems. I came to pretty much the same conclusion this morning before reading this subthread. Outside config values, the callers of expand_user_path() only feed "~/.git$constant", and they are all about "personal customization" that do not want to be shared with other users of the same installation, so "relative to runtime prefix" feature would not be wanted. But we do not know about new caller's needs. For now I am OK to have it in expand_user_path(), possibly renaming the function if people feel it is needed (I don't). Between ~<reserved name> and $VARIABLE_LOOKING_THINGS, I do not have a strong preference either way, but I am getting an impression that the latter is more generally favoured in the discussion?
On Thu, Nov 08, 2018 at 09:30:15AM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Wed, Nov 07, 2018 at 10:36:52PM +0100, Johannes Sixt wrote: > > > > All that said, if we're just interested in allowing this for config, > > then we already have such a wrapper function: git_config_pathname(). > > > > So I don't think it's a big deal to implement it in any of these ways. > > It's much more important to get the syntax right, because that's > > user-facing and will be with us forever. > > All of us are on the same page after seeing the clarification by > Dscho, it seems. I came to pretty much the same conclusion this > morning before reading this subthread. Outside config values, the > callers of expand_user_path() only feed "~/.git$constant", and they > are all about "personal customization" that do not want to be shared > with other users of the same installation, so "relative to runtime > prefix" feature would not be wanted. But we do not know about new > caller's needs. For now I am OK to have it in expand_user_path(), > possibly renaming the function if people feel it is needed (I don't). I think we would want to carefully think about the call in enter_repo(). We do not want git-daemon to accidentally expose repositories in $RUNTIME_PREFIX. Looking over the code, I think this is OK. The expansion happens in enter_repo(), and then we take the path that was found and do our ok_paths checks on it (which makes sense -- even now you'd ask to export "/home/" and it would need to look at "~peff/repo.git" and expand that to "/home/peff/repo.git" before doing a simple string check. > Between ~<reserved name> and $VARIABLE_LOOKING_THINGS, I do not have > a strong preference either way, but I am getting an impression that > the latter is more generally favoured in the discussion? I certainly prefer the latter, but I thought I was the only one to have expressed support so far. ;) -Peff
Jeff King <peff@peff.net> writes: > I think we would want to carefully think about the call in enter_repo(). > We do not want git-daemon to accidentally expose repositories in > $RUNTIME_PREFIX. > > Looking over the code, I think this is OK. The expansion happens in > enter_repo(), and then we take the path that was found and do our > ok_paths checks on it (which makes sense -- even now you'd ask to export > "/home/" and it would need to look at "~peff/repo.git" and expand that > to "/home/peff/repo.git" before doing a simple string check. Yup, that is another reason why I think this new "expansion feature" belongs to the function, not to a wrapper that is aware of this new feature in addition to ~tilde expansion. >> Between ~<reserved name> and $VARIABLE_LOOKING_THINGS, I do not have >> a strong preference either way, but I am getting an impression that >> the latter is more generally favoured in the discussion? > > I certainly prefer the latter, but I thought I was the only one to have > expressed support so far. ;) The first mention of pseudo-variable I saw was in Duy's message, wondering if $ROOT is more appropriate than "/", and I counted it as supporting the $VARIABLE syntax.
Hi, On Thu, 8 Nov 2018, Junio C Hamano wrote: > I am tempted to say "//<token>/<the remainder>" might also be such a > way, even in the POSIX world, but am not brave enough to do so, as I > suspect that may have a fallout in the Windows world X-<. It does. //server/share is the way we refer to UNC paths (AKA network drives). > An earlier suggestion by Duy in [*1*] to pretend as if we take > $VARIABLE and define special variables might be a better direction. My only qualm with this is that `$VARIABLE` is a perfectly allowed part of a path. That is, you *could* create a directory called `$VARIABLE` and reference that, and then the expand_user_path() function (or whatever name we will give it) would always expand this, with no way to switch it off. Granted, this is a highly unlikely scenario, but I would feel a bit more comfortable with something like <RUNTIME_PREFIX>/ssl/certs/ca-bundle.crt Of course, `<RUNTIME_PREFIX>` is *also* a perfectly valid directory name, but I would argue that it is even less likely to exist than `$RUNTIME_PREFIX` because the user would have to escape *two* characters rather than one. > Are there security implications if we started allowing references to > environment varibables in strings we pass expand_user_path()? Probably. But then, the runtime prefix is not even available as environment variable... Ciao, Dscho > If we cannot convince ourselves that it is safe to allow access to any > and all environment variables, then we probably can start by specific > "pseudo variables" under our control, like GIT_EXEC_PATH and > GIT_INSTALL_ROOT, that do not even have to be tied to environment > variables, perhaps with further restriction to allow it only at the > beginning of the string, or something like that, if necessary. > > [References] > > *1* <CACsJy8DmyU_Kn4yytu_PQdpppXO8wLcuuzQ-fjwnyjA0ntB2Dw@mail.gmail.com> >
Hi Peff, On Wed, 7 Nov 2018, Jeff King wrote: > All that said, if we're just interested in allowing this for config, > then we already have such a wrapper function: git_config_pathname(). Good point. I agree that `git_config_pathname()` is a better home for this feature than `expand_user_path()`. But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? The `~` prefix is *already* a reserved character, and while it would probably not be super intuitive to have `~~` be expanded to the runtime prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which *is* a valid directory name). Of course, `~~` is also a valid directory name, but then, so is `~` and we do not have any way to specify *that* because `expand_user_path()` will always expand it to the home directory. Or am I wrong? Do we have a way to disable the expansion? Ciao, Dscho
On Thu, Nov 8, 2018 at 2:14 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Peff, > > On Wed, 7 Nov 2018, Jeff King wrote: > > > All that said, if we're just interested in allowing this for config, > > then we already have such a wrapper function: git_config_pathname(). > > Good point. I agree that `git_config_pathname()` is a better home for this > feature than `expand_user_path()`. > > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > The `~` prefix is *already* a reserved character, and while it would > probably not be super intuitive to have `~~` be expanded to the runtime > prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which > *is* a valid directory name). One thing I had in mind when proposing $VARIABLE is that it opens up a namespace for us to expand more things (*) for example $GIT_DIR (from ~/.gitconfig). (*) but in a controlled way, it may look like an environment variable, but we only accept a selected few. And it's only expanded at the beginning of a path. > Of course, `~~` is also a valid directory name, but then, so is `~` and we > do not have any way to specify *that* because `expand_user_path()` will > always expand it to the home directory. Or am I wrong? Do we have a way to > disable the expansion? > > Ciao, > Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi, > > On Thu, 8 Nov 2018, Junio C Hamano wrote: > >> I am tempted to say "//<token>/<the remainder>" might also be such a >> way, even in the POSIX world, but am not brave enough to do so, as I >> suspect that may have a fallout in the Windows world X-<. > > It does. //server/share is the way we refer to UNC paths (AKA network > drives). Shucks. That would mean the patch that started this thread would not be a good idea, as an end-user could already be writing "//server/share/some/path" and the code with the patch would see '/' that begins it, and start treating it differently than the code before the patch X-<. > Granted, this is a highly unlikely scenario, but I would feel a bit more > comfortable with something like > > <RUNTIME_PREFIX>/ssl/certs/ca-bundle.crt > > Of course, `<RUNTIME_PREFIX>` is *also* a perfectly valid directory name, > but I would argue that it is even less likely to exist than > `$RUNTIME_PREFIX` because the user would have to escape *two* characters > rather than one. Yes, and it is naturally extensible by allowing <OTHER_THINGS> inside the special bra-ket pair (just like $OTHER_THINGS can be a way to extend the system if we used a special variable syntax). >> Are there security implications if we started allowing references to >> environment varibables in strings we pass expand_user_path()? > > Probably. But then, the runtime prefix is not even available as > environment variable... Ah, sorry. I thought it was clear that I would next be suggesting to add an environmet variable for it, _if_ the approach to allow env references turns out to be viable.
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > The `~` prefix is *already* a reserved character,... We would need to prepare for a future where we need yet another special thing to be expanded, and it will quickly become cryptic if you said "~/ is HOME, ~USER/ is USER's HOME, ~~/ is runtime prefix, and ~~~/ is this new thing...". ~runtime_prefix~/ (i.e. carve out the namespace for USERNAME by reserving any names that ends with a tilde) may be a viable way to do this, though. It is just as good as your other idea, <runtime_prefix>, in an earlier message.
Hi Junio, On Thu, 8 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > On Thu, 8 Nov 2018, Junio C Hamano wrote: > > > >> I am tempted to say "//<token>/<the remainder>" might also be such a > >> way, even in the POSIX world, but am not brave enough to do so, as I > >> suspect that may have a fallout in the Windows world X-<. > > > > It does. //server/share is the way we refer to UNC paths (AKA network > > drives). > > Shucks. That would mean the patch that started this thread would > not be a good idea, as an end-user could already be writing > "//server/share/some/path" and the code with the patch would see '/' > that begins it, and start treating it differently than the code > before the patch X-<. Ouch. You're right! > > Granted, this is a highly unlikely scenario, but I would feel a bit more > > comfortable with something like > > > > <RUNTIME_PREFIX>/ssl/certs/ca-bundle.crt > > > > Of course, `<RUNTIME_PREFIX>` is *also* a perfectly valid directory name, > > but I would argue that it is even less likely to exist than > > `$RUNTIME_PREFIX` because the user would have to escape *two* characters > > rather than one. > > Yes, and it is naturally extensible by allowing <OTHER_THINGS> > inside the special bra-ket pair (just like $OTHER_THINGS can be a > way to extend the system if we used a special variable syntax). True. > >> Are there security implications if we started allowing references to > >> environment varibables in strings we pass expand_user_path()? > > > > Probably. But then, the runtime prefix is not even available as > > environment variable... > > Ah, sorry. I thought it was clear that I would next be suggesting to > add an environmet variable for it, _if_ the approach to allow env > references turns out to be viable. Of course, I should have assumed that. Sorry! Ciao, Dscho
Hi Duy, On Thu, 8 Nov 2018, Duy Nguyen wrote: > On Thu, Nov 8, 2018 at 2:14 PM Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: > > > > On Wed, 7 Nov 2018, Jeff King wrote: > > > > > All that said, if we're just interested in allowing this for config, > > > then we already have such a wrapper function: git_config_pathname(). > > > > Good point. I agree that `git_config_pathname()` is a better home for this > > feature than `expand_user_path()`. > > > > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > > The `~` prefix is *already* a reserved character, and while it would > > probably not be super intuitive to have `~~` be expanded to the runtime > > prefix, at least it would be less ambiguous than `$RUNTIME_PREFIX` (which > > *is* a valid directory name). > > One thing I had in mind when proposing $VARIABLE is that it opens up a > namespace for us to expand more things (*) for example $GIT_DIR (from > ~/.gitconfig). > > (*) but in a controlled way, it may look like an environment variable, > but we only accept a selected few. And it's only expanded at the > beginning of a path. I understand that desire, and I even agree. But the fact that it looks like an environment variable, but is not, is actually a point in favor of *not* doing this. It would violate the principle of least astonishment. The form `<RUNTIME_PREFIX>/abc/def` would not be confused with anything that it is not, I would think. The only thing against this form (at least that I can think of) is that some people use this way to talk about paths that vary between different setups, with the implicit assumption that the reader will "interpolate" this *before* applying. So for example, when I tell a user to please edit their <GIT_DIR>/config, I implicitly assume that they know to not type out, literally, <GIT_DIR> but instead fill in the correct path. Ciao, Dscho > > Of course, `~~` is also a valid directory name, but then, so is `~` and we > > do not have any way to specify *that* because `expand_user_path()` will > > always expand it to the home directory. Or am I wrong? Do we have a way to > > disable the expansion? > > > > Ciao, > > Dscho > > > > -- > Duy >
Hi Junio, On Thu, 8 Nov 2018, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > But now I have a really crazy idea: how about ~~/ssl/certs/ca-bundle.crt? > > The `~` prefix is *already* a reserved character,... > > We would need to prepare for a future where we need yet another > special thing to be expanded, and it will quickly become cryptic if > you said "~/ is HOME, ~USER/ is USER's HOME, ~~/ is runtime prefix, > and ~~~/ is this new thing...". ~runtime_prefix~/ (i.e. carve out > the namespace for USERNAME by reserving any names that ends with a > tilde) may be a viable way to do this, though. It is just as good > as your other idea, <runtime_prefix>, in an earlier message. Indeed. Your "cryptic" matches my "unintuitive". Ciao, Dscho
On Thu, Nov 8, 2018 at 10:45 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > On Thu, 8 Nov 2018, Duy Nguyen wrote: > > One thing I had in mind when proposing $VARIABLE is that it opens up a > > namespace for us to expand more things (*) for example $GIT_DIR (from > > ~/.gitconfig). > > > > (*) but in a controlled way, it may look like an environment variable, > > but we only accept a selected few. And it's only expanded at the > > beginning of a path. > > I understand that desire, and I even agree. But the fact that it looks > like an environment variable, but is not, is actually a point in favor of > *not* doing this. It would violate the principle of least astonishment. Perhaps something like $:VARIABLE, which gives the convenience of $VARIABLE, but looks sufficiently different that it shouldn't trip up readers into thinking it is one. (Well-written code checking for a DOS/Windows drive letter at the beginning of a path shouldn't be confused by it.)
Can someone please tell me how to unsubscribe from this email. I am no longer interested in receiving these emails, and cannot find how to unsubscribe. Thank you in advance. Joseph Moisan | Software Engineer Building Technologies and Solutions Tel: +1-978-731-8950 Joseph.Moisan@JCI.com -----Original Message----- From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On Behalf Of Junio C Hamano Sent: Wednesday, November 7, 2018 7:17 PM To: Ramsay Jones <ramsay@ramsayjones.plus.com> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>; git@vger.kernel.org Subject: Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> The cute thing is: your absolute paths would not be moved because we >> are talking about Windows. Therefore your absolute paths would not >> start with a forward slash. > > Ah, sorry, I must have misunderstood a comment in your cover letter: > > The reason is this: something like this (make paths specified e.g. via > http.sslCAInfo relative to the runtime prefix) is potentially useful > also in the non-Windows context, as long as Git was built with the > runtime prefix feature. > > ... so I thought you meant to add this code for POSIX systems as well. > > My mistake. :( Well, I do not think you should feel so bad about it, as you are not alone. It wasn't clear to me either that it was to introduce a new syntax that users would not have otherwise used before (i.e. avoids negatively affecting existing settings---because the users would have used a path that begins with a backslash if they meant "full path down from the top of the current drive") to mean a new thing (i.e. "relative to the runtime prefix") that the patch wanted to do. If that is the motivation, then it does make sense to extend this function, and possibly rename it, like this patch does, as we would want this new syntax available in anywhere we take a pathname to an untracked thing. And for such a pathname, we should be consistently using expand_user_path() *anyway* even without this new "now we know how to spell 'relative to the runtime prefix'" feature. So I agree with the patch that the issue it wants to address is worth addressing, and the function to enhance is this one. I am still unsure about the solution, though. I suspect that any build that uses runtime prefix would want access to this feature, regardless of the platform. The new syntax that is "a pathname that begins with a slash is not an absolute path but is relative to the runtime prefix" cannot avoid ambiguity with users' existing settings on platforms other than Windows, which means this feature cannot be made platform neutral in its posted form. The documentation must say "On windows, the way to spell runtime prefix relative paths is this; on macs, you must spell it differently like this." etc. Which is rather unfortunate. Perhaps we need to make an effort to find a syntax that is universally unambiguous and use it consistently across platforms? I am tempted to say "//<token>/<the remainder>" might also be such a way, even in the POSIX world, but am not brave enough to do so, as I suspect that may have a fallout in the Windows world X-<. An earlier suggestion by Duy in [*1*] to pretend as if we take $VARIABLE and define special variables might be a better direction. Are there security implications if we started allowing references to environment varibables in strings we pass expand_user_path()? If we cannot convince ourselves that it is safe to allow access to any and all environment variables, then we probably can start by specific "pseudo variables" under our control, like GIT_EXEC_PATH and GIT_INSTALL_ROOT, that do not even have to be tied to environment variables, perhaps with further restriction to allow it only at the beginning of the string, or something like that, if necessary. [References] *1* <CACsJy8DmyU_Kn4yytu_PQdpppXO8wLcuuzQ-fjwnyjA0ntB2Dw@mail.gmail.com>
On Thu, Nov 08, 2018 at 04:45:16PM +0100, Johannes Schindelin wrote: > > One thing I had in mind when proposing $VARIABLE is that it opens up a > > namespace for us to expand more things (*) for example $GIT_DIR (from > > ~/.gitconfig). > > > > (*) but in a controlled way, it may look like an environment variable, > > but we only accept a selected few. And it's only expanded at the > > beginning of a path. > > I understand that desire, and I even agree. But the fact that it looks > like an environment variable, but is not, is actually a point in favor of > *not* doing this. It would violate the principle of least astonishment. I agree that it is potentially surprising. OTOH, it is at least pretty obvious when you see in the wild something like: [core] foo = $RUNTIME_PREFIX/bar what it is _trying_ to do. My big concern with "~"-based things is that they're kind of subtle. If I saw: [core] foo = ~~/bar I'm not sure what I would think it does. ;) > The form `<RUNTIME_PREFIX>/abc/def` would not be confused with anything > that it is not, I would think. The only thing against this form (at least > that I can think of) is that some people use this way to talk about paths > that vary between different setups, with the implicit assumption that the > reader will "interpolate" this *before* applying. So for example, when I > tell a user to please edit their <GIT_DIR>/config, I implicitly assume > that they know to not type out, literally, <GIT_DIR> but instead fill in > the correct path. So yeah, some alternative syntax that is verbose but distinct makes sense to me. We use %-substitution elsewhere. Could we do something with that? "%RUNTIME_PREFIX%" gives me too many flashbacks, but something like "%(RUNTIME_PREFIX)" matches our formatting language. I dunno. I actually do not think "$FOO" is so bad, as long as we clearly document that: - we do not allow arbitrary $FOO from the environment (though I am actually not all that opposed to doing so) - we add some special $FOOs that are not actually environment variables -Peff
On Fri, Nov 09, 2018 at 02:05:48AM +0000, Joseph Moisan wrote: > Can someone please tell me how to unsubscribe from this email. I am > no longer interested in receiving these emails, and cannot find how to > unsubscribe. Details are at http://vger.kernel.org/vger-lists.html#git. -Peff
On Fri, Nov 9, 2018 at 11:19 AM Jeff King <peff@peff.net> wrote: > > The form `<RUNTIME_PREFIX>/abc/def` would not be confused with anything > > that it is not, I would think. The only thing against this form (at least > > that I can think of) is that some people use this way to talk about paths > > that vary between different setups, with the implicit assumption that the > > reader will "interpolate" this *before* applying. So for example, when I > > tell a user to please edit their <GIT_DIR>/config, I implicitly assume > > that they know to not type out, literally, <GIT_DIR> but instead fill in > > the correct path. > > So yeah, some alternative syntax that is verbose but distinct makes > sense to me. We use %-substitution elsewhere. Could we do something with > that? "%RUNTIME_PREFIX%" gives me too many flashbacks, but something > like "%(RUNTIME_PREFIX)" matches our formatting language. FWIW I don't have any preference, as long as the variable can still have a name (that is not a symbol). A side question regardless of syntax. What do we do with %(unrecognized name)/foo? I see three options - expand to empty, so "/foo" - keep it and try the literal path "%(unrecognized name)/foo" - somehow tell the caller that the path is invalid and treat it like non-existing path, even if there is some real thing at "%(unrecognized name)/foo" The last option is more predictable. But we need to be more careful about the syntax because if "%(some path like this)" actually exists somewhere, then it will be broken. And I think it's also more work.
Duy Nguyen <pclouds@gmail.com> writes: > FWIW I don't have any preference, as long as the variable can still > have a name (that is not a symbol). Same here. > A side question regardless of syntax. What do we do with > %(unrecognized name)/foo? I see three options > > - expand to empty, so "/foo" > - keep it and try the literal path "%(unrecognized name)/foo" Neither of these is good for future proofing purposes. > - somehow tell the caller that the path is invalid and treat it like > non-existing path, even if there is some real thing at "%(unrecognized > name)/foo" Another thing to worry about is how to spell the real thing that has such a funny name, perhaps by escaping like "%%(unrecognized name)/foo". And from that point of view, "~runtime-prefix~/foo" (i.e. what J6t started) may make the most sense, as I do not think we need to support a username that ends with a tilde by introducing a quoting convention.
diff --git a/path.c b/path.c index 34f0f98349..a72abf0e1f 100644 --- a/path.c +++ b/path.c @@ -11,6 +11,7 @@ #include "path.h" #include "packfile.h" #include "object-store.h" +#include "exec-cmd.h" static int get_st_mode_bits(const char *path, int *mode) { @@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home) if (path == NULL) goto return_null; +#ifdef __MINGW32__ + if (path[0] == '/') + return system_path(path + 1); +#endif if (path[0] == '~') { const char *first_slash = strchrnul(path, '/'); const char *username = path + 1;