* [PATCH v2 1/9] t7006: replace dubious test
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
The idea of the test case "git -p - core.pager is not used from
subdirectory" was to verify that the setup_git_directory() function had
not been called just to obtain the core.pager setting.
However, we are about to fix the early config machinery so that it
*does* work, without messing up the global state.
Once that is done, the core.pager setting *will* be used, even when
running from a subdirectory, and that is a Good Thing.
The intention of that test case, however, was to verify that the
setup_git_directory() function has not run, because it changes global
state such as the current working directory.
To keep that spirit, but fix the incorrect assumption, this patch
replaces that test case by a new one that verifies that the pager is
run in the subdirectory, i.e. that the current working directory has
not been changed at the time the pager is configured and launched, even
if the `rev-parse` command requires a .git/ directory and *will* change
the working directory.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
| 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index c8dc665f2fd..427bfc605ad 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -378,9 +378,19 @@ test_GIT_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_default_pager expect_success test_must_fail 'git -p'
test_PAGER_overrides expect_success test_must_fail 'git -p'
test_local_config_ignored expect_failure test_must_fail 'git -p'
-test_no_local_config_subdir expect_success test_must_fail 'git -p'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
+test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
+ sane_unset GIT_PAGER &&
+ test_config core.pager "cat >cwd-retained" &&
+ (
+ cd sub &&
+ rm -f cwd-retained &&
+ test_terminal git -p rev-parse HEAD &&
+ test -e cwd-retained
+ )
+'
+
test_doesnt_paginate expect_failure test_must_fail 'git -p nonsense'
test_pager_choices 'git shortlog'
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* [PATCH v2 5/9] Make read_early_config() reusable
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
The pager configuration needs to be read early, possibly before
discovering any .git/ directory.
Let's not hide this function in pager.c, but make it available to other
callers.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
cache.h | 1 +
config.c | 31 +++++++++++++++++++++++++++++++
| 31 -------------------------------
3 files changed, 32 insertions(+), 31 deletions(-)
diff --git a/cache.h b/cache.h
index a104b76c02e..6b6780064f0 100644
--- a/cache.h
+++ b/cache.h
@@ -1798,6 +1798,7 @@ 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 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 c6b874a7bf7..9cfbeafd04c 100644
--- a/config.c
+++ b/config.c
@@ -1412,6 +1412,37 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
}
}
+void read_early_config(config_fn_t cb, void *data)
+{
+ git_config_with_options(cb, data, NULL, 1);
+
+ /*
+ * Note that this is a really dirty hack that does the wrong thing in
+ * many cases. The crux of the problem is that we cannot run
+ * setup_git_directory() early on in git's setup, so we have no idea if
+ * we are in a repository or not, and therefore are not sure whether
+ * and how to read repository-local config.
+ *
+ * So if we _aren't_ in a repository (or we are but we would reject its
+ * core.repositoryformatversion), we'll read whatever is in .git/config
+ * blindly. Similarly, if we _are_ in a repository, but not at the
+ * root, we'll fail to find .git/config (because it's really
+ * ../.git/config, etc). See t7006 for a complete set of failures.
+ *
+ * However, we have historically provided this hack because it does
+ * work some of the time (namely when you are at the top-level of a
+ * valid repository), and would rarely make things worse (i.e., you do
+ * not generally have a .git/config file sitting around).
+ */
+ if (!startup_info->have_repository) {
+ struct git_config_source repo_config;
+
+ memset(&repo_config, 0, sizeof(repo_config));
+ repo_config.file = ".git/config";
+ git_config_with_options(cb, data, &repo_config, 1);
+ }
+}
+
static void git_config_check_init(void);
void git_config(config_fn_t fn, void *data)
--git a/pager.c b/pager.c
index ae796433630..73ca8bc3b17 100644
--- a/pager.c
+++ b/pager.c
@@ -43,37 +43,6 @@ static int core_pager_config(const char *var, const char *value, void *data)
return 0;
}
-static void read_early_config(config_fn_t cb, void *data)
-{
- git_config_with_options(cb, data, NULL, 1);
-
- /*
- * Note that this is a really dirty hack that does the wrong thing in
- * many cases. The crux of the problem is that we cannot run
- * setup_git_directory() early on in git's setup, so we have no idea if
- * we are in a repository or not, and therefore are not sure whether
- * and how to read repository-local config.
- *
- * So if we _aren't_ in a repository (or we are but we would reject its
- * core.repositoryformatversion), we'll read whatever is in .git/config
- * blindly. Similarly, if we _are_ in a repository, but not at the
- * root, we'll fail to find .git/config (because it's really
- * ../.git/config, etc). See t7006 for a complete set of failures.
- *
- * However, we have historically provided this hack because it does
- * work some of the time (namely when you are at the top-level of a
- * valid repository), and would rarely make things worse (i.e., you do
- * not generally have a .git/config file sitting around).
- */
- if (!startup_info->have_repository) {
- struct git_config_source repo_config;
-
- memset(&repo_config, 0, sizeof(repo_config));
- repo_config.file = ".git/config";
- git_config_with_options(cb, data, &repo_config, 1);
- }
-}
-
const char *git_pager(int stdout_is_tty)
{
const char *pager;
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* [PATCH v2 6/9] read_early_config(): special-case builtins that create a repository
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
When the command we are about to execute wants to create a repository
(i.e. the command is `init` or `clone`), we *must not* look for a
repository config.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
cache.h | 2 +-
config.c | 3 ++-
git.c | 3 +++
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 6b6780064f0..0af7141242f 100644
--- a/cache.h
+++ b/cache.h
@@ -2070,7 +2070,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
/* setup.c */
struct startup_info {
- int have_repository;
+ int have_repository, creating_repository;
const char *prefix;
};
extern struct startup_info *startup_info;
diff --git a/config.c b/config.c
index 9cfbeafd04c..980fcc6ff2e 100644
--- a/config.c
+++ b/config.c
@@ -1434,7 +1434,8 @@ void read_early_config(config_fn_t cb, void *data)
* valid repository), and would rarely make things worse (i.e., you do
* not generally have a .git/config file sitting around).
*/
- if (!startup_info->have_repository) {
+ if (!startup_info->creating_repository &&
+ !startup_info->have_repository) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
diff --git a/git.c b/git.c
index 33f52acbcc8..9fb9bb90a21 100644
--- a/git.c
+++ b/git.c
@@ -337,6 +337,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
struct stat st;
const char *prefix;
+ if (p->fn == cmd_init_db || p->fn == cmd_clone)
+ startup_info->creating_repository = 1;
+
prefix = NULL;
help = argc == 2 && !strcmp(argv[1], "-h");
if (!help) {
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* [PATCH v2 0/9] Fix the early config
From: Johannes Schindelin @ 2017-03-03 2:03 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1481211338.git.johannes.schindelin@gmx.de>
These patches are an attempt to make Git's startup sequence a bit less
surprising.
The idea here is to discover the .git/ directory gently (i.e. without
changing the current working directory, or global variables), and to use
it to read the .git/config file early, before we actually called
setup_git_directory() (if we ever do that).
This also allows us to fix the early config e.g. to determine the pager
or to resolve aliases in a non-surprising manner.
The first iteration of this patch series still tried to be clever and to
avoid having to disentangle the side effects from the
setup_git_directory_gently_1() function simply by duplicating the logic.
However, Peff suggested in a very short sentence that this would not fly
well.
Little did I know that I would spend the better part of an entire week
on trying to address that innocuous comment! There are simply *so many*
side effects in that code. Who would have thought that a function
called check_repository_format() would set global variables?
But after all that work, I am actually quite a bit satisfied with the
way things turned out.
My dirty little secret is that I actually need this for something else
entirely. I need to patch an internal version of Git to gather
statistics, and to that end I need to read the config before and after
running every Git command. Hence the need for a gentle, and correct
early config.
Notes:
- I do not handle dashed invocations of `init` and `clone` correctly.
That is, even if `git-init` and `git-clone` clearly do not want to
read the local config, they do. It does not matter all that much
because they do not use a pager, but still. It is a wart.
- The read_early_config() function is still called multiple times,
re-reading all the config files and re-discovering the .git/ directory
multiple times, which is quite wasteful. I was tempted to take care of
that but I must not run the danger to spread myself even thinner these
days. If a patch adding that caching were to fly my way, I'd gladly
integrate it, of course... ;-)
Changes since v1:
- the discover_git_directory() function is no longer completely separate
from setup_git_directory(), but a callee of the latter.
- t7006 succeeds now (I removed the incorrect test case in favor of one
that verifies that setup_git_directory() was not run via the tell-tale
that the current working directory has not changed when the pager
runs).
Johannes Schindelin (9):
t7006: replace dubious test
setup_git_directory(): use is_dir_sep() helper
setup_git_directory(): avoid changing global state during discovery
Export the discover_git_directory() function
Make read_early_config() reusable
read_early_config(): special-case builtins that create a repository
read_early_config(): avoid .git/config hack when unneeded
read_early_config(): really discover .git/
Test read_early_config()
cache.h | 4 +-
config.c | 36 +++++++++
git.c | 3 +
pager.c | 31 -------
setup.c | 211 +++++++++++++++++++++++++++++++-----------------
t/helper/test-config.c | 15 ++++
t/t1309-early-config.sh | 50 ++++++++++++
t/t7006-pager.sh | 18 ++++-
8 files changed, 257 insertions(+), 111 deletions(-)
create mode 100755 t/t1309-early-config.sh
base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
Published-As: https://github.com/dscho/git/releases/tag/early-config-v2
Fetch-It-Via: git fetch https://github.com/dscho/git early-config-v2
Interdiff vs v1:
diff --git a/builtin/am.c b/builtin/am.c
index 2b81dabddd9..f7a7a971fbe 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1791,7 +1791,7 @@ static int do_interactive(struct am_state *state)
}
strbuf_release(&msg);
} else if (*reply == 'v' || *reply == 'V') {
- const char *pager = git_pager(1, 1);
+ const char *pager = git_pager(1);
struct child_process cp = CHILD_PROCESS_INIT;
if (!pager)
diff --git a/builtin/blame.c b/builtin/blame.c
index 628ca237da6..cffc6265408 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2919,7 +2919,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
assign_blame(&sb, opt);
if (!incremental)
- setup_pager(1);
+ setup_pager();
free(final_commit_name);
diff --git a/builtin/grep.c b/builtin/grep.c
index f820e4b1c4d..9304c33e750 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1133,7 +1133,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
}
if (show_in_pager == default_pager)
- show_in_pager = git_pager(1, 1);
+ show_in_pager = git_pager(1);
if (show_in_pager) {
opt.color = 0;
opt.name_only = 1;
@@ -1268,7 +1268,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
die(_("option not supported with --recurse-submodules."));
if (!show_in_pager && !opt.status_only)
- setup_pager(1);
+ setup_pager();
if (!use_index && (untracked || cached))
die(_("--cached or --untracked cannot be used with --no-index."));
diff --git a/builtin/log.c b/builtin/log.c
index 96618d38cbf..55d20cc2d88 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
if (rev->line_level_traverse)
line_log_init(rev, line_cb.prefix, &line_cb.args);
- setup_pager(1);
+ setup_pager();
}
static void cmd_log_init(int argc, const char **argv, const char *prefix,
@@ -1600,7 +1600,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (!use_stdout)
output_directory = set_outdir(prefix, output_directory);
else
- setup_pager(1);
+ setup_pager();
if (output_directory) {
if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
diff --git a/builtin/var.c b/builtin/var.c
index 879867b8427..aedbb53a2da 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -19,7 +19,7 @@ static const char *editor(int flag)
static const char *pager(int flag)
{
- const char *pgm = git_pager(1, 1);
+ const char *pgm = git_pager(1);
if (!pgm)
pgm = "cat";
diff --git a/cache.h b/cache.h
index 4d89966d711..0af7141242f 100644
--- a/cache.h
+++ b/cache.h
@@ -518,6 +518,7 @@ extern void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
extern void setup_work_tree(void);
+extern const char *discover_git_directory(struct strbuf *gitdir);
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);
@@ -1428,7 +1429,7 @@ extern const char *fmt_name(const char *name, const char *email);
extern const char *ident_default_name(void);
extern const char *ident_default_email(void);
extern const char *git_editor(void);
-extern const char *git_pager(int stdout_is_tty, int discover_git_dir);
+extern const char *git_pager(int stdout_is_tty);
extern int git_ident_config(const char *, const char *, void *);
extern void reset_ident_date(void);
@@ -1797,8 +1798,7 @@ 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,
- int discover_git_dir);
+extern void read_early_config(config_fn_t cb, void *data);
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,
@@ -1994,12 +1994,12 @@ __attribute__((format (printf, 2, 3)))
extern void write_file(const char *path, const char *fmt, ...);
/* pager.c */
-extern void setup_pager(int discover_git_dir);
+extern void setup_pager(void);
extern int pager_in_use(void);
extern int pager_use_color;
extern int term_columns(void);
extern int decimal_width(uintmax_t);
-extern int check_pager_config(const char *cmd, int discover_git_dir);
+extern int check_pager_config(const char *cmd);
extern void prepare_pager_args(struct child_process *, const char *pager);
extern const char *editor_program;
@@ -2070,7 +2070,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
/* setup.c */
struct startup_info {
- int have_repository;
+ int have_repository, creating_repository;
const char *prefix;
};
extern struct startup_info *startup_info;
diff --git a/config.c b/config.c
index c9f191e1fe3..bcda397d42e 100644
--- a/config.c
+++ b/config.c
@@ -1412,106 +1412,32 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
}
}
-/*
- * A "string_list_each_func_t" function that canonicalizes an entry
- * from GIT_CEILING_DIRECTORIES using real_path_if_valid(), or
- * discards it if unusable. The presence of an empty entry in
- * GIT_CEILING_DIRECTORIES turns off canonicalization for all
- * subsequent entries.
- */
-static int canonicalize_ceiling_entry(struct string_list_item *item,
- void *cb_data)
-{
- int *empty_entry_found = cb_data;
- char *ceil = item->string;
-
- if (!*ceil) {
- *empty_entry_found = 1;
- return 0;
- } else if (!is_absolute_path(ceil)) {
- return 0;
- } else if (*empty_entry_found) {
- /* Keep entry but do not canonicalize it */
- return 1;
- } else {
- const char *real_path = real_path_if_valid(ceil);
- if (!real_path)
- return 0;
- free(item->string);
- item->string = xstrdup(real_path);
- return 1;
- }
-}
-
-/*
- * Note that this is a really dirty hack that replicates what the
- * setup_git_directory() function does, without changing the current
- * working directory. The crux of the problem is that we cannot run
- * setup_git_directory() early on in git's setup, so we have to
- * duplicate the work that setup_git_directory() would otherwise do.
- */
-static int discover_git_directory_gently(struct strbuf *result)
-{
- const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
- int ceiling_offset = -1;
- const char *p;
-
- if (strbuf_getcwd(result) < 0)
- return -1;
- p = real_path_if_valid(result->buf);
- if (!p)
- return -1;
- strbuf_reset(result);
- strbuf_addstr(result, p);
-
- if (env_ceiling_dirs) {
- struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
- int empty_entry_found = 0;
-
- string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP,
- -1);
- filter_string_list(&ceiling_dirs, 0, canonicalize_ceiling_entry,
- &empty_entry_found);
- ceiling_offset = longest_ancestor_length(result->buf,
- &ceiling_dirs);
- string_list_clear(&ceiling_dirs, 0);
- }
-
- if (ceiling_offset < 0 && has_dos_drive_prefix(result->buf))
- ceiling_offset = 1;
-
- for (;;) {
- int len = result->len, i;
-
- strbuf_addstr(result, "/" DEFAULT_GIT_DIR_ENVIRONMENT);
- p = read_gitfile_gently(result->buf, &i);
- if (p) {
- strbuf_reset(result);
- strbuf_addstr(result, p);
- return 0;
- }
- if (is_git_directory(result->buf))
- return 0;
- strbuf_setlen(result, len);
- if (is_git_directory(result->buf))
- return 0;
- for (i = len; --i > ceiling_offset; )
- if (is_dir_sep(result->buf[i]))
- break;
- if (i <= ceiling_offset)
- return -1;
- strbuf_setlen(result, i);
- }
-}
-
-void read_early_config(config_fn_t cb, void *data, int discover_git_dir)
+void read_early_config(config_fn_t cb, void *data)
{
struct strbuf buf = STRBUF_INIT;
git_config_with_options(cb, data, NULL, 1);
- if (discover_git_dir && !have_git_dir() &&
- !discover_git_directory_gently(&buf)) {
+ /*
+ * Note that this is a really dirty hack that does the wrong thing in
+ * many cases. The crux of the problem is that we cannot run
+ * setup_git_directory() early on in git's setup, so we have no idea if
+ * we are in a repository or not, and therefore are not sure whether
+ * and how to read repository-local config.
+ *
+ * So if we _aren't_ in a repository (or we are but we would reject its
+ * core.repositoryformatversion), we'll read whatever is in .git/config
+ * blindly. Similarly, if we _are_ in a repository, but not at the
+ * root, we'll fail to find .git/config (because it's really
+ * ../.git/config, etc). See t7006 for a complete set of failures.
+ *
+ * However, we have historically provided this hack because it does
+ * work some of the time (namely when you are at the top-level of a
+ * valid repository), and would rarely make things worse (i.e., you do
+ * not generally have a .git/config file sitting around).
+ */
+ if (!startup_info->creating_repository && !have_git_dir() &&
+ discover_git_directory(&buf)) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
diff --git a/diff.c b/diff.c
index 1e1d3b85c2a..051761be405 100644
--- a/diff.c
+++ b/diff.c
@@ -5259,6 +5259,6 @@ void setup_diff_pager(struct diff_options *opt)
* --exit-code" in hooks and other scripts, we do not do so.
*/
if (!DIFF_OPT_TST(opt, EXIT_WITH_STATUS) &&
- check_pager_config("diff", 1) != 0)
- setup_pager(1);
+ check_pager_config("diff") != 0)
+ setup_pager();
}
diff --git a/git.c b/git.c
index d4712b25fee..9fb9bb90a21 100644
--- a/git.c
+++ b/git.c
@@ -61,13 +61,13 @@ static void restore_env(int external_alias)
}
}
-static void commit_pager_choice(int discover_git_dir) {
+static void commit_pager_choice(void) {
switch (use_pager) {
case 0:
setenv("GIT_PAGER", "cat", 1);
break;
case 1:
- setup_pager(discover_git_dir);
+ setup_pager();
break;
default:
break;
@@ -261,7 +261,7 @@ static int handle_alias(int *argcp, const char ***argv)
if (alias_string[0] == '!') {
struct child_process child = CHILD_PROCESS_INIT;
- commit_pager_choice(1);
+ commit_pager_choice();
restore_env(1);
child.use_shell = 1;
@@ -318,13 +318,12 @@ static int handle_alias(int *argcp, const char ***argv)
#define RUN_SETUP (1<<0)
#define RUN_SETUP_GENTLY (1<<1)
#define USE_PAGER (1<<2)
-#define CREATES_GIT_DIR (1<<3)
/*
* require working tree to be present -- anything uses this needs
* RUN_SETUP for reading from the configuration file.
*/
-#define NEED_WORK_TREE (1<<4)
-#define SUPPORT_SUPER_PREFIX (1<<5)
+#define NEED_WORK_TREE (1<<3)
+#define SUPPORT_SUPER_PREFIX (1<<4)
struct cmd_struct {
const char *cmd;
@@ -338,6 +337,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
struct stat st;
const char *prefix;
+ if (p->fn == cmd_init_db || p->fn == cmd_clone)
+ startup_info->creating_repository = 1;
+
prefix = NULL;
help = argc == 2 && !strcmp(argv[1], "-h");
if (!help) {
@@ -349,7 +351,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
}
if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
- use_pager = check_pager_config(p->cmd, !(p->option & CREATES_GIT_DIR));
+ use_pager = check_pager_config(p->cmd);
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;
@@ -357,7 +359,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
trace_repo_setup(prefix);
}
- commit_pager_choice(!(p->option & CREATES_GIT_DIR));
+ commit_pager_choice();
if (!help && get_super_prefix()) {
if (!(p->option & SUPPORT_SUPER_PREFIX))
@@ -413,7 +415,7 @@ static struct cmd_struct commands[] = {
{ "cherry", cmd_cherry, RUN_SETUP },
{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
- { "clone", cmd_clone, CREATES_GIT_DIR },
+ { "clone", cmd_clone },
{ "column", cmd_column, RUN_SETUP_GENTLY },
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
@@ -440,7 +442,7 @@ static struct cmd_struct commands[] = {
{ "hash-object", cmd_hash_object },
{ "help", cmd_help },
{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
- { "init", cmd_init_db, CREATES_GIT_DIR },
+ { "init", cmd_init_db },
{ "init-db", cmd_init_db },
{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
{ "log", cmd_log, RUN_SETUP },
@@ -585,8 +587,8 @@ static void execv_dashed_external(const char **argv)
die("%s doesn't support --super-prefix", argv[0]);
if (use_pager == -1)
- use_pager = check_pager_config(argv[0], 1);
- commit_pager_choice(1);
+ use_pager = check_pager_config(argv[0]);
+ commit_pager_choice();
argv_array_pushf(&cmd.args, "git-%s", argv[0]);
argv_array_pushv(&cmd.args, argv + 1);
@@ -684,7 +686,7 @@ int cmd_main(int argc, const char **argv)
skip_prefix(argv[0], "--", &argv[0]);
} else {
/* The user didn't specify a command; give them help */
- commit_pager_choice(1);
+ commit_pager_choice();
printf("usage: %s\n\n", git_usage_string);
list_common_cmds_help();
printf("\n%s\n", _(git_more_info_string));
diff --git a/pager.c b/pager.c
index 16b3cbe2320..73ca8bc3b17 100644
--- a/pager.c
+++ b/pager.c
@@ -43,7 +43,7 @@ static int core_pager_config(const char *var, const char *value, void *data)
return 0;
}
-const char *git_pager(int stdout_is_tty, int discover_git_dir)
+const char *git_pager(int stdout_is_tty)
{
const char *pager;
@@ -53,8 +53,7 @@ const char *git_pager(int stdout_is_tty, int discover_git_dir)
pager = getenv("GIT_PAGER");
if (!pager) {
if (!pager_program)
- read_early_config(core_pager_config, NULL,
- discover_git_dir);
+ read_early_config(core_pager_config, NULL);
pager = pager_program;
}
if (!pager)
@@ -101,9 +100,9 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
setup_pager_env(&pager_process->env_array);
}
-void setup_pager(int discover_git_dir)
+void setup_pager(void)
{
- const char *pager = git_pager(isatty(1), discover_git_dir);
+ const char *pager = git_pager(isatty(1));
if (!pager)
return;
@@ -209,7 +208,7 @@ static int pager_command_config(const char *var, const char *value, void *vdata)
}
/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
-int check_pager_config(const char *cmd, int discover_git_dir)
+int check_pager_config(const char *cmd)
{
struct pager_command_config_data data;
@@ -217,7 +216,7 @@ int check_pager_config(const char *cmd, int discover_git_dir)
data.want = -1;
data.value = NULL;
- read_early_config(pager_command_config, &data, discover_git_dir);
+ read_early_config(pager_command_config, &data);
if (data.value)
pager_program = data.value;
diff --git a/setup.c b/setup.c
index 967f289f1ef..7ceca6cc6ef 100644
--- a/setup.c
+++ b/setup.c
@@ -816,50 +816,49 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
}
}
+enum discovery_result {
+ GIT_DIR_NONE = 0,
+ GIT_DIR_EXPLICIT,
+ GIT_DIR_DISCOVERED,
+ GIT_DIR_BARE,
+ /* these are errors */
+ GIT_DIR_HIT_CEILING = -1,
+ GIT_DIR_HIT_MOUNT_POINT = -2
+};
+
/*
* We cannot decide in this function whether we are in the work tree or
* not, since the config can only be read _after_ this function was called.
+ *
+ * Also, we avoid changing any global state (such as the current working
+ * directory) to allow early callers.
+ *
+ * The directory where the search should start needs to be passed in via the
+ * `dir` parameter; upon return, the `dir` buffer will contain the path of
+ * the directory where the search ended, and `gitdir` will contain the path of
+ * the discovered .git/ directory, if any. This path may be relative against
+ * `dir` (i.e. *not* necessarily the cwd).
*/
-static const char *setup_git_directory_gently_1(int *nongit_ok)
+static enum discovery_result discover_git_directory_1(struct strbuf *dir,
+ struct strbuf *gitdir)
{
const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
- static struct strbuf cwd = STRBUF_INIT;
- const char *gitdirenv, *ret;
- char *gitfile;
- int offset, offset_parent, ceil_offset = -1;
+ const char *gitdirenv;
+ int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 : 1;
dev_t current_device = 0;
int one_filesystem = 1;
/*
- * We may have read an incomplete configuration before
- * setting-up the git directory. If so, clear the cache so
- * that the next queries to the configuration reload complete
- * configuration (including the per-repo config file that we
- * ignored previously).
- */
- git_config_clear();
-
- /*
- * Let's assume that we are in a git repository.
- * If it turns out later that we are somewhere else, the value will be
- * updated accordingly.
- */
- if (nongit_ok)
- *nongit_ok = 0;
-
- if (strbuf_getcwd(&cwd))
- die_errno(_("Unable to read current working directory"));
- offset = cwd.len;
-
- /*
* If GIT_DIR is set explicitly, we're not going
* to do any discovery, but we still do repository
* validation.
*/
gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
- if (gitdirenv)
- return setup_explicit_git_dir(gitdirenv, &cwd, nongit_ok);
+ if (gitdirenv) {
+ strbuf_addstr(gitdir, gitdirenv);
+ return GIT_DIR_EXPLICIT;
+ }
if (env_ceiling_dirs) {
int empty_entry_found = 0;
@@ -867,15 +866,15 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
string_list_split(&ceiling_dirs, env_ceiling_dirs, PATH_SEP, -1);
filter_string_list(&ceiling_dirs, 0,
canonicalize_ceiling_entry, &empty_entry_found);
- ceil_offset = longest_ancestor_length(cwd.buf, &ceiling_dirs);
+ ceil_offset = longest_ancestor_length(dir->buf, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0);
}
- if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
- ceil_offset = 1;
+ if (ceil_offset < 0)
+ ceil_offset = min_offset - 2;
/*
- * Test in the following order (relative to the cwd):
+ * Test in the following order (relative to the dir):
* - .git (file containing "gitdir: <path>")
* - .git/
* - ./ (bare)
@@ -887,61 +886,123 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
*/
one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
if (one_filesystem)
- current_device = get_device_or_die(".", NULL, 0);
+ current_device = get_device_or_die(dir->buf, NULL, 0);
for (;;) {
- gitfile = (char*)read_gitfile(DEFAULT_GIT_DIR_ENVIRONMENT);
- if (gitfile)
- gitdirenv = gitfile = xstrdup(gitfile);
- else {
- if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
- gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
- }
-
+ int offset = dir->len;
+
+ if (offset > min_offset)
+ strbuf_addch(dir, '/');
+ strbuf_addstr(dir, DEFAULT_GIT_DIR_ENVIRONMENT);
+ gitdirenv = read_gitfile(dir->buf);
+ if (!gitdirenv && is_git_directory(dir->buf))
+ gitdirenv = DEFAULT_GIT_DIR_ENVIRONMENT;
+ strbuf_setlen(dir, offset);
if (gitdirenv) {
- ret = setup_discovered_git_dir(gitdirenv,
- &cwd, offset,
- nongit_ok);
- free(gitfile);
- return ret;
+ strbuf_addstr(gitdir, gitdirenv);
+ return GIT_DIR_DISCOVERED;
}
- free(gitfile);
- if (is_git_directory("."))
- return setup_bare_git_dir(&cwd, offset, nongit_ok);
-
- offset_parent = offset;
- while (--offset_parent > ceil_offset && cwd.buf[offset_parent] != '/');
- if (offset_parent <= ceil_offset)
- return setup_nongit(cwd.buf, nongit_ok);
- if (one_filesystem) {
- dev_t parent_device = get_device_or_die("..", cwd.buf,
- offset);
- if (parent_device != current_device) {
- if (nongit_ok) {
- if (chdir(cwd.buf))
- die_errno(_("Cannot come back to cwd"));
- *nongit_ok = 1;
- return NULL;
- }
- strbuf_setlen(&cwd, offset);
- die(_("Not a git repository (or any parent up to mount point %s)\n"
- "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
- cwd.buf);
- }
- }
- if (chdir("..")) {
- strbuf_setlen(&cwd, offset);
- die_errno(_("Cannot change to '%s/..'"), cwd.buf);
+ if (is_git_directory(dir->buf)) {
+ strbuf_addstr(gitdir, ".");
+ return GIT_DIR_BARE;
}
- offset = offset_parent;
+
+ if (offset <= min_offset)
+ return GIT_DIR_HIT_CEILING;
+
+ while (--offset > ceil_offset && !is_dir_sep(dir->buf[offset]));
+ if (offset <= ceil_offset)
+ return GIT_DIR_HIT_CEILING;
+
+ strbuf_setlen(dir, offset > min_offset ? offset : min_offset);
+ if (one_filesystem &&
+ current_device != get_device_or_die(dir->buf, NULL, offset))
+ return GIT_DIR_HIT_MOUNT_POINT;
+ }
+}
+
+const char *discover_git_directory(struct strbuf *gitdir)
+{
+ struct strbuf dir = STRBUF_INIT;
+ int len;
+
+ if (strbuf_getcwd(&dir))
+ return NULL;
+
+ len = dir.len;
+ if (discover_git_directory_1(&dir, gitdir) < 0) {
+ strbuf_release(&dir);
+ return NULL;
+ }
+
+ if (dir.len < len && !is_absolute_path(gitdir->buf)) {
+ strbuf_addch(&dir, '/');
+ strbuf_insert(gitdir, 0, dir.buf, dir.len);
}
+ strbuf_release(&dir);
+
+ return gitdir->buf;
}
const char *setup_git_directory_gently(int *nongit_ok)
{
+ struct strbuf cwd = STRBUF_INIT, dir = STRBUF_INIT, gitdir = STRBUF_INIT;
const char *prefix;
- prefix = setup_git_directory_gently_1(nongit_ok);
+ /*
+ * We may have read an incomplete configuration before
+ * setting-up the git directory. If so, clear the cache so
+ * that the next queries to the configuration reload complete
+ * configuration (including the per-repo config file that we
+ * ignored previously).
+ */
+ git_config_clear();
+
+ /*
+ * Let's assume that we are in a git repository.
+ * If it turns out later that we are somewhere else, the value will be
+ * updated accordingly.
+ */
+ if (nongit_ok)
+ *nongit_ok = 0;
+
+ if (strbuf_getcwd(&cwd))
+ die_errno(_("Unable to read current working directory"));
+ strbuf_addbuf(&dir, &cwd);
+
+ switch (discover_git_directory_1(&dir, &gitdir)) {
+ case GIT_DIR_NONE:
+ prefix = NULL;
+ break;
+ case GIT_DIR_EXPLICIT:
+ prefix = setup_explicit_git_dir(gitdir.buf, &cwd, nongit_ok);
+ break;
+ case GIT_DIR_DISCOVERED:
+ if (dir.len < cwd.len && chdir(dir.buf))
+ die(_("Cannot change to '%s'"), dir.buf);
+ prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
+ nongit_ok);
+ break;
+ case GIT_DIR_BARE:
+ if (dir.len < cwd.len && chdir(dir.buf))
+ die(_("Cannot change to '%s'"), dir.buf);
+ prefix = setup_bare_git_dir(&cwd, dir.len, nongit_ok);
+ break;
+ case GIT_DIR_HIT_CEILING:
+ prefix = setup_nongit(cwd.buf, nongit_ok);
+ break;
+ case GIT_DIR_HIT_MOUNT_POINT:
+ if (nongit_ok) {
+ *nongit_ok = 1;
+ return NULL;
+ }
+ die(_("Not a git repository (or any parent up to mount point %s)\n"
+ "Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)."),
+ dir.buf);
+ default:
+ die("BUG: unhandled discover_git_directory() result");
+ }
+
if (prefix)
setenv(GIT_PREFIX_ENVIRONMENT, prefix, 1);
else
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 51050695876..8e3ed6a76cb 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], 1);
+ read_early_config(early_config_cb, (void *)argv[2]);
return 0;
}
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index c8dc665f2fd..bf89340988b 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -360,27 +360,37 @@ test_pager_choices 'git aliasedlog'
test_default_pager expect_success 'git -p aliasedlog'
test_PAGER_overrides expect_success 'git -p aliasedlog'
test_core_pager_overrides expect_success 'git -p aliasedlog'
-test_core_pager_subdir expect_failure 'git -p aliasedlog'
+test_core_pager_subdir expect_success 'git -p aliasedlog'
test_GIT_PAGER_overrides expect_success 'git -p aliasedlog'
test_default_pager expect_success 'git -p true'
test_PAGER_overrides expect_success 'git -p true'
test_core_pager_overrides expect_success 'git -p true'
-test_core_pager_subdir expect_failure 'git -p true'
+test_core_pager_subdir expect_success 'git -p true'
test_GIT_PAGER_overrides expect_success 'git -p true'
test_default_pager expect_success test_must_fail 'git -p request-pull'
test_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_core_pager_overrides expect_success test_must_fail 'git -p request-pull'
-test_core_pager_subdir expect_failure test_must_fail 'git -p request-pull'
+test_core_pager_subdir expect_success test_must_fail 'git -p request-pull'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_default_pager expect_success test_must_fail 'git -p'
test_PAGER_overrides expect_success test_must_fail 'git -p'
test_local_config_ignored expect_failure test_must_fail 'git -p'
-test_no_local_config_subdir expect_success test_must_fail 'git -p'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
+test_expect_success TTY 'core.pager in repo config works and retains cwd' '
+ sane_unset GIT_PAGER &&
+ test_config core.pager "cat >cwd-retained" &&
+ (
+ cd sub &&
+ rm -f cwd-retained &&
+ test_terminal git -p rev-parse HEAD &&
+ test -e cwd-retained
+ )
+'
+
test_doesnt_paginate expect_failure test_must_fail 'git -p nonsense'
test_pager_choices 'git shortlog'
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply
* [PATCH v2 7/9] read_early_config(): avoid .git/config hack when unneeded
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
So far, we only look whether the startup_info claims to have seen a
git_dir.
However, do_git_config_sequence() (and consequently the
git_config_with_options() call used by read_early_config() asks the
have_git_dir() function whether we have a .git/ directory, which in turn
also looks at git_dir and at the environment variable GIT_DIR. And when
this is the case, the repository config is handled already, so we do not
have to do that again explicitly.
Let's just use the same function, have_git_dir(), to determine whether we
have to handle .git/config explicitly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 980fcc6ff2e..3ee5e5a15d6 100644
--- a/config.c
+++ b/config.c
@@ -1434,8 +1434,7 @@ void read_early_config(config_fn_t cb, void *data)
* valid repository), and would rarely make things worse (i.e., you do
* not generally have a .git/config file sitting around).
*/
- if (!startup_info->creating_repository &&
- !startup_info->have_repository) {
+ if (!startup_info->creating_repository && !have_git_dir()) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* [PATCH v2 9/9] Test read_early_config()
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
So far, we had no explicit tests of that function.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/helper/test-config.c | 15 +++++++++++++++
t/t1309-early-config.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
create mode 100755 t/t1309-early-config.sh
diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 83a4f2ab869..8e3ed6a76cb 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -66,6 +66,16 @@ static int iterate_cb(const char *var, const char *value, void *data)
return 0;
}
+static int early_config_cb(const char *var, const char *value, void *vdata)
+{
+ const char *key = vdata;
+
+ if (!strcmp(key, var))
+ printf("%s\n", value);
+
+ return 0;
+}
+
int cmd_main(int argc, const char **argv)
{
int i, val;
@@ -73,6 +83,11 @@ int cmd_main(int argc, const char **argv)
const struct string_list *strptr;
struct config_set cs;
+ if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
+ read_early_config(early_config_cb, (void *)argv[2]);
+ return 0;
+ }
+
setup_git_directory();
git_configset_init(&cs);
diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
new file mode 100755
index 00000000000..0c55dee514c
--- /dev/null
+++ b/t/t1309-early-config.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='Test read_early_config()'
+
+. ./test-lib.sh
+
+test_expect_success 'read early config' '
+ test_config early.config correct &&
+ test-config read_early_config early.config >output &&
+ test correct = "$(cat output)"
+'
+
+test_expect_success 'in a sub-directory' '
+ test_config early.config sub &&
+ mkdir -p sub &&
+ (
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test sub = "$(cat output)"
+'
+
+test_expect_success 'ceiling' '
+ test_config early.config ceiling &&
+ mkdir -p sub &&
+ (
+ GIT_CEILING_DIRECTORIES="$PWD" &&
+ export GIT_CEILING_DIRECTORIES &&
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test -z "$(cat output)"
+'
+
+test_expect_success 'ceiling #2' '
+ mkdir -p xdg/git &&
+ git config -f xdg/git/config early.config xdg &&
+ test_config early.config ceiling &&
+ mkdir -p sub &&
+ (
+ XDG_CONFIG_HOME="$PWD"/xdg &&
+ GIT_CEILING_DIRECTORIES="$PWD" &&
+ export GIT_CEILING_DIRECTORIES XDG_CONFIG_HOME &&
+ cd sub &&
+ test-config read_early_config early.config
+ ) >output &&
+ test xdg = "$(cat output)"
+'
+
+test_done
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* [PATCH v2 8/9] read_early_config(): really discover .git/
From: Johannes Schindelin @ 2017-03-03 2:04 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
Earlier, we punted and simply assumed that we are in the top-level
directory of the project, and that there is no .git file but a .git/
directory so that we can read directly from .git/config.
However, that is not necessarily true. We may be in a subdirectory. Or
.git may be a gitfile. Or the environment variable GIT_DIR may be set.
To remedy this situation, we just refactored the way
setup_git_directory() discovers the .git/ directory, to make it
reusable, and more importantly, to leave all global variables and the
current working directory alone.
Let's discover the .git/ directory correctly in read_early_config() by
using that new function.
This fixes 4 known breakages in t7006.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 9 +++++++--
| 8 ++++----
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c
index 3ee5e5a15d6..bcda397d42e 100644
--- a/config.c
+++ b/config.c
@@ -1414,6 +1414,8 @@ 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 buf = STRBUF_INIT;
+
git_config_with_options(cb, data, NULL, 1);
/*
@@ -1434,13 +1436,16 @@ void read_early_config(config_fn_t cb, void *data)
* valid repository), and would rarely make things worse (i.e., you do
* not generally have a .git/config file sitting around).
*/
- if (!startup_info->creating_repository && !have_git_dir()) {
+ if (!startup_info->creating_repository && !have_git_dir() &&
+ discover_git_directory(&buf)) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
- repo_config.file = ".git/config";
+ strbuf_addstr(&buf, "/config");
+ repo_config.file = buf.buf;
git_config_with_options(cb, data, &repo_config, 1);
}
+ strbuf_release(&buf);
}
static void git_config_check_init(void);
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 427bfc605ad..bf89340988b 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -360,19 +360,19 @@ test_pager_choices 'git aliasedlog'
test_default_pager expect_success 'git -p aliasedlog'
test_PAGER_overrides expect_success 'git -p aliasedlog'
test_core_pager_overrides expect_success 'git -p aliasedlog'
-test_core_pager_subdir expect_failure 'git -p aliasedlog'
+test_core_pager_subdir expect_success 'git -p aliasedlog'
test_GIT_PAGER_overrides expect_success 'git -p aliasedlog'
test_default_pager expect_success 'git -p true'
test_PAGER_overrides expect_success 'git -p true'
test_core_pager_overrides expect_success 'git -p true'
-test_core_pager_subdir expect_failure 'git -p true'
+test_core_pager_subdir expect_success 'git -p true'
test_GIT_PAGER_overrides expect_success 'git -p true'
test_default_pager expect_success test_must_fail 'git -p request-pull'
test_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_core_pager_overrides expect_success test_must_fail 'git -p request-pull'
-test_core_pager_subdir expect_failure test_must_fail 'git -p request-pull'
+test_core_pager_subdir expect_success test_must_fail 'git -p request-pull'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p request-pull'
test_default_pager expect_success test_must_fail 'git -p'
@@ -380,7 +380,7 @@ test_PAGER_overrides expect_success test_must_fail 'git -p'
test_local_config_ignored expect_failure test_must_fail 'git -p'
test_GIT_PAGER_overrides expect_success test_must_fail 'git -p'
-test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
+test_expect_success TTY 'core.pager in repo config works and retains cwd' '
sane_unset GIT_PAGER &&
test_config core.pager "cat >cwd-retained" &&
(
--
2.12.0.windows.1.3.g8a117c48243
^ permalink raw reply related
* Re: [PATCH v1] Travis: also test on 32-bit Linux
From: Johannes Schindelin @ 2017-03-03 2:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, git
In-Reply-To: <xmqqinnrd098.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 2 Mar 2017, Junio C Hamano wrote:
> Another question is which v3 people mean in the discussion, when you
> and Dscho work on improvements at the same time and each post the
> "next" version marked as "v3", and they comment on one of them?
But then, Lars & I communicate in a more direct way than the mailing list
when discussing teeny tiny details such as: "could you have a look at my
mail" or "how would you change .travis.yml to do XYZ".
With that level of communication, there is almost no danger of two v3s.
Ciao,
Johannes
^ permalink raw reply
* Re: log -S/-G (aka pickaxe) searches binary files by default
From: Junio C Hamano @ 2017-03-03 1:36 UTC (permalink / raw)
To: Thomas Braun; +Cc: GIT Mailing-list
In-Reply-To: <7a0992eb-adb9-a7a1-cfaa-3384bc4d3e5c@virtuell-zuhause.de>
On Thu, Mar 2, 2017 at 4:52 PM, Thomas Braun
<thomas.braun@virtuell-zuhause.de> wrote:
>
> I happen to have quite large binary files in my repos.
>
> Today I realized that a line like
> git log -G a
> searches also files found to be binary (or explicitly marked as binary).
>
> Is that on purpose?
No, it's a mere oversight (as I do not think I never even thought
about special casing binary
files from day one, it is unlikely that you would find _any_ old
version of Git that behaves
differently).
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-03-03 2:19 UTC (permalink / raw)
To: Mike Hommey
Cc: Joey Hess, Junio C Hamano, Jeff King, Ian Jackson,
Git Mailing List
In-Reply-To: <20170303015047.p4lpkdzp4hbpz5vi@glandium.org>
On Thu, Mar 2, 2017 at 5:50 PM, Mike Hommey <mh@glandium.org> wrote:
>
> What if the "object version" is a hash of the content (as opposed to
> header + content like the normal git hash)?
It doesn't actually matter for that attack.
The concept of the attack is actually fairly simple: generate a lot of
collisions in the first hash (and they outline how you only need to
generate 't' serial collisions and turn them into 2**t collisions),
and then just check those collisions against the second hash.
If you have enough collisions in the first one, having a collision in
the second one is inevitable just from the birthday rule.
Now, *In*practice* that attack is not very easy to do. Each collision
is still hard to generate. And because of the git object rules (the
size has to match), it limits you a bit in what collisions you
generate.
But the fact that git requires the right header can be considered just
a variation on the initial state to SHA1, and then the additional
requirement might be as easy as just saying that your collision
generation function just always needs to generate a fixed-size block
(which could be just 64 bytes - the SHA1 blocking size).
So assuming you can arbitrarily generate collisions (brute force:
2**80 operations) you could make the rule be that you generate
something that starts with one 64-byte block that matches the git
rules:
"blob 6454\0"..pad with repeating NUL bytes..
and then you generate 100 pairs of 64-byte SHA1 collisions (where the
first starts with the initial value of that fixed blob prefix, the
next with the state after the first block, etc etc).
Now you can generate 2**100 different sequences that all are exactly
6464 bytes (101 64-byte blocks) and all have the same SHA1 - all all
share that first fixed 64-byte block.
You needed "only" on the order of 100 * 2**80 SHA1 operations to do
that in theory.
An since you have 2**100 objects, you know that you will have a likely
birthday paradox even if your secondary hash is 200 bits long.
So all in all, you generated a collision in on the order of 2**100 operations.
So instead of getting the security of "160+200" bits, you only got 200
bits worth of real collision resistance.
NOTE!! All of the above is very much assuming "brute force". If you
can brute-force the hash, you can completely break any hash. The input
block to SHA1 is 64 bytes, so by definition you have 512 bits of data
to play with, and you're generating a 160-bit output: there is no
question what-so-ever that you couldn't generate any hash you want if
you brute-force things.
The place where things like having a fixed object header can help is
when the attack in question requires some repeated patterns. For
example, if you're not brute-forcing things, your attack on the hash
will likely involve using very particular patterns to change a number
of bits in certain ways, and then combining those particular patterns
to get the hash collision you wanted. And *that* is when you may need
to add some particular data to the middle to make the end result be a
particular match.
But a brute-force attack definitely doesn't need size changes. You can
make the size be pretty much anything you want (modulo really small
inputs, of course - a one-byte input only has 256 different possible
hashes ;) if you have the resources to just go and try every possible
combination until you get the hash you wanted.
I may have overly simplified the paper, but that's the basic idea.
Linus
^ permalink raw reply
* Re: [PATCH v2 1/9] t7006: replace dubious test
From: Jeff King @ 2017-03-03 3:36 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <8f79df34e63a37c052437dd022269b4a6c495108.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:02AM +0100, Johannes Schindelin wrote:
> The intention of that test case, however, was to verify that the
> setup_git_directory() function has not run, because it changes global
> state such as the current working directory.
>
> To keep that spirit, but fix the incorrect assumption, this patch
> replaces that test case by a new one that verifies that the pager is
> run in the subdirectory, i.e. that the current working directory has
> not been changed at the time the pager is configured and launched, even
> if the `rev-parse` command requires a .git/ directory and *will* change
> the working directory.
This is a great solution to the question I had in v1 ("how do we know
when setup_git_directory() is run?"). It not only checks the
internal-code case that we care about, but it also shows how real users
would get bit if we do the wrong thing.
> +test_expect_failure TTY 'core.pager in repo config works and retains cwd' '
> + sane_unset GIT_PAGER &&
> + test_config core.pager "cat >cwd-retained" &&
> + (
> + cd sub &&
> + rm -f cwd-retained &&
> + test_terminal git -p rev-parse HEAD &&
> + test -e cwd-retained
> + )
> +'
Does this actually need TTY and test_terminal? You replace the pager
with something that doesn't care about the terminal, and "-p" should
unconditionally turn it on.
(Also a minor nit: we usually prefer test_path_is_file to "test -e"
these days).
-Peff
^ permalink raw reply
* Re: [PATCH v2 3/9] setup_git_directory(): avoid changing global state during discovery
From: Jeff King @ 2017-03-03 4:24 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <49f66f2b723af38a58c17bc8ae413bcee69e2a2f.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:11AM +0100, Johannes Schindelin wrote:
> For historical reasons, Git searches for the .git/ directory (or the
> .git file) by changing the working directory successively to the parent
> directory of the current directory, until either anything was found or
> until a ceiling or a mount point is hit.
This is starting to get into the meat of changes we've been putting off
writing for ages. I'll read with my fingers crossed. :)
> Further global state may be changed, depending on the actual type of
> discovery, e.g. the global variable `repository_format_precious_objects`
> is set in the `check_repository_format_gently()` function (which is a
> bit surprising, given the function name).
It's gentle in the sense that if it does not find a valid repo, it
touches no state. I do suspect the functions you want are the ones it
builds on: read_repository_format() and verify_repository_format(),
which I added not too long ago for the exact purpose of checking repo
config without touching anything global.
> We do have a use case where we would like to find the .git/ directory
> without having any global state touched, though: when we read the early
> config e.g. for the pager or for alias expansion.
>
> Let's just rename the function `setup_git_directory_gently_1()` to
> `discover_git_directory()` and move all code that changes any global
> state back into `setup_git_directory_gently()`.
Given the earlier paragraph, it sounds like you want to move the
global-state-changing parts out of check_repository_format_gently(). But
that wouldn't be right; it's triggered separate from the discovery
process by things like enter_repo().
However, I don't see that happening in the patch, which is good. I just
wonder if the earlier paragraph should really be complaining about how
setup_git_directory() (and its callees) touches a lot of global state,
not check_repository_format_gently(), whose use is but one of multiple
global-state modifications.
> Note: the new loop is a *little* tricky, as we have to handle the root
> directory specially: we cannot simply strip away the last component
> including the slash, as the root directory only has that slash. To remedy
> that, we introduce the `min_offset` variable that holds the minimal length
> of an absolute path, and using that to special-case the root directory,
> including an early exit before trying to find the parent of the root
> directory.
I wondered how t1509 fared with this, as it is the only test of
repositories at the root directory (and it is not run by default because
it involves a bunch of flaky and expensive chroot setup). Unfortunately
it seems to fail with your patch:
expecting success:
test_cmp_val "foo/" "$(git rev-parse --show-prefix)"
--- expected
+++ result
@@ -1 +1 @@
-foo/
+oo/
not ok 66 - auto gitdir, foo: prefix
Could the problem be this:
> + int ceil_offset = -1, min_offset = has_dos_drive_prefix(dir->buf) ? 3 : 1;
> [...]
> - if (ceil_offset < 0 && has_dos_drive_prefix(cwd.buf))
> - ceil_offset = 1;
> + if (ceil_offset < 0)
> + ceil_offset = min_offset - 2;
It works the same as before in the dos-drive case, but we'd get
ceil_offset = -1 otherwise. Which seems weird, but maybe I don't
understand how ceil_offset is supposed to work.
Interestingly, I don't think this is the bug, though. We still correctly
find /.git as a repository. The problem seems to happen later, in
setup_discovered_git_dir(), which does this:
/* Make "offset" point to past the '/', and add a '/' at the end */
offset++;
strbuf_addch(cwd, '/');
return cwd->buf + offset;
Here, "offset" is the length of the working tree path. The root
directory case differs from normal in that "offset" already accounts for
the trailing slash.
So I think the bug comes from:
> - ret = setup_discovered_git_dir(gitdirenv,
> - &cwd, offset,
> - nongit_ok);
> [...]
> + prefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,
> + nongit_ok);
The original knew that "offset" took into account the off-by-one for the
root, but that's lost when we use dir.len. I haven't studied it enough
to know the best solution, but I suspect you will.
Other than this bug, I very much like the direction that this patch is
taking us.
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/9] setup_git_directory(): use is_dir_sep() helper
From: Jeff King @ 2017-03-03 3:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <d1342d28fd402615f1f94d9190559070ed04b0d7.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:07AM +0100, Johannes Schindelin wrote:
> It is okay in practice to test for forward slashes in the output of
> getcwd(), because we go out of our way to convert backslashes to forward
> slashes in getcwd()'s output on Windows.
>
> Still, the correct way to test for a dir separator is by using the
> helper function we introduced for that very purpose. It also serves as a
> good documentation what the code tries to do (not "how").
Makes sense, but...
> @@ -910,7 +910,8 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
> return setup_bare_git_dir(&cwd, offset, nongit_ok);
>
> offset_parent = offset;
> - while (--offset_parent > ceil_offset && cwd.buf[offset_parent] != '/');
> + while (--offset_parent > ceil_offset &&
> + !is_dir_sep(dir->buf[offset_parent]));
What is "dir"? I'm guessing this patch got reordered and it should stay
as cwd.buf?
-Peff
^ permalink raw reply
* Re: [PATCH v2 9/9] Test read_early_config()
From: Jeff King @ 2017-03-03 5:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <f27a753830b8fb61a5276ce1d8eeba04ae4dbbfd.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:36AM +0100, Johannes Schindelin wrote:
> So far, we had no explicit tests of that function.
Makes sense. The pager tests fixed in an earlier commit were effectively
checking those, but I don't mind making it explicit.
-Peff
^ permalink raw reply
* Re: [PATCH v2 0/9] Fix the early config
From: Jeff King @ 2017-03-03 5:14 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <cover.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:03:56AM +0100, Johannes Schindelin wrote:
> These patches are an attempt to make Git's startup sequence a bit less
> surprising.
>
> The idea here is to discover the .git/ directory gently (i.e. without
> changing the current working directory, or global variables), and to use
> it to read the .git/config file early, before we actually called
> setup_git_directory() (if we ever do that).
Thanks for working on this. I think it's a huge improvement over the
status quo, and over the earlier attempt. I don't see anything hugely
wrong with this series, though I did note one bug, along with some minor
refinements.
> My dirty little secret is that I actually need this for something else
> entirely. I need to patch an internal version of Git to gather
> statistics, and to that end I need to read the config before and after
> running every Git command. Hence the need for a gentle, and correct
> early config.
We do something similar at GitHub, but it falls into two categories:
- stat-gathering that's on all the time, so doesn't need to look at
config (I'm not sure in your case if you want to trigger the
gathering with config, or if config is just one of the things you
are gathering).
- logging that is turned on selectively for some repos, but which
doesn't have to be looked up until we know we are in a repo
I looked into making something upstream-able, and my approach was to
allow GIT_TRACE_* variables to be specified in the config. But of course
that ran into the early-config problem (and I really wanted repo-level
config there, because I'd like to be able to turn on tracing for just a
single problematic repo).
So not really something you need to work on, but maybe food for thought
as you work on your internal project.
-Peff
^ permalink raw reply
* Re: log -S/-G (aka pickaxe) searches binary files by default
From: Jeff King @ 2017-03-03 5:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thomas Braun, GIT Mailing-list
In-Reply-To: <CAPc5daVSY5Z_+cpT1dHY-cM-TzNeu+Vzv+zouoOHW08PTFRQ7A@mail.gmail.com>
On Thu, Mar 02, 2017 at 05:36:17PM -0800, Junio C Hamano wrote:
> On Thu, Mar 2, 2017 at 4:52 PM, Thomas Braun
> <thomas.braun@virtuell-zuhause.de> wrote:
> >
> > I happen to have quite large binary files in my repos.
> >
> > Today I realized that a line like
> > git log -G a
> > searches also files found to be binary (or explicitly marked as binary).
> >
> > Is that on purpose?
>
> No, it's a mere oversight (as I do not think I never even thought
> about special casing binary
> files from day one, it is unlikely that you would find _any_ old
> version of Git that behaves
> differently).
The email focuses on "-G", and I think it is wrong to look in binary
files there, as "grep in diff" does not make sense for a binary file
that we would refuse to diff.
But the subject also mentions "-S". I always assumed it was intentional
to look in binary files there, as it is searching for a pure byte
sequence. I would not mind an option to disable that, but I think the
default should remain on.
-Peff
^ permalink raw reply
* Re: [PATCH v2 7/9] read_early_config(): avoid .git/config hack when unneeded
From: Jeff King @ 2017-03-03 4:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <db619060d18df21f3259653a52cd79f704b34566.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:28AM +0100, Johannes Schindelin wrote:
> So far, we only look whether the startup_info claims to have seen a
> git_dir.
>
> However, do_git_config_sequence() (and consequently the
> git_config_with_options() call used by read_early_config() asks the
> have_git_dir() function whether we have a .git/ directory, which in turn
> also looks at git_dir and at the environment variable GIT_DIR. And when
> this is the case, the repository config is handled already, so we do not
> have to do that again explicitly.
>
> Let's just use the same function, have_git_dir(), to determine whether we
> have to handle .git/config explicitly.
Good, this makes sense.
-Peff
^ permalink raw reply
* Re: [PATCH v2 6/9] read_early_config(): special-case builtins that create a repository
From: Jeff King @ 2017-03-03 4:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <33e692918c8d41507de5ec2b2e2d55982678408e.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:24AM +0100, Johannes Schindelin wrote:
> When the command we are about to execute wants to create a repository
> (i.e. the command is `init` or `clone`), we *must not* look for a
> repository config.
Hmm. I'm not sure if this one is worth the hackery here.
Yes, it would be wrong for init or clone to read something like
core.sharedrepository from a repo it happens to be in. But I wonder if
it would be cleaner to consider calls to read_early_config their own
"pre-command" stage that may respect global config, or config in a repo
directory you happen to be sitting in.
Because I think for aliases, we're going to end up having to do that
anyway (you won't know that your alias is "clone" until you've resolved
it!). And I think the pager fits into this "pre-command" concept, too
(we already have "-p" as a pre-command option on the command-line).
I dunno. It probably doesn't matter _too much_ either way. But it's one
less hack to maintain going forward, and it also makes your "git-init
respects the pager" into the normal, consistent thing.
-Peff
^ permalink raw reply
* Re: [PATCH v7 0/3] Conditional config include
From: Jeff King @ 2017-03-03 6:33 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy
Cc: git, Junio C Hamano, sschuberth, Matthieu Moy, Philip Oakley,
Ramsay Jones
In-Reply-To: <20170301112631.16497-1-pclouds@gmail.com>
On Wed, Mar 01, 2017 at 06:26:28PM +0700, Nguyễn Thái Ngọc Duy wrote:
> I don't have a good answer for Jeff's PS about includeIf ugliness. I
> agree that includeif is ugly but includeIf looks a bit better. I don't
> see a better option (if only "include" does not start or end with a
> vowel...). Maybe includewith? Suggestions are welcome.
I actually think "include-if" _looks_ better, although maybe the
inconsistency with "-" is something we don't want to encourage (though I
also think the implicit include.<cond>.path was OK, too). Feel free to
just ignore me. I will live with it either way.
For those following on the mailing list, there is some discussion at:
https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
I think that is mostly focused around another failing in the
error-handling of the config code, and that does not need to be
addressed by this series (though of course I'd welcome any fixes).
But there's a test failure that probably does need to be dealt with
before this graduates to 'next'.
-Peff
^ permalink raw reply
* Re: [PATCH] t/perf: export variable used in other blocks
From: Jeff King @ 2017-03-03 6:45 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170302195041.1699-1-jonathantanmy@google.com>
On Thu, Mar 02, 2017 at 11:50:41AM -0800, Jonathan Tan wrote:
> In p0001, a variable was created in a test_expect_success block to be
> used in later test_perf blocks, but was not exported. This caused the
> variable to not appear in those blocks (this can be verified by writing
> 'test -n "$commit"' in those blocks), resulting in a slightly different
> invocation than what was intended. Export that variable.
Thanks, this is obviously the right thing to do, and the mistake is mine
from ea97002fc (t/perf: time rev-list with UNINTERESTING commits,
2014-01-20). This is not the first time I've been confused by missing
variables in t/perf scripts, since it behaves differently than the
normal test suite. I wonder if we should turn on "set -a" during t/perf
setup snippets. That's a bit of a blunt tool, but I suspect it would
just be easier to work with.
I was curious that the tests added by ea97002fc showed something useful
even with the bug you're fixing here. But it's because the actual slice
of history we meant to traverse isn't important. It's intentionally
tiny to show off the time spent dealing with the UNINTERESTING commits.
So in effect we were traversing no commits instead of a tiny set, but
the timing results were the same.
I repeated the tests over fbd4a703 given in the commit message of
ee9a7002fc and confirmed that it behaves the same with your fixed
version of the test. I did have to tweak a few other things to get the
test to run against such an old version of git, though. I'll follow-up
with a patch.
-Peff
^ permalink raw reply
* Re: [PATCH v7 3/3] config: add conditional include
From: Jeff King @ 2017-03-03 6:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nguyễn Thái Ngọc Duy, git, sschuberth,
Matthieu Moy, Philip Oakley, Ramsay Jones
In-Reply-To: <xmqqo9xkhosj.fsf@gitster.mtv.corp.google.com>
On Wed, Mar 01, 2017 at 09:47:40AM -0800, Junio C Hamano wrote:
> > @@ -185,6 +271,12 @@ int git_config_include(const char *var, const char *value, void *data)
> >
> > if (!strcmp(var, "include.path"))
> > ret = handle_path_include(value, inc);
> > +
> > + if (!parse_config_key(var, "includeif", &cond, &cond_len, &key) &&
> > + (cond && include_condition_is_true(cond, cond_len)) &&
> > + !strcmp(key, "path"))
> > + ret = handle_path_include(value, inc);
> > +
> > return ret;
> > }
>
> So "includeif.path" (misspelled one without any condition) falls
> through to "return ret" and gives the value we got from inc->fn().
> I am OK with that (i.e. "missing condition is false").
Yeah, I think that is sensible, just as not-understood nonsense like
"include.foobar" would be ignored, as well.
> Or we can make it go to handle_path_include(), effectively making
> the "include.path" a short-hand for "includeIf.path". I am also OK
> with that (i.e. "missing condition is true").
I think we want "missing condition is false". Certainly an empty
condition like "includeIf..path" is false, as are conditions we don't
understand.
> Or we could even have "include.[<condition>.]path" without
> "includeIf"? I am not sure if it is a bad idea that paints
> ourselves in a corner, but somehow I find it tempting.
That was how I had originally envisioned the namespace working when I
introduced include.path long ago. And I think Duy's v1 used that, but
the feedback was that it was not sufficiently obvious that the
subsection was a conditional.
-Peff
^ permalink raw reply
* [PATCH] t/perf: export variable used in other blocks
From: Jonathan Tan @ 2017-03-02 19:50 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, peff, gitster
In-Reply-To: <20170228221236.selqkf5wme3fvued@sigill.intra.peff.net>
In p0001, a variable was created in a test_expect_success block to be
used in later test_perf blocks, but was not exported. This caused the
variable to not appear in those blocks (this can be verified by writing
'test -n "$commit"' in those blocks), resulting in a slightly different
invocation than what was intended. Export that variable.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Performance indeed does drop significantly with this patch. And I cannot
say "at least this is more correct", because it isn't - as Peff thinks,
it is indeed still not 100% accurate because of the resurfacing-object
issue he mentions (I've verified this by constructing a test that fails
even after this patch set).
Also, since we don't want to unify tree_ and blob_ (for the reason Peff
mentioned in another e-mail), feel free to drop this patch set.
Having said that, while looking at the perf test, I noticed an issue
with a non-exported variable, which is corrected in this patch.
t/perf/p0001-rev-list.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/perf/p0001-rev-list.sh b/t/perf/p0001-rev-list.sh
index 16359d51a..ebf172401 100755
--- a/t/perf/p0001-rev-list.sh
+++ b/t/perf/p0001-rev-list.sh
@@ -15,7 +15,8 @@ test_perf 'rev-list --all --objects' '
'
test_expect_success 'create new unreferenced commit' '
- commit=$(git commit-tree HEAD^{tree} -p HEAD)
+ commit=$(git commit-tree HEAD^{tree} -p HEAD) &&
+ test_export commit
'
test_perf 'rev-list $commit --not --all' '
--
2.12.0.rc1.440.g5b76565f74-goog
^ permalink raw reply related
* Re: [PATCH v2 8/9] read_early_config(): really discover .git/
From: Jeff King @ 2017-03-03 5:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano, Duy Nguyen
In-Reply-To: <921faef822901715fa877d6969255ce00d80b925.1488506615.git.johannes.schindelin@gmx.de>
On Fri, Mar 03, 2017 at 03:04:32AM +0100, Johannes Schindelin wrote:
> Earlier, we punted and simply assumed that we are in the top-level
> directory of the project, and that there is no .git file but a .git/
> directory so that we can read directly from .git/config.
>
> However, that is not necessarily true. We may be in a subdirectory. Or
> .git may be a gitfile. Or the environment variable GIT_DIR may be set.
>
> To remedy this situation, we just refactored the way
> setup_git_directory() discovers the .git/ directory, to make it
> reusable, and more importantly, to leave all global variables and the
> current working directory alone.
>
> Let's discover the .git/ directory correctly in read_early_config() by
> using that new function.
>
> This fixes 4 known breakages in t7006.
Yay, this is much nicer.
I think the "dirty hack..." comment in read_early_config() can be
condensed (or go away) now.
I think we _could_ do away with read_early_config() entirely, and just
have the regular config code do this lookup when we're not already in a
repo. But then we'd really need to depend on the "creating_repository"
flag being managed correctly.
I think I prefer the idea that a few "early" spots like pager and alias
config need to use this special function to access the config. That's
less likely to cause surprises when some config option is picked up
before we have run setup_git_directory().
There is one surprising case that I think we need to deal with even now,
though. If I do:
git init repo
git -C repo config pager.upload-pack 'echo whoops'
git upload-pack repo
cd repo && git upload-pack .
the first one is OK, but the second reads and executes the pager config
from the repo, even though we usually consider upload-pack to be OK to
run in an untrusted repo. This _isn't_ a new thing in your patch, but
just something I noticed while we are here.
And maybe it is a non-issue. The early-config bits all happen via the
git wrapper, and normally we use the direct dashed "git-upload-pack" to
access the other repositories. Certainly I have been known to use "git
-c ... upload-pack" while debugging various things.
So I dunno. You really have to jump through some hoops for it to matter,
but I just wonder if the git wrapper should be special-casing
upload-pack, too.
-Peff
^ permalink raw reply
* [PATCH] t/perf: use $MODERN_GIT for all repo-copying steps
From: Jeff King @ 2017-03-03 7:14 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, gitster
In-Reply-To: <20170303064512.khs2seru5onl54mh@sigill.intra.peff.net>
On Fri, Mar 03, 2017 at 01:45:12AM -0500, Jeff King wrote:
> I repeated the tests over fbd4a703 given in the commit message of
> ee9a7002fc and confirmed that it behaves the same with your fixed
> version of the test. I did have to tweak a few other things to get the
> test to run against such an old version of git, though. I'll follow-up
> with a patch.
Here's that patch.
-- >8 --
Subject: [PATCH] t/perf: use $MODERN_GIT for all repo-copying steps
Since 1a0962dee (t/perf: fix regression in testing older
versions of git, 2016-06-22), we point "$MODERN_GIT" to a
copy of git that matches the t/perf script itself, and which
can be used for tasks outside of the actual timings. This is
needed because the setup done by perf scripts keeps moving
forward in time, and may use features that the older
versions of git we are testing do not have.
That commit used $MODERN_GIT to fix a case where we relied
on the relatively recent --git-path option. But if you go
back further still, there are more problems.
Since 7501b5921 (perf: make the tests work in worktrees,
2016-05-13), we use "git -C", but versions of git older than
44e1e4d67 (git: run in a directory given with -C option,
2013-09-09) don't know about "-C". So testing an old version
of git with a new version of t/perf will fail the setup
step.
We can fix this by using $MODERN_GIT during the setup;
there's no need to use the antique version, since it doesn't
affect the timings. Likewise, we'll adjust the "init"
invocation; antique versions of git called this "init-db".
Signed-off-by: Jeff King <peff@peff.net>
---
With this patch I was able to run p0001 against v1.7.0. I don't think we
can go further back than that because the perf library depends on the
presence of bin-wrappers. That's probably enough. Unlike the t/interop
library I proposed recently it's not that interesting to go really far
back in time (and I did hack around the bin-wrappers thing in t/interop;
you really can test against v1.0.0 there).
t/perf/perf-lib.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 46f08ee08..ab4b8b06a 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -83,7 +83,7 @@ test_perf_create_repo_from () {
error "bug in the test script: not 2 parameters to test-create-repo"
repo="$1"
source="$2"
- source_git="$(git -C "$source" rev-parse --git-dir)"
+ source_git="$("$MODERN_GIT" -C "$source" rev-parse --git-dir)"
objects_dir="$("$MODERN_GIT" -C "$source" rev-parse --git-path objects)"
mkdir -p "$repo/.git"
(
@@ -102,7 +102,7 @@ test_perf_create_repo_from () {
) &&
(
cd "$repo" &&
- git init -q && {
+ "$MODERN_GIT" init -q && {
test_have_prereq SYMLINKS ||
git config core.symlinks false
} &&
--
2.12.0.385.gdf4947bc7
^ permalink raw reply related
* Re: git push - 401 unauthorized
From: Jeff King @ 2017-03-03 8:07 UTC (permalink / raw)
To: Alessio Rocchi; +Cc: git
In-Reply-To: <017901d28183$6fb72c60$4f258520$@gmail.com>
On Tue, Feb 07, 2017 at 09:47:45PM +0100, Alessio Rocchi wrote:
> I try to push my commit on a private repository (which has been working
> since about five years).
It wasn't clear to me from your email, but did this work with a
previous version of Git and is now broken?
> me@superstar:/var/www/scorte$ git push --verbose
> Pushing to http://isisenscorte:mypassword@mymachine/scorte_git
> Getting pack list
> Fetching remote heads...
> refs/
> refs/tags/
> refs/heads/
> updating 'refs/heads/master'
> from d9fd2e49cb0c32a6d8fddcff2954f04b4104d176
> to 23d8edfb7fa70bce44c21a7f93064c08a7288e23
> sending 6 objects
> MOVE 33fcba80fdec82f43f995e5c693255da075358be failed, aborting (52/0)
> MOVE 60e1a097d50fe62319413ed20129580cf175d1ca failed, aborting (52/0)
> MOVE cfea41ef02f9aef5cecfbf7eac5a9e55975113f4 failed, aborting (52/0)
> MOVE 3d87ab6ff7652f2b30e48612b70c8335d4625699 failed, aborting (52/0)
> MOVE 4adb1b39e0446e0bfc3182258ff1cd7077871f7f failed, aborting (52/0)
> Updating remote server info
> fatal: git-http-push failed
OK, that looks like the old dumb-http protocol. And you said here:
> Permissions on the unauthorized object folders are 777 everywhere. My git
> version is 1.7.0.4 on both client and server. Do you have any clue of this
> strange behaviour?
...that you're using v1.7.0.4. There were tons of auth-related corner
cases that have been fixed since then. Can you try a more recent version
of Git on the client side (preferably v2.12.0)?
The version on the server shouldn't matter; the 401 is generated by
Apache, and as you are using the dumb-http protocol, git is not involved
on the server side of the request at all.
I'd also suggest that you move to the smart-http protocol on the server
if possible. It's much more efficient, and the dumb-http code paths are
not nearly as well tested, especially around things like authentication.
Running "git help http-backend" gives some sample Apache config.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox