[PATCH] Add option like tcsh's dextract

[PATCH] Add option like tcsh's dextract

[Tim Eliseo]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

Hi all!

This is my first post to the group. I’d been using tcsh as an 
interactive shell since way back when it was still cool, before zsh or 
bash even existed. Zsh is the first shell that’s a worthy successor. One 
of the small things that kept me from jumping to bash (along with 
everyone else in the Linux world) is that it doesn’t have a proper 
implementation of asynchronous notify of job completion (-b), which zsh 
does. Another thing that bugged me about bash was that there is no clean 
way to emulate tcsh’s dextract option, which rearranges the pushd stack 
differently. I eventually discovered that zsh can do the basic function 
through the cd/chdir builtin with the auto_pushd option set, but coding 
a pushd replacement function was complicated to get right for all option 
cases (see attached).

However, I found that adding this pushd mode to zsh natively was trivial 
(simply testing for the new option in one place), and I’m still baffled 
why it wasn’t included a long time ago while someone was looking for 
ways to increase compatibility with other shells. The attached patch 
(based on the current master branch) does just that, and I hope you see 
fit to merge it into the codebase. I believe I’ve done all the 
appropriate option handling, documentation, and unit test to make this 
painless. I didn’t write a ChangeLog entry since I wasn’t sure of the 
appropriate format, or how to derive the number. (Is that an SVN 
revision number?)

Tim


[-- Attachment #2: zsh-pushd_extract.patch.txt --]
[-- Type: text/plain, Size: 4905 bytes --]

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 5393cb149..47c15583b 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -1459,6 +1459,7 @@ pindex(PUSHD_TO_HOME, use of)
 pindex(PUSHD_MINUS, use of)
 pindex(CDABLE_VARS, use of)
 pindex(PUSHD_SILENT, use of)
+pindex(PUSHD_EXTRACT, use of)
 xitem(tt(pushd) [ tt(-qsLP) ] [ var(arg) ])
 xitem(tt(pushd) [ tt(-qsLP) ] var(old) var(new))
 item(tt(pushd) [ tt(-qsLP) ] {tt(PLUS())|tt(-)}var(n))(
@@ -1473,8 +1474,10 @@ Otherwise, var(arg) is interpreted as it would be by tt(cd).
 The meaning of var(old) and var(new) in the second form is also
 the same as for tt(cd).
 
-The third form of tt(pushd) changes directory by rotating the
-directory list.  An argument of the form `tt(PLUS())var(n)' identifies a stack
+The third form of tt(pushd) changes directory either by rotating the
+directory list (the default), or by extracting an entry from the directory
+list and pushing it on top (when the tt(PUSHD_EXTRACT) option is set).
+An argument of the form `tt(PLUS())var(n)' identifies a stack
 entry by counting from the left of the list shown by the tt(dirs)
 command, starting with zero.  An argument of the form `tt(-)var(n)' counts
 from the right.  If the tt(PUSHD_MINUS) option is set, the meanings
diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index cbd3d0f8e..3a3e4b629 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -162,6 +162,18 @@ shells); and any use of a component of tt(CDPATH), including a `tt(.)' but
 excluding an empty component that is otherwise treated as `tt(.)', causes
 the directory to be printed.
 )
+pindex(PUSHD_EXTRACT)
+pindex(NO_PUSHD_EXTRACT)
+pindex(PUSHDEXTRACT)
+pindex(NOPUSHDEXTRACT)
+cindex(directory stack, altering reordering)
+item(tt(PUSHD_EXTRACT))(
+A push using `tt(PLUS())' or `tt(-)' will reorder by moving the specified
+entry to the top, preserving the order of the remainder below (like the
+tt(dextract) option of bf(tcsh)), instead of the default which rotates the
+stack so that the specified entry is on top. This option makes tt(pushd)
+using `tt(PLUS())' or `tt(-)' behave like tt(cd) with tt(AUTO_PUSHD) set.
+)
 pindex(PUSHD_IGNORE_DUPS)
 pindex(NO_PUSHD_IGNORE_DUPS)
 pindex(PUSHDIGNOREDUPS)
diff --git a/NEWS b/NEWS
index 0e726699f..47c063b40 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,9 @@ consistent and better aligned with the POSIX-2017 specification of
 `set -e`. For details on what exactly changed, see the list of
 incompatibilities in the README file.
 
+The option PUSHD_EXTRACT was added to alter how pushd reorders the stack,
+in the same way as the dextract option of tcsh.
+
 Changes since 5.8.1
 -------------------
 
diff --git a/Src/builtin.c b/Src/builtin.c
index 669a47092..3b1705a0f 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -1194,7 +1194,7 @@ cd_new_pwd(int func, LinkNode dir, int quiet)
     struct stat st1, st2;
     int dirstacksize;
 
-    if (func == BIN_PUSHD)
+    if (func == BIN_PUSHD && unset(PUSHDEXTRACT))
 	rolllist(dirstack, dir);
     new_pwd = remnode(dirstack, dir);
 
diff --git a/Src/options.c b/Src/options.c
index a994b563e..b0bee7e6d 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -231,6 +231,7 @@ static struct optname optns[] = {
 {{NULL, "promptpercent",      OPT_NONBOURNE},		 PROMPTPERCENT},
 {{NULL, "promptsp",	      OPT_ALL},			 PROMPTSP},
 {{NULL, "promptsubst",	      OPT_BOURNE},		 PROMPTSUBST},
+{{NULL, "pushdextract",       OPT_EMULATE},		 PUSHDEXTRACT},
 {{NULL, "pushdignoredups",    OPT_EMULATE},		 PUSHDIGNOREDUPS},
 {{NULL, "pushdminus",	      OPT_EMULATE},		 PUSHDMINUS},
 {{NULL, "pushdsilent",	      0},			 PUSHDSILENT},
diff --git a/Src/zsh.h b/Src/zsh.h
index a0243e98e..85bd97fba 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2508,6 +2508,7 @@ enum {
     PROMPTPERCENT,
     PROMPTSP,
     PROMPTSUBST,
+    PUSHDEXTRACT,
     PUSHDIGNOREDUPS,
     PUSHDMINUS,
     PUSHDSILENT,
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 533e08773..89860c78f 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -938,6 +938,25 @@
 >waaah
 >`echo waaah`
 
+  mkdir newpd
+  cd $mydir
+  pushd $mydir/tmpcd
+  pushd $mydir/newpd
+  dirs
+  pushd +1
+  dirs
+  setopt pushdextract
+  pushd +1
+  dirs
+  unsetopt pushdextract
+  popd >/dev/null
+  popd >/dev/null
+  cd $mydir
+0q:PUSHD_EXTRACT option
+>$mydirt/newpd $mydirt/tmpcd $mydirt
+>$mydirt/tmpcd $mydirt $mydirt/newpd
+>$mydirt $mydirt/tmpcd $mydirt/newpd
+
   dirs
   pushd $mydir/tmpcd
   dirs
@@ -1459,7 +1478,7 @@ F:If this test fails at the first unsetopt, refer to P01privileged.ztst.
   fi
   BEL=$'\a'
 0q:RM_STAR_SILENT
-*>zsh: sure you want to delete all 15 files in ${PWD:h}/options.tmp \[yn\]\? ${BEL}(|n)
+*>zsh: sure you want to delete all 16 files in ${PWD:h}/options.tmp \[yn\]\? ${BEL}(|n)
 *>zsh: sure you want to delete (all <->|more than <->) files in / \[yn\]\? ${BEL}(|n)
 
   () {

[-- Attachment #3: pushd_extract.zsh --]
[-- Type: text/plain, Size: 1528 bytes --]

# In zsh, emulate the behavior of pushd with tcsh's dextract option
# Version 1.0, written by Timothy R. Eliseo

# Don't need the function def if native option exists
if ! setopt pushd_extract 2> /dev/null; then
    pushd() {
	setopt local_options extended_glob auto_pushd cd_silent
	zmodload zsh/parameter      # Need $dirstack[]

	# Gather any option flags, duplicating builtin behavior of only recognizing
	# certain options, and only before non-option arguments.
	local -a opts
	while [[ $# -gt 0 && $1 == -[qsLP]## ]]; do
	    opts+=("$1")
	    [[ $1 != *q* ]] || setopt pushd_silent
	    shift
	done

	# The chdir/cd builtin, with one argument in [+|-]n form and with
	# the auto_pushd option set, has the desired stack extract behavior,
	# instead of pushd's stack rotation. For better error output, we also
	# check if the index is in the range that would have different behavior than
	# pushd. This range check is the same regardless of +/- because pushing
	# either the first or last entry has the same result with extraction or
	# rotation.
	if [[ $# -eq 1 && ! -o posix_cd &&
	  $1 == [+-][0-9]## && $1 -ne 0 && ${1:1} -lt ${#dirstack[@]} ]]; then
	    # Use chdir to pushd with extract. cd_silent suppresses its normal
	    # output, and then we execute dirs, as pushd would, if appropriate.
	    builtin chdir "${opts[@]}" "$@" && { [[ ! -o interactive ||
	      -o pushd_silent ]] || builtin dirs; }
	else        # Otherwise just execute pushd with original args
	    builtin pushd "${opts[@]}" "$@"
	fi
    }
fi

Re: [PATCH] Add option like tcsh's dextract

[Mikael Magnusson]

On 7/18/23, Tim Eliseo <tre-zsh@crushedhat.com> wrote:
> Hi all!
>
> This is my first post to the group. I’d been using tcsh as an
> interactive shell since way back when it was still cool, before zsh or
> bash even existed. Zsh is the first shell that’s a worthy successor. One
> of the small things that kept me from jumping to bash (along with
> everyone else in the Linux world) is that it doesn’t have a proper
> implementation of asynchronous notify of job completion (-b), which zsh
> does. Another thing that bugged me about bash was that there is no clean
> way to emulate tcsh’s dextract option, which rearranges the pushd stack
> differently. I eventually discovered that zsh can do the basic function
> through the cd/chdir builtin with the auto_pushd option set, but coding
> a pushd replacement function was complicated to get right for all option
> cases (see attached).

Based on my own experiments and the hunk in the patch, it seems that
the same thing can be accomplished with setopt auto_pushd and using cd
instead of pushd? I'm not sure if it is documented that using cd
instead of pushd with auto_pushd set will not roll the list; in fact i
was not aware that the list would be rolled with pushd because i have
always been too lazy to type it instead of cd :).

> However, I found that adding this pushd mode to zsh natively was trivial
> (simply testing for the new option in one place), and I’m still baffled
> why it wasn’t included a long time ago while someone was looking for
> ways to increase compatibility with other shells. The attached patch
> (based on the current master branch) does just that, and I hope you see
> fit to merge it into the codebase. I believe I’ve done all the
> appropriate option handling, documentation, and unit test to make this
> painless. I didn’t write a ChangeLog entry since I wasn’t sure of the
> appropriate format, or how to derive the number. (Is that an SVN
> revision number?)

The number is the sequence id assigned to your mail by the mailing
list software (X-Seq: 51958) so it would have been quite difficult to
know in advance. Conventionally the changelog entry is created by the
committer of the patch.

-- 
Mikael Magnusson

Re: [PATCH] Add option like tcsh's dextract

[Tim Eliseo]

I posted a patch back in 51958 about which I haven’t seen any new 
comments or commit activity, making me think it got forgotten. Does it 
need further clarification? Maybe the one comment it did receive, which 
inferred that it was redundant, caused it to be dropped by the people 
who do merges? As my sample workaround script illustrates (which, BTW, I 
don’t intend as part of the patch), achieving the original tcsh behavior 
without a builtin option is non-trivial. The takeaway from my 
long-winded introduction is that sometimes it can be small things that 
prevent people from jumping over to zsh.

My previously posted patch still merges cleanly with current master. If 
it was actually just still in the merge queue and not forgotten, and I 
didn't wait long enough, I apologize for the bump.

Tim

On 2023-07-18 02:54:11, Tim Eliseo wrote:
> Hi all!
>
> This is my first post to the group. I’d been using tcsh as an 
> interactive shell since way back when it was still cool, before zsh or 
> bash even existed. Zsh is the first shell that’s a worthy successor. 
> One of the small things that kept me from jumping to bash (along with 
> everyone else in the Linux world) is that it doesn’t have a proper 
> implementation of asynchronous notify of job completion (-b), which 
> zsh does. Another thing that bugged me about bash was that there is no 
> clean way to emulate tcsh’s dextract option, which rearranges the 
> pushd stack differently. I eventually discovered that zsh can do the 
> basic function through the cd/chdir builtin with the auto_pushd option 
> set, but coding a pushd replacement function was complicated to get 
> right for all option cases (see attached).
>
> However, I found that adding this pushd mode to zsh natively was 
> trivial (simply testing for the new option in one place), and I’m 
> still baffled why it wasn’t included a long time ago while someone was 
> looking for ways to increase compatibility with other shells. The 
> attached patch (based on the current master branch) does just that, 
> and I hope you see fit to merge it into the codebase. I believe I’ve 
> done all the appropriate option handling, documentation, and unit test 
> to make this painless. I didn’t write a ChangeLog entry since I wasn’t 
> sure of the appropriate format, or how to derive the number. (Is that 
> an SVN revision number?)
>
> Tim
>

Re: [PATCH] Add option like tcsh's dextract

[Mikael Magnusson]

On 8/4/23, Tim Eliseo <tre-zsh@crushedhat.com> wrote:
> I posted a patch back in 51958 about which I haven’t seen any new
> comments or commit activity, making me think it got forgotten. Does it
> need further clarification? Maybe the one comment it did receive, which
> inferred that it was redundant, caused it to be dropped by the people
> who do merges?

Perhaps replying to that comment would be a start?

-- 
Mikael Magnusson

Re: [PATCH] Add option like tcsh's dextract

[Mikael Magnusson]

On 8/4/23, Tim Eliseo <tre-zsh@crushedhat.com> wrote:
> Mikael,
>
> I didn’t reply because I thought your comment showed that you didn’t
> read my post carefully, and being new to the group, didn’t want to be
> critical. I was hoping that someone else would read my original post and
> pick up the patch, but that didn’t happen.

If you didn't understand my reply, it would have been better to say so
than stay silent for several weeks. What part are you confused about?

-- 
Mikael Magnusson

Re: [PATCH] Add option like tcsh's dextract

[Bart Schaefer]

On Fri, Aug 4, 2023 at 9:47 AM Tim Eliseo <tre-zsh@crushedhat.com> wrote:
>
> As my sample workaround script illustrates [...]
> achieving the original tcsh behavior
> without a builtin option is non-trivial.

# The chdir/cd builtin, with one argument in [+|-]n form and with
# the auto_pushd option set, has the desired stack extract behavior,

I believe Mikael's assertion is that the extra nontrivial checks in
your sample workaround are not needed?  Where are you observing (or
where is Mikael failing to observe) a difference?

Re: [PATCH] Add option like tcsh's dextract

[Tim Eliseo]

On 2023-08-05 16:48:07, Bart Schaefer wrote:
> On Fri, Aug 4, 2023 at 9:47 AM Tim Eliseo <tre-zsh@crushedhat.com> wrote:
>> As my sample workaround script illustrates [...]
>> achieving the original tcsh behavior
>> without a builtin option is non-trivial.
> # The chdir/cd builtin, with one argument in [+|-]n form and with
> # the auto_pushd option set, has the desired stack extract behavior,
>
> I believe Mikael's assertion is that the extra nontrivial checks in
> your sample workaround are not needed?  Where are you observing (or
> where is Mikael failing to observe) a difference?
>
What Mikael said was “it seems that the same thing can be accomplished 
with setopt auto_pushd and using cd instead of pushd”. Yes, stack 
extract (as opposed to roll) behavior will happen using cd with a +/- 
argument and with auto_pushd set, a fact I exploit in my workaround 
script, but that does not make cd with auto_pushd set a suitable 
alternative to pushd if one desires stack extract behavior because:

• cd doesn’t print the directory stack afterwards

• cd behaves differently than pushd with no arguments (go to home 
instead of swap the top two)

• With auto_pushd set, there is no longer a command to change directory 
without pushing the current (i.e. replace the top stack directory)

My script works around this and implements a pushd replacement with 
extraction, and also has to deal with the complexity of other conditions 
like command options, pushd_silent, and interactive/non-interactive 
mode. One thing not possible to do correctly is output “pushd” instead 
of “chdir” in error messages.

I wrote this script as a first attempt to implement the behavior I 
desired (which was to duplicate tcsh’s dextract option). I now regret 
even posting it, as it seems to have caused more confusion.

BUT THERE’S A BETTER WAY! The patch in 51958 implements this new option 
(pushd stack extraction instead of rolling) very cleanly, without 
introducing any backward compatibility issues, and includes updates to 
documentation and a test case. I spent significant time trying to get 
this right, and feel it is a valuable (albeit small) addition to the zsh 
codebase which will help some other users making the transition from tcsh.

I have to say that, as a new contributor, I’m not feeling particularly 
welcomed. Do the zsh maintainers have a process for deciding to accept 
contributions? I tried to be polite and patient, and got criticized for 
it. I asked for a second look, and then got asked what *I* didn’t 
understand. What more do I need to do to move this process forward?

Thanks for your consideration.

Tim

Re: [PATCH] Add option like tcsh's dextract

[Peter Stephenson]

> On 08/08/2023 23:13 Tim Eliseo <tre-zsh@crushedhat.com> wrote:
> I have to say that, as a new contributor, I’m not feeling particularly 
> welcomed. Do the zsh maintainers have a process for deciding to accept 
> contributions? I tried to be polite and patient, and got criticized for 
> it. I asked for a second look, and then got asked what *I* didn’t 
> understand. What more do I need to do to move this process forward?

Just as an editorial comment --- you're certainly not doing anything
wrong, it's just that the list is more a random collection of people
who happen to look at things every now and then and less a formal
development group than you probably imagine.  So a bit more patience
and a bit more prodding is required than you'd perhaps naturally
expect.  Think of it as a bunch of people sitting reading the
newspaper and looking up every now and then rather than a committee
sitting expectantly looking at you...

FWIW, I agree the pushd / popd behaviour isn't ideal and I've
been using the "cdr" contributed function and its relatives for
quite a while now (see zshcontrib).  I wrote this and am happy
to update it.

pws

Re: [PATCH] Add option like tcsh's dextract

[Mikael Magnusson]

On 8/9/23, Tim Eliseo <tre-zsh@crushedhat.com> wrote:
> On 2023-08-05 16:48:07, Bart Schaefer wrote:
>> On Fri, Aug 4, 2023 at 9:47 AM Tim Eliseo <tre-zsh@crushedhat.com> wrote:
>>> As my sample workaround script illustrates [...]
>>> achieving the original tcsh behavior
>>> without a builtin option is non-trivial.
>> # The chdir/cd builtin, with one argument in [+|-]n form and with
>> # the auto_pushd option set, has the desired stack extract behavior,
>>
>> I believe Mikael's assertion is that the extra nontrivial checks in
>> your sample workaround are not needed?  Where are you observing (or
>> where is Mikael failing to observe) a difference?
>>
> What Mikael said was “it seems that the same thing can be accomplished
> with setopt auto_pushd and using cd instead of pushd”. Yes, stack
> extract (as opposed to roll) behavior will happen using cd with a +/-
> argument and with auto_pushd set, a fact I exploit in my workaround
> script, but that does not make cd with auto_pushd set a suitable
> alternative to pushd if one desires stack extract behavior because:
>
> • cd doesn’t print the directory stack afterwards
>
> • cd behaves differently than pushd with no arguments (go to home
> instead of swap the top two)
>
> • With auto_pushd set, there is no longer a command to change directory
> without pushing the current (i.e. replace the top stack directory)
>
> My script works around this and implements a pushd replacement with
> extraction, and also has to deal with the complexity of other conditions
> like command options, pushd_silent, and interactive/non-interactive
> mode. One thing not possible to do correctly is output “pushd” instead
> of “chdir” in error messages.

This would all have been good information to include in the original
post, or as a followup to my initial mail which you never replied to,
where I was clearly not aware of these differences:
> in fact i
> was not aware that the list would be rolled with pushd because i have
> always been too lazy to type it instead of cd :).

> I wrote this script as a first attempt to implement the behavior I
> desired (which was to duplicate tcsh’s dextract option). I now regret
> even posting it, as it seems to have caused more confusion.

The mistake was including some details only in the comments of the
script and not in the mail, I did overlook some comments in it.

> BUT THERE’S A BETTER WAY! The patch in 51958 implements this new option
> (pushd stack extraction instead of rolling) very cleanly, without
> introducing any backward compatibility issues, and includes updates to
> documentation and a test case. I spent significant time trying to get
> this right, and feel it is a valuable (albeit small) addition to the zsh
> codebase which will help some other users making the transition from tcsh.
>
> I have to say that, as a new contributor, I’m not feeling particularly
> welcomed. Do the zsh maintainers have a process for deciding to accept
> contributions? I tried to be polite and patient, and got criticized for
> it. I asked for a second look, and then got asked what *I* didn’t
> understand. What more do I need to do to move this process forward?

Every time I've asked for more details you ignored me, which as I
pointed out earlier is not a good approach. I thought I very clearly
asked you in my first mail why just cd + autopushd was not enough, and
then you wouldn't even reply to my second request to answer my
question which surely put others off as well.

To summarize, when you send a mail with a patch, explain the patch and
why it's needed in the mail; if someone asks for clarifications or
seems to misunderstand you, rather than assume that they can't read,
assume that you were unclear, it is the safer assumption.

-- 
Mikael Magnusson

Re: [PATCH] Add option like tcsh's dextract

[Tim Eliseo]

On 2023-08-09 06:16:54, Mikael Magnusson wrote:
>
> Every time I've asked for more details you ignored me, which as I
> pointed out earlier is not a good approach. I thought I very clearly
> asked you in my first mail why just cd + autopushd was not enough, and
> then you wouldn't even reply to my second request to answer my
> question which surely put others off as well.
>
> To summarize, when you send a mail with a patch, explain the patch and
> why it's needed in the mail; if someone asks for clarifications or
> seems to misunderstand you, rather than assume that they can't read,
> assume that you were unclear, it is the safer assumption.
>
I’ll agree that we had a misunderstanding, and will keep this in mind 
for the future.

Do you have any remaining questions or concerns that prevent this patch 
from being merged?

Tim

Re: [PATCH] Add option like tcsh's dextract

[Mikael Magnusson]

On 8/9/23, Tim Eliseo <tre-zsh@crushedhat.com> wrote:
> On 2023-08-09 06:16:54, Mikael Magnusson wrote:
>>
>> Every time I've asked for more details you ignored me, which as I
>> pointed out earlier is not a good approach. I thought I very clearly
>> asked you in my first mail why just cd + autopushd was not enough, and
>> then you wouldn't even reply to my second request to answer my
>> question which surely put others off as well.
>>
>> To summarize, when you send a mail with a patch, explain the patch and
>> why it's needed in the mail; if someone asks for clarifications or
>> seems to misunderstand you, rather than assume that they can't read,
>> assume that you were unclear, it is the safer assumption.
>>
> I’ll agree that we had a misunderstanding, and will keep this in mind
> for the future.
>
> Do you have any remaining questions or concerns that prevent this patch
> from being merged?

The manpage entry says:
  This option makes tt(pushd) using `tt(PLUS())' or `tt(-)' behave
like tt(cd) with tt(AUTO_PUSHD) set.

This is true as I understand it, but it somewhat implies that pushd
without +/- acts differently from cd, so perhaps it should just say
"This option makes pushd behave like cd with AUTO_PUSHD set.", which
is also true but harder to misinterpret. (I wouldn't oppose keeping it
as it is, it's just something that tripped me up while reading it.)

Another thought I had was that perhaps with this option existing, the
behavior between cd and pushd need not be different at all anymore,
but maybe someone prefers being able to extract with cd and roll with
pushd, so probably not worth messing with that. It would also be
unclear which way the default should be in that case...

-- 
Mikael Magnusson

Re: [PATCH] Add option like tcsh's dextract

[Bart Schaefer]

On Wed, Aug 9, 2023 at 9:25 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On 8/9/23, Tim Eliseo <tre-zsh@crushedhat.com> wrote:
> >
> > Do you have any remaining questions or concerns that prevent this patch
> > from being merged?
>
> The manpage entry says:
>   This option makes tt(pushd) using `tt(PLUS())' or `tt(-)' behave
> like tt(cd) with tt(AUTO_PUSHD) set.

Similar minor nit:

+A push using `tt(PLUS())' or `tt(-)' will reorder by moving the specified

I realize other parts of the manual are not consistent about this, but
passive voice should be avoided, i.e., say "reorders by moving" rather
than "will reorder".

Aside to zsh-workers in general: good grief, the bf() yodl macro is
really inconsistently applied.  So much so that I didn't even realize
it existed until I read this patch.  I can think of at least a few
uses of em() that would probably be better as bf().

Re: [PATCH] Add option like tcsh's dextract

[Tim Eliseo]

[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]

On 2023-08-09 09:24:53, Mikael Magnusson wrote:
> The manpage entry says:
>    This option makes tt(pushd) using `tt(PLUS())' or `tt(-)' behave
> like tt(cd) with tt(AUTO_PUSHD) set.
>
> This is true as I understand it, but it somewhat implies that pushd
> without +/- acts differently from cd, so perhaps it should just say
> "This option makes pushd behave like cd with AUTO_PUSHD set.", which
> is also true but harder to misinterpret. (I wouldn't oppose keeping it
> as it is, it's just something that tripped me up while reading it.)

I qualified this because pushd with no arguments _does_ act differently 
from cd. Pushd (without the pushd_to_home option set) swaps the top two 
directories on the stack, while cd always goes to the home directory, 
never swaps (with or without auto_pushd).

Maybe a better rewording is something like “cd using + or - always 
extracts from the directory stack, never rotates it, regardless of the 
setting of this option.” In this way pushd becomes the reference point 
for stack manipulation (as it should be), instead of cd. The previous 
wording more belongs in the Changelog, since it’s describing what 
behavior is new relative to existing, something that is unimportant to a 
user reading the man page.

> Another thought I had was that perhaps with this option existing, the
> behavior between cd and pushd need not be different at all anymore,
> but maybe someone prefers being able to extract with cd and roll with
> pushd, so probably not worth messing with that.

I personally don’t think that the + and - variants belong with cd at 
all, because with auto_pushd it’s too similar to pushd, and without 
auto_pushd the stack rearrangement doesn’t seem very intuitive or useful 
in either case (rotate or extract). Also the stack isn’t printed after 
the command. If you have a stack that looks like:

     dir1 dir2 dir3 dir4 dir5

Current behavior of “cd +2” doing extract without auto_pushd ends up with:

     dir3 dir2 dir4 dir5

A “cd +2” doing rotate without auto_pushd (not currently possible) might 
end up with:

     dir3 dir4 dir5 dir2

Of course, we’re stuck with it now, but zsh is the only shell that has 
this. Also, as you said, someone might want to be able to use both 
extract and rotate options.

> It would also be unclear which way the default should be in that case...
>
I think unquestionably the default for pushd should be rotate, since all 
other shells that implement pushd (csh, tcsh, bash, and fish) rotate, 
with only tcsh providing the dextract option to switch to extract. If we 
were to implement a separate cd_extract option, I say it should be set 
by default, to match current behavior.

If the goal with this is to allow users to choose rotate or extract, we 
should also add command line options to override, which users can add to 
alias definitions.

I’m going to think about this some more. I’ve been working on a cd and 
pushd feature matrix for all the shells, which I’ll post later.

Tim

[-- Attachment #2: Type: text/html, Size: 4017 bytes --]

Re: [PATCH] Add option like tcsh's dextract

[Tim Eliseo]

On 2023-08-09 10:25:39, Bart Schaefer wrote:
> On Wed, Aug 9, 2023 at 9:25 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>> On 8/9/23, Tim Eliseo <tre-zsh@crushedhat.com> wrote:
>>> Do you have any remaining questions or concerns that prevent this patch
>>> from being merged?
>> The manpage entry says:
>>    This option makes tt(pushd) using `tt(PLUS())' or `tt(-)' behave
>> like tt(cd) with tt(AUTO_PUSHD) set.
> Similar minor nit:
>
> +A push using `tt(PLUS())' or `tt(-)' will reorder by moving the specified
>
> I realize other parts of the manual are not consistent about this, but
> passive voice should be avoided, i.e., say "reorders by moving" rather
> than "will reorder".
Okay, will fix the passive voice, or maybe it’s present instead of 
future tense. Hard to keep track.
> Aside to zsh-workers in general: good grief, the bf() yodl macro is
> really inconsistently applied.  So much so that I didn't even realize
> it existed until I read this patch.  I can think of at least a few
> uses of em() that would probably be better as bf().

I used bf()  because I saw it used for other external command names. Is 
that correct?

Tim

Re: [PATCH] Add option like tcsh's dextract

[Bart Schaefer]

On Wed, Aug 9, 2023 at 4:27 PM Tim Eliseo <tre-zsh@crushedhat.com> wrote:
>
> I used bf()  because I saw it used for other external command names. Is
> that correct?

Your choice was consistent with the way the names of other shells are
presented elsewhere, it's fine.  I was merely remarking that there may
be a number of other places (not in your patch) that should have used
bf() but didn't.