Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries
- From
Jeff King <peff@peff.net>
- Date
- Apr 9, 2013, 16:50 UTC
- Message-ID
- <20130409165032.GB21972@sigill.intra.peff.net>
- In-Reply-To
- <1365499297-8667-3-git-send-email-artagnon@gmail.com>
On Tue, Apr 09, 2013 at 02:51:37PM +0530, Ramkumar Ramachandra wrote:
Show 8 quoted lines
> 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.
Show 9 quoted lines
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com> > --- > 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.
Show 11 quoted lines
> @@ -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?
Show 11 quoted lines
> + 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