testscript: add RequireUniqueNames parameter

[twpayne]

Fixes #184.

[twpayne]

The test failure seems unrelated to this PR.

[mvdan]

Thanks! Apologies it took so long to get back to you. There's also a conflict, but I imagine it's a trivial one.

[mvdan]

Do Mac and Windows always use case insensitive filesystems?

[twpayne]

We both know that they don't always :) It's rare, but possible, for macOS users to run their systems with case sensitive filesystems, for example, even though this tends to break a large number of macOS applications.

The effort of making the tests detect whether the filesystem they're running on is case sensitive or not is fairly large. I think you'd have to create a file with a long random lowercase name (to avoid conflicts with any existing files) and test if it exists with the name uppercased. You'd have to be careful to use the exact directory that the tests run in. You can't use a temporary directory as it by be mounted as a different filesystem.

So, for now, I've removed these parts of the tests.

[twpayne]

There's also a conflict, but I imagine it's a trivial one.

The conflict is indeed trivial, but the tests no longer pass. Specifically, the test that testscript should return an error when duplicate names are encountered fails. I've traced this as far as verifying that ts.Fatalf is called in ts.setup when duplicate names are found, but this does not result in testscript returning an error. It should.

I suspect that #192 might be related. Specifically, does #192 cope with ts.Fatalf being called in ts.setup? I get the impression that the error is now silently swallowed somewhere rather than being returned. cc @rogpeppe

[rogpeppe]

@twpayne You're right: there was indeed an issue in that area that I'd overlooked. I've proposed #193 to fix that.

[mvdan]

Please rebase and try again :)

[twpayne]

Thanks both! I've rebased and now the test passes :)

[rogpeppe]

LGTM with one suggestion, thanks!

[rogpeppe]

nit: use %q rather than single quotes.
But how about an alternative approach? Instead of using an explicit LStat, just pass the write options to os.OpenFile such that it will refuse to overwrite an existing file.

You could define an alternative write-file function to make this easy; for example:

func writeFile(name string, data []byte, excl bool) error {
	oflags := os.O_WRONLY | os.O_CREATE | os.O_TRUNC
	if excl {
		oflags |= os.O_EXCL
	}
	f, err := os.OpenFile(name, oflags, 0o666)
	if err != nil {
		return err
	}
	defer f.Close()
	if _, err := f.Write(data); err != nil {
		return fmt.Errorf("cannot write file contents: %v", err)
	}
	return nil
}

Then below, do:

ts.Check(writeFile(name, f.Data, ts.params.RequireUniqueNames)

I think the resulting "file exists" error is probably sufficient, although a check of the error with os.IsExist(err) could be used to customise it.

This way we're doing exactly the same amount of work as before.

[twpayne]

But how about an alternative approach? Instead of using an explicit LStat, just pass the write options to os.OpenFile such that it will refuse to overwrite an existing file.

Yes, this is a much better approach.

This way we're doing exactly the same amount of work as before.

It's also more resistant to race conditions: creating an exclusive file is a single atomic syscall, whereas lstat/open is vulnerable to races if the file is created between the lstat and the open.

Thanks for the feedback. I'll update this PR with your suggestions.

[mvdan]

Good point Roger, I missed this!