disable --rm on -o command #3450

Cyan4973 · 2023-01-24T03:07:10Z

Up to now, it's possible to combine --rm with -o
concatenating all processed frames into a single output file or device,
while simultaneously deleting input.
Since such a concatenated result will not be able to regenerate input file names nor directory structure,
it is a destructive operation, which may result in permanent damage.

This situation has been partially dampened by adding a warning message and a confirmation prompt,
though both can be circumvented by adding the -f flag.

Update the policy, to disable --rm when -o is used in combination with multiple inputs.
This makes it more similar to -c (aka stdout) convention, itself derived from gzip.

The -o behavior is altered as follows :

With only 1 input : no change, can be combined with --rm
With multiple inputs : automatically disable --rm, display a warning message (level 2) if it was set
With multiple inputs : display a warning message about the impossibility to reverse operation, and prompt for confirmation.
With multiple inputs and -f : do not prompt, just warn and proceed
With multiple inputs and -q but not -f : do not prompt, explain and fail immediately

Adjusted and added test cases accordingly

terrelln · 2023-01-24T18:25:42Z

programs/zstdcli.c

    hasStdout = outFileName && !strcmp(outFileName,stdoutmark);
-    if ((hasStdout || !UTIL_isConsole(stderr)) && (g_displayLevel==2)) g_displayLevel=1;
+    if (hasStdout && (g_displayLevel==2)) g_displayLevel=1;


Yeah this is the line that is causing issues in the test. It is producing logs in the cli-tests when we didn't expect them because stderr is not a console.

When !UTIL_isConsole(stderr) we can instead set the --no-progress flag. Though we should make sure that using --progress will still re-enable progress messages.

Can you separate this change into a separate PR? Since this could change the CLI tests output. And otherwise the CLI tests should be unaffected by this PR.

I have an issue with some tests though.

When displayLevel==2 (default), the CLI expects interactive mode, and will sometimes prompt for confirmation.
There are a few tests which expect that to happen, and pipe a y into stdin to continue the operation successfully.

It happens here: when a user wants to concatenate multiple frames into a single output,
the CLI warns that it's probably not a good idea, and then asks the user for confirmation of this operation.

However, with displayLevel==1, this is interpreted as --quiet, in which case no prompt ever happens, and instead it's an instant failure. Since the test was expecting "success" (by providing the y to the prompt), the test now fails.

With the !UTIL_isConsole(stderr) automatic change of verbosity, CLI tests behave one way locally, and differently on Github Actions. That's a problem, tests require a unified behavior.

There are probably several ways to get around this problem.

For example, one could simply avoid testing interactive prompt scenarios, by forcing such scenarios to be run exclusively with -q or -v.
Of course, such a strategy would make the test suite blind to interactive scenarios.

So I was more in favor of trying to "fix" this behavior, since the intention was just to remove the status updates from stderr when stderr is a log. But if it triggers too many complications, then maybe the answer will be to test less scenarios...

I see what you're trying to test, it makes sense to combine it into this PR then.

terrelln · 2023-01-24T19:09:54Z

programs/zstdcli.c

    hasStdout = outFileName && !strcmp(outFileName,stdoutmark);
-    if ((hasStdout || !UTIL_isConsole(stderr)) && (g_displayLevel==2)) g_displayLevel=1;
+    if (hasStdout && (g_displayLevel==2)) g_displayLevel=1;


I see what you're trying to test, it makes sense to combine it into this PR then.

terrelln · 2023-01-24T19:11:10Z

programs/zstdcli.c

+    if (hasStdout && (g_displayLevel==2)) g_displayLevel=1;
+
+    /* when stderr is not the console, do not pollute it with status updates */
+    if (!UTIL_isConsole(stderr)) FIO_setProgressSetting(FIO_ps_never);


This doesn't quite work, because this breaks the --progress flag. When stderr is not a console, --progress won't work. Which is probably not the intended behavior.

I think you basically want:

if (!UTIL_isConsole(stderr) && FIO_getProgressSetting() == FIO_ps_auto) FIO_setProgressSetting(FIO_ps_never);

Though you'll probably have to add the function FIO_getProgressSetting().

Well, it was the intended behavior, or rather the one I intended,
but indeed, existing tests are expecting something different.
This behavior is not explicitly defined, we end up rebuilding its definition from test results.

My understanding is that the current tests expect that --progress command can force writing status updates into stderr even when it's not the console. Not sure when that is useful, but since I don't have any strong opinion about that behavior, let's keep it.

That will require an additional variable within zstdcli.c, to register commands instead of pushing actions immediately, but that seems doable.

edit: seen your 2nd comment after redacting the answer. Indeed, I made a similar change.
Longer term, it opens up the topic: "where is the right place to adjust all these settings?",
as sometimes, it's unclear if this should be done in zstdcli.c or fileio.c.
But that's a rabbit hole. For the time being, let's focus on the pb at hand.

terrelln · 2023-01-25T02:30:47Z

The remaining CLI test failures are benign, and can be updated with:

./tests/cli-tests/run.py --set-exact-output

Basically, they're just saying that we are calling UTIL_isConsole(stderr) one extra time when the output is stdout.

terrelln · 2023-01-25T21:35:11Z

programs/fileio.c

+    if (fCtx->hasStdoutOutput) assert(prefs->removeSrcFile == 0);
+    if (prefs->testMode) {
+        assert(prefs->removeSrcFile == 0);


nit: Because this is a dangerous operation, and not in the hot loop, I wonder if we want to use EXM_THROW() if prefs->removeSrcFile, and add a message saying that it is a programming error, and to open an issue. So that it is enabled in prod builds.

terrelln · 2023-01-25T21:41:24Z

programs/fileio.c

+        prefs->removeSrcFile = 0;
+    }
+
+    if (fCtx->hasStdoutOutput) return 0;


Why are we not prompting if we have stdout as an output? I think the same behavior as -o for -c would be useful.

Indeed, it would seem logical to have -o and -c sharing the same behavior.
But for -c, we are following the established behavior of gzip, and gzip features no prompt in this case.
-o on the other hand only exists for zstd, so we can select a more protective behavior.

make it more similar to -c (aka `stdout`) convention.

@terrelln

in scenarios where it's supposed to not be possible. suggested by @terrelln

facebook-github-bot added the CLA Signed label Jan 24, 2023

Cyan4973 force-pushed the no_rm_on_o branch 6 times, most recently from 0ca15c6 to 7e895e0 Compare January 24, 2023 05:03

Cyan4973 self-assigned this Jan 24, 2023

Cyan4973 force-pushed the no_rm_on_o branch 3 times, most recently from 7099aa0 to 48a195c Compare January 24, 2023 05:57

terrelln reviewed Jan 24, 2023

View reviewed changes

Cyan4973 force-pushed the no_rm_on_o branch from 48a195c to 9df4f44 Compare January 24, 2023 19:07

terrelln reviewed Jan 24, 2023

View reviewed changes

Cyan4973 force-pushed the no_rm_on_o branch 2 times, most recently from a8c3d79 to 75a7497 Compare January 25, 2023 00:39

Cyan4973 force-pushed the no_rm_on_o branch from 75a7497 to d12aa44 Compare January 25, 2023 17:47

terrelln reviewed Jan 25, 2023

View reviewed changes

terrelln approved these changes Jan 26, 2023

View reviewed changes

Cyan4973 added 2 commits January 25, 2023 16:09

disable --rm on -o command

8c85b29

make it more similar to -c (aka `stdout`) convention.

enforce a hard fail when input files are set to be erased

02434e0

in scenarios where it's supposed to not be possible. suggested by @terrelln

Cyan4973 force-pushed the no_rm_on_o branch from d12aa44 to 02434e0 Compare January 26, 2023 00:19

Cyan4973 merged commit a82e0aa into dev Jan 26, 2023

Cyan4973 deleted the no_rm_on_o branch January 26, 2023 22:44

Cyan4973 mentioned this pull request Feb 9, 2023

release v1.5.4 #3487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disable --rm on -o command #3450

disable --rm on -o command #3450

Cyan4973 commented Jan 24, 2023 •

edited

Loading

terrelln Jan 24, 2023

Cyan4973 Jan 24, 2023 •

edited

Loading

terrelln Jan 24, 2023

terrelln Jan 24, 2023

terrelln Jan 24, 2023

terrelln Jan 24, 2023

Cyan4973 Jan 24, 2023 •

edited

Loading

terrelln commented Jan 25, 2023

terrelln Jan 25, 2023

terrelln Jan 25, 2023

Cyan4973 Jan 25, 2023

disable --rm on -o command #3450

disable --rm on -o command #3450

Conversation

Cyan4973 commented Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Cyan4973 Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cyan4973 Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

terrelln commented Jan 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cyan4973 commented Jan 24, 2023 •

edited

Loading

Cyan4973 Jan 24, 2023 •

edited

Loading

Cyan4973 Jan 24, 2023 •

edited

Loading