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

Junio C Hamano [off-list ref] writes:

quoted

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.

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.