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

[PATCH 0/9] 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.


Johannes Schindelin (9):
  discover_git_directory(): avoid setting invalid git_dir
  config: report correct line number upon error
  help: use early config when autocorrecting aliases
  read_early_config(): optionally return the worktree's top-level
    directory
  t1308: relax the test verifying that empty alias values are disallowed
  t7006: demonstrate a problem with aliases in subdirectories
  alias_lookup(): optionally return top-level directory
  Use the early config machinery to expand aliases
  TODO:

 alias.c                | 33 +++++++++++++++++++++-------
 builtin/help.c         |  2 +-
 cache.h                |  7 +++---
 config.c               |  7 +++---
 git.c                  | 59 ++++++--------------------------------------------
 help.c                 |  2 +-
 pager.c                |  4 ++--
 setup.c                | 13 +++++++++--
 t/helper/test-config.c |  2 +-
 t/t1308-config-set.sh  |  4 +++-
 t/t7006-pager.sh       | 11 ++++++++++
 11 files changed, 70 insertions(+), 74 deletions(-)


base-commit: 8d1b10321b20bd2a73a5b561cfc3cf2e8051b70b
Published-As: https://github.com/dscho/git/releases/tag/alias-early-config-v1
Fetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v1
-- 
2.13.0.windows.1.460.g13f583bedb5

[PATCH 1/9] 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.

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 2/9] 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 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 146cb3452ad..9b88531a70d 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)
+		cf->linenr++;
 	return ret;
 }
 
-- 
2.13.0.windows.1.460.g13f583bedb5

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

[Johannes Schindelin]

Git has this feature where suggests similar commands (including aliases)
in case that 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 4/9] read_early_config(): optionally return the worktree's top-level directory

[Johannes Schindelin]

So far, when we invoked the early config code path, we implicitly
determined the top-level directory of the worktree while discovering the
.git/ directory.

And then we simply forgot that information.

However, when we expand aliases, we very much need that information, as
aliases expanding to shell commands, i.e. whose value starts with an
exclamation point, have to be executed in the top-level directory of the
worktree. There are exceptions abound, not only with non-shell aliases
(which are supposed to be executed in the original working directory
instead), but also when being started inside the .git/ directory or in a
worktree created via `git worktree add`.

In preparation for allowing the alias machinery to make use of the early
config machinery, let's add an optional strbuf parameter to the
read_early_config() function; if not NULL, the path of said top-level
directory is appended to the strbuf. As a special case, nothing is
appended if setup_git_directory() would have restored the original
working directory before returning.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 cache.h                |  5 +++--
 config.c               |  4 ++--
 help.c                 |  2 +-
 pager.c                |  4 ++--
 setup.c                | 12 ++++++++++--
 t/helper/test-config.c |  2 +-
 6 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 4d92aae0e81..a8bbddf5955 100644
--- a/cache.h
+++ b/cache.h
@@ -530,7 +530,7 @@ extern void setup_work_tree(void);
  * appended to gitdir. The return value is either NULL if no repository was
  * found, or pointing to the path inside gitdir's buffer.
  */
-extern const char *discover_git_directory(struct strbuf *gitdir);
+extern const char *discover_git_directory(struct strbuf *gitdir, struct strbuf *worktree_dir);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
 extern char *prefix_path(const char *prefix, int len, const char *path);
@@ -1913,7 +1913,8 @@ extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,
 				     const unsigned char *sha1, void *data);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern void read_early_config(config_fn_t cb, void *data);
+extern void read_early_config(config_fn_t cb, void *data,
+			      struct strbuf *worktree_dir);
 extern void git_config(config_fn_t fn, void *);
 extern int git_config_with_options(config_fn_t fn, void *,
 				   struct git_config_source *config_source,
diff --git a/config.c b/config.c
index 9b88531a70d..3d78c11fc00 100644
--- a/config.c
+++ b/config.c
@@ -1651,7 +1651,7 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
 	}
 }
 
-void read_early_config(config_fn_t cb, void *data)
+void read_early_config(config_fn_t cb, void *data, struct strbuf *worktree_dir)
 {
 	struct config_options opts = {0};
 	struct strbuf buf = STRBUF_INIT;
@@ -1668,7 +1668,7 @@ void read_early_config(config_fn_t cb, void *data)
 	 * notably, the current working directory is still the same after the
 	 * call).
 	 */
-	else if (discover_git_directory(&buf))
+	else if (discover_git_directory(&buf, worktree_dir))
 		opts.git_dir = buf.buf;
 
 	git_config_with_options(cb, data, NULL, &opts);
diff --git a/help.c b/help.c
index b44c55ec2da..f78747e8413 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));
 
-	read_early_config(git_unknown_cmd_config, NULL);
+	read_early_config(git_unknown_cmd_config, NULL, NULL);
 
 	load_command_list("git-", &main_cmds, &other_cmds);
 
diff --git a/pager.c b/pager.c
index c113d898a4a..857cf5ecb32 100644
--- a/pager.c
+++ b/pager.c
@@ -53,7 +53,7 @@ const char *git_pager(int stdout_is_tty)
 	pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
-			read_early_config(core_pager_config, NULL);
+			read_early_config(core_pager_config, NULL, NULL);
 		pager = pager_program;
 	}
 	if (!pager)
@@ -214,7 +214,7 @@ int check_pager_config(const char *cmd)
 	data.want = -1;
 	data.value = NULL;
 
-	read_early_config(pager_command_config, &data);
+	read_early_config(pager_command_config, &data, NULL);
 
 	if (data.value)
 		pager_program = data.value;
diff --git a/setup.c b/setup.c
index 2435186e448..771822fd0ca 100644
--- a/setup.c
+++ b/setup.c
@@ -945,10 +945,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
 	}
 }
 
-const char *discover_git_directory(struct strbuf *gitdir)
+const char *discover_git_directory(struct strbuf *gitdir,
+				   struct strbuf *worktree_dir)
 {
 	struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
-	size_t gitdir_offset = gitdir->len, cwd_len;
+	size_t gitdir_offset = gitdir->len, cwd_len, worktree_dir_offset;
 	struct repository_format candidate;
 
 	if (strbuf_getcwd(&dir))
@@ -973,6 +974,11 @@ const char *discover_git_directory(struct strbuf *gitdir)
 		strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
 	}
 
+	if (worktree_dir) {
+		worktree_dir_offset = worktree_dir->len;
+		strbuf_addbuf(worktree_dir, &dir);
+	}
+
 	strbuf_reset(&dir);
 	strbuf_addf(&dir, "%s/config", gitdir->buf + gitdir_offset);
 	read_repository_format(&candidate, dir.buf);
@@ -983,6 +989,8 @@ const char *discover_git_directory(struct strbuf *gitdir)
 			gitdir->buf + gitdir_offset, err.buf);
 		strbuf_release(&err);
 		strbuf_setlen(gitdir, gitdir_offset);
+		if (worktree_dir)
+			strbuf_setlen(worktree_dir, worktree_dir_offset);
 		return NULL;
 	}
 
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 8e3ed6a76cb..5e1e78f8fa2 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -84,7 +84,7 @@ int cmd_main(int argc, const char **argv)
 	struct config_set cs;
 
 	if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
-		read_early_config(early_config_cb, (void *)argv[2]);
+		read_early_config(early_config_cb, (void *)argv[2], NULL);
 		return 0;
 	}
 
-- 
2.13.0.windows.1.460.g13f583bedb5

[PATCH 5/9] 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 lin 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 6/9] 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 7/9] alias_lookup(): optionally return top-level directory

[Johannes Schindelin]

When an alias expands to a shell command, and when we are inside a
regular worktree's subdirectory (i.e. not inside the .git/ directory nor
in a worktree initialized via `git worktree add`), and only then, the
current working directory is switched to the top-level directory of the
worktree before running the expanded alias.

While this behavior is somewhat confusing, it is well-established and
needs to be preserved, even with the upcoming change to use the early
config machinery to expand aliases.

That means that alias_lookup() needs to determine the working directory
to switch to, in case the alias turns out to expand to a shall command.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 alias.c        | 33 +++++++++++++++++++++++++--------
 builtin/help.c |  2 +-
 cache.h        |  2 +-
 git.c          |  2 +-
 4 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/alias.c b/alias.c
index 3b90397a99d..c1b87154944 100644
--- a/alias.c
+++ b/alias.c
@@ -1,14 +1,31 @@
 #include "cache.h"
 
-char *alias_lookup(const char *alias)
+struct config_alias_data
 {
-	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 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, struct strbuf *worktree_dir)
+{
+	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, worktree_dir);
+	strbuf_release(&data.key);
+
+	return data.v;
 }
 
 #define SPLIT_CMDLINE_BAD_ENDING 1
diff --git a/builtin/help.c b/builtin/help.c
index 49f7a07f85d..6f208ff1ab3 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -435,7 +435,7 @@ static const char *check_git_cmd(const char* cmd)
 	if (is_git_command(cmd))
 		return cmd;
 
-	alias = alias_lookup(cmd);
+	alias = alias_lookup(cmd, NULL);
 	if (alias) {
 		printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
 		free(alias);
diff --git a/cache.h b/cache.h
index a8bbddf5955..77ce8e8c80b 100644
--- a/cache.h
+++ b/cache.h
@@ -2189,7 +2189,7 @@ extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
 /* ls-files */
 void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 
-char *alias_lookup(const char *alias);
+char *alias_lookup(const char *alias, struct strbuf *worktree_dir);
 int split_cmdline(char *cmdline, const char ***argv);
 /* Takes a negative value returned by split_cmdline */
 const char *split_cmdline_strerror(int cmdline_errno);
diff --git a/git.c b/git.c
index 8ff44f081d4..4163beaead4 100644
--- a/git.c
+++ b/git.c
@@ -256,7 +256,7 @@ static int handle_alias(int *argcp, const char ***argv)
 	setup_git_directory_gently(&unused_nongit);
 
 	alias_command = (*argv)[0];
-	alias_string = alias_lookup(alias_command);
+	alias_string = alias_lookup(alias_command, NULL);
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			struct child_process child = CHILD_PROCESS_INIT;
-- 
2.13.0.windows.1.460.g13f583bedb5

[PATCH 8/9] Use the early config machinery to expand aliases

[Johannes Schindelin]

We already taught alias_lookup() to use the early config if the .git/
directory was not yet discovered, of course, however, since we called
setup_git_directory_gently() before expanding the alias (if any), we
only used the early config code path only if outside of any Git-managed
directory.

With this commit, we finally switch over to really using the early
config code path.

Rather than just chdir()ing into the indicated directory in case of an
alias expanding to a shell command, we simply set up the .git/ directory
so that e.g. GIT_PREFIX is set as expected.

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

diff --git a/git.c b/git.c
index 4163beaead4..c82cd455948 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) {
@@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 
 static int handle_alias(int *argcp, const char ***argv)
 {
+	struct strbuf worktree_dir = STRBUF_INIT;
 	int envchanged = 0, ret = 0, saved_errno = errno;
 	int count, option_count;
 	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, NULL);
+	alias_string = alias_lookup(alias_command, &worktree_dir);
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			struct child_process child = CHILD_PROCESS_INIT;
 
+			if (worktree_dir.len)
+				setup_git_directory();
+
 			commit_pager_choice();
-			restore_env(1);
 
 			child.use_shell = 1;
 			argv_array_push(&child.args, alias_string + 1);
 			argv_array_pushv(&child.args, (*argv) + 1);
 
 			ret = run_command(&child);
+			strbuf_release(&worktree_dir);
 			if (ret >= 0)   /* normal exit */
 				exit(ret);
 
 			die_errno("While expanding alias '%s': '%s'",
 			    alias_command, alias_string + 1);
 		}
+		strbuf_release(&worktree_dir);
 		count = split_cmdline(alias_string, &new_argv);
 		if (count < 0)
 			die("Bad alias.%s string: %s", alias_command,
@@ -308,8 +265,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 0/9] Avoid problem where git_dir is set after alias expansion

[Johannes Schindelin]

Hi,

On Wed, 7 Jun 2017, Johannes Schindelin wrote:

> Johannes Schindelin (9):
>   discover_git_directory(): avoid setting invalid git_dir
>   config: report correct line number upon error
>   help: use early config when autocorrecting aliases
>   read_early_config(): optionally return the worktree's top-level
>     directory
>   t1308: relax the test verifying that empty alias values are disallowed
>   t7006: demonstrate a problem with aliases in subdirectories
>   alias_lookup(): optionally return top-level directory
>   Use the early config machinery to expand aliases
>   TODO:

The 9/9 is obviously an empty commit that I used to track things still to
do. Please ignore that "TODO:" entry and rest assured that there is no 9/9
in this first iteration of the patch series.

Ciao,
Johannes

Re: [PATCH 1/9] discover_git_directory(): avoid setting invalid git_dir

[Brandon Williams]

On 06/07, Johannes Schindelin wrote:
> 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.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Seems sane.  This way the strbuf is in the same state it was in before
calling this function (upon a failure that is).

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

There is another part of this function that returns NULL (which isn't
shown by this diff) after performing 'setup_git_dir_gently_1', do we
need to worry about anything that 'setup_git_dir_gently_1' has
potentially appended to 'gitdir' upon 'setup_git_dir_gently_1' failing?

>  	}
>  
> -- 
> 2.13.0.windows.1.460.g13f583bedb5
> 
> 

-- 
Brandon Williams

Re: [PATCH 3/9] help: use early config when autocorrecting aliases

[Brandon Williams]

On 06/07, Johannes Schindelin wrote:
> Git has this feature where suggests similar commands (including aliases)

nit: s/where/which/

> in case that the user specified an unknown command.

s/that//

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

Agreed, and it is more important to be correct than performant.

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

-- 
Brandon Williams

Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory

[Brandon Williams]

On 06/07, Johannes Schindelin wrote:
> So far, when we invoked the early config code path, we implicitly
> determined the top-level directory of the worktree while discovering the
> .git/ directory.
> 
> And then we simply forgot that information.
> 
> However, when we expand aliases, we very much need that information, as
> aliases expanding to shell commands, i.e. whose value starts with an
> exclamation point, have to be executed in the top-level directory of the
> worktree. There are exceptions abound, not only with non-shell aliases
> (which are supposed to be executed in the original working directory
> instead), but also when being started inside the .git/ directory or in a
> worktree created via `git worktree add`.
> 
> In preparation for allowing the alias machinery to make use of the early
> config machinery, let's add an optional strbuf parameter to the
> read_early_config() function; if not NULL, the path of said top-level
> directory is appended to the strbuf. As a special case, nothing is
> appended if setup_git_directory() would have restored the original
> working directory before returning.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  cache.h                |  5 +++--
>  config.c               |  4 ++--
>  help.c                 |  2 +-
>  pager.c                |  4 ++--
>  setup.c                | 12 ++++++++++--
>  t/helper/test-config.c |  2 +-
>  6 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 4d92aae0e81..a8bbddf5955 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -530,7 +530,7 @@ extern void setup_work_tree(void);
>   * appended to gitdir. The return value is either NULL if no repository was
>   * found, or pointing to the path inside gitdir's buffer.
>   */
> -extern const char *discover_git_directory(struct strbuf *gitdir);
> +extern const char *discover_git_directory(struct strbuf *gitdir, struct strbuf *worktree_dir);
>  extern const char *setup_git_directory_gently(int *);
>  extern const char *setup_git_directory(void);
>  extern char *prefix_path(const char *prefix, int len, const char *path);
> @@ -1913,7 +1913,8 @@ extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,
>  				     const unsigned char *sha1, void *data);
>  extern void git_config_push_parameter(const char *text);
>  extern int git_config_from_parameters(config_fn_t fn, void *data);
> -extern void read_early_config(config_fn_t cb, void *data);
> +extern void read_early_config(config_fn_t cb, void *data,
> +			      struct strbuf *worktree_dir);
>  extern void git_config(config_fn_t fn, void *);
>  extern int git_config_with_options(config_fn_t fn, void *,
>  				   struct git_config_source *config_source,
> diff --git a/config.c b/config.c
> index 9b88531a70d..3d78c11fc00 100644
> --- a/config.c
> +++ b/config.c
> @@ -1651,7 +1651,7 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
>  	}
>  }
>  
> -void read_early_config(config_fn_t cb, void *data)
> +void read_early_config(config_fn_t cb, void *data, struct strbuf *worktree_dir)
>  {
>  	struct config_options opts = {0};
>  	struct strbuf buf = STRBUF_INIT;
> @@ -1668,7 +1668,7 @@ void read_early_config(config_fn_t cb, void *data)
>  	 * notably, the current working directory is still the same after the
>  	 * call).
>  	 */
> -	else if (discover_git_directory(&buf))
> +	else if (discover_git_directory(&buf, worktree_dir))
>  		opts.git_dir = buf.buf;

It feels kind of weird to get back worktree info after calling
read_early_config but I understand why you need to get it.  One thing to
consider is the 'if' statement not shown here since you aren't adding any
worktree information if that branch is taken.  Maybe we can drop the
first if statement all together as 'read_early_config' is used before
setup is run and it should really only be triggered when setup has been
run.

>  
>  	git_config_with_options(cb, data, NULL, &opts);
> diff --git a/help.c b/help.c
> index b44c55ec2da..f78747e8413 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));
>  
> -	read_early_config(git_unknown_cmd_config, NULL);
> +	read_early_config(git_unknown_cmd_config, NULL, NULL);
>  
>  	load_command_list("git-", &main_cmds, &other_cmds);
>  
> diff --git a/pager.c b/pager.c
> index c113d898a4a..857cf5ecb32 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -53,7 +53,7 @@ const char *git_pager(int stdout_is_tty)
>  	pager = getenv("GIT_PAGER");
>  	if (!pager) {
>  		if (!pager_program)
> -			read_early_config(core_pager_config, NULL);
> +			read_early_config(core_pager_config, NULL, NULL);
>  		pager = pager_program;
>  	}
>  	if (!pager)
> @@ -214,7 +214,7 @@ int check_pager_config(const char *cmd)
>  	data.want = -1;
>  	data.value = NULL;
>  
> -	read_early_config(pager_command_config, &data);
> +	read_early_config(pager_command_config, &data, NULL);
>  
>  	if (data.value)
>  		pager_program = data.value;
> diff --git a/setup.c b/setup.c
> index 2435186e448..771822fd0ca 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -945,10 +945,11 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  	}
>  }
>  
> -const char *discover_git_directory(struct strbuf *gitdir)
> +const char *discover_git_directory(struct strbuf *gitdir,
> +				   struct strbuf *worktree_dir)
>  {
>  	struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
> -	size_t gitdir_offset = gitdir->len, cwd_len;
> +	size_t gitdir_offset = gitdir->len, cwd_len, worktree_dir_offset;
>  	struct repository_format candidate;
>  
>  	if (strbuf_getcwd(&dir))
> @@ -973,6 +974,11 @@ const char *discover_git_directory(struct strbuf *gitdir)
>  		strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
>  	}
>  
> +	if (worktree_dir) {
> +		worktree_dir_offset = worktree_dir->len;
> +		strbuf_addbuf(worktree_dir, &dir);
> +	}
> +
>  	strbuf_reset(&dir);
>  	strbuf_addf(&dir, "%s/config", gitdir->buf + gitdir_offset);
>  	read_repository_format(&candidate, dir.buf);
> @@ -983,6 +989,8 @@ const char *discover_git_directory(struct strbuf *gitdir)
>  			gitdir->buf + gitdir_offset, err.buf);
>  		strbuf_release(&err);
>  		strbuf_setlen(gitdir, gitdir_offset);
> +		if (worktree_dir)
> +			strbuf_setlen(worktree_dir, worktree_dir_offset);
>  		return NULL;
>  	}
>  
> diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> index 8e3ed6a76cb..5e1e78f8fa2 100644
> --- a/t/helper/test-config.c
> +++ b/t/helper/test-config.c
> @@ -84,7 +84,7 @@ int cmd_main(int argc, const char **argv)
>  	struct config_set cs;
>  
>  	if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
> -		read_early_config(early_config_cb, (void *)argv[2]);
> +		read_early_config(early_config_cb, (void *)argv[2], NULL);
>  		return 0;
>  	}
>  
> -- 
> 2.13.0.windows.1.460.g13f583bedb5
> 
> 

-- 
Brandon Williams

Re: [PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed

[Brandon Williams]

On 06/07, Johannes Schindelin wrote:
> 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.

Not a comment on the patch but just a genuine question: Is there any
reason why they complain in a different way?  Doesn't it make sense for
the errors to be reported consistently?

> 
> Let's not get hung up by the precise wording of the message mentioning
> the lin 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
> 
> 

-- 
Brandon Williams

Re: [PATCH 8/9] Use the early config machinery to expand aliases

[Brandon Williams]

On 06/07, Johannes Schindelin wrote:
> We already taught alias_lookup() to use the early config if the .git/
> directory was not yet discovered, of course, however, since we called
> setup_git_directory_gently() before expanding the alias (if any), we
> only used the early config code path only if outside of any Git-managed
> directory.
> 
> With this commit, we finally switch over to really using the early
> config code path.
> 
> Rather than just chdir()ing into the indicated directory in case of an
> alias expanding to a shell command, we simply set up the .git/ directory
> so that e.g. GIT_PREFIX is set as expected.
> 
> 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>
> ---
>  git.c            | 59 +++++++-------------------------------------------------
>  t/t7006-pager.sh |  2 +-
>  2 files changed, 8 insertions(+), 53 deletions(-)
> 
> diff --git a/git.c b/git.c
> index 4163beaead4..c82cd455948 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]);
> -		}
> -	}
> -}

I like seeing chunks of old code being deleted :D

>  
>  static void commit_pager_choice(void) {
>  	switch (use_pager) {
> @@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  
>  static int handle_alias(int *argcp, const char ***argv)
>  {
> +	struct strbuf worktree_dir = STRBUF_INIT;
>  	int envchanged = 0, ret = 0, saved_errno = errno;
>  	int count, option_count;
>  	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, NULL);
> +	alias_string = alias_lookup(alias_command, &worktree_dir);
>  	if (alias_string) {
>  		if (alias_string[0] == '!') {
>  			struct child_process child = CHILD_PROCESS_INIT;
>  
> +			if (worktree_dir.len)
> +				setup_git_directory();

So if there is a worktree then we run the setup code explicitly.  I
assume that is to ensure that all envvars are setup properly before
running the alias.  Just interesting to note that the actual value of
worktree_dir is never used, its just used essentially as a boolean
indicator of there being a worktree/gitdir/repository.  I'm not
suggesting to change from what you have here, its just food for thought.

> +
>  			commit_pager_choice();
> -			restore_env(1);
>  
>  			child.use_shell = 1;
>  			argv_array_push(&child.args, alias_string + 1);
>  			argv_array_pushv(&child.args, (*argv) + 1);
>  
>  			ret = run_command(&child);
> +			strbuf_release(&worktree_dir);
>  			if (ret >= 0)   /* normal exit */
>  				exit(ret);
>  
>  			die_errno("While expanding alias '%s': '%s'",
>  			    alias_command, alias_string + 1);
>  		}
> +		strbuf_release(&worktree_dir);
>  		count = split_cmdline(alias_string, &new_argv);
>  		if (count < 0)
>  			die("Bad alias.%s string: %s", alias_command,
> @@ -308,8 +265,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 0/9] Avoid problem where git_dir is set after alias expansion

[Brandon Williams]

On 06/07, Johannes Schindelin wrote:
> 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.
> 

Looks good, I don't have any major issues with the series, just some
comments for clarity mostly.  And relevant to this series, you may be
interested in looking at patch 03/31 in my repository object series as
that may have an impact on the early config stuff.

> 
> Johannes Schindelin (9):
>   discover_git_directory(): avoid setting invalid git_dir
>   config: report correct line number upon error
>   help: use early config when autocorrecting aliases
>   read_early_config(): optionally return the worktree's top-level
>     directory
>   t1308: relax the test verifying that empty alias values are disallowed
>   t7006: demonstrate a problem with aliases in subdirectories
>   alias_lookup(): optionally return top-level directory
>   Use the early config machinery to expand aliases
>   TODO:
> 
>  alias.c                | 33 +++++++++++++++++++++-------
>  builtin/help.c         |  2 +-
>  cache.h                |  7 +++---
>  config.c               |  7 +++---
>  git.c                  | 59 ++++++--------------------------------------------
>  help.c                 |  2 +-
>  pager.c                |  4 ++--
>  setup.c                | 13 +++++++++--
>  t/helper/test-config.c |  2 +-
>  t/t1308-config-set.sh  |  4 +++-
>  t/t7006-pager.sh       | 11 ++++++++++
>  11 files changed, 70 insertions(+), 74 deletions(-)
> 
> 
> base-commit: 8d1b10321b20bd2a73a5b561cfc3cf2e8051b70b
> Published-As: https://github.com/dscho/git/releases/tag/alias-early-config-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v1
> -- 
> 2.13.0.windows.1.460.g13f583bedb5
> 

-- 
Brandon Williams

Re: [PATCH 1/9] discover_git_directory(): avoid setting invalid git_dir

[Johannes Schindelin]

Hi Brandon,

On Wed, 7 Jun 2017, Brandon Williams wrote:

> On 06/07, Johannes Schindelin wrote:
>
> > 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;
> 
> There is another part of this function that returns NULL (which isn't
> shown by this diff) after performing 'setup_git_dir_gently_1', do we
> need to worry about anything that 'setup_git_dir_gently_1' has
> potentially appended to 'gitdir' upon 'setup_git_dir_gently_1' failing?

The setup_git_dir_gently_1() function used to set the git_dir variable
directly in case of success, and leave it unchanged in case of error. My
work to allow an early config did not change that, with the exception that
it is the gitdir parameter that is changed instead of git_dir.

In other words: that `return NULL` is safe, as it returns due to an error
reported by the setup_git_directory_gently_1() function (meaning that the
gitdir strbuf has not been touched).

I amended the commit message accordingly, and force-pushed the result.
Meaning: v2 will have that change.

Ciao,
Dscho

Re: [PATCH 3/9] help: use early config when autocorrecting aliases

[Johannes Schindelin]

Hi Brandon,

On Wed, 7 Jun 2017, Brandon Williams wrote:

> On 06/07, Johannes Schindelin wrote:
> > Git has this feature where suggests similar commands (including aliases)
> 
> nit: s/where/which/
> 
> > in case that the user specified an unknown command.
> 
> s/that//

Oh my. Thank you so much!
Dscho

Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory

[Johannes Schindelin]

Hi Brandon,

On Wed, 7 Jun 2017, Brandon Williams wrote:

> On 06/07, Johannes Schindelin wrote:
> > @@ -1668,7 +1668,7 @@ void read_early_config(config_fn_t cb, void *data)
> >  	 * notably, the current working directory is still the same after the
> >  	 * call).
> >  	 */
> > -	else if (discover_git_directory(&buf))
> > +	else if (discover_git_directory(&buf, worktree_dir))
> >  		opts.git_dir = buf.buf;
> 
> It feels kind of weird to get back worktree info after calling
> read_early_config but I understand why you need to get it.

Yeah. It is awkward. Required for backwards-compatibility, though (and it
is hard to explain *when* it is needed, and even harder under what
circumstances it is *not* needed even if there is a worktre).

> One thing to consider is the 'if' statement not shown here since you
> aren't adding any worktree information if that branch is taken.

Right. For lurkers, that `if` statement reads thusly:

	if (have_git_dir())
		opts.git_dir = get_git_dir();

> Maybe we can drop the first if statement all together as
> 'read_early_config' is used before setup is run and it should really
> only be triggered when setup has been run.

The `read_early_config()` function is *sometimes* used *after* setup has
run. Look at `run_builtin()` in git.c:

	if (p->option & RUN_SETUP)
		prefix = setup_git_directory();
	else if (p->option & RUN_SETUP_GENTLY) {
		int nongit_ok;
		prefix = setup_git_directory_gently(&nongit_ok);
	}

	if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
		use_pager = check_pager_config(p->cmd);

For builtins having either the RUN_SETUP or the RUN_SETUP_GENTLY flag, we
do not need to re-discover the .git/ directory at all when checking the
pager config.

Back to the worktree_dir variable.

I think part of the confusion here is that it may be left alone even when
there is a worktree. For example, if we are already in the top-level
directory. Or if the worktree somehow points to a different directory than
the one containing the .git/ directory.

Therefore, I renamed this variable to `cdup_dir` to reflect the fact that
it is only touched if Git determines that it is in a subdirectory of the
directory containing the .git/ directory.

Ciao,
Dscho

Re: [PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed

[Johannes Schindelin]

Hi Brandon,

On Wed, 7 Jun 2017, Brandon Williams wrote:

> On 06/07, Johannes Schindelin wrote:
> > 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.
> 
> Not a comment on the patch but just a genuine question: Is there any
> reason why they complain in a different way?  Doesn't it make sense for
> the errors to be reported consistently?

Yes, I agree that they should not complain in different ways.

I had a brief look to see whether I could quickly fix that "while at it".
But it seems to be quite a bit more involved than I am comfortable
slipping into this patch series (whose purpose is not to fix the dichotomy
between direct and cached config parsing, after all).

Ciao,
Dscho

Re: [PATCH 8/9] Use the early config machinery to expand aliases

[Johannes Schindelin]

Hi Brandon,

On Wed, 7 Jun 2017, Brandon Williams wrote:

> I like seeing chunks of old code being deleted :D

Me, too. In particular as hacky code as this one. It caused quite a couple
of gray hairs here.

> On 06/07, Johannes Schindelin wrote:
> 
> > @@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> >  
> >  static int handle_alias(int *argcp, const char ***argv)
> >  {
> > +	struct strbuf worktree_dir = STRBUF_INIT;
> >  	int envchanged = 0, ret = 0, saved_errno = errno;
> >  	int count, option_count;
> >  	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, NULL);
> > +	alias_string = alias_lookup(alias_command, &worktree_dir);
> >  	if (alias_string) {
> >  		if (alias_string[0] == '!') {
> >  			struct child_process child = CHILD_PROCESS_INIT;
> >  
> > +			if (worktree_dir.len)
> > +				setup_git_directory();
> 
> So if there is a worktree then we run the setup code explicitly.  I
> assume that is to ensure that all envvars are setup properly before
> running the alias.  Just interesting to note that the actual value of
> worktree_dir is never used, its just used essentially as a boolean
> indicator of there being a worktree/gitdir/repository.  I'm not
> suggesting to change from what you have here, its just food for thought.

Indeed.

That is what I tried to indicate by this paragraph in the commit message:

	Rather than just chdir()ing into the indicated directory in case of an
	alias expanding to a shell command, we simply set up the .git/ directory
	so that e.g. GIT_PREFIX is set as expected.

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

[Johannes Schindelin]

Hi Brandon,

On Wed, 7 Jun 2017, Brandon Williams wrote:

> Looks good, I don't have any major issues with the series, just some
> comments for clarity mostly.

Thank you for the review!

I will wait a couple of hours to see whether anybody else sees anything
that needs to be changed, before sending out v2.

> And relevant to this series, you may be interested in looking at patch
> 03/31 in my repository object series as that may have an impact on the
> early config stuff.

Thanks for the prod. I will have a specific look at this patch right after
sending this mail.

Ciao,
Dscho

Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory

[Brandon Williams]

On 06/08, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Wed, 7 Jun 2017, Brandon Williams wrote:
> 
> > On 06/07, Johannes Schindelin wrote:
> > > @@ -1668,7 +1668,7 @@ void read_early_config(config_fn_t cb, void *data)
> > >  	 * notably, the current working directory is still the same after the
> > >  	 * call).
> > >  	 */
> > > -	else if (discover_git_directory(&buf))
> > > +	else if (discover_git_directory(&buf, worktree_dir))
> > >  		opts.git_dir = buf.buf;
> > 
> > It feels kind of weird to get back worktree info after calling
> > read_early_config but I understand why you need to get it.
> 
> Yeah. It is awkward. Required for backwards-compatibility, though (and it
> is hard to explain *when* it is needed, and even harder under what
> circumstances it is *not* needed even if there is a worktre).
> 
> > One thing to consider is the 'if' statement not shown here since you
> > aren't adding any worktree information if that branch is taken.
> 
> Right. For lurkers, that `if` statement reads thusly:
> 
> 	if (have_git_dir())
> 		opts.git_dir = get_git_dir();
> 
> > Maybe we can drop the first if statement all together as
> > 'read_early_config' is used before setup is run and it should really
> > only be triggered when setup has been run.
> 
> The `read_early_config()` function is *sometimes* used *after* setup has
> run. Look at `run_builtin()` in git.c:
> 
> 	if (p->option & RUN_SETUP)
> 		prefix = setup_git_directory();
> 	else if (p->option & RUN_SETUP_GENTLY) {
> 		int nongit_ok;
> 		prefix = setup_git_directory_gently(&nongit_ok);
> 	}
> 
> 	if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
> 		use_pager = check_pager_config(p->cmd);
> 

Ah, ok so my comment was incorrect, thanks for pointing this out.

> For builtins having either the RUN_SETUP or the RUN_SETUP_GENTLY flag, we
> do not need to re-discover the .git/ directory at all when checking the
> pager config.
> 
> Back to the worktree_dir variable.
> 
> I think part of the confusion here is that it may be left alone even when
> there is a worktree. For example, if we are already in the top-level
> directory. Or if the worktree somehow points to a different directory than
> the one containing the .git/ directory.
> 
> Therefore, I renamed this variable to `cdup_dir` to reflect the fact that
> it is only touched if Git determines that it is in a subdirectory of the
> directory containing the .git/ directory.

Ok, maybe I'm just not following but just from reading the variable name I can't
really understand what 'cdup_dir' means.

> 
> Ciao,
> Dscho

-- 
Brandon Williams

Re: [PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed

[Brandon Williams]

On 06/08, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Wed, 7 Jun 2017, Brandon Williams wrote:
> 
> > On 06/07, Johannes Schindelin wrote:
> > > 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.
> > 
> > Not a comment on the patch but just a genuine question: Is there any
> > reason why they complain in a different way?  Doesn't it make sense for
> > the errors to be reported consistently?
> 
> Yes, I agree that they should not complain in different ways.
> 
> I had a brief look to see whether I could quickly fix that "while at it".
> But it seems to be quite a bit more involved than I am comfortable
> slipping into this patch series (whose purpose is not to fix the dichotomy
> between direct and cached config parsing, after all).

Fair enough, thanks!

> 
> Ciao,
> Dscho

-- 
Brandon Williams

Re: [PATCH 8/9] Use the early config machinery to expand aliases

[Brandon Williams]

On 06/08, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Wed, 7 Jun 2017, Brandon Williams wrote:
> 
> > I like seeing chunks of old code being deleted :D
> 
> Me, too. In particular as hacky code as this one. It caused quite a couple
> of gray hairs here.
> 
> > On 06/07, Johannes Schindelin wrote:
> > 
> > > @@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> > >  
> > >  static int handle_alias(int *argcp, const char ***argv)
> > >  {
> > > +	struct strbuf worktree_dir = STRBUF_INIT;
> > >  	int envchanged = 0, ret = 0, saved_errno = errno;
> > >  	int count, option_count;
> > >  	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, NULL);
> > > +	alias_string = alias_lookup(alias_command, &worktree_dir);
> > >  	if (alias_string) {
> > >  		if (alias_string[0] == '!') {
> > >  			struct child_process child = CHILD_PROCESS_INIT;
> > >  
> > > +			if (worktree_dir.len)
> > > +				setup_git_directory();
> > 
> > So if there is a worktree then we run the setup code explicitly.  I
> > assume that is to ensure that all envvars are setup properly before
> > running the alias.  Just interesting to note that the actual value of
> > worktree_dir is never used, its just used essentially as a boolean
> > indicator of there being a worktree/gitdir/repository.  I'm not
> > suggesting to change from what you have here, its just food for thought.
> 
> Indeed.
> 
> That is what I tried to indicate by this paragraph in the commit message:
> 
> 	Rather than just chdir()ing into the indicated directory in case of an
> 	alias expanding to a shell command, we simply set up the .git/ directory
> 	so that e.g. GIT_PREFIX is set as expected.

I was more commenting on "oh that's what he meant by that" for my own
understanding.  But thanks again for pointing out that you did include a
comment in the commit message reflecting that.

-- 
Brandon Williams

Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory

[Johannes Schindelin]

Hi Brandon,

On Thu, 8 Jun 2017, Brandon Williams wrote:

> On 06/08, Johannes Schindelin wrote:
> 
> > Back to the worktree_dir variable.
> > 
> > I think part of the confusion here is that it may be left alone even
> > when there is a worktree. For example, if we are already in the
> > top-level directory. Or if the worktree somehow points to a different
> > directory than the one containing the .git/ directory.
> > 
> > Therefore, I renamed this variable to `cdup_dir` to reflect the fact
> > that it is only touched if Git determines that it is in a subdirectory
> > of the directory containing the .git/ directory.
> 
> Ok, maybe I'm just not following but just from reading the variable name
> I can't really understand what 'cdup_dir' means.

My idea would be that this is in line with the

	git rev-parse --show-cdup

incantation.

Ciao,
Dscho

Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory

[Brandon Williams]

On 06/08, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Thu, 8 Jun 2017, Brandon Williams wrote:
> 
> > On 06/08, Johannes Schindelin wrote:
> > 
> > > Back to the worktree_dir variable.
> > > 
> > > I think part of the confusion here is that it may be left alone even
> > > when there is a worktree. For example, if we are already in the
> > > top-level directory. Or if the worktree somehow points to a different
> > > directory than the one containing the .git/ directory.
> > > 
> > > Therefore, I renamed this variable to `cdup_dir` to reflect the fact
> > > that it is only touched if Git determines that it is in a subdirectory
> > > of the directory containing the .git/ directory.
> > 
> > Ok, maybe I'm just not following but just from reading the variable name
> > I can't really understand what 'cdup_dir' means.
> 
> My idea would be that this is in line with the
> 
> 	git rev-parse --show-cdup
> 
> incantation.

Ah ok, 'cdup' just doesn't really mean much to me.  Is it supposed to
stand for something?

-- 
Brandon Williams

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

[Brandon Williams]

On 06/08, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Wed, 7 Jun 2017, Brandon Williams wrote:
> 
> > Looks good, I don't have any major issues with the series, just some
> > comments for clarity mostly.
> 
> Thank you for the review!

Of course!  I've been poking around the setup logic so this was
interesting to me :)

> 
> I will wait a couple of hours to see whether anybody else sees anything
> that needs to be changed, before sending out v2.
> 
> > And relevant to this series, you may be interested in looking at patch
> > 03/31 in my repository object series as that may have an impact on the
> > early config stuff.
> 
> Thanks for the prod. I will have a specific look at this patch right after
> sending this mail.
> 
> Ciao,
> Dscho

-- 
Brandon Williams

Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory

[Johannes Schindelin]

Hi Brandon,

On Thu, 8 Jun 2017, Brandon Williams wrote:

> On 06/08, Johannes Schindelin wrote:
> > 
> > On Thu, 8 Jun 2017, Brandon Williams wrote:
> > 
> > > On 06/08, Johannes Schindelin wrote:
> > > 
> > > > Back to the worktree_dir variable.
> > > > 
> > > > I think part of the confusion here is that it may be left alone even
> > > > when there is a worktree. For example, if we are already in the
> > > > top-level directory. Or if the worktree somehow points to a different
> > > > directory than the one containing the .git/ directory.
> > > > 
> > > > Therefore, I renamed this variable to `cdup_dir` to reflect the fact
> > > > that it is only touched if Git determines that it is in a subdirectory
> > > > of the directory containing the .git/ directory.
> > > 
> > > Ok, maybe I'm just not following but just from reading the variable name
> > > I can't really understand what 'cdup_dir' means.
> > 
> > My idea would be that this is in line with the
> > 
> > 	git rev-parse --show-cdup
> > 
> > incantation.
> 
> Ah ok, 'cdup' just doesn't really mean much to me.  Is it supposed to
> stand for something?

Yes, it stands for "change directory up", as in `cd ../../`. ;-)

Ciao,
Dscho

Re: [PATCH 4/9] read_early_config(): optionally return the worktree's top-level directory

[Brandon Williams]

On 06/08, Johannes Schindelin wrote:
> Hi Brandon,
> 
> On Thu, 8 Jun 2017, Brandon Williams wrote:
> 
> > On 06/08, Johannes Schindelin wrote:
> > > 
> > > On Thu, 8 Jun 2017, Brandon Williams wrote:
> > > 
> > > > On 06/08, Johannes Schindelin wrote:
> > > > 
> > > > > Back to the worktree_dir variable.
> > > > > 
> > > > > I think part of the confusion here is that it may be left alone even
> > > > > when there is a worktree. For example, if we are already in the
> > > > > top-level directory. Or if the worktree somehow points to a different
> > > > > directory than the one containing the .git/ directory.
> > > > > 
> > > > > Therefore, I renamed this variable to `cdup_dir` to reflect the fact
> > > > > that it is only touched if Git determines that it is in a subdirectory
> > > > > of the directory containing the .git/ directory.
> > > > 
> > > > Ok, maybe I'm just not following but just from reading the variable name
> > > > I can't really understand what 'cdup_dir' means.
> > > 
> > > My idea would be that this is in line with the
> > > 
> > > 	git rev-parse --show-cdup
> > > 
> > > incantation.
> > 
> > Ah ok, 'cdup' just doesn't really mean much to me.  Is it supposed to
> > stand for something?
> 
> Yes, it stands for "change directory up", as in `cd ../../`. ;-)

Thanks for clearing that up! I was reading it as 'c-dup' and not 'cd-up' XD

-- 
Brandon Williams

Re: [PATCH 2/9] config: report correct line number upon error

[Junio C Hamano]

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

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

Sounds sane.

Is this something we can protect easily with a new test or two?

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  config.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index 146cb3452ad..9b88531a70d 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)
> +		cf->linenr++;
>  	return ret;
>  }

Re: [PATCH 3/9] help: use early config when autocorrecting aliases

[Junio C Hamano]

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

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

Yeah, the list of unknown but similar-sounding commands is produced
for interactive human consumption, and the above reasoning is
perfectly sound.

Looks good.

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

Re: [PATCH 5/9] t1308: relax the test verifying that empty alias values are disallowed

[Junio C Hamano]

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

> 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 lin number. It is really sufficient to verify that all the relevant

s/lin/&e/;

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

Re: [PATCH 8/9] Use the early config machinery to expand aliases

[Junio C Hamano]

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

> -static char *orig_env[4];
> -static int save_restore_env_balance;
> -
> -static void save_env_before_alias(void)
> -{
> -...
> -}
> -
> -static void restore_env(int external_alias)
> -{
> -...
> -}
>  
>  static void commit_pager_choice(void) {
>  	switch (use_pager) {
> @@ -245,36 +201,37 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  
>  static int handle_alias(int *argcp, const char ***argv)
>  {
> +	struct strbuf worktree_dir = STRBUF_INIT;
>  	int envchanged = 0, ret = 0, saved_errno = errno;
>  	int count, option_count;
>  	const char **new_argv;
>  	const char *alias_command;
>  	char *alias_string;
> -	int unused_nongit;
> -
> -	save_env_before_alias();
> -	setup_git_directory_gently(&unused_nongit);
> ...
> @@ -308,8 +265,6 @@ static int handle_alias(int *argcp, const char ***argv)
>  		ret = 1;
>  	}
>  
> -	restore_env(0);
> -
>  	errno = saved_errno;

The save/restore sequence was an unnecessary hassle, and being able
to do this without chdir'ing around is really nice.

Thanks for working on this.