Add FOR Command

[MeAreJeenius]

Description

Adds the DOS FOR command.

There are also a few supporting changes:

Tweaked the split functions.

Fixed a few memory leaks in tests.

Added some unit tests.

More details in a comment below.

Related issues

#448

Manual testing

Make sure that parameters with wildcards (* and ?) return the correct files in the current directory.
ie. FOR %T IN (*.BAT) DO ECHO %T should echo all .BAT files in the current directory.

Everything else should (hopefully) be covered by unit tests.

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

• followed the project's contributing guidelines and code of conduct.
• performed a self-review of my code.
• commented on the particularly hard-to-understand areas of my code.
• split my work into well-defined, bisectable commits, and I named my commits well.
• applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
• checked that all my commits can be built.
• confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
• added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
• made corresponding changes to the documentation or the website according to the documentation guidelines.
• locally verified my website or documentation changes.

[MeAreJeenius]

Below is a copy-pasted comment from a couple weeks ago on the original PR which details what changed since everyone originally looked at it:

Changes

Changed the split() function taking a single char to split_with_empties() to reflect the function's unintuitive semantics, and differentiating it from the string_view version, which behaves as expected (or at least how I expect).

You were actually right about using format_string() @johnnovak. For whatever reason, I didn't come to the realization that I could simply change the character used by the variable to an "s" after the parsing stage, making it valid for formatting. Whoops. This moves the format-string creation out of the loop to just before the loop.

Changed some bad variable names.

Tests

Since the shell isn't in a very testable state, the current setup with the test fixture isn't the greatest, the tests aren't as clean or granular as I'd like, but what's there should still be helpful.

Most of the tests are making sure that the echo command isn't called when it's missing a token. The most important tests is the eyesore with the delimiters (which you can't miss). It basically just executes a valid for loop.

Note that it's not an exhaustive test suite, since there are some file operations that should be double-checked. Namely, wilcards in the parameter list (* or ?). These characters are contained within a parameter, that parameter will be replaced with all matching files in the current directory.

Note that there are a couple of extra commits. These just change some collections of raw owned pointers to unique pointers. Basically, it doesn't look like the fixture sets up the "pseudo-destructors" for those sections, so they leak in tests, so LeakSanitizer complains. They're trivial changes. Note that I also removed the iterators for those types, and replaced the couple of loops where they are used with range-based for loops.

I'd also like to mention that the shell tests initially weren't working for me. The cryptic error messages indicated that it was something to do with threading. If I had to guess, it's UB in the fixture, since using the debugger led to more questions then answers. I also know that ThreadSanitizer screams at me when I use it on dosbox, so it certainly wouldn't be off-brand. (To "fix", I statically linked gtest, so tests were able to pass and fail where expected).

[johnnovak]

MS-DOS 6.22 help file in online format:
https://hwiegman.home.xs4all.nl/msdos2/index.html

Relevant pages about the FOR command:

• https://hwiegman.home.xs4all.nl/msdos2/FOR.txt
• https://hwiegman.home.xs4all.nl/msdos2/FOR--Notes.txt
• https://hwiegman.home.xs4all.nl/msdos2/FOR--Examples.txt

[johnnovak]

Nice one @MeAreJeenius. I'm on semi-vacation until the end of the week, so I might only review it next week. We'll see. Of course, others are welcome to start the PR process until then.

[johnnovak]

Some feedback @MeAreJeenius about the help text before I look at the code. Had a very brief look at the code and looks very clean; I'll comment in detail later when I have more time.

Visual style

Can you please update the help text so it follows the style of our existing commands. Pay attention to colour, line spacing, and capitalisation. I have included a few example screenshots.

Note we fixed the ANSI markup stuff recently so you'll need to change the colour names (e.g., change green to light-green, etc.)

Wording

The original MS-DOS 6.22 help text is more user friendly; this description (while technically correct, no doubt) reads as if it was written by a programmer for a programmer 😄

• MS-DOS 6.22 command summary: Runs a specified command for each file in a set of files. You can use this command in batch programs or at the command prompt.
• This command summary: Runs a parametrized command on every element of a parameter list.

So we could make the new description a lot less abstract and describe better what's actually happening. I'm not saying to 100% copy the original MS-DOS version... but maybe do it 70-80% with a few wording tweaks 😄 Won't comment on the rest of the help text; there are other parts that could be made clearer and more friendly to regular users. Give it a shot then I can offer specific wording suggestions later perhaps.

---

COPY command

MIXER command

FOR command

[MeAreJeenius]

Help string is much improved. It's more in line with existing commands, and I simplified the text a bit.

I'm still not 100% percent sure on the colors.

I also opted to use the term "string" for the elements in the set. It's still kind of a programmer-ish term, but it was in the link you posted and I couldn't really think of anything better. The official help string uses "file" primarily, but I think that's too misleading.

In the command itself, I replaced instances of sizeof with strlen, which is what I was actually intending. The correct result was given by the sizeof calls, but that's just because sizeof(char) is 1, which happens to be the same as the length of a single-char string.

[johnnovak]

I'm done with reviewing the code @MeAreJeenius, just a few minor comments. I'll test the command itself in the coming days and do a last pass on the output. Great job so far and thanks for bearing with me & addressing the comments 😄

[kcgen]

Very easy to read, @MeAreJeenius. Just a couple very minor suggestions and some more test cases.

[Grounded0]

MS-DOS 6.22 & MS-DOS 7.0 (Windows 95) COMMAND.COM and FreeDOS FreeCOM 0.85a probably are the gold standards of desired behaviour:

FreeDOS Command Shell (FreeCOM)

FreeCOM 0.85a release

[kcgen]

Here's a self-contained pack containing MS-DOS 6.22 COMMAND.COM: for_test_cases.zip

Loop over filename wildcards:

Attempt and fail with nested FOR loops:

Run one FOR per COMMAND instance:

[MeAreJeenius]

FOR now prevents having another for being the command operated on. I've added a note in the help string, and a unit test.

I did not add a unit test for the command instance example for a few reasons:

• From the perspective of the for command, it's the same case as the successful echo case, which is already tested.
• It can't be tested "honestly". Right now, the shell commands are being verified by seeing if they call a mock function. The subshell calls a real shell, so it doesn't call that mock function. In general, this unit test fixture is extremely hacky (basically just starts dosbox, and the class under test is being mocked).
• Calling COMMAND.COM shouldn't be in a shell unit test regardless, since it's an program external to the shell, not part of the shell itself. The fact that COMMAND.COM is the shell program is irrelevant.

I didn't add a unit test for the file globs, because that requires filesystem access, so I think it should be manually tested instead. Trying to unit test stuff that's not contained within the program is a massive pain and is not worth it in my experience. Ideally the shell would have access to some sort of filesystem object that could be mocked or faked, but right now the code depends on "raw" calls to the dos fs.

Something to consider would be to start a manual testing markdown document in the repo. Having a checkbox in the PR template with that could replace the current manual testing section. This way the manual tests won't effectively die once the PR is closed, and having that as a resource anyone can access can better help spot regression in the (very numerous) places that aren't really unit-testable right now.

[johnnovak]

Something to consider would be to start a manual testing markdown document in the repo. Having a checkbox in the PR template with that could replace the current manual testing section. This way the manual tests won't effectively die once the PR is closed, and having that as a resource anyone can access can better help spot regression in the (very numerous) places that aren't really unit-testable right now.

I agree with that @MeAreJeenius, and I'm planning to do something similar for the MIXER command.

I've already done something similar for testing the various video modes as part of my GrandVgaDrawRefactorExercise(tm)
https://github.com/dosbox-staging/dosbox-staging/wiki/Video-emulation-tests

Feel free to create a new page under the "Dev" wiki section.

I didn't add a unit test for the file globs, because that requires filesystem access, so I think it should be manually tested instead. Trying to unit test stuff that's not contained within the program is a massive pain and is not worth it in my experience. Ideally the shell would have access to some sort of filesystem object that could be mocked or faked, but right now the code depends on "raw" calls to the dos fs.

I agree with that, once half the world is mocked out, the value the tests bring is very questionable. Plus the FOR command's implementation won't change very often, so I think documenting the test scenarios for manual regression testing is the effort/benefit/maintainability sweet spot.

[kcgen]

@MeAreJeenius , @johnnovak -> agree with all your points.

FOR now prevents having another for being the command operated on. I've added a note in the help string, and a unit test.

👍 ; right on - sounds like we're pretty much 1:1 w/ real MS-DOS 6.22!

From the perspective of the for command, it's the same case as the successful echo case, which is already tested.

I didn't add a unit test for the file globs, because that requires filesystem access

Yup - setting up the DOS filesystem access, specifically, requires a lot of the DOS environment itself to be mocked like you said (which is a lot more work than just doing local std::filesystem checks).

The fact you managed to test shell's internal statements using mock is already a big improvement.

[MeAreJeenius]

Added this to the wiki: https://github.com/dosbox-staging/dosbox-staging/wiki/Manual-DOS-CLI-Testing

Also forgot to mention this yesterday, but I had to revert the change in the for command where I used format_string() to replace the variable with the value. It sort of breaks down when there is another format specifier in the command.

As an example, for %t in (hi) do echo %s %t should echo %s hi, but echoes %hi %t.

Manually replacing the variable each time is a lot simpler than finding a good way of sanitizing the string.

[kcgen]

Very nice string splitting replacement calls, in addition to the FOR loop handling. All looks good from my side.

[johnnovak]

Thanks for your contribution @MeAreJeenius 🏅