Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli, config: more granular pagination settings #2927

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

cli, config: more granular pagination settings #2927

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
426ecee
cli, config: more granular pagination settings
jrobsonchase Feb 3, 2024
2ff1999
switch to iter::successors
jrobsonchase Feb 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ use crate::git_util::{print_failed_git_export, print_git_import_stats};
use crate::merge_tools::{ConflictResolveError, DiffEditError, DiffGenerateError};
use crate::template_parser::{TemplateAliasesMap, TemplateParseError};
use crate::templater::Template;
use crate::ui::{ColorChoice, Ui};
use crate::ui::{ColorChoice, PaginationChoice, Ui};
use crate::{commit_templater, text_util};

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -633,6 +633,16 @@ impl CommandHelper {
&self.matches
}

pub fn subcommands(&self) -> impl Iterator<Item = &'_ str> {
let mut next = self.matches().subcommand();

std::iter::from_fn(move || {
let (sub, args) = next.take()?;
next = args.subcommand();
Some(sub)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps, iter::successors(..).map(..) can be used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, neat - I think I knew about this at one time, but forgot about it.

lol, and the source even has a comment alluding to this exact implementation of it 😂

pub fn successors<T, F>(first: Option<T>, succ: F) -> Successors<T, F>
where
    F: FnMut(&T) -> Option<T>,
{
    // If this function returned `impl Iterator<Item=T>`
    // it could be based on `from_fn` and not need a dedicated type.
    // However having a named `Successors<T, F>` type allows it to be `Clone` when `T` and `F` are.
    Successors { next: first, succ }
}

I'll switch to that and likely forget about it and get to rediscover it all over again in the future 👍


pub fn global_args(&self) -> &GlobalArgs {
&self.global_args
}
Expand Down Expand Up @@ -2594,6 +2604,8 @@ pub struct EarlyArgs {
// Parsing with ignore_errors will crash if this is bool, so use
// Option<bool>.
pub no_pager: Option<bool>,
#[arg(long, value_name = "WHEN", global = true)]
pub paginate: Option<PaginationChoice>,
/// Additional configuration options (can be repeated)
// TODO: Introduce a `--config` option with simpler syntax for simple
// cases, designed so that `--config ui.color=auto` works
Expand Down Expand Up @@ -2778,9 +2790,17 @@ fn handle_early_args(
if let Some(choice) = args.color {
args.config_toml.push(format!(r#"ui.color="{choice}""#));
}
if args.no_pager.unwrap_or_default() {
args.config_toml.push(r#"ui.paginate="never""#.to_owned());
if let Some(true) = args.no_pager {
args.paginate = Some(PaginationChoice::Never)
}

// If pagination was explicitly configured on the CLI, blow away any other
// pagination config and use the provided value.
if let Some(paginate) = args.paginate {
args.config_toml
.push(format!(r#"ui.paginate="{}""#, paginate));
}

if !args.config_toml.is_empty() {
layered_configs.parse_config_args(&args.config_toml)?;
ui.reset(&layered_configs.merge())?;
Expand Down Expand Up @@ -2868,7 +2888,7 @@ pub fn handle_command_result(
match inner.kind() {
clap::error::ErrorKind::DisplayHelp
| clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand => {
ui.request_pager()
ui.request_pager(["help"])
}
_ => {}
};
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/branch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ fn cmd_branch_list(
Ok(())
};

ui.request_pager();
ui.request_pager(command.subcommands());
let mut formatter = ui.stdout_formatter();
let formatter = formatter.as_mut();

Expand Down
4 changes: 2 additions & 2 deletions cli/src/commands/cat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ pub(crate) fn cmd_cat(
return Err(user_error("No such path"));
}
MaterializedTreeValue::File { mut reader, .. } => {
ui.request_pager();
ui.request_pager(command.subcommands());
std::io::copy(&mut reader, &mut ui.stdout_formatter().as_mut())?;
}
MaterializedTreeValue::Conflict { contents, .. } => {
ui.request_pager();
ui.request_pager(command.subcommands());
ui.stdout_formatter().write_all(&contents)?;
}
MaterializedTreeValue::Symlink { .. }
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ pub(crate) fn cmd_config_list(
command: &CommandHelper,
args: &ConfigListArgs,
) -> Result<(), CommandError> {
ui.request_pager();
ui.request_pager(command.subcommands());
let name_path = args
.name
.as_ref()
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/diff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ pub(crate) fn cmd_diff(
}
let matcher = workspace_command.matcher_from_values(&args.paths)?;
let diff_formats = diff_formats_for(command.settings(), &args.format)?;
ui.request_pager();
ui.request_pager(command.subcommands());
show_diff(
ui,
ui.stdout_formatter().as_mut(),
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/files.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub(crate) fn cmd_files(
let commit = workspace_command.resolve_single_rev(&args.revision, ui)?;
let tree = commit.tree()?;
let matcher = workspace_command.matcher_from_values(&args.paths)?;
ui.request_pager();
ui.request_pager(command.subcommands());
for (name, _value) in tree.entries_matching(matcher.as_ref()) {
writeln!(
ui.stdout(),
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/interdiff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ pub(crate) fn cmd_interdiff(
let to_tree = to.tree()?;
let matcher = workspace_command.matcher_from_values(&args.paths)?;
let diff_formats = diff_util::diff_formats_for(command.settings(), &args.format)?;
ui.request_pager();
ui.request_pager(command.subcommands());
diff_util::show_diff(
ui,
ui.stdout_formatter().as_mut(),
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/log.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ pub(crate) fn cmd_log(
let with_content_format = LogContentFormat::new(ui, command.settings())?;

{
ui.request_pager();
ui.request_pager(command.subcommands());
let mut formatter = ui.stdout_formatter();
let formatter = formatter.as_mut();

Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/obslog.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ pub(crate) fn cmd_obslog(
let template = workspace_command.parse_commit_template(&template_string)?;
let with_content_format = LogContentFormat::new(ui, command.settings())?;

ui.request_pager();
ui.request_pager(command.subcommands());
let mut formatter = ui.stdout_formatter();
let formatter = formatter.as_mut();
formatter.push_label("log")?;
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/operation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ fn cmd_op_log(
)?;
let with_content_format = LogContentFormat::new(ui, command.settings())?;

ui.request_pager();
ui.request_pager(command.subcommands());
let mut formatter = ui.stdout_formatter();
let formatter = formatter.as_mut();
let iter = op_walk::walk_ancestors(&head_ops).take(args.limit.unwrap_or(usize::MAX));
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/show.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub(crate) fn cmd_show(
let template_string = command.settings().config().get_string("templates.show")?;
let template = workspace_command.parse_commit_template(&template_string)?;
let diff_formats = diff_util::diff_formats_for(command.settings(), &args.format)?;
ui.request_pager();
ui.request_pager(command.subcommands());
let mut formatter = ui.stdout_formatter();
let formatter = formatter.as_mut();
template.format(&commit, formatter)?;
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/status.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub(crate) fn cmd_status(
.get_wc_commit_id()
.map(|id| repo.store().get_commit(id))
.transpose()?;
ui.request_pager();
ui.request_pager(command.subcommands());
let mut formatter = ui.stdout_formatter();
let formatter = formatter.as_mut();

Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/tag.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fn cmd_tag_list(
let repo = workspace_command.repo();
let view = repo.view();

ui.request_pager();
ui.request_pager(command.subcommands());
let mut formatter = ui.stdout_formatter();
let formatter = formatter.as_mut();

Expand Down
110 changes: 104 additions & 6 deletions cli/src/ui.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::collections::HashMap;
use std::io::{IsTerminal as _, Stderr, StderrLock, Stdout, StdoutLock, Write};
use std::ops::ControlFlow;
use std::process::{Child, ChildStdin, Stdio};
use std::str::FromStr;
use std::{env, fmt, io, mem};
Expand Down Expand Up @@ -101,7 +103,7 @@ impl Write for UiStderr<'_> {
pub struct Ui {
color: bool,
pager_cmd: CommandNameAndArgs,
paginate: PaginationChoice,
paginate: Pagination,
progress_indicator: bool,
formatter_factory: FormatterFactory,
output: UiOutput,
Expand Down Expand Up @@ -165,11 +167,88 @@ pub enum PaginationChoice {
Never,
#[default]
Auto,
Always,
}

impl FromStr for PaginationChoice {
type Err = &'static str;

fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"always" => Ok(PaginationChoice::Always),
"never" => Ok(PaginationChoice::Never),
"auto" => Ok(PaginationChoice::Auto),
_ => Err("must be one of always, never, or auto"),
}
}
}

impl fmt::Display for PaginationChoice {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let s = match self {
PaginationChoice::Always => "always",
PaginationChoice::Never => "never",
PaginationChoice::Auto => "auto",
};
write!(f, "{s}")
}
}

#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub struct Pagination {
default: PaginationChoice,

commands: HashMap<String, Pagination>,
}

fn pagination_setting(config: &config::Config) -> Result<PaginationChoice, CommandError> {
impl<'de> serde::de::Deserialize<'de> for Pagination {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
struct PaginationVisitor;

impl<'de> serde::de::Visitor<'de> for PaginationVisitor {
type Value = Pagination;
fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(
formatter,
r#"always, never, auto, or a mapping of command names to pagination"#
)
}

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
where
E: serde::de::Error,
{
Ok(Pagination {
default: PaginationChoice::from_str(v).map_err(E::custom)?,
..Default::default()
})
}

fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
where
A: serde::de::MapAccess<'de>,
{
let mut cfg = Pagination::default();

while let Some(k) = map.next_key::<String>()? {
if k == "default" {
cfg.default = map.next_value::<PaginationChoice>()?;
} else {
cfg.commands.insert(k, map.next_value::<Pagination>()?);
}
}
Ok(cfg)
}
}
deserializer.deserialize_any(PaginationVisitor)
}
}
fn pagination_setting(config: &config::Config) -> Result<Pagination, CommandError> {
config
.get::<PaginationChoice>("ui.paginate")
.get::<Pagination>("ui.paginate")
.map_err(|err| CommandError::ConfigError(format!("Invalid `ui.paginate`: {err}")))
}

Expand Down Expand Up @@ -209,10 +288,29 @@ impl Ui {

/// Switches the output to use the pager, if allowed.
#[instrument(skip_all)]
pub fn request_pager(&mut self) {
match self.paginate {
pub fn request_pager<'a>(&mut self, subcommands: impl IntoIterator<Item = &'a str>) {
let paginate = subcommands.into_iter().try_fold(
(self.paginate.default, &self.paginate),
|(out, current), v| match current.commands.get(v) {
None => ControlFlow::Break((out, current)),
Some(
p @ Pagination {
default: PaginationChoice::Auto,
..
},
) => ControlFlow::Continue((out, p)),
Some(p) => ControlFlow::Continue((p.default, p)),
},
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is interesting, I think it's better to use a flat map of (full_command_name, paginate). Subcommands tree is just a namespace. Child command doesn't inherit parent property in general.

I'm not sure whether the config table should be pager-specific or more general [commands.<name>], but something like this might be useful:

[commands."branch list"]
paginate = ..
pager = ..


let paginate = match paginate {
ControlFlow::Break((out, _)) => out,
ControlFlow::Continue((out, _)) => out,
};

match paginate {
PaginationChoice::Never => return,
PaginationChoice::Auto => {}
PaginationChoice::Always | PaginationChoice::Auto => {}
}

match self.output {
Expand Down
Loading