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

[PATCH v2 0/8] 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 v1:

- 1/8's commit message clarifies why the other early return in
  discover_git_directory() does not need an equivalent resetting of
  git_dir.

- 3/8's commit message fixes awkward language (thanks, Brandon!).

- the `worktree_dir` variables/parameters have been renamed to
  `cdup_dir` to clarify that they only get populated if the search for
  the .git/ directory determined that the current working directory
  is a subdirectory of the directory containing .git/.


Johannes Schindelin (8):
  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

 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-v2
Fetch-It-Via: git fetch https://github.com/dscho/git alias-early-config-v2

Interdiff vs v1:
 diff --git a/alias.c b/alias.c
 index c1b87154944..b7c4a4f0217 100644
 --- a/alias.c
 +++ b/alias.c
 @@ -16,13 +16,13 @@ static int config_alias_cb(const char *key, const char *value, void *d)
  	return 0;
  }
  
 -char *alias_lookup(const char *alias, struct strbuf *worktree_dir)
 +char *alias_lookup(const char *alias, struct strbuf *cdup_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);
 +		read_early_config(config_alias_cb, &data, cdup_dir);
  	strbuf_release(&data.key);
  
  	return data.v;
 diff --git a/cache.h b/cache.h
 index 77ce8e8c80b..65f2e5bf04c 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, struct strbuf *worktree_dir);
 +extern const char *discover_git_directory(struct strbuf *gitdir, struct strbuf *cdup_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);
 @@ -1914,7 +1914,7 @@ extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,
  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,
 -			      struct strbuf *worktree_dir);
 +			      struct strbuf *cdup_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,
 @@ -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, struct strbuf *worktree_dir);
 +char *alias_lookup(const char *alias, struct strbuf *cdup_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/config.c b/config.c
 index 3d78c11fc00..5aec6e4c87c 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, struct strbuf *worktree_dir)
 +void read_early_config(config_fn_t cb, void *data, struct strbuf *cdup_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, struct strbuf *worktree_dir)
  	 * notably, the current working directory is still the same after the
  	 * call).
  	 */
 -	else if (discover_git_directory(&buf, worktree_dir))
 +	else if (discover_git_directory(&buf, cdup_dir))
  		opts.git_dir = buf.buf;
  
  	git_config_with_options(cb, data, NULL, &opts);
 diff --git a/git.c b/git.c
 index c82cd455948..6511595d467 100644
 --- a/git.c
 +++ b/git.c
 @@ -201,7 +201,7 @@ 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;
 +	struct strbuf cdup_dir = STRBUF_INIT;
  	int envchanged = 0, ret = 0, saved_errno = errno;
  	int count, option_count;
  	const char **new_argv;
 @@ -209,12 +209,12 @@ static int handle_alias(int *argcp, const char ***argv)
  	char *alias_string;
  
  	alias_command = (*argv)[0];
 -	alias_string = alias_lookup(alias_command, &worktree_dir);
 +	alias_string = alias_lookup(alias_command, &cdup_dir);
  	if (alias_string) {
  		if (alias_string[0] == '!') {
  			struct child_process child = CHILD_PROCESS_INIT;
  
 -			if (worktree_dir.len)
 +			if (cdup_dir.len)
  				setup_git_directory();
  
  			commit_pager_choice();
 @@ -224,14 +224,14 @@ static int handle_alias(int *argcp, const char ***argv)
  			argv_array_pushv(&child.args, (*argv) + 1);
  
  			ret = run_command(&child);
 -			strbuf_release(&worktree_dir);
 +			strbuf_release(&cdup_dir);
  			if (ret >= 0)   /* normal exit */
  				exit(ret);
  
  			die_errno("While expanding alias '%s': '%s'",
  			    alias_command, alias_string + 1);
  		}
 -		strbuf_release(&worktree_dir);
 +		strbuf_release(&cdup_dir);
  		count = split_cmdline(alias_string, &new_argv);
  		if (count < 0)
  			die("Bad alias.%s string: %s", alias_command,
 diff --git a/setup.c b/setup.c
 index 771822fd0ca..3f6fc1577b7 100644
 --- a/setup.c
 +++ b/setup.c
 @@ -946,10 +946,10 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
  }
  
  const char *discover_git_directory(struct strbuf *gitdir,
 -				   struct strbuf *worktree_dir)
 +				   struct strbuf *cdup_dir)
  {
  	struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
 -	size_t gitdir_offset = gitdir->len, cwd_len, worktree_dir_offset;
 +	size_t gitdir_offset = gitdir->len, cwd_len, cdup_dir_offset;
  	struct repository_format candidate;
  
  	if (strbuf_getcwd(&dir))
 @@ -974,9 +974,9 @@ 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);
 +	if (cdup_dir) {
 +		cdup_dir_offset = cdup_dir->len;
 +		strbuf_addbuf(cdup_dir, &dir);
  	}
  
  	strbuf_reset(&dir);
 @@ -989,8 +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);
 +		if (cdup_dir)
 +			strbuf_setlen(cdup_dir, cdup_dir_offset);
  		return NULL;
  	}
  
-- 
2.13.0.windows.1.460.g13f583bedb5

[PATCH v2 1/8] 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 v2 2/8] 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 v2 3/8] 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 v2 4/8] 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..f769ef1779c 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 *cdup_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 *cdup_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..5aec6e4c87c 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 *cdup_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, cdup_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..3f6fc1577b7 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 *cdup_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, cdup_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 (cdup_dir) {
+		cdup_dir_offset = cdup_dir->len;
+		strbuf_addbuf(cdup_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 (cdup_dir)
+			strbuf_setlen(cdup_dir, cdup_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 v2 5/8] 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 v2 6/8] 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 v2 7/8] 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..b7c4a4f0217 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 *cdup_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, cdup_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 f769ef1779c..65f2e5bf04c 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 *cdup_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 v2 8/8] 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..6511595d467 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 cdup_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, &cdup_dir);
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			struct child_process child = CHILD_PROCESS_INIT;
 
+			if (cdup_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(&cdup_dir);
 			if (ret >= 0)   /* normal exit */
 				exit(ret);
 
 			die_errno("While expanding alias '%s': '%s'",
 			    alias_command, alias_string + 1);
 		}
+		strbuf_release(&cdup_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 v2 0/8] Avoid problem where git_dir is set after alias expansion

[Brandon Williams]

On 06/08, Johannes Schindelin wrote:
> 
> Changes since v1:
> 
> - 1/8's commit message clarifies why the other early return in
>   discover_git_directory() does not need an equivalent resetting of
>   git_dir.
> 
> - 3/8's commit message fixes awkward language (thanks, Brandon!).
> 
> - the `worktree_dir` variables/parameters have been renamed to
>   `cdup_dir` to clarify that they only get populated if the search for
>   the .git/ directory determined that the current working directory
>   is a subdirectory of the directory containing .git/.
> 
> 

v2 addresses some of the comments I had with v1, so everything looks
good to me.

> Johannes Schindelin (8):
>   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
> 
>  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(-)

-- 
Brandon Williams

Re: [PATCH v2 5/8] 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
> 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

Just judging from the looks of these, it appears that the updated
messages are more descriptive than the original.  Am I mistaken?

If so I think the log message undersells the changes made by the
series by sounding as if you are saying "we are too lazy to bother
giving the same message, so here is a workaround".

In any case, s/lin number/line number/ is needed ;-)

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

[Jeff King]

On Thu, Jun 08, 2017 at 09:53:32PM +0200, 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.
> 
> 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.

After reading this, I thought at first this was a user-visible (albeit
obscure) bug where something like:

  git init
  git init bogus
  cd bogus
  git config core.repositoryformatversion 5

would mean that early-config barfs on seeing our bogus ".git", but that
screws up the discovery going back to the real repository root. But
that's not the case, because we always stop walking anyway when we see
such a bogus git dir.

So it's just a cleanup (though obviously a sensible one).

-Peff

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

[Jeff King]

On Thu, Jun 08, 2017 at 09:53:35PM +0200, Johannes Schindelin wrote:

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

Good catch. Would we want a test here, like:

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 13b7851f7..4cad8366a 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 'invalid path' '
+	echo "[bool]var" >invalid &&
+	test_must_fail git config -f invalid --path bool.var 2>actual &&
+	test_i18ngrep "line 1" 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

which currently reports "line 2" instead of line 1.

I wondered if the same bug could be found earlier (e.g,. from
parse_value() when it sees an unpaired quote), but it looks like we do a
cf->linenr-- rollback there already.

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

I think this should be "if (ret < 0)". The caller only considers it an
error if get_value() returns a negative number. As you have it here, I
think a config callback which returned a positive number would end up
with nonsense line numbers.

-Peff

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

[Jeff King]

On Thu, Jun 08, 2017 at 09:53:38PM +0200, Johannes Schindelin wrote:

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

Good explanation, and the patch looks obviously correct.

-Peff

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

[Jeff King]

On Thu, Jun 08, 2017 at 09:53:41PM +0200, 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`.

I understand why you wrote it this way, but it really feels backwards.
We're calling a function that's about config, and oh by the way
sometimes it tells us about which git directory we're in (and sometimes
not because it didn't actually run discover_git_directory anyway).

It would make a lot more sense to me if we told read_early_config() "oh
by the way, I discovered the git directory already, here it is".

The same applies to the later patch to pass the information through
alias_lookup().

Taking a step back, this is really just an optimization, right? The
cleanest thing would be for the alias code, right before launching a
shell alias, to discover_git_directory(). But you don't want to do that
because it's too expensive?

If so, my questions would be:

  1. Is it actually that expensive, or is this premature optimization?
     We are, after all, about to fork and execute a shell. Have we
     measured?

  2. Can we cache the results of discovery_git_directory() between
     calls? That would not only fix your issue here, but it would
     optimize the calls we make when we call read_early_config() for
     multiple items (e.g., both pager and alias config).

     The only trick is making sure our previous value is still valid.
     I suspect it would work to just ignore this, as we only chdir when
     doing setup_git_directory(), and that should generally take
     precedence over discover_git_directory() being called at all. But
     for extra safety, we should be able to key it to the cwd, like:

       strbuf_getcwd(&now_cwd);
       if (!strbuf_cmp(&cached_cwd, &now_cwd) {
               strbuf_addbuf(cdup_dir, &cached_cdup_dir));
               return xstrdup(cached_gitdir);
       }
       strbuf_swap(&cached_cwd, &now_cwd);
       strbuf_release(&now_cwd);
       ... rest of function, but store result in cached_gitdir ...

-Peff

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

[Jeff King]

On Thu, Jun 08, 2017 at 09:53:53PM +0200, 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 cdup_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, &cdup_dir);
>  	if (alias_string) {
>  		if (alias_string[0] == '!') {
>  			struct child_process child = CHILD_PROCESS_INIT;
>  
> +			if (cdup_dir.len)
> +				setup_git_directory();
> +

I'm really confused here. We went to all the trouble to record the
cdup_dir as part of the alias lookup so that we could make sure to
chdir() to it. But we don't seem to ever look at its value. We just use
it as a proxy for "did we find a git-dir". And if we did, we go to all
the trouble to re-discover it via setup_git_directory().

I understand why you must use setup_git_directory() and not just a
chdir(), because the latter sets up variables like GIT_PREFIX and
GIT_DIR that the shell alias may be depending on.

But couldn't we just unconditionally do:

  setup_git_directory_gently();

here to move into the top-level if there is one, without caring about
cdup_dir at all?

That should be equally correct. It does do an extra discovery when we
already know that there isn't a gitdir at all. But my earlier comments
about optimizing apply (measure, and possibly cache). And in the common
case that we are in a repo, it's exactly the same, since the setup call
here would re-discover from scratch (though again, if we care, caching
could solve that).

-Peff

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

[Jeff King]

On Sat, Jun 10, 2017 at 06:07:30AM -0400, Jeff King wrote:

> But couldn't we just unconditionally do:
> 
>   setup_git_directory_gently();
> 
> here to move into the top-level if there is one, without caring about
> cdup_dir at all?

IOW, drop your patch 4, and then squash patches 7 and 8 into this:

diff --git a/alias.c b/alias.c
index 3b90397a9..6bdc93630 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 8ff44f081..4e556fa9a 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,17 @@ 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] == '!') {
+			int nongit_ok;
 			struct child_process child = CHILD_PROCESS_INIT;
 
+			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 +262,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 83881ec3a..20b4d83c2 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" &&
 	(

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Jeff King]

On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:

> -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 strbuf key;
> +	char *v;
> +};
> [...]
> +char *alias_lookup(const char *alias, struct strbuf *cdup_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, cdup_dir);
> +	strbuf_release(&data.key);
> +
> +	return data.v;
>  }

Two optional cleanups here:

  1. We don't need to call config_key_is_valid when using a callback. We
     only needed that to prevent the configset machinery from issuing a
     warning. It does save us reading the config entirely when the
     program name is syntactically invalid as an alias, but that's a
     pretty rare case.

  2. Now that we're not using the configset machinery, we don't need to
     have the alias name as a full string. Instead of using the strbuf,
     we could just pass the "alias" string itself and do:

       if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))

     in the callback.

I don't think either is a big deal, though.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Jun 10, 2017 at 06:07:30AM -0400, Jeff King wrote:
>
>> But couldn't we just unconditionally do:
>> 
>>   setup_git_directory_gently();
>> 
>> here to move into the top-level if there is one, without caring about
>> cdup_dir at all?
>
> IOW, drop your patch 4, and then squash patches 7 and 8 into this:

That looks a lot simpler.

> diff --git a/alias.c b/alias.c
> index 3b90397a9..6bdc93630 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 8ff44f081..4e556fa9a 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,17 @@ 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] == '!') {
> +			int nongit_ok;
>  			struct child_process child = CHILD_PROCESS_INIT;
>  
> +			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 +262,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 83881ec3a..20b4d83c2 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" &&
>  	(

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

[Johannes Schindelin]

Hi Junio,

On Sat, 10 Jun 2017, Junio C Hamano wrote:

> 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
> > 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
> 
> Just judging from the looks of these, it appears that the updated
> messages are more descriptive than the original.  Am I mistaken?

Sadly, I do not think so. It is just different, not better. Maybe less
redundant... See for yourself:

Before:

	error: missing value for 'alias.br'
	fatal: bad config variable 'alias.br' in file '.git/config' at line 2

After:

	error: missing value for 'alias.br'
	fatal: bad config line 2 in file .git/config

> If so I think the log message undersells the changes made by the
> series by sounding as if you are saying "we are too lazy to bother
> giving the same message, so here is a workaround".

I fear this is not underselling anything for an improvement.

The real fix would indeed be (as mentioned by Brandon elsewhere) to unify
the code paths between the cached and the non-cached config machinery, so
as to provide the exact same error message in this case.

It is annoying that I lack the time to make that happen. My official
excuse, though, is that that fix is outside the purpose of this patch
series.

> In any case, s/lin number/line number/ is needed ;-)

Tru. ;-)

Cia,
Dsch

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

[Johannes Schindelin]

Hi Peff,

On Sat, 10 Jun 2017, Jeff King wrote:

> On Thu, Jun 08, 2017 at 09:53:35PM +0200, Johannes Schindelin wrote:
> 
> > 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.
> 
> Good catch. Would we want a test here, like:
> 
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 13b7851f7..4cad8366a 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 'invalid path' '
> +	echo "[bool]var" >invalid &&
> +	test_must_fail git config -f invalid --path bool.var 2>actual &&
> +	test_i18ngrep "line 1" 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
> 
> which currently reports "line 2" instead of line 1.

Mmmmkay.

I am always reluctant to add *even more* stuff to the test suite, in
particular since my patch series implicitly changes t1308 to test for this
very thing.

But I guess my fight against the test suite taking longer and longer and
longer is a fight I must lose, as I am pretty alone in that endeavor, and
would have have to fight fellow developers. So I guess it is time to give
up.

I added the test case you suggested, with a title that I find a bit more
descriptive.

> I wondered if the same bug could be found earlier (e.g,. from
> parse_value() when it sees an unpaired quote), but it looks like we do a
> cf->linenr-- rollback there already.

Right.

The underlying bug, of course, is that we use a different machinery to
handle cached vs non-cached config, but that is a different story.

> > 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;
> >  }
> 
> I think this should be "if (ret < 0)". The caller only considers it an
> error if get_value() returns a negative number. As you have it here, I
> think a config callback which returned a positive number would end up
> with nonsense line numbers.

I think you are half-correct: it should be `if (ret >= 0)` (the linenr
needs to be modified back in case of success, not in case of failure, in
case of failure there will be some reporting going on that needs the same
line number as `fn()` had seen).

Ciao,
Dscho

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Johannes Schindelin]

Hi Peff,

On Sat, 10 Jun 2017, Jeff King wrote:

> On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:
> 
> > -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 strbuf key;
> > +	char *v;
> > +};
> > [...]
> > +char *alias_lookup(const char *alias, struct strbuf *cdup_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, cdup_dir);
> > +	strbuf_release(&data.key);
> > +
> > +	return data.v;
> >  }
> 
> Two optional cleanups here:
> 
>   1. We don't need to call config_key_is_valid when using a callback. We
>      only needed that to prevent the configset machinery from issuing a
>      warning. It does save us reading the config entirely when the
>      program name is syntactically invalid as an alias, but that's a
>      pretty rare case.

It may be a pretty rare case, or it may not be. I do not want to think
hard about this, so I just wanted to keep that test.

But since you suggested it, I will simply blame all the fallout (if any)
on you.

;-)

>   2. Now that we're not using the configset machinery, we don't need to
>      have the alias name as a full string. Instead of using the strbuf,
>      we could just pass the "alias" string itself and do:
> 
>        if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
> 
>      in the callback.

As you probably guessed, I had tried that first and then figured that if
I needed to keep the config_key_is_valid() test anyway, I could just as
well keep the strbuf around for later use.

Will change the code,
Dscho

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

[Johannes Schindelin]

Hi Peff,

On Sat, 10 Jun 2017, Jeff King wrote:

> On Thu, Jun 08, 2017 at 09:53:53PM +0200, 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 cdup_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, &cdup_dir);
> >  	if (alias_string) {
> >  		if (alias_string[0] == '!') {
> >  			struct child_process child = CHILD_PROCESS_INIT;
> >  
> > +			if (cdup_dir.len)
> > +				setup_git_directory();
> > +
> 
> I'm really confused here. We went to all the trouble to record the
> cdup_dir as part of the alias lookup so that we could make sure to
> chdir() to it.

Right, that was my first idea.

But then the test suite taught me that we set at least GIT_PREFIX and that
tests break (or, in other words, promises Git made earlier) if those
variables are not set.

So as a quick fix, I simply called setup_git_directory() instead of
chdir(cdup_dir.buf).

> I understand why you must use setup_git_directory() and not just a
> chdir(), because the latter sets up variables like GIT_PREFIX and
> GIT_DIR that the shell alias may be depending on.

Exactly.

> But couldn't we just unconditionally do:
> 
>   setup_git_directory_gently();

But of course we can do that! Why on earth did I not think of that...

Will change the code accordingly.

Ciao,
Dscho

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

[Jeff King]

On Tue, Jun 13, 2017 at 01:02:49PM +0200, Johannes Schindelin wrote:

> > +test_expect_success 'invalid path' '
> > +	echo "[bool]var" >invalid &&
> > +	test_must_fail git config -f invalid --path bool.var 2>actual &&
> > +	test_i18ngrep "line 1" 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
> > 
> > which currently reports "line 2" instead of line 1.
> 
> Mmmmkay.
> 
> I am always reluctant to add *even more* stuff to the test suite, in
> particular since my patch series implicitly changes t1308 to test for this
> very thing.

If there's another test that covers this, I'm happy to use that. I just
didn't notice it.

> > > +	if (!ret)
> > > +		cf->linenr++;
> > >  	return ret;
> > >  }
> > 
> > I think this should be "if (ret < 0)". The caller only considers it an
> > error if get_value() returns a negative number. As you have it here, I
> > think a config callback which returned a positive number would end up
> > with nonsense line numbers.
> 
> I think you are half-correct: it should be `if (ret >= 0)` (the linenr
> needs to be modified back in case of success, not in case of failure, in
> case of failure there will be some reporting going on that needs the same
> line number as `fn()` had seen).

Oops, right.

-Peff

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Jeff King]

On Tue, Jun 13, 2017 at 01:21:28PM +0200, Johannes Schindelin wrote:

> > Two optional cleanups here:
> > 
> >   1. We don't need to call config_key_is_valid when using a callback. We
> >      only needed that to prevent the configset machinery from issuing a
> >      warning. It does save us reading the config entirely when the
> >      program name is syntactically invalid as an alias, but that's a
> >      pretty rare case.
> 
> It may be a pretty rare case, or it may not be. I do not want to think
> hard about this, so I just wanted to keep that test.
> 
> But since you suggested it, I will simply blame all the fallout (if any)
> on you.
> 
> ;-)

I accept all the blame. :)

(And I was the person who added the is_valid check here, so I can take
both blame and credit for the whole thing).

-Peff

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

[Jeff King]

On Tue, Jun 13, 2017 at 01:25:53PM +0200, Johannes Schindelin wrote:

> > But couldn't we just unconditionally do:
> > 
> >   setup_git_directory_gently();
> 
> But of course we can do that! Why on earth did I not think of that...
> 
> Will change the code accordingly.

Thanks. I was really worried you were going to come back with some
subtle point that I missed. But I'm happy we will be able to take the
simpler route.

-Peff

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Johannes Schindelin]

Hi Peff,

On Tue, 13 Jun 2017, Johannes Schindelin wrote:

> On Sat, 10 Jun 2017, Jeff King wrote:
> 
> > On Thu, Jun 08, 2017 at 09:53:50PM +0200, Johannes Schindelin wrote:
> > 
> > > -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 strbuf key;
> > > +	char *v;
> > > +};
> > > [...]
> > > +char *alias_lookup(const char *alias, struct strbuf *cdup_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, cdup_dir);
> > > +	strbuf_release(&data.key);
> > > +
> > > +	return data.v;
> > >  }
> > 
> > Two optional cleanups here:
> > 
> >   1. We don't need to call config_key_is_valid when using a callback. We
> >      only needed that to prevent the configset machinery from issuing a
> >      warning. It does save us reading the config entirely when the
> >      program name is syntactically invalid as an alias, but that's a
> >      pretty rare case.
> 
> It may be a pretty rare case, or it may not be. I do not want to think
> hard about this, so I just wanted to keep that test.
> 
> But since you suggested it, I will simply blame all the fallout (if any)
> on you.
> 
> ;-)
> 
> >   2. Now that we're not using the configset machinery, we don't need to
> >      have the alias name as a full string. Instead of using the strbuf,
> >      we could just pass the "alias" string itself and do:
> > 
> >        if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
> > 
> >      in the callback.
> 
> As you probably guessed, I had tried that first and then figured that if
> I needed to keep the config_key_is_valid() test anyway, I could just as
> well keep the strbuf around for later use.
> 
> Will change the code,

Alas, I won't change the code after all.

It is really tempting to avoid the extra strbuf, but then the error
message would change from

	error: missing value for 'alias.br'

to

	error: missing value for 'br'

which is of course no good at all.

And since I already have to keep that strbuf, I'll simply keep the
config_key_is_valid() guard, too (because why not).

Ciao,
Dscho

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Jeff King]

On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote:

> > As you probably guessed, I had tried that first and then figured that if
> > I needed to keep the config_key_is_valid() test anyway, I could just as
> > well keep the strbuf around for later use.
> > 
> > Will change the code,
> 
> Alas, I won't change the code after all.
> 
> It is really tempting to avoid the extra strbuf, but then the error
> message would change from
> 
> 	error: missing value for 'alias.br'
> 
> to
> 
> 	error: missing value for 'br'
> 
> which is of course no good at all.
> 
> And since I already have to keep that strbuf, I'll simply keep the
> config_key_is_valid() guard, too (because why not).

Oof, yeah, that is definitely worse. I'm fine with keeping both parts.

-Peff

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

[Junio C Hamano]

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

> Sadly, I do not think so. It is just different, not better. Maybe less
> redundant... See for yourself:

Yup, I noticed and was referring to this "less redundant" as an
improvement, actually.

> The real fix would indeed be (as mentioned by Brandon elsewhere) to unify
> the code paths between the cached and the non-cached config machinery, so
> as to provide the exact same error message in this case.

Yeah, the unifying of the messages would be a good addition in the
mid term but I tend to agree that it can be done after this series
lands.

Thanks for clarification.

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote:
>
>> > As you probably guessed, I had tried that first and then figured that if
>> > I needed to keep the config_key_is_valid() test anyway, I could just as
>> > well keep the strbuf around for later use.
>> > 
>> > Will change the code,
>> 
>> Alas, I won't change the code after all.
>> 
>> It is really tempting to avoid the extra strbuf, but then the error
>> message would change from
>> 
>> 	error: missing value for 'alias.br'
>> 
>> to
>> 
>> 	error: missing value for 'br'
>> 
>> which is of course no good at all.
>> 
>> And since I already have to keep that strbuf, I'll simply keep the
>> config_key_is_valid() guard, too (because why not).
>
> Oof, yeah, that is definitely worse. I'm fine with keeping both parts.

When you replace Dscho's "compare 'var' with 'alias.br' that is in
strbuf naively with the "skip-prefix and compare with br" without
changing anything else, i.e.

    if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
	return git_config_string((const char **)&data->v, key, value);

it would cause the "br" to be fed to git_config_string() and result
in problem reported for "br", not "alias.br".  

But this can be trivially fixed by passing "var" instead of "key" to
git_config_string(), no?  Am I mistaken?

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Jeff King]

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

> >> It is really tempting to avoid the extra strbuf, but then the error
> >> message would change from
> >> 
> >> 	error: missing value for 'alias.br'
> >> 
> >> to
> >> 
> >> 	error: missing value for 'br'
> >> 
> >> which is of course no good at all.
> >> 
> >> And since I already have to keep that strbuf, I'll simply keep the
> >> config_key_is_valid() guard, too (because why not).
> >
> > Oof, yeah, that is definitely worse. I'm fine with keeping both parts.
> 
> When you replace Dscho's "compare 'var' with 'alias.br' that is in
> strbuf naively with the "skip-prefix and compare with br" without
> changing anything else, i.e.
> 
>     if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
> 	return git_config_string((const char **)&data->v, key, value);
> 
> it would cause the "br" to be fed to git_config_string() and result
> in problem reported for "br", not "alias.br".  
> 
> But this can be trivially fixed by passing "var" instead of "key" to
> git_config_string(), no?  Am I mistaken?

Right, I wasn't thinking. That is a perfectly good solution, and matches
the usual mechanism in other config callbacks.

-Peff

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Johannes Schindelin]

Hi Junio,

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

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote:
> >
> >> > As you probably guessed, I had tried that first and then figured that if
> >> > I needed to keep the config_key_is_valid() test anyway, I could just as
> >> > well keep the strbuf around for later use.
> >> > 
> >> > Will change the code,
> >> 
> >> Alas, I won't change the code after all.
> >> 
> >> It is really tempting to avoid the extra strbuf, but then the error
> >> message would change from
> >> 
> >> 	error: missing value for 'alias.br'
> >> 
> >> to
> >> 
> >> 	error: missing value for 'br'
> >> 
> >> which is of course no good at all.
> >> 
> >> And since I already have to keep that strbuf, I'll simply keep the
> >> config_key_is_valid() guard, too (because why not).
> >
> > Oof, yeah, that is definitely worse. I'm fine with keeping both parts.
> 
> When you replace Dscho's "compare 'var' with 'alias.br' that is in
> strbuf naively with the "skip-prefix and compare with br" without
> changing anything else, i.e.
> 
>     if (skip_prefix(var, "alias.", &key) && !strcmp(key, data->key))
> 	return git_config_string((const char **)&data->v, key, value);
> 
> it would cause the "br" to be fed to git_config_string() and result
> in problem reported for "br", not "alias.br".  
> 
> But this can be trivially fixed by passing "var" instead of "key" to
> git_config_string(), no?  Am I mistaken?

Oh my. You are right! The `git_config_string()` function uses the `key`
parameter only for reporting, not for anything else.

So all I had to do was to introduce another variable so that the `key`
pointer would not advance beyond the "alias.".

Will fix,
Dscho