login
Search
Hi, this is the 3rd revision of the work-tree clean up series. Unlike the 1st revision, this passes all the tests. Unlike the 2nd revision, it has a concise and precise logic: --work-tree=bla overrides GIT_WORK_TREE, which overrides core.bare = true, which overrides core.worktree, which overrides GIT_DIR/.. when GIT_DIR ends in /.git, which overrides the directory in which .git/ was found. The distinction between git_dir and work_tree is much clearer now: you can have a work_tree which is inside a git_dir, and programs needing a work_tree will no longer complain. The work tree information is no longer just thrown away. Instead, you can run git from the git_dir and it will work on the work tree without you having to cd back and forth. The wrong distinction between a non-bare repository and a repository with a work_tree is no longer there. A repository is either bare, or it has a working directory. There is no third option. Ciao, Dscho -
This patch adds convenience functions to work with absolute paths.
The function is_absolute_path() should help the efforts to integrate
the MinGW fork.
Note that make_absolute_path() returns a pointer to a static buffer.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Makefile | 2 +-
cache.h | 5 ++++
path.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++
t/t0000-basic.sh | 16 ++++++++++++
test-absolute-path.c | 11 ++++++++
5 files changed, 98 insertions(+), 1 deletions(-)
create mode 100644 test-absolute-path.c
diff --git a/Makefile b/Makefile
index d8100ad..546e008 100644
--- a/Makefile
+++ b/Makefile
@@ -936,7 +936,7 @@ endif
### Testing rules
-TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X
+TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X test-absolute-path$X
all:: $(TEST_PROGRAMS)
diff --git a/cache.h b/cache.h
index 53801b8..98af530 100644
--- a/cache.h
+++ b/cache.h
@@ -358,6 +358,11 @@ int git_config_perm(const char *var, const char *value);
int adjust_shared_perm(const char *path);
int safe_create_leading_directories(char *path);
char *enter_repo(char *path, int strict);
+static inline int is_absolute_path(const char *path)
+{
+ return path[0] == '/';
+}
+const char *make_absolute_path(const char *path);
/* Read and unpack a sha1 file into memory, write memory to a sha1 file */
extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/path.c b/path.c
index dfff41f..dfa6ce0 100644
--- a/path.c
+++ b/path.c
@@ -288,3 +288,68 @@ int adjust_shared_perm(const char *path)
return -2;
return 0;
}
+
+/* We allow "recursive" symbolic links. Only within reason, though. */
+#define MAXDEPTH 5
+
+const char *make_absolute_path(const char *path)
+{
+ static char bufs[2][PATH_MAX + 1], *buf = bufs[0], ...The function get_relative_cwd() works just as getcwd(), only that it
takes an absolute path as additional parameter, returning the prefix
of the current working directory relative to the given path. If the
cwd is no subdirectory of the given path, it returns NULL.
is_inside_dir() is just a trivial wrapper over get_relative_cwd().
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
dir.c | 30 ++++++++++++++++++++++++++++++
dir.h | 3 +++
2 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/dir.c b/dir.c
index 8d8faf5..ef67d6f 100644
--- a/dir.c
+++ b/dir.c
@@ -642,3 +642,33 @@ file_exists(const char *f)
struct stat sb;
return stat(f, &sb) == 0;
}
+
+char *get_relative_cwd(char *buffer, int size, const char *dir)
+{
+ char *cwd = buffer;
+
+ if (!dir)
+ return 0;
+
+ if (!getcwd(buffer, size))
+ return 0;
+
+ if (!is_absolute_path(dir))
+ dir = make_absolute_path(dir);
+
+ while (*dir && *dir == *cwd) {
+ dir++;
+ cwd++;
+ }
+ if (*dir)
+ return NULL;
+ if (*cwd == '/')
+ return cwd + 1;
+ return cwd;
+}
+
+int is_inside_dir(const char *dir)
+{
+ char buffer[PATH_MAX];
+ return get_relative_cwd(buffer, sizeof(buffer), dir) != NULL;
+}
diff --git a/dir.h b/dir.h
index ec0e8ab..f55a87b 100644
--- a/dir.h
+++ b/dir.h
@@ -61,4 +61,7 @@ extern void add_exclude(const char *string, const char *base,
extern int file_exists(const char *);
extern struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len);
+extern char *get_relative_cwd(char *buffer, int size, const char *dir);
+extern int is_inside_dir(const char *dir);
+
#endif
--
1.5.3.rc3.28.g1406
-
Some lines were not indented by tabs but by spaces.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Just to make the next patch look nicer.
setup.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/setup.c b/setup.c
index 7b07144..b54d65f 100644
--- a/setup.c
+++ b/setup.c
@@ -382,11 +382,11 @@ int git_config_perm(const char *var, const char *value)
int check_repository_format_version(const char *var, const char *value)
{
- if (strcmp(var, "core.repositoryformatversion") == 0)
- repository_format_version = git_config_int(var, value);
+ if (strcmp(var, "core.repositoryformatversion") == 0)
+ repository_format_version = git_config_int(var, value);
else if (strcmp(var, "core.sharedrepository") == 0)
shared_repository = git_config_perm(var, value);
- return 0;
+ return 0;
}
int check_repository_format(void)
--
1.5.3.rc3.28.g1406
-
The old version of work-tree support was an unholy mess, barely readable, and not to the point. For example, why do you have to provide a worktree, when it is not used? As in "git status". Now it works. Another riddle was: if you can have work trees inside the git dir, why are some programs complaining that they need a work tree? IOW when inside repo.git/work, where GIT_DIR points to repo.git and GIT_WORK_TREE to work, and cwd is work, --is-inside-git-dir _must_ return true, because it is _in the git dir_, but scripts _must_ test for the right thing. Also, GIT_DIR=../.git should behave the same as if no GIT_DIR was specified, unless there is a repository in the current working directory. It does now. The logic to determine if a repository is bare, or has a work tree (tertium non datur), is this: --work-tree=bla overrides GIT_WORK_TREE, which overrides core.bare = true, which overrides core.worktree, which overrides GIT_DIR/.. when GIT_DIR ends in /.git, which overrides the directory in which .git/ was found. In related news, a long standing bug was fixed: when in .git/bla/x.git/, which is a bare repository, git formerly assumed ../.. to be the appropriate git dir. This problem was reported by Shawn Pearce to have caused much pain, where a colleague mistakenly ran "git init" in "/" a long time ago, and bare repositories just would not work. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- This version no longer removes more lines than it adds, but it adds a lot more documentation than it removes. IMHO it is also easier to follow, implements a sane logic, and stands a chance to be fixed by somebody else than the original author. builtin-rev-parse.c | 7 ++ cache.h | 2 + environment.c | 35 +++++-- setup.c | 279 +++++++++++++++++++++++---------------------------- 4 files changed, 162 insertions(+), 161 deletions(-) diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c index ...
With the function set_git_dir() you can reset the path that will
be used for git_path(), git_dir() and friends.
The responsibility to close files and throw away information from the
old git_dir lies with the caller.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
cache.h | 1 +
environment.c | 8 ++++++++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index c0cab34..36963f2 100644
--- a/cache.h
+++ b/cache.h
@@ -216,6 +216,7 @@ extern char *get_refs_directory(void);
extern char *get_index_file(void);
extern char *get_graft_file(void);
extern const char *get_git_work_tree(void);
+extern int set_git_dir(const char *path);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
diff --git a/environment.c b/environment.c
index 26f41af..2af12fd 100644
--- a/environment.c
+++ b/environment.c
@@ -124,3 +124,11 @@ char *get_graft_file(void)
setup_git_env();
return git_graft_file;
}
+
+int set_git_dir(const char *path)
+{
+ if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
+ return error("Could not set GIT_DIR to '%s'", path);
+ setup_git_env();
+ return 0;
+}
--
1.5.3.rc3.28.g1406
-
It is allowed to call
$ git --git-dir=../ --work-tree=. bla
when you really want to. In this case, you are both in the git directory
and in the working tree.
The earlier handling of this situation was seriously bogus. For regular
working tree operations, it checked if inside git dir. That makes no
sense, of course, since the check should be for a work tree, and nothing
else.
Fix that.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin-ls-files.c | 8 +++++---
git-sh-setup.sh | 3 +--
git.c | 11 ++++++++---
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 61577ea..d36181a 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -469,9 +469,11 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
break;
}
- if (require_work_tree &&
- (!is_inside_work_tree() || is_inside_git_dir()))
- die("This operation must be run in a work tree");
+ if (require_work_tree && !is_inside_work_tree()) {
+ const char *work_tree = get_git_work_tree();
+ if (!work_tree || chdir(work_tree))
+ die("This operation must be run in a work tree");
+ }
pathspec = get_pathspec(prefix, argv + i);
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c51985e..7bef43f 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -59,8 +59,7 @@ cd_to_toplevel () {
}
require_work_tree () {
- test $(git rev-parse --is-inside-work-tree) = true &&
- test $(git rev-parse --is-inside-git-dir) = false ||
+ test $(git rev-parse --is-inside-work-tree) = true ||
die "fatal: $0 cannot be used without a working tree."
}
diff --git a/git.c b/git.c
index a647f9c..2433355 100644
--- a/git.c
+++ b/git.c
@@ -272,9 +272,14 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
prefix = setup_git_directory();
if (p->option & USE_PAGER)
setup_pager();
- if ((p->option & NEED_WORK_TREE) &&
- ...Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin-init-db.c | 48 +++++++++++-------------------------------------
1 files changed, 11 insertions(+), 37 deletions(-)
diff --git a/builtin-init-db.c b/builtin-init-db.c
index 66ddaeb..0d9b1e0 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -174,36 +174,7 @@ static void copy_templates(const char *git_dir, int len, const char *template_di
closedir(dir);
}
-/*
- * Get the full path to the working tree specified in $GIT_WORK_TREE
- * or NULL if no working tree is specified.
- */
-static const char *get_work_tree(void)
-{
- const char *git_work_tree;
- char cwd[PATH_MAX];
- static char worktree[PATH_MAX];
-
- git_work_tree = getenv(GIT_WORK_TREE_ENVIRONMENT);
- if (!git_work_tree)
- return NULL;
- if (!getcwd(cwd, sizeof(cwd)))
- die("Unable to read current working directory");
- if (chdir(git_work_tree))
- die("Cannot change directory to specified working tree '%s'",
- git_work_tree);
- if (git_work_tree[0] != '/') {
- if (!getcwd(worktree, sizeof(worktree)))
- die("Unable to read current working directory");
- git_work_tree = worktree;
- }
- if (chdir(cwd))
- die("Cannot come back to cwd");
- return git_work_tree;
-}
-
-static int create_default_files(const char *git_dir, const char *git_work_tree,
- const char *template_path)
+static int create_default_files(const char *git_dir, const char *template_path)
{
unsigned len = strlen(git_dir);
static char path[PATH_MAX];
@@ -282,16 +253,16 @@ static int create_default_files(const char *git_dir, const char *git_work_tree,
}
git_config_set("core.filemode", filemode ? "true" : "false");
- if (is_bare_repository() && !git_work_tree) {
+ if (is_bare_repository())
git_config_set("core.bare", "true");
- }
else {
+ const char *work_tree = get_git_work_tree();
git_config_set("core.bare", "false");
/* allow template config file to override the default */
if (log_all_ref_updates == -1)
...t1501 is still a PITA to debug. But I decided not to change it, so that it is easier to see what the differences in the logic are. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- t/t1501-worktree.sh | 24 +++++++++++++++++++----- 1 files changed, 19 insertions(+), 5 deletions(-) diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh index aadeeab..7322161 100755 --- a/t/t1501-worktree.sh +++ b/t/t1501-worktree.sh @@ -33,17 +33,17 @@ mv .git repo.git || exit 1 say "core.worktree = relative path" export GIT_DIR=repo.git -export GIT_CONFIG=$GIT_DIR/config +export GIT_CONFIG="$(pwd)"/$GIT_DIR/config unset GIT_WORK_TREE git config core.worktree ../work test_rev_parse 'outside' false false false cd work || exit 1 export GIT_DIR=../repo.git -export GIT_CONFIG=$GIT_DIR/config +export GIT_CONFIG="$(pwd)"/$GIT_DIR/config test_rev_parse 'inside' false false true '' cd sub/dir || exit 1 export GIT_DIR=../../../repo.git -export GIT_CONFIG=$GIT_DIR/config +export GIT_CONFIG="$(pwd)"/$GIT_DIR/config test_rev_parse 'subdirectory' false false true sub/dir/ cd ../../.. || exit 1 @@ -84,9 +84,23 @@ test_rev_parse 'in repo.git' false true false cd objects || exit 1 test_rev_parse 'in repo.git/objects' false true false cd ../work || exit 1 -test_rev_parse 'in repo.git/work' false false true '' +test_rev_parse 'in repo.git/work' false true true '' cd sub/dir || exit 1 -test_rev_parse 'in repo.git/sub/dir' false false true sub/dir/ +test_rev_parse 'in repo.git/sub/dir' false true true sub/dir/ cd ../../../.. || exit 1 +test_expect_success 'repo finds its work tree' ' + (cd repo.git && + : > work/sub/dir/untracked && + test sub/dir/untracked = "$(git ls-files --others)") +' + +test_expect_success 'repo finds its work tree from work tree, too' ' + (cd repo.git/work/sub/dir && + : > tracked && + git --git-dir=../../.. add tracked && + cd ../../.. && + test sub/dir/tracked = ...
When GIT_DIR=../.git, and no worktree is specified, it is reasonable
to assume that the repository is not bare, that the work tree is ".."
and that the prefix is the basename of the current directory.
This is the sane behavior.
t1500 tested for the old behavior, which was plain wrong. And this
patch fixes it minimally.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t1500-rev-parse.sh | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index ec49966..bea40cb 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -31,9 +31,9 @@ test_rev_parse() {
test_rev_parse toplevel false false true ''
cd .git || exit 1
-test_rev_parse .git/ false true true .git/
+test_rev_parse .git/ true true false ''
cd objects || exit 1
-test_rev_parse .git/objects/ false true true .git/objects/
+test_rev_parse .git/objects/ true true false ''
cd ../.. || exit 1
mkdir -p sub/dir || exit 1
@@ -42,7 +42,7 @@ test_rev_parse subdirectory false false true sub/dir/
cd ../.. || exit 1
git config core.bare true
-test_rev_parse 'core.bare = true' true false true
+test_rev_parse 'core.bare = true' true false false
git config --unset core.bare
test_rev_parse 'core.bare undefined' false false true
@@ -50,28 +50,28 @@ test_rev_parse 'core.bare undefined' false false true
mkdir work || exit 1
cd work || exit 1
export GIT_DIR=../.git
-export GIT_CONFIG="$GIT_DIR"/config
+export GIT_CONFIG="$(pwd)"/../.git/config
git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true work/
git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false true ''
+test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false ...Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
This patch is just here to demonstrate how a core.bare = true /
core.worktree conflict could be caught.
Personally, I do not deem it worth the effort, as can be seen by
the lack of adjustments to t1501, which fails utterly with this
patch.
environment.c | 3 +--
setup.c | 8 ++++++--
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/environment.c b/environment.c
index 2af12fd..4c5db32 100644
--- a/environment.c
+++ b/environment.c
@@ -82,8 +82,7 @@ const char *get_git_work_tree(void)
static int initialized = 0;
if (!initialized) {
work_tree = getenv(GIT_WORK_TREE_ENVIRONMENT);
- /* core.bare = true overrides implicit and config work tree */
- if (!work_tree && is_bare_repository_cfg < 1) {
+ if (!work_tree) {
work_tree = git_work_tree_cfg;
/* make_absolute_path also normalizes the path */
if (work_tree && !is_absolute_path(work_tree))
diff --git a/setup.c b/setup.c
index 8237fe3..b0febed 100644
--- a/setup.c
+++ b/setup.c
@@ -342,13 +342,17 @@ int check_repository_format_version(const char *var, const char *value)
shared_repository = git_config_perm(var, value);
else if (strcmp(var, "core.bare") == 0) {
is_bare_repository_cfg = git_config_bool(var, value);
- if (is_bare_repository_cfg == 1)
- inside_work_tree = -1;
+ if (is_bare_repository_cfg == 1 && inside_work_tree < 0)
+ die ("Contradicting config settings for "
+ "bare and worktree");
} else if (strcmp(var, "core.worktree") == 0) {
if (git_work_tree_cfg)
free(git_work_tree_cfg);
git_work_tree_cfg = xstrdup(value);
inside_work_tree = -1;
+ if (is_bare_repository_cfg == 1)
+ die ("Contradicting config settings for "
+ "bare and worktree");
}
return 0;
}
--
1.5.3.rc3.28.g1406
-
Hi, so this is yet another revision of the work-tree clean ups (sorry to all those who grow tired of it; I feel with you: I am tired of it, too). Junio rightfully pointed out that the tests do not succeed after each single step of the series. Alas, after thinking about it for quite some time, I do not think there is any way around squashing all the earlier steps 4,6--9 into one step. There is not really much that can be done about step 6/9: if we are in a work tree: that does not mean that we are _not_ in the git_dir. (And no, this does not break git-clean, as a work tree is a work tree is a work tree. If the user was stupid enough to specify the same directory as GIT_DIR and GIT_WORK_TREE, then that is _her_ problem. Git is a powerful tool, and you can harm yourself with it. Tough.) Patch 7/9 is needed, because the old logic in git-init was wrong, and only hidden by the fact that the work-tree logic was implemented wrongly to begin with. Patches 8 and 9/9 only updated the tests to ensure a sane logic, instead of an unsane one. So they are needed, too. Note: if you are in a bare repository (a repository which either says "core.bare = false" in the config, or which is a direct ancestor directory, i.e. ../[...]/.. of the current working directory) there will _not_ be an automatic working directory assignment. You will be operating _without_ any work tree, unless you specify one. I somehow feel that core.bare = true weighs more than core.worktree = /some/thing, and therefore I implemented it that way, but hey, if enough people disagree, then I'll change it. Maybe I should add two comments? Namely that setup_git_directory_gently() does _not_ check the config if the repository version is right, and where the working directory is, and if the repository is bare. setup_git_directory() does... And that setup_git_directory_gently() _does_ try to be smart about get_git_work_tree(), is_inside_git_dir() and is_inside_work_tree() by assigning ...
This patch adds convenience functions to work with absolute paths.
The function is_absolute_path() should help the efforts to integrate
the MinGW fork.
Note that make_absolute_path() returns a pointer to a static buffer.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Makefile | 2 +-
cache.h | 5 ++++
path.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++
t/t0000-basic.sh | 16 ++++++++++++
test-absolute-path.c | 11 ++++++++
5 files changed, 98 insertions(+), 1 deletions(-)
create mode 100644 test-absolute-path.c
diff --git a/Makefile b/Makefile
index 149df1b..41c4de0 100644
--- a/Makefile
+++ b/Makefile
@@ -937,7 +937,7 @@ endif
### Testing rules
-TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X
+TEST_PROGRAMS = test-chmtime$X test-genrandom$X test-date$X test-delta$X test-sha1$X test-match-trees$X test-absolute-path$X
all:: $(TEST_PROGRAMS)
diff --git a/cache.h b/cache.h
index 53801b8..98af530 100644
--- a/cache.h
+++ b/cache.h
@@ -358,6 +358,11 @@ int git_config_perm(const char *var, const char *value);
int adjust_shared_perm(const char *path);
int safe_create_leading_directories(char *path);
char *enter_repo(char *path, int strict);
+static inline int is_absolute_path(const char *path)
+{
+ return path[0] == '/';
+}
+const char *make_absolute_path(const char *path);
/* Read and unpack a sha1 file into memory, write memory to a sha1 file */
extern int sha1_object_info(const unsigned char *, unsigned long *);
diff --git a/path.c b/path.c
index dfff41f..27b4ac9 100644
--- a/path.c
+++ b/path.c
@@ -288,3 +288,68 @@ int adjust_shared_perm(const char *path)
return -2;
return 0;
}
+
+/* We allow "recursive" symbolic links. Only within reason, though. */
+#define MAXDEPTH 5
+
+const char *make_absolute_path(const char *path)
+{
+ static char bufs[2][PATH_MAX + 1], *buf = bufs[0], ...The function get_relative_cwd() works just as getcwd(), only that it
takes an absolute path as additional parameter, returning the prefix
of the current working directory relative to the given path. If the
cwd is no subdirectory of the given path, it returns NULL.
is_inside_dir() is just a trivial wrapper over get_relative_cwd().
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
dir.c | 32 ++++++++++++++++++++++++++++++++
dir.h | 3 +++
2 files changed, 35 insertions(+), 0 deletions(-)
diff --git a/dir.c b/dir.c
index 8d8faf5..5ea269a 100644
--- a/dir.c
+++ b/dir.c
@@ -642,3 +642,35 @@ file_exists(const char *f)
struct stat sb;
return stat(f, &sb) == 0;
}
+
+/*
+ * get_relative_cwd() gets the prefix of the current working directory
+ * relative to 'dir'. If we are not inside 'dir', it returns NULL.
+ * As a convenience, it also returns NULL if 'dir' is already NULL.
+ */
+char *get_relative_cwd(char *buffer, int size, const char *dir)
+{
+ char *cwd = buffer;
+
+ if (!dir || !getcwd(buffer, size))
+ return NULL;
+
+ if (!is_absolute_path(dir))
+ dir = make_absolute_path(dir);
+
+ while (*dir && *dir == *cwd) {
+ dir++;
+ cwd++;
+ }
+ if (*dir)
+ return NULL;
+ if (*cwd == '/')
+ return cwd + 1;
+ return cwd;
+}
+
+int is_inside_dir(const char *dir)
+{
+ char buffer[PATH_MAX];
+ return get_relative_cwd(buffer, sizeof(buffer), dir) != NULL;
+}
diff --git a/dir.h b/dir.h
index ec0e8ab..f55a87b 100644
--- a/dir.h
+++ b/dir.h
@@ -61,4 +61,7 @@ extern void add_exclude(const char *string, const char *base,
extern int file_exists(const char *);
extern struct dir_entry *dir_add_name(struct dir_struct *dir, const char *pathname, int len);
+extern char *get_relative_cwd(char *buffer, int size, const char *dir);
+extern int is_inside_dir(const char *dir);
+
#endif
--
1.5.3.rc3.94.g3024
#
-
When is it not a fatal error if get_relative_cwd() is called with a NULL dir parameter, or getcwd() fails? If there is no valid such cases, I would rather have this die(), former with "BUG" and the latter with strerror(errno). -
Heh, it turns out that there is this lazy or clever (depending on the viewpoint) caller that passes the return value of get_git_work_tree() to this function and expect this to return NULL when no work tree is found. The callers of the is_* functions are much cleaner and in that sense the series is a definite improvement, but this one particular obscurity makes me wonder if it is replacing one unholy mess with a smaller but still unholy mess. Will apply on "master" and will be part of -rc4, but we probably would want to have a longer pre-final freeze than usual to really make sure this one is good. -
Hi, Right. I thought I had said that (something along the lines: it is more I'll provide a patch which makes the callers of get_relative_cwd() holy, and skip the check in get_relative_cwd(), okay? Ciao, Dscho -
Earlier, get_relative_cwd() interpreted "dir == NULL" as "outside of the dir",
and therefore returned NULL. Be more strict now.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
As promised.
Okay, I made up my mind. Allowing "dir == NULL" is not only a matter of
convenience. It is the most natural way to say that "dir" is an invalid
or non-existing directory.
Besides, this patch adds 14.286% more lines than it removes ;-)
But ultimately, it is your decision, Junio, and I am d'accord with
what you choose.
dir.c | 3 ---
setup.c | 10 +++++++---
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/dir.c b/dir.c
index b3329f4..cfcde13 100644
--- a/dir.c
+++ b/dir.c
@@ -646,7 +646,6 @@ file_exists(const char *f)
/*
* get_relative_cwd() gets the prefix of the current working directory
* relative to 'dir'. If we are not inside 'dir', it returns NULL.
- * As a convenience, it also returns NULL if 'dir' is already NULL.
*/
char *get_relative_cwd(char *buffer, int size, const char *dir)
{
@@ -656,8 +655,6 @@ char *get_relative_cwd(char *buffer, int size, const char *dir)
* a lazy caller can pass a NULL returned from get_git_work_tree()
* and rely on this function to return NULL.
*/
- if (!dir)
- return NULL;
if (!getcwd(buffer, size))
die("can't find the current directory: %s", strerror(errno));
diff --git a/setup.c b/setup.c
index 3653092..2f720f8 100644
--- a/setup.c
+++ b/setup.c
@@ -183,8 +183,10 @@ int is_inside_git_dir(void)
int is_inside_work_tree(void)
{
- if (inside_work_tree < 0)
- inside_work_tree = is_inside_dir(get_git_work_tree());
+ if (inside_work_tree < 0) {
+ const char *work_tree = get_git_work_tree();
+ inside_work_tree = work_tree ? is_inside_dir(work_tree) : 0;
+ }
return inside_work_tree;
}
@@ -370,10 +372,12 @@ const char *setup_git_directory(void)
/* If the work tree is not the default one, recompute prefix */
if (inside_work_tree < 0) ...That is also fine; it only needs to be clarified somehow to people who might wonder what get_relative_cwd() function is used for and how to use it in their programs. The comment that says "As a convenience" may need to become a bit more elaborate to say why it is convenient for the callers to do so (e.g. "The caller may have called another function that returns a directory to obtain the 'dir', which may have returned NULL as a way to say 'there is nothing applicable in your case', and in such a case, your $(cwd) relative to that 'dir' is also something that cannot be used, hence a NULL is returned"). -
The comment did not make a good case why it makes sense. It does so now.
Pointed out by Junio Hamano.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
dir.c | 11 ++++++++++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/dir.c b/dir.c
index b3329f4..791c566 100644
--- a/dir.c
+++ b/dir.c
@@ -646,7 +646,16 @@ file_exists(const char *f)
/*
* get_relative_cwd() gets the prefix of the current working directory
* relative to 'dir'. If we are not inside 'dir', it returns NULL.
- * As a convenience, it also returns NULL if 'dir' is already NULL.
+ *
+ * As a convenience, it also returns NULL if 'dir' is already NULL. The
+ * reason for this behaviour is that it is natural for functions returning
+ * directory names to return NULL to say "this directory does not exist"
+ * or "this directory is invalid". These cases are usually handled the
+ * same as if the cwd is not inside 'dir' at all, so get_relative_cwd()
+ * returns NULL for both of them.
+ *
+ * Most notably, get_relative_cwd(buffer, size, get_git_work_tree())
+ * unifies the handling of "outside work tree" with "no work tree at all".
*/
char *get_relative_cwd(char *buffer, int size, const char *dir)
{
--
1.5.3.rc3.112.gf60b6
-
With the function set_git_dir() you can reset the path that will
be used for git_path(), git_dir() and friends.
The responsibility to close files and throw away information from the
old git_dir lies with the caller.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
cache.h | 1 +
environment.c | 8 ++++++++
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/cache.h b/cache.h
index 98af530..e1f94cb 100644
--- a/cache.h
+++ b/cache.h
@@ -214,6 +214,7 @@ extern char *get_object_directory(void);
extern char *get_refs_directory(void);
extern char *get_index_file(void);
extern char *get_graft_file(void);
+extern int set_git_dir(const char *path);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
diff --git a/environment.c b/environment.c
index f83fb9e..a571fae 100644
--- a/environment.c
+++ b/environment.c
@@ -107,3 +107,11 @@ char *get_graft_file(void)
setup_git_env();
return git_graft_file;
}
+
+int set_git_dir(const char *path)
+{
+ if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
+ return error("Could not set GIT_DIR to '%s'", path);
+ setup_git_env();
+ return 0;
+}
--
1.5.3.rc3.94.g3024
-
The old version of work-tree support was an unholy mess, barely readable, and not to the point. For example, why do you have to provide a worktree, when it is not used? As in "git status". Now it works. Another riddle was: if you can have work trees inside the git dir, why are some programs complaining that they need a work tree? IOW it is allowed to call $ git --git-dir=../ --work-tree=. bla when you really want to. In this case, you are both in the git directory and in the working tree. So, programs have to actually test for the right thing, namely if they are inside a working tree, and not if they are inside a git directory. Also, GIT_DIR=../.git should behave the same as if no GIT_DIR was specified, unless there is a repository in the current working directory. It does now. The logic to determine if a repository is bare, or has a work tree (tertium non datur), is this: --work-tree=bla overrides GIT_WORK_TREE, which overrides core.bare = true, which overrides core.worktree, which overrides GIT_DIR/.. when GIT_DIR ends in /.git, which overrides the directory in which .git/ was found. In related news, a long standing bug was fixed: when in .git/bla/x.git/, which is a bare repository, git formerly assumed ../.. to be the appropriate git dir. This problem was reported by Shawn Pearce to have caused much pain, where a colleague mistakenly ran "git init" in "/" a long time ago, and bare repositories just would not work. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- builtin-init-db.c | 48 ++------- builtin-ls-files.c | 8 +- builtin-rev-parse.c | 7 ++ cache.h | 2 + environment.c | 35 +++++-- git-sh-setup.sh | 3 +- git.c | 11 ++- setup.c | 279 +++++++++++++++++++++++--------------------------- t/t1500-rev-parse.sh | 20 ++-- t/t1501-worktree.sh | 24 ++++- 10 files changed, 216 insertions(+), 221 deletions(-) diff --git ...
Without continuing with negatives, let's try to define the new, corrected world order. I do not think the following is exactly what your cleaned-up version tries to perform, but I am writing this down primarily to demonstrate the style and the level of detail I expect to accompany a clean-up patch like this. ---------------------------------------------------------------- Definitions: - You can have "checked out" files on the filesystem, and such files are said to be in your "work tree". The directory on the filesystem that corresponds to the toplevel entries of the index and the tree objects directly contained in the commit objects is called "the toplevel of the work tree", or simply "work tree" if it is not ambiguous from the context. - The directory that holds git repository information is called "git directory". This is typically .git directory at the toplevel of your work tree, but not necessarily so. - You can perform many git operations without a work tree, but some operations fundamentally require you to have one (e.g. checkout and diff, unless two tree-ishes are given, do not make sense without a work tree). There are four predicates, two interrogators, and two manipulators: - is_inside_git_dir(): this returns true if the $cwd is the git directory or its subdirectory. [IS THIS STILL NEEDED???] - is_inside_work_tree(): this returns true if the $cwd is inside work tree (i.e. either at the toplevel of the work tree or its subdirectory). [NEEDSHELP: is .git in the usual layout considered "is_inside_work_tree()"? Should it?] - is_bare_repository(): this returns true if no work tree is found. There is a corresponding function usable from the scripts. - require_work_tree (shell): this is called by scripts that needs to have a work tree to operate, and barfs otherwise. - get_git_dir(): this returns the location of the git directory. With GIT_DIR environment variable, or ...
Hi, After reading your description I sink into the ground in shame. I really like the style this has, and agree that something as nice as this should .git/ is not considered part of the work tree, even if it is _physically_ Yes. Builtins which need a working tree expect to start at the toplevel of the work tree (which I like to call "working directory", because it is described as such in the glossary AFAIR), and therefore they chdir() to the toplevel in any case. I'll be running "master"+worktree+branch-newdir for the remainder of the 1.5.3-rc period, to be sure that all works as intended. Ciao, Dscho -
Not too late. There always is a room for Documentation/ improvements ;-) Seriously, I think config.txt mentions GIT_WORK_TREE and GIT_DIR and git.txt has separate sections for environment and config, but I do not see a good section that gives a comprehensive view of how they work together to affect your git experience. Perhaps between File/Directory Structure and Terminology sections we would want to have a description of how repository and worktree relate to each other. -
Similarly to this change, I am wondering if we would want to fix
verify_non_filename() in setup.c, which does this:
/*
* Verify a filename that we got as an argument for a pathspec
* entry. Note that a filename that begins with "-" never verifies
* as true, because even if such a filename were to exist, we want
* it to be preceded by the "--" marker (or we want the user to
* use a format like "./-filename")
*/
void verify_filename(const char *prefix, const char *arg)
{
...
}
/*
* Opposite of the above: the command line did not have -- marker
* and we parsed the arg as a refname. It should not be interpretable
* as a filename.
*/
void verify_non_filename(const char *prefix, const char *arg)
{
const char *name;
struct stat st;
if (!is_inside_work_tree() || is_inside_git_dir())
return;
if (*arg == '-')
return; /* flag */
name = prefix ? prefix_filename(prefix, strlen(prefix), arg) : arg;
if (!lstat(name, &st))
die("ambiguous argument '%s': both revision and filename\n"
"Use '--' to separate filenames from revisions", arg);
if (errno != ENOENT)
die("'%s': %s", arg, strerror(errno));
}
At this point, we are given an ambiguous parameter, that could
be naming a path in the work tree. If we are not in the work
tree, then it is understandable that we do not have to barf.
The other check (i.e. "|| is_inside_git_dir()") does not hurt
(iow, it is not an incorrect check per-se), because if you did
"cd .git && git log HEAD" then the HEAD parameter cannot be
naming the path ".git/HEAD" in the work tree, but (1) that is
already covered by .git/ being "outside of work tree", and (2)
it is not something this function wants to check anyway
(i.e. "can the parameter be naming a file in the work tree?").
Am I mistaken and/or confused?
-
Hi, I think you are completely right. Inside a bare repository, "git log FETCH_HEAD" should not need to complain. And I think that the "is_inside_git_dir()" could be _replaced_ by "!is_inside_work_tree()", since that is the intent of that call. Ciao, Dscho -
I think we might have a slight misunderstanding. The "clean"
issue that was raised in an ancient thread was this sequence:
$ git init
$ cd .git
$ git clean
It did not involve GIT_DIR (nor GIT_WORK_TREE as it was not even
there). The point was that not all subdirectories of the
toplevel (i.e. the directory on the filesystem that corresponds
to the root level of your index entries and trees contained in
your commits) were safe to answer yes when asked "are we safe to
perform this worktree oriented command here". Because no trees
nor index would have .git/ subdirectory tracked, "git clean"
will happily remove everything under .git/ (which is $cwd in the
above sequence).
I personally feel that the above sequence is a pilot error and
not worth worrying about, but as people wanted to have that
extra safety, and as we added that (arguably stupid) safety way
before the WORK_TREE stuff, we should mention it if we are
Sorry, I cannot interpret the condition part of the sentence,
nor "There will _not_ be an automatic assignment" part.
By the latter, do you mean to say your $cwd is assumed to be the
top of the working tree unless GIT_WORK_TREE or core.worktree,
if you are in a bare repository? Or it is assumed that you do
not have a worktree and worktree oriented operations that
require a worktree such as "git diff-files" and "git status"
Personally, I think
[core]
bare = true
worktree = /some/where
is a configuration error, but probably I am missing a useful use
Agreed; gently() is there primarily because some commands do not
mind not having a git repository at all and if we do have a
repository to work against we probably should do the same checks
as setup_git_directory() would.
-
Hi, I very much _did_ mean that case. When "git clean" is run in ".git/", it should not say that it is in the working tree. But I guess that my patch series is not really looking out for that; I'll make that an add-on patch. (But that _will_ have to wait until tomorrow afternoon.) Ciao, Dscho -
Hi,
I should not have answered so early.
In setup.c, I put in a comment that explains clearly where (in the absence
of GIT_DIR) setup_git_directory_gently() looks for the git directory:
/*
* Test in the following order (relative to the cwd):
* - .git/
* - ./ (bare)
* - ../.git/
* - ../ (bare)
* - ../../.git/
* etc.
*/
At least I hope that this explanation is clear.
So what happens in this case:
$ git init
$ cd .git
$ git clean
In setup_git_directory_gently(), it is tested first if there is a
subdirectory .git/. No, none. Then it is tested if "." is a git
directory. Yes! So, work_tree is set to NULL tentatively (to be
overridden by either core.worktree or GIT_WORK_TREE), and it is assumed to
be bare (also subject to overriding). So all is well!
You might have noticed that I left out --work-tree= handling; when
--work-tree=<something> is specified, GIT_WORK_TREE is _forced_ to the new
value, so it is literally handled by the same code as GIT_WORK_TREE.
Ciao,
Dscho
-
