Re: Pristines-on-demand: printing progress notifications

[Daniel Shahaf <d....@daniel.shahaf.name>]

Karl Fogel wrote on Wed, Mar 30, 2022 at 17:58:55 -0500:
> On 30 Mar 2022, Julian Foad wrote:
> > Karl Fogel wrote:
> > > I think printing these messages to stderr makes the most sense.
> > > There are plenty of programs out there that parse the stdout of
> > > 'svn'; we don't want to interfere with them.
> > > 
> > > As you point out, it's especially important for 'svn diff' and 'svn
> > > cat' that stdout remain untainted.  Therefore, we can either always
> > > print these messages to stderr (across all commands), or not print
> > > them for 'svn diff' and 'svn cat' (but that would be an odd
> > > inconsistency).
> > > 
> > > > Anybody want to recommend what we should do for 'cat' and
> > > > 'diff'?
> > > 
> > > As per above: I think we should print the messages to stderr for
> > > everything, including those two.
> > 
> > Printing progress notifications for data-output commands (diff, cat) to
> > stderr does however invite bikeshedding. Currently in our test suite we
> > assume any stderr output (from diff, cat) should be flagged as a test
> > failure. We can change that, but it indicates that some users may
> > consider it a failure too. We would need to agree and decide whether
> > that's going to be unconditional or if the user needs to be able to turn
> > it off for convenience and for backward compatibility.
> > 
> > Because this could be dragged out I'm filing it as a lower priority for
> > now. We can get back to it. (If someone feels able to resolve it,
> > great.)
> 
> Good points, and +1 to prioritizing it lower down relative to shipping the
> main thing!

I'm a bit hesitant about disabling notifications _entirely_ in cat-cmd.c
and diff-cmd.c.  Disabling all notifications (as opposed to only
hydration-related notifications which we focus on right now) seems like
it could easily have unintended consequences.  Do we do that elsewhere?
E.g., in --xml mode?

«diff» isn't unique in having parseable output.  On the contrary, all
our subcommands have parseable output, whether it's "unidiffs" or "seven
columns, then a column of spaces, then filenames" or "XML in <this>
schema" or "RFC822-esque data with localized field names".  Sure, «svn
diff»'s output may be consumed by other tools, but that also goes for
other subcommands (e.g., help/info/ls/proplist/status all have
machine-parseable output even in non-«--xml» mode).  So, I am leaning to
the opinion that «svn diff» isn't a special case, and «diff» and those
other commands should all use the same rules for their output.

What are those rules?  Good question.  stdout isn't a good place for the
hydration notifications, since those are of interest to the (human)
producer of the output but not to the consumer of the output; stderr
isn't a good place, since CLI consumers may interpret that as an error
indication (though the exit code would be zero, EXIT_SUCCESS); and
/dev/null isn't a good place either, since we added those notifications
for a reason.

«svn cat», on the other hand, _is_ unique, in that it's the only command
whose output format is not controlled by us.  That means we can't emit
anything to stdout in «svn cat» without possibly mangling user data.
So, for «svn cat» we don't have the option of emitting notifications to
stdout; we have only the other options discussed above.

Cheers,

Daniel

Re: Pristines-on-demand: printing progress notifications

[Julian Foad <ju...@apache.org>]

Daniel Shahaf wrote:
> I'm a bit hesitant about disabling notifications _entirely_ in cat-cmd.c
> and diff-cmd.c.  Disabling all notifications (as opposed to only
> hydration-related notifications which we focus on right now) seems like
> it could easily have unintended consequences.  Do we do that elsewhere?

Thanks for the review and discussion.

In "svn" we set the callback to null in a few other places, not many:

  * if (opt_state.quiet)  # all subcommands
  * if (opt_state.xml)  # all subcommands
  * svn propedit  # /* We do our own notifications */
  * svn changelist --quiet  # redundant with "opt_state.quiet"?
  * svn x-shelve/x-unshelve --quiet  # redundant again?

Maybe doing that for "diff" is suppressing too much, as you suggest.

I couldn't previously think of other notifications we might use in
"diff" but now I can imagine things such as "now processing an external
at path X" (now (I haven't checked) or in the future).

It would be best to preserve the possibility of any such notifications
during "diff".

The same applies to "cat", logically, the only difference being a
difference of expectation (we don't expect any other progress
notifications to be printed for "cat" anyway, while for "diff" we might).

Done: in "diff" and "cat", suppress just these particular notifications:

  https://svn.apache.org/r1900110


The rest of my reply (below) is just discussion.

> «diff» isn't unique in having parseable output. [...] I am leaning to
> the opinion that «svn diff» isn't a special case, and «diff» and those
> other commands should all use the same rules for their output.
> [...]
> What are those rules?  Good question.  stdout isn't a good place for the
> hydration notifications, since those are of interest to the (human)
> producer of the output but not to the consumer of the output; [...]
>  
> «svn cat», on the other hand, _is_ unique, in that it's the only command
> whose output format is not controlled by us. [...]

Let me first summarise the logic that led me to the current implementation.
Diff and cat are (presently) the only two data-outputting commands that
may hydrate; the rest are action commands. The output of action commands
is generally feedback to the operator of the command, and adding more
forms of notification to their output is a natural fit.

For the data-outputting commands, I preferred to avoid changing the
output as it would potentially have more consequences on its consumers
than we are willing to deal with. Obviously "cat" is particularly
sensitive, as you point out, and we agree it should print no feedback.

That leaves "diff", which as you point out is categorically similar to
other data outputting commands. A particularity of diff format is it
explicitly allows additional output which parsers should ignore. In that
respect it would be acceptable to add new notifications in its stdout,
even more so than for the other commands whose output we control. But
the issue remains that progress feedback logically belongs to the
operator of the command and not to the consumer of its output (although
the two are often the same). It is reasonably for a consumer of the
output to expect that the output will be repeatable for the same logical
WC state, and will not change depending on the caching state of
Subversion's metadata area. So adding feedback to data-outputting
commands, mixed in with their output, is not a great idea.

Ideally progress feedback would go neither to stdout nor to stderr but
to a third stream. Technically we could implement this easily as an
OS-level stream, but while it's the obvious approach in a GUI, I don't
think the idea of a separate feedback stream is widely used in
command-line environments so it would not be readily understandable and
accessible to users.

Similarly, we could add options to control whether and where such
feedback goes. But at this stage I don't think that added complexity is
warranted, at least not as an add-on handling only this specific case.
Maybe if we were to re-think the command line interface in its entirety,
say as a "2.0" project, that's where it could make sense.

For now it doesn't seem to me that any further changes are needed at
this time, other than the change agreed above to the conditional logic
for the feedback suppression.

- Julian