* [PATCH] Add option like tcsh's dextract
@ 2023-07-18 9:54 Tim Eliseo
2023-07-18 15:17 ` Mikael Magnusson
2023-08-04 16:47 ` Tim Eliseo
0 siblings, 2 replies; 15+ messages in thread
From: Tim Eliseo @ 2023-07-18 9:54 UTC (permalink / raw)
To: zsh-workers
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-07-18 9:54 [PATCH] Add option like tcsh's dextract Tim Eliseo
@ 2023-07-18 15:17 ` Mikael Magnusson
2023-08-04 16:47 ` Tim Eliseo
1 sibling, 0 replies; 15+ messages in thread
From: Mikael Magnusson @ 2023-07-18 15:17 UTC (permalink / raw)
To: Tim Eliseo; +Cc: zsh-workers
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-07-18 9:54 [PATCH] Add option like tcsh's dextract Tim Eliseo
2023-07-18 15:17 ` Mikael Magnusson
@ 2023-08-04 16:47 ` Tim Eliseo
2023-08-04 16:58 ` Mikael Magnusson
2023-08-05 23:48 ` Bart Schaefer
1 sibling, 2 replies; 15+ messages in thread
From: Tim Eliseo @ 2023-08-04 16:47 UTC (permalink / raw)
To: zsh-workers
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-04 16:47 ` Tim Eliseo
@ 2023-08-04 16:58 ` Mikael Magnusson
[not found] ` <ee258794-761e-8f48-01d2-f0db6d8b6dc7@crushedhat.com>
2023-08-05 23:48 ` Bart Schaefer
1 sibling, 1 reply; 15+ messages in thread
From: Mikael Magnusson @ 2023-08-04 16:58 UTC (permalink / raw)
To: Tim Eliseo; +Cc: zsh-workers
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
[not found] ` <ee258794-761e-8f48-01d2-f0db6d8b6dc7@crushedhat.com>
@ 2023-08-04 18:08 ` Mikael Magnusson
0 siblings, 0 replies; 15+ messages in thread
From: Mikael Magnusson @ 2023-08-04 18:08 UTC (permalink / raw)
To: Tim Eliseo; +Cc: zsh-workers
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-04 16:47 ` Tim Eliseo
2023-08-04 16:58 ` Mikael Magnusson
@ 2023-08-05 23:48 ` Bart Schaefer
2023-08-08 22:13 ` Tim Eliseo
1 sibling, 1 reply; 15+ messages in thread
From: Bart Schaefer @ 2023-08-05 23:48 UTC (permalink / raw)
To: Tim Eliseo; +Cc: zsh-workers
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?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-05 23:48 ` Bart Schaefer
@ 2023-08-08 22:13 ` Tim Eliseo
2023-08-09 8:50 ` Peter Stephenson
2023-08-09 13:16 ` Mikael Magnusson
0 siblings, 2 replies; 15+ messages in thread
From: Tim Eliseo @ 2023-08-08 22:13 UTC (permalink / raw)
To: Bart Schaefer; +Cc: zsh-workers
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-08 22:13 ` Tim Eliseo
@ 2023-08-09 8:50 ` Peter Stephenson
2023-08-09 13:16 ` Mikael Magnusson
1 sibling, 0 replies; 15+ messages in thread
From: Peter Stephenson @ 2023-08-09 8:50 UTC (permalink / raw)
To: Tim Eliseo, zsh-workers
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-08 22:13 ` Tim Eliseo
2023-08-09 8:50 ` Peter Stephenson
@ 2023-08-09 13:16 ` Mikael Magnusson
2023-08-09 14:19 ` Tim Eliseo
1 sibling, 1 reply; 15+ messages in thread
From: Mikael Magnusson @ 2023-08-09 13:16 UTC (permalink / raw)
To: Tim Eliseo; +Cc: Bart Schaefer, zsh-workers
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-09 13:16 ` Mikael Magnusson
@ 2023-08-09 14:19 ` Tim Eliseo
2023-08-09 16:24 ` Mikael Magnusson
0 siblings, 1 reply; 15+ messages in thread
From: Tim Eliseo @ 2023-08-09 14:19 UTC (permalink / raw)
To: Mikael Magnusson; +Cc: zsh-workers
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-09 14:19 ` Tim Eliseo
@ 2023-08-09 16:24 ` Mikael Magnusson
2023-08-09 17:25 ` Bart Schaefer
2023-08-09 22:49 ` Tim Eliseo
0 siblings, 2 replies; 15+ messages in thread
From: Mikael Magnusson @ 2023-08-09 16:24 UTC (permalink / raw)
To: Tim Eliseo; +Cc: zsh-workers
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-09 16:24 ` Mikael Magnusson
@ 2023-08-09 17:25 ` Bart Schaefer
2023-08-09 23:27 ` Tim Eliseo
2023-08-09 22:49 ` Tim Eliseo
1 sibling, 1 reply; 15+ messages in thread
From: Bart Schaefer @ 2023-08-09 17:25 UTC (permalink / raw)
To: Mikael Magnusson; +Cc: Tim Eliseo, zsh-workers
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().
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-09 16:24 ` Mikael Magnusson
2023-08-09 17:25 ` Bart Schaefer
@ 2023-08-09 22:49 ` Tim Eliseo
1 sibling, 0 replies; 15+ messages in thread
From: Tim Eliseo @ 2023-08-09 22:49 UTC (permalink / raw)
To: Mikael Magnusson; +Cc: zsh-workers
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-09 17:25 ` Bart Schaefer
@ 2023-08-09 23:27 ` Tim Eliseo
2023-08-12 3:42 ` Bart Schaefer
0 siblings, 1 reply; 15+ messages in thread
From: Tim Eliseo @ 2023-08-09 23:27 UTC (permalink / raw)
To: Bart Schaefer; +Cc: zsh-workers
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Add option like tcsh's dextract
2023-08-09 23:27 ` Tim Eliseo
@ 2023-08-12 3:42 ` Bart Schaefer
0 siblings, 0 replies; 15+ messages in thread
From: Bart Schaefer @ 2023-08-12 3:42 UTC (permalink / raw)
To: Tim Eliseo; +Cc: zsh-workers
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-08-12 3:43 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-18 9:54 [PATCH] Add option like tcsh's dextract Tim Eliseo
2023-07-18 15:17 ` Mikael Magnusson
2023-08-04 16:47 ` Tim Eliseo
2023-08-04 16:58 ` Mikael Magnusson
[not found] ` <ee258794-761e-8f48-01d2-f0db6d8b6dc7@crushedhat.com>
2023-08-04 18:08 ` Mikael Magnusson
2023-08-05 23:48 ` Bart Schaefer
2023-08-08 22:13 ` Tim Eliseo
2023-08-09 8:50 ` Peter Stephenson
2023-08-09 13:16 ` Mikael Magnusson
2023-08-09 14:19 ` Tim Eliseo
2023-08-09 16:24 ` Mikael Magnusson
2023-08-09 17:25 ` Bart Schaefer
2023-08-09 23:27 ` Tim Eliseo
2023-08-12 3:42 ` Bart Schaefer
2023-08-09 22:49 ` Tim Eliseo
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).