On Thu, Aug 1, 2013 at 3:29 AM, Dale R. Worley <wor...@alum.mit.edu> wrote:
> I've run into a problem (with Git 1.8.3.3) where I cannot add a
> symbolic link (as such) to the repository *if* its path is given
> absolutely; instead Git adds the file the symbolic link points to.
> (If I give the path relatively, Git does what I expect, that is, adds
> the symbolic link.)
>
> I've written a test script that shows the problem and included it
> below.
>
> I would not expect *how* a path is presented to Git to change how Git
> processes the path. In the test case, I would expect "/bin/awk" and
> "../../bin/awk" to produce the same effect when used as arguments to
> "git add".
>
> What is going on in the code is this: In "git add", all paths are
> normalized by the function prefix_path_gently() in abspath.c. That
> function removes symbolic links from the pathspec *only if* it is
> absolute, as shown in the first few lines of the function:
>
> static char *prefix_path_gently(const char *prefix, int len, const char
> *path)
> {
> const char *orig = path;
> char *sanitized;
> if (is_absolute_path(orig)) {
> - const char *temp = real_path(path);
> + const char *temp = absolute_path(path);
> sanitized = xmalloc(len + strlen(temp) + 1);
> strcpy(sanitized, temp);
> } else {
>
> real_path() is specified to remove symbolic links. As shown, I've
> replaced real_path() with absolute_path(), based on the comment at the
> top of real_path():
>
> /*
> * Return the real path (i.e., absolute path, with symlinks resolved
> * and extra slashes removed) equivalent to the specified path. (If
> * you want an absolute path but don't mind links, use
> * absolute_path().) The return value is a pointer to a static
> * buffer.
> *
>
> If I replace real_path() with absolute_path() as shown, the problem I
> am testing for disappears.
I think it also reverts 18e051a (setup: translate symlinks in filename
when using absolute paths - 2010-12-27). real_path() (or
make_absolute_path() back then) is added to resolve symlinks, at least
ones leading to the work tree, not ones inside the work tree, if I
understand the commit message correctly.
>
> With the above change, the test suite runs with zero failures, so it
> doesn't affect any common Git usage.
It means the test suite is incomplete. As you can see, the commit
introducing this change does not come with a test case to catch people
changing this.
>
> But I don't know enough about the internal architecture of Git to know
> that my change is correct in all cases. I'm almost certain that the
> normalization process for pathspecs should *not* normalize a final
> component that is a symbolic link. But I would expect it would be
> desirable to normalize non-final components that are symbolic links.
> On the other hand, that might not matter.
>
> Can someone give me advice on what this code *should* do?
It does as the function name says: given cwd, a prefix (i.e. a
relative path with no ".." components) and a path relative to
cwd+prefix, convert 'path' to something relative to cwd. In the
simplest case, prepending the prefix to 'path' is enough. cwd is also
get_git_work_tree().
I agree with you that this code should not resolve anything in the
components after 'cwd', after rebasing the path to 'cwd' (not just the
final component). Not sure how to do it correctly though because we do
need to resolve symlinks before cwd. Maybe a new variant of real_path
that stops at 'cwd'?
We may also have problems with resolve symlinks before cwd when 'path'
is relative, as normalize_path_copy() does not resolve symlinks. We
just found out emacs has this bug [1] but did not realize we also have
one :-P.
[1] http://thread.gmane.org/gmane.comp.version-control.git/231268
>
> I believe I can prepare a proper test for the test suite for this, so
> once I know what the code change should be, I can prepare a proper
> patch for it.
>
> Dale
--
Duy
--
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
