git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Add support for GIT_ONE_FILESYSTEM
@ 2010-03-17 19:55 Lars R. Damerow
  2010-03-17 19:55 ` [PATCH 1/3] config.c: remove static keyword from git_env_bool() Lars R. Damerow
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Lars R. Damerow @ 2010-03-17 19:55 UTC (permalink / raw)
  To: git


Here's another try for GIT_ONE_FILESYSTEM.

I took Duy's recommendation not to die() in gentle mode as well as to just use
cwd in my error messages (after truncating them). I also fixed the problem
with the existing error message's use of an untruncated cwd.

Thanks for all of the suggestions!

Lars R. Damerow (3):
      config.c: remove static keyword from git_env_bool()
      truncate cwd string before printing error message
      Add support for GIT_ONE_FILESYSTEM

 Documentation/git.txt |    3 +++
 cache.h               |    1 +
 config.c              |    2 +-
 setup.c               |   28 +++++++++++++++++++++++++++-
 4 files changed, 32 insertions(+), 2 deletions(-)

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/3] config.c: remove static keyword from git_env_bool()
  2010-03-17 19:55 [PATCH v3 0/3] Add support for GIT_ONE_FILESYSTEM Lars R. Damerow
@ 2010-03-17 19:55 ` Lars R. Damerow
  2010-03-17 19:55 ` [PATCH 2/3] truncate cwd string before printing error message Lars R. Damerow
  2010-03-17 19:55 ` [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM Lars R. Damerow
  2 siblings, 0 replies; 20+ messages in thread
From: Lars R. Damerow @ 2010-03-17 19:55 UTC (permalink / raw)
  To: git

Since this function is the preferred way to handle boolean environment
variables it's useful to have it available to other files.

Signed-off-by: Lars R. Damerow <lars@pixar.com>
---
 cache.h  |    1 +
 config.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index 89f6a40..c29030f 100644
--- a/cache.h
+++ b/cache.h
@@ -945,6 +945,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int
 extern int git_config_rename_section(const char *, const char *);
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value, void *cb);
+extern int git_env_bool(const char *, int);
 extern int git_config_system(void);
 extern int git_config_global(void);
 extern int config_error_nonbool(const char *);
diff --git a/config.c b/config.c
index 6963fbe..70e4600 100644
--- a/config.c
+++ b/config.c
@@ -683,7 +683,7 @@ const char *git_etc_gitconfig(void)
 	return system_wide;
 }
 
-static int git_env_bool(const char *k, int def)
+int git_env_bool(const char *k, int def)
 {
 	const char *v = getenv(k);
 	return v ? git_config_bool(k, v) : def;
-- 
1.6.5.2

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/3] truncate cwd string before printing error message
  2010-03-17 19:55 [PATCH v3 0/3] Add support for GIT_ONE_FILESYSTEM Lars R. Damerow
  2010-03-17 19:55 ` [PATCH 1/3] config.c: remove static keyword from git_env_bool() Lars R. Damerow
@ 2010-03-17 19:55 ` Lars R. Damerow
  2010-03-17 19:55 ` [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM Lars R. Damerow
  2 siblings, 0 replies; 20+ messages in thread
From: Lars R. Damerow @ 2010-03-17 19:55 UTC (permalink / raw)
  To: git

Without this truncation the error message printed only shows the cwd
from the start of the search, not where it failed.

Signed-off-by: Lars R. Damerow <lars@pixar.com>
---
 setup.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/setup.c b/setup.c
index 5716d90..f0b56b9 100644
--- a/setup.c
+++ b/setup.c
@@ -422,8 +422,10 @@ 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 (chdir(".."))
+		if (chdir("..")) {
+			cwd[offset] = '\0';
 			die_errno("Cannot change to '%s/..'", cwd);
+		}
 	}
 
 	inside_git_dir = 0;
-- 
1.6.5.2

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-17 19:55 [PATCH v3 0/3] Add support for GIT_ONE_FILESYSTEM Lars R. Damerow
  2010-03-17 19:55 ` [PATCH 1/3] config.c: remove static keyword from git_env_bool() Lars R. Damerow
  2010-03-17 19:55 ` [PATCH 2/3] truncate cwd string before printing error message Lars R. Damerow
@ 2010-03-17 19:55 ` Lars R. Damerow
  2010-03-28  9:22   ` Jeff King
  2 siblings, 1 reply; 20+ messages in thread
From: Lars R. Damerow @ 2010-03-17 19:55 UTC (permalink / raw)
  To: git

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               |   24 ++++++++++++++++++++++++
 2 files changed, 27 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 f0b56b9..ef23e79 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, one_filesystem = 0;
+	struct stat buf;
 
 	/*
 	 * Let's assume that we are in a git repository.
@@ -390,6 +392,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 *   etc.
 	 */
 	offset = len = strlen(cwd);
+	if ((one_filesystem = git_env_bool("GIT_ONE_FILESYSTEM", 0))) {
+		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 +429,23 @@ 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 (one_filesystem) {
+			if (stat("..", &buf)) {
+				cwd[offset] = '\0';
+				die_errno("failed to stat '%s/..'", cwd);
+			}
+			if (buf.st_dev != current_device) {
+				if (nongit_ok) {
+					if (chdir(cwd))
+						die_errno("Cannot come back to cwd");
+					*nongit_ok = 1;
+					return NULL;
+				}
+				cwd[offset] = '\0';
+				die("Not a git repository (or any parent up to mount parent %s)\n"
+					"Stopping at filesystem boundary since GIT_ONE_FILESYSTEM is set.", cwd);
+			}
+		}
 		if (chdir("..")) {
 			cwd[offset] = '\0';
 			die_errno("Cannot change to '%s/..'", cwd);
-- 
1.6.5.2

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-17 19:55 ` [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM Lars R. Damerow
@ 2010-03-28  9:22   ` Jeff King
  2010-03-28 12:05     ` Jeff King
  2010-03-28 16:44     ` Junio C Hamano
  0 siblings, 2 replies; 20+ messages in thread
From: Jeff King @ 2010-03-28  9:22 UTC (permalink / raw)
  To: Lars R. Damerow; +Cc: git

On Wed, Mar 17, 2010 at 12:55:53PM -0700, Lars R. Damerow wrote:

> 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.

Thanks, this version (and the whole series) looks good to me, with two
minor nits below.

As far as a command-line option or a config option, I don't see the
point. It seems to me something you would want enabled all the time, not
a one-shot option, so a command-line option doesn't make much sense
(after all, you could just use --git-dir at that point, which would be
even faster). A config option might be useful, but I think it might be
tricky to implement due to the startup sequence.  If anybody disagrees,
they can always build on top of your series very easily.

> 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.
> +

We should give the user a little more information than that.
Specifically:

  - what form should the value take?

  - how does it interact with GIT_DIR?

Also, we use the phrase "repository directory" in
GIT_CEILING_DIRECTORIES, so we should probably do the same here.

So how about this squashed into your patch?

diff --git a/Documentation/git.txt b/Documentation/git.txt
index e7ebaaa..bf1b45e 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -531,7 +531,10 @@ git so take care if using Cogito etc.
 	(Useful for excluding slow-loading network directories.)
 
 'GIT_ONE_FILESYSTEM'::
-	Stop at filesystem boundaries when looking for .git or objects.
+	If set to a true value ("true" or a non-zero integer), stop at
+	filesystem boundaries when looking for a repository directory.
+	Like 'GIT_CEILING_DIRECTORIES', it will not affect an explicit
+	respository directory set via 'GIT_DIR' or on the command line.
 
 git Commits
 ~~~~~~~~~~~

> --- a/setup.c
> +++ b/setup.c
> @@ -390,6 +392,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  	 *   etc.
>  	 */
>  	offset = len = strlen(cwd);
> +	if ((one_filesystem = git_env_bool("GIT_ONE_FILESYSTEM", 0))) {
> +		if (stat(".", &buf))
> +			die_errno("failed to stat '.'");
> +		current_device = buf.st_dev;
> +	}

Style nit. I know you did the proper () to silence the compiler warning,
but we usually avoid assignment-inside-conditional altogether.
Personally I don't really care either way.

With those fixes, I think it should be ready for 'next'.

-Peff

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-28  9:22   ` Jeff King
@ 2010-03-28 12:05     ` Jeff King
  2010-03-28 16:44     ` Junio C Hamano
  1 sibling, 0 replies; 20+ messages in thread
From: Jeff King @ 2010-03-28 12:05 UTC (permalink / raw)
  To: Lars R. Damerow; +Cc: git

On Sun, Mar 28, 2010 at 05:22:54AM -0400, Jeff King wrote:

> > --- a/setup.c
> > +++ b/setup.c
> > @@ -390,6 +392,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
> >  	 *   etc.
> >  	 */
> >  	offset = len = strlen(cwd);
> > +	if ((one_filesystem = git_env_bool("GIT_ONE_FILESYSTEM", 0))) {
> > +		if (stat(".", &buf))
> > +			die_errno("failed to stat '.'");
> > +		current_device = buf.st_dev;
> > +	}
> 
> Style nit. I know you did the proper () to silence the compiler warning,
> but we usually avoid assignment-inside-conditional altogether.
> Personally I don't really care either way.

Actually I take this back. It is in our CodingStyle guide, but we
violate it all over the place (my grep counted 522). Perhaps it should
be removed from CodingStyle.

-Peff

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-28  9:22   ` Jeff King
  2010-03-28 12:05     ` Jeff King
@ 2010-03-28 16:44     ` Junio C Hamano
  2010-03-28 17:32       ` Lars Damerow
  2010-03-30 22:43       ` Linus Torvalds
  1 sibling, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2010-03-28 16:44 UTC (permalink / raw)
  To: Jeff King; +Cc: Lars R. Damerow, git

Jeff King <peff@peff.net> writes:

> With those fixes, I think it should be ready for 'next'.

Yeah, looks nice; thanks both.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-28 16:44     ` Junio C Hamano
@ 2010-03-28 17:32       ` Lars Damerow
  2010-03-30 15:58         ` Jeff King
  2010-03-30 22:43       ` Linus Torvalds
  1 sibling, 1 reply; 20+ messages in thread
From: Lars Damerow @ 2010-03-28 17:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git

>From Junio C Hamano <gitster@pobox.com>, Sun, Mar 28, 2010 at 09:44:09AM -0700:
> Jeff King <peff@peff.net> writes:
> 
> > With those fixes, I think it should be ready for 'next'.
> 
> Yeah, looks nice; thanks both.

Thanks all for the suggestions! Jeff, shall I just squash in the doc
patch you sent and resubmit the patches to the list?

-lars

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-28 17:32       ` Lars Damerow
@ 2010-03-30 15:58         ` Jeff King
  0 siblings, 0 replies; 20+ messages in thread
From: Jeff King @ 2010-03-30 15:58 UTC (permalink / raw)
  To: Lars Damerow; +Cc: Junio C Hamano, git

On Sun, Mar 28, 2010 at 10:32:25AM -0700, Lars Damerow wrote:

> > > With those fixes, I think it should be ready for 'next'.
> > 
> > Yeah, looks nice; thanks both.
> 
> Thanks all for the suggestions! Jeff, shall I just squash in the doc
> patch you sent and resubmit the patches to the list?

No need. Usually Junio's "thanks both" means he picked up the patches
and squashed it himself, and indeed, it looks like he has your patches
(with my doc change) in his 'pu' branch.

-Peff

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-28 16:44     ` Junio C Hamano
  2010-03-28 17:32       ` Lars Damerow
@ 2010-03-30 22:43       ` Linus Torvalds
  2010-03-30 22:59         ` Thomas Rast
  2010-03-30 23:02         ` Jeff King
  1 sibling, 2 replies; 20+ messages in thread
From: Linus Torvalds @ 2010-03-30 22:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, Lars R. Damerow, git



On Sun, 28 Mar 2010, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > With those fixes, I think it should be ready for 'next'.
> 
> Yeah, looks nice; thanks both.

I realize that I'm late to the party, but I do wonder if the "one 
filesystem" mode shouldn't be the default, rather than be enabled by a 
config option? IOW, just switch the meaning of the config option the other 
way.

I suspect that it is _very_ unusual to have a source repo that crosses 
multiple filesystems, and the original reason for this patch-series seems 
to me to be likely to be more common than that multi-fs case. So having 
the logic go the other way would seem to match the common case, no?

			Linus

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-30 22:43       ` Linus Torvalds
@ 2010-03-30 22:59         ` Thomas Rast
  2010-03-30 23:04           ` Jeff King
  2010-03-30 23:02         ` Jeff King
  1 sibling, 1 reply; 20+ messages in thread
From: Thomas Rast @ 2010-03-30 22:59 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Junio C Hamano, Jeff King, Lars R. Damerow, git

Linus Torvalds wrote:
> 
> I suspect that it is _very_ unusual to have a source repo that crosses 
> multiple filesystems, and the original reason for this patch-series seems 
> to me to be likely to be more common than that multi-fs case. So having 
> the logic go the other way would seem to match the common case, no?

Not sure if I'm the only one, but I noticed at some point that
mounting the t/ directory of git.git on tmpfs gives a huge speed boost
to the test suite...

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-30 22:43       ` Linus Torvalds
  2010-03-30 22:59         ` Thomas Rast
@ 2010-03-30 23:02         ` Jeff King
  2010-03-30 23:10           ` Junio C Hamano
  2010-03-30 23:12           ` Linus Torvalds
  1 sibling, 2 replies; 20+ messages in thread
From: Jeff King @ 2010-03-30 23:02 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Junio C Hamano, Lars R. Damerow, git

On Tue, Mar 30, 2010 at 03:43:01PM -0700, Linus Torvalds wrote:

> I realize that I'm late to the party, but I do wonder if the "one 
> filesystem" mode shouldn't be the default, rather than be enabled by a 
> config option? IOW, just switch the meaning of the config option the other 
> way.

Fashionably late, of course. I agree with your reasoning that it is
a more sane default. The only thing that would make me hesitate on it
now is that it is a behavior change. I suspect the group we would be
breaking is small or even zero, though.

-Peff

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-30 22:59         ` Thomas Rast
@ 2010-03-30 23:04           ` Jeff King
  0 siblings, 0 replies; 20+ messages in thread
From: Jeff King @ 2010-03-30 23:04 UTC (permalink / raw)
  To: Thomas Rast; +Cc: Linus Torvalds, Junio C Hamano, Lars R. Damerow, git

On Wed, Mar 31, 2010 at 12:59:33AM +0200, Thomas Rast wrote:

> Linus Torvalds wrote:
> > 
> > I suspect that it is _very_ unusual to have a source repo that crosses 
> > multiple filesystems, and the original reason for this patch-series seems 
> > to me to be likely to be more common than that multi-fs case. So having 
> > the logic go the other way would seem to match the common case, no?
> 
> Not sure if I'm the only one, but I noticed at some point that
> mounting the t/ directory of git.git on tmpfs gives a huge speed boost
> to the test suite...

I noticed it, too, but my solution was a little different:

  $ git show f423ef5f
    tests: allow user to specify trash directory location

    The tests generate a large amount of I/O activity creating
    and destroying repositories and files. We can improve the
    time it takes to run the test suite by creating trash
    directories on filesystems with better performance
    characteristic, even though we may not want the rest of the
    git repository on those filesystems (e.g., because they are
    not network connected, or because they are temporary
    ramdisks).

    For example, on a dual processor system:

      $ cd t && time make -j32
      real    1m51.562s
      user    0m59.260s
      sys     1m20.933s

      # /dev/shm is tmpfs
      $ cd t && time make -j32 GIT_TEST_OPTS="--root=/dev/shm"
      real    1m1.484s
      user    0m53.555s
      sys     1m5.264s

-Peff

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-30 23:02         ` Jeff King
@ 2010-03-30 23:10           ` Junio C Hamano
  2010-03-30 23:12           ` Linus Torvalds
  1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2010-03-30 23:10 UTC (permalink / raw)
  To: Jeff King; +Cc: Linus Torvalds, Junio C Hamano, Lars R. Damerow, git

Jeff King <peff@peff.net> writes:

> On Tue, Mar 30, 2010 at 03:43:01PM -0700, Linus Torvalds wrote:
>
>> I realize that I'm late to the party, but I do wonder if the "one 
>> filesystem" mode shouldn't be the default, rather than be enabled by a 
>> config option? IOW, just switch the meaning of the config option the other 
>> way.
>
> Fashionably late, of course. I agree with your reasoning that it is
> a more sane default. The only thing that would make me hesitate on it
> now is that it is a behavior change. I suspect the group we would be
> breaking is small or even zero, though.

Yeah, I think I agree with the reasoning, and it is not even _late_, as
the feature hasn't escaped the lab yet.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-30 23:02         ` Jeff King
  2010-03-30 23:10           ` Junio C Hamano
@ 2010-03-30 23:12           ` Linus Torvalds
  2010-03-31  0:54             ` Erik Faye-Lund
  1 sibling, 1 reply; 20+ messages in thread
From: Linus Torvalds @ 2010-03-30 23:12 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Lars R. Damerow, git



On Tue, 30 Mar 2010, Jeff King wrote:
> 
> Fashionably late, of course. I agree with your reasoning that it is
> a more sane default. The only thing that would make me hesitate on it
> now is that it is a behavior change. I suspect the group we would be
> breaking is small or even zero, though.

Well, I have to admit that I'm a _tiny_ bit nervous that some odd OS or 
filesystem has special magic st_dev rules so that it changes randomly even 
within a filesystem, but that would be very non-posix (think of the 
confusion it would cause standard UNIX tools like 'find -xdev' etc), so 
it's more a worry of "I have no idea what st_dev means on Windows" than 
anything really solid.

			Linus

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM
  2010-03-30 23:12           ` Linus Torvalds
@ 2010-03-31  0:54             ` Erik Faye-Lund
  2010-04-04 18:00               ` [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries Junio C Hamano
  0 siblings, 1 reply; 20+ messages in thread
From: Erik Faye-Lund @ 2010-03-31  0:54 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Jeff King, Junio C Hamano, Lars R. Damerow, git

On Wed, Mar 31, 2010 at 1:12 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Tue, 30 Mar 2010, Jeff King wrote:
>>
>> Fashionably late, of course. I agree with your reasoning that it is
>> a more sane default. The only thing that would make me hesitate on it
>> now is that it is a behavior change. I suspect the group we would be
>> breaking is small or even zero, though.
>
> Well, I have to admit that I'm a _tiny_ bit nervous that some odd OS or
> filesystem has special magic st_dev rules so that it changes randomly even
> within a filesystem, but that would be very non-posix (think of the
> confusion it would cause standard UNIX tools like 'find -xdev' etc), so
> it's more a worry of "I have no idea what st_dev means on Windows" than
> anything really solid.
>

st_dev means "Drive number of the disk containing the file (same as
st_rdev)."[1], and mounting file systems as subdirs of other file
systems is rare (but possible) on Windows.

However, in our (f)stat implementation, st_dev means 0 -- but this
might be possible to improve on.

[1]: http://msdn.microsoft.com/en-us/library/14h5k7ff(VS.71).aspx

-- 
Erik "kusma" Faye-Lund

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries
  2010-03-31  0:54             ` Erik Faye-Lund
@ 2010-04-04 18:00               ` Junio C Hamano
  2010-04-04 22:30                 ` [PATCH] Rename ONE_FILESYSTEM to DISCOVERY_ACROSS_FILESYSTEM Junio C Hamano
  2010-04-04 22:39                 ` [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries Junio C Hamano
  0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2010-04-04 18:00 UTC (permalink / raw)
  To: Lars R. Damerow; +Cc: Linus Torvalds, Jeff King, Erik Faye-Lund, git

Regarding the new environment variable, Linus Torvalds
<torvalds@linux-foundation.org> writes on Tue, 30 Mar 2010 in
<alpine.LFD.2.00.1003301537150.3707@i5.linux-foundation.org>:

    I suspect that it is _very_ unusual to have a source repo that crosses
    multiple filesystems, and the original reason for this patch-series
    seems to me to be likely to be more common than that multi-fs case. So
    having the logic go the other way would seem to match the common case,
    no?

The "crossing filesystem boundary" condition is checked by comparing
st_dev field in the result from stat(2).  This is slightly worrysome if
non-POSIX ports return different values in the field even for directories
in the same work tree extracted to the same "filesystem".  Erik Faye-Lund
confirms that in the msysgit port st_dev is 0, so this should be safe, as
"even Windows is safe" ;-)

This will affect those who use /.git to cram /etc and /home/me in the same
repostiory, /home is mounted from non-root filesystem, and a git operation
is done from inside /home/me/src.  But that is such a corner case we don't
want to give preference over helping people who will benefit from having
this default so that they do not have to suffer from slow automounters.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I agree with Linus that it make sense to flip the default, but this
   should probably have to wait for at least two release cycles for the
   usual backward-compatibility rules.

   I wonder if "git add" and friends should also notice it and warn.  If
   you have more than one values of ce->ce_dev in the index, it means that
   the working tree spans more than one filesystem and from a subdirectory
   with an entry that has a ce->ce_dev different from the value for a path
   at the top of the work tree, you will not be able to discover the top
   of the tree without GIT_ONE_FILESYSTEM set to true.  A likely scenario
   for this to happen would be:

    (1) You have a tarball of some sort; you extract it $there;

	$ mkdir $there && cd $there
        $ tar xf /var/tmp/tarball.tar

    (2) You notice the filesystem lacks enough free space, and move some
        part (say "images/") to a separate filesystem, and bind-mount;

        $ mv images $another/. && rm -fr images && mkdir images
        $ mount --bind $another/images images

    (3) You add everything to start the project;

        $ git init && git add .

        Up to this point it would work (you are at the top of the working
        tree).  And this is the point we _could_ notice and warn that you
        will have trouble in step (4).

    (4) Go down to a subdirectory and start futzing;

	$ cd images && gimp naughty.jpg && git add -u

 Documentation/git.txt |   12 ++++++++----
 setup.c               |    4 ++--
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index bf1b45e..aa62083 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -531,10 +531,14 @@ git so take care if using Cogito etc.
 	(Useful for excluding slow-loading network directories.)
 
 'GIT_ONE_FILESYSTEM'::
-	If set to a true value ("true" or a non-zero integer), stop at
-	filesystem boundaries when looking for a repository directory.
-	Like 'GIT_CEILING_DIRECTORIES', it will not affect an explicit
-	respository directory set via 'GIT_DIR' or on the command line.
+	When run in a directory that does not have ".git" repository
+	directory, git tries to find such a directory in the parent
+	directories to find the top of the working tree, but by default it
+	does not cross filesystem boundaries.  This environment variable
+	can be set to false value ("false" or zero) to tell git not to
+	stop at filesystem boundaries.  Like 'GIT_CEILING_DIRECTORIES',
+	this will not affect an explicit respository directory set via
+	'GIT_DIR' or on the command line.
 
 git Commits
 ~~~~~~~~~~~
diff --git a/setup.c b/setup.c
index 8b911b1..d290633 100644
--- a/setup.c
+++ b/setup.c
@@ -323,7 +323,7 @@ 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, one_filesystem = 0;
+	int current_device = 0, one_filesystem = 1;
 	struct stat buf;
 
 	/*
@@ -444,7 +444,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 				}
 				cwd[offset] = '\0';
 				die("Not a git repository (or any parent up to mount parent %s)\n"
-					"Stopping at filesystem boundary since GIT_ONE_FILESYSTEM is set.", cwd);
+				"Stopping at filesystem boundary since GIT_ONE_FILESYSTEM is true.", cwd);
 			}
 		}
 		if (chdir("..")) {
-- 
1.7.0.4.552.gc303c1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH] Rename ONE_FILESYSTEM to DISCOVERY_ACROSS_FILESYSTEM
  2010-04-04 18:00               ` [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries Junio C Hamano
@ 2010-04-04 22:30                 ` Junio C Hamano
  2010-04-04 22:37                   ` Matthieu Moy
  2010-04-04 22:39                 ` [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries Junio C Hamano
  1 sibling, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2010-04-04 22:30 UTC (permalink / raw)
  To: Lars R. Damerow; +Cc: Linus Torvalds, Jeff King, Erik Faye-Lund, git

If a missing ONE_FILESYSTEM defaults to true, the only users who set this
variable set it to false to tell git not to limit the discovery to one
filesystem; there are too many negations in one sentence to make a simple
panda brain dizzy.

Use the variable GIT_DISCOVERY_ACROSS_FILESYSTEM that changes the
behaviour from the default "limit to one filesystem" to "cross the
boundary as I ask you to"; makes the semantics much more straight
forward.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Documentation/git.txt |   10 +++++-----
 setup.c               |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index aa62083..eb78a2b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -530,15 +530,15 @@ 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'::
+'GIT_DISCOVERY_ACROSS_FILESYSTEM'::
 	When run in a directory that does not have ".git" repository
 	directory, git tries to find such a directory in the parent
 	directories to find the top of the working tree, but by default it
 	does not cross filesystem boundaries.  This environment variable
-	can be set to false value ("false" or zero) to tell git not to
-	stop at filesystem boundaries.  Like 'GIT_CEILING_DIRECTORIES',
-	this will not affect an explicit respository directory set via
-	'GIT_DIR' or on the command line.
+	can be set to true to tell git not to stop at filesystem
+	boundaries.  Like 'GIT_CEILING_DIRECTORIES', this will not affect
+	an explicit respository directory set via 'GIT_DIR' or on the
+	command line.
 
 git Commits
 ~~~~~~~~~~~
diff --git a/setup.c b/setup.c
index d290633..5a26b5b 100644
--- a/setup.c
+++ b/setup.c
@@ -392,7 +392,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 *   etc.
 	 */
 	offset = len = strlen(cwd);
-	one_filesystem = git_env_bool("GIT_ONE_FILESYSTEM", 0);
+	one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
 	if (one_filesystem) {
 		if (stat(".", &buf))
 			die_errno("failed to stat '.'");
-- 
1.7.0.4.552.gc303c1

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] Rename ONE_FILESYSTEM to DISCOVERY_ACROSS_FILESYSTEM
  2010-04-04 22:30                 ` [PATCH] Rename ONE_FILESYSTEM to DISCOVERY_ACROSS_FILESYSTEM Junio C Hamano
@ 2010-04-04 22:37                   ` Matthieu Moy
  0 siblings, 0 replies; 20+ messages in thread
From: Matthieu Moy @ 2010-04-04 22:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Lars R. Damerow, Linus Torvalds, Jeff King, Erik Faye-Lund, git

Junio C Hamano <gitster@pobox.com> writes:

> If a missing ONE_FILESYSTEM defaults to true, the only users who set this
> variable set it to false to tell git not to limit the discovery to one
> filesystem; there are too many negations in one sentence to make a simple
> panda brain dizzy.

Right.

+	an explicit respository directory set via 'GIT_DIR' or on the

While you're there, you can spell respository as "repository" ;-).

> diff --git a/setup.c b/setup.c
> index d290633..5a26b5b 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -392,7 +392,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  	 *   etc.
>  	 */
>  	offset = len = strlen(cwd);
> -	one_filesystem = git_env_bool("GIT_ONE_FILESYSTEM", 0);
> +	one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
>  	if (one_filesystem) {
>  		if (stat(".", &buf))
>  			die_errno("failed to stat '.'");

I guess you still have one instance of GIT_ONE_FILESYSTEM a little
below in the error message:

"Stopping at filesystem boundary since GIT_ONE_FILESYSTEM is true."

which should become

"Stopping at filesystem boundary since GIT_DISCOVERY_ACROSS_FILESYSTEM\n"
"is not set."

or so.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries
  2010-04-04 18:00               ` [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries Junio C Hamano
  2010-04-04 22:30                 ` [PATCH] Rename ONE_FILESYSTEM to DISCOVERY_ACROSS_FILESYSTEM Junio C Hamano
@ 2010-04-04 22:39                 ` Junio C Hamano
  1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2010-04-04 22:39 UTC (permalink / raw)
  To: Lars R. Damerow; +Cc: Linus Torvalds, Jeff King, Erik Faye-Lund, git

Junio C Hamano <gitster@pobox.com> writes:

>    I wonder if "git add" and friends should also notice it and warn.  If
>    you have more than one values of ce->ce_dev in the index, it means that
>    the working tree spans more than one filesystem and from a subdirectory
>    with an entry that has a ce->ce_dev different from the value for a path
>    at the top of the work tree, you will not be able to discover the top
>    of the tree without GIT_ONE_FILESYSTEM set to true.  A likely scenario
>    for this to happen would be:
>
>     (1) You have a tarball of some sort; you extract it $there;
>
> 	$ mkdir $there && cd $there
>         $ tar xf /var/tmp/tarball.tar
>
>     (2) You notice the filesystem lacks enough free space, and move some
>         part (say "images/") to a separate filesystem, and bind-mount;
>
>         $ mv images $another/. && rm -fr images && mkdir images
>         $ mount --bind $another/images images
>
>     (3) You add everything to start the project;
>
>         $ git init && git add .
>
>         Up to this point it would work (you are at the top of the working
>         tree).  And this is the point we _could_ notice and warn that you
>         will have trouble in step (4).
>
>     (4) Go down to a subdirectory and start futzing;
>
> 	$ cd images && gimp naughty.jpg && git add -u

And this adds such a check.

 read-cache.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index f1f789b..486bb2a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1550,6 +1550,7 @@ int write_index(struct index_state *istate, int newfd)
 	struct cache_entry **cache = istate->cache;
 	int entries = istate->cache_nr;
 	struct stat st;
+	int more_than_one_dev;
 
 	for (i = removed = extended = 0; i < entries; i++) {
 		if (cache[i]->ce_flags & CE_REMOVE)
@@ -1572,6 +1573,7 @@ int write_index(struct index_state *istate, int newfd)
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
+	more_than_one_dev = 0;
 	for (i = 0; i < entries; i++) {
 		struct cache_entry *ce = cache[i];
 		if (ce->ce_flags & CE_REMOVE)
@@ -1580,8 +1582,15 @@ int write_index(struct index_state *istate, int newfd)
 			ce_smudge_racily_clean_entry(ce);
 		if (ce_write_entry(&c, newfd, ce) < 0)
 			return -1;
+		if (i && ce->ce_dev != cache[0]->ce_dev)
+			more_than_one_dev = 1;
 	}
 
+	if (more_than_one_dev &&
+	    !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0))
+		warning("working tree spans across filesystems but "
+			"GIT_DISCOVERY_ACROSS_FILESYSTEM is not set.");
+
 	/* Write extension data here */
 	if (istate->cache_tree) {
 		struct strbuf sb = STRBUF_INIT;

^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2010-04-04 22:40 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-17 19:55 [PATCH v3 0/3] Add support for GIT_ONE_FILESYSTEM Lars R. Damerow
2010-03-17 19:55 ` [PATCH 1/3] config.c: remove static keyword from git_env_bool() Lars R. Damerow
2010-03-17 19:55 ` [PATCH 2/3] truncate cwd string before printing error message Lars R. Damerow
2010-03-17 19:55 ` [PATCH 3/3] Add support for GIT_ONE_FILESYSTEM Lars R. Damerow
2010-03-28  9:22   ` Jeff King
2010-03-28 12:05     ` Jeff King
2010-03-28 16:44     ` Junio C Hamano
2010-03-28 17:32       ` Lars Damerow
2010-03-30 15:58         ` Jeff King
2010-03-30 22:43       ` Linus Torvalds
2010-03-30 22:59         ` Thomas Rast
2010-03-30 23:04           ` Jeff King
2010-03-30 23:02         ` Jeff King
2010-03-30 23:10           ` Junio C Hamano
2010-03-30 23:12           ` Linus Torvalds
2010-03-31  0:54             ` Erik Faye-Lund
2010-04-04 18:00               ` [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries Junio C Hamano
2010-04-04 22:30                 ` [PATCH] Rename ONE_FILESYSTEM to DISCOVERY_ACROSS_FILESYSTEM Junio C Hamano
2010-04-04 22:37                   ` Matthieu Moy
2010-04-04 22:39                 ` [PATCH] GIT_ONE_FILESYSTEM: flip the default to stop at filesystem boundaries Junio C Hamano

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 NNTP newsgroup(s).