{"thread":{"id":"47221","subject":"[PATCH 0/1] RUNTIME_PREFIX on POSIX systems.","startedAt":"2017-11-16T17:06:15Z","lastAt":"2017-11-17T05:09:04Z","messageCount":3,"participants":["Dan Jacques","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"332675","messageId":"20171116170523.28696-1-dnj@google.com","threadId":"47221","inReplyTo":null,"subject":"[PATCH 0/1] RUNTIME_PREFIX on POSIX systems.","fromName":"Dan Jacques","fromEmail":"dnj@google.com","sentAt":"2017-11-16T17:05:22Z","receivedAt":"2017-11-16T17:06:15Z","isPatch":true,"sender":{"key":"dnj@google.com","avatar":"https://avatars.githubusercontent.com/u/9965104?v=4"},"body":"Hello! This would be my first contribution to the Git project, so please\naccept my apology in advance for any mistakes and let me know what I can\ndo better.\n\nThis patch expands support for the RUNTIME_PREFIX configuration flag,\ncurrently only used on Windows builds, to include Linux, Darwin, and\nFreeBSD. When Git is built with RUNTIME_PREFIX enabled, it resolves its\nancillary paths relative to the runtime location of its executable\nrather than hard-coding them at compile-time, allowing a Git\ninstallation to be deployed to a path other than the one in which it\nwas installed.\n\nIt was useful to create Git distribution bundles that could unpack\nfully-functional Git deployments to arbitrary locations in support of the\nChromium project. Chromium has been using Git bundles built with a patch\nsimilar to this one on its Linux and Mac continuous integration fleet (plus\nsome developer systems) for almost a year now.\n\nRUNTIME_PREFIX remains an optional configuration flag, so standard Git\nbuilds will not see any changes. However, with this patch applied,\nLinux, Darwin, and FreeBSD users can now optionally use \"config.mak\" to\nenable RUNTIME_PREFIX and build relocatable Git distributions. An\nexample \"config.mak\" that builds relocatable Git binaries for Linux/Mac\nis:\n\n# BEGIN: config.mak\nRUNTIME_PREFIX = YesPlease\ngitexecdir = libexec/git-core\ntemplate_dir = share/git-core/templates\nsysconfdir = etc\n# END: config.mak\n\nImplementation notes:\n\nIt is unfortunately not straightforward to resolve the full absolute path\nof the currently-running binary. On some operating systems, notably\nWindows, this path is executively supplied as argv[0]. On other\noperating systems, however, argv[0] is supplied by the invoker (shell,\nscript, kernel, etc.), and is not a reliable source of information about\nthe running Git binary.\n\nThe specific method that this patch employs for binary directory resolution\nvaries depending on the operating system. On Linux and FreeBSD,\nGit resolves \"/proc/self/exe\" and \"/proc/curproc/file\" respectively. On\nDarwin, Git uses the \"_NSGetExecutablePath\" function. On all operating\nsystems, notably Windows, Git continues to fall back to resolution\nagainst argv[0] when it is an absolute path.\n\nWhen RUNTIME_PREFIX is enabled, the resolved runtime path needs to be\npassed to ancillary Git tools for their own resolution requirements:\n\n- C-source Git programs will use the EXEC_PATH_ENVIRONMENT environment\n  variable that Git already exports, ensuring that any launched tools use\n  the same runtime prefix as the entry point.\n\n- PERL tooling needs to know how to locate Git's support libraries. When\n  RUNTIME_PREFIX is configured, Git now exports the GITPERLLIB environment\n  variable, a mechanism that Git's PERL tooling supports that appears to be\n  built for testing. PERL scripts installed using MakeMaker incorporate the\n  builder system's PERL version into their installation path, making\n  it inconsistent to hard-code; consequently, this patch opts to disable\n  MakeMaker for RUNTIME_PREFIX builds in order to deterministically control\n  the destination of Git's support libraries.\n\n- Git also exports the GIT_TEXTDOMAINDIR environment variable when\n  RUNTIME_PREFIX is set so that its locale configuration can be leveraged\n  by Git tooling gettext().\n\nPlease note that this patch affects Windows Git builds, since the Windows\nGit project uses RUNTIME_PREFIX to support arbitrary installation paths.\nNotably, PERL scripts are now always installed without MakeMaker (if they\nweren't before), and EXEC_PATH_ENVIRONMENT is preferred by tools instead of\nre-resolving argv[0]. Chromium uses the stock redistributable Windows Git\npackage, so I haven't had an opportunity to test this patch on that\nplatform.\n\nPlease take a look and let me know what you think. Thanks!\n-Dan\n\nDan Jacques (1):\n  exec_cmd: RUNTIME_PREFIX on some POSIX systems\n\n Makefile               |  29 ++++++--\n builtin/receive-pack.c |   2 +-\n cache.h                |   2 +\n common-main.c          |   4 +-\n config.mak.uname       |   4 ++\n exec_cmd.c             | 189 +++++++++++++++++++++++++++++++++++++++++++------\n exec_cmd.h             |   6 +-\n gettext.c              |   7 +-\n git.c                  |   7 +-\n http-backend.c         |   2 +-\n shell.c                |   2 +-\n upload-pack.c          |   2 +-\n 12 files changed, 217 insertions(+), 39 deletions(-)\n\n-- \n2.15.0.448.gf294e3d99a-goog\n\n"},{"id":"332676","messageId":"20171116170523.28696-2-dnj@google.com","threadId":"47221","inReplyTo":"20171116170523.28696-1-dnj@google.com","subject":"[PATCH 1/1] exec_cmd: RUNTIME_PREFIX on some POSIX systems","fromName":"Dan Jacques","fromEmail":"dnj@google.com","sentAt":"2017-11-16T17:05:23Z","receivedAt":"2017-11-16T17:06:24Z","isPatch":true,"sender":{"key":"dnj@google.com","avatar":"https://avatars.githubusercontent.com/u/9965104?v=4"},"body":"Enable Git to resolve its own binary location using a variety of\nOS-specific and generic methods, including:\n\n- procfs via \"/proc/self/exe\" (Linux)\n- _NSGetExecutablePath (Darwin)\n- argv0, if absolute (all, including Windows).\n\nThis is used to enable RUNTIME_PREFIX support for non-Windows systems,\nnotably Linux and Darwin. When configured with RUNTIME_PREFIX, Git will\ndo a best-effort resolution of its executable path and automatically use\nthis as its \"exec_path\" for relative helper and data lookups, unless\nexplicitly overridden.\n\nGit will also always export and consume its resolved \"exec_path\" using\nthe EXEC_PATH_ENVIRONMENT regardless of whether the user has overridden\nit, simplifying future lookups and ensuring consistency in Git tooling\nexecution.\n\nWhen building with a runtime prefix, Git's PERL libraries are now\ninstalled to a consistently-named directory. This path is resolved and\nexported to Git's delegate PERL invocations using the GITPERLLIB\nenvironment variable. This enables Git's delegate PERL scripts to import\nGit's own PERL libraries from a path relative to the executable.\n\nSmall incidental formatting cleanup of \"exec_cmd.c\".\n\nSigned-off-by: Dan Jacques <dnj@google.com>\n---\n Makefile               |  29 ++++++--\n builtin/receive-pack.c |   2 +-\n cache.h                |   2 +\n common-main.c          |   4 +-\n config.mak.uname       |   4 ++\n exec_cmd.c             | 189 +++++++++++++++++++++++++++++++++++++++++++------\n exec_cmd.h             |   6 +-\n gettext.c              |   7 +-\n git.c                  |   7 +-\n http-backend.c         |   2 +-\n shell.c                |   2 +-\n upload-pack.c          |   2 +-\n 12 files changed, 217 insertions(+), 39 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex ee9d5eb11..80db01706 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -296,7 +296,8 @@ all::\n # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n #\n # Define NO_PERL_MAKEMAKER if you cannot use Makefiles generated by perl's\n-# MakeMaker (e.g. using ActiveState under Cygwin).\n+# MakeMaker (e.g. using ActiveState under Cygwin, or building with a fixed\n+# runtime prefix).\n #\n # Define NO_PERL if you do not want Perl scripts or libraries at all.\n #\n@@ -462,6 +463,7 @@ ARFLAGS = rcs\n #   mandir\n #   infodir\n #   htmldir\n+#   localedir\n # This can help installing the suite in a relocatable way.\n \n prefix = $(HOME)\n@@ -485,6 +487,7 @@ pathsep = :\n mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))\n infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))\n htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))\n+localedir_relative = $(patsubst $(prefix)/%,%,$(localedir))\n \n export prefix bindir sharedir sysconfdir gitwebdir localedir\n \n@@ -1522,9 +1525,6 @@ ifdef SHA1_MAX_BLOCK_SIZE\n \tLIB_OBJS += compat/sha1-chunked.o\n \tBASIC_CFLAGS += -DSHA1_MAX_BLOCK_SIZE=\"$(SHA1_MAX_BLOCK_SIZE)\"\n endif\n-ifdef NO_PERL_MAKEMAKER\n-\texport NO_PERL_MAKEMAKER\n-endif\n ifdef NO_HSTRERROR\n \tCOMPAT_CFLAGS += -DNO_HSTRERROR\n \tCOMPAT_OBJS += compat/hstrerror.o\n@@ -1547,6 +1547,14 @@ else\n endif\n ifdef RUNTIME_PREFIX\n \tCOMPAT_CFLAGS += -DRUNTIME_PREFIX\n+\n+\t# Control PERL library location so it can be referenced by relocatable\n+\t# code.\n+\tNO_PERL_MAKEMAKER = YesPlease\n+endif\n+\n+ifdef NO_PERL_MAKEMAKER\n+\texport NO_PERL_MAKEMAKER\n endif\n \n ifdef NO_PTHREADS\n@@ -1632,6 +1640,15 @@ ifdef HAVE_GETDELIM\n \tBASIC_CFLAGS += -DHAVE_GETDELIM\n endif\n \n+ifneq ($(PROCFS_EXECUTABLE_PATH),)\n+\tprocfs_executable_path_SQ = $(subst ','\\'',$(PROCFS_EXECUTABLE_PATH))\n+\tBASIC_CFLAGS += '-DPROCFS_EXECUTABLE_PATH=\"$(procfs_executable_path_SQ)\"'\n+endif\n+\n+ifdef HAVE_NS_GET_EXECUTABLE_PATH\n+\tBASIC_CFLAGS += -DHAVE_NS_GET_EXECUTABLE_PATH\n+endif\n+\n ifeq ($(TCLTK_PATH),)\n NO_TCLTK = NoThanks\n endif\n@@ -1714,6 +1731,7 @@ bindir_relative_SQ = $(subst ','\\'',$(bindir_relative))\n mandir_relative_SQ = $(subst ','\\'',$(mandir_relative))\n infodir_relative_SQ = $(subst ','\\'',$(infodir_relative))\n localedir_SQ = $(subst ','\\'',$(localedir))\n+localedir_relative_SQ = $(subst ','\\'',$(localedir_relative))\n gitexecdir_SQ = $(subst ','\\'',$(gitexecdir))\n template_dir_SQ = $(subst ','\\'',$(template_dir))\n htmldir_relative_SQ = $(subst ','\\'',$(htmldir_relative))\n@@ -2130,6 +2148,7 @@ endif\n exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX\n exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \\\n \t'-DGIT_EXEC_PATH=\"$(gitexecdir_SQ)\"' \\\n+\t'-DGIT_LOCALE_PATH=\"$(localedir_relative_SQ)\"' \\\n \t'-DBINDIR=\"$(bindir_relative_SQ)\"' \\\n \t'-DPREFIX=\"$(prefix_SQ)\"'\n \n@@ -2147,7 +2166,7 @@ attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \\\n \n gettext.sp gettext.s gettext.o: GIT-PREFIX\n gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \\\n-\t-DGIT_LOCALE_PATH='\"$(localedir_SQ)\"'\n+\t-DGIT_LOCALE_PATH='\"$(localedir_relative_SQ)\"'\n \n http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \\\n \t-DCURL_DISABLE_TYPECHECK\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 4d37a160d..51203cc3d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1947,7 +1947,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)\n \n \tservice_dir = argv[0];\n \n-\tsetup_path();\n+\tsetup_path_and_env();\n \n \tif (!enter_repo(service_dir, 0))\n \t\tdie(\"'%s' does not appear to be a git repository\", service_dir);\ndiff --git a/cache.h b/cache.h\nindex cb7fb7c00..9ef59f1cc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -445,6 +445,8 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_ICASE_PATHSPECS_ENVIRONMENT \"GIT_ICASE_PATHSPECS\"\n #define GIT_QUARANTINE_ENVIRONMENT \"GIT_QUARANTINE_PATH\"\n #define GIT_OPTIONAL_LOCKS_ENVIRONMENT \"GIT_OPTIONAL_LOCKS\"\n+#define GIT_PERL_LIB_ENVIRONMENT \"GITPERLLIB\"\n+#define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT \"GIT_TEXTDOMAINDIR\"\n \n /*\n  * This environment variable is expected to contain a boolean indicating\ndiff --git a/common-main.c b/common-main.c\nindex 6a689007e..6516a1f89 100644\n--- a/common-main.c\n+++ b/common-main.c\n@@ -32,12 +32,12 @@ int main(int argc, const char **argv)\n \t */\n \tsanitize_stdfds();\n \n+\tgit_resolve_executable_dir(argv[0]);\n+\n \tgit_setup_gettext();\n \n \tattr_start();\n \n-\tgit_extract_argv0_path(argv[0]);\n-\n \trestore_sigpipe_to_default();\n \n \treturn cmd_main(argc, argv);\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 685a80d13..bd5c326d4 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -37,6 +37,7 @@ ifeq ($(uname_S),Linux)\n \tHAVE_GETDELIM = YesPlease\n \tSANE_TEXT_GREP=-a\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n+\tPROCFS_EXECUTABLE_PATH = /proc/self/exe\n endif\n ifeq ($(uname_S),GNU/kFreeBSD)\n \tHAVE_ALLOCA_H = YesPlease\n@@ -111,6 +112,7 @@ ifeq ($(uname_S),Darwin)\n \tBASIC_CFLAGS += -DPROTECT_HFS_DEFAULT=1\n \tHAVE_BSD_SYSCTL = YesPlease\n \tFREAD_READS_DIRECTORIES = UnfortunatelyYes\n+\tHAVE_NS_GET_EXECUTABLE_PATH = YesPlease\n endif\n ifeq ($(uname_S),SunOS)\n \tNEEDS_SOCKET = YesPlease\n@@ -218,6 +220,7 @@ ifeq ($(uname_S),OpenBSD)\n \tBASIC_LDFLAGS += -L/usr/local/lib\n \tHAVE_PATHS_H = YesPlease\n \tHAVE_BSD_SYSCTL = YesPlease\n+\tPROCFS_EXECUTABLE_PATH = /proc/curproc/file\n endif\n ifeq ($(uname_S),MirBSD)\n \tNO_STRCASESTR = YesPlease\n@@ -236,6 +239,7 @@ ifeq ($(uname_S),NetBSD)\n \tUSE_ST_TIMESPEC = YesPlease\n \tHAVE_PATHS_H = YesPlease\n \tHAVE_BSD_SYSCTL = YesPlease\n+\tPROCFS_EXECUTABLE_PATH = /proc/curproc/exe\n endif\n ifeq ($(uname_S),AIX)\n \tDEFAULT_PAGER = more\ndiff --git a/exec_cmd.c b/exec_cmd.c\nindex ce192a2d6..10ca7a75a 100644\n--- a/exec_cmd.c\n+++ b/exec_cmd.c\n@@ -2,53 +2,187 @@\n #include \"exec_cmd.h\"\n #include \"quote.h\"\n #include \"argv-array.h\"\n+\n+#if defined(RUNTIME_PREFIX) && defined(HAVE_NS_GET_EXECUTABLE_PATH)\n+#include <mach-o/dyld.h>\n+#endif\n+\n #define MAX_ARGS\t32\n \n static const char *argv_exec_path;\n \n+static const char *system_prefix(void);\n+\n #ifdef RUNTIME_PREFIX\n-static const char *argv0_path;\n+\n+/**\n+ * When using a runtime prefix, Git dynamically resolves paths relative to its\n+ * executable.\n+ *\n+ * The method for determining the path of the executable is highly\n+ * platform-specific.\n+ */\n+\n+/**\n+ * Path to the current Git executable. Resolved on startup by\n+ * 'git_resolve_executable_dir'.\n+ */\n+static const char *executable_dirname;\n \n static const char *system_prefix(void)\n {\n \tstatic const char *prefix;\n \n-\tassert(argv0_path);\n-\tassert(is_absolute_path(argv0_path));\n+\tassert(executable_dirname);\n+\tassert(is_absolute_path(executable_dirname));\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+\t    !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&\n+\t    !(prefix = strip_path_suffix(executable_dirname, BINDIR)) &&\n+\t    !(prefix = strip_path_suffix(executable_dirname, \"git\"))) {\n \t\tprefix = PREFIX;\n \t\ttrace_printf(\"RUNTIME_PREFIX requested, \"\n-\t\t\t\t\"but prefix computation failed.  \"\n-\t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n+\t\t\t     \"but prefix computation failed.  \"\n+\t\t\t     \"Using static fallback '%s'.\\n\",\n+\t\t\t     prefix);\n \t}\n \treturn prefix;\n }\n \n-void git_extract_argv0_path(const char *argv0)\n+/*\n+ * Resolves the executable path from argv[0], only if it is absolute.\n+ *\n+ * Returns 0 on success, -1 on failure.\n+ */\n+static int git_get_exec_path_from_argv0(struct strbuf *buf, const char *argv0)\n {\n \tconst char *slash;\n \n \tif (!argv0 || !*argv0)\n-\t\treturn;\n+\t\treturn -1;\n \n \tslash = find_last_dir_sep(argv0);\n+\tif (slash) {\n+\t\ttrace_printf(\"Determined executable path from argv0: %s\\n\",\n+\t\t\t     argv0);\n+\t\tstrbuf_add_absolute_path(buf, argv0);\n+\t\treturn 0;\n+\t}\n+\treturn -1;\n+}\n+\n+#ifdef PROCFS_EXECUTABLE_PATH\n+/*\n+ * Resolves the executable path by examining a procfs symlink.\n+ *\n+ * Returns 0 on success, -1 on failure.\n+ */\n+static int git_get_exec_path_procfs(struct strbuf *buf)\n+{\n+\tchar *path = realpath(PROCFS_EXECUTABLE_PATH, NULL);\n+\tif (path) {\n+\t\ttrace_printf(\"Determined executable path from procfs: %s\\n\",\n+\t\t\t     path);\n+\t\tstrbuf_addstr(buf, path);\n+\t\tfree(path);\n+\t\treturn 0;\n+\t}\n+\treturn -1;\n+}\n+#endif /* PROCFS_EXECUTABLE_PATH */\n+\n+#ifdef HAVE_NS_GET_EXECUTABLE_PATH\n+/*\n+ * Resolves the executable path by querying Darwin applicaton stack.\n+ *\n+ * Returns 0 on success, -1 on failure.\n+ */\n+static int git_get_exec_path_darwin(struct strbuf *buf)\n+{\n+\tchar path[PATH_MAX];\n+\tuint32_t size = sizeof(path);\n+\tif (!_NSGetExecutablePath(path, &size)) {\n+\t\ttrace_printf(\n+\t\t\t\"Determined executable path from Darwin stack: %s\\n\",\n+\t\t\tpath);\n+\t\tstrbuf_addstr(buf, path);\n+\t\treturn 0;\n+\t}\n+\treturn -1;\n+}\n+#endif /* HAVE_NS_GET_EXECUTABLE_PATH */\n+\n+/*\n+ * Resolves the absolute path of the current executable by employing\n+ * one or more platform-specific methods.\n+ *\n+ * Returns 0 on success, -1 on failure.\n+ */\n+static int git_get_exec_path(struct strbuf *buf, const char *argv0)\n+{\n+\tif (\n+#ifdef PROCFS_EXECUTABLE_PATH\n+\t\tgit_get_exec_path_procfs(buf) &&\n+#endif /* PROCFS_EXECUTABLE_PATH */\n+#ifdef HAVE_NS_GET_EXECUTABLE_PATH\n+\t\tgit_get_exec_path_darwin(buf) &&\n+#endif /* HAVE_NS_GET_EXECUTABLE_PATH */\n+\t\tgit_get_exec_path_from_argv0(buf, argv0)) {\n+\t\treturn -1;\n+\t}\n+\n+\tif (strbuf_normalize_path(buf)) {\n+\t\ttrace_printf(\"Could not normalize path: %s\\n\", buf->buf);\n+\t\treturn -1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+void git_resolve_executable_dir(const char *argv0)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tchar *resolved;\n+\tconst char *slash;\n+\tconst char *path;\n+\n+\tpath = getenv(EXEC_PATH_ENVIRONMENT);\n+\tif (path) {\n+\t\ttrace_printf(\"Determined executable path from ENV: %s\\n\", path);\n+\t\texecutable_dirname = path;\n+\t} else {\n+\t\tif (git_get_exec_path(&buf, argv0)) {\n+\t\t\ttrace_printf(\n+\t\t\t\t\"Could not determine executable path from: %s\\n\",\n+\t\t\t\targv0);\n+\t\t\tstrbuf_release(&buf);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tresolved = strbuf_detach(&buf, NULL);\n+\t\tslash = find_last_dir_sep(resolved);\n+\t\tif (slash)\n+\t\t\tresolved[slash - resolved] = '\\0';\n+\n+\t\texecutable_dirname = resolved;\n+\t}\n \n-\tif (slash)\n-\t\targv0_path = xstrndup(argv0, slash - argv0);\n+\ttrace_printf(\"Determined executable dir: %s\\n\", executable_dirname);\n }\n \n #else\n \n+/**\n+ * When not using a runtime prefix, Git uses a hard-coded path, and there is\n+ * nothing to resolve.\n+ */\n+\n static const char *system_prefix(void)\n {\n \treturn PREFIX;\n }\n \n-void git_extract_argv0_path(const char *argv0)\n+void git_resolve_executable_dir(const char *argv0)\n {\n }\n \n@@ -65,7 +199,7 @@ char *system_path(const char *path)\n \treturn strbuf_detach(&d, NULL);\n }\n \n-void git_set_argv_exec_path(const char *exec_path)\n+void git_set_exec_path(const char *exec_path)\n {\n \targv_exec_path = exec_path;\n \t/*\n@@ -74,7 +208,6 @@ void git_set_argv_exec_path(const char *exec_path)\n \tsetenv(EXEC_PATH_ENVIRONMENT, exec_path, 1);\n }\n \n-\n /* Returns the highest-priority, location to look for git programs. */\n const char *git_exec_path(void)\n {\n@@ -101,12 +234,14 @@ static void add_path(struct strbuf *out, const char *path)\n \t}\n }\n \n-void setup_path(void)\n+void setup_path_and_env(void)\n {\n+\tconst char *exec_path = git_exec_path();\n \tconst char *old_path = getenv(\"PATH\");\n \tstruct strbuf new_path = STRBUF_INIT;\n \n-\tadd_path(&new_path, git_exec_path());\n+\tgit_set_exec_path(exec_path);\n+\tadd_path(&new_path, exec_path);\n \n \tif (old_path)\n \t\tstrbuf_addstr(&new_path, old_path);\n@@ -116,6 +251,20 @@ void setup_path(void)\n \tsetenv(\"PATH\", new_path.buf, 1);\n \n \tstrbuf_release(&new_path);\n+\n+\t/*\n+\t * If RUNTIME_PREFIX is set, the relative PERL library and locale paths\n+\t * must be exported for invoked programs to inherit the calculated\n+\t * runtime path.\n+\t *\n+\t * When RUNTIME_PREFIX is defined, Git's PERL include path is always\n+\t * \"$(PREFIX)/lib\" (by setting NO_PERL_MAKEMAKER).\n+\t */\n+#ifdef RUNTIME_PREFIX\n+\tsetenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT, system_path(GIT_LOCALE_PATH),\n+\t       1);\n+\tsetenv(GIT_PERL_LIB_ENVIRONMENT, system_path(\"lib\"), 1);\n+#endif /* RUNTIME_PREFIX */\n }\n \n const char **prepare_git_cmd(struct argv_array *out, const char **argv)\n@@ -125,7 +274,8 @@ const char **prepare_git_cmd(struct argv_array *out, const char **argv)\n \treturn out->argv;\n }\n \n-int execv_git_cmd(const char **argv) {\n+int execv_git_cmd(const char **argv)\n+{\n \tstruct argv_array nargv = ARGV_ARRAY_INIT;\n \n \tprepare_git_cmd(&nargv, argv);\n@@ -140,8 +290,7 @@ int execv_git_cmd(const char **argv) {\n \treturn -1;\n }\n \n-\n-int execl_git_cmd(const char *cmd,...)\n+int execl_git_cmd(const char *cmd, ...)\n {\n \tint argc;\n \tconst char *argv[MAX_ARGS + 1];\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex ff0b48048..490493745 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -3,10 +3,10 @@\n \n struct argv_array;\n \n-extern void git_set_argv_exec_path(const char *exec_path);\n-extern void git_extract_argv0_path(const char *path);\n+extern void git_set_exec_path(const char *exec_path);\n+extern void git_resolve_executable_dir(const char *path);\n extern const char *git_exec_path(void);\n-extern void setup_path(void);\n+extern void setup_path_and_env(void);\n extern const char **prepare_git_cmd(struct argv_array *out, const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n LAST_ARG_MUST_BE_NULL\ndiff --git a/gettext.c b/gettext.c\nindex db727ea02..f1c162cd8 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -2,7 +2,8 @@\n  * Copyright (c) 2010 Ævar Arnfjörð Bjarmason\n  */\n \n-#include \"git-compat-util.h\"\n+#include \"cache.h\"\n+#include \"exec_cmd.h\"\n #include \"gettext.h\"\n #include \"strbuf.h\"\n #include \"utf8.h\"\n@@ -157,10 +158,10 @@ static void init_gettext_charset(const char *domain)\n \n void git_setup_gettext(void)\n {\n-\tconst char *podir = getenv(\"GIT_TEXTDOMAINDIR\");\n+\tconst char *podir = getenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT);\n \n \tif (!podir)\n-\t\tpodir = GIT_LOCALE_PATH;\n+\t\tpodir = system_path(GIT_LOCALE_PATH);\n \tbindtextdomain(\"git\", podir);\n \tsetlocale(LC_MESSAGES, \"\");\n \tsetlocale(LC_TIME, \"\");\ndiff --git a/git.c b/git.c\nindex 9e96dd409..f6336bca0 100644\n--- a/git.c\n+++ b/git.c\n@@ -65,7 +65,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t */\n \t\tif (skip_prefix(cmd, \"--exec-path\", &cmd)) {\n \t\t\tif (*cmd == '=')\n-\t\t\t\tgit_set_argv_exec_path(cmd + 1);\n+\t\t\t\tgit_set_exec_path(cmd + 1);\n \t\t\telse {\n \t\t\t\tputs(git_exec_path());\n \t\t\t\texit(0);\n@@ -676,8 +676,11 @@ int cmd_main(int argc, const char **argv)\n \t * precedence paths: the \"--exec-path\" option, the GIT_EXEC_PATH\n \t * environment, and the $(gitexecdir) from the Makefile at build\n \t * time.\n+\t *\n+\t * If RUNTIME_PREFIX is defined, we export some additional environment\n+\t * variables to help subprocess code identify runtime-derived paths.\n \t */\n-\tsetup_path();\n+\tsetup_path_and_env();\n \n \twhile (1) {\n \t\tint was_alias = run_argv(&argc, &argv);\ndiff --git a/http-backend.c b/http-backend.c\nindex f3dc218b2..200d4dc13 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -697,7 +697,7 @@ int cmd_main(int argc, const char **argv)\n \tif (!cmd)\n \t\tnot_found(&hdr, \"Request not supported: '%s'\", dir);\n \n-\tsetup_path();\n+\tsetup_path_and_env();\n \tif (!enter_repo(dir, 0))\n \t\tnot_found(&hdr, \"Not a git repository: '%s'\", dir);\n \tif (!getenv(\"GIT_HTTP_EXPORT_ALL\") &&\ndiff --git a/shell.c b/shell.c\nindex 234b2d4f1..82177f5bc 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -12,7 +12,7 @@ static int do_generic_cmd(const char *me, char *arg)\n {\n \tconst char *my_argv[4];\n \n-\tsetup_path();\n+\tsetup_path_and_env();\n \tif (!arg || !(arg = sq_dequote(arg)) || *arg == '-')\n \t\tdie(\"bad argument\");\n \tif (!starts_with(me, \"git-\"))\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 6d5f3c0d3..aa3d693ab 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1058,7 +1058,7 @@ int cmd_main(int argc, const char **argv)\n \tif (timeout)\n \t\tdaemon_mode = 1;\n \n-\tsetup_path();\n+\tsetup_path_and_env();\n \n \tdir = argv[0];\n \n-- \n2.15.0.448.gf294e3d99a-goog\n\n"},{"id":"332741","messageId":"xmqqh8ttsd94.fsf@gitster.mtv.corp.google.com","threadId":"47221","inReplyTo":"20171116170523.28696-2-dnj@google.com","subject":"Re: [PATCH 1/1] exec_cmd: RUNTIME_PREFIX on some POSIX systems","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-17T05:08:55Z","receivedAt":"2017-11-17T05:09:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dan Jacques <dnj@google.com> writes:\n\n> Enable Git to resolve its own binary location using a variety of\n> OS-specific and generic methods, including:\n>\n> - procfs via \"/proc/self/exe\" (Linux)\n> - _NSGetExecutablePath (Darwin)\n> - argv0, if absolute (all, including Windows).\n>\n> This is used to enable RUNTIME_PREFIX support for non-Windows systems,\n> notably Linux and Darwin. When configured with RUNTIME_PREFIX, Git will\n> do a best-effort resolution of its executable path and automatically use\n> this as its \"exec_path\" for relative helper and data lookups, unless\n> explicitly overridden.\n\nYay.\n\n> Git will also always export and consume its resolved \"exec_path\" using\n> the EXEC_PATH_ENVIRONMENT regardless of whether the user has overridden\n> it, simplifying future lookups and ensuring consistency in Git tooling\n> execution.\n\nThe \"regardless of whether the user has overridden it\" part sounded\nalarming and made me wince twice.  I think you meant\n\n - If the user already has GIT_EXEC_PATH in the environment pointing\n   somewhere, when we end up calling git_set_exec_path() to export\n   the variable, the value we have at hand to be exported is what\n   originally came from the user, bypassing the auto-detection logic\n   this patch adds.\n\n - If the user did not have GIT_EXEC_PATH, the auto-detection logic\n   is exercised, and the result is exported when git_set_exec_path()\n   is called.  Any program that is spawned by us as a subprocess\n   will inherit the same GIT_EXEC_PATH we detected.\n\nAs I have multiple installations of various versions of Git, I find\nthe latter somewhat disturbing.  \n\nSuppose that I designate one stable version of Git and install it at\n$HOME/git-stable/{bin,libexec,...}/, and want that version to always\nbe used in my hooks and other helper scripts that are spawned by\nGit, even when I am trying out a newer version of Git that is under\ntesting.\n\nMy hooks would be running $HOME/git-stable/bin/git subcommand\" (or\nmore realistically, it would do \"PATH=$HOME/git-stable/bin:$PATH\"\nupfront), but with this patch (and with runtime-prefix layout) they\nwould use GIT_EXEC_PATH that was discovered and exported by the\nversion of Git under testing that invoked the hook, leading to an\ninconsistent and hard to debug behaviour, no?\n\nOn the other hand, as long as existing GIT_EXEC_PATH is passed-thru\n(i.e. the first point above), I think that is a sensible thing to\ndo.\n\n> When building with a runtime prefix, Git's PERL libraries are now\n> installed to a consistently-named directory. This path is resolved and\n> exported to Git's delegate PERL invocations using the GITPERLLIB\n> environment variable. This enables Git's delegate PERL scripts to import\n> Git's own PERL libraries from a path relative to the executable.\n\nSounds good.\n\n> Small incidental formatting cleanup of \"exec_cmd.c\".\n\nWe usually frown upon these because they often gets distracting, but\nI didn't find the ones in this patch too bad.\n\n> diff --git a/Makefile b/Makefile\n> index ee9d5eb11..80db01706 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -296,7 +296,8 @@ all::\n>  # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).\n>  #\n>  # Define NO_PERL_MAKEMAKER if you cannot use Makefiles generated by perl's\n> -# MakeMaker (e.g. using ActiveState under Cygwin).\n> +# MakeMaker (e.g. using ActiveState under Cygwin, or building with a fixed\n> +# runtime prefix).\n\nWindows folks may want to comment (either positively or negatively)\nas they are the only ones that are currently using runtime-prefix\nlayout, especially on this part:\n\n> @@ -1547,6 +1547,14 @@ else\n>  endif\n>  ifdef RUNTIME_PREFIX\n>  \tCOMPAT_CFLAGS += -DRUNTIME_PREFIX\n> +\n> +\t# Control PERL library location so it can be referenced by relocatable\n> +\t# code.\n> +\tNO_PERL_MAKEMAKER = YesPlease\n> +endif\n\n\n> diff --git a/cache.h b/cache.h\n> index cb7fb7c00..9ef59f1cc 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -445,6 +445,8 @@ static inline enum object_type object_type(unsigned int mode)\n>  #define GIT_ICASE_PATHSPECS_ENVIRONMENT \"GIT_ICASE_PATHSPECS\"\n>  #define GIT_QUARANTINE_ENVIRONMENT \"GIT_QUARANTINE_PATH\"\n>  #define GIT_OPTIONAL_LOCKS_ENVIRONMENT \"GIT_OPTIONAL_LOCKS\"\n> +#define GIT_PERL_LIB_ENVIRONMENT \"GITPERLLIB\"\n> +#define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT \"GIT_TEXTDOMAINDIR\"\n\nThis is a nice touch.  The other day I noticed that we stopped\ndefining these when we start using a new enviornment variable, which\nshould be rectified (this is not within the scope of this patch--I\nam just welcoming this change that fixes the existing issue a bit\nwithout getting distracing).\n\n> diff --git a/common-main.c b/common-main.c\n> index 6a689007e..6516a1f89 100644\n> --- a/common-main.c\n> +++ b/common-main.c\n> @@ -32,12 +32,12 @@ int main(int argc, const char **argv)\n>  \t */\n>  \tsanitize_stdfds();\n>  \n> +\tgit_resolve_executable_dir(argv[0]);\n> +\n>  \tgit_setup_gettext();\n>  \n>  \tattr_start();\n>  \n> -\tgit_extract_argv0_path(argv[0]);\n> -\n\nI presume that this is because we may need to know where to find the\nlocale stuff before calling git_setup_gettext(); makes sense.\n\n> diff --git a/exec_cmd.c b/exec_cmd.c\n> index ce192a2d6..10ca7a75a 100644\n> --- a/exec_cmd.c\n> +++ b/exec_cmd.c\n> @@ -2,53 +2,187 @@\n>  #include \"exec_cmd.h\"\n>  #include \"quote.h\"\n>  #include \"argv-array.h\"\n> +\n> +#if defined(RUNTIME_PREFIX) && defined(HAVE_NS_GET_EXECUTABLE_PATH)\n> +#include <mach-o/dyld.h>\n> +#endif\n> +\n>  #define MAX_ARGS\t32\n>  \n>  static const char *argv_exec_path;\n>  \n> +static const char *system_prefix(void);\n> +\n>  #ifdef RUNTIME_PREFIX\n> -static const char *argv0_path;\n> +\n> +/**\n> + * When using a runtime prefix, Git dynamically resolves paths relative to its\n> + * executable.\n> + *\n> + * The method for determining the path of the executable is highly\n> + * platform-specific.\n> + */\n> +\n> +/**\n> + * Path to the current Git executable. Resolved on startup by\n> + * 'git_resolve_executable_dir'.\n> + */\n> +static const char *executable_dirname;\n\nOK, so that is a more appropriate name for the variable that is a\nlogical successor of argv0_path.  I wonder if the file-scope static\nvariable argv_exec_path we see above would want to move to somewhere\ncloser to one of these \"platform specific methods\", though.\n\n>  static const char *system_prefix(void)\n>  {\n>  \tstatic const char *prefix;\n>  \n> -\tassert(argv0_path);\n> -\tassert(is_absolute_path(argv0_path));\n> +\tassert(executable_dirname);\n> +\tassert(is_absolute_path(executable_dirname));\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> +\t    !(prefix = strip_path_suffix(executable_dirname, GIT_EXEC_PATH)) &&\n> +\t    !(prefix = strip_path_suffix(executable_dirname, BINDIR)) &&\n> +\t    !(prefix = strip_path_suffix(executable_dirname, \"git\"))) {\n>  \t\tprefix = PREFIX;\n>  \t\ttrace_printf(\"RUNTIME_PREFIX requested, \"\n> -\t\t\t\t\"but prefix computation failed.  \"\n> -\t\t\t\t\"Using static fallback '%s'.\\n\", prefix);\n> +\t\t\t     \"but prefix computation failed.  \"\n> +\t\t\t     \"Using static fallback '%s'.\\n\",\n> +\t\t\t     prefix);\n>  \t}\n>  \treturn prefix;\n>  }\n>  \n> -void git_extract_argv0_path(const char *argv0)\n> +/*\n> + * Resolves the executable path from argv[0], only if it is absolute.\n> + *\n> + * Returns 0 on success, -1 on failure.\n> + */\n> +static int git_get_exec_path_from_argv0(struct strbuf *buf, const char *argv0)\n>  {\n>  \tconst char *slash;\n>  \n>  \tif (!argv0 || !*argv0)\n> -\t\treturn;\n> +\t\treturn -1;\n>  \n>  \tslash = find_last_dir_sep(argv0);\n> +\tif (slash) {\n> +\t\ttrace_printf(\"Determined executable path from argv0: %s\\n\",\n> +\t\t\t     argv0);\n> +\t\tstrbuf_add_absolute_path(buf, argv0);\n> +\t\treturn 0;\n> +\t}\n> +\treturn -1;\n> +}\n> +\n> +#ifdef PROCFS_EXECUTABLE_PATH\n> +/*\n> + * Resolves the executable path by examining a procfs symlink.\n> + *\n> + * Returns 0 on success, -1 on failure.\n> + */\n> +static int git_get_exec_path_procfs(struct strbuf *buf)\n> +{\n> +\tchar *path = realpath(PROCFS_EXECUTABLE_PATH, NULL);\n\nI think this is our first use of realpath(), which is XSI.\n\n    http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html\n\nI'd assume it is available on any platform on which we would want to\nuse PROCFS_EXECUTABLE_PATH, so this probably is OK.\n\n> +\tif (path) {\n> +\t\ttrace_printf(\"Determined executable path from procfs: %s\\n\",\n> +\t\t\t     path);\n> +\t\tstrbuf_addstr(buf, path);\n> +\t\tfree(path);\n> +\t\treturn 0;\n> +\t}\n> +\treturn -1;\n> +}\n> +#endif /* PROCFS_EXECUTABLE_PATH */\n\n> +/*\n> + * Resolves the absolute path of the current executable by employing\n> + * one or more platform-specific methods.\n> + *\n> + * Returns 0 on success, -1 on failure.\n> + */\n> +static int git_get_exec_path(struct strbuf *buf, const char *argv0)\n> +{\n> +\tif (\n> +#ifdef PROCFS_EXECUTABLE_PATH\n> +\t\tgit_get_exec_path_procfs(buf) &&\n> +#endif /* PROCFS_EXECUTABLE_PATH */\n> +#ifdef HAVE_NS_GET_EXECUTABLE_PATH\n> +\t\tgit_get_exec_path_darwin(buf) &&\n> +#endif /* HAVE_NS_GET_EXECUTABLE_PATH */\n> +\t\tgit_get_exec_path_from_argv0(buf, argv0)) {\n\nThe contract between this caller and these platform-specific helpers\nis for the callee to return 0 when they succeed, and -1 when they\npunt with \"I dunno\", so the &&-cascade stops after the first one\nthat returns 0.  I wonder why argv0 (i.e. the full-path case) is not\nthe first one to try, though---isn't that one the simplest?\n\nAlso, I wonder if this caller gets simpler to read and understand if\neach of these \"platform specific\" ones are done like so:\n\n\t#ifdef FOO_EXECUTABLE_PATH\n\tstatic int git_get_exec_path_foo(struct strbuf *buf)\n\t{\n\t\t...\n\t}\n\t#else /* FOO_EXECUTABLE_PATH */\n\t#define git_get_exec_path_foo(buf) (-1)\n\t#endif\n\nThen the caller can lose the ifdef/endif that is noisier than the\nactual calls we see above, i.e.\n\n\tif (!git_get_exec_path_from_argv0(buf, argv0) ||\n\t    !git_get_exec_path_procfs(buf) ||\n            !git_get_exec_path_darwin(buf)) {\n\t\t/* at least one of them found what we wanted */\n\t\t... do the success case ...\n\t} else {\n\t\t... oops, we failed to find ...\n\t}\n\n> +void git_resolve_executable_dir(const char *argv0)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tchar *resolved;\n> +\tconst char *slash;\n> +\tconst char *path;\n> +\n> +\tpath = getenv(EXEC_PATH_ENVIRONMENT);\n> +\tif (path) {\n> +\t\ttrace_printf(\"Determined executable path from ENV: %s\\n\", path);\n> +\t\texecutable_dirname = path;\n> +\t} else {\n> +\t\tif (git_get_exec_path(&buf, argv0)) {\n> +\t\t\ttrace_printf(\n> +\t\t\t\t\"Could not determine executable path from: %s\\n\",\n> +\t\t\t\targv0);\n> +\t\t\tstrbuf_release(&buf);\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\tresolved = strbuf_detach(&buf, NULL);\n> +\t\tslash = find_last_dir_sep(resolved);\n> +\t\tif (slash)\n> +\t\t\tresolved[slash - resolved] = '\\0';\n> +\n> +\t\texecutable_dirname = resolved;\n> +\t}\n>  \n> -\tif (slash)\n> -\t\targv0_path = xstrndup(argv0, slash - argv0);\n> +\ttrace_printf(\"Determined executable dir: %s\\n\", executable_dirname);\n>  }\n\nOK.\n\n> -void git_set_argv_exec_path(const char *exec_path)\n> +void git_set_exec_path(const char *exec_path)\n>  {\n>  \targv_exec_path = exec_path;\n>  \t/*\n\nI wonder if we want to clean up this part of the system, in that\ngit_exec_path() is the only user of argv_exec_path which is now a\ngross misnomer (the way you figured out exec_path may not be based\non argv at all).  Once resolve-executable-dir is called and we have\na value in executable_dirname, this argv_exec_path variable feels\nmore or less redundant.  But perhaps that is outside the scope of\nthis patch.  I dunno.\n\n> @@ -101,12 +234,14 @@ static void add_path(struct strbuf *out, const char *path)\n>  \t}\n>  }\n>  \n> -void setup_path(void)\n> +void setup_path_and_env(void)\n>  {\n> +\tconst char *exec_path = git_exec_path();\n>  \tconst char *old_path = getenv(\"PATH\");\n>  \tstruct strbuf new_path = STRBUF_INIT;\n>  \n> -\tadd_path(&new_path, git_exec_path());\n> +\tgit_set_exec_path(exec_path);\n> +\tadd_path(&new_path, exec_path);\n>  \n>  \tif (old_path)\n>  \t\tstrbuf_addstr(&new_path, old_path);\n> @@ -116,6 +251,20 @@ void setup_path(void)\n>  \tsetenv(\"PATH\", new_path.buf, 1);\n>  \n>  \tstrbuf_release(&new_path);\n> +\n> +\t/*\n> +\t * If RUNTIME_PREFIX is set, the relative PERL library and locale paths\n> +\t * must be exported for invoked programs to inherit the calculated\n> +\t * runtime path.\n> +\t *\n> +\t * When RUNTIME_PREFIX is defined, Git's PERL include path is always\n> +\t * \"$(PREFIX)/lib\" (by setting NO_PERL_MAKEMAKER).\n> +\t */\n> +#ifdef RUNTIME_PREFIX\n> +\tsetenv(GIT_TEXT_DOMAIN_DIR_ENVIRONMENT, system_path(GIT_LOCALE_PATH),\n> +\t       1);\n> +\tsetenv(GIT_PERL_LIB_ENVIRONMENT, system_path(\"lib\"), 1);\n> +#endif /* RUNTIME_PREFIX */\n>  }\n\nSo..., now this function exports textdomain and perllib environment\nwhen we are compiled with runtime-prefix, and exports GIT_EXEC_PATH\neven when we are *not* compiled with runtime-prefix (by calling the\ngit_set_exec_path() function that does the exporting)?  That makes\nthe worry I expressed in an earlier part of the message even worse,\nas I assumed this gratuitous exporting would be in effect only when\nruntime-prefix is in use.  I hope I am mistaken...\n\n> @@ -125,7 +274,8 @@ const char **prepare_git_cmd(struct argv_array *out, const char **argv)\n>  \treturn out->argv;\n>  }\n>  \n> -int execv_git_cmd(const char **argv) {\n> +int execv_git_cmd(const char **argv)\n> +{\n>  \tstruct argv_array nargv = ARGV_ARRAY_INIT;\n>  \n>  \tprepare_git_cmd(&nargv, argv);\n> @@ -140,8 +290,7 @@ int execv_git_cmd(const char **argv) {\n>  \treturn -1;\n>  }\n>  \n> -\n> -int execl_git_cmd(const char *cmd,...)\n> +int execl_git_cmd(const char *cmd, ...)\n>  {\n>  \tint argc;\n>  \tconst char *argv[MAX_ARGS + 1];\n\nThese two are good changes (reducing the inter-function spacing\nmight not be in that \"we'd probably prefer there wasn't an extra\nblank line from the beginning, but it is not worth a patch noise to\nremove it\" sense).  Even though they are somewhat distracting,\nseeing them after finishing reading the real changes is much less\nbad than having to see distracting changes in the middle of real\nchanges during a review.\n\nThanks.\n"}]}