Re: Bug Report: git add

Jeff King [off-list ref] writes:

quoted

  mkdir settings
  cd settings
  git init
  touch x
...
  cd ..
  git init
  git add settings/
   (should complain)

If you do "git add settings" (without the slash) it will add the
repository as a submodule.  Which is not the behavior you asked for, but
is at least reasonable. So the real bug seems to me the fact that "git
add settings/" and "git add settings" behave differently.

Also if "git add settings/x" does not complain, that would be a bigger
issue, whose solution would probably be in the same area.

It may finally be the time to rename has_symlink_leading_path() function
and enhance its feature.  It happens to check leading symlinks, but its
real purpose is to check if the given path is outside the working tree,
and a path that is in a subdirectory with its own .git repository
certainly is outside the working tree.  The callers should be able to say
what check is being asked for by naming the _purpose_ of the test ("is
this path outside the working tree?"), not the mechanics ("does the path
have a symlink that points outside?").

On Wed, Apr 06, 2011 at 06:09:37PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

quoted

  mkdir settings
  cd settings
  git init
  touch x
...
  cd ..
  git init
  git add settings/
   (should complain)

If you do "git add settings" (without the slash) it will add the
repository as a submodule.  Which is not the behavior you asked for, but
is at least reasonable. So the real bug seems to me the fact that "git
add settings/" and "git add settings" behave differently.

Also if "git add settings/x" does not complain, that would be a bigger
issue, whose solution would probably be in the same area.

It does not complain, and probably should. I'm unsure what "settings/"
should do: produce an error as "settings/x" does (or will do,
eventually, we hope), or behave as if "settings" was given.

Generally it is useful to collapse stray slashes for the user, but I
wonder if there is some use to differentiate between "settings" and "the
contents of the settings directory", as rsync does.

-Peff

On Wed, Apr 06, 2011 at 06:09:37PM -0700, Junio C Hamano wrote:

quoted

If you do "git add settings" (without the slash) it will add the
repository as a submodule.  Which is not the behavior you asked for, but
is at least reasonable. So the real bug seems to me the fact that "git
add settings/" and "git add settings" behave differently.

Also if "git add settings/x" does not complain, that would be a bigger
issue, whose solution would probably be in the same area.

It may finally be the time to rename has_symlink_leading_path() function
and enhance its feature.  It happens to check leading symlinks, but its
real purpose is to check if the given path is outside the working tree,
and a path that is in a subdirectory with its own .git repository
certainly is outside the working tree.  The callers should be able to say
what check is being asked for by naming the _purpose_ of the test ("is
this path outside the working tree?"), not the mechanics ("does the path
have a symlink that points outside?").

So here's a quick-and-dirty, "this is the first time I've ever looked at
this code" patch that catches this behavior. It doesn't do any renaming,
and it does an ugly job of exposing the flag constants.

Interestingly, builtin/add.c also has treat_gitlinks which specifically
bails on trying to add something in a submodule. But it misses this
case, because its test is to check the index for gitlink entries.

diff --git a/builtin/add.c b/builtin/add.c
index 024aae4..5754863 100644
--- a/builtin/add.c
+++ b/builtin/add.c

@@ -202,9 +202,15 @@ static const char **validate_pathspec(int argc, const char **argv, const char *p
 	if (pathspec) {
 		const char **p;
 		for (p = pathspec; *p; p++) {
-			if (has_symlink_leading_path(*p, strlen(*p))) {
+			int r = has_symlink_leading_path(*p, strlen(*p));
+			if (r) {
 				int len = prefix ? strlen(prefix) : 0;
-				die(_("'%s' is beyond a symbolic link"), *p + len);
+				if (r & FL_SYMLINK)
+					die(_("'%s' is beyond a symbolic link"), *p + len);
+				else if (r & FL_GITREPO)
+					die(_("'%s' is inside a submodule"), *p + len);
+				die("BUG: '%s' ignored for unknown reason %d",
+				    *p + len, r);
 			}
 		}
 	}

diff --git a/cache.h b/cache.h
index 21bcc52..2784f67 100644
--- a/cache.h
+++ b/cache.h

@@ -880,6 +880,8 @@ struct cache_def {
 	int prefix_len_stat_func;
 };
 
+#define FL_SYMLINK  (1 << 2)
+#define FL_GITREPO  (1 << 6)
 extern int has_symlink_leading_path(const char *name, int len);
 extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
 extern int check_leading_path(const char *name, int len);

diff --git a/symlinks.c b/symlinks.c
index 034943b..22152fd 100644
--- a/symlinks.c
+++ b/symlinks.c

@@ -47,7 +47,6 @@ static inline void reset_lstat_cache(struct cache_def *cache)
 
 #define FL_DIR      (1 << 0)
 #define FL_NOENT    (1 << 1)
-#define FL_SYMLINK  (1 << 2)
 #define FL_LSTATERR (1 << 3)
 #define FL_ERR      (1 << 4)
 #define FL_FULLPATH (1 << 5)

@@ -92,7 +91,7 @@ static int lstat_cache_matchlen(struct cache_def *cache,
 		match_len = last_slash =
 			longest_path_match(name, len, cache->path, cache->len,
 					   &previous_slash);
-		*ret_flags = cache->flags & track_flags & (FL_NOENT|FL_SYMLINK);
+		*ret_flags = cache->flags & track_flags & (FL_NOENT|FL_SYMLINK|FL_GITREPO);
 
 		if (!(track_flags & FL_FULLPATH) && match_len == len)
 			match_len = last_slash = previous_slash;

@@ -139,8 +138,21 @@ static int lstat_cache_matchlen(struct cache_def *cache,
 			if (errno == ENOENT)
 				*ret_flags |= FL_NOENT;
 		} else if (S_ISDIR(st.st_mode)) {
-			last_slash_dir = last_slash;
-			continue;
+			struct stat junk;
+			struct strbuf gitdir = STRBUF_INIT;
+			strbuf_add(&gitdir, cache->path, match_len);
+			strbuf_addstr(&gitdir, "/.git");
+			if (lstat(gitdir.buf, &junk) < 0) {
+				if (errno == ENOENT) {
+					last_slash_dir = last_slash;
+					strbuf_release(&gitdir);
+					continue;
+				}
+				*ret_flags = FL_LSTATERR;
+			}
+			else
+				*ret_flags = FL_GITREPO;
+			strbuf_release(&gitdir);
 		} else if (S_ISLNK(st.st_mode)) {
 			*ret_flags = FL_SYMLINK;
 		} else {

@@ -154,7 +166,7 @@ static int lstat_cache_matchlen(struct cache_def *cache,
 	 * path types, FL_NOENT, FL_SYMLINK and FL_DIR, can be cached
 	 * for the moment!
 	 */
-	save_flags = *ret_flags & track_flags & (FL_NOENT|FL_SYMLINK);
+	save_flags = *ret_flags & track_flags & (FL_NOENT|FL_SYMLINK|FL_GITREPO);
 	if (save_flags && last_slash > 0 && last_slash <= PATH_MAX) {
 		cache->path[last_slash] = '\0';
 		cache->len = last_slash;

@@ -197,7 +209,7 @@ static int lstat_cache(struct cache_def *cache, const char *name, int len,
  */
 int threaded_has_symlink_leading_path(struct cache_def *cache, const char *name, int len)
 {
-	return lstat_cache(cache, name, len, FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) & FL_SYMLINK;
+	return lstat_cache(cache, name, len, FL_SYMLINK|FL_DIR|FL_GITREPO, USE_ONLY_LSTAT) & (FL_SYMLINK|FL_GITREPO);
 }
 
 /*