* [PATCH 0/1] mingw: handle absolute paths in expand_user_path() @ 2018-11-06 14:53 Johannes Schindelin via GitGitGadget 2018-11-06 14:53 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget 2021-07-01 16:03 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget 0 siblings, 2 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2018-11-06 14:53 UTC (permalink / raw) To: git; +Cc: Junio C Hamano While this patch has been "in production" in Git for Windows for a good while, this patch series is half meant as a request for comments. 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. The main problem with non-Windows platforms is that paths starting with a single slash are unambiguously absolute, whereas we can say with certainty that they are not absolute Windows paths. So maybe someone on this list has a clever idea how we could specify paths (unambiguously, even on non-Windows platforms) that Git should interpret as relative to the runtime prefix? Johannes Schindelin (1): mingw: handle absolute paths in expand_user_path() path.c | 5 +++++ 1 file changed, 5 insertions(+) base-commit: cd69ec8cde54af1817630331fc441f493866f0d4 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-66%2Fdscho%2Fmingw-expand-absolute-user-path-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/66 -- gitgitgadget ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 14:53 [PATCH 0/1] mingw: handle absolute paths in expand_user_path() Johannes Schindelin via GitGitGadget @ 2018-11-06 14:53 ` Johannes Schindelin via GitGitGadget 2018-11-06 15:54 ` Ramsay Jones ` (2 more replies) 2021-07-01 16:03 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget 1 sibling, 3 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2018-11-06 14:53 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Johannes Schindelin 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 if (path[0] == '~') { const char *first_slash = strchrnul(path, '/'); const char *username = path + 1; -- gitgitgadget ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 14:53 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget @ 2018-11-06 15:54 ` Ramsay Jones 2018-11-06 16:10 ` Ramsay Jones 2018-11-07 1:21 ` Junio C Hamano 2018-11-06 18:24 ` Duy Nguyen 2018-11-06 21:32 ` Johannes Sixt 2 siblings, 2 replies; 53+ messages in thread From: Ramsay Jones @ 2018-11-06 15:54 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget, git Cc: Junio C Hamano, Johannes Schindelin 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; > ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 15:54 ` Ramsay Jones @ 2018-11-06 16:10 ` Ramsay Jones 2018-11-06 18:27 ` Duy Nguyen 2018-11-07 1:21 ` Junio C Hamano 1 sibling, 1 reply; 53+ messages in thread From: Ramsay Jones @ 2018-11-06 16:10 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget, git Cc: Junio C Hamano, Johannes Schindelin 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 16:10 ` Ramsay Jones @ 2018-11-06 18:27 ` Duy Nguyen 0 siblings, 0 replies; 53+ messages in thread From: Duy Nguyen @ 2018-11-06 18:27 UTC (permalink / raw) To: Ramsay Jones Cc: gitgitgadget, Git Mailing List, Junio C Hamano, Johannes Schindelin 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 -- Duy ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 15:54 ` Ramsay Jones 2018-11-06 16:10 ` Ramsay Jones @ 2018-11-07 1:21 ` Junio C Hamano 2018-11-07 11:19 ` Johannes Schindelin 1 sibling, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-11-07 1:21 UTC (permalink / raw) To: Ramsay Jones Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 1:21 ` Junio C Hamano @ 2018-11-07 11:19 ` Johannes Schindelin 2018-11-07 17:42 ` Ramsay Jones 0 siblings, 1 reply; 53+ messages in thread From: Johannes Schindelin @ 2018-11-07 11:19 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ramsay Jones, Johannes Schindelin via GitGitGadget, git 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 11:19 ` Johannes Schindelin @ 2018-11-07 17:42 ` Ramsay Jones 2018-11-08 0:16 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Ramsay Jones @ 2018-11-07 17:42 UTC (permalink / raw) To: Johannes Schindelin, Junio C Hamano Cc: Johannes Schindelin via GitGitGadget, git 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 17:42 ` Ramsay Jones @ 2018-11-08 0:16 ` Junio C Hamano 2018-11-08 13:04 ` Johannes Schindelin 2018-11-09 2:05 ` Joseph Moisan 0 siblings, 2 replies; 53+ messages in thread From: Junio C Hamano @ 2018-11-08 0:16 UTC (permalink / raw) To: Ramsay Jones Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git 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> ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 0:16 ` Junio C Hamano @ 2018-11-08 13:04 ` Johannes Schindelin 2018-11-08 14:43 ` Junio C Hamano 2018-11-09 2:05 ` Joseph Moisan 1 sibling, 1 reply; 53+ messages in thread From: Johannes Schindelin @ 2018-11-08 13:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ramsay Jones, Johannes Schindelin via GitGitGadget, git 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> > ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 13:04 ` Johannes Schindelin @ 2018-11-08 14:43 ` Junio C Hamano 2018-11-08 15:39 ` Johannes Schindelin 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-11-08 14:43 UTC (permalink / raw) To: Johannes Schindelin Cc: Ramsay Jones, Johannes Schindelin via GitGitGadget, git 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 14:43 ` Junio C Hamano @ 2018-11-08 15:39 ` Johannes Schindelin 0 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin @ 2018-11-08 15:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ramsay Jones, Johannes Schindelin via GitGitGadget, git 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* RE: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 0:16 ` Junio C Hamano 2018-11-08 13:04 ` Johannes Schindelin @ 2018-11-09 2:05 ` Joseph Moisan 2018-11-09 10:21 ` Jeff King 1 sibling, 1 reply; 53+ messages in thread From: Joseph Moisan @ 2018-11-09 2:05 UTC (permalink / raw) To: git 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> ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-09 2:05 ` Joseph Moisan @ 2018-11-09 10:21 ` Jeff King 0 siblings, 0 replies; 53+ messages in thread From: Jeff King @ 2018-11-09 10:21 UTC (permalink / raw) To: Joseph Moisan; +Cc: git 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 14:53 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget 2018-11-06 15:54 ` Ramsay Jones @ 2018-11-06 18:24 ` Duy Nguyen 2018-11-07 11:19 ` Johannes Schindelin 2018-11-06 21:32 ` Johannes Sixt 2 siblings, 1 reply; 53+ messages in thread From: Duy Nguyen @ 2018-11-06 18:24 UTC (permalink / raw) To: gitgitgadget; +Cc: Git Mailing List, Junio C Hamano, Johannes Schindelin 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 -- Duy ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 18:24 ` Duy Nguyen @ 2018-11-07 11:19 ` Johannes Schindelin 0 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin @ 2018-11-07 11:19 UTC (permalink / raw) To: Duy Nguyen; +Cc: gitgitgadget, Git Mailing List, Junio C Hamano 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 > ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 14:53 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget 2018-11-06 15:54 ` Ramsay Jones 2018-11-06 18:24 ` Duy Nguyen @ 2018-11-06 21:32 ` Johannes Sixt 2018-11-07 11:23 ` Johannes Schindelin 2 siblings, 1 reply; 53+ messages in thread From: Johannes Sixt @ 2018-11-06 21:32 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget, Johannes Schindelin Cc: git, Junio C Hamano 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; > ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-06 21:32 ` Johannes Sixt @ 2018-11-07 11:23 ` Johannes Schindelin 2018-11-07 18:52 ` Johannes Sixt 0 siblings, 1 reply; 53+ messages in thread From: Johannes Schindelin @ 2018-11-07 11:23 UTC (permalink / raw) To: Johannes Sixt; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano 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; > > > > > ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 11:23 ` Johannes Schindelin @ 2018-11-07 18:52 ` Johannes Sixt 2018-11-07 20:41 ` Jeff King 0 siblings, 1 reply; 53+ messages in thread From: Johannes Sixt @ 2018-11-07 18:52 UTC (permalink / raw) To: Johannes Schindelin Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 18:52 ` Johannes Sixt @ 2018-11-07 20:41 ` Jeff King 2018-11-07 21:36 ` Johannes Sixt 0 siblings, 1 reply; 53+ messages in thread From: Jeff King @ 2018-11-07 20:41 UTC (permalink / raw) To: Johannes Sixt Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 20:41 ` Jeff King @ 2018-11-07 21:36 ` Johannes Sixt 2018-11-07 22:03 ` Jeff King 0 siblings, 1 reply; 53+ messages in thread From: Johannes Sixt @ 2018-11-07 21:36 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 21:36 ` Johannes Sixt @ 2018-11-07 22:03 ` Jeff King 2018-11-08 0:30 ` Junio C Hamano 2018-11-08 13:11 ` Johannes Schindelin 0 siblings, 2 replies; 53+ messages in thread From: Jeff King @ 2018-11-07 22:03 UTC (permalink / raw) To: Johannes Sixt Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 22:03 ` Jeff King @ 2018-11-08 0:30 ` Junio C Hamano 2018-11-08 1:18 ` Jeff King 2018-11-08 13:11 ` Johannes Schindelin 1 sibling, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-11-08 0:30 UTC (permalink / raw) To: Jeff King Cc: Johannes Sixt, Johannes Schindelin, Johannes Schindelin via GitGitGadget, git 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? ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 0:30 ` Junio C Hamano @ 2018-11-08 1:18 ` Jeff King 2018-11-08 3:26 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Jeff King @ 2018-11-08 1:18 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Sixt, Johannes Schindelin, Johannes Schindelin via GitGitGadget, git 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 1:18 ` Jeff King @ 2018-11-08 3:26 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-11-08 3:26 UTC (permalink / raw) To: Jeff King Cc: Johannes Sixt, Johannes Schindelin, Johannes Schindelin via GitGitGadget, git 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-07 22:03 ` Jeff King 2018-11-08 0:30 ` Junio C Hamano @ 2018-11-08 13:11 ` Johannes Schindelin 2018-11-08 14:25 ` Duy Nguyen 2018-11-08 14:47 ` Junio C Hamano 1 sibling, 2 replies; 53+ messages in thread From: Johannes Schindelin @ 2018-11-08 13:11 UTC (permalink / raw) To: Jeff King Cc: Johannes Sixt, Johannes Schindelin via GitGitGadget, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 13:11 ` Johannes Schindelin @ 2018-11-08 14:25 ` Duy Nguyen 2018-11-08 15:45 ` Johannes Schindelin 2018-11-08 14:47 ` Junio C Hamano 1 sibling, 1 reply; 53+ messages in thread From: Duy Nguyen @ 2018-11-08 14:25 UTC (permalink / raw) To: Johannes Schindelin Cc: Jeff King, Johannes Sixt, gitgitgadget, Git Mailing List, Junio C Hamano 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 -- Duy ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 14:25 ` Duy Nguyen @ 2018-11-08 15:45 ` Johannes Schindelin 2018-11-08 17:40 ` Eric Sunshine 2018-11-09 10:19 ` Jeff King 0 siblings, 2 replies; 53+ messages in thread From: Johannes Schindelin @ 2018-11-08 15:45 UTC (permalink / raw) To: Duy Nguyen Cc: Jeff King, Johannes Sixt, gitgitgadget, Git Mailing List, Junio C Hamano 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 > ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 15:45 ` Johannes Schindelin @ 2018-11-08 17:40 ` Eric Sunshine 2018-11-09 10:19 ` Jeff King 1 sibling, 0 replies; 53+ messages in thread From: Eric Sunshine @ 2018-11-08 17:40 UTC (permalink / raw) To: Johannes Schindelin Cc: Nguyễn Thái Ngọc Duy, Jeff King, Johannes Sixt, gitgitgadget, Git List, Junio C Hamano 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.) ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 15:45 ` Johannes Schindelin 2018-11-08 17:40 ` Eric Sunshine @ 2018-11-09 10:19 ` Jeff King 2018-11-09 16:16 ` Duy Nguyen 1 sibling, 1 reply; 53+ messages in thread From: Jeff King @ 2018-11-09 10:19 UTC (permalink / raw) To: Johannes Schindelin Cc: Duy Nguyen, Johannes Sixt, gitgitgadget, Git Mailing List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-09 10:19 ` Jeff King @ 2018-11-09 16:16 ` Duy Nguyen 2018-11-12 3:03 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Duy Nguyen @ 2018-11-09 16:16 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin, Johannes Sixt, gitgitgadget, Git Mailing List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-09 16:16 ` Duy Nguyen @ 2018-11-12 3:03 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-11-12 3:03 UTC (permalink / raw) To: Duy Nguyen Cc: Jeff King, Johannes Schindelin, Johannes Sixt, gitgitgadget, Git Mailing List 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 13:11 ` Johannes Schindelin 2018-11-08 14:25 ` Duy Nguyen @ 2018-11-08 14:47 ` Junio C Hamano 2018-11-08 15:46 ` Johannes Schindelin 1 sibling, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-11-08 14:47 UTC (permalink / raw) To: Johannes Schindelin Cc: Jeff King, Johannes Sixt, Johannes Schindelin via GitGitGadget, git 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path() 2018-11-08 14:47 ` Junio C Hamano @ 2018-11-08 15:46 ` Johannes Schindelin 0 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin @ 2018-11-08 15:46 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Johannes Sixt, Johannes Schindelin via GitGitGadget, git 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 0/2] mingw: handle absolute paths in expand_user_path() 2018-11-06 14:53 [PATCH 0/1] mingw: handle absolute paths in expand_user_path() Johannes Schindelin via GitGitGadget 2018-11-06 14:53 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget @ 2021-07-01 16:03 ` Johannes Schindelin via GitGitGadget 2021-07-01 16:03 ` [PATCH v2 1/2] tests: exercise the RUNTIME_PREFIX feature Johannes Schindelin via GitGitGadget ` (3 more replies) 1 sibling, 4 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-01 16:03 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin In Git for Windows, we ran with a patch "in production" for quite a while where paths starting with a slash (i.e. looking like Unix paths, not like Windows paths) were interpreted as being relative to the runtime prefix, when expanded via expand_user_path(). This was sent to the Git mailing list as a discussion starter, and it was pointed out that this is neither portable nor unambiguous. After the dust settled, I thought about the presented ideas for a while (quite a while...), and ended up with the following: any path starting with <RUNTIME-PREFIX>/ is expanded. This is ambiguous because it could be a valid path. But then, it is unlikely, and if someone really wants to specify such a path, it is easy to slap a ./ in front and they're done. Changes since v1: * Included a test for the RUNTIME_PREFIX that I had meant to send for ages already, and based on which... * A test case was added to verify that this actually works as intended * It is no longer Windows-specific * I added some documentation Johannes Schindelin (2): tests: exercise the RUNTIME_PREFIX feature expand_user_path(): support specifying paths relative to the runtime prefix Documentation/config.txt | 10 ++++++++++ Makefile | 5 +++++ path.c | 5 +++++ t/t0060-path-utils.sh | 26 ++++++++++++++++++++++++++ 4 files changed, 46 insertions(+) base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-66%2Fdscho%2Fmingw-expand-absolute-user-path-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/66 Range-diff vs v1: -: ----------- > 1: cc8f09baba9 tests: exercise the RUNTIME_PREFIX feature 1: 2287dd96cf0 ! 2: 66df56f5db0 mingw: handle absolute paths in expand_user_path() @@ Metadata Author: Johannes Schindelin <Johannes.Schindelin@gmx.de> ## Commit message ## - mingw: handle absolute paths in expand_user_path() + expand_user_path(): support specifying paths relative to the runtime prefix - On Windows, an absolute POSIX path needs to be turned into a Windows - one. + Ever since Git learned to detect its install location at runtime, there + was the slightly awkward problem that it was impossible to specify paths + relative to said location. + + For example, if a version of Git was shipped with custom SSL + certificates to use, there was no portable way to specify + `http.sslCAInfo`. + + In Git for Windows, the problem was "solved" for years by interpreting + paths starting with a slash as relative to the runtime prefix. + + However, this is not correct: such paths _are_ legal on Windows, and + they are interpreted as absolute paths in the same drive as the current + directory. + + After a lengthy discussion, and a way lengthier time to mull over the + problem and its best solution, we decided to introduce support for the + magic sequence `<RUNTIME-PREFIX>/`. If a path starts with this, the + remainder is interpreted as relative to the detected runtime prefix. + + This solves the problem, but what new problems does it stir up? Here are + the two most obvious ones: + + - What if Git was not compiled with support for a runtime prefix? + + In that case, we will simply use the compiled-in hard-coded prefix. + + - What if a user _wants_ to specify a path starting with the magic + sequence? + + In that case, the user will simply need to prefix the magic sequence + with `./` and voilà, the path won't be expanded. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> + ## Documentation/config.txt ## +@@ Documentation/config.txt: pathname:: + tilde expansion happens to such a string: `~/` + is expanded to the value of `$HOME`, and `~user/` to the + specified user's home directory. +++ ++If a path starts with `<RUNTIME-PREFIX>/`, the remainder is ++interpreted as a path relative to Git's "runtime prefix", i.e. relative ++to the location where Git itself was installed. For example, ++`<RUNTIME-PREFIX>/bin/` refers to the directory in which the Git ++executable itself lives. If Git was compiled without runtime prefix ++support, the compiled-in prefix will be subsituted instead. In the ++unlikely event that a literal path needs to be specified that should ++_not_ be expanded, it needs to be prefixed by `./`, like so: ++`./<RUNTIME-PREFIX>/bin`. + + + Variables + ## path.c ## @@ - #include "path.h" #include "packfile.h" #include "object-store.h" + #include "lockfile.h" +#include "exec-cmd.h" static int get_st_mode_bits(const char *path, int *mode) @@ path.c: 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 (skip_prefix(path, "<RUNTIME-PREFIX>/", &path)) ++ return system_path(path); ++ if (path[0] == '~') { const char *first_slash = strchrnul(path, '/'); const char *username = path + 1; + + ## t/t0060-path-utils.sh ## +@@ t/t0060-path-utils.sh: test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' ' + cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && + GIT_EXEC_PATH= ./pretend/bin/git here >actual && + echo HERE >expect && ++ test_cmp expect actual' ++ ++test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '<RUNTIME-PREFIX>/ works' ' ++ mkdir -p pretend/bin && ++ cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && ++ git config yes.path "<RUNTIME-PREFIX>/yes" && ++ GIT_EXEC_PATH= ./pretend/bin/git config --path yes.path >actual && ++ echo "$(pwd)/pretend/yes" >expect && + test_cmp expect actual + ' + -- gitgitgadget ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 1/2] tests: exercise the RUNTIME_PREFIX feature 2021-07-01 16:03 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget @ 2021-07-01 16:03 ` Johannes Schindelin via GitGitGadget 2021-07-01 16:03 ` [PATCH v2 2/2] expand_user_path(): support specifying paths relative to the runtime prefix Johannes Schindelin via GitGitGadget ` (2 subsequent siblings) 3 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-01 16:03 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> Originally, we refrained from adding a regression test in 7b6c6496374 (system_path(): Add prefix computation at runtime if RUNTIME_PREFIX set, 2008-08-10), and in 226c0ddd0d6 (exec_cmd: RUNTIME_PREFIX on some POSIX systems, 2018-04-10). The reason was that it was deemed too tricky to test. Turns out that it is not tricky to test at all: we simply create a pseudo-root, copy the `git` executable into the `git/` subdirectory of that pseudo-root, then copy a script into the `libexec/git-core/` directory and expect that to be picked up. As long as the trash directory is in a location where binaries can be executed, this works. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Makefile | 5 +++++ t/t0060-path-utils.sh | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/Makefile b/Makefile index c3565fc0f8f..4c3e4ccabcd 100644 --- a/Makefile +++ b/Makefile @@ -2826,6 +2826,11 @@ ifdef GIT_TEST_INDEX_VERSION endif ifdef GIT_TEST_PERL_FATAL_WARNINGS @echo GIT_TEST_PERL_FATAL_WARNINGS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_PERL_FATAL_WARNINGS)))'\' >>$@+ +endif +ifdef RUNTIME_PREFIX + @echo RUNTIME_PREFIX=\'true\' >>$@+ +else + @echo RUNTIME_PREFIX=\'false\' >>$@+ endif @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index de4960783f0..a76728c27bf 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -525,4 +525,22 @@ test_expect_success MINGW 'is_valid_path() on Windows' ' "PRN./abc" ' +test_lazy_prereq RUNTIME_PREFIX ' + test true = "$RUNTIME_PREFIX" +' + +test_lazy_prereq CAN_EXEC_IN_PWD ' + cp "$GIT_EXEC_PATH"/git$X ./ && + ./git rev-parse +' + +test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' ' + mkdir -p pretend/bin pretend/libexec/git-core && + echo "echo HERE" | write_script pretend/libexec/git-core/git-here && + cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && + GIT_EXEC_PATH= ./pretend/bin/git here >actual && + echo HERE >expect && + test_cmp expect actual +' + test_done -- gitgitgadget ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v2 2/2] expand_user_path(): support specifying paths relative to the runtime prefix 2021-07-01 16:03 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget 2021-07-01 16:03 ` [PATCH v2 1/2] tests: exercise the RUNTIME_PREFIX feature Johannes Schindelin via GitGitGadget @ 2021-07-01 16:03 ` Johannes Schindelin via GitGitGadget 2021-07-01 17:42 ` [PATCH v2 0/2] mingw: handle absolute paths in expand_user_path() Junio C Hamano 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget 3 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-01 16:03 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> Ever since Git learned to detect its install location at runtime, there was the slightly awkward problem that it was impossible to specify paths relative to said location. For example, if a version of Git was shipped with custom SSL certificates to use, there was no portable way to specify `http.sslCAInfo`. In Git for Windows, the problem was "solved" for years by interpreting paths starting with a slash as relative to the runtime prefix. However, this is not correct: such paths _are_ legal on Windows, and they are interpreted as absolute paths in the same drive as the current directory. After a lengthy discussion, and a way lengthier time to mull over the problem and its best solution, we decided to introduce support for the magic sequence `<RUNTIME-PREFIX>/`. If a path starts with this, the remainder is interpreted as relative to the detected runtime prefix. This solves the problem, but what new problems does it stir up? Here are the two most obvious ones: - What if Git was not compiled with support for a runtime prefix? In that case, we will simply use the compiled-in hard-coded prefix. - What if a user _wants_ to specify a path starting with the magic sequence? In that case, the user will simply need to prefix the magic sequence with `./` and voilà, the path won't be expanded. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Documentation/config.txt | 10 ++++++++++ path.c | 5 +++++ t/t0060-path-utils.sh | 8 ++++++++ 3 files changed, 23 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index bf82766a6a2..fd56e2c1220 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -298,6 +298,16 @@ pathname:: tilde expansion happens to such a string: `~/` is expanded to the value of `$HOME`, and `~user/` to the specified user's home directory. ++ +If a path starts with `<RUNTIME-PREFIX>/`, the remainder is +interpreted as a path relative to Git's "runtime prefix", i.e. relative +to the location where Git itself was installed. For example, +`<RUNTIME-PREFIX>/bin/` refers to the directory in which the Git +executable itself lives. If Git was compiled without runtime prefix +support, the compiled-in prefix will be subsituted instead. In the +unlikely event that a literal path needs to be specified that should +_not_ be expanded, it needs to be prefixed by `./`, like so: +`./<RUNTIME-PREFIX>/bin`. Variables diff --git a/path.c b/path.c index 7bccd830e95..d8542a7b27b 100644 --- a/path.c +++ b/path.c @@ -12,6 +12,7 @@ #include "packfile.h" #include "object-store.h" #include "lockfile.h" +#include "exec-cmd.h" static int get_st_mode_bits(const char *path, int *mode) { @@ -732,6 +733,10 @@ char *expand_user_path(const char *path, int real_home) if (path == NULL) goto return_null; + + if (skip_prefix(path, "<RUNTIME-PREFIX>/", &path)) + return system_path(path); + if (path[0] == '~') { const char *first_slash = strchrnul(path, '/'); const char *username = path + 1; diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index a76728c27bf..cb7fbfb9af2 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -540,6 +540,14 @@ test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' ' cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && GIT_EXEC_PATH= ./pretend/bin/git here >actual && echo HERE >expect && + test_cmp expect actual' + +test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '<RUNTIME-PREFIX>/ works' ' + mkdir -p pretend/bin && + cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && + git config yes.path "<RUNTIME-PREFIX>/yes" && + GIT_EXEC_PATH= ./pretend/bin/git config --path yes.path >actual && + echo "$(pwd)/pretend/yes" >expect && test_cmp expect actual ' -- gitgitgadget ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 0/2] mingw: handle absolute paths in expand_user_path() 2021-07-01 16:03 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget 2021-07-01 16:03 ` [PATCH v2 1/2] tests: exercise the RUNTIME_PREFIX feature Johannes Schindelin via GitGitGadget 2021-07-01 16:03 ` [PATCH v2 2/2] expand_user_path(): support specifying paths relative to the runtime prefix Johannes Schindelin via GitGitGadget @ 2021-07-01 17:42 ` Junio C Hamano 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget 3 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2021-07-01 17:42 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Ramsay Jones, Duy Nguyen, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > In Git for Windows, we ran with a patch "in production" for quite a while > where paths starting with a slash (i.e. looking like Unix paths, not like > Windows paths) were interpreted as being relative to the runtime prefix, > when expanded via expand_user_path(). > > This was sent to the Git mailing list as a discussion starter, and it was > pointed out that this is neither portable nor unambiguous. > > After the dust settled, I thought about the presented ideas for a while > (quite a while...), and ended up with the following: any path starting with > <RUNTIME-PREFIX>/ is expanded. This is ambiguous because it could be a valid > path. But then, it is unlikely, and if someone really wants to specify such > a path, it is easy to slap a ./ in front and they're done. I just went back to briefly scan the discussion in late 2018. I think the rough consensus back then was that * It indeed is a problem that there is no syntax for users of "relocatable Git" to use to point at things that come as part of the "relocatable Git" package. * A change to expand_user_path() would be too broad, it makes sense for this feature to be in git_config_pathname(); * We need to get the syntax right. As to the last item, there were a handful of choices raised: - Use "~~"--the downside is that this is not extensible. Use "~runtime-prefix/" would be a better choice (the likelyhood of <RUNTIME-PREFIX>, $RUNTIME_PREFIX, and any other random choice happens to be used as a valid directory name is just as slim as the likelyhood of "runtime-prefix" used as a valid username). - "$RUNTIME_PREFIX" to make it read like a variable---the downside was that it looked TOO MUCH like a variable and risked user confusion (e.g. it may be upsetting that "$HOME/.gitconfig" is not expanded like "~/.gitconfig" is). - %(RUNTIME-PREFIX) to make it similar to how Git replaces various tokens that are understood only in the context of Git. - <RUNTIME-PREFIX>---the downside is that this is an unnecessary new syntax we do not have to introduce. If <RUNTIME-PREFIX> is unlikely to be used as a valid directory name, %(RUNTIME-PREFIX) is just as unlikely. There might have been other ideas floated back then. I have to say that the one you chose in this round is the least favourite of mine, and if you consulted me privately before redoing this round, I would probably have said %(RUNTIME_PREFIX) would make the most sense among the candidates. Thanks. ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 0/5] mingw: handle absolute paths in expand_user_path() 2021-07-01 16:03 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget ` (2 preceding siblings ...) 2021-07-01 17:42 ` [PATCH v2 0/2] mingw: handle absolute paths in expand_user_path() Junio C Hamano @ 2021-07-24 22:06 ` Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 1/5] tests: exercise the RUNTIME_PREFIX feature Johannes Schindelin via GitGitGadget ` (5 more replies) 3 siblings, 6 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin In Git for Windows, we ran with a patch "in production" for quite a while where paths starting with a slash (i.e. looking like Unix paths, not like Windows paths) were interpreted as being relative to the runtime prefix, when expanded via expand_user_path(). This was sent to the Git mailing list as a discussion starter, and it was pointed out that this is neither portable nor unambiguous. After the dust settled, I thought about the presented ideas for a while (quite a while...), ended up with a solution, then adapted it to Junio's preference: any path starting with %(prefix)/ is expanded. This is ambiguous because it could be a valid path. But then, it is unlikely, and if someone really wants to specify such a path, it is easy to slap a ./ in front and they're done. Changes since v2: * Adjusted to Junio's preference %(...) over <...>. Of course, the preferred preference has the disadvantage of actually being allowed in Win32 filenames, but then, the workaround is as easy as on non-Windows platforms. * Since our convention of %(...) interpolation does not involve uppercase keywords, I now use a lowercase one. * Since this keyword is interpolated to the compiled-in prefix if built without runtime prefix support, I dropped the runtime part of the keyword. * Renamed the expand_user_path() to interpolate_path(), to remove the distraction as to the implementation detail which things get to be interpolated (because we extend it to interpolate more than just a home directory, which might well be unclear from the former name, anyway). * Adjusted the code comment above the interpolate_path() to remove a stale part, clarify another part, and to extend it to talk about the prefix expansion, too. Changes since v1: * Included a test for the RUNTIME_PREFIX that I had meant to send for ages already, and based on which... * A test case was added to verify that this actually works as intended * It is no longer Windows-specific * I added some documentation Johannes Schindelin (5): tests: exercise the RUNTIME_PREFIX feature expand_user_path(): remove stale part of the comment expand_user_path(): clarify the role of the `real_home` parameter Use a better name for the function interpolating paths interpolate_path(): allow specifying paths relative to the runtime prefix Documentation/config.txt | 9 +++++++++ Makefile | 5 +++++ builtin/credential-cache.c | 2 +- builtin/credential-store.c | 2 +- builtin/gc.c | 2 +- cache.h | 2 +- config.c | 8 ++++---- path.c | 19 +++++++++++++------ sequencer.c | 2 +- t/t0060-path-utils.sh | 26 ++++++++++++++++++++++++++ 10 files changed, 62 insertions(+), 15 deletions(-) base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-66%2Fdscho%2Fmingw-expand-absolute-user-path-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-66/dscho/mingw-expand-absolute-user-path-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/66 Range-diff vs v2: 1: cc8f09baba9 = 1: cc8f09baba9 tests: exercise the RUNTIME_PREFIX feature -: ----------- > 2: a4ff3a461bc expand_user_path(): remove stale part of the comment -: ----------- > 3: 7b79ba66dd0 expand_user_path(): clarify the role of the `real_home` parameter -: ----------- > 4: 19fd9c3c803 Use a better name for the function interpolating paths 2: 66df56f5db0 ! 5: d286583082e expand_user_path(): support specifying paths relative to the runtime prefix @@ Metadata Author: Johannes Schindelin <Johannes.Schindelin@gmx.de> ## Commit message ## - expand_user_path(): support specifying paths relative to the runtime prefix + interpolate_path(): allow specifying paths relative to the runtime prefix Ever since Git learned to detect its install location at runtime, there was the slightly awkward problem that it was impossible to specify paths @@ Commit message they are interpreted as absolute paths in the same drive as the current directory. - After a lengthy discussion, and a way lengthier time to mull over the - problem and its best solution, we decided to introduce support for the - magic sequence `<RUNTIME-PREFIX>/`. If a path starts with this, the - remainder is interpreted as relative to the detected runtime prefix. + After a lengthy discussion, and an even lengthier time to mull over the + problem and its best solution, and then more discussions, we eventually + decided to introduce support for the magic sequence `%(prefix)/`. If a + path starts with this, the remainder is interpreted as relative to the + detected (runtime) prefix. If built without runtime prefix support, Git + will simply interpolate the compiled-in prefix. - This solves the problem, but what new problems does it stir up? Here are - the two most obvious ones: - - - What if Git was not compiled with support for a runtime prefix? - - In that case, we will simply use the compiled-in hard-coded prefix. - - - What if a user _wants_ to specify a path starting with the magic - sequence? - - In that case, the user will simply need to prefix the magic sequence - with `./` and voilà, the path won't be expanded. + If a user _wants_ to specify a path starting with the magic sequence, + they can prefix the magic sequence with `./` and voilà, the path won't + be expanded. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> @@ Documentation/config.txt: pathname:: is expanded to the value of `$HOME`, and `~user/` to the specified user's home directory. ++ -+If a path starts with `<RUNTIME-PREFIX>/`, the remainder is -+interpreted as a path relative to Git's "runtime prefix", i.e. relative -+to the location where Git itself was installed. For example, -+`<RUNTIME-PREFIX>/bin/` refers to the directory in which the Git -+executable itself lives. If Git was compiled without runtime prefix -+support, the compiled-in prefix will be subsituted instead. In the -+unlikely event that a literal path needs to be specified that should -+_not_ be expanded, it needs to be prefixed by `./`, like so: -+`./<RUNTIME-PREFIX>/bin`. ++If a path starts with `%(prefix)/`, the remainder is interpreted as a ++path relative to Git's "runtime prefix", i.e. relative to the location ++where Git itself was installed. For example, `%(prefix)/bin/` refers to ++the directory in which the Git executable itself lives. If Git was ++compiled without runtime prefix support, the compiled-in prefix will be ++subsituted instead. In the unlikely event that a literal path needs to ++be specified that should _not_ be expanded, it needs to be prefixed by ++`./`, like so: `./%(prefix)/bin`. Variables @@ path.c static int get_st_mode_bits(const char *path, int *mode) { -@@ path.c: char *expand_user_path(const char *path, int real_home) +@@ path.c: static struct passwd *getpw_str(const char *username, size_t len) + * failure or if path is NULL. + * + * If real_home is true, strbuf_realpath($HOME) is used in the `~/` expansion. ++ * ++ * If the path starts with `%(prefix)/`, the remainder is interpreted as ++ * relative to where Git is installed, and expanded to the absolute path. + */ + char *interpolate_path(const char *path, int real_home) + { +@@ path.c: char *interpolate_path(const char *path, int real_home) if (path == NULL) goto return_null; + -+ if (skip_prefix(path, "<RUNTIME-PREFIX>/", &path)) ++ if (skip_prefix(path, "%(prefix)/", &path)) + return system_path(path); + if (path[0] == '~') { @@ t/t0060-path-utils.sh: test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTI echo HERE >expect && + test_cmp expect actual' + -+test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '<RUNTIME-PREFIX>/ works' ' ++test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' ' + mkdir -p pretend/bin && + cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && -+ git config yes.path "<RUNTIME-PREFIX>/yes" && ++ git config yes.path "%(prefix)/yes" && + GIT_EXEC_PATH= ./pretend/bin/git config --path yes.path >actual && + echo "$(pwd)/pretend/yes" >expect && test_cmp expect actual -- gitgitgadget ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 1/5] tests: exercise the RUNTIME_PREFIX feature 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 ` Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 2/5] expand_user_path(): remove stale part of the comment Johannes Schindelin via GitGitGadget ` (4 subsequent siblings) 5 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> Originally, we refrained from adding a regression test in 7b6c6496374 (system_path(): Add prefix computation at runtime if RUNTIME_PREFIX set, 2008-08-10), and in 226c0ddd0d6 (exec_cmd: RUNTIME_PREFIX on some POSIX systems, 2018-04-10). The reason was that it was deemed too tricky to test. Turns out that it is not tricky to test at all: we simply create a pseudo-root, copy the `git` executable into the `git/` subdirectory of that pseudo-root, then copy a script into the `libexec/git-core/` directory and expect that to be picked up. As long as the trash directory is in a location where binaries can be executed, this works. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Makefile | 5 +++++ t/t0060-path-utils.sh | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/Makefile b/Makefile index c3565fc0f8f..4c3e4ccabcd 100644 --- a/Makefile +++ b/Makefile @@ -2826,6 +2826,11 @@ ifdef GIT_TEST_INDEX_VERSION endif ifdef GIT_TEST_PERL_FATAL_WARNINGS @echo GIT_TEST_PERL_FATAL_WARNINGS=\''$(subst ','\'',$(subst ','\'',$(GIT_TEST_PERL_FATAL_WARNINGS)))'\' >>$@+ +endif +ifdef RUNTIME_PREFIX + @echo RUNTIME_PREFIX=\'true\' >>$@+ +else + @echo RUNTIME_PREFIX=\'false\' >>$@+ endif @if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index de4960783f0..a76728c27bf 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -525,4 +525,22 @@ test_expect_success MINGW 'is_valid_path() on Windows' ' "PRN./abc" ' +test_lazy_prereq RUNTIME_PREFIX ' + test true = "$RUNTIME_PREFIX" +' + +test_lazy_prereq CAN_EXEC_IN_PWD ' + cp "$GIT_EXEC_PATH"/git$X ./ && + ./git rev-parse +' + +test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' ' + mkdir -p pretend/bin pretend/libexec/git-core && + echo "echo HERE" | write_script pretend/libexec/git-core/git-here && + cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && + GIT_EXEC_PATH= ./pretend/bin/git here >actual && + echo HERE >expect && + test_cmp expect actual +' + test_done -- gitgitgadget ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 2/5] expand_user_path(): remove stale part of the comment 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 1/5] tests: exercise the RUNTIME_PREFIX feature Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 ` Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 3/5] expand_user_path(): clarify the role of the `real_home` parameter Johannes Schindelin via GitGitGadget ` (3 subsequent siblings) 5 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> In 395de250d9d (Expand ~ and ~user in core.excludesfile, commit.template, 2009-11-17), the `user_path()` function was refactored into the `expand_user_path()`. During that refactoring, the `buf` parameter was lost, but the code comment above said function still talks about it. Let's remove that stale part of the comment. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- path.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/path.c b/path.c index 7bccd830e95..3318ad24336 100644 --- a/path.c +++ b/path.c @@ -719,9 +719,8 @@ static struct passwd *getpw_str(const char *username, size_t len) } /* - * Return a string with ~ and ~user expanded via getpw*. If buf != NULL, - * then it is a newly allocated string. Returns NULL on getpw failure or - * if path is NULL. + * Return a string with ~ and ~user expanded via getpw*. Returns NULL on getpw + * failure or if path is NULL. * * If real_home is true, strbuf_realpath($HOME) is used in the expansion. */ -- gitgitgadget ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 3/5] expand_user_path(): clarify the role of the `real_home` parameter 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 1/5] tests: exercise the RUNTIME_PREFIX feature Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 2/5] expand_user_path(): remove stale part of the comment Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 ` Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 4/5] Use a better name for the function interpolating paths Johannes Schindelin via GitGitGadget ` (2 subsequent siblings) 5 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> The `real_home` parameter only has an effect when expanding paths starting with `~/`, not when expanding paths starting with `~<user>/`. Let's make that clear. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- path.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/path.c b/path.c index 3318ad24336..bf329e535cf 100644 --- a/path.c +++ b/path.c @@ -722,7 +722,7 @@ static struct passwd *getpw_str(const char *username, size_t len) * Return a string with ~ and ~user expanded via getpw*. Returns NULL on getpw * failure or if path is NULL. * - * If real_home is true, strbuf_realpath($HOME) is used in the expansion. + * If real_home is true, strbuf_realpath($HOME) is used in the `~/` expansion. */ char *expand_user_path(const char *path, int real_home) { -- gitgitgadget ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget ` (2 preceding siblings ...) 2021-07-24 22:06 ` [PATCH v3 3/5] expand_user_path(): clarify the role of the `real_home` parameter Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 ` Johannes Schindelin via GitGitGadget 2021-07-26 22:05 ` Junio C Hamano 2021-07-24 22:06 ` [PATCH v3 5/5] interpolate_path(): allow specifying paths relative to the runtime prefix Johannes Schindelin via GitGitGadget 2021-07-26 17:56 ` [PATCH v3 0/5] mingw: handle absolute paths in expand_user_path() Junio C Hamano 5 siblings, 1 reply; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> It is not immediately clear what `expand_user_path()` means, so let's rename it to `interpolate_path()`. This also opens the path for interpolating more than just a home directory. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- builtin/credential-cache.c | 2 +- builtin/credential-store.c | 2 +- builtin/gc.c | 2 +- cache.h | 2 +- config.c | 8 ++++---- path.c | 4 ++-- sequencer.c | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c index 76a6ba37223..e8a74157471 100644 --- a/builtin/credential-cache.c +++ b/builtin/credential-cache.c @@ -90,7 +90,7 @@ static char *get_socket_path(void) { struct stat sb; char *old_dir, *socket; - old_dir = expand_user_path("~/.git-credential-cache", 0); + old_dir = interpolate_path("~/.git-credential-cache", 0); if (old_dir && !stat(old_dir, &sb) && S_ISDIR(sb.st_mode)) socket = xstrfmt("%s/socket", old_dir); else diff --git a/builtin/credential-store.c b/builtin/credential-store.c index ae3c1ba75fe..62a4f3c2653 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -173,7 +173,7 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix) if (file) { string_list_append(&fns, file); } else { - if ((file = expand_user_path("~/.git-credentials", 0))) + if ((file = interpolate_path("~/.git-credentials", 0))) string_list_append_nodup(&fns, file); file = xdg_config_home("credentials"); if (file) diff --git a/builtin/gc.c b/builtin/gc.c index f05d2f0a1ac..6ce5ca45126 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1542,7 +1542,7 @@ static char *launchctl_service_filename(const char *name) struct strbuf filename = STRBUF_INIT; strbuf_addf(&filename, "~/Library/LaunchAgents/%s.plist", name); - expanded = expand_user_path(filename.buf, 1); + expanded = interpolate_path(filename.buf, 1); if (!expanded) die(_("failed to expand path '%s'"), filename.buf); diff --git a/cache.h b/cache.h index ba04ff8bd36..87e4cbe9c5f 100644 --- a/cache.h +++ b/cache.h @@ -1246,7 +1246,7 @@ typedef int create_file_fn(const char *path, void *cb); int raceproof_create_file(const char *path, create_file_fn fn, void *cb); int mkdir_in_gitdir(const char *path); -char *expand_user_path(const char *path, int real_home); +char *interpolate_path(const char *path, int real_home); const char *enter_repo(const char *path, int strict); static inline int is_absolute_path(const char *path) { diff --git a/config.c b/config.c index f9c400ad306..c17b9797292 100644 --- a/config.c +++ b/config.c @@ -137,7 +137,7 @@ static int handle_path_include(const char *path, struct config_include_data *inc if (!path) return config_error_nonbool("include.path"); - expanded = expand_user_path(path, 0); + expanded = interpolate_path(path, 0); if (!expanded) return error(_("could not expand include path '%s'"), path); path = expanded; @@ -185,7 +185,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat) char *expanded; int prefix = 0; - expanded = expand_user_path(pat->buf, 1); + expanded = interpolate_path(pat->buf, 1); if (expanded) { strbuf_reset(pat); strbuf_addstr(pat, expanded); @@ -1270,7 +1270,7 @@ int git_config_pathname(const char **dest, const char *var, const char *value) { if (!value) return config_error_nonbool(var); - *dest = expand_user_path(value, 0); + *dest = interpolate_path(value, 0); if (!*dest) die(_("failed to expand user dir in: '%s'"), value); return 0; @@ -1844,7 +1844,7 @@ void git_global_config(char **user_out, char **xdg_out) char *xdg_config = NULL; if (!user_config) { - user_config = expand_user_path("~/.gitconfig", 0); + user_config = interpolate_path("~/.gitconfig", 0); xdg_config = xdg_config_home("config"); } diff --git a/path.c b/path.c index bf329e535cf..8dc5ad2cb55 100644 --- a/path.c +++ b/path.c @@ -724,7 +724,7 @@ static struct passwd *getpw_str(const char *username, size_t len) * * If real_home is true, strbuf_realpath($HOME) is used in the `~/` expansion. */ -char *expand_user_path(const char *path, int real_home) +char *interpolate_path(const char *path, int real_home) { struct strbuf user_path = STRBUF_INIT; const char *to_copy = path; @@ -811,7 +811,7 @@ const char *enter_repo(const char *path, int strict) strbuf_add(&validated_path, path, len); if (used_path.buf[0] == '~') { - char *newpath = expand_user_path(used_path.buf, 0); + char *newpath = interpolate_path(used_path.buf, 0); if (!newpath) return NULL; strbuf_attach(&used_path, newpath, strlen(newpath), diff --git a/sequencer.c b/sequencer.c index 0bec01cf38e..007a851804d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1241,7 +1241,7 @@ N_("Your name and email address were configured automatically based\n" static const char *implicit_ident_advice(void) { - char *user_config = expand_user_path("~/.gitconfig", 0); + char *user_config = interpolate_path("~/.gitconfig", 0); char *xdg_config = xdg_config_home("config"); int config_exists = file_exists(user_config) || file_exists(xdg_config); -- gitgitgadget ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-24 22:06 ` [PATCH v3 4/5] Use a better name for the function interpolating paths Johannes Schindelin via GitGitGadget @ 2021-07-26 22:05 ` Junio C Hamano 2021-07-27 7:57 ` Fabian Stelzer 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2021-07-26 22:05 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget, Fabian Stelzer Cc: git, Ramsay Jones, Duy Nguyen, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > It is not immediately clear what `expand_user_path()` means, so let's > rename it to `interpolate_path()`. This also opens the path for > interpolating more than just a home directory. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > ... > diff --git a/cache.h b/cache.h > index ba04ff8bd36..87e4cbe9c5f 100644 > --- a/cache.h > +++ b/cache.h > @@ -1246,7 +1246,7 @@ typedef int create_file_fn(const char *path, void *cb); > int raceproof_create_file(const char *path, create_file_fn fn, void *cb); > > int mkdir_in_gitdir(const char *path); > -char *expand_user_path(const char *path, int real_home); > +char *interpolate_path(const char *path, int real_home); This of course breaks any topic in flight that adds more places to use expand_user_path(). I think Fabian's "ssh signing" is not as ready as this topic, and it can afford to wait by rebasing on top of this topic. By the time "ssh signing" gets into testable shape (right now, it does not pass tests when merged to 'seen'), hopefully the "expand install-prefix" topic may already be in 'next' if not in 'master'. In the meantime, I am adding this band-aid at the tip of this topic to help these two topics play together better. Thanks. diff --git a/cache.h b/cache.h index 87e4cbe9c5..679a27e17c 100644 --- a/cache.h +++ b/cache.h @@ -1247,6 +1247,8 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb); int mkdir_in_gitdir(const char *path); char *interpolate_path(const char *path, int real_home); +/* NEEDSWORK: remove this synonym once in-flight topics have migrated */ +#define expand_user_path interpolate_path const char *enter_repo(const char *path, int strict); static inline int is_absolute_path(const char *path) { ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-26 22:05 ` Junio C Hamano @ 2021-07-27 7:57 ` Fabian Stelzer 2021-07-27 22:56 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Fabian Stelzer @ 2021-07-27 7:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Ævar Arnfjörð Bjarmason On 27.07.21 00:05, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > >> From: Johannes Schindelin <johannes.schindelin@gmx.de> >> >> It is not immediately clear what `expand_user_path()` means, so let's >> rename it to `interpolate_path()`. This also opens the path for >> interpolating more than just a home directory. >> >> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> >> --- >> ... >> diff --git a/cache.h b/cache.h >> index ba04ff8bd36..87e4cbe9c5f 100644 >> --- a/cache.h >> +++ b/cache.h >> @@ -1246,7 +1246,7 @@ typedef int create_file_fn(const char *path, void *cb); >> int raceproof_create_file(const char *path, create_file_fn fn, void *cb); >> >> int mkdir_in_gitdir(const char *path); >> -char *expand_user_path(const char *path, int real_home); >> +char *interpolate_path(const char *path, int real_home); > This of course breaks any topic in flight that adds more places to > use expand_user_path(). > > I think Fabian's "ssh signing" is not as ready as this topic, and it > can afford to wait by rebasing on top of this topic. By the time > "ssh signing" gets into testable shape (right now, it does not pass > tests when merged to 'seen'), hopefully the "expand install-prefix" > topic may already be in 'next' if not in 'master'. I think the test problem is not due to my patch. Like Ævar wrote it's "hn/reftable probably interacting with my ab/refs-files-cleanup" [1] The failed tests i can see in [2] are either t0031-reftable.sh or a compile failure referencing reftable as well. If i merge the ssh code into the current seen branch everything works fine for me. Let me know if you have any other results / CI runs that might help. [1] https://lore.kernel.org/git/87a6mevkrx.fsf@evledraar.gmail.com/ [2] https://github.com/git/git/actions/runs/1053603028 > > In the meantime, I am adding this band-aid at the tip of this topic > to help these two topics play together better. > > Thanks. > > > diff --git a/cache.h b/cache.h > index 87e4cbe9c5..679a27e17c 100644 > --- a/cache.h > +++ b/cache.h > @@ -1247,6 +1247,8 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb); > > int mkdir_in_gitdir(const char *path); > char *interpolate_path(const char *path, int real_home); > +/* NEEDSWORK: remove this synonym once in-flight topics have migrated */ > +#define expand_user_path interpolate_path > const char *enter_repo(const char *path, int strict); > static inline int is_absolute_path(const char *path) > { ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-27 7:57 ` Fabian Stelzer @ 2021-07-27 22:56 ` Junio C Hamano 2021-07-28 0:14 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2021-07-27 22:56 UTC (permalink / raw) To: Fabian Stelzer; +Cc: git, Ævar Arnfjörð Bjarmason Fabian Stelzer <fs@gigacodes.de> writes: >> I think Fabian's "ssh signing" is not as ready as this topic, and it >> can afford to wait by rebasing on top of this topic. By the time >> "ssh signing" gets into testable shape (right now, it does not pass >> tests when merged to 'seen'), hopefully the "expand install-prefix" >> topic may already be in 'next' if not in 'master'. > I think the test problem is not due to my patch. I've been seeing these test failers locally, every time fs/ssh-signing topic is merged to 'seen' (without the reftable thing). Test Summary Report ------------------- t5534-push-signed.sh (Wstat: 256 Tests: 13 Failed: 2) Failed tests: 8, 12 Non-zero exit status: 1 t7528-signed-commit-ssh.sh (Wstat: 256 Tests: 23 Failed: 2) Failed tests: 13, 17 Non-zero exit status: 1 When reftable thing is merged, either compilation fails or t0031 fails, and I suspect that these are not due to the ssh signing topic. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-27 22:56 ` Junio C Hamano @ 2021-07-28 0:14 ` Junio C Hamano 2021-07-28 0:39 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2021-07-28 0:14 UTC (permalink / raw) To: Fabian Stelzer; +Cc: git, Ævar Arnfjörð Bjarmason Junio C Hamano <gitster@pobox.com> writes: > Fabian Stelzer <fs@gigacodes.de> writes: > >>> I think Fabian's "ssh signing" is not as ready as this topic, and it >>> can afford to wait by rebasing on top of this topic. By the time >>> "ssh signing" gets into testable shape (right now, it does not pass >>> tests when merged to 'seen'), hopefully the "expand install-prefix" >>> topic may already be in 'next' if not in 'master'. >> I think the test problem is not due to my patch. > > I've been seeing these test failers locally, every time > fs/ssh-signing topic is merged to 'seen' (without the reftable > thing). > > Test Summary Report > ------------------- > t5534-push-signed.sh (Wstat: 256 Tests: 13 Failed: 2) > Failed tests: 8, 12 > Non-zero exit status: 1 > t7528-signed-commit-ssh.sh (Wstat: 256 Tests: 23 Failed: 2) > Failed tests: 13, 17 > Non-zero exit status: 1 > > When reftable thing is merged, either compilation fails or t0031 > fails, and I suspect that these are not due to the ssh signing > topic. Interesting. It seems that the failure has some correlation with the use of --root=<trash directory> option. $ sh t5534-push-signed.sh -i ok 1 - setup ok 2 - unsigned push does not send push certificate ok 3 - talking with a receiver without push certificate support ok 4 - push --signed fails with a receiver without push certificate support ok 5 - push --signed=1 is accepted ok 6 - no certificate for a signed push with no update ok 7 - signed push sends push certificate ok 8 - ssh signed push sends push certificate ok 9 - inconsistent push options in signed push not allowed ok 10 - fail without key and heed user.signingkey ok 11 - fail without key and heed user.signingkey x509 ok 12 - fail without key and heed user.signingkey ssh ok 13 - failed atomic push does not execute GPG # passed all 13 test(s) 1..13 passes just fine, but $ TESTPEN=/dev/shm/testpen.$$ $ rm -fr "$TESTPEN" && mkdir "$TESTPEN" $ sh t5534-push-signed.sh --root=$TESTPEN -i -v dies like this: Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/.git/ expecting success of 5534.1 'setup': # main, ff and noff branches pointing at the same commit test_tick && git commit --allow-empty -m initial && git checkout -b noop && git checkout -b ff && git checkout -b noff && # noop stays the same, ff advances, noff rewrites test_tick && git commit --allow-empty --amend -m rewritten && git checkout ff && test_tick && git commit --allow-empty -m second [main (root-commit) 66fe8b3] initial Author: A U Thor <author@example.com> Switched to a new branch 'noop' Switched to a new branch 'ff' Switched to a new branch 'noff' [noff 6391b7f] rewritten Author: A U Thor <author@example.com> Date: Thu Apr 7 15:13:13 2005 -0700 Switched to branch 'ff' [ff 566fbd3] second Author: A U Thor <author@example.com> ok 1 - setup expecting success of 5534.2 'unsigned push does not send push certificate': prepare_dst && mkdir -p dst/.git/hooks && write_script dst/.git/hooks/post-receive <<-\EOF && # discard the update list cat >/dev/null # record the push certificate if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi EOF git push dst noop ff +noff && ! test -f dst/push-cert Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff To dst 66fe8b3..566fbd3 ff -> ff + 66fe8b3...6391b7f noff -> noff (forced update) ok 2 - unsigned push does not send push certificate expecting success of 5534.3 'talking with a receiver without push certificate support': prepare_dst && mkdir -p dst/.git/hooks && write_script dst/.git/hooks/post-receive <<-\EOF && # discard the update list cat >/dev/null # record the push certificate if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi EOF git push dst noop ff +noff && ! test -f dst/push-cert Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff To dst 66fe8b3..566fbd3 ff -> ff + 66fe8b3...6391b7f noff -> noff (forced update) ok 3 - talking with a receiver without push certificate support expecting success of 5534.4 'push --signed fails with a receiver without push certificate support': prepare_dst && mkdir -p dst/.git/hooks && test_must_fail git push --signed dst noop ff +noff 2>err && test_i18ngrep "the receiving end does not support" err Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff fatal: the receiving end does not support --signed push ok 4 - push --signed fails with a receiver without push certificate support expecting success of 5534.5 'push --signed=1 is accepted': prepare_dst && mkdir -p dst/.git/hooks && test_must_fail git push --signed=1 dst noop ff +noff 2>err && test_i18ngrep "the receiving end does not support" err Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff fatal: the receiving end does not support --signed push ok 5 - push --signed=1 is accepted checking prerequisite: GPG mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-GPG" && ( cd "$TRASH_DIRECTORY/prereq-test-dir-GPG" && gpg_version=$(gpg --version 2>&1) test $? != 127 || exit 1 # As said here: http://www.gnupg.org/documentation/faqs.html#q6.19 # the gpg version 1.0.6 did not parse trust packets correctly, so for # that version, creation of signed tags using the generated key fails. case "$gpg_version" in "gpg (GnuPG) 1.0.6"*) say "Your version of gpg (1.0.6) is too buggy for testing" exit 1 ;; *) # Available key info: # * Type DSA and Elgamal, size 2048 bits, no expiration date, # name and email: C O Mitter <committer@example.com> # * Type RSA, size 2048 bits, no expiration date, # name and email: Eris Discordia <discord@example.net> # No password given, to enable non-interactive operation. # To generate new key: # gpg --homedir /tmp/gpghome --gen-key # To write armored exported key to keyring: # gpg --homedir /tmp/gpghome --export-secret-keys \ # --armor 0xDEADBEEF >> lib-gpg/keyring.gpg # gpg --homedir /tmp/gpghome --export \ # --armor 0xDEADBEEF >> lib-gpg/keyring.gpg # To export ownertrust: # gpg --homedir /tmp/gpghome --export-ownertrust \ # > lib-gpg/ownertrust mkdir "$GNUPGHOME" && chmod 0700 "$GNUPGHOME" && (gpgconf --kill gpg-agent || : ) && gpg --homedir "${GNUPGHOME}" --import \ "$TEST_DIRECTORY"/lib-gpg/keyring.gpg && gpg --homedir "${GNUPGHOME}" --import-ownertrust \ "$TEST_DIRECTORY"/lib-gpg/ownertrust && gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null \ --sign -u committer@example.com ;; esac ) gpg: keybox '/dev/shm/testpen.9441/trash directory.t5534-push-signed/gpghome/pubring.kbx' created gpg: /dev/shm/testpen.9441/trash directory.t5534-push-signed/gpghome/trustdb.gpg: trustdb created gpg: key 13B6F51ECDDE430D: public key "C O Mitter <committer@example.com>" imported gpg: key 13B6F51ECDDE430D: secret key imported gpg: key 61092E85B7227189: public key "Eris Discordia <discord@example.net>" imported gpg: key 61092E85B7227189: secret key imported gpg: key 13B6F51ECDDE430D: "C O Mitter <committer@example.com>" not changed gpg: key 61092E85B7227189: "Eris Discordia <discord@example.net>" not changed gpg: Total number processed: 4 gpg: imported: 2 gpg: unchanged: 2 gpg: secret keys read: 2 gpg: secret keys imported: 2 gpg: inserting ownertrust of 6 gpg: inserting ownertrust of 3 prerequisite GPG ok expecting success of 5534.6 'no certificate for a signed push with no update': prepare_dst && mkdir -p dst/.git/hooks && write_script dst/.git/hooks/post-receive <<-\EOF && if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi EOF git push dst noop && ! test -f dst/push-cert Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff Everything up-to-date ok 6 - no certificate for a signed push with no update expecting success of 5534.7 'signed push sends push certificate': prepare_dst && mkdir -p dst/.git/hooks && git -C dst config receive.certnonceseed sekrit && write_script dst/.git/hooks/post-receive <<-\EOF && # discard the update list cat >/dev/null # record the push certificate if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi && cat >../push-cert-status <<E_O_F SIGNER=${GIT_PUSH_CERT_SIGNER-nobody} KEY=${GIT_PUSH_CERT_KEY-nokey} STATUS=${GIT_PUSH_CERT_STATUS-nostatus} NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus} NONCE=${GIT_PUSH_CERT_NONCE-nononce} E_O_F EOF git push --signed dst noop ff +noff && ( cat <<-\EOF && SIGNER=C O Mitter <committer@example.com> KEY=13B6F51ECDDE430D STATUS=G NONCE_STATUS=OK EOF sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert ) >expect && noop=$(git rev-parse noop) && ff=$(git rev-parse ff) && noff=$(git rev-parse noff) && grep "$noop $ff refs/heads/ff" dst/push-cert && grep "$noop $noff refs/heads/noff" dst/push-cert && test_cmp expect dst/push-cert-status Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff To dst 66fe8b3..566fbd3 ff -> ff + 66fe8b3...6391b7f noff -> noff (forced update) 66fe8b3f2df5c2a6e67944af865f3a0893093d69 566fbd34a75c18947f0bcd052512caf55e7144ba refs/heads/ff 66fe8b3f2df5c2a6e67944af865f3a0893093d69 6391b7f36bc1393eab3cad0aaf8c08cdacbe78fa refs/heads/noff ok 7 - signed push sends push certificate checking prerequisite: GPGSSH mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-GPGSSH" && ( cd "$TRASH_DIRECTORY/prereq-test-dir-GPGSSH" && ssh_version=$(ssh-keygen -Y find-principals -n "git" 2>&1) test $? != 127 || exit 1 echo $ssh_version | grep -q "find-principals:missing signature file" test $? = 0 || exit 1; mkdir -p "${GNUPGHOME}" && chmod 0700 "${GNUPGHOME}" && ssh-keygen -t ed25519 -N "" -f "${GNUPGHOME}/ed25519_ssh_signing_key" >/dev/null && ssh-keygen -t rsa -b 2048 -N "" -f "${GNUPGHOME}/rsa_2048_ssh_signing_key" >/dev/null && ssh-keygen -t ed25519 -N "super_secret" -f "${GNUPGHOME}/protected_ssh_signing_key" >/dev/null && find "${GNUPGHOME}" -name *ssh_signing_key.pub -exec cat {} \; | awk "{print \"\\\"principal with number \" NR \"\\\" \" \$0}" > "${GNUPGHOME}/ssh.all_valid.allowedSignersFile" && cat "${GNUPGHOME}/ssh.all_valid.allowedSignersFile" && ssh-keygen -t ed25519 -N "" -f "${GNUPGHOME}/untrusted_ssh_signing_key" >/dev/null ) "principal with number 1" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIK+hDFZT7sYN3M+1ciMGLTM6pu/xmJrNDTK/vN+QIVnq jch@gitster.c.googlers.com "principal with number 2" ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDWLnqVpDYNstR7jCPKr1FaWzxt7NNw/kEV61GgKThW9S54/p/0WN+SCtUI0j7dCv/NkNhy87WNhohC9rCvZowPf/HRflHZ28vVk5d0DERCmlLBHnTq65Buyzj2R5xMtyVOBBMd+Io6tXk6GfJ6Dvq9l1wIJLv3OodOLBym3idV+8C+GOw9teTOlQWwkZD2hAt0n1Dy5WtaeGm3O2aUwRyg7KuxxmsgadBJ13a9thMYpwkS1McnwB+7oS81VXeeF0WcKAa2tGvxlg8NdBZuvvLJkl5vcISnpjt0NIuhqHzMlBIprpKujkZ5ze18YHXA52w6Y+9hG40n819EKjQfBVtD jch@gitster.c.googlers.com "principal with number 3" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAgtFx51cu1d0gzZOjIdw4M9oBYgV+tX6Bsm2L+riv/Z jch@gitster.c.googlers.com prerequisite GPGSSH ok expecting success of 5534.8 'ssh signed push sends push certificate': prepare_dst && mkdir -p dst/.git/hooks && git -C dst config gpg.ssh.allowedSignersFile "${SIGNING_ALLOWED_SIGNERS}" && git -C dst config receive.certnonceseed sekrit && write_script dst/.git/hooks/post-receive <<-\EOF && # discard the update list cat >/dev/null # record the push certificate if test -n "${GIT_PUSH_CERT-}" then git cat-file blob $GIT_PUSH_CERT >../push-cert fi && cat >../push-cert-status <<E_O_F SIGNER=${GIT_PUSH_CERT_SIGNER-nobody} KEY=${GIT_PUSH_CERT_KEY-nokey} STATUS=${GIT_PUSH_CERT_STATUS-nostatus} NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus} NONCE=${GIT_PUSH_CERT_NONCE-nononce} E_O_F EOF test_config gpg.format ssh && test_config user.signingkey "${SIGNING_KEY_PRIMARY}" && FINGERPRINT=$(ssh-keygen -lf "${SIGNING_KEY_PRIMARY}" | awk "{print \$2;}") && git push --signed dst noop ff +noff && ( cat <<-\EOF && SIGNER=principal with number 1 KEY=FINGERPRINT STATUS=G NONCE_STATUS=OK EOF sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert ) | sed -e "s|FINGERPRINT|$FINGERPRINT|" >expect && noop=$(git rev-parse noop) && ff=$(git rev-parse ff) && noff=$(git rev-parse noff) && grep "$noop $ff refs/heads/ff" dst/push-cert && grep "$noop $noff refs/heads/noff" dst/push-cert && test_cmp expect dst/push-cert-status Initialized empty Git repository in /dev/shm/testpen.9441/trash directory.t5534-push-signed/dst/.git/ To dst * [new branch] main -> noop * [new branch] main -> ff * [new branch] main -> noff To dst 66fe8b3..566fbd3 ff -> ff + 66fe8b3...6391b7f noff -> noff (forced update) 66fe8b3f2df5c2a6e67944af865f3a0893093d69 566fbd34a75c18947f0bcd052512caf55e7144ba refs/heads/ff 66fe8b3f2df5c2a6e67944af865f3a0893093d69 6391b7f36bc1393eab3cad0aaf8c08cdacbe78fa refs/heads/noff --- expect 2021-07-28 00:11:20.863019887 +0000 +++ dst/push-cert-status 2021-07-28 00:11:20.855019156 +0000 @@ -1,4 +1,4 @@ -SIGNER=principal with number 1 +SIGNER=principal with number 3 KEY=SHA256:Szd5rzYOrMBJFTR+gnRUu60YRVqg98UvpcSvmAm89rE STATUS=G NONCE_STATUS=OK not ok 8 - ssh signed push sends push certificate # # prepare_dst && # mkdir -p dst/.git/hooks && # git -C dst config gpg.ssh.allowedSignersFile "${SIGNING_ALLOWED_SIGNERS}" && # git -C dst config receive.certnonceseed sekrit && # write_script dst/.git/hooks/post-receive <<-\EOF && # # discard the update list # cat >/dev/null # # record the push certificate # if test -n "${GIT_PUSH_CERT-}" # then # git cat-file blob $GIT_PUSH_CERT >../push-cert # fi && # # cat >../push-cert-status <<E_O_F # SIGNER=${GIT_PUSH_CERT_SIGNER-nobody} # KEY=${GIT_PUSH_CERT_KEY-nokey} # STATUS=${GIT_PUSH_CERT_STATUS-nostatus} # NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus} # NONCE=${GIT_PUSH_CERT_NONCE-nononce} # E_O_F # # EOF # # test_config gpg.format ssh && # test_config user.signingkey "${SIGNING_KEY_PRIMARY}" && # FINGERPRINT=$(ssh-keygen -lf "${SIGNING_KEY_PRIMARY}" | awk "{print \$2;}") && # git push --signed dst noop ff +noff && # # ( # cat <<-\EOF && # SIGNER=principal with number 1 # KEY=FINGERPRINT # STATUS=G # NONCE_STATUS=OK # EOF # sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert # ) | sed -e "s|FINGERPRINT|$FINGERPRINT|" >expect && # # noop=$(git rev-parse noop) && # ff=$(git rev-parse ff) && # noff=$(git rev-parse noff) && # grep "$noop $ff refs/heads/ff" dst/push-cert && # grep "$noop $noff refs/heads/noff" dst/push-cert && # test_cmp expect dst/push-cert-status # ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-28 0:14 ` Junio C Hamano @ 2021-07-28 0:39 ` Junio C Hamano 2021-07-28 5:43 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2021-07-28 0:39 UTC (permalink / raw) To: Fabian Stelzer; +Cc: git, Ævar Arnfjörð Bjarmason Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Fabian Stelzer <fs@gigacodes.de> writes: >> >>>> I think Fabian's "ssh signing" is not as ready as this topic, and it >>>> can afford to wait by rebasing on top of this topic. By the time >>>> "ssh signing" gets into testable shape (right now, it does not pass >>>> tests when merged to 'seen'), hopefully the "expand install-prefix" >>>> topic may already be in 'next' if not in 'master'. >>> I think the test problem is not due to my patch. >> >> I've been seeing these test failers locally, every time >> fs/ssh-signing topic is merged to 'seen' (without the reftable >> thing). >> >> Test Summary Report >> ------------------- >> t5534-push-signed.sh (Wstat: 256 Tests: 13 Failed: 2) >> Failed tests: 8, 12 >> Non-zero exit status: 1 >> t7528-signed-commit-ssh.sh (Wstat: 256 Tests: 23 Failed: 2) >> Failed tests: 13, 17 >> Non-zero exit status: 1 >> >> When reftable thing is merged, either compilation fails or t0031 >> fails, and I suspect that these are not due to the ssh signing >> topic. > > Interesting. It seems that the failure has some correlation with > the use of --root=<trash directory> option. > > $ sh t5534-push-signed.sh -i And 7528 fails with --root set to a /dev/shm/ trash directory. So,... this "principal with number #" differences come from the differences in location of the trash directory? Why? > --- expect 2021-07-28 00:11:20.863019887 +0000 > +++ dst/push-cert-status 2021-07-28 00:11:20.855019156 +0000 > @@ -1,4 +1,4 @@ > -SIGNER=principal with number 1 > +SIGNER=principal with number 3 > KEY=SHA256:Szd5rzYOrMBJFTR+gnRUu60YRVqg98UvpcSvmAm89rE > STATUS=G > NONCE_STATUS=OK ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-28 0:39 ` Junio C Hamano @ 2021-07-28 5:43 ` Junio C Hamano 2021-07-28 8:18 ` Fabian Stelzer 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2021-07-28 5:43 UTC (permalink / raw) To: Fabian Stelzer; +Cc: git, Ævar Arnfjörð Bjarmason Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >>> Fabian Stelzer <fs@gigacodes.de> writes: >>> >>>>> I think Fabian's "ssh signing" is not as ready as this topic, and it >>>>> can afford to wait by rebasing on top of this topic. By the time >>>>> "ssh signing" gets into testable shape (right now, it does not pass >>>>> tests when merged to 'seen'), hopefully the "expand install-prefix" >>>>> topic may already be in 'next' if not in 'master'. >>>> I think the test problem is not due to my patch. >>> >>> I've been seeing these test failers locally, every time >>> fs/ssh-signing topic is merged to 'seen' (without the reftable >>> thing). >>> ... >> Interesting. It seems that the failure has some correlation with >> the use of --root=<trash directory> option. >> >> $ sh t5534-push-signed.sh -i > > And 7528 fails with --root set to a /dev/shm/ trash directory. An update. The same failure can be seen _without_ merging the topic to 'seen'. The topic by itself will fail t5534 and t7528 when run with --root= set to somewhere: $ make $ testpen=/dev/shm/testpen.$$ $ rm -fr "$testpen" && mkdir "$testpen" $ cd t $ sh t5534-*.sh --root=$testpen -i $ sh t7528-*.sh --root=$testpen -i on the branch itself, without getting interference by any other topic, should hopefully be an easy enough way to reproduce the problem. Thanks. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-28 5:43 ` Junio C Hamano @ 2021-07-28 8:18 ` Fabian Stelzer 2021-07-28 17:08 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Fabian Stelzer @ 2021-07-28 8:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Ævar Arnfjörð Bjarmason On 28.07.21 07:43, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >>> Junio C Hamano <gitster@pobox.com> writes: >>> >>>> Fabian Stelzer <fs@gigacodes.de> writes: >>>> >>>>>> I think Fabian's "ssh signing" is not as ready as this topic, and it >>>>>> can afford to wait by rebasing on top of this topic. By the time >>>>>> "ssh signing" gets into testable shape (right now, it does not pass >>>>>> tests when merged to 'seen'), hopefully the "expand install-prefix" >>>>>> topic may already be in 'next' if not in 'master'. >>>>> I think the test problem is not due to my patch. >>>> I've been seeing these test failers locally, every time >>>> fs/ssh-signing topic is merged to 'seen' (without the reftable >>>> thing). >>>> ... >>> Interesting. It seems that the failure has some correlation with >>> the use of --root=<trash directory> option. >>> >>> $ sh t5534-push-signed.sh -i >> And 7528 fails with --root set to a /dev/shm/ trash directory. > An update. The same failure can be seen _without_ merging the topic > to 'seen'. The topic by itself will fail t5534 and t7528 when run > with --root= set to somewhere: > > $ make > $ testpen=/dev/shm/testpen.$$ > $ rm -fr "$testpen" && mkdir "$testpen" > $ cd t > $ sh t5534-*.sh --root=$testpen -i > $ sh t7528-*.sh --root=$testpen -i > > on the branch itself, without getting interference by any other > topic, should hopefully be an easy enough way to reproduce the > problem. > > Thanks. ok, funny issue. in the ssh test setup i generated a few ssh keys for testing and (wanting to be clever) concatenated them with a prefixed principal into an allowedSigners file using find & awk. Turns out the directory entries in /dev/shm are the other way around. touch ./t1 ./t2 /dev/shm/t1 /dev/shm/t2 find ./ -name 't[0-9]' results in: ./t1 ./t2 a find /dev/shm -name 't[0-9]' returns: /dev/shm/t2 /dev/shm/t1 I'll change the test setup code to do this statically for each key. Not such a good idea to rely on the file order in the dir anyway. Thanks ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 4/5] Use a better name for the function interpolating paths 2021-07-28 8:18 ` Fabian Stelzer @ 2021-07-28 17:08 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2021-07-28 17:08 UTC (permalink / raw) To: Fabian Stelzer; +Cc: git, Ævar Arnfjörð Bjarmason Fabian Stelzer <fs@gigacodes.de> writes: > ok, funny issue. in the ssh test setup i generated a few ssh keys for > testing and (wanting to be clever) concatenated them with a prefixed > principal into an allowedSigners file using find & awk. > > Turns out the directory entries in /dev/shm are the other way around. Good finding. Yes, relying on the order "find" discovers filesystem entities is asking for trouble. > I'll change the test setup code to do this statically for each > key. Not such a good idea to rely on the file order in the dir anyway. Thanks. ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 5/5] interpolate_path(): allow specifying paths relative to the runtime prefix 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget ` (3 preceding siblings ...) 2021-07-24 22:06 ` [PATCH v3 4/5] Use a better name for the function interpolating paths Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 ` Johannes Schindelin via GitGitGadget 2021-07-26 17:56 ` [PATCH v3 0/5] mingw: handle absolute paths in expand_user_path() Junio C Hamano 5 siblings, 0 replies; 53+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2021-07-24 22:06 UTC (permalink / raw) To: git Cc: Ramsay Jones, Duy Nguyen, Junio C Hamano, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> Ever since Git learned to detect its install location at runtime, there was the slightly awkward problem that it was impossible to specify paths relative to said location. For example, if a version of Git was shipped with custom SSL certificates to use, there was no portable way to specify `http.sslCAInfo`. In Git for Windows, the problem was "solved" for years by interpreting paths starting with a slash as relative to the runtime prefix. However, this is not correct: such paths _are_ legal on Windows, and they are interpreted as absolute paths in the same drive as the current directory. After a lengthy discussion, and an even lengthier time to mull over the problem and its best solution, and then more discussions, we eventually decided to introduce support for the magic sequence `%(prefix)/`. If a path starts with this, the remainder is interpreted as relative to the detected (runtime) prefix. If built without runtime prefix support, Git will simply interpolate the compiled-in prefix. If a user _wants_ to specify a path starting with the magic sequence, they can prefix the magic sequence with `./` and voilà, the path won't be expanded. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Documentation/config.txt | 9 +++++++++ path.c | 8 ++++++++ t/t0060-path-utils.sh | 8 ++++++++ 3 files changed, 25 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index bf82766a6a2..0c0e6b859f1 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -298,6 +298,15 @@ pathname:: tilde expansion happens to such a string: `~/` is expanded to the value of `$HOME`, and `~user/` to the specified user's home directory. ++ +If a path starts with `%(prefix)/`, the remainder is interpreted as a +path relative to Git's "runtime prefix", i.e. relative to the location +where Git itself was installed. For example, `%(prefix)/bin/` refers to +the directory in which the Git executable itself lives. If Git was +compiled without runtime prefix support, the compiled-in prefix will be +subsituted instead. In the unlikely event that a literal path needs to +be specified that should _not_ be expanded, it needs to be prefixed by +`./`, like so: `./%(prefix)/bin`. Variables diff --git a/path.c b/path.c index 8dc5ad2cb55..0bc788ea40f 100644 --- a/path.c +++ b/path.c @@ -12,6 +12,7 @@ #include "packfile.h" #include "object-store.h" #include "lockfile.h" +#include "exec-cmd.h" static int get_st_mode_bits(const char *path, int *mode) { @@ -723,6 +724,9 @@ static struct passwd *getpw_str(const char *username, size_t len) * failure or if path is NULL. * * If real_home is true, strbuf_realpath($HOME) is used in the `~/` expansion. + * + * If the path starts with `%(prefix)/`, the remainder is interpreted as + * relative to where Git is installed, and expanded to the absolute path. */ char *interpolate_path(const char *path, int real_home) { @@ -731,6 +735,10 @@ char *interpolate_path(const char *path, int real_home) if (path == NULL) goto return_null; + + if (skip_prefix(path, "%(prefix)/", &path)) + return system_path(path); + if (path[0] == '~') { const char *first_slash = strchrnul(path, '/'); const char *username = path + 1; diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index a76728c27bf..34d1061f321 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -540,6 +540,14 @@ test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD 'RUNTIME_PREFIX works' ' cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && GIT_EXEC_PATH= ./pretend/bin/git here >actual && echo HERE >expect && + test_cmp expect actual' + +test_expect_success RUNTIME_PREFIX,CAN_EXEC_IN_PWD '%(prefix)/ works' ' + mkdir -p pretend/bin && + cp "$GIT_EXEC_PATH"/git$X pretend/bin/ && + git config yes.path "%(prefix)/yes" && + GIT_EXEC_PATH= ./pretend/bin/git config --path yes.path >actual && + echo "$(pwd)/pretend/yes" >expect && test_cmp expect actual ' -- gitgitgadget ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 0/5] mingw: handle absolute paths in expand_user_path() 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget ` (4 preceding siblings ...) 2021-07-24 22:06 ` [PATCH v3 5/5] interpolate_path(): allow specifying paths relative to the runtime prefix Johannes Schindelin via GitGitGadget @ 2021-07-26 17:56 ` Junio C Hamano 5 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2021-07-26 17:56 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget Cc: git, Ramsay Jones, Duy Nguyen, Johannes Sixt, Jeff King, Eric Sunshine, Johannes Schindelin "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > * Since our convention of %(...) interpolation does not involve uppercase > keywords, I now use a lowercase one. Makes sense. > * Since this keyword is interpolated to the compiled-in prefix if built > without runtime prefix support, I dropped the runtime part of the > keyword. I have this nagging feeling that %(prefix) may be (mistakenly) expected to interporate to $(git rev-parse --show-prefix). Of course, nobody would expect that in paths in the configuration files, but because we are borrowing %(token) convention from elsewhere, it is not outragous to imagine that either "for-each-ref" family or "log" family of placeholders may want to use %(prefix)" for such purpose (for that matter, they may also be helped to have the runtime-prefix information available). Perhaps %(installPrefix) or something may have less chance of making us regret later. I am just raising this as a possible problem; I personally would not be confused if we settled on the %(prefix). > * Renamed the expand_user_path() to interpolate_path(), to remove the > distraction as to the implementation detail which things get to be > interpolated (because we extend it to interpolate more than just a home > directory, which might well be unclear from the former name, anyway). Great. > * Adjusted the code comment above the interpolate_path() to remove a stale > part, clarify another part, and to extend it to talk about the prefix > expansion, too. These looked good, too. Thanks. ^ permalink raw reply [flat|nested] 53+ messages in thread
end of thread, other threads:[~2021-07-28 17:08 UTC | newest] Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-06 14:53 [PATCH 0/1] mingw: handle absolute paths in expand_user_path() Johannes Schindelin via GitGitGadget 2018-11-06 14:53 ` [PATCH 1/1] " Johannes Schindelin via GitGitGadget 2018-11-06 15:54 ` Ramsay Jones 2018-11-06 16:10 ` Ramsay Jones 2018-11-06 18:27 ` Duy Nguyen 2018-11-07 1:21 ` Junio C Hamano 2018-11-07 11:19 ` Johannes Schindelin 2018-11-07 17:42 ` Ramsay Jones 2018-11-08 0:16 ` Junio C Hamano 2018-11-08 13:04 ` Johannes Schindelin 2018-11-08 14:43 ` Junio C Hamano 2018-11-08 15:39 ` Johannes Schindelin 2018-11-09 2:05 ` Joseph Moisan 2018-11-09 10:21 ` Jeff King 2018-11-06 18:24 ` Duy Nguyen 2018-11-07 11:19 ` Johannes Schindelin 2018-11-06 21:32 ` Johannes Sixt 2018-11-07 11:23 ` Johannes Schindelin 2018-11-07 18:52 ` Johannes Sixt 2018-11-07 20:41 ` Jeff King 2018-11-07 21:36 ` Johannes Sixt 2018-11-07 22:03 ` Jeff King 2018-11-08 0:30 ` Junio C Hamano 2018-11-08 1:18 ` Jeff King 2018-11-08 3:26 ` Junio C Hamano 2018-11-08 13:11 ` Johannes Schindelin 2018-11-08 14:25 ` Duy Nguyen 2018-11-08 15:45 ` Johannes Schindelin 2018-11-08 17:40 ` Eric Sunshine 2018-11-09 10:19 ` Jeff King 2018-11-09 16:16 ` Duy Nguyen 2018-11-12 3:03 ` Junio C Hamano 2018-11-08 14:47 ` Junio C Hamano 2018-11-08 15:46 ` Johannes Schindelin 2021-07-01 16:03 ` [PATCH v2 0/2] " Johannes Schindelin via GitGitGadget 2021-07-01 16:03 ` [PATCH v2 1/2] tests: exercise the RUNTIME_PREFIX feature Johannes Schindelin via GitGitGadget 2021-07-01 16:03 ` [PATCH v2 2/2] expand_user_path(): support specifying paths relative to the runtime prefix Johannes Schindelin via GitGitGadget 2021-07-01 17:42 ` [PATCH v2 0/2] mingw: handle absolute paths in expand_user_path() Junio C Hamano 2021-07-24 22:06 ` [PATCH v3 0/5] " Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 1/5] tests: exercise the RUNTIME_PREFIX feature Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 2/5] expand_user_path(): remove stale part of the comment Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 3/5] expand_user_path(): clarify the role of the `real_home` parameter Johannes Schindelin via GitGitGadget 2021-07-24 22:06 ` [PATCH v3 4/5] Use a better name for the function interpolating paths Johannes Schindelin via GitGitGadget 2021-07-26 22:05 ` Junio C Hamano 2021-07-27 7:57 ` Fabian Stelzer 2021-07-27 22:56 ` Junio C Hamano 2021-07-28 0:14 ` Junio C Hamano 2021-07-28 0:39 ` Junio C Hamano 2021-07-28 5:43 ` Junio C Hamano 2021-07-28 8:18 ` Fabian Stelzer 2021-07-28 17:08 ` Junio C Hamano 2021-07-24 22:06 ` [PATCH v3 5/5] interpolate_path(): allow specifying paths relative to the runtime prefix Johannes Schindelin via GitGitGadget 2021-07-26 17:56 ` [PATCH v3 0/5] mingw: handle absolute paths in expand_user_path() Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.