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

[Johannes Schindelin <johannes.schindelin@gmx.de>]

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