From 31eff96cb732cac8cb9fbd29ad6435078d133754 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 8 May 2021 23:37:05 -0700 Subject: [PATCH] cli: record full argv in operation log When using the command line interface (which is the only interface so far), it seems more useful to see the exact command that was run than a logical description of what it does. This patch makes the CLI record that information in the operation metadata in a new key/value field. I put it in a generic key/value field instead of a more specialized field because the key/value field seems like a useful thing to have in general. However, that means that we "have to" do shell-escaping when saving the data instead of leaving the data unescaped and adding the shell-escaping when presenting it. I added very simple shell-escaping for now. --- lib/protos/op_store.proto | 1 + lib/src/op_store.rs | 4 +- lib/src/simple_op_store.rs | 3 + lib/src/transaction.rs | 10 +++- src/commands.rs | 109 +++++++++++++++++++++++++------------ 5 files changed, 91 insertions(+), 36 deletions(-) diff --git a/lib/protos/op_store.proto b/lib/protos/op_store.proto index 018aac20f..7090ae598 100644 --- a/lib/protos/op_store.proto +++ b/lib/protos/op_store.proto @@ -47,4 +47,5 @@ message OperationMetadata { string description = 3; string hostname = 4; string username = 5; + map tags = 6; } diff --git a/lib/src/op_store.rs b/lib/src/op_store.rs index 8d5761f9f..056e51438 100644 --- a/lib/src/op_store.rs +++ b/lib/src/op_store.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::{BTreeMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::fmt::{Debug, Error, Formatter}; use crate::store::{CommitId, Timestamp}; @@ -101,6 +101,7 @@ pub struct OperationMetadata { pub description: String, pub hostname: String, pub username: String, + pub tags: HashMap, } impl OperationMetadata { @@ -114,6 +115,7 @@ impl OperationMetadata { description, hostname, username, + tags: Default::default(), } } } diff --git a/lib/src/simple_op_store.rs b/lib/src/simple_op_store.rs index d0ffb17af..16418ca76 100644 --- a/lib/src/simple_op_store.rs +++ b/lib/src/simple_op_store.rs @@ -149,6 +149,7 @@ fn operation_metadata_to_proto( proto.set_description(metadata.description.clone()); proto.set_hostname(metadata.hostname.clone()); proto.set_username(metadata.username.clone()); + proto.set_tags(metadata.tags.clone()); proto } @@ -160,12 +161,14 @@ fn operation_metadata_from_proto( let description = proto.get_description().to_owned(); let hostname = proto.get_hostname().to_owned(); let username = proto.get_username().to_owned(); + let tags = proto.get_tags().clone(); OperationMetadata { start_time, end_time, description, hostname, username, + tags, } } diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index 212f08589..9cf4b50b6 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashMap; use std::sync::{Arc, Mutex}; use crate::evolution::ReadonlyEvolution; @@ -29,6 +30,7 @@ pub struct Transaction { parents: Vec, description: String, start_time: Timestamp, + tags: HashMap, closed: bool, } @@ -40,6 +42,7 @@ impl Transaction { parents, description: description.to_owned(), start_time: Timestamp::now(), + tags: Default::default(), closed: false, } } @@ -52,6 +55,10 @@ impl Transaction { self.parents = parents; } + pub fn set_tag(&mut self, key: String, value: String) { + self.tags.insert(key, value); + } + pub fn mut_repo(&mut self) -> &mut MutableRepo { Arc::get_mut(self.repo.as_mut().unwrap()).unwrap() } @@ -74,8 +81,9 @@ impl Transaction { let view = mut_view.freeze(); let view_id = base_repo.op_store().write_view(view.store_view()).unwrap(); - let operation_metadata = + let mut operation_metadata = OperationMetadata::new(self.description.clone(), self.start_time.clone()); + operation_metadata.tags = self.tags.clone(); let store_operation = op_store::Operation { view_id, parents: self.parents.clone(), diff --git a/src/commands.rs b/src/commands.rs index f0bef649f..71c308d0d 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -127,12 +127,16 @@ fn get_repo(ui: &Ui, matches: &ArgMatches) -> Result, CommandE } struct CommandHelper<'args> { + string_args: Vec, root_matches: ArgMatches<'args>, } impl<'args> CommandHelper<'args> { - fn new(root_matches: ArgMatches<'args>) -> Self { - Self { root_matches } + fn new(string_args: Vec, root_matches: ArgMatches<'args>) -> Self { + Self { + string_args, + root_matches, + } } fn root_matches(&self) -> &ArgMatches { @@ -140,13 +144,14 @@ impl<'args> CommandHelper<'args> { } fn repo_helper(&self, ui: &Ui) -> Result { - RepoCommandHelper::new(ui, &self.root_matches) + RepoCommandHelper::new(ui, self.string_args.clone(), &self.root_matches) } } // Provides utilities for writing a command that works on a repo (like most // commands do). struct RepoCommandHelper { + string_args: Vec, settings: UserSettings, repo: Arc, working_copy_committed: bool, @@ -156,9 +161,14 @@ struct RepoCommandHelper { } impl RepoCommandHelper { - fn new(ui: &Ui, root_matches: &ArgMatches) -> Result { + fn new( + ui: &Ui, + string_args: Vec, + root_matches: &ArgMatches, + ) -> Result { let repo = get_repo(ui, &root_matches)?; Ok(RepoCommandHelper { + string_args, settings: ui.settings().clone(), repo, working_copy_committed: false, @@ -233,6 +243,35 @@ impl RepoCommandHelper { commit } + fn start_transaction(&self, description: &str) -> Transaction { + let mut tx = self.repo.start_transaction(description); + // TODO: Either do better shell-escaping here or store the values in some list + // type (which we currently don't have). + let shell_escape = |arg: &String| { + if arg.as_bytes().iter().all(|b| { + matches!(b, + b'A'..=b'Z' + | b'a'..=b'z' + | b'0'..=b'9' + | b',' + | b'-' + | b'.' + | b'/' + | b':' + | b'@' + | b'_' + ) + }) { + arg.clone() + } else { + format!("'{}'", arg.replace("'", "\\'")) + } + }; + let quoted_strings: Vec<_> = self.string_args.iter().map(shell_escape).collect(); + tx.set_tag("args".to_string(), quoted_strings.join(" ")); + tx + } + fn finish_transaction( &mut self, ui: &mut Ui, @@ -722,8 +761,8 @@ fn cmd_checkout( let mut repo_command = command.repo_helper(ui)?.auto_update_checkout(false); let new_commit = repo_command.resolve_revision_arg(sub_matches)?; repo_command.commit_working_copy(); - let repo = repo_command.repo(); - let mut tx = repo.start_transaction(&format!("check out commit {}", new_commit.id().hex())); + let mut tx = + repo_command.start_transaction(&format!("check out commit {}", new_commit.id().hex())); tx.mut_repo().check_out(ui.settings(), &new_commit); let stats = repo_command.finish_transaction(ui, tx)?; match stats { @@ -1287,7 +1326,7 @@ fn cmd_describe( } else { description = edit_description(&repo, commit.description()); } - let mut tx = repo.start_transaction(&format!("describe commit {}", commit.id().hex())); + let mut tx = repo_command.start_transaction(&format!("describe commit {}", commit.id().hex())); CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &commit) .set_description(description) .write_to_repo(tx.mut_repo()); @@ -1303,7 +1342,7 @@ fn cmd_open( let mut repo_command = command.repo_helper(ui)?; let commit = repo_command.resolve_revision_arg(sub_matches)?; let repo = repo_command.repo(); - let mut tx = repo.start_transaction(&format!("open commit {}", commit.id().hex())); + let mut tx = repo_command.start_transaction(&format!("open commit {}", commit.id().hex())); CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &commit) .set_open(true) .write_to_repo(tx.mut_repo()); @@ -1330,7 +1369,7 @@ fn cmd_close( description = commit.description().to_string(); } commit_builder = commit_builder.set_description(description); - let mut tx = repo.start_transaction(&format!("close commit {}", commit.id().hex())); + let mut tx = repo_command.start_transaction(&format!("close commit {}", commit.id().hex())); commit_builder.write_to_repo(tx.mut_repo()); repo_command.finish_transaction(ui, tx)?; Ok(()) @@ -1344,7 +1383,8 @@ fn cmd_duplicate( let mut repo_command = command.repo_helper(ui)?; let predecessor = repo_command.resolve_revision_arg(sub_matches)?; let repo = repo_command.repo(); - let mut tx = repo.start_transaction(&format!("duplicate commit {}", predecessor.id().hex())); + let mut tx = + repo_command.start_transaction(&format!("duplicate commit {}", predecessor.id().hex())); let mut_repo = tx.mut_repo(); let new_commit = CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &predecessor) .generate_new_change_id() @@ -1369,7 +1409,8 @@ fn cmd_prune( "Cannot prune the root commit", ))); } - let mut tx = repo.start_transaction(&format!("prune commit {}", predecessor.id().hex())); + let mut tx = + repo_command.start_transaction(&format!("prune commit {}", predecessor.id().hex())); CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &predecessor) .set_pruned(true) .write_to_repo(tx.mut_repo()); @@ -1391,7 +1432,7 @@ fn cmd_new( parent.id().clone(), parent.tree().id().clone(), ); - let mut tx = repo.start_transaction("new empty commit"); + let mut tx = repo_command.start_transaction("new empty commit"); let mut_repo = tx.mut_repo(); let new_commit = commit_builder.write_to_repo(mut_repo); if mut_repo.view().checkout() == parent.id() { @@ -1421,7 +1462,7 @@ fn cmd_squash( "Cannot squash into the root commit", ))); } - let mut tx = repo.start_transaction(&format!("squash commit {}", commit.id().hex())); + let mut tx = repo_command.start_transaction(&format!("squash commit {}", commit.id().hex())); let mut_repo = tx.mut_repo(); let new_parent_tree_id; if sub_matches.is_present("interactive") { @@ -1468,7 +1509,7 @@ fn cmd_unsquash( "Cannot unsquash from the root commit", ))); } - let mut tx = repo.start_transaction(&format!("unsquash commit {}", commit.id().hex())); + let mut tx = repo_command.start_transaction(&format!("unsquash commit {}", commit.id().hex())); let mut_repo = tx.mut_repo(); let parent_base_tree = merge_commit_trees(repo.as_repo_ref(), &parent.parents()); let new_parent_tree_id; @@ -1503,8 +1544,7 @@ fn cmd_discard( ) -> Result<(), CommandError> { let mut repo_command = command.repo_helper(ui)?; let commit = repo_command.resolve_revision_arg(sub_matches)?; - let repo = repo_command.repo(); - let mut tx = repo.start_transaction(&format!("discard commit {}", commit.id().hex())); + let mut tx = repo_command.start_transaction(&format!("discard commit {}", commit.id().hex())); let mut_repo = tx.mut_repo(); mut_repo.remove_head(&commit); for parent in commit.parents() { @@ -1557,7 +1597,7 @@ fn cmd_restore( if &tree_id == destination_commit.tree().id() { ui.write("Nothing changed.\n")?; } else { - let mut tx = repo.start_transaction(&format!( + let mut tx = repo_command.start_transaction(&format!( "restore into commit {}", destination_commit.id().hex() )); @@ -1587,7 +1627,7 @@ fn cmd_edit( if &tree_id == commit.tree().id() { ui.write("Nothing changed.\n")?; } else { - let mut tx = repo.start_transaction(&format!("edit commit {}", commit.id().hex())); + let mut tx = repo_command.start_transaction(&format!("edit commit {}", commit.id().hex())); let mut_repo = tx.mut_repo(); let new_commit = CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &commit) .set_tree(tree_id) @@ -1613,7 +1653,7 @@ fn cmd_split( if &tree_id == commit.tree().id() { ui.write("Nothing changed.\n")?; } else { - let mut tx = repo.start_transaction(&format!("split commit {}", commit.id().hex())); + let mut tx = repo_command.start_transaction(&format!("split commit {}", commit.id().hex())); let mut_repo = tx.mut_repo(); // TODO: Add a header or footer to the decription where we describe to the user // that this is the first commit @@ -1666,7 +1706,7 @@ fn cmd_merge( description = edit_description(&repo, ""); } let merged_tree = merge_commit_trees(repo.as_repo_ref(), &commits); - let mut tx = repo.start_transaction("merge commits"); + let mut tx = repo_command.start_transaction("merge commits"); CommitBuilder::for_new_commit(ui.settings(), repo.store(), merged_tree.id().clone()) .set_parents(parent_ids) .set_description(description) @@ -1689,8 +1729,8 @@ fn cmd_rebase( let destination = repo_command.resolve_single_rev(revision_str)?; parents.push(destination); } - let repo = repo_command.repo(); - let mut tx = repo.start_transaction(&format!("rebase commit {}", commit_to_rebase.id().hex())); + let mut tx = + repo_command.start_transaction(&format!("rebase commit {}", commit_to_rebase.id().hex())); rebase_commit(ui.settings(), tx.mut_repo(), &commit_to_rebase, &parents); repo_command.finish_transaction(ui, tx)?; @@ -1709,8 +1749,7 @@ fn cmd_backout( let destination = repo_command.resolve_single_rev(revision_str)?; parents.push(destination); } - let repo = repo_command.repo(); - let mut tx = repo.start_transaction(&format!( + let mut tx = repo_command.start_transaction(&format!( "back out commit {}", commit_to_back_out.id().hex() )); @@ -1811,8 +1850,7 @@ fn cmd_evolve<'s>( // have only one Ui instance we write to across threads? let user_settings = ui.settings().clone(); let mut listener = Listener { ui }; - let repo = repo_command.repo(); - let mut tx = repo.start_transaction("evolve"); + let mut tx = repo_command.start_transaction("evolve"); evolve(&user_settings, tx.mut_repo(), &mut listener); repo_command.finish_transaction(ui, tx)?; @@ -2006,7 +2044,9 @@ fn cmd_op_log( styler.add_label("description".to_string())?; styler.write_str(&metadata.description)?; styler.remove_label()?; - + for (key, value) in &metadata.tags { + styler.write_str(&format!("\n{}: {}", key, value))?; + } styler.remove_label()?; Ok(()) @@ -2061,7 +2101,7 @@ fn cmd_op_undo( )); } - let mut tx = repo.start_transaction(&format!("undo operation {}", bad_op.id().hex())); + let mut tx = repo_command.start_transaction(&format!("undo operation {}", bad_op.id().hex())); let bad_repo = repo.loader().load_at(&bad_op)?; let parent_repo = repo.loader().load_at(&parent_ops[0])?; tx.mut_repo().merge(&bad_repo, &parent_repo); @@ -2078,7 +2118,8 @@ fn cmd_op_restore( let mut repo_command = command.repo_helper(ui)?; let repo = repo_command.repo(); let target_op = resolve_single_op(&repo, _cmd_matches.value_of("operation").unwrap())?; - let mut tx = repo.start_transaction(&format!("restore to operation {}", target_op.id().hex())); + let mut tx = + repo_command.start_transaction(&format!("restore to operation {}", target_op.id().hex())); tx.mut_repo().set_view(target_op.view().take_store_view()); repo_command.finish_transaction(ui, tx)?; @@ -2121,7 +2162,7 @@ fn cmd_git_fetch( let repo = repo_command.repo(); let git_repo = get_git_repo(repo.store())?; let remote_name = cmd_matches.value_of("remote").unwrap(); - let mut tx = repo.start_transaction(&format!("fetch from git remote {}", remote_name)); + let mut tx = repo_command.start_transaction(&format!("fetch from git remote {}", remote_name)); git::fetch(tx.mut_repo(), &git_repo, remote_name) .map_err(|err| CommandError::UserError(err.to_string()))?; tx.commit(); @@ -2185,7 +2226,7 @@ fn cmd_git_push( let branch_name = cmd_matches.value_of("branch").unwrap(); git::push_commit(&git_repo, &commit, remote_name, branch_name) .map_err(|err| CommandError::UserError(err.to_string()))?; - let mut tx = repo.start_transaction("import git refs"); + let mut tx = repo_command.start_transaction("import git refs"); git::import_refs(tx.mut_repo(), &git_repo) .map_err(|err| CommandError::UserError(err.to_string()))?; tx.commit(); @@ -2201,7 +2242,7 @@ fn cmd_git_refresh( let repo_command = command.repo_helper(ui)?; let repo = repo_command.repo(); let git_repo = get_git_repo(repo.store())?; - let mut tx = repo.start_transaction("import git refs"); + let mut tx = repo_command.start_transaction("import git refs"); git::import_refs(tx.mut_repo(), &git_repo) .map_err(|err| CommandError::UserError(err.to_string()))?; tx.commit(); @@ -2272,8 +2313,8 @@ where } } let string_args = resolve_alias(&mut ui, string_args); - let matches = get_app().get_matches_from(string_args); - let command_helper = CommandHelper::new(matches.clone()); + let matches = get_app().get_matches_from(&string_args); + let command_helper = CommandHelper::new(string_args, matches.clone()); let result = if let Some(sub_matches) = command_helper.root_matches.subcommand_matches("init") { cmd_init(&mut ui, &command_helper, &sub_matches) } else if let Some(sub_matches) = matches.subcommand_matches("checkout") {