* Re: [PATCH 4/5] Make sequencer abort safer
From: Johannes Schindelin @ 2016-12-08 15:28 UTC (permalink / raw)
To: Stephan Beyer; +Cc: git, Christian Couder, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20161207215133.13433-4-s-beyer@gmx.net>
Hi,
On Wed, 7 Dec 2016, Stephan Beyer wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 30b10ba14..c9b560ac1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
> static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
> static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
> static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
Is it required by law to have a four-letter infix, or can we have a nicer
variable name (e.g. git_path_current_file)?
Ciao,
Dscho
^ permalink raw reply
* [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
From: Johannes Schindelin @ 2016-12-08 15:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
Hopefully these patches will lead to something that we can integrate,
and that eventually will make Git's startup sequence much less
surprising.
The idea here is to discover the .git/ directory gently (i.e. without
changing the current working directory), and to use it to read the
.git/config file early, before we actually called setup_git_directory()
(if we ever do that).
Notes:
- I find the diff pretty ugly: I wish there was a more elegant way to
*disable* discovery of .git/ *just* for `init` and `clone`. I
considered a function `about_to_create_git_dir()` that is called in a
hard-coded manner *only* for `init` and `clone`, but that would
introduce another magic side effect, when all I want is to reduce those.
- For the moment, I do not handle dashed invocations of `init` and
`clone` correctly. The real problem is the continued existence of
the dashed git-init and git-clone, of course.
- There is still duplicated code. For the sake of this RFC, I did not
address that yet.
- The read_early_config() function is called multiple times, re-reading
all the config files and re-discovering the .git/ directory multiple
times, which is quite wasteful. For the sake of this RFC, I did not
address that yet.
- t7006 fails and the error message is a bit cryptic (not to mention the
involved function trace, which is mind-boggling for what is supposed
to be a simply, shell script-based test suite). I did not have time to
look into that yet.
- after discover_git_directory_gently() did its work, the code happily
uses its result *only* for the current read_early_config() run, and
lets setup_git_dir_gently() do the whole work *again*. For the sake of
this RFC, I did not address that yet.
Johannes Schindelin (7):
Make read_early_config() reusable
read_early_config(): avoid .git/config hack when unneeded
Mark builtins that create .git/ directories
read_early_config(): special-case `init` and `clone`
read_early_config(): really discover .git/
WIP read_config_early(): respect ceiling directories
WIP: read_early_config(): add tests
builtin/am.c | 2 +-
builtin/blame.c | 2 +-
builtin/grep.c | 4 +-
builtin/log.c | 4 +-
builtin/var.c | 2 +-
cache.h | 8 ++--
config.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++
diff.c | 4 +-
git.c | 25 +++++------
pager.c | 44 +++----------------
t/helper/test-config.c | 15 +++++++
t/t1309-early-config.sh | 50 ++++++++++++++++++++++
12 files changed, 209 insertions(+), 61 deletions(-)
create mode 100755 t/t1309-early-config.sh
base-commit: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2
Published-As: https://github.com/dscho/git/releases/tag/early-config-v1
Fetch-It-Via: git fetch https://github.com/dscho/git early-config-v1
--
2.11.0.rc3.windows.1
^ permalink raw reply
* [PATCH/RFC 3/7] Mark builtins that create .git/ directories
From: Johannes Schindelin @ 2016-12-08 15:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1481211338.git.johannes.schindelin@gmx.de>
To refactor read_early_config() so that it discovers .git/config
properly, we have to make certain that commands such as `git init` (i.e.
commands that create their own .git/ and therefore do *not* want to
be affected by any other .git/ directory) skip this discovery.
Let's introduce a flag that states for every builtin whether it creates
its own .git/ directory or not.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
git.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/git.c b/git.c
index dce529fcbf..61df78afc8 100644
--- a/git.c
+++ b/git.c
@@ -318,12 +318,13 @@ 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<<3)
-#define SUPPORT_SUPER_PREFIX (1<<4)
+#define NEED_WORK_TREE (1<<4)
+#define SUPPORT_SUPER_PREFIX (1<<5)
struct cmd_struct {
const char *cmd;
@@ -412,7 +413,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 },
+ { "clone", cmd_clone, CREATES_GIT_DIR },
{ "column", cmd_column, RUN_SETUP_GENTLY },
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
@@ -438,7 +439,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 },
+ { "init", cmd_init_db, CREATES_GIT_DIR },
{ "init-db", cmd_init_db },
{ "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
{ "log", cmd_log, RUN_SETUP },
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH/RFC 1/7] Make read_early_config() reusable
From: Johannes Schindelin @ 2016-12-08 15:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1481211338.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 a50a61a197..96e0cb2523 100644
--- a/cache.h
+++ b/cache.h
@@ -1692,6 +1692,7 @@ extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,
const char *name, const char *buf, size_t len, 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 83fdecb1bc..7dcd8d8622 100644
--- a/config.c
+++ b/config.c
@@ -1385,6 +1385,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 ae79643363..73ca8bc3b1 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.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH/RFC 2/7] read_early_config(): avoid .git/config hack when unneeded
From: Johannes Schindelin @ 2016-12-08 15:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1481211338.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.
Let's just use the same function, have_git_dir(), to determine whether we
have to look for .git/config specifically.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 7dcd8d8622..104c3c3dcd 100644
--- a/config.c
+++ b/config.c
@@ -1407,7 +1407,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->have_repository) {
+ if (!have_git_dir()) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH/RFC 4/7] read_early_config(): special-case `init` and `clone`
From: Johannes Schindelin @ 2016-12-08 15:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1481211338.git.johannes.schindelin@gmx.de>
The `init` and `clone` commands create their own .git/ directory,
therefore we must be careful not to read any repository config when
determining the pager settings.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/am.c | 2 +-
builtin/blame.c | 2 +-
builtin/grep.c | 4 ++--
builtin/log.c | 4 ++--
builtin/var.c | 2 +-
cache.h | 9 +++++----
config.c | 4 ++--
diff.c | 4 ++--
git.c | 16 ++++++++--------
| 13 +++++++------
10 files changed, 31 insertions(+), 29 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce9..e6c2ee01bc 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);
+ const char *pager = git_pager(1, 1);
struct child_process cp = CHILD_PROCESS_INIT;
if (!pager)
diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..5b7daa3686 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2913,7 +2913,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
assign_blame(&sb, opt);
if (!incremental)
- setup_pager();
+ setup_pager(1);
free(final_commit_name);
diff --git a/builtin/grep.c b/builtin/grep.c
index 8887b6addb..363a753369 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -800,7 +800,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
}
if (show_in_pager == default_pager)
- show_in_pager = git_pager(1);
+ show_in_pager = git_pager(1, 1);
if (show_in_pager) {
opt.color = 0;
opt.name_only = 1;
@@ -896,7 +896,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
}
if (!show_in_pager && !opt.status_only)
- setup_pager();
+ setup_pager(1);
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 55d20cc2d8..96618d38cb 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();
+ setup_pager(1);
}
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();
+ setup_pager(1);
if (output_directory) {
if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53a2d..879867b842 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);
+ const char *pgm = git_pager(1, 1);
if (!pgm)
pgm = "cat";
diff --git a/cache.h b/cache.h
index 96e0cb2523..6627239de8 100644
--- a/cache.h
+++ b/cache.h
@@ -1325,7 +1325,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);
+extern const char *git_pager(int stdout_is_tty, int discover_git_dir);
extern int git_ident_config(const char *, const char *, void *);
extern void reset_ident_date(void);
@@ -1692,7 +1692,8 @@ extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,
const char *name, const char *buf, size_t len, void *data);
extern void git_config_push_parameter(const char *text);
extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern void read_early_config(config_fn_t cb, void *data);
+extern void read_early_config(config_fn_t cb, void *data,
+ int discover_git_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,
@@ -1888,12 +1889,12 @@ __attribute__((format (printf, 2, 3)))
extern void write_file(const char *path, const char *fmt, ...);
/* pager.c */
-extern void setup_pager(void);
+extern void setup_pager(int discover_git_dir);
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);
+extern int check_pager_config(const char *cmd, int discover_git_dir);
extern void prepare_pager_args(struct child_process *, const char *pager);
extern const char *editor_program;
diff --git a/config.c b/config.c
index 104c3c3dcd..4c756edca1 100644
--- a/config.c
+++ b/config.c
@@ -1385,7 +1385,7 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
}
}
-void read_early_config(config_fn_t cb, void *data)
+void read_early_config(config_fn_t cb, void *data, int discover_git_dir)
{
git_config_with_options(cb, data, NULL, 1);
@@ -1407,7 +1407,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 (!have_git_dir()) {
+ if (discover_git_dir && !have_git_dir()) {
struct git_config_source repo_config;
memset(&repo_config, 0, sizeof(repo_config));
diff --git a/diff.c b/diff.c
index ec8728362d..0769c0590a 100644
--- a/diff.c
+++ b/diff.c
@@ -5267,6 +5267,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") != 0)
- setup_pager();
+ check_pager_config("diff", 1) != 0)
+ setup_pager(1);
}
diff --git a/git.c b/git.c
index 61df78afc8..2b007b83ec 100644
--- a/git.c
+++ b/git.c
@@ -61,13 +61,13 @@ static void restore_env(int external_alias)
}
}
-static void commit_pager_choice(void) {
+static void commit_pager_choice(int discover_git_dir) {
switch (use_pager) {
case 0:
setenv("GIT_PAGER", "cat", 1);
break;
case 1:
- setup_pager();
+ setup_pager(discover_git_dir);
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();
+ commit_pager_choice(1);
restore_env(1);
child.use_shell = 1;
@@ -349,7 +349,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);
+ use_pager = check_pager_config(p->cmd, !(p->option & CREATES_GIT_DIR));
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;
@@ -357,7 +357,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();
+ commit_pager_choice(!(p->option & CREATES_GIT_DIR));
if (!help && get_super_prefix()) {
if (!(p->option & SUPPORT_SUPER_PREFIX))
@@ -584,8 +584,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]);
- commit_pager_choice();
+ use_pager = check_pager_config(argv[0], 1);
+ commit_pager_choice(1);
strbuf_addf(&cmd, "git-%s", argv[0]);
@@ -688,7 +688,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();
+ commit_pager_choice(1);
printf("usage: %s\n\n", git_usage_string);
list_common_cmds_help();
printf("\n%s\n", _(git_more_info_string));
--git a/pager.c b/pager.c
index 73ca8bc3b1..16b3cbe232 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)
+const char *git_pager(int stdout_is_tty, int discover_git_dir)
{
const char *pager;
@@ -53,7 +53,8 @@ const char *git_pager(int stdout_is_tty)
pager = getenv("GIT_PAGER");
if (!pager) {
if (!pager_program)
- read_early_config(core_pager_config, NULL);
+ read_early_config(core_pager_config, NULL,
+ discover_git_dir);
pager = pager_program;
}
if (!pager)
@@ -100,9 +101,9 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
setup_pager_env(&pager_process->env_array);
}
-void setup_pager(void)
+void setup_pager(int discover_git_dir)
{
- const char *pager = git_pager(isatty(1));
+ const char *pager = git_pager(isatty(1), discover_git_dir);
if (!pager)
return;
@@ -208,7 +209,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 check_pager_config(const char *cmd, int discover_git_dir)
{
struct pager_command_config_data data;
@@ -216,7 +217,7 @@ int check_pager_config(const char *cmd)
data.want = -1;
data.value = NULL;
- read_early_config(pager_command_config, &data);
+ read_early_config(pager_command_config, &data, discover_git_dir);
if (data.value)
pager_program = data.value;
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH/RFC 5/7] read_early_config(): really discover .git/
From: Johannes Schindelin @ 2016-12-08 15:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1481211338.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.
Let's discover the .git/ directory correctly.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 69 +++++++++++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 49 insertions(+), 20 deletions(-)
diff --git a/config.c b/config.c
index 4c756edca1..286d3cad66 100644
--- a/config.c
+++ b/config.c
@@ -1385,35 +1385,64 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
}
}
+/*
+ * 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 *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);
+ 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 > 0; )
+ if (is_dir_sep(result->buf[--i]))
+ break;
+ if (!i)
+ return -1;
+ strbuf_setlen(result, i);
+ }
+}
+
void read_early_config(config_fn_t cb, void *data, int discover_git_dir)
{
+ struct strbuf buf = STRBUF_INIT;
+
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 (discover_git_dir && !have_git_dir()) {
+ if (discover_git_dir && !have_git_dir() &&
+ !discover_git_directory_gently(&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);
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* Re: Serious bug with Git-2.11.0-64-bit and Git-LFS
From: Nick Warr @ 2016-12-08 15:36 UTC (permalink / raw)
To: Lars Schneider; +Cc: git
In-Reply-To: <D1437EA2-F4D3-4190-8D79-020C06CFA3DB@gmail.com>
On 8 December 2016 at 14:18, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>> On 08 Dec 2016, at 15:00, Nick Warr <nick.warr@bossastudios.com> wrote:
>>
>> That looks pretty much like the error we're dealing with, any reason
>> why going back a point version on Git (not git-lfs) would resolve the
>> issue however?
>
> Going back to GitLFS 1.4.* would make the error disappear. However, I think
> you should fix your repository. Try this:
>
> git lfs clone <YOUR REPO URL>
> cd <YOUR REPO>
> git rm --cached "workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi Effects/Effects/Debris/Meshes/debris_junk.FBX"
> git add --force "workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi Effects/Effects/Debris/Meshes/debris_junk.FBX"
> git commit -m "Move files properly to GitLFS"
> git push
>
> Afterwards you should be able to use the latest version of Git and GitLFS
> without trouble.
>
> Cheers,
> Lars
>
> PS: Top posting is not that popular in the Git community ;-)
>
>
>>
>> On 8 December 2016 at 13:57, Lars Schneider <larsxschneider@gmail.com> wrote:
>>>
>>>> On 08 Dec 2016, at 12:46, Nick Warr <nick.warr@bossastudios.com> wrote:
>>>>
>>>> Using Git-2.11.0 with the latest git-lfs 1.5.2 (also tested with
>>>> 1.5.3) cloning from our locally hosted gitlab CE server via HTTPS.
>>>>
>>>> When cloning a repo (large, 3.3 gig in git, 10.3 in LFS) for the
>>>> first time the clone will finish the checkout of the git part, then
>>>> when it starts downloading the LFS parts it will reliably finish with
>>>> a smudge filter error.
>>>>
>>>> This leaves the repo in an unstable condition, you can then fetch the
>>>> lfs part without issue, but checking out the lfs files or trying a git
>>>> reset --hard will continue to spit out the same error. As you can see,
>>>> the actual error is not particularly useful.
>>>>
>>>> fatal: workers/unity/Assets/3rdPartyAssets/FORGE3D/Sci-Fi
>>>> Effects/Effects/Debris/Meshes/debris_junk.FBX: smudge filter lfs
>>>> failed
>>>> Unknown command ""
>>>>
>>>> Possibly it's due to the file extension being all capital letters, we
>>>> did manage to change the error by recommitting the file with a
>>>> lowercase extension, but it failed on the next file (which also had a
>>>> capital letter extension).
>>>>
>>>> This has happened on multiple fresh windows 10 64 bit installs,
>>>> different machines and target directories (to hopefully remove the
>>>> possibility of file permissions) where cloning is taking place.
>>>>
>>>> The solution is to back level to Git 2.10.2 and the error disappears.
>>>>
>>>> More than willing to provide any further information,
>>>
>>> Hi Nick,
>>>
>>> debris_junk.FBX is not stored properly in Git LFS.
>>> I explained the problem in detail here:
>>> https://github.com/git-lfs/git-lfs/issues/1729
>>>
>>> You should add the file properly to GitLFS to fix the problem.
>>> However, I think this is a regression in GitLFS and I hope it will
>>> be fixed in the next version.
>>>
>>> No change/fix in Git is required.
>>>
>>> Cheers,
>>> Lars
>
Thank gmail for the top posting, hard enough getting it to send non html mail :(
Just to add a bit more to the discussion, we went and had a look at
that directory which was freshly cloned with 2.10.2 and 1.5.3 and the
files were there (400 KB + FBX files, not 2KB pointers) after the git
reset --hard, we're actually looking at renaming all those files, as
that isn't the only one unfortunately..
^ permalink raw reply
* [PATCH/RFC 7/7] WIP: read_early_config(): add tests
From: Johannes Schindelin @ 2016-12-08 15:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1481211338.git.johannes.schindelin@gmx.de>
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 83a4f2ab86..5105069587 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], 1);
+ 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 0000000000..0c55dee514
--- /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.11.0.rc3.windows.1
^ permalink raw reply related
* [PATCH/RFC 6/7] WIP read_config_early(): respect ceiling directories
From: Johannes Schindelin @ 2016-12-08 15:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <cover.1481211338.git.johannes.schindelin@gmx.de>
This ports the part from setup_git_directory_gently_1() where the
GIT_CEILING_DIRECTORIES environment variable is handled.
TODO: DRY up code again (exporting canonicalize_ceiling_directories()?)
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/config.c b/config.c
index 286d3cad66..eda3546491 100644
--- a/config.c
+++ b/config.c
@@ -1386,6 +1386,37 @@ 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
@@ -1394,6 +1425,8 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
*/
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)
@@ -1403,6 +1436,23 @@ static int discover_git_directory_gently(struct strbuf *result)
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;
@@ -1418,10 +1468,10 @@ static int discover_git_directory_gently(struct strbuf *result)
strbuf_setlen(result, len);
if (is_git_directory(result->buf))
return 0;
- for (i = len; i > 0; )
- if (is_dir_sep(result->buf[--i]))
+ for (i = len; --i > ceiling_offset; )
+ if (is_dir_sep(result->buf[i]))
break;
- if (!i)
+ if (i <= ceiling_offset)
return -1;
strbuf_setlen(result, i);
}
--
2.11.0.rc3.windows.1
^ permalink raw reply related
* Re: [PATCH v2] commit: make --only --allow-empty work without paths
From: Junio C Hamano @ 2016-12-08 16:47 UTC (permalink / raw)
To: Andreas Krey; +Cc: git, Jeff King
In-Reply-To: <20161208135029.GA16292@inner.h.apk.li>
Andreas Krey <a.krey@gmx.de> writes:
> Ok, I've removed the clever message, as Junio suggested.
> I don't know what else to do to make it acceptable. :-)
> We're going to deploy it internally anyway, but I think
> it belongs in git.git as well (aka 'Can I has "will queue"?').
Oh, sorry for being unclear. Before I started saying "Slightly
related topic.", after quoting "The patch itself looks good to me."
by Peff, I meant to say "Yeah, this looks good; thanks.", but
apparently I forgot.
Removal of "Clever" is a separate issue and it may make sense to do
so, but it deserves its own commit with its own justification.
Sorry for making you send an extra round; let's queue the original,
and if you still are interested, have the "Clever" removal as its
own patch.
Thanks.
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Junio C Hamano @ 2016-12-08 16:54 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Brandon Williams, git, sbeller, peff, jacob.keller
In-Reply-To: <1767f01a-4125-d99b-37db-3f4a56aaa28a@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 07.12.2016 um 23:29 schrieb Brandon Williams:
>> Instead of assuming root is "/"
>> I'll need to extract what root is from an absolute path. Aside from
>> what root looks like, do most other path constructs behave similarly in
>> unix and windows? (like ".." and "." as examples)
>
> Yes, .. and . work the same way, except that they cannot appear in the
> \\server\share part. I also think that .. does not cancel these parts.
>
> As long as you use is_absolute_path() and do not simplify path
> components before offset_1st_component(), you should be on the safe
> side.
>
>> Since I don't really have a windows machine to test things it might be
>> slightly difficult to get everything correct quickly but hopefully we can
>> get this working :)
>
> I'll lend a hand, of course, as time permits.
Thanks, as always, for working well together ;-).
^ permalink raw reply
* Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config
From: Jeff King @ 2016-12-08 17:26 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1481211338.git.johannes.schindelin@gmx.de>
On Thu, Dec 08, 2016 at 04:35:56PM +0100, Johannes Schindelin wrote:
> The idea here is to discover the .git/ directory gently (i.e. without
> changing the current working directory), and to use it to read the
> .git/config file early, before we actually called setup_git_directory()
> (if we ever do that).
Great. Thanks for taking a stab at this.
> Notes:
>
> - I find the diff pretty ugly: I wish there was a more elegant way to
> *disable* discovery of .git/ *just* for `init` and `clone`. I
> considered a function `about_to_create_git_dir()` that is called in a
> hard-coded manner *only* for `init` and `clone`, but that would
> introduce another magic side effect, when all I want is to reduce those.
It looks like a lot of that ugliness comes from passing around the "are
we OK to peek at git-dir config" flag through the various pager-related
calls. I don't think it would be bad to use a global for "we do not
want a repo". After all, it's just modifying the _existing_ global for
"are we in a repo". And I do not see that global going away anytime
soon. Sometimes it's good to make incremental steps towards an end goal,
but in this case it seems like we just pay the cost of the step without
any real plan for reaping the benefit in the long term.
As an alternative, I also think it would be OK to just always have the
pager config read from local repo, even for init/clone. In other words,
to loosen the idea that git-init can _never_ look in the current
git-dir, and declare that there is a stage before the command is
initiated, and during which git may read local-repo config. Aliases
would fall into this, too, so:
git config --local alias.foo init
git foo /some/other/dir
would work (as it must, because we cannot know that "foo" is "init"
until we read the config!).
I have a feeling you may declare that too magical, but I think it's
consistent and practical.
> - For the moment, I do not handle dashed invocations of `init` and
> `clone` correctly. The real problem is the continued existence of
> the dashed git-init and git-clone, of course.
I assume you mean setting the CREATES_GIT_DIR flag here? I think it
would apply to the dashed invocations, too. They strip off the "git-"
and end up in handle_builtin() just like "git clone" does. I didn't test
it, though.
> - There is still duplicated code. For the sake of this RFC, I did not
> address that yet.
Yeah, I did not read your discover function very carefully. Because I
think the one thing we really don't want to do here is introduce a
separate lookup process that is not shared by setup_git_directory(). The
only sane path, I think, is to refactor setup_git_directory() to build
on top of discover_git_directory(), which implies that the latter
handles all of the cases.
> - The read_early_config() function is called multiple times, re-reading
> all the config files and re-discovering the .git/ directory multiple
> times, which is quite wasteful. For the sake of this RFC, I did not
> address that yet.
We already have a config-caching system. If we went with a global
"config_discover_refs", then I think the sequence for something like
git-init would become:
1. When git.c starts, config_discover_refs is set to "true". Pager and
alias lookup may look in .git/config if it's available, even if
they go through the configset cache.
2. As soon as git-init starts, it disables config_discover_refs, and
it flushes the config cache. Any configset lookups will now examine
the reduced config.
3. When git-init has set up the real repo it is operating on, it can
reenable config_discover_refs (though it may not even need to; that
flag probably wouldn't have any effect once we've entered the
repository and have_git_dir() returns true).
> - t7006 fails and the error message is a bit cryptic (not to mention the
> involved function trace, which is mind-boggling for what is supposed
> to be a simply, shell script-based test suite). I did not have time to
> look into that yet.
Running t7006 I see a lot of old failures turned into successes, which
is good (because running from a subdirectory now actually respects local
pager config). The one failure looks like it is testing the wrong thing.
It is checking that we _don't_ respect a local core.pager in some cases,
as a proxy for whether or not we are breaking things by doing setup too
early. But the whole point of your series is to fix that, and respect
the config without causing the setup breakage. After your patches, the
proxy behavior and the failure case are disconnected. The test should be
flipped, and ideally another one added that confirms we didn't actually
run setup_git_directory(), but I'm not sure how to test that directly.
> - after discover_git_directory_gently() did its work, the code happily
> uses its result *only* for the current read_early_config() run, and
> lets setup_git_dir_gently() do the whole work *again*. For the sake of
> this RFC, I did not address that yet.
If caching happens at the config layer, then we'd probably only call
this once anyway (or if we did call it again after a config flush, it
would be a good sign that we should compute its value again).
-Peff
^ permalink raw reply
* Re: [PATCH 4/5] Make sequencer abort safer
From: Junio C Hamano @ 2016-12-08 17:27 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Stephan Beyer, git, Christian Couder, SZEDER Gábor
In-Reply-To: <alpine.DEB.2.20.1612081627290.23160@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Wed, 7 Dec 2016, Stephan Beyer wrote:
>
>> diff --git a/sequencer.c b/sequencer.c
>> index 30b10ba14..c9b560ac1 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
>> static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
>> static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
>> static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
>
> Is it required by law to have a four-letter infix, or can we have a nicer
> variable name (e.g. git_path_current_file)?
I agree with you that, as other git_path_*_file variables match the
actual name on the filesystem, this one should too, together with
the update_curr_file() function.
By the way, this step seems to be a fix to an existing problem, and
the new test added in 3/5 seems to be a demonstration of the issue.
If that is the case, shouldn't the new test initially expect failure
and updated by this step to expect success?
I'll queue this on top of step 4/5 as "SQUASH???" as usual. The
other SQUASH??? that must come after 3/5 for t3510 should be trivial
(the reverse of what appears here).
Thanks.
sequencer.c | 22 +++++++++++-----------
t/t3510-cherry-pick-sequence.sh | 2 +-
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index c9b560ac15..ce04377f8e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
-static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
+static GIT_PATH_FUNC(git_path_current_file, "sequencer/current")
/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -311,7 +311,7 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
}
-static void update_curr_file()
+static void update_current_file(void)
{
struct object_id head;
@@ -320,9 +320,9 @@ static void update_curr_file()
return;
if (!get_oid("HEAD", &head))
- write_file(git_path_curr_file(), "%s", oid_to_hex(&head));
+ write_file(git_path_current_file(), "%s", oid_to_hex(&head));
else
- write_file(git_path_curr_file(), "%s", "");
+ write_file(git_path_current_file(), "%s", "");
}
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -354,7 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
strbuf_release(&sb);
strbuf_release(&err);
ref_transaction_free(transaction);
- update_curr_file();
+ update_current_file();
return 0;
}
@@ -829,7 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
leave:
free_message(commit, &msg);
- update_curr_file();
+ update_current_file();
return res;
}
@@ -1149,23 +1149,23 @@ static int save_head(const char *head)
return 0;
}
-static int rollback_is_safe()
+static int rollback_is_safe(void)
{
struct strbuf sb = STRBUF_INIT;
struct object_id expected_head, actual_head;
- if (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) {
+ if (strbuf_read_file(&sb, git_path_current_file(), 0) >= 0) {
strbuf_trim(&sb);
if (get_oid_hex(sb.buf, &expected_head)) {
strbuf_release(&sb);
- die(_("could not parse %s"), git_path_curr_file());
+ die(_("could not parse %s"), git_path_current_file());
}
strbuf_release(&sb);
}
else if (errno == ENOENT)
oidclr(&expected_head);
else
- die_errno(_("could not read '%s'"), git_path_curr_file());
+ die_errno(_("could not read '%s'"), git_path_current_file());
if (get_oid("HEAD", &actual_head))
oidclr(&actual_head);
@@ -1441,7 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
- update_curr_file();
+ update_current_file();
res = pick_commits(&todo_list, opts);
todo_list_release(&todo_list);
return res;
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index efcd4fc485..372307c21b 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
git diff-index --exit-code HEAD
'
-test_expect_failure '--abort does not unsafely change HEAD' '
+test_expect_success '--abort does not unsafely change HEAD' '
pristine_detach initial &&
test_must_fail git cherry-pick picked anotherpick &&
git reset --hard base &&
^ permalink raw reply related
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Jeff King @ 2016-12-08 17:46 UTC (permalink / raw)
To: Stefan Beller; +Cc: vi0oss, git@vger.kernel.org, Stefan Beller
In-Reply-To: <CAGZ79kZU401JRp4EtwTGHzk3Zq+snQhX3GArDfF6SpKxsSwtWg@mail.gmail.com>
On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:
> On Wed, Dec 7, 2016 at 4:39 PM, <vi0oss@gmail.com> wrote:
>
> >
> > Previously test contained errorneous
> > test_must_fail, which was masked by
> > missing &&.
>
> I wonder if we could make either
> the test_must_fail intelligent to detect such a broken && call chain
> or the test_expect_success macro to see for those broken chains.
I don't think test_must_fail is relevant for &&-chains. Even something
like:
test_must_fail foo
bar
or:
bar
test_must_fail foo
will both trigger on the &&-chain linter, because it uses a magic exit
code to detect the breakage. I think the problem is just that the
&&-chain linter cannot peek inside subshells, and that's where the bug
was in this case.
I wish we could improve that, but I spend a lot of brain cycles on it at
one point and couldn't come up with a workable solution.
-Peff
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Brandon Williams @ 2016-12-08 17:50 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Stefan Beller, git@vger.kernel.org, Jeff King, Jacob Keller
In-Reply-To: <CACsJy8BdUsGD-Bx=-qiP3cWt2AjwD1P3NTbqEnjnKa1v0TvySQ@mail.gmail.com>
On 12/08, Duy Nguyen wrote:
> On Tue, Dec 6, 2016 at 3:16 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/05, Stefan Beller wrote:
> >> > static const char *real_path_internal(const char *path, int die_on_error)
> >> > {
> >> > - static struct strbuf sb = STRBUF_INIT;
> >> > + static struct strbuf resolved = STRBUF_INIT;
> >>
> >> Also by having this static here, it is not quite thread safe, yet.
> >>
> >> By removing the static here we cannot do the early cheap check as:
> >>
> >> > /* We've already done it */
> >> > - if (path == sb.buf)
> >> > + if (path == resolved.buf)
> >> > return path;
> >>
> >> I wonder how often we run into this case; are there some callers explicitly
> >> relying on real_path_internal being cheap for repeated calls?
> >> (Maybe run the test suite with this early return instrumented? Not sure how
> >> to assess the impact of removing the cheap out return optimization)
> >>
> >> The long tail (i.e. the actual functionality) should actually be
> >> faster, I'd imagine
> >> as we do less than with using chdir.
> >
> > Depends on how expensive the chdir calls were. And I'm working to get
> > rid of the static buffer. Just need have the callers own the memory
> > first.
>
> I suggest you turn this real_path_internal into strbuf_real_path. In
> other words, it takes a strbuf and writes the result there, allocating
> memory if needed.
>
> This function can replace the two strbuf_addstr(..., real_path(..));
> we have in setup.c and sha1_file.c. real_path() can own a static
> strbuf buffer to retain current behavior. We could also have a new
> wrapper real_pathdup() around strbuf_real_path(), which can replace 9
> instances of xstrdup(real_path(...)) (and Stefan is adding a few more;
> that's what led me back to these mails)
Sounds like a plan, thanks for the advice.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Junio C Hamano @ 2016-12-08 18:01 UTC (permalink / raw)
To: Paul Tan; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <CACRoPnRpZr=E6SW81Vg-2TiOr=RJo1YouAt5iZoE0CNBx-qesg@mail.gmail.com>
Paul Tan <pyokagan@gmail.com> writes:
> Hmm, to add on, looking at the three other call sites of this
> function, two of them (builtin/commit.c and builtin/describe.c)
> basically do:
>
> if (0 <= fd)
> update_index_if_able(...)
>
> with that 0 <= fd conditional. With this patch it becomes three out of
> four.
The other one is diff.c::refresh_index_quietly() that you are not
counting, I think, but if you look at it again, it also is not
called after hold_locked_index() fails to acquire the lock, so with
this fix everybody refrains from calling it when it does not hold
the lock.
> Perhaps the repeated use of this conditional is a sign that the
> 0 <= fd check could be built into update_index_if_able()?
That condition is "do we have the lock? Otherwise we are not even
allowed to update it", so in that sense it may make sense.
> I think there is precedent for building in these kind of checks --
> rollback_lock_file() also does not fail if the lock file was not
> successfully opened.
>
> That said, the number of call sites is quite low so it's probably not
> worth doing this.
Yeah, I can go either way. At least with the change things are
consistent.
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-08 18:04 UTC (permalink / raw)
To: Jeff King; +Cc: vi0oss, git@vger.kernel.org, Stefan Beller
In-Reply-To: <20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net>
On Thu, Dec 8, 2016 at 9:46 AM, Jeff King <peff@peff.net> wrote:
>
> will both trigger on the &&-chain linter, because it uses a magic exit
> code to detect the breakage. I think the problem is just that the
> &&-chain linter cannot peek inside subshells, and that's where the bug
> was in this case.
Uh, yeah in the subshell, but the patch v2 did have it not in
subshells, I'll take another look.
>
> I wish we could improve that, but I spend a lot of brain cycles on it at
> one point and couldn't come up with a workable solution.
Is it possible to overwrite what happens when you open a subshell with ( ) ?
i.e. I imagine in the global test-setup we'd setup that ( ) not just
opens /bin/sh
but a shell with benefits such that we can execute just one && chain.
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: vi0oss @ 2016-12-08 18:04 UTC (permalink / raw)
To: Jeff King; +Cc: Stefan Beller, git@vger.kernel.org, Stefan Beller
In-Reply-To: <20161208174633.bsktiflql6jpn5t3@sigill.intra.peff.net>
On 12/08/2016 08:46 PM, Jeff King wrote:
> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:
>
>> On Wed, Dec 7, 2016 at 4:39 PM, <vi0oss@gmail.com> wrote:
>>
>>> Previously test contained errorneous
>>> test_must_fail, which was masked by
>>> missing &&.
>> I wonder if we could make either
>> the test_must_fail intelligent to detect such a broken && call chain
>> or the test_expect_success macro to see for those broken chains.
>>
>>
>> I wish we could improve that, but I spend a lot of brain cycles on it at
>> one point and couldn't come up with a workable solution.
>>
>> -Peff
>>
Why Git test use &&-chains instead of proper "set -e"?
^ permalink raw reply
* Re: [PATCH 01/17] mv: convert to using pathspec struct interface
From: Brandon Williams @ 2016-12-08 18:06 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8DVuOdRsu4x9iFTwweSVn=RRJYzBTm43ufBevTWMf4Qmg@mail.gmail.com>
On 12/08, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 7:36 AM, Brandon Williams <bmwill@google.com> wrote:
> >> > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,
> >> > {
> >> > int i;
> >> > const char **result;
> >> > + struct pathspec ps;
> >> > ALLOC_ARRAY(result, count + 1);
> >> > - COPY_ARRAY(result, pathspec, count);
> >> > - result[count] = NULL;
> >> > +
> >> > + /* Create an intermediate copy of the pathspec based on the flags */
> >> > for (i = 0; i < count; i++) {
> >> > - int length = strlen(result[i]);
> >> > + int length = strlen(pathspec[i]);
> >> > int to_copy = length;
> >> > + char *it;
> >> > while (!(flags & KEEP_TRAILING_SLASH) &&
> >> > - to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
> >> > + to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
> >> > to_copy--;
> >> > - if (to_copy != length || flags & DUP_BASENAME) {
> >> > - char *it = xmemdupz(result[i], to_copy);
> >> > - if (flags & DUP_BASENAME) {
> >> > - result[i] = xstrdup(basename(it));
> >> > - free(it);
> >> > - } else
> >> > - result[i] = it;
> >> > - }
> >> > +
> >> > + it = xmemdupz(pathspec[i], to_copy);
> >> > + if (flags & DUP_BASENAME) {
> >> > + result[i] = xstrdup(basename(it));
> >> > + free(it);
> >> > + } else
> >> > + result[i] = it;
> >> > + }
> >> > + result[count] = NULL;
> >> > +
> >> > + parse_pathspec(&ps,
> >> > + PATHSPEC_ALL_MAGIC &
> >> > + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> >> > + PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
> >> > + prefix, result);
> >> > + assert(count == ps.nr);
> >> > +
> >> > + /* Copy the pathspec and free the old intermediate strings */
> >> > + for (i = 0; i < count; i++) {
> >> > + const char *match = xstrdup(ps.items[i].match);
> >> > + free((char *) result[i]);
> >> > + result[i] = match;
> >>
> >> Sigh.. it looks so weird that we do all the parsing (in a _copy_
> >> pathspec function) then remove struct pathspec and return the plain
> >> string. I guess we can't do anything more until we rework cmd_mv code
> >> to handle pathspec natively.
> >>
> >> At the least I think we should rename this function to something else.
> >> But if you have time I really wish we could kill this function. I
> >> haven't stared at cmd_mv() long and hard, but it looks to me that we
> >> combining two separate functionalities in the same function here.
> >>
> >> If "mv" takes n arguments, then the first <n-1> arguments may be
> >> pathspec, the last one is always a plain path. The "dest_path =
> >> internal_copy_pathspec..." could be as simple as "dest_path =
> >> prefix_path(argv[argc - 1])". the special treatment for this last
> >> argument [1] can live here. Then, we can do parse_pathspec for the
> >> <n-1> arguments in cmd_mv(). It's still far from perfect, because
> >> cmd_mv can't handle pathspec properly, but it reduces the messy mess
> >> in internal_copy_pathspec a bit, I hope.
> >>
> >
> > Actually, after looking at this a bit more it seems like we could
> > technically use prefix_path for both source and dest (based on how the
> > current code is structured) since the source's provied must all exist (as
> > in no wildcards are allowed). We could drop using the pathspec struct
> > completely in addition to renaming the function (to what I'm still
> > unsure).
>
> Yeah that sounds good too (with a caveat: I'm not a heavy user of
> git-mv nor touching this code a lot, I might be missing something).
> It'll take some looong time before somebody starts converting it to
> use pathspec properly, I guess. prefix_path() would keep the code
> clean meanwhile.
K for now I'll switch to using prefix_path() and rename the function
`internal_prefix_pathspec()` as that is a bit more descriptive.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
From: Robbie Iannucci @ 2016-12-08 18:10 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.2.20.1612081252490.23160@virtualbox>
Thanks all for taking a look at this, I didn't expect such a quick response :).
No hard feelings re: breakage; I know how gnarly these sorts of
refactors can be (and more libification is always better :)).
Robbie
On Thu, Dec 8, 2016 at 3:53 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Junio,
>
> On Wed, 7 Dec 2016, Junio C Hamano wrote:
>
>> The "libify sequencer" topic stopped passing the die_on_error option
>> to hold_locked_index(), and this lost an error message from "git
>> merge --ff-only $commit" when there are competing updates in
>> progress.
>
> Sorry for the breakage.
>
> When libifying the code, I tried to be careful to retain the error
> messages when not dying, and mistakenly assumed that hold_locked_index()
> would do the same.
>
> Ciao,
> Dscho
^ permalink raw reply
* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-08 18:19 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8DtUwnOjBV49navkfgqPzEsNuX2LVaeVU=Ap2PWLpGFdA@mail.gmail.com>
On 12/08, Duy Nguyen wrote:
> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/07, Duy Nguyen wrote:
> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> >> > Convert 'create_simplify()' to use the pathspec struct interface from
> >> > using the '_raw' entry in the pathspec.
> >>
> >> It would be even better to kill this create_simplify() and let
> >> simplify_away() handle struct pathspec directly.
> >>
> >> There is a bug in this code, that might have been found if we
> >> simpify_away() handled pathspec directly: the memcmp() in
> >> simplify_away() will not play well with :(icase) magic. My bad. If
> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> >> ignore exclude patterns there too (although not excluding is not a
> >> bug).
> >
> > So are you implying that the simplify struct needs to be killed? That
> > way the pathspec struct itself is being passed around instead?
>
> Yes. simplify struct was a thing when pathspec was an array of char *.
> At this point I think it can retire (when we have time to retire it)
Alright, then for now I can leave this change as is and have a follow up
series that kills the simplify struct.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-08 18:21 UTC (permalink / raw)
To: vi0oss; +Cc: Jeff King, git@vger.kernel.org, Stefan Beller
In-Reply-To: <d445a6c3-5375-22cf-4f03-1717559f1157@gmail.com>
On Thu, Dec 8, 2016 at 10:04 AM, vi0oss <vi0oss@gmail.com> wrote:
> On 12/08/2016 08:46 PM, Jeff King wrote:
>>
>> On Wed, Dec 07, 2016 at 05:22:30PM -0800, Stefan Beller wrote:
>>
>>> On Wed, Dec 7, 2016 at 4:39 PM, <vi0oss@gmail.com> wrote:
>>>
>>>> Previously test contained errorneous
>>>> test_must_fail, which was masked by
>>>> missing &&.
>>>
>>> I wonder if we could make either
>>> the test_must_fail intelligent to detect such a broken && call chain
>>> or the test_expect_success macro to see for those broken chains.
>>>
>>>
>>> I wish we could improve that, but I spend a lot of brain cycles on it at
>>> one point and couldn't come up with a workable solution.
>>>
>>> -Peff
>>>
> Why Git test use &&-chains instead of proper "set -e"?
>
"Because set -e kills the shell and we would want to keep going
until the test suite is finished and display a summary what failed"
would be my first reaction, but let's dig into history:
bb79af9d09 might be helpful on that, but it doesn't explain why
we use && chains.
I could not find any commit explaining the use of && chains.
e1970ce43abf might be interesting (the introduction of the test
suite), as that did not contain && chains.
I guess it would be hard(er) to implement e.g. test_must_fail
in an environment where -e is set.
^ permalink raw reply
* Re: [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
From: Junio C Hamano @ 2016-12-08 18:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Robbie Iannucci
In-Reply-To: <alpine.DEB.2.20.1612081252490.23160@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Sorry for the breakage.
Apologies from me, too. Once a topic is merged, the credit still
remains with the contributor, but the blame is shared by the project
as a whole, with those who missed breakages during their reviews,
and those who didn't review or test and let breakages pass.
> When libifying the code, I tried to be careful to retain the error
> messages when not dying,...
Quite honestly, I do not think either of us cared about preserving
the exact error message the end-user was getting from each failure
sites that the series changed a call with die-on-error=1 to a call
with die-on-error=0 that is followed by a negative return while
reviewing this series. As I wrote in the proposed log message for
3/3, this one was noticed as a end-user breaking change because it
was the only one that has become totally silent. For example, this
bit from sequencer.c::write_message() we can see in the output from
"git show --first-parent 2a4062a4a8" does not preserve the message
at all:
diff --git a/sequencer.c b/sequencer.c
index 3804fa931d..eec8a60d6b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -180,17 +180,20 @@
...
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
{
static struct lock_file msg_file;
- int msg_fd = hold_lock_file_for_update(&msg_file, filename,
- LOCK_DIE_ON_ERROR);
+ int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
+ if (msg_fd < 0)
+ return error_errno(_("Could not lock '%s'"), filename);
And I do not think it is necessarily bad that the error message
changed with this conversion. In other words, I do not think it
should have been the goal to preserve the exact error message.
hold_lock*() can afford to give a detailed message that strongly
sounds as being the final decision when called with die-on-error=1
because it knows it is dying. However, the message from the updated
write_message(), "could not lock", cannot be final---the caller may
want to add something else after it to describe what failed in a
larger picture.
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Johannes Sixt @ 2016-12-08 18:41 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Brandon Williams, Junio C Hamano, Ramsay Jones, git, sbeller,
peff, jacob.keller
In-Reply-To: <20161208075555.GA23595@tb-raspi>
Am 08.12.2016 um 08:55 schrieb Torsten Bögershausen:
> Some conversion may be done in mingw.c:
> https://github.com/github/git-msysgit/blob/master/compat/mingw.c
> So what I understand, '/' in Git are already converted into '\' if needed ?
Only if needed, and there are not many places where this is the case.
(Actually, I can't find one place where we do.) In particular, typical
file accesses does not require the conversion.
> It seams that we may wnat a function get_start_of_path(uncpath),
> which returns:
>
> get_start_of_path_win("//?/D:/very-long-path") "/very-long-path"
We have offset_1st_component().
-- Hannes
^ 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