From b937ec2bf461ec06bf601e854f694e86060eba59 Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Tue, 22 Sep 2020 22:18:18 +0300 Subject: Use system git rather than bundled one for some operations on Windows --- bdep/ci.cxx | 4 ++ bdep/git.cxx | 134 +++++++++++++++++++++++++++++++++++++++++++----- bdep/git.hxx | 43 ++++++++++++++-- bdep/git.ixx | 6 +++ bdep/git.txx | 79 +++++++++++++++++----------- bdep/project-author.cxx | 1 + bdep/publish.cxx | 30 ++++++++--- bdep/release.cxx | 24 +++++++-- bdep/types.hxx | 1 + 9 files changed, 261 insertions(+), 61 deletions(-) diff --git a/bdep/ci.cxx b/bdep/ci.cxx index 081529a..797725f 100644 --- a/bdep/ci.cxx +++ b/bdep/ci.cxx @@ -81,7 +81,11 @@ namespace bdep << s.branch << "'" << info << "run 'git push --set-upstream' to set"; + // It's unlikely that the branch remote is configured globally, so we + // use the bundled git. + // optional rem (git_line (git_ver, + false /* system */, prj, false /* ignore_error */, "config", diff --git a/bdep/git.cxx b/bdep/git.cxx index 1c672c7..1749efe 100644 --- a/bdep/git.cxx +++ b/bdep/git.cxx @@ -11,38 +11,139 @@ using namespace butl; namespace bdep { - static optional git_ver; + static pair> sys_git; + static optional sys_git_ver; + +// On POSIX the system git is always assumed. +// +#ifndef _WIN32 + static pair>& bun_git (sys_git); + static optional& bun_git_ver (sys_git_ver); +#else + static pair> bun_git; + static optional bun_git_ver; +#endif // Check that git is at least of the specified minimum supported version. // void - git_check_version (const semantic_version& min_ver) + git_check_version (const semantic_version& min_ver, bool system) { // Query and cache git version on the first call. // - if (!git_ver) + optional& gv (system ? sys_git_ver : bun_git_ver); + + if (!gv) { // Make sure that the getline() function call doesn't end up with an // infinite recursion. // - git_ver = semantic_version (); + gv = semantic_version (); - optional s (git_line (*git_ver, - false /* ignore_error */, - "--version")); + optional s ( + git_line (*gv, system, false /* ignore_error */, "--version")); - if (!s || !(git_ver = git_version (*s))) + if (!s || !(gv = git_version (*s))) fail << "unable to obtain git version"; } // Note that we don't expect the min_ver to contain the build component, // that doesn't matter functionality-wise for git. // - if (*git_ver < min_ver) - fail << "unsupported git version " << *git_ver << + if (*gv < min_ver) + fail << "unsupported git version " << *gv << info << "minimum supported version is " << min_ver << endf; } + // Return git process path and the --exec-path option, if it is required for + // the execution. + // + const pair>& + git_search (bool system) + { + tracer trace ("git_search"); + + // On the first call search for the git process path, calculate the + // --exec-path option value required for its execution, and cache them for + // subsequent use. + // + pair>& git (system ? sys_git : bun_git); + + if (git.first.empty ()) + { + // On startup git prepends the PATH environment variable value with the + // computed directory path where its sub-programs are supposedly located + // (--exec-path option, GIT_EXEC_PATH environment variable, etc; see + // cmd_main() in git's git.c for details). + // + // Then, when git needs to run itself or one of its components as a + // child process, it resolves the full executable path searching in + // directories listed in PATH (see locate_in_PATH() in git's + // run-command.c for details). + // + // On Windows we install git and its components into a place where it is + // not expected to be, which results in the wrong path in PATH as set by + // git (for example, c:/build2/libexec/git-core) which in turn may lead + // to running some other git that appears in the PATH variable. To + // prevent this we pass the git's exec directory via the --exec-path + // option explicitly. + // + process_path pp; + +#ifdef _WIN32 + + // Figure out the bundle absolute directory path based on our own + // process path and realize it for comparison. Though, we only need it + // when searching for the system git. + // + dir_path bd; + + if (system) + { + // We should be able to construct/realize our process path so we don't + // handle invalid_path. + // + bd = path (process::path_search (argv0, true /* init */). + effect_string ()).directory ().realize (); + + // Search in PATH first and fallback to the bundle directory. + // + pp = process::path_search ("git", + true /* init */, + bd, + true /* path_only */); + } + else +#endif + pp = process::path_search ("git", true /* init */); + + optional ep; +#ifdef _WIN32 + + // Note that we need to add --exec-path not only if the bundled git is + // requested but also if we ended up with the bundled git while + // searching for the system one. + + // Realize for comparison. + // + // We should be able to realize the existing program parent directory + // path so we don't handle invalid_path. + // + dir_path d (pp.effect.directory ().realize ()); + + if (!system || d == bd) + ep = "--exec-path=" + d.string (); + + l4 ([&]{trace << (system ? "system: '" : "bundled: '") << pp.effect + << "'" << (ep ? " " + *ep : "");}); +#endif + + git = make_pair (move (pp), move (ep)); + } + + return git; + } + optional git_line (process&& pr, fdpipe&& pipe, bool ie, char delim) { @@ -93,9 +194,13 @@ namespace bdep { auto git_config = [&repo] (const char* name) -> optional { + // It's unlikely that the remote URL is configured globally, thus we use + // the bundled git. + // return git_line (semantic_version {2, 1, 0}, + false /* system */, repo, - true /* ignore_error */, + true /* ignore_error */, "config", "--get", name); @@ -259,10 +364,11 @@ namespace bdep fdpipe pipe (open_pipe ()); // Text mode seems appropriate. process pr (start_git (semantic_version {2, 11, 0}, + false /* system */, repo, - 0 /* stdin */, - pipe /* stdout */, - 2 /* stderr */, + 0 /* stdin */, + pipe /* stdout */, + 2 /* stderr */, "status", "--porcelain=2", "--branch")); diff --git a/bdep/git.hxx b/bdep/git.hxx index dcf3407..bfc5cd2 100644 --- a/bdep/git.hxx +++ b/bdep/git.hxx @@ -17,15 +17,30 @@ namespace bdep // All functions that start git process take the minimum supported git // version as an argument. // + // They also take the system flag (only considered on Windows) that if true + // causes these functions to prefer the system git program (determined via + // PATH) over the bundled one (the bundled git is still used as a fallback). + // Normally, the caller should use the system git when operating on behalf + // of the user (commits, pushes, etc), so that the user interacts with the + // git program they expect. For the query requests (git repository status, + // etc) the bundled git should be used instead to avoid compatibility issues + // (e.g., symlink handling, etc; but when querying for potentially global + // configuration values such as author, system git should be used). Note + // that on POSIX the system git is used for everything. + // // Start git process. // template process - start_git (const semantic_version&, I&& in, O&& out, E&& err, A&&... args); + start_git (const semantic_version&, + bool system, + I&& in, O&& out, E&& err, + A&&... args); template process start_git (const semantic_version&, + bool system, const dir_path& repo, I&& in, O&& out, E&& err, A&&... args); @@ -33,11 +48,13 @@ namespace bdep template inline process start_git (const semantic_version& min_ver, + bool system, dir_path& repo, I&& in, O&& out, E&& err, A&&... args) { return start_git (min_ver, + system, const_cast (repo), forward (in), forward (out), forward (err), forward (args)...); @@ -53,15 +70,23 @@ namespace bdep template void run_git (const semantic_version&, + bool system, bool progress, const dir_path& repo, A&&... args); template inline void - run_git (const semantic_version& min_ver, const dir_path& repo, A&&... args) + run_git (const semantic_version& min_ver, + bool system, + const dir_path& repo, + A&&... args) { - run_git (min_ver, true /* progress */, repo, forward (args)...); + run_git (min_ver, + system, + true /* progress */, + repo, + forward (args)...); } // Return the first line of the git output. If ignore_error is true, then @@ -69,11 +94,15 @@ namespace bdep // template optional - git_line (const semantic_version&, bool ignore_error, A&&... args); + git_line (const semantic_version&, + bool system, + bool ignore_error, + A&&... args); template optional git_line (const semantic_version&, + bool system, const dir_path& repo, bool ignore_error, A&&... args); @@ -88,11 +117,15 @@ namespace bdep // template optional - git_string (const semantic_version&, bool ignore_error, A&&... args); + git_string (const semantic_version&, + bool system, + bool ignore_error, + A&&... args); template optional git_string (const semantic_version&, + bool system, const dir_path& repo, bool ignore_error, A&&... args); diff --git a/bdep/git.ixx b/bdep/git.ixx index 8289544..2913cc6 100644 --- a/bdep/git.ixx +++ b/bdep/git.ixx @@ -6,11 +6,13 @@ namespace bdep template inline process start_git (const semantic_version& min_ver, + bool system, const dir_path& repo, I&& in, O&& out, E&& err, A&&... args) { return start_git (min_ver, + system, forward (in), forward (out), forward (err), "-C", repo, forward (args)...); @@ -25,11 +27,13 @@ namespace bdep template inline optional git_line (const semantic_version& min_ver, + bool system, const dir_path& repo, bool ie, A&&... args) { return git_line (min_ver, + system, ie, "-C", repo, forward (args)...); @@ -38,11 +42,13 @@ namespace bdep template inline optional git_string (const semantic_version& min_ver, + bool system, const dir_path& repo, bool ie, A&&... args) { return git_string (min_ver, + system, ie, "-C", repo, forward (args)...); diff --git a/bdep/git.txx b/bdep/git.txx index 858c452..47705df 100644 --- a/bdep/git.txx +++ b/bdep/git.txx @@ -6,6 +6,7 @@ namespace bdep template void run_git (const semantic_version& min_ver, + bool system, bool progress, const dir_path& repo, A&&... args) @@ -28,6 +29,7 @@ namespace bdep // messages to stdout. // process pr (start_git (min_ver, + system, repo, 0 /* stdin */, err /* stdout */, @@ -71,43 +73,38 @@ namespace bdep } void - git_check_version (const semantic_version& min_ver); + git_check_version (const semantic_version& min_ver, bool system); + + const pair>& + git_search (bool system); template process start_git (const semantic_version& min_ver, + bool system, I&& in, O&& out, E&& err, A&&... args) { - git_check_version (min_ver); + git_check_version (min_ver, system); try { - using namespace butl; + const pair>& git (git_search (system)); - // On startup git prepends the PATH environment variable value with the - // computed directory path where its sub-programs are supposedly located - // (--exec-path option, GIT_EXEC_PATH environment variable, etc; see - // cmd_main() in git's git.c for details). - // - // Then, when git needs to run itself or one of its components as a - // child process, it resolves the full executable path searching in - // directories listed in PATH (see locate_in_PATH() in git's - // run-command.c for details). - // - // On Windows we install git and its components into a place where it is - // not expected to be, which results in the wrong path in PATH as set by - // git (for example, c:/build2/libexec/git-core) which in turn may lead - // to running some other git that appear in the PATH variable. To - // prevent this we pass the git's exec directory via the --exec-path - // option explicitly. + const optional& ep (git.second); // --exec-path + + // Note that if we are running the bundled git (on Windows) and there is + // no vi editor in PATH, then some commands (e.g. commit) may fail with + // the 'cannot spawn vi' error. To avoid this error, we will pass the + // EDITOR=notepad environment variable as a fallback for the git's + // editor search sequence (GIT_EDITOR, core.editor, VISUAL, EDITOR, vi), + // unless it is already set. // - string ep; - process_path pp (process::path_search ("git", true /* init */)); + const char* vars[] = {nullptr, nullptr}; + process_env pe (git.first, vars); -#ifdef _WIN32 - ep = "--exec-path=" + pp.effect.directory ().string (); -#endif + if (ep && !getenv ("EDITOR")) + vars[0] = "EDITOR=notepad"; return process_start_callback ( [] (const char* const args[], size_t n) @@ -116,8 +113,8 @@ namespace bdep print_process (args, n); }, forward (in), forward (out), forward (err), - pp, - !ep.empty () ? ep.c_str () : nullptr, + pe, + ep, forward (args)...); } catch (const process_error& e) @@ -128,12 +125,17 @@ namespace bdep template optional - git_line (const semantic_version& min_ver, bool ie, char delim, A&&... args) + git_line (const semantic_version& min_ver, + bool system, + bool ie, + char delim, + A&&... args) { fdpipe pipe (open_pipe ()); auto_fd null (ie ? open_null () : auto_fd ()); process pr (start_git (min_ver, + system, 0 /* stdin */, pipe /* stdout */, ie ? null.get () : 2 /* stderr */, @@ -144,16 +146,30 @@ namespace bdep template inline optional - git_line (const semantic_version& min_ver, bool ie, A&&... args) + git_line (const semantic_version& min_ver, + bool system, + bool ie, + A&&... args) { - return git_line (min_ver, ie, '\n' /* delim */, forward (args)...); + return git_line (min_ver, + system, + ie, + '\n' /* delim */, + forward (args)...); } template inline optional - git_string (const semantic_version& min_ver, bool ie, A&&... args) + git_string (const semantic_version& min_ver, + bool system, + bool ie, + A&&... args) { - return git_line (min_ver, ie, '\0' /* delim */, forward (args)...); + return git_line (min_ver, + system, + ie, + '\0' /* delim */, + forward (args)...); } template @@ -184,6 +200,7 @@ namespace bdep v.push_back ("-v"); run_git (semantic_version {2, 1, 0}, + true /* system */, progress, repo, "push", diff --git a/bdep/project-author.cxx b/bdep/project-author.cxx index 723dbe9..e4aae46 100644 --- a/bdep/project-author.cxx +++ b/bdep/project-author.cxx @@ -37,6 +37,7 @@ namespace bdep // be queried with the GIT_AUTHOR_IDENT logical variable. // if (optional l = git_line (semantic_version {2, 1, 0}, + true /* system */, prj, true /* ignore_error */, "var", "GIT_AUTHOR_IDENT")) diff --git a/bdep/publish.cxx b/bdep/publish.cxx index c97bdd5..e5e6f26 100644 --- a/bdep/publish.cxx +++ b/bdep/publish.cxx @@ -405,6 +405,7 @@ namespace bdep auto_fd null (q ? open_null () : auto_fd ()); process pr (start_git (git_ver, + true /* system */, prj, 0 /* stdin */, q ? null.get () : 1 /* stdout */, @@ -433,7 +434,13 @@ namespace bdep auto worktree_prune = [&prj] () { - run_git (git_ver, prj, "worktree", "prune", verb > 2 ? "-v" : nullptr); + run_git (git_ver, + true /* system */, + prj, + "worktree", + "prune", + verb > 2 ? "-v" : + nullptr); }; // Create the build2-control branch if it doesn't exist, from scratch if @@ -449,6 +456,7 @@ namespace bdep // fetch command and re-try. // bool local_exists (git_line (git_ver, + false /* system */, prj, false /* ignore_error */, "branch", @@ -459,6 +467,7 @@ namespace bdep // everywhere) via the --remote option or smth? Maybe later. // bool remote_exists (git_line (git_ver, + false /* system */, prj, false /* ignore_error */, "branch", @@ -492,10 +501,11 @@ namespace bdep fdpipe pipe (open_pipe ()); process pr (start_git (git_ver, + true /* system */, prj, - null /* stdin */, - pipe /* stdout */, - 2 /* stderr */, + null /* stdin */, + pipe /* stdout */, + 2 /* stderr */, "hash-object", "-wt", "tree", "--stdin")); @@ -508,6 +518,7 @@ namespace bdep // Create the (empty) root commit. // optional commit (git_line (git_ver, + true /* system */, prj, false, "commit-tree", @@ -524,11 +535,12 @@ namespace bdep // tighten things up a bit. // run_git (git_ver, + true /* system */, prj, "update-ref", "refs/heads/build2-control", *commit, - "" /* oldvalue */); + "" /* oldvalue */); // Checkout the branch. Note that the upstream branch is not setup // for it yet. This will be done by the push operation. @@ -547,6 +559,7 @@ namespace bdep // branch. // run_git (git_ver, + true /* system */, prj, "branch", verb < 2 ? "-q" : nullptr, @@ -618,10 +631,11 @@ namespace bdep // Note that fast-forwarding can potentially fail. That will mean the // local branch has diverged from the remote one for some reason // (e.g., inability to revert the commit, etc.). We again leave it to - // the use to deal with. + // the user to deal with. // if (local_exists && remote_exists) run_git (git_ver, + true /* system */, wd, "merge", verb < 2 ? "-q" : verb > 2 ? "-v" : nullptr, @@ -662,6 +676,7 @@ namespace bdep } run_git (git_ver, + true /* system */, wd, "add", verb > 2 ? "-v" : nullptr, @@ -700,6 +715,7 @@ namespace bdep } run_git (git_ver, + true /* system */, wd, "commit", verb < 2 ? "-q" : verb > 2 ? "-v" : nullptr, @@ -730,6 +746,7 @@ namespace bdep worktree_remove (); // Release the branch before removal. run_git (git_ver, + true /* system */, prj, "branch", verb < 2 ? "-q" : nullptr, @@ -738,6 +755,7 @@ namespace bdep } else run_git (git_ver, + true /* system */, wd, "reset", verb < 2 ? "-q" : nullptr, diff --git a/bdep/release.cxx b/bdep/release.cxx index 66b4a30..3f13427 100644 --- a/bdep/release.cxx +++ b/bdep/release.cxx @@ -934,10 +934,11 @@ namespace bdep // process pr ( start_git (git_ver, + true /* system */, prj.path, - 0 /* stdin */, - 2 /* stdout */, - 2 /* stderr */, + 0 /* stdin */, + 2 /* stdout */, + 2 /* stderr */, "commit", verb < 1 ? "-q" : verb >= 2 ? "-v" : nullptr, amend ? "--amend" : nullptr, @@ -1040,8 +1041,9 @@ namespace bdep // optional ms ( git_string (git_ver, + false /* system */, prj.path, - false /* ignore_error */, + false /* ignore_error */, "log", "--format=%B", "HEAD~" + to_string (o.squash ()) + "..HEAD")); @@ -1062,6 +1064,7 @@ namespace bdep // Squash commits, reverting them into the index. // run_git (git_ver, + true /* system */, prj.path, "reset", verb < 1 ? "-q" : nullptr, @@ -1085,6 +1088,7 @@ namespace bdep if (e.normal ()) { run_git (git_ver, + true /* system */, prj.path, "reset", verb < 1 ? "-q" : nullptr, @@ -1131,6 +1135,7 @@ namespace bdep const optional& ct (prj.cur_tag); run_git (git_ver, + true /* system */, prj.path, "tag", (ct && ct->action == cmd_release_current_tag::update @@ -1141,7 +1146,12 @@ namespace bdep *prj.tag); if (ct && ct->action == cmd_release_current_tag::remove) - run_git (git_ver, prj.path, "tag", "--delete", ct->name); + run_git (git_ver, + true /* system */, + prj.path, + "tag", + "--delete", + ct->name); } // Open. @@ -1197,7 +1207,11 @@ namespace bdep string remote; string brspec; { + // It's unlikely that the branch remote is configured globally, so we + // use the bundled git. + // optional rem (git_line (git_ver, + false /* system */, prj.path, false /* ignore_error */, "config", diff --git a/bdep/types.hxx b/bdep/types.hxx index d2a0355..2564851 100644 --- a/bdep/types.hxx +++ b/bdep/types.hxx @@ -99,6 +99,7 @@ namespace bdep // // using butl::process; + using butl::process_env; using butl::process_path; using butl::process_exit; using butl::process_error; -- cgit v1.1