Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Ramkumar Ramachandra]

Junio C Hamano wrote:
 And I also misread we currently don't handle above as but we
 really should allow adding d/f when d is at the top of the working
 tree of another project, but that was not what you meant to say.
 Instead, We do not notice such a bad case in today's code yet was
 what you meant.  But if we are to use there are cases in [1/n] and
 start [2/n] with Now we have renamed, let's do this, then we do
 not have to bother saying anything in [1/n] about the upcoming
 change in [2/n], especially the patches come back-to-back in a
 single series.

Exactly.  Yeah, I don't think you patch makes sense as a standalone
anyway: I'll use appropriate wording when I roll the series, so it
follows nicely.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Ramkumar Ramachandra]

Junio C Hamano wrote:
 One can have symlinks to anywhere all one wants.  We track symlinks.
 [...]

Yes, I know.  We store symlinks as blobs containing one line, the path
to the file, without a trailing newline.  And we have a mode for it to
distinguish it from regular files.

What I meant is:

echo baz newfile
cd foo/bar/quux
ln -s ../../../newfile
cd ../../..# Back to toplevel
git add foo/bar/quux/newfile

This is allowed.  While:

cd foo/bar/quux
echo baz newfile
cd ../../..# Back to toplevel
ln -s foo/bar/quux
git add quux/newfile

is disallowed.  Then again, if we were to replace the last line with:

cd quux
git add newfile

and it works.

Notice that both symlinks are pointing to paths inside out repository,
and the only difference is that the second example attempts to add a
path with a symlink as the non-final component.  The path is not
pointing outside our repository, as the function name would
indicate.

Anyway, it's just a minor detail that would be nice to fix in the
future.  Nothing urgent.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Ramkumar Ramachandra artag...@gmail.com writes:

 Exactly.  Yeah, I don't think you patch makes sense as a standalone
 anyway.

Yes, it was purely a preparatory step.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Jeff King]

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

 @@ -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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Ramkumar Ramachandra artag...@gmail.com writes:

 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.

 Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
 ---

 @@ -166,6 +166,7 @@ static const char **validate_pathspec(const char **argv, 
 const char *prefix)
   const char **p;
   for (p = pathspec; *p; p++) {
   die_if_path_beyond_symlink(*p, prefix);
 + die_if_path_beyond_gitrepo(*p, prefix);
   }
   }
 diff --git a/cache.h b/cache.h
 index e1e8ce8..987d7f3 100644
 --- a/cache.h
 +++ b/cache.h
 @@ -962,6 +962,8 @@ struct cache_def {
  
  extern int has_symlink_leading_path(const char *name, int len);
 +extern int has_gitrepo_leading_path(const char *name, int len);

I looked at the output from grep has_symlink_leading_path and also
for die_if_path_beyond; all of these places are checking I have
this multi-level path; I want to know if the path does not (should
not) be part of the current project, I think.  Certainly the one in
the update-index is about the same operation as git add you are
patching.

Isn't it a better approach to _rename_ the existing function not to
single out symlink-ness of the path first ?  A symlink in the
middle of such a multi-level path that leads to a place outside the
project is _not_ the only way to step out of our project boundary.  A
directory in the middle of a multi-level path that is the top-level
of the working tree of a foreign project is another way to step out
of our project boundary.  Perhaps

die_if_path_outside_our_project()
path_outside_our_project()

And then update the implementation of path_outside_our_project(),
which only took symlink in the middle into account so far, and
teach it that such a top-level of the working tree of a foreign
project is also stepping out of our project?

That way, you do not have to settle on fixing the bug only in git
add and keep the bug in git update-index, I think.

I think the hit in builtin/apply.c deals with the same beyond
symlink is outside our project check and can be updated like so.  I
didn't look at the ones in diff-lib.c and dir.c so you may want to
double check on what they use it for.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 Ramkumar Ramachandra artag...@gmail.com writes:

 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.

 Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
 ---

 @@ -166,6 +166,7 @@ static const char **validate_pathspec(const char **argv, 
 const char *prefix)
  const char **p;
  for (p = pathspec; *p; p++) {
  die_if_path_beyond_symlink(*p, prefix);
 +die_if_path_beyond_gitrepo(*p, prefix);
  }
  }
 diff --git a/cache.h b/cache.h
 index e1e8ce8..987d7f3 100644
 --- a/cache.h
 +++ b/cache.h
 @@ -962,6 +962,8 @@ struct cache_def {
  
  extern int has_symlink_leading_path(const char *name, int len);
 +extern int has_gitrepo_leading_path(const char *name, int len);

 I looked at the output from grep has_symlink_leading_path and also
 for die_if_path_beyond; all of these places are checking I have
 this multi-level path; I want to know if the path does not (should
 not) be part of the current project, I think.  Certainly the one in
 the update-index is about the same operation as git add you are
 patching.

 Isn't it a better approach to _rename_ the existing function not to
 single out symlink-ness of the path first ?  A symlink in the
 middle of such a multi-level path that leads to a place outside the
 project is _not_ the only way to step out of our project boundary.  A
 directory in the middle of a multi-level path that is the top-level
 of the working tree of a foreign project is another way to step out
 of our project boundary.  Perhaps

   die_if_path_outside_our_project()
 path_outside_our_project()

 And then update the implementation of path_outside_our_project(),
 which only took symlink in the middle into account so far, and
 teach it that such a top-level of the working tree of a foreign
 project is also stepping out of our project?

 That way, you do not have to settle on fixing the bug only in git
 add and keep the bug in git update-index, I think.

 I think the hit in builtin/apply.c deals with the same beyond
 symlink is outside our project check and can be updated like so.  I
 didn't look at the ones in diff-lib.c and dir.c so you may want to
 double check on what they use it for.

The first step (renaming and adjusting comments) would look like
this.


 builtin/add.c  |  6 +++---
 builtin/apply.c|  8 ++--
 builtin/check-ignore.c |  2 +-
 builtin/update-index.c |  4 ++--
 cache.h|  4 ++--
 diff-lib.c |  2 +-
 dir.c  |  2 +-
 pathspec.c |  6 +++---
 pathspec.h |  2 +-
 preload-index.c|  2 +-
 symlinks.c | 10 +-
 t/t0008-ignores.sh |  2 +-
 12 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8..7cb80ef 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -155,8 +155,8 @@ static void refresh(int verbose, const char **pathspec)
 
 /*
  * Normalizes argv relative to prefix, via get_pathspec(), and then
- * runs die_if_path_beyond_symlink() on each path in the normalized
- * list.
+ * runs die_if_path_outside_our_project() on each path in the
+ * normalized list.
  */
 static const char **validate_pathspec(const char **argv, const char *prefix)
 {
@@ -165,7 +165,7 @@ static const char **validate_pathspec(const char **argv, 
const char *prefix)
if (pathspec) {
const char **p;
for (p = pathspec; *p; p++) {
-   die_if_path_beyond_symlink(*p, prefix);
+   die_if_path_outside_our_project(*p, prefix);
}
}
 
diff --git a/builtin/apply.c b/builtin/apply.c
index 5b882d0..d0b408e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3469,10 +3469,14 @@ static int check_to_create(const char *new_name, int 
ok_if_exists)
 * A leading component of new_name might be a symlink
 * that is going to be removed with this patch, but
 * still pointing at somewhere that has the path.
-* In such a case, path new_name does not exist as
+* Or it could be the top-level of a working tree of
+* a different project that is embedded in our working
+* tree.
+*
+* In such cases, path new_name does not exist as
 * far as git is concerned.
 */
-   if (has_symlink_leading_path(new_name, strlen(new_name)))

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 I looked at the output from grep has_symlink_leading_path and also
 for die_if_path_beyond; all of these places are checking I have
 this multi-level path; I want to know if the path does not (should
 not) be part of the current project, I think.  Certainly the one in
 the update-index is about the same operation as git add you are
 patching.

 Isn't it a better approach to _rename_ the existing function not to
 single out symlink-ness of the path first ?  A symlink in the
 middle of such a multi-level path that leads to a place outside the
 project is _not_ the only way to step out of our project boundary.  A
 directory in the middle of a multi-level path that is the top-level
 of the working tree of a foreign project is another way to step out
 of our project boundary.  Perhaps

  die_if_path_outside_our_project()
 path_outside_our_project()

 And then update the implementation of path_outside_our_project(),
 which only took symlink in the middle into account so far, and
 teach it that such a top-level of the working tree of a foreign
 project is also stepping out of our project?

 That way, you do not have to settle on fixing the bug only in git
 add and keep the bug in git update-index, I think.

 I think the hit in builtin/apply.c deals with the same beyond
 symlink is outside our project check and can be updated like so.  I
 didn't look at the ones in diff-lib.c and dir.c so you may want to
 double check on what they use it for.

 The first step (renaming and adjusting comments) would look like
 this.

Actually, there is another function check_leading_path() you may
want also adjust.

/*
 * Return zero if path 'name' has a leading symlink component or
 * if some leading path component does not exists.
 *
 * Return -1 if leading path exists and is a directory.
 *
 * Return path length if leading path exists and is neither a
 * directory nor a symlink.
 */
int check_leading_path(const char *name, int len)
{
return threaded_check_leading_path(default_cache, name, len);
}

I think what the callers of this function care about is if the name
is a path that should not be added to our index (i.e. points
outside the repository).  If you had a symlink d that points at e
when our project does have a subdirectory e with file f,

check_leading_path(d/f)

wants to say bad, even though the real file pointed at, i.e. e/f
is inside our working tree, so outside our working tree is not
quite correct in the strict sense (this applies equally to
has_symlink_leading_path), but I think we should treat the case
where d (and d/f) belongs to the working tree of a repository
for a separate project, that is embedded in our working tree the
same way.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Ramkumar Ramachandra]

Junio C Hamano wrote:
 The first step (renaming and adjusting comments) would look like
 this.

Thanks for this!  I like the name die_if_path_outside_our_project().
I'll take care of the rest.`
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Sorry for repeated rerolls.  I had missed another instance in t0008
and also the explanation was lacking.

-- 8 --
Subject: [PATCH] symlinks: rename has_symlink_leading_path() to
 path_outside_our_project()

The purpose of the function is to prevent a path from getting added
to our project when its path component steps outside our working
tree, like this:

ln -s /etc myetc
git add myetc/passwd

We do not want to end up with myetc/passwd in our index.  To make
sure an attempt to add such a path is caught, the implementation
checks if there is any leading symbolic link in the given path
(adding myetc itself as a symbolic link to our project is
accepted).

But there are other cases to attempt to add a path that do not
belong to our project, which do not have to involve a symbolic link
in the leading path.

Rename the function, and die_if_path_beyond_symlink() function, to
clarify what they are really checking, not how they are checking.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 builtin/add.c  |  6 +++---
 builtin/apply.c|  8 ++--
 builtin/check-ignore.c |  2 +-
 builtin/update-index.c |  4 ++--
 cache.h|  4 ++--
 diff-lib.c |  2 +-
 dir.c  |  2 +-
 pathspec.c |  6 +++---
 pathspec.h |  2 +-
 preload-index.c|  2 +-
 symlinks.c | 10 +-
 t/t0008-ignores.sh |  4 ++--
 12 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8..7cb80ef 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -155,8 +155,8 @@ static void refresh(int verbose, const char **pathspec)
 
 /*
  * Normalizes argv relative to prefix, via get_pathspec(), and then
- * runs die_if_path_beyond_symlink() on each path in the normalized
- * list.
+ * runs die_if_path_outside_our_project() on each path in the
+ * normalized list.
  */
 static const char **validate_pathspec(const char **argv, const char *prefix)
 {
@@ -165,7 +165,7 @@ static const char **validate_pathspec(const char **argv, 
const char *prefix)
if (pathspec) {
const char **p;
for (p = pathspec; *p; p++) {
-   die_if_path_beyond_symlink(*p, prefix);
+   die_if_path_outside_our_project(*p, prefix);
}
}
 
diff --git a/builtin/apply.c b/builtin/apply.c
index 5b882d0..d0b408e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3469,10 +3469,14 @@ static int check_to_create(const char *new_name, int 
ok_if_exists)
 * A leading component of new_name might be a symlink
 * that is going to be removed with this patch, but
 * still pointing at somewhere that has the path.
-* In such a case, path new_name does not exist as
+* Or it could be the top-level of a working tree of
+* a different project that is embedded in our working
+* tree.
+*
+* In such cases, path new_name does not exist as
 * far as git is concerned.
 */
-   if (has_symlink_leading_path(new_name, strlen(new_name)))
+   if (path_outside_our_project(new_name, strlen(new_name)))
return 0;
 
return EXISTS_IN_WORKTREE;
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 0240f99..bce378d 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -88,7 +88,7 @@ static int check_ignore(const char *prefix, const char 
**pathspec)
full_path = prefix_path(prefix, prefix
? strlen(prefix) : 0, path);
full_path = check_path_for_gitlink(full_path);
-   die_if_path_beyond_symlink(full_path, prefix);
+   die_if_path_outside_our_project(full_path, prefix);
if (!seen[i]) {
exclude = last_exclude_matching_path(check, full_path,
 -1, dtype);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5c7762e..7c47fa2 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -186,8 +186,8 @@ static int process_path(const char *path)
struct cache_entry *ce;
 
len = strlen(path);
-   if (has_symlink_leading_path(path, len))
-   return error('%s' is beyond a symbolic link, path);
+   if (path_outside_our_project(path, len))
+   return error('%s' is outside our working tree, path);
 
pos = cache_name_pos(path, len);
ce = pos  0 ? NULL : active_cache[pos];
diff --git a/cache.h b/cache.h
index e1e8ce8..f6359b5 100644
--- a/cache.h
+++ b/cache.h
@@ -960,8 +960,8 @@ struct cache_def {
int prefix_len_stat_func;
 };
 
-extern int has_symlink_leading_path(const char *name, int len);
-extern int 

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Ramkumar Ramachandra]

Junio C Hamano wrote:
 I think what the callers of this function care about is if the name
 is a path that should not be added to our index (i.e. points
 outside the repository).  If you had a symlink d that points at e
 when our project does have a subdirectory e with file f,

 check_leading_path(d/f)

 wants to say bad, even though the real file pointed at, i.e. e/f
 is inside our working tree, so outside our working tree is not
 quite correct in the strict sense (this applies equally to
 has_symlink_leading_path), but

Actually, you introduced one naming regression:
has_symlink_leading_path() is a good name for what the function does,
as opposed to die_if_path_outside_our_tree(), which is misleading.
What about die_if_path_contains_links() to encapsulate gitlinks and
symlinks?

 I think we should treat the case
 where d (and d/f) belongs to the working tree of a repository
 for a separate project, that is embedded in our working tree the
 same way.

I'm not too sure about this.  It means that I can have symlinks to
files in various parts of my worktree, but not to directories.  Isn't
this an absurd limitation to impose?  I'm not saying that it's
particularly useful to have a symlink at / pointing to a directory
deeply nested in your repository, but that limitations must have some
concrete rationale.

Anyway, since we're not introducing any regressions (as
has_symlink_leading_path imposes the same absurd limitation), we don't
have to fix this now.  But it's certainly something worth fixing in
the future, I think.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Ramkumar Ramachandra]

Junio C Hamano wrote:
 But there are other cases to attempt to add a path that do not
 belong to our project, which do not have to involve a symbolic link
 in the leading path.

The reader is now wondering what this could possibly be, and why you
didn't send this patch earlier.  Perhaps clarify with: s/there are
cases/there may be cases/ and append One such case that we currently
don't handle yet is a path inside another git repository in our
worktree, as demonstrated by test t.X.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Jakub Narębski]

W dniu 09.04.2013 19:34, Junio C Hamano pisze:

 - if (has_symlink_leading_path(path, len))
 - return error('%s' is beyond a symbolic link, path);
 + if (path_outside_our_project(path, len))
 + return error('%s' is outside our working tree, path);
  

Don't we lose important information here?  Or we shouldn't care?

-- 
Jakub Narębski
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Ramkumar Ramachandra artag...@gmail.com writes:

 Junio C Hamano wrote:
 I think what the callers of this function care about is if the name
 is a path that should not be added to our index (i.e. points
 outside the repository).  If you had a symlink d that points at e
 when our project does have a subdirectory e with file f,

 check_leading_path(d/f)

 wants to say bad, even though the real file pointed at, i.e. e/f
 is inside our working tree, so outside our working tree is not
 quite correct in the strict sense (this applies equally to
 has_symlink_leading_path), but

 Actually, you introduced one naming regression:
 has_symlink_leading_path() is a good name for what the function does,
 as opposed to die_if_path_outside_our_tree(), which is misleading.
 What about die_if_path_contains_links() to encapsulate gitlinks and
 symlinks?

The cases we know that $d/f (where $d is a path that is one or
more levels, e.g. dir, d/i, or d/i/r) is bad are when

 - $d is a symlink, because what you could add to the index is $d
   and nothing underneath it; or

 - $d is a directory that is the top level of the working tree
   that is controled by $d/.git, because what you could add to the
   index is $d and nothing underneath it.

If $d were added to our index, the former will make 12 entry
and the latter will make 16 entry.  But the user may not want to
add $d ever to our project, so in that case, neither will give us a
symlink or a gitlink.

We should find a word that makes it clear that this path is beyond
something we _could_ add.  I do not think link is a good word for
it.  It shares the same mistake that led to the original misnomer,
i.e. the case we happened to notice was when we have symlink so
let's name it with 'symlink' somewhere in it.

 I think we should treat the case
 where d (and d/f) belongs to the working tree of a repository
 for a separate project, that is embedded in our working tree the
 same way.

 I'm not too sure about this.  It means that I can have symlinks to
 files in various parts of my worktree, but not to directories.

It does not mean that.  It is valid to do

ln -s myetc /etc
git add myetc

It is NOT valid to do

git add myetc/passwd

One can have symlinks to anywhere all one wants.  We track symlinks.

It is the same for the top-level of the working tree of a separate
project, be it a submodule or not.  It is valid to do

mkdir foo  (cd foo  git init  file)
git add foo

It is NOT valid to do

git add foo/file



--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Ramkumar Ramachandra artag...@gmail.com writes:

 The reader is now wondering what this could possibly be, and why you
 didn't send this patch earlier. 

Because it wasn't written back then?

 Perhaps clarify with: s/there are
 cases/there may be cases/ and append One such case that we currently
 don't handle yet is a path inside another git repository in our
 worktree, as demonstrated by test t.X.

I think we currently don't handle is a misstatement.  It is not a
bug that we don't.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Jakub Narębski jna...@gmail.com writes:

 W dniu 09.04.2013 19:34, Junio C Hamano pisze:

 -if (has_symlink_leading_path(path, len))
 -return error('%s' is beyond a symbolic link, path);
 +if (path_outside_our_project(path, len))
 +return error('%s' is outside our working tree, path);
  

 Don't we lose important information here?  Or we shouldn't care?

What important information is it?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Jakub Narębski]

Junio C Hamano wrote:
 Jakub Narębski jna...@gmail.com writes:
 
 W dniu 09.04.2013 19:34, Junio C Hamano pisze:

 -   if (has_symlink_leading_path(path, len))
 -   return error('%s' is beyond a symbolic link, path);
 +   if (path_outside_our_project(path, len))
 +   return error('%s' is outside our working tree, path);
  

 Don't we lose important information here?  Or we shouldn't care?
 
 What important information is it?

That the cause is symbolic link (or other git repository, in the future).

-- 
Jakub Narębski
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 Ramkumar Ramachandra artag...@gmail.com writes:

 The reader is now wondering what this could possibly be, and why you
 didn't send this patch earlier. 

 Because it wasn't written back then?

 Perhaps clarify with: s/there are
 cases/there may be cases/ and append One such case that we currently
 don't handle yet is a path inside another git repository in our
 worktree, as demonstrated by test t.X.

 I think we currently don't handle is a misstatement.  It is not a
 bug that we don't.

We can think of it this way.

In your working tree, there is an upper bound for the paths you can
include in your commit.  When you are at the top-level of your
working tree, you do not say git add ../f or git add ../d/f.
The root-level of your working tree is an upper bound and you do not
cross that boundary.

It turns out that there are lower bounds for the paths as well.
When we say Git tracks symbolic links, anything that appears
beyond a symbolic link is beyond that boundary.  If we track a
symbolic link l, we can of course add l. When l leads to
a directory somewhere else, the filesystem gives you an illusion
that there are things under l (e.g. l points at /etc and there
is l/passwd there), but that is beyond the boundary.  You do not
add l/passwd.  Otherwise git add l would become meaningless.
Does it add the symbolic link itself, or all the files in there,
pretending l is actually a directory?  We have chosen to say it is
the former, and apply that rule consistently.

It is the same for Git tracks submodules, which defines that the
top-level of the submodule working tree as such a lower boundary.




--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Jakub Narębski jna...@gmail.com writes:

 Junio C Hamano wrote:
 Jakub Narębski jna...@gmail.com writes:
 
 W dniu 09.04.2013 19:34, Junio C Hamano pisze:

 -  if (has_symlink_leading_path(path, len))
 -  return error('%s' is beyond a symbolic link, path);
 +  if (path_outside_our_project(path, len))
 +  return error('%s' is outside our working tree, path);
  

 Don't we lose important information here?  Or we shouldn't care?
 
 What important information is it?

 That the cause is symbolic link (or other git repository, in the future).

And in what way is it important?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries

[Junio C Hamano]

Ramkumar Ramachandra artag...@gmail.com writes:

 Junio C Hamano wrote:
 But there are other cases to attempt to add a path that do not
 belong to our project, which do not have to involve a symbolic link
 in the leading path.

 The reader is now wondering what this could possibly be, and why you
 didn't send this patch earlier.  Perhaps clarify with: s/there are
 cases/there may be cases/ and append One such case that we currently
 don't handle yet is a path inside another git repository in our
 worktree, as demonstrated by test t.X.

I _think_ I misread what you meant to say in the above.

We can go either way between are cases or may be cases.  I meant
it as an immediate predecessor ([PATCH 1/n]) to the patch you were
working on ([PATCH 2/n] and later), so in that context, it does not
matter.  [PATCH 2/n] will start as Now the naming is saner, let's
start noticing when the user gives a path is beyond our project
boundary because it is under control of another repository by adding
necessary logic to that function.

And I also misread we currently don't handle above as but we
really should allow adding d/f when d is at the top of the working
tree of another project, but that was not what you meant to say.
Instead, We do not notice such a bad case in today's code yet was
what you meant.  But if we are to use there are cases in [1/n] and
start [2/n] with Now we have renamed, let's do this, then we do
not have to bother saying anything in [1/n] about the upcoming
change in [2/n], especially the patches come back-to-back in a
single series.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html