The only code that cares about the value of the global variable
saved_env_before_alias after the previous fix is handle_builtin()
that turns into a glorified no-op when the variable is true, so the
logic could safely be lifted to its caller, i.e. the caller can
refrain from calling it when the variable is set.
This variable tells us if save_env_before_alias() was called (with
or without matching restore_env()), but the sole caller of the
function, handle_alias(), always calls it the first as thing, so it
essentially keeps track of the fact that handle_alias() has ever
been called.
It turns out that handle_builtin() and handle_alias() are called
only from one function in a way that the value of the variable
matters, which is run_argv(), and it already keeps track of the fact
that it called handle_alias().
So we can simplify the whole thing by:
- Change handle_builtin() to always make a direct call to the
builtin implementation it finds, and make sure the caller
refrains from calling it if handle_alias() has ever been
called;
- Remove saved_env_before_alias variable, and instead use the
local "done_alias" variable maintained inside run_argv() to
make the above decision.
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
* I do not mean to say that it is unnecessary to plug the leak,
which should be the fourth patch in this three-patch series, but
it should rather be obvious to implement, so I omitted from this
series, which is primarily meant to be "how about doing it this
way" illustration.
git.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/git.c b/git.c
index e39b972..c8d7b56 100644
--- a/git.c
+++ b/git.c
@@ -25,13 +25,11 @@ static const char *env_names[] = {
GIT_PREFIX_ENVIRONMENT
};
static char *orig_env[4];
-static int saved_env_before_alias;
static int save_restore_env_balance;
static void save_env_before_alias(void)
{
int i;
- saved_env_before_alias = 1;
assert(save_restore_env_balance == 0);
save_restore_env_balance = 1;
@@ -533,16 +531,8 @@ static void handle_builtin(int argc, const char **argv)
}
builtin = get_builtin(cmd);
- if (builtin) {
- /*
- * XXX: if we can figure out cases where it is _safe_
- * to do, we can avoid spawning a new process when
- * saved_env_before_alias is true
- * (i.e. setup_git_dir* has been run once)
- */
- if (!saved_env_before_alias)
- exit(run_builtin(builtin, argc, argv));
- }
+ if (builtin)
+ exit(run_builtin(builtin, argc, argv));
}
static void execv_dashed_external(const char **argv)
@@ -586,8 +576,17 @@ static int run_argv(int *argcp, const char ***argv)
int done_alias = 0;
while (1) {
- /* See if it's a builtin */
- handle_builtin(*argcp, *argv);
+ /*
+ * If we tried alias and futzed with our environment,
+ * it no longer is safe to invoke builtins directly in
+ * general. We have to spawn them as dashed externals.
+ *
+ * NEEDSWORK: if we can figure out cases
+ * where it is safe to do, we can avoid spawning a new
+ * process.
+ */
+ if (!done_alias)
+ handle_builtin(*argcp, *argv);
/* .. then try the external ones */
execv_dashed_external(*argv);
@@ -598,9 +597,9 @@ static int run_argv(int *argcp, const char ***argv)
*/
if (done_alias)
break;
+ done_alias = 1;
if (!handle_alias(argcp, argv))
break;
- done_alias = 1;
}
return done_alias;
--
2.7.0-368-g4610598
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html