[PATCH 1/3] Tests: Add tests for the ':a' and ':A' modifiers.

[PATCH 1/3] Tests: Add tests for the ':a' and ':A' modifiers.

[Daniel Shahaf]

---
 Test/D02glob.ztst | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index a6b704a..8618378 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -654,4 +654,19 @@
   [[ "z" != [$~cset] ]] || print Fail 4
   [[ "1" = [$~cset] ]] || print Fail 5
   [[ "b" != [$~cset] ]] || print Fail 6
-0:character set specified as active variabe
+0:character set specified as active variable
+
+ () { print -l -- $@:a } / /..{,/} /1 /nonexistent/..{,/} /deeper/nonexistent/..{,/}
+0:modifier ':a' doesn't require existence
+>/
+>/
+>/
+>/1
+>/
+>/
+>/deeper
+>/deeper
+
+ () { set -- ${PWD}/$^@; print -l -- $@:A } glob.tmp/nonexistent/foo/bar/baz
+0:modifier ':A' doesn't require existence
+*>*/glob.tmp/nonexistent/foo/bar/baz

[PATCH 2/3] Fix the ':A' word modifier on paths with '..' components.

[Daniel Shahaf]

The fix is to stop calling chabspath() at the top of chrealpath().

Also remove an incorrect comment (passing a non-absolute path would have been
fine because the chabspath() call would have made it absolute).
---

This is an incompatible change; see the test and docs changes for details.

Daniel

 Doc/Zsh/expn.yo   |  6 ++++--
 README            |  9 +++++++++
 Src/hist.c        | 22 ++++++++++------------
 Test/D02glob.ztst |  6 ++++++
 4 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index c6e7b6f..50b8479 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -225,9 +225,11 @@ intervening directories do not exist.
 )
 item(tt(A))(
 As `tt(a)', but also resolve use of symbolic links where possible.
-Note that resolution of `tt(..)' occurs em(before) resolution of symbolic
-links.  This call is equivalent to tt(a) unless your system has the
+This call is equivalent to tt(a) unless your system has the
 tt(realpath) system call (modern systems do).
+
+em(Note): In zsh 5.2 and earlier, resolution of `tt(..)' occurred em(before)
+resolution of symbolic links.
 )
 item(tt(c))(
 Resolve a command name into an absolute path by searching the command
diff --git a/README b/README
index d5343db..84bb6bc 100644
--- a/README
+++ b/README
@@ -79,6 +79,15 @@ Other aspects of EXIT trap handling have not changed --- there is still
 only one EXIT trap at any point in a programme, so it is not generally
 useful to combine POSIX and non-POSIX behaviour in the same script.
 
+4) On systems that have the realpath(3) library function, the ':A' word
+modifier now resolves symbolic links before '..' path components.  This
+could lead to different, but usually more desirable, results: the
+tranformed value will now always identify the same directory entry as the
+the pre-transformation value.
+
+The behaviour of 5.2 and older can be achieved by chaining modifiers:
+'<expression>:a:A'.
+
 Incompatibilities between 5.0.8 and 5.2
 ---------------------------------------
 
diff --git a/Src/hist.c b/Src/hist.c
index 5fc40bd..07f56b0 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1837,8 +1837,8 @@ chabspath(char **junkptr)
 int
 chrealpath(char **junkptr)
 {
-    char *str;
 #ifdef HAVE_REALPATH
+    char *str;
 # ifdef REALPATH_ACCEPTS_NULL
     char *lastpos, *nonreal, *real;
 # else
@@ -1850,19 +1850,17 @@ chrealpath(char **junkptr)
     if (!**junkptr)
 	return 1;
 
-    /* Notice that this means ..'s are applied before symlinks are resolved! */
-    if (!chabspath(junkptr))
-	return 0;
-
 #ifndef HAVE_REALPATH
-    return 1;
+    return chabspath(junkptr);
 #else
-    /*
-     * Notice that this means you cannot pass relative paths into this
-     * function!
-     */
-    if (**junkptr != '/')
+
+    if (**junkptr != '/') {
+	*junkptr = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP), "/", *junkptr);
+    }
+    if (**junkptr != '/') {
+	/* Can happen after 'rmdir $PWD; zsh' */
 	return 0;
+    }
 
     unmetafy(*junkptr, NULL);
 
@@ -1909,9 +1907,9 @@ chrealpath(char **junkptr)
     } else {
 	*junkptr = metafy(nonreal, lastpos - nonreal + 1, META_HEAPDUP);
     }
-#endif
 
     return 1;
+#endif
 }
 
 /**/
diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index 8618378..dc1a655 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -670,3 +670,9 @@
  () { set -- ${PWD}/$^@; print -l -- $@:A } glob.tmp/nonexistent/foo/bar/baz
 0:modifier ':A' doesn't require existence
 *>*/glob.tmp/nonexistent/foo/bar/baz
+
+ ln -s dir3/subdir glob.tmp/link
+ () { print ${1:A} } glob.tmp/link/../../hello
+ rm glob.tmp/link
+0:modifier ':A' resolves symlinks before '..' components
+*>*glob.tmp/hello

[PATCH 3/3] Clean up chabspath() [':a' word modifier].

[Daniel Shahaf]

Fail fast when the first character isn't "/", and use that to simplify the
'..' handling.

The incumbent code was subtly wrong in various ways (for example,
the first 'else if' branch didn't check that dest[-4] was a slash).
---
 Src/hist.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/Src/hist.c b/Src/hist.c
index 07f56b0..2cce970 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1767,6 +1767,10 @@ chabspath(char **junkptr)
     if (**junkptr != '/') {
 	*junkptr = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP), "/", *junkptr);
     }
+    if (**junkptr != '/') {
+	/* Can happen after 'rmdir $PWD; zsh' */
+	return 0;
+    }
 
     current = *junkptr;
     dest = *junkptr;
@@ -1783,7 +1787,9 @@ chabspath(char **junkptr)
 
     for (;;) {
 	if (*current == '/') {
+	    /* Contract multiple slashes. */
 #ifdef __CYGWIN__
+	    /* Exception: on Cygwin, two slashes at the very start are significant. */
 	    if (current == *junkptr && current[1] == '/')
 		*dest++ = *current++;
 #endif
@@ -1791,28 +1797,17 @@ chabspath(char **junkptr)
 	    while (*current == '/')
 		current++;
 	} else if (!*current) {
+	    /* Remove trailing slashes. */
 	    while (dest > *junkptr + 1 && dest[-1] == '/')
 		dest--;
 	    *dest = '\0';
 	    break;
 	} else if (current[0] == '.' && current[1] == '.' &&
 		   (!current[2] || current[2] == '/')) {
-		if (current == *junkptr || dest == *junkptr) {
-		    *dest++ = '.';
-		    *dest++ = '.';
-		    current += 2;
-		} else if (dest > *junkptr + 2 &&
-			   !strncmp(dest - 3, "../", 3)) {
-		    *dest++ = '.';
-		    *dest++ = '.';
-		    current += 2;
-		} else if (dest > *junkptr + 1) {
-		    *dest = '\0';
+		if (dest > *junkptr + 1) {
 		    for (dest--;
-			 dest > *junkptr + 1 && dest[-1] != '/';
+			 dest[-1] != '/';
 			 dest--);
-		    if (dest[-1] != '/')
-			dest--;
 		    current += 2;
 		    if (*current == '/')
 			current++;

Re: [PATCH 2/3] Fix the ':A' word modifier on paths with '..' components.

[Mikael Magnusson]

On Fri, Jun 10, 2016 at 7:36 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> The fix is to stop calling chabspath() at the top of chrealpath().
>
> Also remove an incorrect comment (passing a non-absolute path would have been
> fine because the chabspath() call would have made it absolute).
> ---
>
> This is an incompatible change; see the test and docs changes for details.

This seems pretty controversial, but I can see people wanting it
either way. See for example the chasedots / chaselinks options,
allowing the user to select which of these behaviors to use for the cd
builtin. I don't think this should be pushed without more discussion
at least.

-- 
Mikael Magnusson

Re: [PATCH 2/3] Fix the ':A' word modifier on paths with '..' components.

[Bart Schaefer]

On Jun 10,  8:54pm, Mikael Magnusson wrote:
}
} This seems pretty controversial, but I can see people wanting it
} either way. See for example the chasedots / chaselinks options,

Perhaps :A should honor those options.  Of course that would make it
marginally more difficult for script writers to reliably use :A, but
both of those options are reset in every "emulate" mode.

Re: [PATCH 2/3] Fix the ':A' word modifier on paths with '..' components.

[Daniel Shahaf]

Bart Schaefer wrote on Fri, Jun 10, 2016 at 12:46:23 -0700:
> On Jun 10,  8:54pm, Mikael Magnusson wrote:
> }
> } This seems pretty controversial, but I can see people wanting it
> } either way. See for example the chasedots / chaselinks options,
> 
> Perhaps :A should honor those options.  Of course that would make it
> marginally more difficult for script writers to reliably use :A, but
> both of those options are reset in every "emulate" mode.

So, how about:

- :a remains as it is today
- :A when CHASE_DOTS set: same as :A today (resolves '..' first and
  symlinks second)
- :A when CHASE_DOTS unset: as in the patch: equivalent to realpath(3)
  except that non-existing trailing path components are permitted

Would that address everyone's concerns?

Note this doesn't use CHASE_LINKS at all, neither in :A nor in :a.

Mikael: I wasn't planning to push this until everyone has had ample
opportunity to comment on the proposed change.

Cheers,

Daniel

Re: [PATCH 2/3] Fix the ':A' word modifier on paths with '..' components.

[Bart Schaefer]

On Jun 12,  2:28pm, Daniel Shahaf wrote:
}
} So, how about:
} 
} - :a remains as it is today
} - :A when CHASE_DOTS set: same as :A today (resolves '..' first and
}   symlinks second)
} - :A when CHASE_DOTS unset: as in the patch: equivalent to realpath(3)
}   except that non-existing trailing path components are permitted
} 
} Would that address everyone's concerns?

I believe it would resolve most of mine.  :A is a relatively new feature
so it would be understandable for it to evolve a bit.  However, it is in
use in a few places already (e.g., expand-absolute-path, VCS_INFO_quilt,
zsh-mime-handler).  I don't think any of those uses would be compromised
by this change, but if anyone wants to do a more thorough search for use
cases, don't be shy.

} Note this doesn't use CHASE_LINKS at all, neither in :A nor in :a.

Makes sense; :a plus CHASE_LINKS would be :A.

Re: [PATCH 2/3] Fix the ':A' word modifier on paths with '..' components.

[Daniel Shahaf]

Bart Schaefer wrote on Sun, Jun 12, 2016 at 09:49:16 -0700:
> On Jun 12,  2:28pm, Daniel Shahaf wrote:
> }
> } So, how about:
> } 
> } - :a remains as it is today
> } - :A when CHASE_DOTS set: same as :A today (resolves '..' first and
> }   symlinks second)
> } - :A when CHASE_DOTS unset: as in the patch: equivalent to realpath(3)
> }   except that non-existing trailing path components are permitted
> } 
> } Would that address everyone's concerns?
> 
> I believe it would resolve most of mine.  :A is a relatively new feature
> so it would be understandable for it to evolve a bit.  However, it is in
> use in a few places already (e.g., expand-absolute-path, VCS_INFO_quilt,
> zsh-mime-handler).  I don't think any of those uses would be compromised
> by this change, but if anyone wants to do a more thorough search for use
> cases, don't be shy.
> 

On the contrary: I expect that some existing usages are broken when
their operand contains '..' path components; i.e., that existing usages
assume :A is syntactic sugar for realpath(3).

Anyway, I'll code up this alternative and post it once I have.
Code-wise I think it boils down to adding «

    if (isset(CHASEDOTS))
      chabspath(junkptr);

» at the top of chrealpath().

Cheers,

Daniel

> } Note this doesn't use CHASE_LINKS at all, neither in :A nor in :a.
> 
> Makes sense; :a plus CHASE_LINKS would be :A.
> 

[PATCH v2 1/3] Tests: Add tests for the ':a' and ':A' modifiers.

[Daniel Shahaf]

---
Differences to v1 of this series:

* Added dependency on CHASEDOTS (please review); docs updated accordingly
* Tests updated

Cheers,

daniel

 Test/D02glob.ztst | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index a6b704a..7befbc2 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -654,4 +654,27 @@
   [[ "z" != [$~cset] ]] || print Fail 4
   [[ "1" = [$~cset] ]] || print Fail 5
   [[ "b" != [$~cset] ]] || print Fail 6
-0:character set specified as active variabe
+0:character set specified as active variable
+
+ () { print -l -- $@:a } / /..{,/} /1 /nonexistent/..{,/} /deeper/nonexistent/..{,/}
+0:modifier ':a' doesn't require existence
+>/
+>/
+>/
+>/1
+>/
+>/
+>/deeper
+>/deeper
+
+ () { set -- ${PWD}/$^@; print -l -- $@:A } glob.tmp/nonexistent/foo/bar/baz
+0:modifier ':A' doesn't require existence
+*>*/glob.tmp/nonexistent/foo/bar/baz
+
+ ln -s dir3/subdir glob.tmp/link
+ () {
+   print ${1:A} | grep glob.tmp
+ } glob.tmp/link/../../hello
+ rm glob.tmp/link
+0:modifier ':A' resolves '..' components before symlinks
+# There should be no output

[PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Daniel Shahaf]

The fix is to stop calling chabspath() at the top of chrealpath().

Preserve the old behaviour when CHASE_DOTS is set.

Also remove an incorrect comment (passing a non-absolute path would have been
fine because the chabspath() call would have made it absolute).
---
 Doc/Zsh/expn.yo    | 12 +++++++++---
 Doc/Zsh/options.yo |  7 ++++++-
 README             |  9 +++++++++
 Src/hist.c         | 27 +++++++++++++++------------
 Test/D02glob.ztst  | 11 ++++++++++-
 5 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index c6e7b6f..82d2c9f 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -225,9 +225,15 @@ intervening directories do not exist.
 )
 item(tt(A))(
 As `tt(a)', but also resolve use of symbolic links where possible.
-Note that resolution of `tt(..)' occurs em(before) resolution of symbolic
-links.  This call is equivalent to tt(a) unless your system has the
-tt(realpath) system call (modern systems do).
+
+By default, symbolic links are resolved before `tt(..)' components.  However,
+if the tt(CHASE_DOTS) option is set, `tt(..)' components are resolved before
+symbolic links.  Furthermore, on systems that lack the tt(realpath+LPAR()RPAR())
+system call (modern systems have it), symbolic links are never resolved and
+this modifier is equivalent to the `tt(a)' modifier.
+
+em(Note): In zsh 5.2 and earlier, resolution of `tt(..)' occurred em(before)
+resolution of symbolic links regardless of the tt(CHASE_DOTS) option.
 )
 item(tt(c))(
 Resolve a command name into an absolute path by searching the command
diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index 0eaaeca..da93912 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -106,6 +106,11 @@ Without this option set, `tt(cd /foo/bar/..)' changes to tt(/foo); with it
 set, it changes to tt(/alt).  The same applies if the current directory
 is tt(/foo/bar) and `tt(cd ..)' is used.  Note that all other symbolic
 links in the path will also be resolved.
+
+This option also affects the interpretation of the `tt(:A)' history modifier,
+see
+ifzman(the section `Modifiers' in zmanref(zshexpn))\
+ifnzman(noderef(Modifiers)).
 )
 pindex(CHASE_LINKS)
 pindex(NO_CHASE_LINKS)
@@ -569,7 +574,7 @@ Substitutions using the tt(:s) and tt(:&) history modifiers are performed
 with pattern matching instead of string matching.  This occurs wherever
 history modifiers are valid, including glob qualifiers and parameters.
 See
-ifzman(the section Modifiers in zmanref(zshexpn))\
+ifzman(the section `Modifiers' in zmanref(zshexpn))\
 ifnzman(noderef(Modifiers)).
 )
 pindex(IGNORE_BRACES)
diff --git a/README b/README
index d5343db..6857253 100644
--- a/README
+++ b/README
@@ -79,6 +79,15 @@ Other aspects of EXIT trap handling have not changed --- there is still
 only one EXIT trap at any point in a programme, so it is not generally
 useful to combine POSIX and non-POSIX behaviour in the same script.
 
+4) On systems that have the realpath(3) library function, the ':A' word
+modifier now resolves symbolic links before '..' path components.  This
+could lead to different, but usually more desirable, results: the
+tranformed value will now always identify the same directory entry as the
+the pre-transformation value.
+
+The behaviour of 5.2 and older can be achieved by chaining modifiers,
+as in '<expression>:a:A', or by setting the CHASE_DOTS option.
+
 Incompatibilities between 5.0.8 and 5.2
 ---------------------------------------
 
diff --git a/Src/hist.c b/Src/hist.c
index 5fc40bd..30a1bef 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1837,8 +1837,8 @@ chabspath(char **junkptr)
 int
 chrealpath(char **junkptr)
 {
-    char *str;
 #ifdef HAVE_REALPATH
+    char *str;
 # ifdef REALPATH_ACCEPTS_NULL
     char *lastpos, *nonreal, *real;
 # else
@@ -1850,19 +1850,22 @@ chrealpath(char **junkptr)
     if (!**junkptr)
 	return 1;
 
-    /* Notice that this means ..'s are applied before symlinks are resolved! */
-    if (!chabspath(junkptr))
-	return 0;
-
 #ifndef HAVE_REALPATH
-    return 1;
+    return chabspath(junkptr);
 #else
-    /*
-     * Notice that this means you cannot pass relative paths into this
-     * function!
-     */
-    if (**junkptr != '/')
+
+    /* With CHASE_DOTS, resolve '..' components before symlinks.  (This was always
+     * done first in 5.2 and earlier.) */
+    if (isset(CHASEDOTS))
+      chabspath(junkptr);
+
+    if (**junkptr != '/') {
+	*junkptr = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP), "/", *junkptr);
+    }
+    if (**junkptr != '/') {
+	/* Can happen after 'rmdir $PWD; zsh' */
 	return 0;
+    }
 
     unmetafy(*junkptr, NULL);
 
@@ -1909,9 +1912,9 @@ chrealpath(char **junkptr)
     } else {
 	*junkptr = metafy(nonreal, lastpos - nonreal + 1, META_HEAPDUP);
     }
-#endif
 
     return 1;
+#endif
 }
 
 /**/
diff --git a/Test/D02glob.ztst b/Test/D02glob.ztst
index 7befbc2..bec9826 100644
--- a/Test/D02glob.ztst
+++ b/Test/D02glob.ztst
@@ -673,8 +673,17 @@
 
  ln -s dir3/subdir glob.tmp/link
  () {
+   setopt localoptions chasedots
    print ${1:A} | grep glob.tmp
  } glob.tmp/link/../../hello
  rm glob.tmp/link
-0:modifier ':A' resolves '..' components before symlinks
+0:modifier ':A' resolves '..' components before symlinks with CHASE_DOTS
 # There should be no output
+
+ ln -s dir3/subdir glob.tmp/link
+ () {
+   print ${1:A} | grep glob.tmp
+ } glob.tmp/link/../../hello
+ rm glob.tmp/link
+0:modifier ':A' resolves symlinks before '..' components with NO_CHASE_DOTS
+*>*glob.tmp/hello

[PATCH v2 3/3] Clean up chabspath() [':a' word modifier].

[Daniel Shahaf]

Fail fast when the first character isn't "/", and use that to simplify the
'..' handling.

The incumbent code was subtly wrong in various ways (for example,
the first 'else if' branch didn't check that dest[-4] was a slash).
---
 Src/hist.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/Src/hist.c b/Src/hist.c
index 30a1bef..2623a32 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -1767,6 +1767,10 @@ chabspath(char **junkptr)
     if (**junkptr != '/') {
 	*junkptr = zhtricat(metafy(zgetcwd(), -1, META_HEAPDUP), "/", *junkptr);
     }
+    if (**junkptr != '/') {
+	/* Can happen after 'rmdir $PWD; zsh' */
+	return 0;
+    }
 
     current = *junkptr;
     dest = *junkptr;
@@ -1783,7 +1787,9 @@ chabspath(char **junkptr)
 
     for (;;) {
 	if (*current == '/') {
+	    /* Contract multiple slashes. */
 #ifdef __CYGWIN__
+	    /* Exception: on Cygwin, two slashes at the very start are significant. */
 	    if (current == *junkptr && current[1] == '/')
 		*dest++ = *current++;
 #endif
@@ -1791,28 +1797,17 @@ chabspath(char **junkptr)
 	    while (*current == '/')
 		current++;
 	} else if (!*current) {
+	    /* Remove trailing slashes. */
 	    while (dest > *junkptr + 1 && dest[-1] == '/')
 		dest--;
 	    *dest = '\0';
 	    break;
 	} else if (current[0] == '.' && current[1] == '.' &&
 		   (!current[2] || current[2] == '/')) {
-		if (current == *junkptr || dest == *junkptr) {
-		    *dest++ = '.';
-		    *dest++ = '.';
-		    current += 2;
-		} else if (dest > *junkptr + 2 &&
-			   !strncmp(dest - 3, "../", 3)) {
-		    *dest++ = '.';
-		    *dest++ = '.';
-		    current += 2;
-		} else if (dest > *junkptr + 1) {
-		    *dest = '\0';
+		if (dest > *junkptr + 1) {
 		    for (dest--;
-			 dest > *junkptr + 1 && dest[-1] != '/';
+			 dest[-1] != '/';
 			 dest--);
-		    if (dest[-1] != '/')
-			dest--;
 		    current += 2;
 		    if (*current == '/')
 			current++;

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Mikael Magnusson]

On Tue, Jun 21, 2016 at 3:53 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> The fix is to stop calling chabspath() at the top of chrealpath().
>
> Preserve the old behaviour when CHASE_DOTS is set.

I think this is backwards, cd symlink/.. gets you to the current dir
if chasedots is unset, and to wherever symlink points' parent
directory when it is set.

-- 
Mikael Magnusson

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Daniel Shahaf]

Mikael Magnusson wrote on Tue, Jun 21, 2016 at 05:08:16 +0200:
> On Tue, Jun 21, 2016 at 3:53 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > The fix is to stop calling chabspath() at the top of chrealpath().
> >
> > Preserve the old behaviour when CHASE_DOTS is set.
> 
> I think this is backwards, cd symlink/.. gets you to the current dir
> if chasedots is unset, and to wherever symlink points' parent
> directory when it is set.

True.

However, I don't want to just flip the condition (change "if isset()" to
"if unset()") since I think the new behaviour (resolving symlinks before
'..' components) should be the default for :A.

So how about adding a new option and having chrealpath() use the new
behaviour if the new option is at its default value, and the old
(':a'-ish) behaviour otherwise?

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Mikael Magnusson]

On Sat, Jun 25, 2016 at 6:28 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Mikael Magnusson wrote on Tue, Jun 21, 2016 at 05:08:16 +0200:
>> On Tue, Jun 21, 2016 at 3:53 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>> > The fix is to stop calling chabspath() at the top of chrealpath().
>> >
>> > Preserve the old behaviour when CHASE_DOTS is set.
>>
>> I think this is backwards, cd symlink/.. gets you to the current dir
>> if chasedots is unset, and to wherever symlink points' parent
>> directory when it is set.
>
> True.
>
> However, I don't want to just flip the condition (change "if isset()" to
> "if unset()") since I think the new behaviour (resolving symlinks before
> '..' components) should be the default for :A.
>
> So how about adding a new option and having chrealpath() use the new
> behaviour if the new option is at its default value, and the old
> (':a'-ish) behaviour otherwise?

That sounds pretty pointless, you would still break existing scripts,
and make everyone have to versioncheck to see if they have to enable
an option or not.

-- 
Mikael Magnusson

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Daniel Shahaf]

Mikael Magnusson wrote on Sat, Jun 25, 2016 at 18:47:58 +0200:
> On Sat, Jun 25, 2016 at 6:28 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Mikael Magnusson wrote on Tue, Jun 21, 2016 at 05:08:16 +0200:
> >> On Tue, Jun 21, 2016 at 3:53 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >> > The fix is to stop calling chabspath() at the top of chrealpath().
> >> >
> >> > Preserve the old behaviour when CHASE_DOTS is set.
> >>
> >> I think this is backwards, cd symlink/.. gets you to the current dir
> >> if chasedots is unset, and to wherever symlink points' parent
> >> directory when it is set.
> >
> > True.
> >
> > However, I don't want to just flip the condition (change "if isset()" to
> > "if unset()") since I think the new behaviour (resolving symlinks before
> > '..' components) should be the default for :A.
> >
> > So how about adding a new option and having chrealpath() use the new
> > behaviour if the new option is at its default value, and the old
> > (':a'-ish) behaviour otherwise?
> 
> That sounds pretty pointless, you would still break existing scripts,

Yes, that's the whole point: I think the "new" semantics should be the
default.

> and make everyone have to versioncheck to see if they have to enable
> an option or not.

No version check is needed, people can change foo:A to foo:a:A or do

    setopt colonA_please_be_compatible_with_fivedottwo 2>/dev/null || true

once at the top.

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Bart Schaefer]

On Jun 27, 12:20am, Daniel Shahaf wrote:
}
} Mikael Magnusson wrote on Sat, Jun 25, 2016 at 18:47:58 +0200:
} > That sounds pretty pointless, you would still break existing scripts,
} 
} Yes, that's the whole point: I think the "new" semantics should be the
} default.

Obviously breaking scripts is not the point.  On the other hand, I don't
think many scripts would care.  :A is only used in four places in zsh's
Completion and Functions trees and none of those would suffer from the
change in semantics as far as I can tell.

I floated the idea of testing CHASE_DOTS so that there would be a way
to globally revert to the old behavior without having to update a lot
of scripts, but I hadn't considered the "backward" semantics, and I'm
not in favor of introducing a new option specifically for :A control.
However, with the current implementation there is NO way to obtain the
semantics Daniel wants.

Therefore I think the only reasonable solutions are:
(1) keep the current default and respect CHASE_DOTS to get the function
Daniel wants, or
(2) change the default and use :a:A to get the old behavior.

I lean slightly towards (1) but only because the doc has explicitly
called out the "resolve .. first" behavior for the past 7 years.

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Daniel Shahaf]

Bart Schaefer wrote on Tue, Jun 28, 2016 at 07:48:51 -0700:
> Therefore I think the only reasonable solutions are:
> (1) keep the current default and respect CHASE_DOTS to get the function
> Daniel wants, or
> (2) change the default and use :a:A to get the old behavior.
> 
> I lean slightly towards (1) but only because the doc has explicitly
> called out the "resolve .. first" behavior for the past 7 years.

What's the use-case for the "resolve '..' before symlinks" behaviour?

There doesn't seem to be a C stdlib function for it.

That behaviour (and the :A modifier) was added in 26731, but that thread
doesn't answer this question.

Was it an intentional design feature, or simply a documented bug?
(Honest question.)

> I floated the idea of testing CHASE_DOTS so that there would be a way
> to globally revert to the old behavior without having to update a lot
> of scripts, but I hadn't considered the "backward" semantics, and I'm
> not in favor of introducing a new option specifically for :A control.

(Perhaps instead of a new option, a new syntax; e.g., have $foo:A retain
its meaning and $foo:A:A have the new meaning.  Or make :A take an
optional argument.)

I get that a multitude of options is undesirable, and that making the
new behaviour conditional on an unset-by-default option helps
compatibility, but I'm wary of making a single option have two distinct
effects.

Each knob should do one thing and do it well.  Making chasedots affect
both 'cd' and :A will cause one of two things: either scripts that
'emulate -L zsh && setopt chasedots' for the setopt's effect on 'cd'
will get the new :A behaviour without explicitly opting in to it [which
seems to be your goal here], or people who wish :A to behave like
realpath(3) will have to use the chasedots version of 'cd'.

> However, with the current implementation there is NO way to obtain the
> semantics Daniel wants.

Thanks,

Daniel

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Bart Schaefer]

On Jul 1,  5:11am, Daniel Shahaf wrote:
} Subject: Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' 
}
} What's the use-case for the "resolve '..' before symlinks" behaviour?
} 
} Was it an intentional design feature, or simply a documented bug?
} (Honest question.)

PWS will have to weigh in on that one.  The use-case may simply have
been the intention to make :A a superset of :a.

} (Perhaps instead of a new option, a new syntax; e.g., have $foo:A retain
} its meaning and $foo:A:A have the new meaning.  Or make :A take an
} optional argument.)

Not ideal IMO because either would make :A differ from all other history
modifiers in this respect.  I.e. :A:A should mean to resolve to real
path and then resolve to real path again, like :h:h means to chop two
elements off the tail (and thus the second :A should be a no-op like
repeating :t); and no other has an optional argument.  However, worth
considering.

Sorry to just sort of leave things there.  I don't have a strong
objection to changing the :A default given the :a:A equivalence, and
that seems the least confusing final outcome if we conclude that the
likelihood of breaking scripts is small, but I would like to reach a
consensus decision.

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Peter Stephenson]

On Fri, 1 Jul 2016 09:05:29 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jul 1,  5:11am, Daniel Shahaf wrote:
> } Subject: Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' 
> }
> } What's the use-case for the "resolve '..' before symlinks" behaviour?
> } 
> } Was it an intentional design feature, or simply a documented bug?
> } (Honest question.)
> 
> PWS will have to weigh in on that one.  The use-case may simply have
> been the intention to make :A a superset of :a.

I think making :A a superset of :a was all there was to it.  I don't
think there was any specific strategy for dealing with non-obvious cases
involving "..".

pws

Re: [PATCH v2 2/3] Fix the ':A' word modifier on paths with '..' components.

[Daniel Shahaf]

Bart Schaefer wrote on Fri, Jul 01, 2016 at 09:05:29 -0700:
> Sorry to just sort of leave things there.  I don't have a strong
> objection to changing the :A default given the :a:A equivalence, and
> that seems the least confusing final outcome if we conclude that the
> likelihood of breaking scripts is small, but I would like to reach a
> consensus decision.

So do I.  Based on the feedback so far, I won't be pushing this patch,
since clearly there is no consensus for doing so.  I'll go ahead and
solicit additional input on the issue from -users@.

pws: I don't think this change should block 5.3.  If the release train
is ready before this change is, let the train leave the station and this
patch can wait for 5.4.

Thanks for the discussion,

Daniel