Bug Report: git add

Bug Report: git add

[Darren Cook]

(Apologies if there is a proper place to report bugs; but I could not
find one.)

PROBLEM: "git add" adds sub-directories without checking to see if there
is already a git repository already there.

WHY BAD: This causes files to be in two repositories (leading to a mess
if you don't notice for a while...)

ONE SOLUTION: When adding files from a directory (except root of the
repository, of course) look for a .git subdirectory, and complain if
found. Allow --force to override this.

MORE SOPHISTICATED:
 1. Offer to merge in all that history, followed by removing that old
.git subdirectory.

 2. Look inside the .git subdirectory to see if the file being added is
actually under control there. If not, no need to complain.


EXAMPLE OF PROBLEM

The problem can arise when people are just dipping their toe into git,
and decide to try it on just one directory, then later expand its use to
the whole project.

  mkdir test
  cd test

  mkdir settings
  cd settings
  git init
  touch x
  git add x
  git commit -m "xx"

  (time passes)

  cd ..
  git init
  git add settings/
   (should complain)



Thanks for taking the time to read this,

Darren




-- 
Darren Cook, Software Researcher/Developer

http://dcook.org/work/ (About me and my work)
http://dcook.org/blogs.html (My blogs and articles)

Re: Bug Report: git add

[Ramkumar Ramachandra]

Hi Darren,

Darren Cook writes:
> (Apologies if there is a proper place to report bugs; but I could not
> find one.)

There is no bugtracker, so this is the right place to report it.
Thanks for reporting.

> PROBLEM: "git add" adds sub-directories without checking to see if there
> is already a git repository already there.
> 
> WHY BAD: This causes files to be in two repositories (leading to a mess
> if you don't notice for a while...)
> 
> ONE SOLUTION: When adding files from a directory (except root of the
> repository, of course) look for a .git subdirectory, and complain if
> found. Allow --force to override this.

This is a good suggestion.  It already has a way to handle gitlinks
(for submodules), so this seems like a very reasonable feature.

> MORE SOPHISTICATED:
>  1. Offer to merge in all that history, followed by removing that old
> .git subdirectory.

Hm, I don't like this one -- there are too many ways to "merge" the
history, and I can't see a sane default (or even a sane subset of
defaults).

>  2. Look inside the .git subdirectory to see if the file being added is
> actually under control there. If not, no need to complain.

I don't like this one either.  Tangling up two Git repositories like
this is not a good idea -- the user should use submodules or similar.

Next steps: Me (or someone else who has the time) will post a patch
fixing this shortly.

-- Ram

Re: Bug Report: git add

[Jeff King]

On Wed, Apr 06, 2011 at 08:18:54AM +0900, Darren Cook wrote:

> PROBLEM: "git add" adds sub-directories without checking to see if there
> is already a git repository already there.

Sort of...

> EXAMPLE OF PROBLEM
> 
> The problem can arise when people are just dipping their toe into git,
> and decide to try it on just one directory, then later expand its use to
> the whole project.
> 
>   mkdir test
>   cd test
> 
>   mkdir settings
>   cd settings
>   git init
>   touch x
>   git add x
>   git commit -m "xx"
> 
>   (time passes)
> 
>   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.

-Peff

Re: Bug Report: git add

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>   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?").

Re: Bug Report: git add

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> >>   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

Re: Bug Report: git add

[Jeff King]

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

> > 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);
 }
 
 /*

Re: Bug Report: git add

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> @@ -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;

This only checks "does the directory have .git in it?".

It probably is sufficient, but setup.c:is_git_directory() may do a more
appropriate check, I think.  That ".git" thing could be a regular file
(i.e. "gitdir: path"), so depending on the junk.st_mode, you may have to
call read_gitfile_gently() on it before checking with is_git_directory().

Alternatively, resolve_gitlink_ref() might be usable, but because it is
primarily meant to deal with a submodule, there is a slight impedance
mismatch with the test we are trying to do.  In this codepath, all we care
about is if the subdirectory is controlled by a separate git repository,
and it does not matter if that is an untracked directory from the
superproject's point of view, or if it is bound to the superproject as a
submodule.

Also I suspect resolve_gitlink_ref() may not work for a submodule that
does not have an initial commit but that is a separate issue.

Re: Bug Report: git add

[Jeff King]

On Thu, Apr 07, 2011 at 12:28:26AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > @@ -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;
> 
> This only checks "does the directory have .git in it?".

Yeah. I was trying to keep the test as inexpensive as possible, since
this is a very frequently called codepath. But really, doing a more
elaborate test shouldn't matter. The common case will be that the stat
fails, and we do nothing else.

I do worry about adding an extra lstat for each directory having
noticeable overhead. Maybe it doesn't matter because of the stat
caching, but I didn't measure.

> It probably is sufficient, but setup.c:is_git_directory() may do a more
> appropriate check, I think.  That ".git" thing could be a regular file
> (i.e. "gitdir: path"), so depending on the junk.st_mode, you may have to
> call read_gitfile_gently() on it before checking with is_git_directory().

I worry a little about the PATH_MAX check and die in is_git_directory. I
would hate for a deep hierarchy to start failing because of this extra
check. OTOH, it is only 5 extra characters to append ".git", so it is
unlikely that a path was that close to PATH_MAX but not exceeding it.

Similarly, read_gitfile_gently is anything but gentle. It die()s if we
can't open the '.git' file or it is in an invalid format, which would be
a regression here.

-Peff

Re: Bug Report: git add

[Jeff King]

On Fri, Apr 08, 2011 at 03:15:54PM -0400, Jeff King wrote:

> > This only checks "does the directory have .git in it?".
> 
> Yeah. I was trying to keep the test as inexpensive as possible, since
> this is a very frequently called codepath. But really, doing a more
> elaborate test shouldn't matter. The common case will be that the stat
> fails, and we do nothing else.
> 
> I do worry about adding an extra lstat for each directory having
> noticeable overhead. Maybe it doesn't matter because of the stat
> caching, but I didn't measure.

I just did some measurements on linux-2.6 (which is a fair bit more
directory-heavy than something like git.git). The difference is
measurable, but not significant.

The average for "git add ." (on a clean, warm-cache repository) over
many trials with the extra lstats is about 2-3% slower.  But the error
margin between trials is something like 3-4%. IOW, even though the
average was worse, the best trial for the new code was still faster than
the worst trial for the old.

This is on Linux, though. It would probably be worse on an OS with a
slower stat.

-Peff

Re: Bug Report: git add

[Jakub Narebski]

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Darren Cook writes:

> > PROBLEM: "git add" adds sub-directories without checking to see if there
> > is already a git repository already there.
> > 
> > WHY BAD: This causes files to be in two repositories (leading to a mess
> > if you don't notice for a while...)
> > 
> > ONE SOLUTION: When adding files from a directory (except root of the
> > repository, of course) look for a .git subdirectory, and complain if
> > found. Allow --force to override this.
> 
> This is a good suggestion.  It already has a way to handle gitlinks
> (for submodules), so this seems like a very reasonable feature.

I just hope that a suboptimal workflow that I use won't stop working.

Currently I have TODO file in gitweb/ subdirectory, which is stored in
gitweb/.git repository.  Still it doesn't prevent me from "git add"-ing
e.g. 'gitweb/gitweb.perl' to git repository itself.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: Bug Report: git add

[Ramkumar Ramachandra]

Hi Jakub,

Jakub Narebski writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> > Darren Cook writes:
> 
> > > PROBLEM: "git add" adds sub-directories without checking to see if there
> > > is already a git repository already there.
> > > 
> > > WHY BAD: This causes files to be in two repositories (leading to a mess
> > > if you don't notice for a while...)
> > > 
> > > ONE SOLUTION: When adding files from a directory (except root of the
> > > repository, of course) look for a .git subdirectory, and complain if
> > > found. Allow --force to override this.
> > 
> > This is a good suggestion.  It already has a way to handle gitlinks
> > (for submodules), so this seems like a very reasonable feature.
> 
> I just hope that a suboptimal workflow that I use won't stop working.
> 
> Currently I have TODO file in gitweb/ subdirectory, which is stored in
> gitweb/.git repository.  Still it doesn't prevent me from "git add"-ing
> e.g. 'gitweb/gitweb.perl' to git repository itself.

It shouldn't.  The idea is merely to make the porcelain show a
friendly warning, which can be overriden with a '--force'.

-- Ram

Re: Bug Report: git add

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Currently I have TODO file in gitweb/ subdirectory, which is stored in
> gitweb/.git repository.  Still it doesn't prevent me from "git add"-ing
> e.g. 'gitweb/gitweb.perl' to git repository itself.

I would have to say that it is somewhat a sensible thing to want to do
from an individual contributor's point of view to keep track of personal
notes on a subpart of a project in a separate repository.

It however directly contradicts with the approach I suggested earlier,
which resulted in Peff's patch

  http://thread.gmane.org/gmane.comp.version-control.git/170937/focus=171040 

and will be broken, I think.

We could skip the check for a directory D where there already is a path
D/P (where P has one or more path components, e.g. "file", "subdir/file")
tracked and check only if D/.git is a git directory when adding path in D
if there is no other D/P is tracked (and perhaps require --force), if we
really wanted to keep supporting what you are doing.  But I do not think
it is worth doing, considering the possibility that such a loophole would
lead to even more confusing behaviour to new users.

If there is an equally easy way of keeping track of personal notes in a
subpart of a larger project like you do, without having an unrelated .git/
directory in a worktree that is controlled by a project and mixing files
in a single directory in such a way that some belong to the main project
while others belong to the unrelated "personal notes" project, I would
rather see us recommend such an approach, and declare that your use case
is forbidden, as it would give us a far easier to explain rule: "files in
one directory can be controlled only by one .git/ directory".

Besides, if the subpart of the project you are interested in and want to
have personal notes about were at the top-level of the project, you
wouldn't be using the same workflow as you currently do, as you cannot
obviously have two .git/ at the same level.

Re: Bug Report: git add

[Jeff King]

On Mon, Apr 11, 2011 at 10:55:31AM -0700, Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Currently I have TODO file in gitweb/ subdirectory, which is stored in
> > gitweb/.git repository.  Still it doesn't prevent me from "git add"-ing
> > e.g. 'gitweb/gitweb.perl' to git repository itself.
> 
> I would have to say that it is somewhat a sensible thing to want to do
> from an individual contributor's point of view to keep track of personal
> notes on a subpart of a project in a separate repository.
> 
> It however directly contradicts with the approach I suggested earlier,
> which resulted in Peff's patch
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/170937/focus=171040 
> 
> and will be broken, I think.

Interestingly, I came across a similar situation yesterday. I have a
project with asciidoc documentation, and I was looking at making the
built forms available on a separate ref (similar to how you do the
html/man docs in git). One way I came up was to make a repo in
"Documentation/.git" which ignored all of the "*.txt" sources (which
tracked by the main repo) but track all of the built "*.html" files
(which are ignored by the main repo).

Which is exactly the kind of setup my patch tries to declare invalid.
But then I decided that what I was considering was too gross and
error-prone, and maybe it _should_ be banned. :)

In particular, some flaws I considered are:

  1. it is easy to get meta-files like ".gitignore" mixed up
     between the two repositories. And in fact, you have no way in the
     sub-repository to ignore the files from the parent repository
     except by using the non-tracked $GIT_DIR/info/exclude file. And
     without that, you can never use "git clean" in the sub-repository.

  2. When you want to add/commit files in the sub-directory to the main
     repository, you must do so from _outside_ that directory. Otherwise
     git will find the sub-repository's .git dir and you accidentally
     add them to the sub-repository.

> If there is an equally easy way of keeping track of personal notes in a
> subpart of a larger project like you do, without having an unrelated .git/
> directory in a worktree that is controlled by a project and mixing files
> in a single directory in such a way that some belong to the main project
> while others belong to the unrelated "personal notes" project, I would
> rather see us recommend such an approach, and declare that your use case
> is forbidden, as it would give us a far easier to explain rule: "files in
> one directory can be controlled only by one .git/ directory".

It works fine if you just put the notes in their own directory, i.e.:

  git init gitweb/local &&
  $EDITOR gitweb/local/notes

And if you have just the single-file case, you can always do:

  ln -s local/notes gitweb/notes &&
  echo gitweb/notes >.git/info/exclude

to overlay it in the parent tree. You do have to chdir into the sub-repo
to actually commit, but that is a _good_ thing, because it means you're
not accidentally commiting changes to "gitweb.perl" to the sub-repo.

I do something similar with my config.mak, which I track in my own Meta
sub-repo but symlink into place in the main git repository.

-Peff