git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <johannes.schindelin@gmx.de>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Brandon Williams <bmwill@google.com>
Subject: [PATCH v4 6/6] Use the early config machinery to expand aliases
Date: Wed, 14 Jun 2017 13:36:00 +0200 (CEST)	[thread overview]
Message-ID: <b75481173390d70a64a1156421aaa0ec5aa4abc2.1497440104.git.johannes.schindelin@gmx.de> (raw)
In-Reply-To: <cover.1497440104.git.johannes.schindelin@gmx.de>

Instead of discovering the .git/ directory, reading the config and then
trying to painstakingly reset all the global state if we did not find a
matching alias, let's use the early config machinery instead.

It may look like unnecessary work to discover the .git/ directory in the
early config machinery and then call setup_git_directory_gently() in the
case of a shell alias, repeating the very same discovery *again*.
However, we have to do this as the early config machinery takes pains
*not* to touch any global state, while shell aliases expect a possibly
changed working directory and at least the GIT_PREFIX and GIT_DIR
variables to be set.

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>
---
 alias.c          | 29 ++++++++++++++++++++++-------
 git.c            | 55 ++++---------------------------------------------------
 t/t7006-pager.sh |  2 +-
 3 files changed, 27 insertions(+), 59 deletions(-)

diff --git a/alias.c b/alias.c
index 3b90397a99d..5df052ae4c4 100644
--- a/alias.c
+++ b/alias.c
@@ -1,14 +1,29 @@
 #include "cache.h"
 
+struct config_alias_data
+{
+	const char *alias;
+	char *v;
+};
+
+static int config_alias_cb(const char *key, const char *value, void *d)
+{
+	struct config_alias_data *data = d;
+	const char *p;
+
+	if (skip_prefix(key, "alias.", &p) && !strcmp(p, data->alias))
+		return git_config_string((const char **)&data->v, key, value);
+
+	return 0;
+}
+
 char *alias_lookup(const char *alias)
 {
-	char *v = NULL;
-	struct strbuf key = STRBUF_INIT;
-	strbuf_addf(&key, "alias.%s", alias);
-	if (git_config_key_is_valid(key.buf))
-		git_config_get_string(key.buf, &v);
-	strbuf_release(&key);
-	return v;
+	struct config_alias_data data = { alias, NULL };
+
+	read_early_config(config_alias_cb, &data);
+
+	return data.v;
 }
 
 #define SPLIT_CMDLINE_BAD_ENDING 1
diff --git a/git.c b/git.c
index 8ff44f081d4..58ef570294d 100644
--- a/git.c
+++ b/git.c
@@ -16,50 +16,6 @@ const char git_more_info_string[] =
 	   "to read about a specific subcommand or concept.");
 
 static int use_pager = -1;
-static char *orig_cwd;
-static const char *env_names[] = {
-	GIT_DIR_ENVIRONMENT,
-	GIT_WORK_TREE_ENVIRONMENT,
-	GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
-	GIT_PREFIX_ENVIRONMENT
-};
-static char *orig_env[4];
-static int save_restore_env_balance;
-
-static void save_env_before_alias(void)
-{
-	int i;
-
-	assert(save_restore_env_balance == 0);
-	save_restore_env_balance = 1;
-	orig_cwd = xgetcwd();
-	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
-		orig_env[i] = getenv(env_names[i]);
-		orig_env[i] = xstrdup_or_null(orig_env[i]);
-	}
-}
-
-static void restore_env(int external_alias)
-{
-	int i;
-
-	assert(save_restore_env_balance == 1);
-	save_restore_env_balance = 0;
-	if (!external_alias && orig_cwd && chdir(orig_cwd))
-		die_errno("could not move to %s", orig_cwd);
-	free(orig_cwd);
-	for (i = 0; i < ARRAY_SIZE(env_names); i++) {
-		if (external_alias &&
-		    !strcmp(env_names[i], GIT_PREFIX_ENVIRONMENT))
-			continue;
-		if (orig_env[i]) {
-			setenv(env_names[i], orig_env[i], 1);
-			free(orig_env[i]);
-		} else {
-			unsetenv(env_names[i]);
-		}
-	}
-}
 
 static void commit_pager_choice(void) {
 	switch (use_pager) {
@@ -250,19 +206,18 @@ static int handle_alias(int *argcp, const char ***argv)
 	const char **new_argv;
 	const char *alias_command;
 	char *alias_string;
-	int unused_nongit;
-
-	save_env_before_alias();
-	setup_git_directory_gently(&unused_nongit);
 
 	alias_command = (*argv)[0];
 	alias_string = alias_lookup(alias_command);
 	if (alias_string) {
 		if (alias_string[0] == '!') {
 			struct child_process child = CHILD_PROCESS_INIT;
+			int nongit_ok;
+
+			/* Aliases expect GIT_PREFIX, GIT_DIR etc to be set */
+			setup_git_directory_gently(&nongit_ok);
 
 			commit_pager_choice();
-			restore_env(1);
 
 			child.use_shell = 1;
 			argv_array_push(&child.args, alias_string + 1);
@@ -308,8 +263,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.1.windows.1.1.ga36e14b3aaa

  parent reply	other threads:[~2017-06-14 11:36 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-14 11:35 [PATCH v4 0/6] Avoid problem where git_dir is set after alias expansion Johannes Schindelin
2017-06-14 11:35 ` [PATCH v4 1/6] discover_git_directory(): avoid setting invalid git_dir Johannes Schindelin
2017-06-14 11:35 ` [PATCH v4 2/6] config: report correct line number upon error Johannes Schindelin
2017-06-14 11:35 ` [PATCH v4 3/6] help: use early config when autocorrecting aliases Johannes Schindelin
2017-06-14 11:35 ` [PATCH v4 4/6] t1308: relax the test verifying that empty alias values are disallowed Johannes Schindelin
2017-06-14 11:35 ` [PATCH v4 5/6] t7006: demonstrate a problem with aliases in subdirectories Johannes Schindelin
2017-06-14 11:36 ` Johannes Schindelin [this message]
2017-06-15 19:36   ` [PATCH v4 6/6] Use the early config machinery to expand aliases Junio C Hamano
2017-06-15 19:44     ` Johannes Schindelin
2017-06-15  6:37 ` [PATCH v4 0/6] Avoid problem where git_dir is set after alias expansion Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b75481173390d70a64a1156421aaa0ec5aa4abc2.1497440104.git.johannes.schindelin@gmx.de \
    --to=johannes.schindelin@gmx.de \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).