{"thread":{"id":"42035","subject":"[PATCH] Extend runtime prefix computation","startedAt":"2016-04-15T14:30:01Z","lastAt":"2016-04-20T17:52:07Z","messageCount":4,"participants":["Michael Weiser","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"283530","messageId":"20160415143001.GA67437@dinsnail.net","threadId":"42035","inReplyTo":null,"subject":"[PATCH] Extend runtime prefix computation","fromName":"Michael Weiser","fromEmail":"michael.weiser@gmx.de","sentAt":"2016-04-15T14:30:01Z","receivedAt":"2016-04-15T14:30:01Z","isPatch":true,"sender":{"key":"michael.weiser@gmx.de","avatar":null},"body":"Make git fully relocatable at runtime extending the runtime prefix\ncalculation. Handle absolute and relative paths in argv0. Handle no path\nat all in argv0 in a system-specific manner.  Replace assertions with\ninitialised variables and checks that lead to fallback to the static\nprefix.\n---\n\nNotes:\n    Tested-by: David Abdurachmanov <David.Abdurachmanov@cern.ch>\n    Pull-Request: https://github.com/git/git/pull/224\n\n exec_cmd.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++------------\n 1 file changed, 58 insertions(+), 14 deletions(-)\n\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex 9d5703a..f0db070 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -3,30 +3,27 @@\n #include \"quote.h\"\n #include \"argv-array.h\"\n #define MAX_ARGS\t32\n+#if defined(__APPLE__)\n+#include <mach-o/dyld.h>\n+#endif\n \n static const char *argv_exec_path;\n static const char *argv0_path;\n \n char *system_path(const char *path)\n {\n-#ifdef RUNTIME_PREFIX\n-\tstatic const char *prefix;\n-#else\n \tstatic const char *prefix = PREFIX;\n-#endif\n \tstruct strbuf d = STRBUF_INIT;\n \n \tif (is_absolute_path(path))\n \t\treturn xstrdup(path);\n \n #ifdef RUNTIME_PREFIX\n-\tassert(argv0_path);\n-\tassert(is_absolute_path(argv0_path));\n-\n-\tif (!prefix &&\n-\t    !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&\n-\t    !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&\n-\t    !(prefix = strip_path_suffix(argv0_path, \"git\"))) {\n+\tif (!argv0_path ||\n+\t    !is_absolute_path(argv0_path) ||\n+\t    (!(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&\n+\t     !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&\n+\t     !(prefix = strip_path_suffix(argv0_path, \"git\")))) {\n \t\tprefix = PREFIX;\n \t\ttrace_printf(\"RUNTIME_PREFIX requested, \"\n \t\t\t\t\"but prefix computation failed.  \"\n@@ -41,6 +38,8 @@ char *system_path(const char *path)\n const char *git_extract_argv0_path(const char *argv0)\n {\n \tconst char *slash;\n+\tchar *to_resolve = NULL;\n+\tconst char *resolved;\n \n \tif (!argv0 || !*argv0)\n \t\treturn NULL;\n@@ -48,11 +47,56 @@ const char *git_extract_argv0_path(const char *argv0)\n \tslash = find_last_dir_sep(argv0);\n \n \tif (slash) {\n-\t\targv0_path = xstrndup(argv0, slash - argv0);\n-\t\treturn slash + 1;\n+\t\t/* ... it's either an absolute path */\n+\t\tif (is_absolute_path(argv0)) {\n+\t\t\targv0_path = xstrndup(argv0, slash - argv0);\n+\t\t\treturn slash + 1;\n+\t\t}\n+\n+\t\t/* ... or a relative path, in which case we have to make it\n+\t\t * absolute first and do the whole thing again */\n+\t\tto_resolve = xstrdup(argv0);\n+\t} else {\n+\t\t/* argv0 is no path at all, just a name. Resolve it into a\n+\t\t * path. Unfortunately, this gets system specific. */\n+#if defined(__linux__)\n+\t\tstruct stat st;\n+\t\tif (!stat(\"/proc/self/exe\", &st))\n+\t\t\tto_resolve = xstrdup(\"/proc/self/exe\");\n+#elif defined(__APPLE__)\n+\t\t/* call with len == 0 queries the necessary buffer size */\n+\t\tuint32_t len = 0;\n+\t\tif(_NSGetExecutablePath(NULL, &len) != 0) {\n+\t\t\tto_resolve = malloc(len);\n+\t\t\tif (to_resolve) {\n+\t\t\t\t/* get the executable path now we have a buffer\n+\t\t\t\t * of proper size */\n+\t\t\t\tif(_NSGetExecutablePath(to_resolve, &len) != 0) {\n+\t\t\t\t\tfree(to_resolve);\n+\t\t\t\t\treturn argv0;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+#endif\n+\n+\t\t/* if to_resolve is still NULL here, something failed above or\n+\t\t * we are on an unsupported system. system_path() will warn\n+\t\t * and fall back to the static prefix */\n+\t\tif (!to_resolve)\n+\t\t\treturn argv0;\n \t}\n \n-\treturn argv0;\n+\t/* resolve path from absolute to canonical */\n+\tresolved = real_path(to_resolve);\n+\tfree(to_resolve);\n+\n+\tslash = find_last_dir_sep(resolved);\n+\tif (!slash)\n+\t\treturn argv0;\n+\n+\targv0_path = xstrndup(resolved, slash - resolved);\n+\tslash = xstrdup(slash + 1);\n+\treturn slash;\n }\n \n void git_set_argv_exec_path(const char *exec_path)\n-- \n2.6.4 (Apple Git-63)\n"},{"id":"283536","messageId":"xmqqfuumddqm.fsf@gitster.mtv.corp.google.com","threadId":"42035","inReplyTo":"20160415143001.GA67437@dinsnail.net","subject":"Re: [PATCH] Extend runtime prefix computation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-15T16:43:29Z","receivedAt":"2016-04-15T16:43:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Weiser <michael.weiser@gmx.de> writes:\n\n> Make git fully relocatable at runtime extending the runtime prefix\n> calculation. Handle absolute and relative paths in argv0. Handle no path\n> at all in argv0 in a system-specific manner.  Replace assertions with\n> initialised variables and checks that lead to fallback to the static\n> prefix.\n\nThat's a dense description of \"what\" without saying much about\n\"why\".  Hint: start by describing what case(s) the current code\nfails to find the correct runtime prefix.  That would give readers a\nbetter understanding of what problem you are trying to solve.\n\nMissing sign-off.\n\n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index 9d5703a..f0db070 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -3,30 +3,27 @@\n>  #include \"quote.h\"\n>  #include \"argv-array.h\"\n>  #define MAX_ARGS\t32\n> +#if defined(__APPLE__)\n> +#include <mach-o/dyld.h>\n> +#endif\n\nWe tend to avoid system specific includes in individual *.c files.\n\nPerhaps implement platform specific bits in compat/?  E.g. each\nplatform specific file in compat/ may implement and export the same\ngit_extract_argv_path() function, perhaps, so that this file does\nnot even need to know what platforms it supports?\n\n>  char *system_path(const char *path)\n>  {\n> -#ifdef RUNTIME_PREFIX\n> -\tstatic const char *prefix;\n> -#else\n>  \tstatic const char *prefix = PREFIX;\n> -#endif\n>  \tstruct strbuf d = STRBUF_INIT;\n>  \n>  \tif (is_absolute_path(path))\n>  \t\treturn xstrdup(path);\n>  \n>  #ifdef RUNTIME_PREFIX\n> -\tassert(argv0_path);\n> -\tassert(is_absolute_path(argv0_path));\n\nAren't these protecting against future and careless change that\nforgets to call extract-argv0-path or make that function return\nsomething that is not an absolute path?\n\nIs it a good idea to drop these checks, and if so why?\n\n> -\tif (!prefix &&\n> -\t    !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&\n> -\t    !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&\n> -\t    !(prefix = strip_path_suffix(argv0_path, \"git\"))) {\n> +\tif (!argv0_path ||\n> +\t    !is_absolute_path(argv0_path) ||\n> +\t    (!(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&\n> +\t     !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&\n> +\t     !(prefix = strip_path_suffix(argv0_path, \"git\")))) {\n>  \t\tprefix = PREFIX;\n>  \t\ttrace_printf(\"RUNTIME_PREFIX requested, \"\n>  \t\t\t\t\"but prefix computation failed.  \"\n> @@ -41,6 +38,8 @@ char *system_path(const char *path)\n>  const char *git_extract_argv0_path(const char *argv0)\n>  {\n>  \tconst char *slash;\n> +\tchar *to_resolve = NULL;\n> +\tconst char *resolved;\n>  \n>  \tif (!argv0 || !*argv0)\n>  \t\treturn NULL;\n> @@ -48,11 +47,56 @@ const char *git_extract_argv0_path(const char *argv0)\n>  \tslash = find_last_dir_sep(argv0);\n>  \n>  \tif (slash) {\n> -\t\targv0_path = xstrndup(argv0, slash - argv0);\n> -\t\treturn slash + 1;\n> +\t\t/* ... it's either an absolute path */\n> +\t\tif (is_absolute_path(argv0)) {\n> +\t\t\targv0_path = xstrndup(argv0, slash - argv0);\n> +\t\t\treturn slash + 1;\n> +\t\t}\n> +\n> +\t\t/* ... or a relative path, in which case we have to make it\n> +\t\t * absolute first and do the whole thing again */\n> +\t\tto_resolve = xstrdup(argv0);\n> +\t} else {\n> +\t\t/* argv0 is no path at all, just a name. Resolve it into a\n> +\t\t * path. Unfortunately, this gets system specific. */\n> +#if defined(__linux__)\n> +\t\tstruct stat st;\n> +\t\tif (!stat(\"/proc/self/exe\", &st))\n> +\t\t\tto_resolve = xstrdup(\"/proc/self/exe\");\n> +#elif defined(__APPLE__)\n> +\t\t/* call with len == 0 queries the necessary buffer size */\n> +\t\tuint32_t len = 0;\n> +\t\tif(_NSGetExecutablePath(NULL, &len) != 0) {\n> +\t\t\tto_resolve = malloc(len);\n> +\t\t\tif (to_resolve) {\n> +\t\t\t\t/* get the executable path now we have a buffer\n> +\t\t\t\t * of proper size */\n> +\t\t\t\tif(_NSGetExecutablePath(to_resolve, &len) != 0) {\n> +\t\t\t\t\tfree(to_resolve);\n> +\t\t\t\t\treturn argv0;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +#endif\n> +\n> +\t\t/* if to_resolve is still NULL here, something failed above or\n> +\t\t * we are on an unsupported system. system_path() will warn\n> +\t\t * and fall back to the static prefix */\n> +\t\tif (!to_resolve)\n> +\t\t\treturn argv0;\n>  \t}\n>  \n> -\treturn argv0;\n> +\t/* resolve path from absolute to canonical */\n> +\tresolved = real_path(to_resolve);\n> +\tfree(to_resolve);\n> +\n> +\tslash = find_last_dir_sep(resolved);\n> +\tif (!slash)\n> +\t\treturn argv0;\n> +\n> +\targv0_path = xstrndup(resolved, slash - resolved);\n> +\tslash = xstrdup(slash + 1);\n> +\treturn slash;\n>  }\n>  \n>  void git_set_argv_exec_path(const char *exec_path)\n"},{"id":"283722","messageId":"alpine.DEB.2.20.1604180918500.2967@virtualbox","threadId":"42035","inReplyTo":"xmqqfuumddqm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Extend runtime prefix computation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-18T07:20:25Z","receivedAt":"2016-04-18T07:20:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 15 Apr 2016, Junio C Hamano wrote:\n\n> Michael Weiser <michael.weiser@gmx.de> writes:\n> \n> > Make git fully relocatable at runtime extending the runtime prefix\n> > calculation. Handle absolute and relative paths in argv0. Handle no path\n> > at all in argv0 in a system-specific manner.  Replace assertions with\n> > initialised variables and checks that lead to fallback to the static\n> > prefix.\n> \n> That's a dense description of \"what\" without saying much about\n> \"why\".  Hint: start by describing what case(s) the current code\n> fails to find the correct runtime prefix.  That would give readers a\n> better understanding of what problem you are trying to solve.\n\nI have to admit that I am really, *really* skeptical. To me, it looks like\nthis patch opens the door very wide to unintended consequences.\n\n> >  #ifdef RUNTIME_PREFIX\n> > -\tassert(argv0_path);\n> > -\tassert(is_absolute_path(argv0_path));\n> \n> Aren't these protecting against future and careless change that\n> forgets to call extract-argv0-path or make that function return\n> something that is not an absolute path?\n\nThis (first) assert() indeed saved me a couple of times from hunting for\nbugs in the wrong place. Let's keep it.\n\nCiao,\nDscho\n"},{"id":"283979","messageId":"20160420175207.GA28640@weiser.dinsnail.net","threadId":"42035","inReplyTo":"alpine.DEB.2.20.1604180918500.2967@virtualbox","subject":"Re: [PATCH] Extend runtime prefix computation","fromName":"Michael Weiser","fromEmail":"michael@weiser.dinsnail.net","sentAt":"2016-04-20T17:52:07Z","receivedAt":"2016-04-20T17:52:07Z","isPatch":true,"sender":{"key":"michael@weiser.dinsnail.net","avatar":null},"body":"Hi,\n\nOn Fri, Apr 15, 2016 at 09:43:29AM -0700, Junio C Hamano wrote:\n\n> We tend to avoid system specific includes in individual *.c files.\n\n> Perhaps implement platform specific bits in compat/?  E.g. each\n> platform specific file in compat/ may implement and export the same\n> git_extract_argv_path() function, perhaps, so that this file does\n> not even need to know what platforms it supports?\n\nOn Mon, Apr 18, 2016 at 09:20:25AM +0200, Johannes Schindelin wrote:\n\n> I have to admit that I am really, *really* skeptical. To me, it looks like\n> this patch opens the door very wide to unintended consequences.\n\nTo make it short: I had received interest from another user for\nrelocatable git installations and an updated patch against current git\n(based on my last post of the code to this list in 2012). This\nconfirmed to me that the use case is still valid. However, I do not see\nmyself in a position to iterate the code through the review and\nimprovement process that starting to unfold now. So if someone sometime\npicks this up and carries on, I'd be happy. Otherwise please disregard.\n-- \nThanks,\nMichael\n"}]}