Difficulty adding a symbolic link, part 3

Difficulty adding a symbolic link, part 3

[Dale R. Worley]

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.

With the above change, the test suite runs with zero failures, so it
doesn't affect any common Git usage.

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?

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
----------------------------------------------------------------------		
Here's a test case for adding a symbolic link.  This test exploits the
fact that on my system, /bin/awk is a symbolic link to "gawk".  As you
can see, the behavior of Git differs if the link's path is given to
"git add" as an absolute path or a relative path.

Here is the test script:
----------------------------------------------------------------------
#! /bin/bash

# Illustrates a problem with applying "git add" to a symbolic link.

set -x

# To be run from a directory one step below the root directory.  E.g.,
# "/tmp".
# On this system, /bin/awk is a symbolic link to "gawk", which
# means /tmp/gawk.

# Show the Git version.
git version

# Make a test directory and cd to it.
DIR=temp.$$
mkdir $DIR
cd $DIR

# Create a Git repository.
git init
# Set the worktree to be /
git config core.worktree /
# Create an empty commit.
git commit --allow-empty -m Empty.

# Add the symbolic link using its absolute name.
ABSOLUTE=/bin/awk
ls -l $ABSOLUTE
git add $ABSOLUTE
# Notice that the target of the link is added, not the link itself.
git status -uno

# Reset the index.
git reset

# Add the symbolic link using its relative name.
# Remember that we are two directory levels below the root directory now.
RELATIVE=../..$ABSOLUTE
ls -l $RELATIVE
git add $RELATIVE
# Notice that now the link itself is added.
git status -uno
----------------------------------------------------------------------
Here is sample output of the script:
----------------------------------------------------------------------
+ git version
git version 1.8.3.3.756.g07a2553.dirty
+ DIR=temp.22366
+ mkdir temp.22366
+ cd temp.22366
+ git init
Initialized empty Git repository in /git-add-link/temp.22366/.git/
+ git config core.worktree /
+ git commit --allow-empty -m Empty.
[master (root-commit) fb232e5] Empty.
+ ABSOLUTE=/bin/awk
+ ls -l /bin/awk
lrwxrwxrwx. 1 root root 4 Nov  2  2012 /bin/awk -> gawk
+ git add /bin/awk
+ git status -uno
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	new file:   ../../bin/gawk
#
# Untracked files not listed (use -u option to show untracked files)
+ git reset
+ RELATIVE=../../bin/awk
+ ls -l ../../bin/awk
lrwxrwxrwx. 1 root root 4 Nov  2  2012 ../../bin/awk -> gawk
+ git add ../../bin/awk
+ git status -uno
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	new file:   ../../bin/awk
#
# Untracked files not listed (use -u option to show untracked files)
----------------------------------------------------------------------

Dale

Re: Difficulty adding a symbolic link, part 3

[Duy Nguyen]

On Thu, Aug 1, 2013 at 3:29 AM, Dale R. Worley <worley@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

Re: Difficulty adding a symbolic link, part 3

[Dale R. Worley]

> From: Duy Nguyen <pclouds@gmail.com>

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

Who should be blamed for omitting the test?

> > 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().

But as you can see, the exact behavior that the function is intended
to exhibit regarding symlinks is not clear from the function name;
there should have been a real explanation in the comment above the
function.

Dale

Re: Difficulty adding a symbolic link, part 3

[Dale R. Worley]

> From: Duy Nguyen <pclouds@gmail.com>

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

Thanks for the detailed information.  It seems to me that the minimum
needed change is that the handling of relative and absolute paths
should be made consistent.

> [1] http://thread.gmane.org/gmane.comp.version-control.git/231268

That problem isn't so much a matter of not resolving symlinks but
rather the question of what ".." means.  In the case shown in that
message, the current directory *is* {topdir}/z/b, but it was entered
with "cd {topdir}/b" -- and the shell records the latter as the value
of $PWD.  (Actually, the bash shell can handle symbolic-linked
directories *either* way, depending on whether it is given the "-P"
option.)

When Emacs is given the file name "../a/file", does the ".." mean to
go up one directory *textually* in the pathspec based on $PWD

"{topdir}/b/../a/file" -> "{topdir}/a/file" (which does not exist)

or according to the directory linkage on the disk (where the ".."
entry in the current directory goes to the directory named {topdir}/z,
which does contain a file a/file.  It looks like Emacs uses the first
method.

The decision of which implementation to use depends on the semantics
you *want*.  As I noted, bash allows the user to choose *either*
interpretation, which shows that both semantics are considered valid
(by some people).

Dale

Re: Difficulty adding a symbolic link, part 3

[Duy Nguyen]

On Thu, Aug 1, 2013 at 10:00 PM, Dale R. Worley <worley@alum.mit.edu> wrote:
>> From: Duy Nguyen <pclouds@gmail.com>
>
>> > 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.
>
> Thanks for the detailed information.  It seems to me that the minimum
> needed change is that the handling of relative and absolute paths
> should be made consistent.

Agreed.

>
>> [1] http://thread.gmane.org/gmane.comp.version-control.git/231268
>
> That problem isn't so much a matter of not resolving symlinks but
> rather the question of what ".." means.  In the case shown in that
> message, the current directory *is* {topdir}/z/b, but it was entered
> with "cd {topdir}/b" -- and the shell records the latter as the value
> of $PWD.  (Actually, the bash shell can handle symbolic-linked
> directories *either* way, depending on whether it is given the "-P"
> option.)
>
> When Emacs is given the file name "../a/file", does the ".." mean to
> go up one directory *textually* in the pathspec based on $PWD
>
> "{topdir}/b/../a/file" -> "{topdir}/a/file" (which does not exist)
>
> or according to the directory linkage on the disk (where the ".."
> entry in the current directory goes to the directory named {topdir}/z,
> which does contain a file a/file.  It looks like Emacs uses the first
> method.
>
> The decision of which implementation to use depends on the semantics
> you *want*.  As I noted, bash allows the user to choose *either*
> interpretation, which shows that both semantics are considered valid
> (by some people).

We pass paths around, the semantics must be one that others expect,
not what we want. For one, the kernel seems to resolve symlinks first,
then "..". We use chdir() and getcwd() provided by the kernel. We need
to agree with it on ".." semantics.
-- 
Duy