[PATCH] Add support for GIT_ONE_FILESYSTEM

[PATCH] Add support for GIT_ONE_FILESYSTEM

[Lars Damerow]

This patch makes git pay attention to the GIT_ONE_FILESYSTEM environment
variable. When that variable is set, git will stop searching for a
GIT_DIR when it attempts to cross a filesystem boundary.

When working in an environment with too many automount points to make
maintaining a GIT_CEILING_DIRECTORIES list enjoyable, GIT_ONE_FILESYSTEM
gives the option of turning all such attempts off with one setting.

Signed-off-by: Lars R. Damerow <lars@pixar.com>
---
 Documentation/git.txt |    3 +++
 setup.c               |   11 +++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 35c0c79..dbb590f 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -529,6 +529,9 @@ git so take care if using Cogito etc.
 	a GIT_DIR set on the command line or in the environment.
 	(Useful for excluding slow-loading network directories.)
 
+'GIT_ONE_FILESYSTEM'::
+	Stop at filesystem boundaries when looking for .git or objects.
+
 git Commits
 ~~~~~~~~~~~
 'GIT_AUTHOR_NAME'::
diff --git a/setup.c b/setup.c
index 5716d90..63dc52e 100644
--- a/setup.c
+++ b/setup.c
@@ -323,6 +323,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	const char *gitdirenv;
 	const char *gitfile_dir;
 	int len, offset, ceil_offset, root_len;
+	int current_device = 0;
+	struct stat buf;
 
 	/*
 	 * Let's assume that we are in a git repository.
@@ -390,6 +392,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 *   etc.
 	 */
 	offset = len = strlen(cwd);
+	if (stat(".", &buf))
+		die_errno("failed to stat '.'");
+	current_device = buf.st_dev;
 	for (;;) {
 		gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
 		if (gitfile_dir) {
@@ -422,6 +427,12 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			}
 			die("Not a git repository (or any of the parent directories): %s", DEFAULT_GIT_DIR_ENVIRONMENT);
 		}
+		if (getenv("GIT_ONE_FILESYSTEM") != NULL) {
+			if (stat("..", &buf))
+				die_errno("failed to stat '..'");
+			if (buf.st_dev != current_device)
+				die("refusing to cross filesystem boundary '%s/..'", cwd);
+		}
 		if (chdir(".."))
 			die_errno("Cannot change to '%s/..'", cwd);
 	}
-- 
1.6.5.2

Re: [PATCH] Add support for GIT_ONE_FILESYSTEM

[Sverre Rabbelier]

Heya,

On Mon, Mar 15, 2010 at 22:40, Lars Damerow <lars@pixar.com> wrote:
> +                       if (buf.st_dev != current_device)
> +                               die("refusing to cross filesystem boundary '%s/..'", cwd);

Great, that's going to tell the unfortunate user who runs into this a
whole lot of nothing. Shouldn't you at least tell them what git was
trying to do, how they can fix it if this is not the desired behavior,
etc. etc.?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Add support for GIT_ONE_FILESYSTEM

[Lars Damerow]

>From Sverre Rabbelier <srabbelier@gmail.com>, Tue, Mar 16, 2010 at 12:16:19AM +0100:
> Heya,
> 
> On Mon, Mar 15, 2010 at 22:40, Lars Damerow <lars@pixar.com> wrote:
> > if (buf.st_dev != current_device)
> >     die("refusing to cross filesystem boundary '%s/..'", cwd);
> 
> Great, that's going to tell the unfortunate user who runs into this a
> whole lot of nothing. Shouldn't you at least tell them what git was
> trying to do, how they can fix it if this is not the desired behavior,
> etc. etc.?

Hi Sverre,

You're right, that's a badly-worded error. How about this?

    Not a git repository (or any parent up to /path/to/mountpoint)
    Stopping at filesystem boundary since GIT_ONE_FILESYSTEM is set.

-lars

--
lars r. damerow :: button pusher :: pixar animation studios

Science Factoid:  The common cockroach shares 99.5 percent of its DNA code
                  with e-mail spammers and telemarketers.

Re: [PATCH] Add support for GIT_ONE_FILESYSTEM

[Jeff King]

On Mon, Mar 15, 2010 at 02:40:03PM -0700, Lars Damerow wrote:

> +		if (getenv("GIT_ONE_FILESYSTEM") != NULL) {

Should this really trigger for GIT_ONE_FILESYSTEM=0? We already have
git_env_bool, which will handle 0/1, true/false, etc. Probably you
should use it here.

I am not a big fan of the environment variable name, either, but I don't
have another good suggestion. It is closely related to
GIT_CEILING_DIRECTORIES (in fact, you could probably solve the same
problem with GIT_CEILING_DIRECTORIES, but I think your solution is much
nicer in that it lets the user get away with being less verbose).

> +			if (stat("..", &buf))
> +				die_errno("failed to stat '..'");
> +			if (buf.st_dev != current_device)
> +				die("refusing to cross filesystem boundary '%s/..'", cwd);
> +		}

I agree with Sverre that this message isn't descriptive enough, but I
like the suggestion you posted elsewhere in the thread.

-Peff

Re: [PATCH] Add support for GIT_ONE_FILESYSTEM

[Lars Damerow]

>From Jeff King <peff@peff.net>, Mon, Mar 15, 2010 at 10:33:06PM -0400:
> On Mon, Mar 15, 2010 at 02:40:03PM -0700, Lars Damerow wrote:
> 
> > +		if (getenv("GIT_ONE_FILESYSTEM") != NULL) {
> 
> Should this really trigger for GIT_ONE_FILESYSTEM=0? We already have
> git_env_bool, which will handle 0/1, true/false, etc. Probably you
> should use it here.

Thanks for the pointer--I wasn't aware of git_env_bool. I'll use it
instead.

> I am not a big fan of the environment variable name, either, but I don't
> have another good suggestion. It is closely related to
> GIT_CEILING_DIRECTORIES (in fact, you could probably solve the same
> problem with GIT_CEILING_DIRECTORIES, but I think your solution is much
> nicer in that it lets the user get away with being less verbose).

I was thinking about cp's -x option and its description in the man page
when I chose GIT_ONE_FILESYSTEM. Before I wrote this patch I configured
my machines to have their most popular automount points in
GIT_CEILING_DIRECTORIES, but need a more complete solution and don't
want to have a cron job running to keep the list up to date.

Do the overall idea and implementation have enough merit that the patch
could get accepted?

thanks,
-lars

--
lars r. damerow :: button pusher :: pixar animation studios

The big print giveth and the small print taketh away.

Re: [PATCH] Add support for GIT_ONE_FILESYSTEM

[Jeff King]

On Mon, Mar 15, 2010 at 08:28:58PM -0700, Lars Damerow wrote:

> I was thinking about cp's -x option and its description in the man page
> when I chose GIT_ONE_FILESYSTEM. Before I wrote this patch I configured

Ah, I always just used "-x" and didn't know about "--one-file-system".
So in that way, it makes sense.

> Do the overall idea and implementation have enough merit that the patch
> could get accepted?

Yeah, I think it is an improvement overall.

-Peff

Re: [PATCH] Add support for GIT_ONE_FILESYSTEM

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 15, 2010 at 02:40:03PM -0700, Lars Damerow wrote:
>
>> +		if (getenv("GIT_ONE_FILESYSTEM") != NULL) {
>
> Should this really trigger for GIT_ONE_FILESYSTEM=0? We already have
> git_env_bool, which will handle 0/1, true/false, etc. Probably you
> should use it here.
>
> I am not a big fan of the environment variable name, either, but I don't
> have another good suggestion. It is closely related to
> GIT_CEILING_DIRECTORIES (in fact, you could probably solve the same
> problem with GIT_CEILING_DIRECTORIES, but I think your solution is much
> nicer in that it lets the user get away with being less verbose).

Yeah, that, or toplevel global option --one-file-system given to "git",
but I think that is probably too cumbersome.

How inefficient is it to repeatedly call getenv() in a loop, by the way?
I think the patch would become easier to read if env_bool() is called once
at the beginning to set a local variable, and not even do the initial stat()
for "." if this feature is not asked for.

Re: [PATCH] Add support for GIT_ONE_FILESYSTEM

[Lars Damerow]

>From Junio C Hamano <gitster@pobox.com>, Mon, Mar 15, 2010 at 09:19:17PM -0700:
> 
> How inefficient is it to repeatedly call getenv() in a loop, by the way?
> I think the patch would become easier to read if env_bool() is called once
> at the beginning to set a local variable, and not even do the initial stat()
> for "." if this feature is not asked for.

Agreed, I didn't do this too efficiently. Thanks for the feedback! I'll
make a new patch and resubmit.

cheers,
-lars

--
lars r. damerow :: button pusher :: pixar animation studios

When in doubt, use brute force.