From: Junio C Hamano <gitster@pobox.com>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: git@vger.kernel.org, Anders Kaseorg <andersk@MIT.EDU>,
David Aguilar <davvid@gmail.com>,
Jiang Xin <worldhello.net@gmail.com>,
Lea Wiemann <lewiemann@gmail.com>,
David Reiss <dreiss@facebook.com>, Johannes Sixt <j6t@kdbg.org>,
"Lars R. Damerow" <lars@pixar.com>, Jeff King <peff@peff.net>,
Marc Jordan <marc.jordan@disneyanimation.com>
Subject: Re: [RFC] Provide a mechanism to turn off symlink resolution in ceiling paths
Date: Thu, 21 Feb 2013 14:53:42 -0800 [thread overview]
Message-ID: <7vk3q1th1l.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1361351364-15479-1-git-send-email-mhagger@alum.mit.edu> (Michael Haggerty's message of "Wed, 20 Feb 2013 10:09:24 +0100")
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Unfortunately I am swamped with other work right now so I don't have
> time to test the code and might not be able to respond promptly to
> feedback.
A note like the above is a good way to give a cue to others so that
we can work together to pick up, tie the loose ends and move us
closer to the goal, and is very much appreciated.
I think the patch makes sense; I expanded on the part that has
Anders's report in the log message and added a trivial test.
Testing and eyeballing by others would help very much. We'd
obviously need our sign-off as well ;-)
Thanks.
-- >8 --
From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Wed, 20 Feb 2013 10:09:24 +0100
Subject: [PATCH] Provide a mechanism to turn off symlink resolution in ceiling paths
Commit 1b77d83cab 'setup_git_directory_gently_1(): resolve symlinks
in ceiling paths' changed the setup code to resolve symlinks in the
entries in GIT_CEILING_DIRECTORIES. Because those entries are
compared textually to the symlink-resolved current directory, an
entry in GIT_CEILING_DIRECTORIES that contained a symlink would have
no effect. It was known that this could cause performance problems
if the symlink resolution *itself* touched slow filesystems, but it
was thought that such use cases would be unlikely. The intention of
the earlier change was to deal with a case when the user has this:
GIT_CEILING_DIRECTORIES=/home/gitster
but in reality, /home/gitster is a symbolic link to somewhere else,
e.g. /net/machine/home4/gitster. A textual comparison between the
specified value /home/gitster and the location getcwd(3) returns
would not help us, but readlink("/home/gitster") would still be
fast.
After this change was released, Anders Kaseorg <andersk@mit.edu>
reported:
> [...] my computer has been acting so slow when I’m not connected to
> the network. I put various network filesystem paths in
> $GIT_CEILING_DIRECTORIES, such as
> /afs/athena.mit.edu/user/a/n/andersk (to avoid hitting its parents
> /afs/athena.mit.edu, /afs/athena.mit.edu/user/a, and
> /afs/athena.mit.edu/user/a/n which all live in different AFS
> volumes). Now when I’m not connected to the network, every
> invocation of Git, including the __git_ps1 in my shell prompt, waits
> for AFS to timeout.
To allow users to work this around, give them a mechanism to turn
off symlink resolution in GIT_CEILING_DIRECTORIES entries. All the
entries that follow an empty entry will not be checked for symbolic
links and used literally in comparison. E.g. with these:
GIT_CEILING_DIRECTORIES=:/foo/bar:/xyzzy or
GIT_CEILING_DIRECTORIES=/foo/bar::/xyzzy
we will not readlink("/xyzzy"), and with the former, we will not
readlink("/foo/bar"), either.
---
Documentation/git.txt | 19 +++++++++++++------
setup.c | 32 ++++++++++++++++++++++----------
t/t1504-ceiling-dirs.sh | 17 +++++++++++++++++
3 files changed, 52 insertions(+), 16 deletions(-)
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6710cb0..5c03616 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -653,12 +653,19 @@ git so take care if using Cogito etc.
The '--namespace' command-line option also sets this value.
'GIT_CEILING_DIRECTORIES'::
- This should be a colon-separated list of absolute paths.
- If set, it is a list of directories that git should not chdir
- up into while looking for a repository directory.
- It will not exclude the current working directory or
- a GIT_DIR set on the command line or in the environment.
- (Useful for excluding slow-loading network directories.)
+ This should be a colon-separated list of absolute paths. If
+ set, it is a list of directories that git should not chdir up
+ into while looking for a repository directory (useful for
+ excluding slow-loading network directories). It will not
+ exclude the current working directory or a GIT_DIR set on the
+ command line or in the environment. Normally, Git has to read
+ the entries in this list are read to resolve any symlinks that
+ might be present in order to compare them with the current
+ directory. However, if even this access is slow, you
+ can add an empty entry to the list to tell Git that the
+ subsequent entries are not symlinks and needn't be resolved;
+ e.g.,
+ 'GIT_CEILING_DIRECTORIES=/maybe/symlink::/very/slow/non/symlink'.
'GIT_DISCOVERY_ACROSS_FILESYSTEM'::
When run in a directory that does not have ".git" repository
diff --git a/setup.c b/setup.c
index f108c4b..1b12017 100644
--- a/setup.c
+++ b/setup.c
@@ -624,22 +624,32 @@ static dev_t get_device_or_die(const char *path, const char *prefix, int prefix_
/*
* 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.
+ * 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 *unused)
+ void *cb_data)
{
+ int *empty_entry_found = cb_data;
char *ceil = item->string;
- const char *real_path;
- if (!*ceil || !is_absolute_path(ceil))
+ if (!*ceil) {
+ *empty_entry_found = 1;
return 0;
- real_path = real_path_if_valid(ceil);
- if (!real_path)
+ } else if (!is_absolute_path(ceil)) {
return 0;
- free(item->string);
- item->string = xstrdup(real_path);
- return 1;
+ } 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;
+ }
}
/*
@@ -679,9 +689,11 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
return setup_explicit_git_dir(gitdirenv, cwd, len, nongit_ok);
if (env_ceiling_dirs) {
+ 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, NULL);
+ canonicalize_ceiling_entry, &empty_entry_found);
ceil_offset = longest_ancestor_length(cwd, &ceiling_dirs);
string_list_clear(&ceiling_dirs, 0);
}
diff --git a/t/t1504-ceiling-dirs.sh b/t/t1504-ceiling-dirs.sh
index cce87a5..3d51615 100755
--- a/t/t1504-ceiling-dirs.sh
+++ b/t/t1504-ceiling-dirs.sh
@@ -44,6 +44,10 @@ test_prefix ceil_at_sub ""
GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/"
test_prefix ceil_at_sub_slash ""
+if test_have_prereq SYMLINKS
+then
+ ln -s sub top
+fi
mkdir -p sub/dir || exit 1
cd sub/dir || exit 1
@@ -68,6 +72,19 @@ test_fail subdir_ceil_at_sub
GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/"
test_fail subdir_ceil_at_sub_slash
+if test_have_prereq SYMLINKS
+then
+ GIT_CEILING_DIRECTORIES="$TRASH_ROOT/top"
+ test_fail subdir_ceil_at_top
+ GIT_CEILING_DIRECTORIES="$TRASH_ROOT/top/"
+ test_fail subdir_ceil_at_top_slash
+
+ GIT_CEILING_DIRECTORIES=":$TRASH_ROOT/top"
+ test_prefix subdir_ceil_at_top_no_resolve "sub/dir/"
+ GIT_CEILING_DIRECTORIES=":$TRASH_ROOT/top/"
+ test_prefix subdir_ceil_at_top_slash_no_resolve "sub/dir/"
+fi
+
GIT_CEILING_DIRECTORIES="$TRASH_ROOT/sub/dir"
test_prefix subdir_ceil_at_subdir "sub/dir/"
--
1.8.2.rc0.152.g52dbac4
next prev parent reply other threads:[~2013-02-21 22:54 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-21 5:57 [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks Michael Haggerty
2012-10-21 5:57 ` [PATCH v3 1/8] Introduce new static function real_path_internal() Michael Haggerty
2012-10-21 5:57 ` [PATCH v3 2/8] real_path_internal(): add comment explaining use of cwd Michael Haggerty
2012-10-21 5:57 ` [PATCH v3 3/8] Introduce new function real_path_if_valid() Michael Haggerty
2012-10-21 5:57 ` [PATCH v3 4/8] longest_ancestor_length(): use string_list_split() Michael Haggerty
2012-10-21 5:57 ` [PATCH v3 5/8] longest_ancestor_length(): take a string_list argument for prefixes Michael Haggerty
2012-10-21 5:57 ` [PATCH v3 6/8] longest_ancestor_length(): require prefix list entries to be normalized Michael Haggerty
2012-10-22 20:04 ` Johannes Sixt
2012-10-21 5:57 ` [PATCH v3 7/8] normalize_ceiling_entry(): resolve symlinks Michael Haggerty
2012-10-21 5:57 ` [PATCH v3 8/8] string_list_longest_prefix(): remove function Michael Haggerty
2012-10-21 6:51 ` [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks Junio C Hamano
2012-10-22 8:26 ` Michael Haggerty
2012-10-29 0:15 ` David Aguilar
2012-10-29 1:42 ` Junio C Hamano
2012-10-29 5:10 ` Michael Haggerty
2012-11-12 17:47 ` Junio C Hamano
2012-11-13 20:50 ` David Aguilar
2012-11-15 8:18 ` Michael Haggerty
2013-02-20 6:20 ` Anders Kaseorg
2013-02-20 6:55 ` Junio C Hamano
2013-02-20 9:09 ` [RFC] Provide a mechanism to turn off symlink resolution in ceiling paths Michael Haggerty
2013-02-20 17:41 ` Junio C Hamano
2013-02-21 22:53 ` Junio C Hamano [this message]
2013-02-22 7:23 ` Michael Haggerty
2013-02-20 9:39 ` [PATCH v3 0/8] Fix GIT_CEILING_DIRECTORIES that contain symlinks Anders Kaseorg
2012-10-29 5:34 ` Lars Damerow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vk3q1th1l.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=andersk@MIT.EDU \
--cc=davvid@gmail.com \
--cc=dreiss@facebook.com \
--cc=git@vger.kernel.org \
--cc=j6t@kdbg.org \
--cc=lars@pixar.com \
--cc=lewiemann@gmail.com \
--cc=marc.jordan@disneyanimation.com \
--cc=mhagger@alum.mit.edu \
--cc=peff@peff.net \
--cc=worldhello.net@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).