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
