[PATCH v2 1/3] git: remove an early return from save_env_before_alias()

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 27, 2016, 23:18 UTC
Message-ID: <xmqqy4ba627n.fsf_-_@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqtwlz8c4w.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 18 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Wed, Jan 27, 2016 at 1:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> I spoke too soon, I am afraid.
>>>> ...
>>>> I wonder if this would be better done as a multi-part series that
>>>> goes like this:
>>>> ...
>>>
>>> So here is the first of the three-patch series to replace it.
>>
>> This is much better (the whole series, not just the first patch). We
>> probably should add a test about help.autocorrect though, maybe in the
>> first patch, because it's not tested at all in the test suite.
>
> Patches welcome.  Thanks.

Here is an updated 1/3; this feature does not fit very well to any category (it is certainly not basic, or command that is primarily about objects or worktree; I just picked "Git tools" t9xxx as that is the closest thing to "direct end user support").

-- >8 -- When help.autocorrect is in effect, an attempt to auto-execute an uniquely corrected result of a misspelt alias will result in an irrelevant error message. The codepath that causes this calls save_env_before_alias() and restore_env() in handle_alias(), and that happens twice. A global variable orig_cwd is allocated to hold the return value of getcwd() in save_env_before_alias(), which is then used in restore_env() to go back to that directory and finally free(3)'d there.

However, save_env_before_alias() is not prepared to be called twice. It returns early when it knows it has already been called, leaving orig_cwd undefined, which is then checked in the second call to restore_env(), and by that time, the memory that used to hold the contents of orig_cwd is either freed or reused to hold something else, and this is fed to chdir(2), causing it to fail. Even if it did not fail (i.e. reading of the already free'd piece of memory yielded a directory path that we can chdir(2) to), it then gets free(3)'d.

Fix this by making sure save_env() does do the saving when called.

While at it, add a minimal test for help.autocorrect facility.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git.c                       |  2 --
 t/t9003-help-autocorrect.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100755 t/t9003-help-autocorrect.sh

diff --git a/git.c b/git.c
index 98d4412..a57a4cb 100644
--- a/git.c
+++ b/git.c
@@ -30,8 +30,6 @@ static int saved_env_before_alias;
 static void save_env_before_alias(void)
 {
 	int i;
-	if (saved_env_before_alias)
-		return;
 	saved_env_before_alias = 1;
 	orig_cwd = xgetcwd();
 	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
diff --git a/t/t9003-help-autocorrect.sh b/t/t9003-help-autocorrect.sh
new file mode 100755
index 0000000..dfe95c9
--- /dev/null
+++ b/t/t9003-help-autocorrect.sh
@@ -0,0 +1,52 @@
+#!/bin/sh
+
+test_description='help.autocorrect finding a match'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	# An alias
+	git config alias.lgf "log --format=%s --first-parent" &&
+
+	# A random user-defined command
+	write_script git-distimdistim <<-EOF &&
+		echo distimdistim was called
+	EOF
+
+	PATH="$PATH:." &&
+	export PATH &&
+
+	git commit --allow-empty -m "a single log entry" &&
+
+	# Sanity check
+	git lgf >actual &&
+	echo "a single log entry" >expect &&
+	test_cmp expect actual &&
+
+	git distimdistim >actual &&
+	echo "distimdistim was called" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'autocorrect showing candidates' '
+	git config help.autocorrect 0 &&
+
+	test_must_fail git lfg 2>actual &&
+	sed -e "1,/^Did you mean this/d" actual | grep lgf &&
+
+	test_must_fail git distimdist 2>actual &&
+	sed -e "1,/^Did you mean this/d" actual | grep distimdistim
+'
+
+test_expect_success 'autocorrect running commands' '
+	git config help.autocorrect -1 &&
+
+	git lfg >actual &&
+	echo "a single log entry" >expect &&
+	test_cmp expect actual &&
+
+	git distimdist >actual &&
+	echo "distimdistim was called" >expect &&
+	test_cmp expect actual
+'
+
+test_done

-- 
2.7.0-368-gb6e04f9