ksh93 diff for _cd() in dirs

[Andras Farkas]

https://github.com/att/ast/blob/master/src/cmd/ksh93/functions/dirs
I have a diff which solves an issue.
Right now, the _cd in this file errors if you call cd without arguments.
However, it is clear to me that this is unintended. This cd is meant
to be compatible with regular ksh93 cd.
This diff fixes it.

[Kurtis Rader]

Thank you for bringing this to our attention. Yes, the current function definition is broken. However, your fix introduces a different bug. I've opened a pull request to fix this and a related bug having to do with how autoloaded functions are found and loaded. See

https://github.com/att/ast/pull/1420

P.S., It's always a good idea to mention the version of the program you are using and the platform you are running it on. 

--

Kurtis Rader

Caretaker of the exceptional canines Junior and Hank

[Andras Farkas]

On Sat, Oct 26, 2019 at 1:21 AM Kurtis Rader <kra...@skepticism.us> wrote:
> Thank you for bringing this to our attention. Yes, the current function definition is broken. However, your fix introduces a different bug. I've opened a pull request to fix this and a related bug having to do with how autoloaded functions are found and loaded. See

Oh, huh. What bug does my fix introduce? I'd like to know and understand.

> https://github.com/att/ast/pull/1420
> P.S., It's always a good idea to mention the version of the program you are using and the platform you are running it on.

I tested my diff both in u+ on KDE Neon (basically Ubuntu) and in
2020.0.0-beta1 on FreeBSD

[Andras Farkas]

Please just fix the bug before you add or change any functionality. I
just read through the github issue.
You can change things later.
That's a ridiculous huge diff you have there on github, and I still
haven't been told what bug my diff introduces.

[Andras Farkas]

what the fuck, dude

[Kurtis Rader]

On Tue, Oct 29, 2019 at 10:19 PM Andras Farkas <deepblu...@gmail.com> wrote:

Please just fix the bug before you add or change any functionality.  I
just read through the github issue.
You can change things later.
That's a ridiculous huge diff you have there on github, and I still
haven't been told what bug my diff introduces.

Please think about how your rant is likely to be perceived by others. Hint: it makes me want to ignore you.

The "huge diff" is because an earlier change to always make these functions available overlooked the fact the structure of the code meant you would have to run "dirs" in order to automatically have access to the augmented "cd" and "mcd" commands. We are not fixing bugs in the ksh93u+ release. We are trying to keep ksh relevant by improving the project. This means making the "cd" function available by default rather than requiring the user to "source /some/path/dirs" first. The bug introduced by your change is in assuming `dir` and `$#` are interchangeable.

[Andras Farkas]

It wasn't a rant. It was me being curious about the lack of
communication, and curious about why a bug fix and other diff were
rolled into one instead of being separate.
I'm sorry if you interpreted my email as something it wasn't. My
apologies, how could I have been clearer? I thought I was being
polite, and writing in a polite tone. (unlike you right now)

> The bug introduced by your change is in assuming `dir` and `$#` are interchangeable

I did not make that assumption. I used $# for what it was meant for:
to figure out if any arguments were given.

[Andras Farkas]

On Sat, Nov 2, 2019 at 12:24 AM Kurtis Rader <kra...@skepticism.us> wrote:
> Your tone was not polite. The "lack of communication" is because the Korn shell project is now entirely dependent on unpaid contributors who have "day jobs". Which is to say, you can't reasonably expect an immediate response.
I wasn't expecting anything immediate. uwu I did get a reply fast
enough when I was told I introduced a bug. But what it was wasn't
mentioned in the same email.
I had also seen that stuff was going on in the GitHub pull request
already while leaving me out of the loop.
This has never happened to me in places like the OpenBSD mailing lists, etc.
> Your argument for using "$#" does not negate the fact the `_cd` function might have set `dir` independent of whether `$#` is non-zero.
True, it could've been set, but the code's logic shows this could not
possibly happen.
https://github.com/att/ast/blob/6511635fb5dca43417e3771c510b7ff59e425415/src/cmd/ksh93/functions/dirs
With a null argument or nonexistent argument to _cd, none of the code
that sets dir (other than the first typeset to null) is traversed
prior to the actual \cd, where the value of dir is used. set -xv
confirms this.
Thank you for this explanation though.

[Andras Farkas]

On Tue, Nov 5, 2019 at 12:16 AM Kurtis Rader <kra...@skepticism.us> wrote:

>
> On Fri, Nov 1, 2019 at 9:43 PM Andras Farkas <deepblu...@gmail.com> wrote:
>>
>> I had also seen that stuff was going on in the GitHub pull request
>> already while leaving me out of the loop.
>> This has never happened to me in places like the OpenBSD mailing lists, etc.
>
>

> What are you talking about? You did not create a pull request. I did. How should I have included you "in the loop" beyond what I wrote in this discussion?

>
> > Your argument for using "$#" does not negate the fact the `_cd` function might have set `dir` independent of whether `$#` is non-zero.
>>
>> True, it could've been set, but the code's logic shows this could not
>> possibly happen.
>> https://github.com/att/ast/blob/6511635fb5dca43417e3771c510b7ff59e425415/src/cmd/ksh93/functions/dirs
>> With a null argument or nonexistent argument to _cd, none of the code
>> that sets dir (other than the first typeset to null) is traversed
>> prior to the actual \cd, where the value of dir is used. set -xv
>> confirms this.
>
>

> That is irrelevant. My point was that given how the code is structured, with or without your change, you should not assume $# is relevant where you use it. It is safer to reference `$dir`; especially to future proof that code.
Thank you. (genuinely!) Your explanation is finally clear. You have
a good point, and I'll keep it in mind.
>
> P.S., I am deliberately not including the mailing list address in this reply. Same as my earlier, private, replies in order to avoid making disagreements that are personality based public. That you are deliberately making them public tells me you are an asshole.
In the world of open-source: nothing to fear, nothing to hide.
You clearly have an agenda, and it shows.
You want to sweep things under the rug and stifle people and you find
it convenient to do so in this google group.
I'm sure you'd be behaving at least slightly differently if I had a
GitHub account and was discussing this over there instead, under a
larger public eye.
But even so, there's much evidence you're just as hostile and
close-minded there:
https://github.com/att/ast/issues/240#issuecomment-357861900
https://github.com/att/ast/issues/240#issuecomment-358534787
https://github.com/att/ast/issues/240#issuecomment-358578848
https://github.com/att/ast/issues/240#issuecomment-359168532
kdudka is especially right about you.
With the way you behave, please consider renaming the shell to Kurtis
shell. The asshole, unfortunately, is you.

[Thomas Swan]

The patch to change cd behavior in the shell breaks by causing repeated cd operations without a cd - to exhaust the stack.  It is an unintended consequence which I consider a regression.  Shell scripts may run for a long time sometimes years with repeated operations. I have seen and worked with scripts that monitored automount directories by changing to the target directory.  These scripts would run continuously and they never used the cd - or the new cd -{n} operator.  cd only maintained the previous directory (one state and one state only).  cd - would fail if no previous directory had been set.  cd - should toggle between two directories assuming a previous directory exists as OLDPWD.  Pushing each directory into a stack guarantees that there is a finite number of chdir operations which can happen before cd fails.

If someone is interested in changing directories and recalling to a specific depth, they should be using pushd/popd rather than changing cd to a stack based operation.

On the behaviour of outputting to stderr the name of the new working directory.  It is based on the premise of display the result which is not literally on the line.  Consider the following example.

$ mkdir -p /tmp/$(id -un)/{a/{c.d},e/{f,g}}

$ CDPATH=/tmp/$(id -un)/a:/tmp/$(id -un)/e:.

$ cd /tmp/$(id -un)

$ cd g

/tmp/testuser/e/g

$

cd prints the new working directory on stderr to notify the user that the new working directory is different than the argument relative to the current working directory.

The same applies to the next example.

$ cd -

It does not chdir to "-" it changes the current directory to the value set in $OLDPWD if it has been set by a previous cd.   If no previous directory had been set, cd - would result in an error which is correct.

I would respectfully ask that this change be rejected or reverted.

--
You received this message because you are subscribed to the Google Groups "Korn Shell" group.
To unsubscribe from this group and stop receiving emails from it, send an email to korn-shell+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/korn-shell/CAA0nTRtxE9OvrNutLd9TBAEEFiwC27B%2BJWigiJEt1hSKs7v3GA%40mail.gmail.com.

--

Thomas Swan

[Kurtis Rader]

On Wed, Nov 6, 2019 at 2:06 PM Thomas Swan <thoma...@gmail.com> wrote:

The patch to change cd behavior in the shell breaks by causing repeated cd operations without a cd - to exhaust the stack.  It is an unintended consequence which I consider a regression.  Shell scripts may run for a long time sometimes years with repeated operations. I have seen and worked with scripts that monitored automount directories by changing to the target directory.  These scripts would run continuously and they never used the cd - or the new cd -{n} operator.  cd only maintained the previous directory (one state and one state only).  cd - would fail if no previous directory had been set.  cd - should toggle between two directories assuming a previous directory exists as OLDPWD.  Pushing each directory into a stack guarantees that there is a finite number of chdir operations which can happen before cd fails.

Thank you, Thomas, for bringing this to our attention. Your bug report is a good example for why I wanted to enable these functions by default in the standard ksh distribution. I am not surprised these functions have bugs. That's because for all intents and purposes no one was using them. Yet they were included in the ksh source code as functions that people should use. Note that these functions, including the `_cd` override are useful, even if you don't like the `pushd`/`popd` pattern (which includes me). Nonetheless, overriding the `cd` builtin is still useful for enabling functions like `mcd` (menu change dir). The solution is to fix the bugs in these long neglected functions -- not to remove them.

 

If someone is interested in changing directories and recalling to a specific depth, they should be using pushd/popd rather than changing cd to a stack based operation.

The `cd` command has to cooperate in maintaining the directory stack for the other functions, such as `mcd`, to work correctly.

 

On the behaviour of outputting to stderr the name of the new working directory.  It is based on the premise of display the result which is not literally on the line.  Consider the following example.

$ mkdir -p /tmp/$(id -un)/{a/{c.d},e/{f,g}}

$ CDPATH=/tmp/$(id -un)/a:/tmp/$(id -un)/e:.

$ cd /tmp/$(id -un)

$ cd g

/tmp/testuser/e/g

$

cd prints the new working directory on stderr to notify the user that the new working directory is different than the argument relative to the current working directory.

I can see the value in that argument. Nonetheless, I think it is noise and should be omitted. The fish shell, which has a `cd` function that augments the `cd` builtin`, does not do this. And that is a far friendlier interactive shall than ksh is. 

[Kurtis Rader]

@Thomas, Are you actually using ksh built from the master branch of this project or some other version I ask because doing lots of `cd` commands work correctly and the "cd" stack is (more or less) correctly handled. It is not clear what you mean when you say "to exhaust the stack.". In my testing the "cd" stack is bounded.

On Wed, Nov 6, 2019 at 2:06 PM Thomas Swan <thoma...@gmail.com> wrote:

[Andras Farkas]

On Wed, Nov 6, 2019 at 5:06 PM Thomas Swan <thoma...@gmail.com> wrote:
> On the behaviour of outputting to stderr the name of the new working directory. It is based on the premise of display the result which is not literally on the line. Consider the following example.

Agreed. It's the same reason why `cd - ` shows what directory you
change to, as you state.

> It does not chdir to "-" it changes the current directory to the value set in $OLDPWD if it has been set by a previous cd. If no previous directory had been set, cd - would result in an error which is correct.
>
> I would respectfully ask that this change be rejected or reverted.

I fully agree with you. It has always been clear to me that
dirs/_cd/mcd was meant to be optional and opt-in for the user, from
the way it was presented and distributed.
But I do think it's good they're included with ksh as an option, and
thus should work right when people enable them.

[Andras Farkas]

On Wed, Nov 6, 2019 at 10:38 PM Kurtis Rader <kra...@skepticism.us> wrote:
> Thank you, Thomas, for bringing this to our attention. Your bug report is a good example for why I wanted to enable these functions by default in the standard ksh distribution. I am not surprised these functions have bugs. That's because for all intents and purposes no one was using them. Yet they were included in the ksh source code as functions that people should use. Note that these functions, including the `_cd` override are useful, even if you don't like the `pushd`/`popd` pattern (which includes me). Nonetheless, overriding the `cd` builtin is still useful for enabling functions like `mcd` (menu change dir). The solution is to fix the bugs in these long neglected functions -- not to remove them.

Both the way they're included with ksh (in the /examples/ directory)
and especially the way they're presented in the book The New KornShell
Command and Programming Language it's clear that they're example code
people _could_ use rather than _should_ use. On page 267: "This
chapter contains several functions and programs written in the
KornShell language. They are included here primarily for illustrative
purposes. However, you may find some of these functions and/or
programs useful." That said, yes, all these should be made to work,
but it's clear they're opt-in rather than opt-out. The rest of the
KornShell book doesn't use any of the functions in chapter 17 (which
includes dirs, _cd, mcd, popd, and pushd, in nearly the same form as
they're included with ksh)
Also, while that's said, I don't mean to slavishly adhere to the past:
I just don't think it makes sense for these to be enabled by default.
Perhaps better advertising their existence can get more people to use
them! :D

> The `cd` command has to cooperate in maintaining the directory stack for the other functions, such as `mcd`, to work correctly.

Correct.

> I can see the value in that argument. Nonetheless, I think it is noise and should be omitted. The fish shell, which has a `cd` function that augments the `cd` builtin`, does not do this. And that is a far friendlier interactive shall than ksh is.

Not everyone has a perfect memory.
Fortunately, since the _cd command is a shell function, its
customisable by the user. You could comment out the lines that print
the directory when using cd -, while using _cd.

[Andras Farkas]

On Wed, Nov 6, 2019 at 10:38 PM Kurtis Rader <kra...@skepticism.us> wrote:
> I can see the value in that argument. Nonetheless, I think it is noise and should be omitted. The fish shell, which has a `cd` function that augments the `cd` builtin`, does not do this. And that is a far friendlier interactive shall than ksh is.

An additional comment I have on this is that people who use ksh have a
set of expectations originating both in ksh and the bourne shell. To
have `cd -` and other similar commands no longer print the directory
changed to would violate the principle of least astonishment. This is
because people who use ksh have used ksh and sh before. If we
expected most people using ksh to have used fish before, then we (and
they) would have different expectations.