v2, 8 partsAvoid 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.

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

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

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

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

Hi Peff,

On Sat, 10 Jun 2017, Jeff King wrote:

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.

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.

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

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

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

Oops, right.

-Peff

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

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

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

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

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

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

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

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

Good explanation, and the patch looks obviously correct.

-Peff

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

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

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

Hi Junio,

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

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

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.

Tru. ;-)

Cia, Dsch

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

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

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.

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

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

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

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

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

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

Jeff King <peff@peff.net> writes:

That looks a lot simpler.

Hi Peff,

On Sat, 10 Jun 2017, Jeff King wrote:

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

Exactly.

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

Will change the code accordingly.

Ciao, Dscho

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

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

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

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

  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

Hi Peff,

On Sat, 10 Jun 2017, Jeff King wrote:

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.

;-)

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

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

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

Hi Peff,

On Tue, 13 Jun 2017, Johannes Schindelin wrote:

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

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

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

-Peff

Jeff King <peff@peff.net> writes:

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?

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

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

-Peff

Hi Junio,

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

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

On 06/08, Johannes Schindelin wrote:

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