On Tue, Apr 09, 2013 at 02:51:37PM +0530, Ramkumar Ramachandra wrote:
> Currently, git add has the logic for refusing to add gitlinks using
> treat_path(), which in turn calls check_path_for_gitlink(). However,
> this only checks for an in-index submodule (or gitlink cache_entry).
> A path inside a git repository in the worktree still adds fine, and
> this is a bug. The logic for denying it is very similar to denying
> adding paths beyond symbolic links: die_if_path_beyond_symlink().
> Follow its example and write a die_if_path_beyond_gitrepo() to fix
> this bug.
Thanks for working on this.
I think the direction is a good one. It does disallow Jakub's crazy
shared-directory setup. I am not too sad to see that disallowed, at
least by default, because there are so many ways to screw yourself while
using it if you are not careful (I tried something similar once, and
gave up because I kept running into problematic cases).
I am not opposed to having an escape hatch to operate in that mode, but
it should be triggered explicitly so it doesn't catch users unaware.
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
> ---
> builtin/add.c | 5 +++--
> cache.h | 2 ++
> pathspec.c | 12 ++++++++++++
> pathspec.h | 1 +
> symlinks.c | 43 +++++++++++++++++++++++++++++++++++++------
> t/t3700-add.sh | 2 +-
> 6 files changed, 56 insertions(+), 9 deletions(-)
I am not super-familiar with this part of the code, but having worked on
it once two years ago for the same problem, your solution looks like the
right thing.
> @@ -142,8 +143,22 @@ 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;
> + /* Check to see if the directory contains a
> + git repository */
> + struct stat st;
> + struct strbuf dotgitentry = STRBUF_INIT;
> + strbuf_addf(&dotgitentry, "%s/.git", cache->path);
Can we use mkpath here to avoid an allocation? Or even better,
cache->path is PATH_MAX+1 bytes, and we munge it earlier in the
function. Can we just check the length and stick "/.git" on the end?
> + if (lstat(dotgitentry.buf, &st) < 0) {
> + if (errno == ENOENT) {
> + strbuf_release(&dotgitentry);
> + last_slash_dir = last_slash;
> + continue;
> + }
> + *ret_flags = FL_LSTATERR;
> + }
> + else
> + *ret_flags = FL_GITREPO;
> + strbuf_release(&dotgitentry);
In my original patch long ago, Junio asked if we should be checking
is_git_directory() when we find a ".git" entry, to make sure it is not a
false positive. I don't have a strong opinion either way, but if we do
that, we would possibly want to update treat_path to do the same thing
for consistency.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html