[PATCH] alias: detect loops in mixed execution mode

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 18, 2018, 22:57 UTC
Message-ID: <20181018225739.28857-1-avarab@gmail.com>
In-Reply-To: <87o9dar9qc.fsf@evledraar.gmail.com>

Add detection for aliasing loops in cases where one of the aliases re-invokes git as a shell command. This catches cases like:

    [alias]
    foo = !git bar
    bar = !git foo

Before this change running "git {foo,bar}" would create a forkbomb. Now using the aliasing loop detection and call history reporting added in 82f71d9a5a ("alias: show the call history when an alias is looping", 2018-09-16) and c6d75bc17a ("alias: add support for aliases of an alias", 2018-09-16) we'll instead report:

    fatal: alias loop detected: expansion of 'foo' does not terminate:
      foo <==
      bar ==>

Since the implementation carries the call history in an environment variable, using the same sort of trick as used for -c (see 2b64fc894d ("pass "git -c foo=bar" params through environment", 2010-08-23) ). For example:

    [alias]
    one = two
    two = !git three
    three = four
    four = !git five
    five = two

Will, on "git one" report:

    fatal: alias loop detected: expansion of 'one' does not terminate:
      one
      two <==
      three
      four
      five ==>

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Implements what I suggested in https://public-inbox.org/git/87o9dar9qc.fsf@evledraar.gmail.com/

 cache.h          |  1 +
 git.c            | 36 ++++++++++++++++++++++++++++++++++--
 t/t0001-init.sh  |  1 +
 t/t0014-alias.sh | 15 ++++++---------
 4 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index d508f3d4f8..00cbd25f1c 100644
--- a/cache.h
+++ b/cache.h
@@ -478,6 +478,7 @@ static inline enum object_type object_type(unsigned int mode)
 #define TEMPLATE_DIR_ENVIRONMENT "GIT_TEMPLATE_DIR"
 #define CONFIG_ENVIRONMENT "GIT_CONFIG"
 #define CONFIG_DATA_ENVIRONMENT "GIT_CONFIG_PARAMETERS"
+#define COMMAND_HISTORY_ENVIRONMENT "GIT_COMMAND_HISTORY"
 #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
 #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
 #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"
diff --git a/git.c b/git.c
index 5920f8019b..cba242836c 100644
--- a/git.c
+++ b/git.c
@@ -672,12 +672,43 @@ static void execv_dashed_external(const char **argv)
 		exit(128);
 }
 
+static void init_cmd_history(struct strbuf *env, struct string_list *cmd_list)
+{
+	const char *old = getenv(COMMAND_HISTORY_ENVIRONMENT);
+	struct strbuf **cmd_history, **ptr;
+
+	if (!old || !*old)
+		return;
+
+	strbuf_addstr(env, old);
+	strbuf_rtrim(env);
+
+	cmd_history = strbuf_split_buf(old, strlen(old), ' ', 0);
+	for (ptr = cmd_history; *ptr; ptr++) {
+		strbuf_rtrim(*ptr);
+		string_list_append(cmd_list, (*ptr)->buf);
+	}
+	strbuf_list_free(cmd_history);
+}
+
+static void add_cmd_history(struct strbuf *env, struct string_list *cmd_list,
+			    const char *cmd)
+{
+	string_list_append(cmd_list, cmd);
+	if (env->len)
+		strbuf_addch(env, ' ');
+	strbuf_addstr(env, cmd);
+	setenv(COMMAND_HISTORY_ENVIRONMENT, env->buf, 1);
+}
+
 static int run_argv(int *argcp, const char ***argv)
 {
 	int done_alias = 0;
-	struct string_list cmd_list = STRING_LIST_INIT_NODUP;
+	struct string_list cmd_list = STRING_LIST_INIT_DUP;
 	struct string_list_item *seen;
+	struct strbuf env = STRBUF_INIT;
 
+	init_cmd_history(&env, &cmd_list);
 	while (1) {
 		/*
 		 * If we tried alias and futzed with our environment,
@@ -711,7 +742,7 @@ static int run_argv(int *argcp, const char ***argv)
 			      " not terminate:%s"), cmd_list.items[0].string, sb.buf);
 		}
 
-		string_list_append(&cmd_list, *argv[0]);
+		add_cmd_history(&env, &cmd_list, *argv[0]);
 
 		/*
 		 * It could be an alias -- this works around the insanity
@@ -724,6 +755,7 @@ static int run_argv(int *argcp, const char ***argv)
 	}
 
 	string_list_clear(&cmd_list, 0);
+	strbuf_release(&env);
 
 	return done_alias;
 }
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 182da069f1..eb2ca8a172 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -93,6 +93,7 @@ test_expect_success 'No extra GIT_* on alias scripts' '
 		sed -n \
 			-e "/^GIT_PREFIX=/d" \
 			-e "/^GIT_TEXTDOMAINDIR=/d" \
+			-e "/^GIT_COMMAND_HISTORY=/d" \
 			-e "/^GIT_/s/=.*//p" |
 		sort
 	EOF
diff --git a/t/t0014-alias.sh b/t/t0014-alias.sh
index a070e645d7..9ed03a4a4f 100755
--- a/t/t0014-alias.sh
+++ b/t/t0014-alias.sh
@@ -27,14 +27,11 @@ test_expect_success 'looping aliases - internal execution' '
 	test_i18ngrep "^fatal: alias loop detected: expansion of" output
 '
 
-# This test is disabled until external loops are fixed, because would block
-# the test suite for a full minute.
-#
-#test_expect_failure 'looping aliases - mixed execution' '
-#	git config alias.loop-mixed-1 loop-mixed-2 &&
-#	git config alias.loop-mixed-2 "!git loop-mixed-1" &&
-#	test_must_fail git loop-mixed-1 2>output &&
-#	test_i18ngrep "^fatal: alias loop detected: expansion of" output
-#'
+test_expect_success 'looping aliases - mixed execution' '
+	git config alias.loop-mixed-1 loop-mixed-2 &&
+	git config alias.loop-mixed-2 "!git loop-mixed-1" &&
+	test_must_fail git loop-mixed-1 2>output &&
+	test_i18ngrep "^fatal: alias loop detected: expansion of" output
+'
 
 test_done

-- 
2.19.1.568.g152ad8e336