Re: [PATCH] Extend runtime prefix computation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 15, 2016, 16:43 UTC
- Message-ID
- <xmqqfuumddqm.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160415143001.GA67437@dinsnail.net>
Michael Weiser <michael.weiser@gmx.de> writes:
Show 5 quoted lines
> Make git fully relocatable at runtime extending the runtime prefix > calculation. Handle absolute and relative paths in argv0. Handle no path > at all in argv0 in a system-specific manner. Replace assertions with > initialised variables and checks that lead to fallback to the static > prefix.
That's a dense description of "what" without saying much about "why". Hint: start by describing what case(s) the current code fails to find the correct runtime prefix. That would give readers a better understanding of what problem you are trying to solve.
Missing sign-off.
Show 11 quoted lines
> diff --git a/exec_cmd.c b/exec_cmd.c > index 9d5703a..f0db070 100644 > --- a/exec_cmd.c > +++ b/exec_cmd.c > @@ -3,30 +3,27 @@ > #include "quote.h" > #include "argv-array.h" > #define MAX_ARGS 32 > +#if defined(__APPLE__) > +#include <mach-o/dyld.h> > +#endif
We tend to avoid system specific includes in individual *.c files.
Perhaps implement platform specific bits in compat/? E.g. each platform specific file in compat/ may implement and export the same git_extract_argv_path() function, perhaps, so that this file does not even need to know what platforms it supports?
Show 15 quoted lines
> char *system_path(const char *path)
> {
> -#ifdef RUNTIME_PREFIX
> - static const char *prefix;
> -#else
> static const char *prefix = PREFIX;
> -#endif
> struct strbuf d = STRBUF_INIT;
>
> if (is_absolute_path(path))
> return xstrdup(path);
>
> #ifdef RUNTIME_PREFIX
> - assert(argv0_path);
> - assert(is_absolute_path(argv0_path));Aren't these protecting against future and careless change that forgets to call extract-argv0-path or make that function return something that is not an absolute path?
Is it a good idea to drop these checks, and if so why?
Show 81 quoted lines
> - if (!prefix &&
> - !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&
> - !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&
> - !(prefix = strip_path_suffix(argv0_path, "git"))) {
> + if (!argv0_path ||
> + !is_absolute_path(argv0_path) ||
> + (!(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&
> + !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&
> + !(prefix = strip_path_suffix(argv0_path, "git")))) {
> prefix = PREFIX;
> trace_printf("RUNTIME_PREFIX requested, "
> "but prefix computation failed. "
> @@ -41,6 +38,8 @@ char *system_path(const char *path)
> const char *git_extract_argv0_path(const char *argv0)
> {
> const char *slash;
> + char *to_resolve = NULL;
> + const char *resolved;
>
> if (!argv0 || !*argv0)
> return NULL;
> @@ -48,11 +47,56 @@ const char *git_extract_argv0_path(const char *argv0)
> slash = find_last_dir_sep(argv0);
>
> if (slash) {
> - argv0_path = xstrndup(argv0, slash - argv0);
> - return slash + 1;
> + /* ... it's either an absolute path */
> + if (is_absolute_path(argv0)) {
> + argv0_path = xstrndup(argv0, slash - argv0);
> + return slash + 1;
> + }
> +
> + /* ... or a relative path, in which case we have to make it
> + * absolute first and do the whole thing again */
> + to_resolve = xstrdup(argv0);
> + } else {
> + /* argv0 is no path at all, just a name. Resolve it into a
> + * path. Unfortunately, this gets system specific. */
> +#if defined(__linux__)
> + struct stat st;
> + if (!stat("/proc/self/exe", &st))
> + to_resolve = xstrdup("/proc/self/exe");
> +#elif defined(__APPLE__)
> + /* call with len == 0 queries the necessary buffer size */
> + uint32_t len = 0;
> + if(_NSGetExecutablePath(NULL, &len) != 0) {
> + to_resolve = malloc(len);
> + if (to_resolve) {
> + /* get the executable path now we have a buffer
> + * of proper size */
> + if(_NSGetExecutablePath(to_resolve, &len) != 0) {
> + free(to_resolve);
> + return argv0;
> + }
> + }
> + }
> +#endif
> +
> + /* if to_resolve is still NULL here, something failed above or
> + * we are on an unsupported system. system_path() will warn
> + * and fall back to the static prefix */
> + if (!to_resolve)
> + return argv0;
> }
>
> - return argv0;
> + /* resolve path from absolute to canonical */
> + resolved = real_path(to_resolve);
> + free(to_resolve);
> +
> + slash = find_last_dir_sep(resolved);
> + if (!slash)
> + return argv0;
> +
> + argv0_path = xstrndup(resolved, slash - resolved);
> + slash = xstrdup(slash + 1);
> + return slash;
> }
>
> void git_set_argv_exec_path(const char *exec_path)