{"thread":{"id":"34578","subject":"Difficulty adding a symbolic link, part 3","startedAt":"2013-07-31T20:29:04Z","lastAt":"2013-08-02T01:54:42Z","messageCount":5,"participants":["Dale R. Worley","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"224386","messageId":"201307312029.r6VKT4vC023100@hobgoblin.ariadne.com","threadId":"34578","inReplyTo":null,"subject":"Difficulty adding a symbolic link, part 3","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-07-31T20:29:04Z","receivedAt":"2013-07-31T20:29:04Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"I've run into a problem (with Git 1.8.3.3) where I cannot add a\nsymbolic link (as such) to the repository *if* its path is given\nabsolutely; instead Git adds the file the symbolic link points to.\n(If I give the path relatively, Git does what I expect, that is, adds\nthe symbolic link.)\n\nI've written a test script that shows the problem and included it\nbelow.\n\nI would not expect *how* a path is presented to Git to change how Git\nprocesses the path.  In the test case, I would expect \"/bin/awk\" and\n\"../../bin/awk\" to produce the same effect when used as arguments to\n\"git add\".\n\nWhat is going on in the code is this:  In \"git add\", all paths are\nnormalized by the function prefix_path_gently() in abspath.c.  That\nfunction removes symbolic links from the pathspec *only if* it is\nabsolute, as shown in the first few lines of the function:\n\n static char *prefix_path_gently(const char *prefix, int len, const char *path)\n {\n\t const char *orig = path;\n\t char *sanitized;\n\t if (is_absolute_path(orig)) {\n-\t         const char *temp = real_path(path);\n+\t         const char *temp = absolute_path(path);\n\t\t sanitized = xmalloc(len + strlen(temp) + 1);\n\t\t strcpy(sanitized, temp);\n\t } else {\n\nreal_path() is specified to remove symbolic links.  As shown, I've\nreplaced real_path() with absolute_path(), based on the comment at the\ntop of real_path():\n\n/*\n * Return the real path (i.e., absolute path, with symlinks resolved\n * and extra slashes removed) equivalent to the specified path.  (If\n * you want an absolute path but don't mind links, use\n * absolute_path().)  The return value is a pointer to a static\n * buffer.\n *\n\nIf I replace real_path() with absolute_path() as shown, the problem I\nam testing for disappears.\n\nWith the above change, the test suite runs with zero failures, so it\ndoesn't affect any common Git usage.\n\nBut I don't know enough about the internal architecture of Git to know\nthat my change is correct in all cases.  I'm almost certain that the\nnormalization process for pathspecs should *not* normalize a final\ncomponent that is a symbolic link.  But I would expect it would be\ndesirable to normalize non-final components that are symbolic links.\nOn the other hand, that might not matter.\n\nCan someone give me advice on what this code *should* do?\n\nI believe I can prepare a proper test for the test suite for this, so\nonce I know what the code change should be, I can prepare a proper\npatch for it.\n\nDale\n----------------------------------------------------------------------\t\t\nHere's a test case for adding a symbolic link.  This test exploits the\nfact that on my system, /bin/awk is a symbolic link to \"gawk\".  As you\ncan see, the behavior of Git differs if the link's path is given to\n\"git add\" as an absolute path or a relative path.\n\nHere is the test script:\n----------------------------------------------------------------------\n#! /bin/bash\n\n# Illustrates a problem with applying \"git add\" to a symbolic link.\n\nset -x\n\n# To be run from a directory one step below the root directory.  E.g.,\n# \"/tmp\".\n# On this system, /bin/awk is a symbolic link to \"gawk\", which\n# means /tmp/gawk.\n\n# Show the Git version.\ngit version\n\n# Make a test directory and cd to it.\nDIR=temp.$$\nmkdir $DIR\ncd $DIR\n\n# Create a Git repository.\ngit init\n# Set the worktree to be /\ngit config core.worktree /\n# Create an empty commit.\ngit commit --allow-empty -m Empty.\n\n# Add the symbolic link using its absolute name.\nABSOLUTE=/bin/awk\nls -l $ABSOLUTE\ngit add $ABSOLUTE\n# Notice that the target of the link is added, not the link itself.\ngit status -uno\n\n# Reset the index.\ngit reset\n\n# Add the symbolic link using its relative name.\n# Remember that we are two directory levels below the root directory now.\nRELATIVE=../..$ABSOLUTE\nls -l $RELATIVE\ngit add $RELATIVE\n# Notice that now the link itself is added.\ngit status -uno\n----------------------------------------------------------------------\nHere is sample output of the script:\n----------------------------------------------------------------------\n+ git version\ngit version 1.8.3.3.756.g07a2553.dirty\n+ DIR=temp.22366\n+ mkdir temp.22366\n+ cd temp.22366\n+ git init\nInitialized empty Git repository in /git-add-link/temp.22366/.git/\n+ git config core.worktree /\n+ git commit --allow-empty -m Empty.\n[master (root-commit) fb232e5] Empty.\n+ ABSOLUTE=/bin/awk\n+ ls -l /bin/awk\nlrwxrwxrwx. 1 root root 4 Nov  2  2012 /bin/awk -> gawk\n+ git add /bin/awk\n+ git status -uno\n# On branch master\n# Changes to be committed:\n#   (use \"git reset HEAD <file>...\" to unstage)\n#\n#\tnew file:   ../../bin/gawk\n#\n# Untracked files not listed (use -u option to show untracked files)\n+ git reset\n+ RELATIVE=../../bin/awk\n+ ls -l ../../bin/awk\nlrwxrwxrwx. 1 root root 4 Nov  2  2012 ../../bin/awk -> gawk\n+ git add ../../bin/awk\n+ git status -uno\n# On branch master\n# Changes to be committed:\n#   (use \"git reset HEAD <file>...\" to unstage)\n#\n#\tnew file:   ../../bin/awk\n#\n# Untracked files not listed (use -u option to show untracked files)\n----------------------------------------------------------------------\n\nDale\n"},{"id":"224413","messageId":"CACsJy8DZ11zFEgL_vZ-kG2tuwodU=vBJZqZxUPWtYh9u8o-_Tg@mail.gmail.com","threadId":"34578","inReplyTo":"201307312029.r6VKT4vC023100@hobgoblin.ariadne.com","subject":"Re: Difficulty adding a symbolic link, part 3","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-01T01:17:08Z","receivedAt":"2013-08-01T01:17:08Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 1, 2013 at 3:29 AM, Dale R. Worley <worley@alum.mit.edu> wrote:\n> I've run into a problem (with Git 1.8.3.3) where I cannot add a\n> symbolic link (as such) to the repository *if* its path is given\n> absolutely; instead Git adds the file the symbolic link points to.\n> (If I give the path relatively, Git does what I expect, that is, adds\n> the symbolic link.)\n>\n> I've written a test script that shows the problem and included it\n> below.\n>\n> I would not expect *how* a path is presented to Git to change how Git\n> processes the path.  In the test case, I would expect \"/bin/awk\" and\n> \"../../bin/awk\" to produce the same effect when used as arguments to\n> \"git add\".\n>\n> What is going on in the code is this:  In \"git add\", all paths are\n> normalized by the function prefix_path_gently() in abspath.c.  That\n> function removes symbolic links from the pathspec *only if* it is\n> absolute, as shown in the first few lines of the function:\n>\n>  static char *prefix_path_gently(const char *prefix, int len, const char *path)\n>  {\n>          const char *orig = path;\n>          char *sanitized;\n>          if (is_absolute_path(orig)) {\n> -                const char *temp = real_path(path);\n> +                const char *temp = absolute_path(path);\n>                  sanitized = xmalloc(len + strlen(temp) + 1);\n>                  strcpy(sanitized, temp);\n>          } else {\n>\n> real_path() is specified to remove symbolic links.  As shown, I've\n> replaced real_path() with absolute_path(), based on the comment at the\n> top of real_path():\n>\n> /*\n>  * Return the real path (i.e., absolute path, with symlinks resolved\n>  * and extra slashes removed) equivalent to the specified path.  (If\n>  * you want an absolute path but don't mind links, use\n>  * absolute_path().)  The return value is a pointer to a static\n>  * buffer.\n>  *\n>\n> If I replace real_path() with absolute_path() as shown, the problem I\n> am testing for disappears.\n\nI think it also reverts 18e051a (setup: translate symlinks in filename\nwhen using absolute paths - 2010-12-27). real_path() (or\nmake_absolute_path() back then) is added to resolve symlinks, at least\nones leading to the work tree, not ones inside the work tree, if I\nunderstand the commit message correctly.\n\n>\n> With the above change, the test suite runs with zero failures, so it\n> doesn't affect any common Git usage.\n\nIt means the test suite is incomplete. As you can see, the commit\nintroducing this change does not come with a test case to catch people\nchanging this.\n\n>\n> But I don't know enough about the internal architecture of Git to know\n> that my change is correct in all cases.  I'm almost certain that the\n> normalization process for pathspecs should *not* normalize a final\n> component that is a symbolic link.  But I would expect it would be\n> desirable to normalize non-final components that are symbolic links.\n> On the other hand, that might not matter.\n>\n> Can someone give me advice on what this code *should* do?\n\nIt does as the function name says: given cwd, a prefix (i.e. a\nrelative path with no \"..\" components) and a path relative to\ncwd+prefix, convert 'path' to something relative to cwd. In the\nsimplest case, prepending the prefix to 'path' is enough. cwd is also\nget_git_work_tree().\n\nI agree with you that this code should not resolve anything in the\ncomponents after 'cwd', after rebasing the path to 'cwd' (not just the\nfinal component). Not sure how to do it correctly though because we do\nneed to resolve symlinks before cwd. Maybe a new variant of real_path\nthat stops at 'cwd'?\n\nWe may also have problems with resolve symlinks before cwd when 'path'\nis relative, as normalize_path_copy() does not resolve symlinks. We\njust found out emacs has this bug [1] but did not realize we also have\none :-P.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/231268\n\n>\n> I believe I can prepare a proper test for the test suite for this, so\n> once I know what the code change should be, I can prepare a proper\n> patch for it.\n>\n> Dale\n-- \nDuy\n"},{"id":"224417","messageId":"201308011356.r71DuKUK007167@freeze.ariadne.com","threadId":"34578","inReplyTo":"CACsJy8DZ11zFEgL_vZ-kG2tuwodU=vBJZqZxUPWtYh9u8o-_Tg@mail.gmail.com","subject":"Re: Difficulty adding a symbolic link, part 3","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-01T13:56:20Z","receivedAt":"2013-08-01T13:56:20Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Duy Nguyen <pclouds@gmail.com>\n\n> > With the above change, the test suite runs with zero failures, so it\n> > doesn't affect any common Git usage.\n> \n> It means the test suite is incomplete. As you can see, the commit\n> introducing this change does not come with a test case to catch people\n> changing this.\n\nWho should be blamed for omitting the test?\n\n> > Can someone give me advice on what this code *should* do?\n> \n> It does as the function name says: given cwd, a prefix (i.e. a\n> relative path with no \"..\" components) and a path relative to\n> cwd+prefix, convert 'path' to something relative to cwd. In the\n> simplest case, prepending the prefix to 'path' is enough. cwd is also\n> get_git_work_tree().\n\nBut as you can see, the exact behavior that the function is intended\nto exhibit regarding symlinks is not clear from the function name;\nthere should have been a real explanation in the comment above the\nfunction.\n\nDale\n"},{"id":"224418","messageId":"201308011500.r71F03HJ008579@freeze.ariadne.com","threadId":"34578","inReplyTo":"CACsJy8DZ11zFEgL_vZ-kG2tuwodU=vBJZqZxUPWtYh9u8o-_Tg@mail.gmail.com","subject":"Re: Difficulty adding a symbolic link, part 3","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-01T15:00:03Z","receivedAt":"2013-08-01T15:00:03Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Duy Nguyen <pclouds@gmail.com>\n\n> > Can someone give me advice on what this code *should* do?\n> \n> It does as the function name says: given cwd, a prefix (i.e. a\n> relative path with no \"..\" components) and a path relative to\n> cwd+prefix, convert 'path' to something relative to cwd. In the\n> simplest case, prepending the prefix to 'path' is enough. cwd is also\n> get_git_work_tree().\n> \n> I agree with you that this code should not resolve anything in the\n> components after 'cwd', after rebasing the path to 'cwd' (not just the\n> final component). Not sure how to do it correctly though because we do\n> need to resolve symlinks before cwd. Maybe a new variant of real_path\n> that stops at 'cwd'?\n> \n> We may also have problems with resolve symlinks before cwd when 'path'\n> is relative, as normalize_path_copy() does not resolve symlinks. We\n> just found out emacs has this bug [1] but did not realize we also have\n> one :-P.\n\nThanks for the detailed information.  It seems to me that the minimum\nneeded change is that the handling of relative and absolute paths\nshould be made consistent.\n\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/231268\n\nThat problem isn't so much a matter of not resolving symlinks but\nrather the question of what \"..\" means.  In the case shown in that\nmessage, the current directory *is* {topdir}/z/b, but it was entered\nwith \"cd {topdir}/b\" -- and the shell records the latter as the value\nof $PWD.  (Actually, the bash shell can handle symbolic-linked\ndirectories *either* way, depending on whether it is given the \"-P\"\noption.)\n\nWhen Emacs is given the file name \"../a/file\", does the \"..\" mean to\ngo up one directory *textually* in the pathspec based on $PWD\n\n\"{topdir}/b/../a/file\" -> \"{topdir}/a/file\" (which does not exist)\n\nor according to the directory linkage on the disk (where the \"..\"\nentry in the current directory goes to the directory named {topdir}/z,\nwhich does contain a file a/file.  It looks like Emacs uses the first\nmethod.\n\nThe decision of which implementation to use depends on the semantics\nyou *want*.  As I noted, bash allows the user to choose *either*\ninterpretation, which shows that both semantics are considered valid\n(by some people).\n\nDale\n"},{"id":"224438","messageId":"CACsJy8BSRE9L+K1BA6XUsaMviLxSEGGZsuj23J-AoYC2OxputQ@mail.gmail.com","threadId":"34578","inReplyTo":"201308011500.r71F03HJ008579@freeze.ariadne.com","subject":"Re: Difficulty adding a symbolic link, part 3","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-08-02T01:54:42Z","receivedAt":"2013-08-02T01:54:42Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 1, 2013 at 10:00 PM, Dale R. Worley <worley@alum.mit.edu> wrote:\n>> From: Duy Nguyen <pclouds@gmail.com>\n>\n>> > Can someone give me advice on what this code *should* do?\n>>\n>> It does as the function name says: given cwd, a prefix (i.e. a\n>> relative path with no \"..\" components) and a path relative to\n>> cwd+prefix, convert 'path' to something relative to cwd. In the\n>> simplest case, prepending the prefix to 'path' is enough. cwd is also\n>> get_git_work_tree().\n>>\n>> I agree with you that this code should not resolve anything in the\n>> components after 'cwd', after rebasing the path to 'cwd' (not just the\n>> final component). Not sure how to do it correctly though because we do\n>> need to resolve symlinks before cwd. Maybe a new variant of real_path\n>> that stops at 'cwd'?\n>>\n>> We may also have problems with resolve symlinks before cwd when 'path'\n>> is relative, as normalize_path_copy() does not resolve symlinks. We\n>> just found out emacs has this bug [1] but did not realize we also have\n>> one :-P.\n>\n> Thanks for the detailed information.  It seems to me that the minimum\n> needed change is that the handling of relative and absolute paths\n> should be made consistent.\n\nAgreed.\n\n>\n>> [1] http://thread.gmane.org/gmane.comp.version-control.git/231268\n>\n> That problem isn't so much a matter of not resolving symlinks but\n> rather the question of what \"..\" means.  In the case shown in that\n> message, the current directory *is* {topdir}/z/b, but it was entered\n> with \"cd {topdir}/b\" -- and the shell records the latter as the value\n> of $PWD.  (Actually, the bash shell can handle symbolic-linked\n> directories *either* way, depending on whether it is given the \"-P\"\n> option.)\n>\n> When Emacs is given the file name \"../a/file\", does the \"..\" mean to\n> go up one directory *textually* in the pathspec based on $PWD\n>\n> \"{topdir}/b/../a/file\" -> \"{topdir}/a/file\" (which does not exist)\n>\n> or according to the directory linkage on the disk (where the \"..\"\n> entry in the current directory goes to the directory named {topdir}/z,\n> which does contain a file a/file.  It looks like Emacs uses the first\n> method.\n>\n> The decision of which implementation to use depends on the semantics\n> you *want*.  As I noted, bash allows the user to choose *either*\n> interpretation, which shows that both semantics are considered valid\n> (by some people).\n\nWe pass paths around, the semantics must be one that others expect,\nnot what we want. For one, the kernel seems to resolve symlinks first,\nthen \"..\". We use chdir() and getcwd() provided by the kernel. We need\nto agree with it on \"..\" semantics.\n-- \nDuy\n"}]}