Re: [PATCH 2/2] builtins: setup repository before print unknown command error

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 26, 2010, 20:58 UTC
Message-ID: <20100326205848.GB11395@progeny.tock>
In-Reply-To: <1269524295-24569-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy wrote:

Show 9 quoted lines

> As it is now, because
>  - alias handling will always be done before help_unknown_cmd()
>  - alias handling code will search and set up repository if found
>  - alias handline code will not undo repository setup
> 
> These ensure that repository will always be set up (or attempted to
> set up) before help_unknown_cmd(), so there is no issue. But the setup
> dependency here is subtle. It may break some day if someone reorders
> the loop, for example.

It took me a while to figure out what was going on here. I think it is too complicated. Could it make sense to use

	/*
	 * help_unknown_cmd() requires that the repository has searched
	 * for and set up if found.
	 * Luckily, the alias handling code already took care of this.
	 */
	if (!startup_info->have_run_setup_gitdir)
		die("internal error handling unknown command");

instead, to document the current assumption?

Alternatively, one might add a new ensure_git_directory_is_set_up() function that only runs setup_git_repository_gently if it has not already been run. I do not like that so much --- too easy to hide bugs.

Better to run setup_git_repository_gently once unconditionally, before handle_argv. Unfortunately, that breaks git-init. Actually, the current series breaks git-init when run through an alias.

Thoughts?

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 6757734..792abe2 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -33,6 +33,21 @@ test_expect_success 'plain' '
 	check_config plain/.git false unset
 '
 
+test_expect_failure 'plain through aliased command' '
+	(
+		unset GIT_DIR GIT_WORK_TREE GIT_CONFIG_NOGLOBAL &&
+		HOME=$(pwd)/alias-config &&
+		export HOME &&
+		mkdir alias-config &&
+		echo "[alias] aliasedinit = init" >alias-config/.gitconfig &&
+		mkdir plain-aliased &&
+		cd plain-aliased &&
+		git aliasedinit
+	) &&
+	check_config plain-aliased/.git false unset
+'
+
 test_expect_success 'plain with GIT_WORK_TREE' '
 	if (
 		unset GIT_DIR