threads / patch / 46133

patch, 9 partsdiscover_git_directory(): avoid setting invalid git_dir

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

## tl;dr

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

replies: 33people: 3as markdown or json

Johannes Schindelin· Jun 7, 2017, 16:06 UTC · lore

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

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
Johannes Schindelin· Jun 7, 2017, 16:06 UTC · re: Johannes Schindelin · lore

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(+)
Show changes to setup.c +1 −0
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
Brandon Williams· Jun 7, 2017, 17:45 UTC · re: Johannes Schindelin · lore

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

On 06/07, Johannes Schindelin wrote:
Show 11 quoted lines
> 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).

Show 14 quoted lines
> ---
>  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?

Show 6 quoted lines
>  	}
>  
> -- 
> 2.13.0.windows.1.460.g13f583bedb5
> 
> 
-- 
Brandon Williams
Johannes Schindelin· Jun 8, 2017, 10:00 UTC · re: Brandon Williams · lore

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

Hi Brandon,
On Wed, 7 Jun 2017, Brandon Williams wrote:
Show 17 quoted lines
> 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

Johannes Schindelin· Jun 7, 2017, 16:06 UTC · re: Johannes Schindelin · lore

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

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

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

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 config.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to config.c +2 −1
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
Junio C Hamano· Jun 9, 2017, 17:01 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 10 quoted lines
> 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?
Show 19 quoted lines
>
> 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;
>  }
Johannes Schindelin· Jun 7, 2017, 16:06 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to help.c +1 −1
diff --git a/help.c b/help.c
index db7f3d79a01..b44c55ec2da 100644
--- a/help.c
+++ b/help.c
@@ -289,7 +289,7 @@ const char *help_unknown_cmd(const char *cmd)
 	memset(&other_cmds, 0, sizeof(other_cmds));
 	memset(&aliases, 0, sizeof(aliases));
 
-	git_config(git_unknown_cmd_config, NULL);
+	read_early_config(git_unknown_cmd_config, NULL);
 
 	load_command_list("git-", &main_cmds, &other_cmds);
 

-- 
2.13.0.windows.1.460.g13f583bedb5
Brandon Williams· Jun 7, 2017, 17:51 UTC · re: Johannes Schindelin · lore

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

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//
Show 19 quoted lines
> 
> 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.
Show 23 quoted lines
> 
> 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
Johannes Schindelin· Jun 8, 2017, 10:02 UTC · re: Brandon Williams · lore

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

Hi Brandon,
On Wed, 7 Jun 2017, Brandon Williams wrote:
Show 8 quoted lines
> 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

Junio C Hamano· Jun 9, 2017, 17:05 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 5 quoted lines
> 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.
Show 17 quoted lines
> 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);
Johannes Schindelin· Jun 7, 2017, 16:06 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 6 files +19 −10

cache.h, config.c, help.c, pager.c, setup.c, t/helper/test-config.c

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
Brandon Williams· Jun 7, 2017, 18:13 UTC · re: Johannes Schindelin · lore

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

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

Show 93 quoted lines
>  
>  	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
Johannes Schindelin· Jun 8, 2017, 10:20 UTC · re: Brandon Williams · lore

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

Hi Brandon,
On Wed, 7 Jun 2017, Brandon Williams wrote:
Show 11 quoted lines
> 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

Brandon Williams· Jun 8, 2017, 14:46 UTC · re: Johannes Schindelin · lore

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

On 06/08, Johannes Schindelin wrote:
Show 45 quoted lines
> 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.
Show 14 quoted lines
> 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
Johannes Schindelin· Jun 8, 2017, 15:30 UTC · re: Brandon Williams · lore

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

Hi Brandon,
On Thu, 8 Jun 2017, Brandon Williams wrote:
Show 15 quoted lines
> 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

Brandon Williams· Jun 8, 2017, 16:32 UTC · re: Johannes Schindelin · lore

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

On 06/08, Johannes Schindelin wrote:
Show 25 quoted lines
> 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
Johannes Schindelin· Jun 8, 2017, 18:52 UTC · re: Brandon Williams · lore

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

Hi Brandon,
On Thu, 8 Jun 2017, Brandon Williams wrote:
Show 28 quoted lines
> 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

Brandon Williams· Jun 8, 2017, 18:54 UTC · re: Johannes Schindelin · lore

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

On 06/08, Johannes Schindelin wrote:
Show 34 quoted lines
> 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
Johannes Schindelin· Jun 7, 2017, 16:06 UTC · re: Johannes Schindelin · lore

[PATCH 7/9] alias_lookup(): optionally return top-level directory

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(-)
Show changes to 4 files +28 −11

alias.c, builtin/help.c, cache.h, git.c

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
Johannes Schindelin· Jun 7, 2017, 16:06 UTC · re: Johannes Schindelin · lore

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

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

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

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

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

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

-- 
2.13.0.windows.1.460.g13f583bedb5
Johannes Schindelin· Jun 7, 2017, 16:06 UTC · re: Johannes Schindelin · lore

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

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

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

Let's not get hung up by the precise wording of the message mentioning the 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(-)
Show changes to t/t1308-config-set.sh +3 −1
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 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· Jun 7, 2017, 18:15 UTC · re: Johannes Schindelin · lore

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

On 06/07, Johannes Schindelin wrote:
Show 5 quoted lines
> 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?

Show 29 quoted lines
> 
> 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
Johannes Schindelin· Jun 8, 2017, 10:22 UTC · re: Brandon Williams · lore

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

Hi Brandon,
On Wed, 7 Jun 2017, Brandon Williams wrote:
Show 10 quoted lines
> 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

Brandon Williams· Jun 8, 2017, 14:47 UTC · re: Johannes Schindelin · lore

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

On 06/08, Johannes Schindelin wrote:
Show 21 quoted lines
> 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
Junio C Hamano· Jun 10, 2017, 01:28 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 8 quoted lines
> 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/;
Show 22 quoted lines
> 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' '
Johannes Schindelin· Jun 7, 2017, 16:07 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 2 files +8 −53

git.c, t/t7006-pager.sh

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
Brandon Williams· Jun 7, 2017, 18:26 UTC · re: Johannes Schindelin · lore

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

On 06/07, Johannes Schindelin wrote:
Show 75 quoted lines
> 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
Show 27 quoted lines
>  
>  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.

Show 44 quoted lines
> +
>  			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
Johannes Schindelin· Jun 8, 2017, 10:25 UTC · re: Brandon Williams · lore

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

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.

Show 33 quoted lines
> 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.
Brandon Williams· Jun 8, 2017, 14:51 UTC · re: Johannes Schindelin · lore

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

On 06/08, Johannes Schindelin wrote:
Show 50 quoted lines
> 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
Junio C Hamano· Jun 10, 2017, 01:33 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 37 quoted lines
> -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.
Johannes Schindelin· Jun 7, 2017, 16:09 UTC · re: Johannes Schindelin · lore

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

Hi,
On Wed, 7 Jun 2017, Johannes Schindelin wrote:
Show 11 quoted lines
> 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

Brandon Williams· Jun 7, 2017, 18:30 UTC · re: Johannes Schindelin · lore

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

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

Show 33 quoted lines
> 
> 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
Johannes Schindelin· Jun 8, 2017, 10:27 UTC · re: Brandon Williams · lore

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

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

Brandon Williams· Jun 8, 2017, 16:33 UTC · re: Johannes Schindelin · lore

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

On 06/08, Johannes Schindelin wrote:
Show 8 quoted lines
> 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 :)

Show 13 quoted lines
> 
> 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

← back to recent threads