threads / patch / 46178

v4, 6 partsAvoid problem where git_dir is set after alias expansion

Subject: [PATCH v4 0/6] Avoid problem where git_dir is set after alias expansion

## tl;dr

10 messages between Jun 14, 2017 and Jun 15, 2017. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Johannes Schindelin· Jun 14, 2017, 11:35 UTC · lore

When expanding an alias in a subdirectory, we setup the git_dir (gently), read the config, and then restore the "env" (e.g. the current working directory) so that the command specified by the alias can run correctly.

What we failed to reset was the git_dir, meaning that in the most common case, it was now pointing to a .git/ directory *in the subdirectory*.

This problem was identified in the GVFS fork, where a pre-command hook was introduced to allow pre-fetching missing blobs.

An early quick fix in the GVFS fork simply built on top of the save_env_before_alias() hack, introducing another hack that saves the git_dir and restores it after an alias is expanded:

	https://github.com/Microsoft/git/commit/2d859ba3b

That is very hacky, though, and it is much better (although much more involved, too) to fix this "properly", i.e. by replacing the ugly save/restore logic by simply using the early config code path.

However, aliases are strange beasts.

When an alias refers to a single Git command (originally the sole intention of aliases), the current working directory is restored to what it had been before expanding the alias.

But when an alias starts with an exclamation point, i.e. referring to a command-line to be interpreted by the shell, the current working directory is no longer in the subdirectory but instead in the worktree's top-level directory.

This is even true for worktrees added by `git worktree add`.

But when we are inside the .git/ directory, the current working directory is *restored* to the subdirectory inside the .git/ directory.

In short, the rules (what is the expected current working directory after expanding an alias and before actually running it) are a bit complicated.

This patch series does *not* address the problem identified by Brandon Williams where the early config machinery fails to look into the *commondir* in worktrees added by `git worktree add`. My hope is that Brandon is okay with applying this patch series before his config patch series.

Changes since v3:
- avoided the strbuf in alias_lookup() by using skip_prefix() and !strcmp()
  instead.
- fixed tyop ("read" instead of "reading", causing Junio to stumble) in the
  commit message of 6/6.
Johannes Schindelin (6):
  discover_git_directory(): avoid setting invalid git_dir
  config: report correct line number upon error
  help: use early config when autocorrecting aliases
  t1308: relax the test verifying that empty alias values are disallowed
  t7006: demonstrate a problem with aliases in subdirectories
  Use the early config machinery to expand aliases
 alias.c                | 29 +++++++++++++++++++-------
 config.c               |  3 ++-
 git.c                  | 55 ++++----------------------------------------------
 help.c                 |  2 +-
 setup.c                |  1 +
 t/t1300-repo-config.sh |  6 ++++++
 t/t1308-config-set.sh  |  4 +++-
 t/t7006-pager.sh       | 11 ++++++++++
 8 files changed, 50 insertions(+), 61 deletions(-)
base-commit: 02a2850ad58eff6de70eb2dc5f96345c463857ac
Published-As: https://github.com/dscho/git/releases/tag/alias-early-config-v4
Fetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v4
Interdiff vs v3:
 diff --git a/alias.c b/alias.c
 index 6bdc9363037..5df052ae4c4 100644
 --- a/alias.c
 +++ b/alias.c
 @@ -2,15 +2,16 @@
  
  struct config_alias_data
  {
 -	struct strbuf key;
 +	const char *alias;
  	char *v;
  };
  
  static int config_alias_cb(const char *key, const char *value, void *d)
  {
  	struct config_alias_data *data = d;
 +	const char *p;
  
 -	if (!strcmp(key, data->key.buf))
 +	if (skip_prefix(key, "alias.", &p) && !strcmp(p, data->alias))
  		return git_config_string((const char **)&data->v, key, value);
  
  	return 0;
 @@ -18,12 +19,9 @@ static int config_alias_cb(const char *key, const char *value, void *d)
  
  char *alias_lookup(const char *alias)
  {
 -	struct config_alias_data data = { STRBUF_INIT, NULL };
 +	struct config_alias_data data = { alias, NULL };
  
 -	strbuf_addf(&data.key, "alias.%s", alias);
 -	if (git_config_key_is_valid(data.key.buf))
 -		read_early_config(config_alias_cb, &data);
 -	strbuf_release(&data.key);
 +	read_early_config(config_alias_cb, &data);
  
  	return data.v;
  }
-- 
2.13.1.windows.1.1.ga36e14b3aaa
Johannes Schindelin· Jun 14, 2017, 11:35 UTC · re: Johannes Schindelin · lore

[PATCH v4 1/6] discover_git_directory(): avoid setting invalid git_dir

When discovering a .git/ directory, we take pains to ensure that its repository format version matches Git's expectations, and we return NULL otherwise.

However, we still appended the invalid path to the strbuf passed as argument.

Let's just reset the strbuf to the state before we appended the .git/ directory that was eventually rejected.

There is another early return path in that function, when setup_git_directory_gently_1() returns GIT_DIR_NONE or an error. In that case, the gitdir parameter has not been touched, therefore there is no need for an equivalent change in that code path.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 setup.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to setup.c +1 −0
diff --git a/setup.c b/setup.c
index ba6e8551788..ad12d79ac02 100644
--- a/setup.c
+++ b/setup.c
@@ -982,6 +982,7 @@ const char *discover_git_directory(struct strbuf *gitdir)
 		warning("ignoring git dir '%s': %s",
 			gitdir->buf + gitdir_offset, err.buf);
 		strbuf_release(&err);
+		strbuf_setlen(gitdir, gitdir_offset);
 		return NULL;
 	}
 
-- 
2.13.1.windows.1.1.ga36e14b3aaa
Johannes Schindelin· Jun 14, 2017, 11:35 UTC · re: Johannes Schindelin · lore

[PATCH v4 3/6] help: use early config when autocorrecting aliases

Git has this feature which suggests similar commands (including aliases) in case the user specified an unknown command.

This feature currently relies on a side effect of the way we expand aliases right now: when a command is not a builtin, we use the regular config machinery (meaning: discovering the .git/ directory and initializing global state such as the config cache) to see whether the command refers to an alias.

However, we will change the way aliases are expanded in the next commits, to use the early config instead. That means that the autocorrect feature can no longer discover the available aliases by looking at the config cache (because it has not yet been initialized).

So let's just use the early config machinery instead.

This is slightly less performant than the previous way, as the early config is used *twice*: once to see whether the command refers to an alias, and then to see what aliases are most similar. However, this is hardly a performance-critical code path, so performance is less important here.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 help.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to help.c +1 −1
diff --git a/help.c b/help.c
index db7f3d79a01..b44c55ec2da 100644
--- a/help.c
+++ b/help.c
@@ -289,7 +289,7 @@ const char *help_unknown_cmd(const char *cmd)
 	memset(&other_cmds, 0, sizeof(other_cmds));
 	memset(&aliases, 0, sizeof(aliases));
 
-	git_config(git_unknown_cmd_config, NULL);
+	read_early_config(git_unknown_cmd_config, NULL);
 
 	load_command_list("git-", &main_cmds, &other_cmds);
 
-- 
2.13.1.windows.1.1.ga36e14b3aaa
Johannes Schindelin· Jun 14, 2017, 11:35 UTC · re: Johannes Schindelin · lore

[PATCH v4 4/6] t1308: relax the test verifying that empty alias values are disallowed

We are about to change the way aliases are expanded, to use the early config machinery.

This machinery reports errors in a slightly different manner than the cached config machinery.

Let's not get hung up by the precise wording of the message mentioning the line number. It is really sufficient to verify that all the relevant information is given to the user.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1308-config-set.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to t/t1308-config-set.sh +3 −1
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index e495a616161..bafed5c9b88 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -226,7 +226,9 @@ test_expect_success 'check line errors for malformed values' '
 		br
 	EOF
 	test_expect_code 128 git br 2>result &&
-	test_i18ngrep "fatal: .*alias\.br.*\.git/config.*line 2" result
+	test_i18ngrep "missing value for .alias\.br" result &&
+	test_i18ngrep "fatal: .*\.git/config" result &&
+	test_i18ngrep "fatal: .*line 2" result
 '
 
 test_expect_success 'error on modifying repo config without repo' '
-- 
2.13.1.windows.1.1.ga36e14b3aaa
Johannes Schindelin· Jun 14, 2017, 11:35 UTC · re: Johannes Schindelin · lore

[PATCH v4 2/6] config: report correct line number upon error

When get_value() parses a key/value pair, it is possible that the line number is decreased (because the \n has been consumed already) before the key/value pair is passed to the callback function, to allow for the correct line to be attributed in case of an error.

However, when git_parse_source() asks get_value() to parse the key/value pair, the error reporting is performed *after* get_value() returns.

Which means that we have to be careful not to increase the line number in get_value() after the callback function returned an error.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.c               | 3 ++-
 t/t1300-repo-config.sh | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)
Show changes to 2 files +8 −1

config.c, t/t1300-repo-config.sh

diff --git a/config.c b/config.c
index 34a139c40bd..547daf87d40 100644
--- a/config.c
+++ b/config.c
@@ -604,7 +604,8 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 	 */
 	cf->linenr--;
 	ret = fn(name->buf, value, data);
-	cf->linenr++;
+	if (ret >= 0)
+		cf->linenr++;
 	return ret;
 }
 
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 13b7851f7c2..a37ef042221 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -703,6 +703,12 @@ test_expect_success 'invalid unit' '
 	test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual
 '
 
+test_expect_success 'line number is reported correctly' '
+	printf "[bool]\n\tvar\n" >invalid &&
+	test_must_fail git config -f invalid --path bool.var 2>actual &&
+	test_i18ngrep "line 2" actual
+'
+
 test_expect_success 'invalid stdin config' '
 	echo "[broken" | test_must_fail git config --list --file - >output 2>&1 &&
 	test_i18ngrep "bad config line 1 in standard input" output
-- 
2.13.1.windows.1.1.ga36e14b3aaa
Johannes Schindelin· Jun 14, 2017, 11:35 UTC · re: Johannes Schindelin · lore

[PATCH v4 5/6] t7006: demonstrate a problem with aliases in subdirectories

When expanding aliases, the git_dir is set during the alias expansion (by virtue of running setup_git_directory_gently()).

This git_dir may be relative to the current working directory, and indeed often is simply ".git/".

When the alias expands to a shell command, we restore the original working directory, though, yet we do not reset git_dir.

As a consequence, subsequent read_early_config() runs will mistake the git_dir to be populated properly and not find the correct config.

Demonstrate this problem by adding a test case.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7006-pager.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)
Show changes to t/t7006-pager.sh +11 −0
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 4f3794d415e..83881ec3a0c 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -391,6 +391,17 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' '
 	)
 '
 
+test_expect_failure TTY 'core.pager is found via alias in subdirectory' '
+	sane_unset GIT_PAGER &&
+	test_config core.pager "cat >via-alias" &&
+	(
+		cd sub &&
+		rm -f via-alias &&
+		test_terminal git -c alias.r="-p rev-parse" r HEAD &&
+		test_path_is_file via-alias
+	)
+'
+
 test_doesnt_paginate      expect_failure test_must_fail 'git -p nonsense'
 
 test_pager_choices                       'git shortlog'
-- 
2.13.1.windows.1.1.ga36e14b3aaa
Johannes Schindelin· Jun 14, 2017, 11:36 UTC · re: Johannes Schindelin · lore

[PATCH v4 6/6] Use the early config machinery to expand aliases

Instead of discovering the .git/ directory, reading the config and then trying to painstakingly reset all the global state if we did not find a matching alias, let's use the early config machinery instead.

It may look like unnecessary work to discover the .git/ directory in the early config machinery and then call setup_git_directory_gently() in the case of a shell alias, repeating the very same discovery *again*. However, we have to do this as the early config machinery takes pains *not* to touch any global state, while shell aliases expect a possibly changed working directory and at least the GIT_PREFIX and GIT_DIR variables to be set.

This change also fixes a known issue where Git tried to read the pager config from an incorrect path in a subdirectory of a Git worktree if an alias expanded to a shell command.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 alias.c          | 29 ++++++++++++++++++++++-------
 git.c            | 55 ++++---------------------------------------------------
 t/t7006-pager.sh |  2 +-
 3 files changed, 27 insertions(+), 59 deletions(-)
Show changes to 3 files +27 −59

alias.c, git.c, t/t7006-pager.sh

diff --git a/alias.c b/alias.c
index 3b90397a99d..5df052ae4c4 100644
--- a/alias.c
+++ b/alias.c
@@ -1,14 +1,29 @@
 #include "cache.h"
 
+struct config_alias_data
+{
+	const char *alias;
+	char *v;
+};
+
+static int config_alias_cb(const char *key, const char *value, void *d)
+{
+	struct config_alias_data *data = d;
+	const char *p;
+
+	if (skip_prefix(key, "alias.", &p) && !strcmp(p, data->alias))
+		return git_config_string((const char **)&data->v, key, value);
+
+	return 0;
+}
+
 char *alias_lookup(const char *alias)
 {
-	char *v = NULL;
-	struct strbuf key = STRBUF_INIT;
-	strbuf_addf(&key, "alias.%s", alias);
-	if (git_config_key_is_valid(key.buf))
-		git_config_get_string(key.buf, &v);
-	strbuf_release(&key);
-	return v;
+	struct config_alias_data data = { alias, NULL };
+
+	read_early_config(config_alias_cb, &data);
+
+	return data.v;
 }
 
 #define SPLIT_CMDLINE_BAD_ENDING 1
diff --git a/git.c b/git.c
index 8ff44f081d4..58ef570294d 100644
--- a/git.c
+++ b/git.c
@@ -16,50 +16,6 @@ const char git_more_info_string[] =
 	   "to read about a specific subcommand or concept.");
 
 static int use_pager = -1;
-static char *orig_cwd;
-static const char *env_names[] = {
-	GIT_DIR_ENVIRONMENT,
-	GIT_WORK_TREE_ENVIRONMENT,
-	GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
-	GIT_PREFIX_ENVIRONMENT
-};
-static char *orig_env[4];
-static int save_restore_env_balance;
-
-static void save_env_before_alias(void)
-{
-	int i;
-
-	assert(save_restore_env_balance == 0);
-	save_restore_env_balance = 1;
-	orig_cwd = xgetcwd();
-	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
-		orig_env[i] = getenv(env_names[i]);
-		orig_env[i] = xstrdup_or_null(orig_env[i]);
-	}
-}
-
-static void restore_env(int external_alias)
-{
-	int i;
-
-	assert(save_restore_env_balance == 1);
-	save_restore_env_balance = 0;
-	if (!external_alias && orig_cwd && chdir(orig_cwd))
-		die_errno("could not move to %s", orig_cwd);
-	free(orig_cwd);
-	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
-		if (external_alias &&
-		    !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))
-			continue;
-		if (orig_env[i]) {
-			setenv(env_names[i], orig_env[i], 1);
-			free(orig_env[i]);
-		} else {
-			unsetenv(env_names[i]);
-		}
-	}
-}
 
 static void commit_pager_choice(void) {
 	switch (use_pager) {
@@ -250,19 +206,18 @@ static int handle_alias(int *argcp, const char ***argv)
 	const char **new_argv;
 	const char *alias_command;
 	char *alias_string;
-	int unused_nongit;
-
-	save_env_before_alias();
-	setup_git_directory_gently(&unused_nongit);
 
 	alias_command = (*argv)[0];
 	alias_string = alias_lookup(alias_command);
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			struct child_process child = CHILD_PROCESS_INIT;
+			int nongit_ok;
+
+			/* Aliases expect GIT_PREFIX, GIT_DIR etc to be set */
+			setup_git_directory_gently(&nongit_ok);
 
 			commit_pager_choice();
-			restore_env(1);
 
 			child.use_shell = 1;
 			argv_array_push(&child.args, alias_string + 1);
@@ -308,8 +263,6 @@ static int handle_alias(int *argcp, const char ***argv)
 		ret = 1;
 	}
 
-	restore_env(0);
-
 	errno = saved_errno;
 
 	return ret;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 83881ec3a0c..20b4d83c281 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -391,7 +391,7 @@ test_expect_success TTY 'core.pager in repo config works and retains cwd' '
 	)
 '
 
-test_expect_failure TTY 'core.pager is found via alias in subdirectory' '
+test_expect_success TTY 'core.pager is found via alias in subdirectory' '
 	sane_unset GIT_PAGER &&
 	test_config core.pager "cat >via-alias" &&
 	(
-- 
2.13.1.windows.1.1.ga36e14b3aaa
Junio C Hamano· Jun 15, 2017, 19:36 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4 6/6] Use the early config machinery to expand aliases

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> +struct config_alias_data
> +{
Style: "struct config_alias_data {"
which I can tweak while applying.
Other than that, everything looks good.
Thanks.
Johannes Schindelin· Jun 15, 2017, 19:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 6/6] Use the early config machinery to expand aliases

Hi Junio,
On Thu, 15 Jun 2017, Junio C Hamano wrote:
Show 8 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +struct config_alias_data
> > +{
> 
> Style: "struct config_alias_data {"
> 
> which I can tweak while applying.
Please do.
> Other than that, everything looks good.

Thanks, Dscho

Jeff King· Jun 15, 2017, 06:37 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4 0/6] Avoid problem where git_dir is set after alias expansion

On Wed, Jun 14, 2017 at 01:35:22PM +0200, Johannes Schindelin wrote:
Show 7 quoted lines
> Changes since v3:
> 
> - avoided the strbuf in alias_lookup() by using skip_prefix() and !strcmp()
>   instead.
> 
> - fixed tyop ("read" instead of "reading", causing Junio to stumble) in the
>   commit message of 6/6.

Thanks, this whole series looks good to me (and it sounds like Brandon will address the common-dir thing on top).

-Peff

← back to recent threads