Incorrect evaluation of ~ test in ternary conditional

Incorrect evaluation of ~ test in ternary conditional

[Felix Uhl]

Hi everybody!

Let there be a directory ~/work. Using the ~ test character in a 
conditional ternary prompt will return incorrect results when the 
argument is 2 as shown below:

$ cd && print -P "%(2~:true:false)"
false
$ cd work/.. && print -P "%(2~:true:false)"
true

The docs say: 
(http://zsh.sourceforge.net/Doc/Release/Prompt-Expansion.html#Conditional-Substrings-in-Prompts)

 > c [...] . [...] ~ [...] True if the current path, with prefix 
replacement, has at least n elements relative to the root directory, 
hence / is counted as 0 elements.

So the first command behaves properly, but the second one doesn't.

I'm using zsh 5.0.5. Can somebody confirm this for the current version 
of zsh as well?

BR, Felix

Re: Incorrect evaluation of ~ test in ternary conditional

[Bart Schaefer]

On Fri, Dec 15, 2017 at 5:55 AM, Felix Uhl <felix.uhl@outlook.de> wrote:
> Hi everybody!
>
> Let there be a directory ~/work. Using the ~ test character in a
> conditional ternary prompt will return incorrect results when the
> argument is 2 as shown below:
>
> $ cd && print -P "%(2~:true:false)"
> false
> $ cd work/.. && print -P "%(2~:true:false)"
> true

I actually get randomly incorrect results in the FIRST of your examples:

repeat 5 do cd && print -P "%~ %(2~:true:false)" ; done
~ true
~ false
~ true
~ false
~ false

The second case is consistent but is always wrong:

repeat 5 do cd work/.. && print -P "%~ %(2~:true:false)" ; done
~ true
~ true
~ true
~ true
~ true

Both of the foregoing happen in each of 5.0.2 and the latest
development version.  I don't have 5.0.5 handy.

This seems to be the fix:

diff --git a/Src/prompt.c b/Src/prompt.c
index c478e69..63e8093 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -315,7 +315,7 @@ putpromptchar(int doprint, int endchar, unsigned
int *txtchangep)
                case '/':
                case 'C':
                    /* `/' gives 0, `/any' gives 1, etc. */
-                   if (*ss++ == '/' && *ss)
+                   if (*ss && *ss++ == '/' && *ss)
                        arg--;
                    for (; *ss; ss++)
                        if (*ss == '/')

I'm not sure whether (*ss == '/' && *++ss) would be equivalent, i.e.,
I don't know why the original formulation skips over the first
character whether or not it is a '/'.  Possibly to skip a leading '~'?

Re: Incorrect evaluation of ~ test in ternary conditional

[Felix Uhl]

On 16.12.2017 23:01, Bart Schaefer wrote:
> On Fri, Dec 15, 2017 at 5:55 AM, Felix Uhl <felix.uhl@outlook.de> wrote:
>> Hi everybody!
>>
>> Let there be a directory ~/work. Using the ~ test character in a
>> conditional ternary prompt will return incorrect results when the
>> argument is 2 as shown below:
>>
>> $ cd && print -P "%(2~:true:false)"
>> false
>> $ cd work/.. && print -P "%(2~:true:false)"
>> true
> I actually get randomly incorrect results in the FIRST of your examples:
>
> repeat 5 do cd && print -P "%~ %(2~:true:false)" ; done
> ~ true
> ~ false
> ~ true
> ~ false
> ~ false
>
> The second case is consistent but is always wrong:
>
> repeat 5 do cd work/.. && print -P "%~ %(2~:true:false)" ; done
> ~ true
> ~ true
> ~ true
> ~ true
> ~ true
>
> Both of the foregoing happen in each of 5.0.2 and the latest
> development version.  I don't have 5.0.5 handy.
>
>

Huh, interesting. Even at 5000 repetitions, I don't get a single wrong 
result on the first example (takes about 2 minutes to run though):

$ repeat 5000 do cd && print -nP "%(2~:true:)" ; done
$

I wonder where that discrepancy comes from.

> This seems to be the fix:
>
> diff --git a/Src/prompt.c b/Src/prompt.c
> index c478e69..63e8093 100644
> --- a/Src/prompt.c
> +++ b/Src/prompt.c
> @@ -315,7 +315,7 @@ putpromptchar(int doprint, int endchar, unsigned
> int *txtchangep)
>                  case '/':
>                  case 'C':
>                      /* `/' gives 0, `/any' gives 1, etc. */
> -                   if (*ss++ == '/' && *ss)
> +                   if (*ss && *ss++ == '/' && *ss)
>                          arg--;
>                      for (; *ss; ss++)
>                          if (*ss == '/')
>
> I'm not sure whether (*ss == '/' && *++ss) would be equivalent, i.e.,
> I don't know why the original formulation skips over the first
> character whether or not it is a '/'.  Possibly to skip a leading '~'?

The original implementation doesn't skip the first character, does it?
C operator precedence will treat *ss++ like *(ss++), so if ss is (for 
the sake of demonstration) 10, it is incremented to 11, but the postfix 
operator returns the old value of 10, which is then dereferenced by *. 
So the whole condition checks whether the first character is '/' and 
whether the second character is not 0.

I'd probably write that as (*ss == '/' && *(++ss)), seems much clearer 
to me.

I fail to understand why your fix should work, the expressions (*ss && 
*ss++ == '/') and (*ss++ == '/') on their own are logically equivalent 
to (*ss == '/') and have equivalent side-effects as well.
Did you actually test it?

Re: Incorrect evaluation of ~ test in ternary conditional

[Bart Schaefer]

Catching up on some old stuff ...

On Mon, Dec 18, 2017 at 7:07 AM, Felix Uhl <felix.uhl@outlook.de> wrote:
>
>> -                   if (*ss++ == '/' && *ss)
>> +                   if (*ss && *ss++ == '/' && *ss)
>>                          arg--;
>>
>> I'm not sure whether (*ss == '/' && *++ss) would be equivalent, i.e.,
>> I don't know why the original formulation skips over the first
>> character whether or not it is a '/'.  Possibly to skip a leading '~'?
>
> The original implementation doesn't skip the first character, does it?

It does, because it increments ss regardless of whether or not the
first character is '/'.  So if ss = "a", (*ss++ == '/' && *ss) is
false because 'a' != '/', but ss now points at '\0' instead of at 'a'.

> So the whole condition checks whether the first character is '/' and
> whether the second character is not 0.

Correct, but it has the side-effect of always advancing ss by one position.

> I'd probably write that as (*ss == '/' && *(++ss)), seems much clearer
> to me.

That avoids the side-effect, but I'm not sure if that side-effect was
intentional.

> I fail to understand why your fix should work, the expressions (*ss &&
> *ss++ == '/') and (*ss++ == '/') on their own are logically equivalent

Not when ss = "".  If ss points to empty string, the first of those
does not move ss past the null byte into garbage, but the second one
does. The question is what should happen when ss is neither the empty
string nor begins with a slash.

> to (*ss == '/') and have equivalent side-effects as well.

This is incorrect with respect to the side-effects.

> Did you actually test it?

Yes.

Re: Incorrect evaluation of ~ test in ternary conditional

[Felix Uhl]

On 30.12.2017 03:40, Bart Schaefer wrote:
> Catching up on some old stuff ...
>
> On Mon, Dec 18, 2017 at 7:07 AM, Felix Uhl <felix.uhl@outlook.de> wrote:
>>> -                   if (*ss++ == '/' && *ss)
>>> +                   if (*ss && *ss++ == '/' && *ss)
>>>                           arg--;
>>>
>>> I'm not sure whether (*ss == '/' && *++ss) would be equivalent, i.e.,
>>> I don't know why the original formulation skips over the first
>>> character whether or not it is a '/'.  Possibly to skip a leading '~'?
>> The original implementation doesn't skip the first character, does it?
> It does, because it increments ss regardless of whether or not the
> first character is '/'.  So if ss = "a", (*ss++ == '/' && *ss) is
> false because 'a' != '/', but ss now points at '\0' instead of at 'a'.
>
>> Did you actually test it?
> Yes.

Ah, you're absolutely correct. Thanks for the explanation!

I guess the only thing left then is to find out whether your fix breaks 
anything, correct?