nohistsubstpattern and :s//

nohistsubstpattern and :s//

[Mikael Magnusson]

% setopt histsubstpattern; a=(aaa bab cac) b=d; echo $a:s/a/${b}/
daa bdb cdc
% setopt nohistsubstpattern; a=(aaa bab cac) b=d; echo $a:s/a/${b}/
${b}aa b${b}b cdc

(reported on irc).

I looked at the documentation of histsubstpattern and the :s/l/r/
flag, and it mostly talks about the effect of the option on the l
part. Even if this effect is somehow desired, it seems inconsistent
that the final element still gets the value of the parameter
substituted.

-- 
Mikael Magnusson

Re: nohistsubstpattern and :s//

[Bart Schaefer]

Peter, this is yours from way back in workers/22934 -- any memory left
from 2006?

On Jul 4,  3:22pm, Mikael Magnusson wrote:
} Subject: nohistsubstpattern and :s//
}
} % setopt histsubstpattern; a=(aaa bab cac) b=d; echo $a:s/a/${b}/
} daa bdb cdc

Hmm ... hist_subst_pattern doesn't say anything about expansions in
the replacement part of s/pat/rep/, so as far as I can tell this is
contrary to the documentation.  However, see below.

} % setopt nohistsubstpattern; a=(aaa bab cac) b=d; echo $a:s/a/${b}/
} ${b}aa b${b}b cdc

This has been like this for as long as the hist_subst_pattern option
has existed, it appears.  Random old builds I happen to have around
show behavior matching the doc in 3.0.6 and 3.0.8 --

torch% echo $ZSH_VERSION
3.0.6-pre-5
torch% a=(aaa bab cac) b=d; echo $a:s/a/${b}/
${b}aa b${b}b c${b}c

torch% echo $ZSH_VERSION
3.0.8
torch% a=(aaa bab cac) b=d; echo $a:s/a/${b}/
${b}aa b${b}b c${b}c

-- but then after hist_subst_pattern is added in 4.3.3 --

torch% echo $ZSH_VERSION                                                        
4.3.17-test-2
torch% a=(aaa bab cac) b=d; echo $a:s/a/${b}/
${b}aa b${b}b cdc

-- and (still 4.3.17-test-2) --

torch% setopt histsubstpattern
torch% a=(aaa bab cac) b=d; echo $a:s/a/${b}/
daa bdb cdc


So this undocumented behaver of the replacement has also been around
ever since the option was first implemented.  The original test case
in E01options.ztst even expects expansions in the replacement, so it
seems to have been intentional, to allow references to $match in the
event of backrefs in the pattern.

This also changed the behavior for scalars when hist_subst_pattern is
NOT set (note "g" added below):

torch% echo $ZSH_VERSION
3.0.8
torch% a="aaa bab cac" b=d; echo $a:gs/a/${b}/
${b}${b}${b} b${b}b c${b}c

torch% echo $ZSH_VERSION
4.3.17-test-2
torch% a="aaa bab cac" b=d; echo $a:gs/a/${b}/
ddd bdb cdc

So I suspect the same logic error (?) that changed this for scalars is
also somehow affecting the last element of an array.

Re: nohistsubstpattern and :s//

[Peter Stephenson]

On Sat, 4 Jul 2015 17:01:47 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Peter, this is yours from way back in workers/22934 -- any memory left
> from 2006?

I've forgotten the option completely, but I'd be very surprised if it
was deliberate.  I'd at least have stuck in a snotty note saying this
was for caompatibility.  It doesn't look at all useful.

pws

Re: nohistsubstpattern and :s//

[Bart Schaefer]

There's a patch near the bottom of this, which needs as many other
eyeballs on it as possible because it's changing some really old code.

On Jul 5, 11:30am, Peter Stephenson wrote:
} Subject: Re: nohistsubstpattern and :s//
}
} On Sat, 4 Jul 2015 17:01:47 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > Peter, this is yours from way back in workers/22934 -- any memory left
} > from 2006?
} 
} I've forgotten the option completely, but I'd be very surprised if it
} was deliberate.  I'd at least have stuck in a snotty note saying this
} was for caompatibility.  It doesn't look at all useful.

Not sure which "it" doesn't look useful, but here's a deeper look at a
few things -- first, the change to scalar expansion:

torch% a="aaa bab cac" b=d; echo ${a:gs/a/${b}/}

This goes into subst() in hist.c with ${b} already metafied, and comes
out again looking like

    ${b}${b}${b} b${b}b c${b}c

with all the ${b} still metafied.  I think this is exactly as expected.

#0  subst (strptr=0xbfee4ca4, in=0x82d0e00 "a", out=0x82d0e10 "\205\217b\220", 
    gbal=1) at ../../zsh-5.0/Src/hist.c:2125
#1  0x080c5a4c in modify (str=0xbfee4ca4, ptr=0xbfee4ce4)
    at ../../zsh-5.0/Src/subst.c:4210
#2  0x080c32d5 in paramsubst (l=0xb7d67758, n=0xb7d67770, str=0xbfee4d58, 
    qt=0, pf_flags=0) at ../../zsh-5.0/Src/subst.c:3237
#3  0x080bd8ae in stringsubst (list=0xb7d67758, node=0xb7d67770, pf_flags=0, 
    asssub=0) at ../../zsh-5.0/Src/subst.c:236
#4  0x080bd05e in prefork (list=0xb7d67758, flags=0)
    at ../../zsh-5.0/Src/subst.c:77

At this point stringsubst() [frame #3] keeps looping until all of the
metafied ${b} have been replaced by their expansions.

Further, if I then try a straight history substitution rather than use
a history modifier in a parameter substitution:

torch% !:0:gs/a/${b}/:p
${b}="${b}${b}${b} b${b}b c${b}c"

Here the ${b} is not metafied on the way in to subst(), because we're
doing the replacement very early in the lexing of the command line.

So the expansion of ${b} isn't really related to hist_subst_pattern at
all, it must merely be approximately coincident with it in time.  I
have no idea where to go next with this one, the hist_subst_pattern
test case relies on it behaving this way -- so maybe we should just
document it (in history section 14.1.4 under modifiers, or in Rule #7
for parameter expansion, or where)?

One more comment on this at the end of this message.


Now let's look at 

torch% a=(aaa bab cac) b=d; echo $a:gs/a/${b}/

#0  subst (strptr=0xb7d67850, in=0x82cfe28 "a", out=0x82d8e88 "\205\217b\220", 
    gbal=1) at ../../zsh-5.0/Src/hist.c:2125
#1  0x080c5a4c in modify (str=0xb7d67850, ptr=0xbfee4bb4)
    at ../../zsh-5.0/Src/subst.c:4210
#2  0x080c3354 in paramsubst (l=0xb7d67810, n=0xb7d67828, str=0xbfee4d58, 
    qt=0, pf_flags=0) at ../../zsh-5.0/Src/subst.c:3246
#3  0x080bd8ae in stringsubst (list=0xb7d67810, node=0xb7d67828, pf_flags=0, 
    asssub=0) at ../../zsh-5.0/Src/subst.c:236
#4  0x080bd05e in prefork (list=0xb7d67810, flags=0)
    at ../../zsh-5.0/Src/subst.c:77

Note modify() is being called from a different place, the loop at 3244
that applies it to each element of the array.

Passed down to stringsubst() was "node" pointing at the first element
of the array, but returning from stringsubst() to paramsubst() we have
*str pointing into the last node of the array, and assign "node" there
to that last node.  Then we continue looping until the ${b} in "c${b}c"
is expanded, but we have thus skipped over the first two elements of
the array.

This in turn all comes from the "if (isarr)" block at line 3652, where
we record "Linknode on = n" at line 3657 but conditionally restore n at
3838 based on "if (eval)".

With the patch below, removing that conditional restore, all tests still
pass and the array version of :s// works like the scalar version:

torch% a=(aaa bab cac) b=d; echo $a:gs/a/${b}/
ddd bdb cdc

The patch simply removes that conditional restore and does it always,
but restores both the node and the string pointer instead of only the
node.  There's a little more discussion after the patch.


diff --git a/Src/subst.c b/Src/subst.c
index 81d34d2..021d234 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -3834,8 +3834,14 @@ paramsubst(LinkList l, LinkNode n, char **str, int qt, int pf_flags)
 		y = dupstring(nulstring);
 	    insertlinknode(l, n, (void *) y), incnode(n);
 	}
-	if (eval)
-	    n = on;
+	/* This used to omit restoring of *str and instead test
+	 *   if (eval)
+	 *       n = on;
+	 * but that causes strange behavior of history modifiers when
+	 * applied across all values of an array.  What is magic about
+	 * eval here that *str seemed not to need restoring?
+	 */
+	*str = getdata(n = on);
     } else {
 	/*
 	 * Scalar value.  Handle last minute transformations
diff --git a/Test/D04parameter.ztst b/Test/D04parameter.ztst
index d06a73a..0a9e253 100644
--- a/Test/D04parameter.ztst
+++ b/Test/D04parameter.ztst
@@ -1711,3 +1711,12 @@
 0:Avoid confusion after overloaded characters in braceless substitution in sh
 >13
 >0-1
+
+  a="aaa bab cac"
+  b=d
+  echo $a:gs/a/${b}/
+  a=(aaa bab cac)
+  echo $a:gs/a/${b}/
+0:History modifier works the same for scalar and array substitution
+>ddd bdb cdc
+>ddd bdb cdc



Parameter substitution also occurs in the replacement part when the
modifier is applied as a glob qualfier, in case that matters.  In this
case the replacement happens during parsing of the qualifier rather
than after the substitution:

torch% echo *a*
config.modules.local config.status Makefile stamp-h
torch% b=1
torch% print *a*(:s/a/$[b++]/) $b
config.modules.loc1l config.st1tus M1kefile st1mp-h 2
torch% a=(*a*)
torch% print ${a:s/a/$[b++]/} $b
config.modules.loc1l config.st2tus M3kefile st4mp-h 5

So we have three different behaviors of the same substitution depending
on the context.

-- 
Barton E. Schaefer

Re: nohistsubstpattern and :s//

[Peter Stephenson]

On Sun, 5 Jul 2015 14:11:16 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> There's a patch near the bottom of this, which needs as many other
> eyeballs on it as possible because it's changing some really old code.

I'd say use it --- I suspect you've exercised this code more than anyone
in years and if this breaks something, it's going to have to be very
obscure.

I don't think it's *that* much of a problem that the modifiers behave
differently in contexts that are themselves intrinsically rather
different.

pws

Re: nohistsubstpattern and :s//

[Bart Schaefer]

On Jul 6,  9:49am, Peter Stephenson wrote:
}
} I don't think it's *that* much of a problem that the modifiers behave
} differently in contexts that are themselves intrinsically rather
} different.

Some doc below, but there's still some odd stuff going on.

$b != ${b} in parameter expansion:

torch% a=(abc dbe fbg)
torch% unset b
torch% print ${a:gs/b/$b/}  
a d f
torch% print ${a:gs/b/${b}} 
ac de fg
torch% b=/
torch% print ${a:gs/b/$b/}     
a d f
torch% print ${a:gs/b/${b}}
a/c d/e f/g

(I have no idea why the strings are being truncated in the $b case no
matter what the value of $b.)

No way to quote "/" when using in a glob qualifier:

torch% ls *a*
config.modules.local  config.status  Makefile  stamp-h
torch% b=/
torch% print *a*(:s/a/$b/)
config.modules.locl config.sttus Mkefile stmp-h
torch% b=X
torch% print *a*(:s/a/$b/)
config.modules.locXl config.stXtus MXkefile stXmp-h

(Perhaps ${(b)...} should put backslashes in front of slashes? But two
backslashes are needed:

torch% print *a*(:s/a/\//)
config.modules.locl config.sttus Mkefile stmp-h
torch% print *a*(:s/a/\\//)
config.modules.loc/l config.st/tus M/kefile st/mp-h

so maybe this just calls for using a different delimiter.)


diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index 7d4e6fc..1e1fc52 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -319,6 +319,19 @@ forms of expansion.
 Note that if a `tt(&)' is used within glob qualifiers an extra backslash
 is needed as a tt(&) is a special character in this case.
 
+Also note that the order of expansions affects the interpretation of
+var(l) and var(r).  When used in a history expansion, which occurs before
+any other expansions, var(l) and var(r) are treated as literal strings
+(except as explained for tt(HIST_SUBST_PATTERN) below).  When used in
+parameter expansion, the replacement of var(r) into the parameter's value
+is done first, and then any additional process, parameter, command,
+arithmetic, or brace references are applied, which may evaluate those
+substitutions and expansions more than once if var(l) appears more than
+once in the starting value.  When used in a glob qualifier, any
+substitutions or expansions are performed once at the time the qualifier
+is parsed, even before the `tt(:s)' expression itself is divided into
+var(l) and var(r) sides.
+
 If the option tt(HIST_SUBST_PATTERN) is set, var(l) is treated as
 a pattern of the usual form described in
 ifzman(the section FILENAME GENERATION below)\

Re: nohistsubstpattern and :s//

[Mikael Magnusson]

On Mon, Jul 6, 2015 at 10:51 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Jul 6,  9:49am, Peter Stephenson wrote:
> }
> } I don't think it's *that* much of a problem that the modifiers behave
> } differently in contexts that are themselves intrinsically rather
> } different.
[...]
> No way to quote "/" when using in a glob qualifier:
>
> torch% ls *a*
> config.modules.local  config.status  Makefile  stamp-h
> torch% b=/
> torch% print *a*(:s/a/$b/)
> config.modules.locl config.sttus Mkefile stmp-h
> torch% b=X
> torch% print *a*(:s/a/$b/)
> config.modules.locXl config.stXtus MXkefile stXmp-h
>
> (Perhaps ${(b)...} should put backslashes in front of slashes? But two
> backslashes are needed:
>
> torch% print *a*(:s/a/\//)
> config.modules.locl config.sttus Mkefile stmp-h
> torch% print *a*(:s/a/\\//)
> config.modules.loc/l config.st/tus M/kefile st/mp-h
>
> so maybe this just calls for using a different delimiter.)

You can use any character instead of a /, even a literal NUL
character, so it's not really something ${(b)...} can/should know
about, I think?

% a=f/o/o/; echo $a:gs^@/^@_^@
f_o_o_
% a=moo; echo $a:spoopypants
myants
% echo ..(:gs^@.^@hello\\^@there^@)|cat -v
hello^@therehello^@there

-- 
Mikael Magnusson

Re: nohistsubstpattern and :s//

[Peter Stephenson]

On Mon, 6 Jul 2015 13:51:07 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Jul 6,  9:49am, Peter Stephenson wrote:
> }
> } I don't think it's *that* much of a problem that the modifiers behave
> } differently in contexts that are themselves intrinsically rather
> } different.
> 
> Some doc below, but there's still some odd stuff going on.
> 
> $b != ${b} in parameter expansion:
> 
> torch% a=(abc dbe fbg)
> torch% unset b
> torch% print ${a:gs/b/$b/}  
> a d f

Perhaps the line is turning into "a$bc", "d$be", "f$bg" before the
substitution?

This is new:  the existing code gives

ac de fg

so maybe this is a clue to why the current code works the way it does.

pws

Re: nohistsubstpattern and :s//

[Peter Stephenson]

On Tue, 7 Jul 2015 12:15:35 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> > torch% a=(abc dbe fbg)
> > torch% unset b
> > torch% print ${a:gs/b/$b/}  
> > a d f
> 
> Perhaps the line is turning into "a$bc", "d$be", "f$bg" before the
> substitution?
> 
> This is new:  the existing code gives
> 
> ac de fg
> 
> so maybe this is a clue to why the current code works the way it does.

Hmm, it seems to have gone back to the old way after a full
recompilation --- unless there's something flaky in the memory
or pointer management.

pws

Re: nohistsubstpattern and :s//

[Bart Schaefer]

On Jul 7, 12:54pm, Peter Stephenson wrote:
}
} Hmm, it seems to have gone back to the old way after a full
} recompilation --- unless there's something flaky in the memory
} or pointer management.

"Gone back to the old way" means what, precisely?  Sorry I lost track.

Re: nohistsubstpattern and :s//

[Peter Stephenson]

On Tue, 7 Jul 2015 08:11:01 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jul 7, 12:54pm, Peter Stephenson wrote:
> }
> } Hmm, it seems to have gone back to the old way after a full
> } recompilation --- unless there's something flaky in the memory
> } or pointer management.
> 
> "Gone back to the old way" means what, precisely?  Sorry I lost track.

"Old way" is misleading: I mean the expected result with b -> empty
string because b is unset.  I've been updating and recompiling without
properly keeping track of the version.

Even this afternoon(!) I'm still getting

% a=(abc dbe fbg)
% setopt histsubstpattern
% unset b
% print ${a:gs/b/$b/}
ac de fg

and I confirmed with zsh -f, too.

pws

Re: nohistsubstpattern and :s//

[Bart Schaefer]

On Jul 7,  4:28pm, Peter Stephenson wrote:
}
} Even this afternoon(!) I'm still getting
} 
} % a=(abc dbe fbg)
} % setopt histsubstpattern
} % unset b
} % print ${a:gs/b/$b/}
} ac de fg
} 
} and I confirmed with zsh -f, too.

Ah!

torch% a=(abc dbe fbg)
torch% setopt histsubstpattern
torch% print ${a:gs/b/$b/}
ac de fg
torch% setopt nohistsubstpattern
torch% print ${a:gs/b/$b/}
a d f

So with histsubstpattern set it looks like $b is expanded before the
substitution takes place, but with nohistsubstpattern (the default,
which is why I never showed the option change in any of my previous
examples) the expansion happens after the substitution.

Demonstrable with a scalar substitution instead of an array:

torch% a=(abc dbe fbg) 
torch% b=1         
torch% print ${a:s/b/$[b++]/}
a1c d2e f3g
torch% setopt histsubstpattern
torch% print ${a:s/b/$[b++]/} 
a4c d5e f6g
torch% a="$a"				<-- scalar + histsubstpattern
torch% print ${a:gs/b/$[b++]/} 
a7c d7e f7g				<-- only one increment of b
torch% unsetopt histsubstpattern
torch% print ${a:gs/b/$[b++]/}  
a8c d9e f10g				<-- three increments of b

No memory errors found by valgrind in either case.

Re: nohistsubstpattern and :s//

[Peter Stephenson]

On Tue, 7 Jul 2015 08:48:51 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> So with histsubstpattern set it looks like $b is expanded before the
> substitution takes place, but with nohistsubstpattern (the default,
> which is why I never showed the option change in any of my previous
> examples) the expansion happens after the substitution.

Oh, in that case it's perfectly sensible.  The only gotcha is to
remember that because modifiers operate on raw input, which is how they
came to exist in the first place, if you don't want a $ to be active you
need to quote it.

% print ${a:gs/b/\$b/}
a$bc d$be f$bg

Not wanting this effect is probably a sign you should be using

% print ${a//b/$b}

where the rules are roughly "if it quacks like a single word, it's
expanded like a single word", so any expansion is always done (or
suppressed) within the substitution argument, and you get

% print ${a//b/$b}
ac de fg
% print ${a//b/\$b}
a$bc d$be f$bg

pws

Re: nohistsubstpattern and :s//

[Bart Schaefer]

On Jul 7,  5:10pm, Peter Stephenson wrote:
} Subject: Re: nohistsubstpattern and :s//
}
} On Tue, 7 Jul 2015 08:48:51 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > So with histsubstpattern set it looks like $b is expanded before the
} > substitution takes place, but with nohistsubstpattern (the default,
} > which is why I never showed the option change in any of my previous
} > examples) the expansion happens after the substitution.
} 
} Oh, in that case it's perfectly sensible.

Except that's not QUITE what's going on ... if the expansion were simply
being done before the replacement, this wouldn't work:

torch% a=(abc dbe fbg)
torch% setopt histsubstpattern extendedglob
torch% print -R ${a:s/(#b)b(?)/${match}X/}
acX deX fgX
torch% a="$a"
torch% print -R ${a:gs/(#b)b(?)/${match}X/}
acX deX fgX

So what's actually happening (hist.c 2131-2175) is that hist_subst_pattern
causes both the left and right sides to be expanded *during* replacement
[via getmatch() -> igetmatch() in glob.c].

} The only gotcha is to remember that because modifiers operate on raw
} input, which is how they came to exist in the first place, if you
} don't want a $ to be active you need to quote it.

Not exactly the only gotcha.  The after vs. during difference is a the
least worth mentioning, particularly because of this:

torch% print *a*
config.modules.local config.status Makefile stamp-h
torch% x=':gs/(#b)(?)?/X$match/'
torch% setopt histsubstpattern extendedglob
torch% print *a*($x)          
XcXnXiX.XoXuXeX.XoXa XcXnXiX.XtXts XMXkXfXl XsXaXph

Note that I didn't even need to use $~x there, because it's expanded one
time during parsing the glob qualifier and then again during the modifier.
In fact it's actually WRONG to use $~x there:

torch% print *a*($~x)                    
zsh: bad pattern: *a*(:gs/(#b)(?)?/X$match/)

Need to think a while about the right way to document this.

Re: nohistsubstpattern and :s//

[Bart Schaefer]

Sorry to keep coming back to this ...

On Jul 7, 11:10am, Bart Schaefer wrote:
}
} Not exactly the only gotcha.  The after vs. during difference is a the
} least worth mentioning, particularly because of this:
} 
} torch% print *a*
} config.modules.local config.status Makefile stamp-h
} torch% x=':gs/(#b)(?)?/X$match/'
} torch% setopt histsubstpattern extendedglob
} torch% print *a*($x)          
} XcXnXiX.XoXuXeX.XoXa XcXnXiX.XtXts XMXkXfXl XsXaXph

OK, this is even more confusing:

torch% setopt histsubstpattern
torch% x=1
torch% !s:gs/t/$[x++]/:p
se1op1 his1subs1pa11ern
torch% a="setopt histsubstpattern"
torch% print ${a:gs/t/$[x++]/}
se2op2 his2subs2pa22ern
torch% setopt extendedglob
torch% print ${a:gs/(#b)t(?)/$match$[x++]/}
seo3p 4hiss5ubsp6at7ern
torch% print ${a:gs/t/$[x++]/}
se8op8 his8subs8pa88ern
torch% print ${a:gs/(#b)t(?)/$[x++]/}
se9p10his11ubs12a13ern
torch% !s:gs/(#b)t(?)/$[x++]/:p
se13p14his15ubs16a17ern ex18ndedglob

Not surprising that unlike no_hist_subst_pattern, history references
expand variables in the replacement.  What IS surprising is that
when there are backreferences, $match and $[x++] are evaluated for
each replacement, even if $match is not mentioned.  When there are
no backreferences, $[x++] is evaluated only once.

Re: nohistsubstpattern and :s//

[Bart Schaefer]

On Jul 7, 12:15pm, Peter Stephenson wrote:
} Subject: Re: nohistsubstpattern and :s//
}
} > 
} > torch% a=(abc dbe fbg)
} > torch% unset b
} > torch% print ${a:gs/b/$b/}  
} > a d f
} 
} Perhaps the line is turning into "a$bc", "d$be", "f$bg" before the
} substitution?

Oh!  Yes, of course.

} This is new:  the existing code gives
} 
} ac de fg

?? Existing (as in, 5.0.8) code for me gives

    a$bc d$be f

which would have immediately explained the foregoing if I had thought
to compare.