[PATCH] New :P history modifier.

[PATCH] New :P history modifier.

[Daniel Shahaf]

---
This is the realpath() modifier from the :A threads.  See users/21742 and
users/21762 for the "which letter to use" bikeshed.

It just uses the existing function xsymlink(); no stealing of code from
compatibly-licensed libraries was needed.

The new completion texts could be improved.

WDYT?

Cheers,

Daniel
(I'll wait for feedback before committing this)

 Completion/Base/Completer/_external_pwds |  2 +-
 Completion/Zsh/Type/_history_modifiers   |  5 +++--
 Doc/Zsh/contrib.yo                       |  2 +-
 Doc/Zsh/expn.yo                          |  9 +++++++++
 Functions/MIME/zsh-mime-handler          |  2 +-
 Functions/VCS_Info/VCS_INFO_quilt        |  2 +-
 Functions/Zle/expand-absolute-path       |  2 +-
 NEWS                                     | 11 +++++++++++
 Src/params.c                             |  2 +-
 Src/subst.c                              | 13 +++++++++++++
 Src/utils.c                              | 14 ++++++++------
 Test/D02glob.ztst                        |  8 ++++++++
 12 files changed, 58 insertions(+), 14 deletions(-)

diff --git a/Completion/Base/Completer/_external_pwds b/Completion/Base/Completer/_external_pwds
index 4ad50f0..79e3ba0 100644
--- a/Completion/Base/Completer/_external_pwds
+++ b/Completion/Base/Completer/_external_pwds
@@ -22,7 +22,7 @@ case $OSTYPE in
     )
   ;;
   linux*)
-    dirs=( /proc/${^$(pidof zsh):#$$}/cwd(N:A) )
+    dirs=( /proc/${^$(pidof zsh):#$$}/cwd(N:P) )
     dirs=( $^dirs(N^@) )
   ;;
   *)
diff --git a/Completion/Zsh/Type/_history_modifiers b/Completion/Zsh/Type/_history_modifiers
index 658f9f3..e25dc9a 100644
--- a/Completion/Zsh/Type/_history_modifiers
+++ b/Completion/Zsh/Type/_history_modifiers
@@ -64,8 +64,8 @@ while true; do
       )
     if (( ! global )); then
       list+=(
-	"a:absolute path"
-	"A:absolute path resolving symbolic links"
+	"a:absolute path, resolve '..' logically"
+	"A:same, then resolve symlinks"
 	"c:PATH search for command"
 	"g:globally apply s or &"
 	"h:head - strip trailing path element"
@@ -73,6 +73,7 @@ while true; do
 	"r:root - strip suffix"
 	"e:leave only extension"
 	"Q:strip quotes"
+	"P:realpath, resolve '..' physically"
 	"l:lower case all words"
 	"u:upper case all words"
 	)
diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
index 00ed080..63df292 100644
--- a/Doc/Zsh/contrib.yo
+++ b/Doc/Zsh/contrib.yo
@@ -3477,7 +3477,7 @@ will ensure that any files found in that area will be executed as MIME
 types even if they are executable.  As this example shows, the complete
 file name is matched against the pattern, regardless of how the file
 was passed to the handler.  The file is resolved to a full path using
-the tt(:A) modifier described in
+the tt(:P) modifier described in
 ifzman(the subsection Modifiers in zmanref(zshexpn))\
 ifnzman(noderef(Modifiers));
 this means that symbolic links are resolved where possible, so that
diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index ca4b94f..ecb1877 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -240,6 +240,7 @@ function, symbolic links are not resolved, so on those systems `tt(a)' and
 `tt(A)' are equivalent.
 
 Note: tt(foo:A) and tt(realpath+LPAR()foo+RPAR()) are different on some inputs.
+For tt(realpath+LPAR()foo+RPAR()) semantics, see the `tt(P)` modifier.
 )
 item(tt(c))(
 Resolve a command name into an absolute path by searching the command
@@ -265,6 +266,14 @@ item(tt(p))(
 Print the new command but do not execute it.  Only works with history
 expansion.
 )
+item(tt(P))(
+Turn a file name into an absolute path, like tt(realpath+LPAR()3+RPAR()).
+The resulting path will be absolute, have neither `tt(.)' nor `tt(..)' components,
+and refer to the same directory entry as the input filename.
+
+Unlike tt(realpath+LPAR()3+RPAR()), non-existent trailing components are
+permitted and preserved.
+)
 item(tt(q))(
 Quote the substituted words, escaping further substitutions.  Works
 with history expansion and parameter expansion, though for parameters
diff --git a/Functions/MIME/zsh-mime-handler b/Functions/MIME/zsh-mime-handler
index 24e5184..288a079 100644
--- a/Functions/MIME/zsh-mime-handler
+++ b/Functions/MIME/zsh-mime-handler
@@ -127,7 +127,7 @@ for pattern in $exec_asis; do
   files=(${dirpref}${~pattern})
   if [[ -n ${files[(r)$1]} ]]; then
     for pattern in $exec_never; do
-      [[ ${1:A} = ${~pattern} ]] && break 2
+      [[ ${1:P} = ${~pattern} ]] && break 2
     done
     if (( list )); then
       for (( i = 1; i <= $#; i++ )); do
diff --git a/Functions/VCS_Info/VCS_INFO_quilt b/Functions/VCS_Info/VCS_INFO_quilt
index e7cd89f..6adf0a3 100644
--- a/Functions/VCS_Info/VCS_INFO_quilt
+++ b/Functions/VCS_Info/VCS_INFO_quilt
@@ -170,7 +170,7 @@ function VCS_INFO_quilt() {
             applied=()
         fi
         patches=$(<$pc/.quilt_patches)
-        patches=`builtin cd -q "${pc:h}" && print -r - ${patches:A}`
+        patches=`builtin cd -q "${pc:h}" && print -r - ${patches:P}`
     fi
     if zstyle -t "${context}" get-unapplied; then
         # This zstyle call needs to be moved further up if `quilt' needs
diff --git a/Functions/Zle/expand-absolute-path b/Functions/Zle/expand-absolute-path
index b857576..4887f3c 100644
--- a/Functions/Zle/expand-absolute-path
+++ b/Functions/Zle/expand-absolute-path
@@ -10,7 +10,7 @@ autoload -Uz modify-current-argument
 if (( ! ${+functions[glob-expand-absolute-path]} )); then
   glob-expand-absolute-path() {
     local -a files
-    files=(${~1}(N:A))
+    files=(${~1}(N:P))
     (( ${#files} )) || return
     REPLY=${(D)files[1]}
   }
diff --git a/NEWS b/NEWS
index 15822ad..d676e40 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,17 @@ CHANGES FROM PREVIOUS VERSIONS OF ZSH
 
 Note also the list of incompatibilities in the README file.
 
+Changes from 5.2 to 5.3
+-----------------------
+
+The new word modifier ':P' computes the realpath() of the argument.
+It is different from the existing ':a' modifier which does not resolve
+symlinks, and from the existing ':A' modifier which always resolves
+/before/here/../after to /before/after --- even if /before/here is
+a symbolic link.  It is recommended to review uses of ':A' and, if
+appropriate, convert them to ':P' as soon as compatibility with 5.2 is
+no longer a requirement.
+
 Changes from 5.1.1 to 5.2
 -------------------------
 
diff --git a/Src/params.c b/Src/params.c
index 0eda784..842b2f0 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -4336,7 +4336,7 @@ void
 homesetfn(UNUSED(Param pm), char *x)
 {
     zsfree(home);
-    if (x && isset(CHASELINKS) && (home = xsymlink(x)))
+    if (x && isset(CHASELINKS) && (home = xsymlink(x, 0)))
 	zsfree(x);
     else
 	home = x ? x : ztrdup("");
diff --git a/Src/subst.c b/Src/subst.c
index e3af156..fe378b6 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -4070,6 +4070,7 @@ modify(char **str, char **ptr)
 	    case 'u':
 	    case 'q':
 	    case 'Q':
+	    case 'P':
 		c = **ptr;
 		break;
 
@@ -4276,6 +4277,12 @@ modify(char **str, char **ptr)
 			    untokenize(copy);
 			}
 			break;
+		    case 'P':
+			if (*copy != '/') {
+			    copy = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP), "/", copy);
+			}
+			copy = xsymlink(copy, 1);
+			break;
 		    }
 		    tc = *tt;
 		    *tt = '\0';
@@ -4352,6 +4359,12 @@ modify(char **str, char **ptr)
 			untokenize(*str);
 		    }
 		    break;
+		case 'P':
+		    if (**str != '/') {
+			*str = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP), "/", *str);
+		    }
+		    *str = xsymlink(*str, 1);
+		    break;
 		}
 	    }
 	    if (rec < 0) {
diff --git a/Src/utils.c b/Src/utils.c
index 0a5954f..45fd192 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -801,9 +801,9 @@ findpwd(char *s)
     char *t;
 
     if (*s == '/')
-	return xsymlink(s);
+	return xsymlink(s, 0);
     s = tricat((pwd[1]) ? pwd : "", "/", s);
-    t = xsymlink(s);
+    t = xsymlink(s, 0);
     zsfree(s);
     return t;
 }
@@ -969,11 +969,13 @@ xsymlinks(char *s, int full)
 /*
  * expand symlinks in s, and remove other weird things:
  * note that this always expands symlinks.
+ *
+ * 'heap' indicates whether to malloc() or allocate on the heap.
  */
 
 /**/
 char *
-xsymlink(char *s)
+xsymlink(char *s, int heap)
 {
     if (*s != '/')
 	return NULL;
@@ -981,8 +983,8 @@ xsymlink(char *s)
     if (xsymlinks(s + 1, 1) < 0)
 	zwarn("path expansion failed, using root directory");
     if (!*xbuf)
-	return ztrdup("/");
-    return ztrdup(xbuf);
+	return heap ? dupstring("/") : ztrdup("/");
+    return heap ? dupstring(xbuf) : ztrdup(xbuf);
 }
 
 /**/
@@ -1260,7 +1262,7 @@ getnameddir(char *name)
 	/* Retrieve an entry from the password table/database for this user. */
 	struct passwd *pw;
 	if ((pw = getpwnam(name))) {
-	    char *dir = isset(CHASELINKS) ? xsymlink(pw->pw_dir)
+	    char *dir = isset(CHASELINKS) ? xsymlink(pw->pw_dir, 0)
 		: ztrdup(pw->pw_dir);
 	    if (dir) {
 		adduserdir(name, dir, ND_USERNAME, 1);
diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index 7befbc2..1385d57 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -678,3 +678,11 @@
  rm glob.tmp/link
 0:modifier ':A' resolves '..' components before symlinks
 # There should be no output
+
+ ln -s dir3/subdir glob.tmp/link
+ () {
+   print ${1:P}
+ } glob.tmp/link/../../hello/world
+ rm glob.tmp/link
+0:modifier ':P' resolves symlinks before '..' components
+*>*glob.tmp/hello/world

Re: [PATCH] New :P history modifier.

[Bart Schaefer]

On Aug 16, 11:54pm, Daniel Shahaf wrote:
}
} WDYT?

The code changes are essentially OK; my only thought is, maybe we
should just remove the dependency on the POSIX realpath() call even
from :A, and use xsymlink() everywhere?

Picking at the docs ...

Given that we went to the trouble of hashing it out, it is probably
worth noting that :a is intended to result in the path to along which
"cd" would change under the default setopts (no_chase_dots), and :A
is meant to result in the physical directory at the end of that path.

} +	"a:absolute path, resolve '..' logically"
} +	"A:same, then resolve symlinks"
} +	"P:realpath, resolve '..' physically"

You should spell out what "same" means, because the two descriptions
may not always appear together.  I'm not sure "logical" and "physical"
are the right words here, but "by text replacement" and "by filesystem
reference" seem a bit too verbose, so I don't have a suggestion, just
calling attention.

(Aside:  The doc for "cd" doesn't ever describe how it behaves when
CHASE_DOTS is not set; the user is left to infer it from the doc that
explains what happens when CHASE_DOTS is set.)

} +The new word modifier ':P' computes the realpath() of the argument.
} +It is different from the existing ':a' modifier which does not resolve
} +symlinks, and from the existing ':A' modifier which always resolves
   ^^^^^^^^

If you're going to compare to both :a and :A, symlinks aren't the largest
difference vs. :a -- following ".." is.  In fact I'd say following ".."
is more important to the distinction than symlinks are.

} +/before/here/../after to /before/after --- even if /before/here is
} +a symbolic link.  It is recommended to review uses of ':A' and, if
} +appropriate, convert them to ':P' as soon as compatibility with 5.2 is
} +no longer a requirement.

So how about e.g.:

 The new word modifier ':P' computes the physical path of the argument.
 it is different from the existing ':a' modifier which does always
 resolves /before/here/../after to /before/after, and differs from the
 existing ':A' modifier which resolves symlinks only after here/.. is
 removed even when /before/here is itself a symbolic link.

That's all.

Re: [PATCH] New :P history modifier.

[Daniel Shahaf]

Bart Schaefer wrote on Wed, Aug 17, 2016 at 00:02:40 -0700:
> On Aug 16, 11:54pm, Daniel Shahaf wrote:
> }
> } WDYT?
> 
> The code changes are essentially OK; my only thought is, maybe we
> should just remove the dependency on the POSIX realpath() call even
> from :A, and use xsymlink() everywhere?
> 

Two thoughts:

- Does anyone actually build zsh on a system that doesn't have realpath()?

- xsymlink() is not a drop-in replacement: it tolerates trailing
  non-existing path components.  The single callsite in current master
  wouldn't care, though.

> Picking at the docs ...
> 
> Given that we went to the trouble of hashing it out, it is probably
> worth noting that :a is intended to result in the path to along which
> "cd" would change under the default setopts (no_chase_dots),

38945 made such a change; do you think further changes are needed?

> and :A is meant to result in the physical directory at the end of that
> path.

No wording comes to mind, here.

> 
> } +	"a:absolute path, resolve '..' logically"
> } +	"A:same, then resolve symlinks"
> } +	"P:realpath, resolve '..' physically"
> 
> I'm not sure "logical" and "physical"
> are the right words here, but "by text replacement" and "by filesystem
> reference" seem a bit too verbose, so I don't have a suggestion, just
> calling attention.

They might work as parentheticals: "a:... logically (by text
replacement)" and "P:... physically (by filesystem lookup)"?

Some other options:

- :a   syntactically / lexically / "like 'cd'"
- :P   semantically / "like stat(2)"

(yes, "like 'cd'" is inaccurate if CHASE_* are set)

> You should spell out what "same" means, because the two descriptions
> may not always appear together.

I'll change "same" to "as ':a'".

I wonder if there's a way to make the :A completion text convey "Unless
you're trying to predict what 'cd' is about to do, you probably want :P,
not :A"...

> } +The new word modifier ':P' computes the realpath() of the argument.
> } +It is different from the existing ':a' modifier which does not resolve
> } +symlinks, and from the existing ':A' modifier which always resolves
>    ^^^^^^^^
> 
> If you're going to compare to both :a and :A, symlinks aren't the largest
> difference vs. :a -- following ".." is.  In fact I'd say following ".."
> is more important to the distinction than symlinks are.
>
> } +/before/here/../after to /before/after --- even if /before/here is
> } +a symbolic link.  It is recommended to review uses of ':A' and, if
> } +appropriate, convert them to ':P' as soon as compatibility with 5.2 is
> } +no longer a requirement.
> 
> So how about e.g.:
> 
>  The new word modifier ':P' computes the physical path of the argument.
>  it is different from the existing ':a' modifier which does always
>  resolves /before/here/../after to /before/after, and differs from the
>  existing ':A' modifier which resolves symlinks only after here/.. is
>  removed even when /before/here is itself a symbolic link.
> 

Looks good to me.

What about the "It is recommended [to audit uses of :A and change them
to :P]" sentence that the original patch had, should it be kept or
removed?

> That's all.

Thanks for the review!

Daniel

Re: [PATCH] New :P history modifier.

[Bart Schaefer]

On Aug 17,  4:31pm, Daniel Shahaf wrote:
}
} - xsymlink() is not a drop-in replacement: it tolerates trailing
}   non-existing path components.  The single callsite in current master
}   wouldn't care, though.

Pretty much my point; if there's only one call and they're equivalent
for that instance, why leave the special case?
 
} > Picking at the docs ...
} > 
} > Given that we went to the trouble of hashing it out, it is probably
} > worth noting that :a is intended to result in the path to along which
} > "cd" would change under the default setopts (no_chase_dots),
} 
} 38945 made such a change; do you think further changes are needed?

Nah, that's probably good enough.

} > } +	"a:absolute path, resolve '..' logically"
} > } +	"A:same, then resolve symlinks"
} > } +	"P:realpath, resolve '..' physically"
} > 
} > I'm not sure "logical" and "physical"

"lexical" and "physical" are probably good enough, given the mnemonic
helpfulness (P -> physical, a -> absolute).

} >  The new word modifier ':P' computes the physical path of the argument.
} >  it is different from the existing ':a' modifier which does always
} >  resolves /before/here/../after to /before/after, and differs from the
} >  existing ':A' modifier which resolves symlinks only after here/.. is
} >  removed even when /before/here is itself a symbolic link.
} 
} What about the "It is recommended [to audit uses of :A and change them
} to :P]" sentence that the original patch had, should it be kept or
} removed?

IMO it can be kept.

-- 
Barton E. Schaefer

Re: [PATCH] New :P history modifier.

[Daniel Shahaf]

Bart Schaefer wrote on Wed, Aug 17, 2016 at 10:06:14 -0700:
> On Aug 17,  4:31pm, Daniel Shahaf wrote:
> }
> } - xsymlink() is not a drop-in replacement: it tolerates trailing
> }   non-existing path components.  The single callsite in current master
> }   wouldn't care, though.
> 
> Pretty much my point; if there's only one call and they're equivalent
> for that instance, why leave the special case?

*nod*

> } >  The new word modifier ':P' computes the physical path of the argument.
> } >  it is different from the existing ':a' modifier which does always
> } >  resolves /before/here/../after to /before/after, and differs from the
> } >  existing ':A' modifier which resolves symlinks only after here/.. is
> } >  removed even when /before/here is itself a symbolic link.
> } 
> } What about the "It is recommended [to audit uses of :A and change them
> } to :P]" sentence that the original patch had, should it be kept or
> } removed?
> 
> IMO it can be kept.

Kept.  The interdiff so far follows.

Thanks again for the review,

Daniel

diff --git a/Completion/Zsh/Type/_history_modifiers b/Completion/Zsh/Type/_history_modifiers
index 5141d80..1a049d6 100644
--- a/Completion/Zsh/Type/_history_modifiers
+++ b/Completion/Zsh/Type/_history_modifiers
@@ -64,8 +64,8 @@ while true; do
       )
     if (( ! global )); then
       list+=(
-	"a:absolute path, resolve '..' logically"
-	"A:same, then resolve symlinks"
+	"a:absolute path, resolve '..' lexically"
+	"A:as ':a', then resolve symlinks"
 	"c:PATH search for command"
 	"g:globally apply s or &"
 	"h:head - strip trailing path element"
diff --git a/NEWS b/NEWS
index d676e40..65b246d 100644
--- a/NEWS
+++ b/NEWS
@@ -7,13 +7,13 @@ Note also the list of incompatibilities in the README file.
 Changes from 5.2 to 5.3
 -----------------------
 
-The new word modifier ':P' computes the realpath() of the argument.
-It is different from the existing ':a' modifier which does not resolve
-symlinks, and from the existing ':A' modifier which always resolves
-/before/here/../after to /before/after --- even if /before/here is
-a symbolic link.  It is recommended to review uses of ':A' and, if
-appropriate, convert them to ':P' as soon as compatibility with 5.2 is
-no longer a requirement.
+The new word modifier ':P' computes the physical path of the argument.
+It is different from the existing ':a' modifier which always resolves
+'/before/here/../after' to '/before/after', and differs from the
+existing ':A' modifier which resolves symlinks only after 'here/..' is
+removed, even when /before/here is itself a symbolic link.  It is
+recommended to review uses of ':A' and, if appropriate, convert them
+to ':P' as soon as compatibility with 5.2 is no longer a requirement.
 
 Changes from 5.1.1 to 5.2
 -------------------------