git: simplify environment save/restore logic

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 as the first thing, so we can consider that the variable 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 already 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 same decision. Signed-off-by: Junio C Hamano <gitster@pobox.com>

Junio C Hamano committed Jan 26, 2016 at 22:52 UTC 441981bc85ea2b648d7ffb2515b371071208e657
1 file changed +13 -14
git.c
+13 -14
@@ -25,13 +25,11 @@ static const char *env_names[] = {
25 GIT_PREFIX_ENVIRONMENT
26 };
27 static char *orig_env[4];
28 -static int saved_env_before_alias;
28 static int save_restore_env_balance;
29
30 static void save_env_before_alias(void)
31 {
32 int i;
34 - saved_env_before_alias = 1;
33
34 assert(save_restore_env_balance == 0);
35 save_restore_env_balance = 1;
@@ -533,16 +531,8 @@ static void handle_builtin(int argc, const char **argv)
531 }
532
533 builtin = get_builtin(cmd);
536 - if (builtin) {
537 - /*
538 - * XXX: if we can figure out cases where it is _safe_
539 - * to do, we can avoid spawning a new process when
540 - * saved_env_before_alias is true
541 - * (i.e. setup_git_dir* has been run once)
542 - */
543 - if (!saved_env_before_alias)
544 - exit(run_builtin(builtin, argc, argv));
545 - }
534 + if (builtin)
535 + exit(run_builtin(builtin, argc, argv));
536 }
537
538 static void execv_dashed_external(const char **argv)
@@ -586,8 +576,17 @@ static int run_argv(int *argcp, const char ***argv)
576 int done_alias = 0;
577
578 while (1) {
589 - /* See if it's a builtin */
590 - handle_builtin(*argcp, *argv);
579 + /*
580 + * If we tried alias and futzed with our environment,
581 + * it no longer is safe to invoke builtins directly in
582 + * general. We have to spawn them as dashed externals.
583 + *
584 + * NEEDSWORK: if we can figure out cases
585 + * where it is safe to do, we can avoid spawning a new
586 + * process.
587 + */
588 + if (!done_alias)
589 + handle_builtin(*argcp, *argv);
590
591 /* .. then try the external ones */
592 execv_dashed_external(*argv);