[PATCH v3 0/6] Avoid problem where git_dir is set after alias expansion

[PATCH v3 0/6] Avoid problem where git_dir is set after alias expansion

[Johannes Schindelin]

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 logic is a bit complicated what is the expected current
working directory after expanding an alias and before actually running
it.

That is why this patch series had to expand the signature of the early
config machinery to return the additional information for aliases'
benefit.

Changes since v2:

- fixed tyop in the commit message where "line number" was lacking the
  first "e"

- added a test for the "report line number" fix, and test the `ret` variable
  to be non-negative (instead of zero)

- dropped 'read_early_config(): optionally return the worktree's
  top-level directory' as well as most parts of 'alias_lookup():
  optionally return top-level directory', as we have to run
  setup_git_directory() in the shell alias code path anyway


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                | 31 +++++++++++++++++++++-------
 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, 52 insertions(+), 61 deletions(-)


base-commit: 41dd4330a1210003bd702ec4a9301ed68e60864d
Published-As: https://github.com/dscho/git/releases/tag/alias-early-config-v3
Fetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v3

Interdiff vs v2:
 $(git diff $rebasedtag..$branchname)
-- 
2.13.0.windows.1.460.g13f583bedb5

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

[Johannes Schindelin]

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(+)

diff --git a/setup.c b/setup.c
index e3f7699a902..2435186e448 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.0.windows.1.460.g13f583bedb5

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

[Johannes Schindelin]

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(-)

diff --git a/config.c b/config.c
index 146cb3452ad..12c172e298a 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.0.windows.1.460.g13f583bedb5

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

[Johannes Schindelin]

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(-)

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.0.windows.1.460.g13f583bedb5

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

[Johannes Schindelin]

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(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ff50960ccae..69a0aa56d6d 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -215,7 +215,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.0.windows.1.460.g13f583bedb5

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

[Johannes Schindelin]

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(+)

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.0.windows.1.460.g13f583bedb5

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

[Johannes Schindelin]

Instead of discovering the .git/ directory, read 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.

Also, one might be tempted to streamline the code in alias_lookup() to
*not* use a strbuf for the key. However, if the config reports an error,
it is far superior to tell the user that the `alias.xyz` key had a
problem than to claim that it was the `xyz` key.

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          | 31 ++++++++++++++++++++++++-------
 git.c            | 55 ++++---------------------------------------------------
 t/t7006-pager.sh |  2 +-
 3 files changed, 29 insertions(+), 59 deletions(-)

diff --git a/alias.c b/alias.c
index 3b90397a99d..6bdc9363037 100644
--- a/alias.c
+++ b/alias.c
@@ -1,14 +1,31 @@
 #include "cache.h"
 
+struct config_alias_data
+{
+	struct strbuf key;
+	char *v;
+};
+
+static int config_alias_cb(const char *key, const char *value, void *d)
+{
+	struct config_alias_data *data = d;
+
+	if (!strcmp(key, data->key.buf))
+		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 = { STRBUF_INIT, 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);
+
+	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.0.windows.1.460.g13f583bedb5

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

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Instead of discovering the .git/ directory, read 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.

s/read/&ing/, I think.  My reading hiccupped while trying to figure
out what two alternative approaches are being compared.

> 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.

Makes sense.  Nicely explained.

> Also, one might be tempted to streamline the code in alias_lookup() to
> *not* use a strbuf for the key. However, if the config reports an error,
> it is far superior to tell the user that the `alias.xyz` key had a
> problem than to claim that it was the `xyz` key.

The mention of "streamline" is puzzling to me.  When we are trying
"git xyz", "alias.xyz" is the key we would look up, not "xyz"; it is
not clear to anybody who didn't read the discussion on v2 (which
includes the readers of "git log" in a few months) what kind of flawed
streamlining could look up "xyz" and result in a bad configuration
reported on it.

>  alias.c          | 31 ++++++++++++++++++++++++-------
>  git.c            | 55 ++++---------------------------------------------------
>  t/t7006-pager.sh |  2 +-
>  3 files changed, 29 insertions(+), 59 deletions(-)

Happy to see the deletion of all the save/restore-env stuff.

Except for the puzzlement in one paragraph in the log, looks very
good.  Thanks for a pleasant reading.

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

[Brandon Williams]

On 06/13, Johannes Schindelin wrote:
> Instead of discovering the .git/ directory, read 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.
> 
> Also, one might be tempted to streamline the code in alias_lookup() to
> *not* use a strbuf for the key. However, if the config reports an error,
> it is far superior to tell the user that the `alias.xyz` key had a
> problem than to claim that it was the `xyz` key.
> 
> 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>

So because I've been looking at the config machinery lately, I've
noticed a lot of issues with how things are handled with respect to
gitdir vs commondir.  Essentially the config resides at commondir/config
always, and only at gitdir/config when not working with a worktree.
Because of this, your patches point out a bug in how early config is
handled.  I'll illustrate this using aliases.

Before this series (because aliases are read using the standard config
machinery):

  > git init main
  > git -C main config alias.test '!echo hello'
  > git -C main test
    hello
  > git -C main worktree add ../worktree
  > git -C worktree test
    hello

After this series (using read_early_config()):

  > git init main
  > git -C main config alias.test '!echo hello'
  > git -C main test
    hello
  > git -C main worktree add ../worktree
  > git -C worktree test
    git: 'test' is not a git command. See 'git --help'.

The issue is that read_early_config passes the gitdir and not the
commondir when reading the config.

The solution would be to add a 'commondir' field to the config_options
struct and populate that before reading the config.  I'm planning on
fixing this in v2 of my config cleanup series which I'll hopefully have
finished by the end of the day.

> ---
>  alias.c          | 31 ++++++++++++++++++++++++-------
>  git.c            | 55 ++++---------------------------------------------------
>  t/t7006-pager.sh |  2 +-
>  3 files changed, 29 insertions(+), 59 deletions(-)
> 
> diff --git a/alias.c b/alias.c
> index 3b90397a99d..6bdc9363037 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -1,14 +1,31 @@
>  #include "cache.h"
>  
> +struct config_alias_data
> +{
> +	struct strbuf key;
> +	char *v;
> +};
> +
> +static int config_alias_cb(const char *key, const char *value, void *d)
> +{
> +	struct config_alias_data *data = d;
> +
> +	if (!strcmp(key, data->key.buf))
> +		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 = { STRBUF_INIT, 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);
> +
> +	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.0.windows.1.460.g13f583bedb5

-- 
Brandon Williams

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

[Jeff King]

On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:

> So because I've been looking at the config machinery lately, I've
> noticed a lot of issues with how things are handled with respect to
> gitdir vs commondir.  Essentially the config resides at commondir/config
> always, and only at gitdir/config when not working with a worktree.
> Because of this, your patches point out a bug in how early config is
> handled.  I'll illustrate this using aliases.
> 
> Before this series (because aliases are read using the standard config
> machinery):
> 
>   > git init main
>   > git -C main config alias.test '!echo hello'
>   > git -C main test
>     hello
>   > git -C main worktree add ../worktree
>   > git -C worktree test
>     hello
> 
> After this series (using read_early_config()):
> 
>   > git init main
>   > git -C main config alias.test '!echo hello'
>   > git -C main test
>     hello
>   > git -C main worktree add ../worktree
>   > git -C worktree test
>     git: 'test' is not a git command. See 'git --help'.
> 
> The issue is that read_early_config passes the gitdir and not the
> commondir when reading the config.

Good catch.

> The solution would be to add a 'commondir' field to the config_options
> struct and populate that before reading the config.  I'm planning on
> fixing this in v2 of my config cleanup series which I'll hopefully have
> finished by the end of the day.

I think that read_early_config() really meant to set the commondir, as
it was always about actual config-file lookup. So it was sort-of buggy
already, though I suspect it was pretty hard to trigger.

But I agree that since include_by_gitdir now reads the same struct
field, swapping it out fixes the config-reading at the cost of breaking
that function. And we really need to pass both in.

I'm not sure whether we should care about this for Dscho's series or
not. I think his series _does_ make the problem easier to trigger. But
it's a minor enough bug that I think I'd be OK with letting your
solution proceed independently.

-Peff

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

[Jeff King]

On Tue, Jun 13, 2017 at 09:21:30AM -0700, Junio C Hamano wrote:

> >  alias.c          | 31 ++++++++++++++++++++++++-------
> >  git.c            | 55 ++++---------------------------------------------------
> >  t/t7006-pager.sh |  2 +-
> >  3 files changed, 29 insertions(+), 59 deletions(-)
> 
> Happy to see the deletion of all the save/restore-env stuff.
> 
> Except for the puzzlement in one paragraph in the log, looks very
> good.  Thanks for a pleasant reading.

The whole thing looks good to me, too, though we should decide what to
do with the point that Brandon raised.

As far as the "streamline" puzzlement goes, I'm OK with either of:

  - taking your suggestion from the other thread and actually doing that
    streamlining, which is one less thing to have to explain

  - leaving the code as-is and tweaking the commit message (though it
    made sense to me)

-Peff

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

[Johannes Schindelin]

Hi Junio,

On Tue, 13 Jun 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Instead of discovering the .git/ directory, read 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.
> 
> s/read/&ing/, I think.  My reading hiccupped while trying to figure
> out what two alternative approaches are being compared.

Whoa. Brainfart on my side. Sorry.

> > Also, one might be tempted to streamline the code in alias_lookup() to
> > *not* use a strbuf for the key. However, if the config reports an error,
> > it is far superior to tell the user that the `alias.xyz` key had a
> > problem than to claim that it was the `xyz` key.
> 
> The mention of "streamline" is puzzling to me.  When we are trying
> "git xyz", "alias.xyz" is the key we would look up, not "xyz"; it is
> not clear to anybody who didn't read the discussion on v2 (which
> includes the readers of "git log" in a few months) what kind of flawed
> streamlining could look up "xyz" and result in a bad configuration
> reported on it.

As I changed it (thanks for pointing out my mistake to assume that
skip_prefix() has to change the pointer passed as first parameter), this
paragraph is now gone anyway.

> >  alias.c          | 31 ++++++++++++++++++++++++-------
> >  git.c            | 55 ++++---------------------------------------------------
> >  t/t7006-pager.sh |  2 +-
> >  3 files changed, 29 insertions(+), 59 deletions(-)
> 
> Happy to see the deletion of all the save/restore-env stuff.

Yep.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Peff,

On Wed, 14 Jun 2017, Jeff King wrote:

> On Tue, Jun 13, 2017 at 09:21:30AM -0700, Junio C Hamano wrote:
> 
> > >  alias.c          | 31 ++++++++++++++++++++++++-------
> > >  git.c            | 55 ++++---------------------------------------------------
> > >  t/t7006-pager.sh |  2 +-
> > >  3 files changed, 29 insertions(+), 59 deletions(-)
> > 
> > Happy to see the deletion of all the save/restore-env stuff.
> > 
> > Except for the puzzlement in one paragraph in the log, looks very
> > good.  Thanks for a pleasant reading.
> 
> The whole thing looks good to me, too, though we should decide what to
> do with the point that Brandon raised.
> 
> As far as the "streamline" puzzlement goes, I'm OK with either of:
> 
>   - taking your suggestion from the other thread and actually doing that
>     streamlining, which is one less thing to have to explain
> 
>   - leaving the code as-is and tweaking the commit message (though it
>     made sense to me)

I fixed the thing in the way you suggested (which worked after Junio hit
me with the cluebat about skip_prefix() and git_config_string()).

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Peff & Brandon,

On Wed, 14 Jun 2017, Jeff King wrote:

> On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:
> 
> > So because I've been looking at the config machinery lately, I've
> > noticed a lot of issues with how things are handled with respect to
> > gitdir vs commondir.  Essentially the config resides at commondir/config
> > always, and only at gitdir/config when not working with a worktree.
> > Because of this, your patches point out a bug in how early config is
> > handled.  I'll illustrate this using aliases.
> > 
> > Before this series (because aliases are read using the standard config
> > machinery):
> > 
> >   > git init main
> >   > git -C main config alias.test '!echo hello'
> >   > git -C main test
> >     hello
> >   > git -C main worktree add ../worktree
> >   > git -C worktree test
> >     hello
> > 
> > After this series (using read_early_config()):
> > 
> >   > git init main
> >   > git -C main config alias.test '!echo hello'
> >   > git -C main test
> >     hello
> >   > git -C main worktree add ../worktree
> >   > git -C worktree test
> >     git: 'test' is not a git command. See 'git --help'.
> > 
> > The issue is that read_early_config passes the gitdir and not the
> > commondir when reading the config.
> 
> Good catch.

Oh wow.

> > The solution would be to add a 'commondir' field to the config_options
> > struct and populate that before reading the config.  I'm planning on
> > fixing this in v2 of my config cleanup series which I'll hopefully have
> > finished by the end of the day.
> 
> I think that read_early_config() really meant to set the commondir, as
> it was always about actual config-file lookup. So it was sort-of buggy
> already, though I suspect it was pretty hard to trigger.
> 
> But I agree that since include_by_gitdir now reads the same struct
> field, swapping it out fixes the config-reading at the cost of breaking
> that function. And we really need to pass both in.
> 
> I'm not sure whether we should care about this for Dscho's series or
> not. I think his series _does_ make the problem easier to trigger. But
> it's a minor enough bug that I think I'd be OK with letting your
> solution proceed independently.

Brandon, how hard would it be to build on top of my series? I ask because
I have to take care of some other things and would not have the time to
adjust my patch series on top of yours anytime soon.

Ciao,
Dscho

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

[Brandon Williams]

On 06/14, Johannes Schindelin wrote:
> Hi Peff & Brandon,
> 
> On Wed, 14 Jun 2017, Jeff King wrote:
> 
> > On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:
> > 
> > > So because I've been looking at the config machinery lately, I've
> > > noticed a lot of issues with how things are handled with respect to
> > > gitdir vs commondir.  Essentially the config resides at commondir/config
> > > always, and only at gitdir/config when not working with a worktree.
> > > Because of this, your patches point out a bug in how early config is
> > > handled.  I'll illustrate this using aliases.
> > > 
> > > Before this series (because aliases are read using the standard config
> > > machinery):
> > > 
> > >   > git init main
> > >   > git -C main config alias.test '!echo hello'
> > >   > git -C main test
> > >     hello
> > >   > git -C main worktree add ../worktree
> > >   > git -C worktree test
> > >     hello
> > > 
> > > After this series (using read_early_config()):
> > > 
> > >   > git init main
> > >   > git -C main config alias.test '!echo hello'
> > >   > git -C main test
> > >     hello
> > >   > git -C main worktree add ../worktree
> > >   > git -C worktree test
> > >     git: 'test' is not a git command. See 'git --help'.
> > > 
> > > The issue is that read_early_config passes the gitdir and not the
> > > commondir when reading the config.
> > 
> > Good catch.
> 
> Oh wow.
> 
> > > The solution would be to add a 'commondir' field to the config_options
> > > struct and populate that before reading the config.  I'm planning on
> > > fixing this in v2 of my config cleanup series which I'll hopefully have
> > > finished by the end of the day.
> > 
> > I think that read_early_config() really meant to set the commondir, as
> > it was always about actual config-file lookup. So it was sort-of buggy
> > already, though I suspect it was pretty hard to trigger.
> > 
> > But I agree that since include_by_gitdir now reads the same struct
> > field, swapping it out fixes the config-reading at the cost of breaking
> > that function. And we really need to pass both in.
> > 
> > I'm not sure whether we should care about this for Dscho's series or
> > not. I think his series _does_ make the problem easier to trigger. But
> > it's a minor enough bug that I think I'd be OK with letting your
> > solution proceed independently.
> 
> Brandon, how hard would it be to build on top of my series? I ask because
> I have to take care of some other things and would not have the time to
> adjust my patch series on top of yours anytime soon.

It should be pretty easy to just rebase ontop of your series.  I'll make
sure to do that before sending out the next revision of mine.

-- 
Brandon Williams