mvdan / sh

A shell parser, formatter, and interpreter with bash support; includes shfmt
https://pkg.go.dev/mvdan.cc/sh/v3
BSD 3-Clause "New" or "Revised" License
7.29k stars 345 forks source link

builtin(cd): Support hyphen-minus operand #766

Closed donorp closed 2 years ago

donorp commented 2 years ago

Hi, thanks for this amazing package, I'm relying on it for cross platform build automation tasks.

And I came across the unexpected behavior of cd -, which errored when there is no directory named -, instead of chdir into last working directory.

In this pull request, the existence of directory named - in the current working directory is not checked as the man page says cd - is actually an alias of cd ${OLDPWD}, so to my understanding, a directory named - is ignored in this case (and I can confirm ash, zsh, bash on macos and linux all have this behavior).

donorp commented 2 years ago

may be related to the cd issue mentioned in #705

donorp commented 2 years ago

cd implemented in ash, bash, zsh errors if there is no OLDPWD env set, but in this package, OLDPWD is always set before running, so the following patch does nothing (and not applied in this pull request).

diff --git a/interp/builtin.go b/interp/builtin.go
index 08bbdf6c..5cb910f6 100644
--- a/interp/builtin.go
+++ b/interp/builtin.go
@@ -218,7 +218,12 @@ func (r *Runner) builtinCode(ctx context.Context, pos syntax.Pos, name string, a
            // replicate the commonly implemented behavior of `cd -`
            // ref: https://www.man7.org/linux/man-pages/man1/cd.1p.html#OPERANDS
            if len(path) == 1 && path[0] == '-' {
-               path = r.envGet("OLDPWD")
+               v := r.lookupVar("OLDPWD")
+               if !v.IsSet() {
+                   r.errf("cd: OLDPWD not set\n")
+                   return 1
+               }
+               path = v.String()
            }
        default:
            r.errf("usage: cd [dir]\n")

I guess we are fine with such minor behavior difference from traditional shells.

mvdan commented 2 years ago

I guess we are fine with such minor behavior difference from traditional shells.

I agree that the behavior you added is fine.

may be related to the cd issue mentioned in #705

Not sure - they didn't really give enough details :)