From c7d5ffe51b82ebeece2486c03e61400d3ecf774d Mon Sep 17 00:00:00 2001
From: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Date: Thu, 1 Dec 2022 01:11:01 +0100
Subject: [PATCH] cli: fix ambiguity between mutually exclusive args

There are some cases where it is just safe to skip the `--x` flag
because it is already set to true by default, and other cases where we
need to use an option value.

Also, I may miss some team code conventions regarding the PartialEq and
Eq macros to enable the quality on the `Comment` type!

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
---
 radicle-cli/src/commands/patch.rs  | 13 +++++++++----
 radicle-cli/src/commands/push.rs   | 12 ++++++++----
 radicle-cli/src/commands/review.rs |  8 +++++---
 radicle-cli/src/terminal/patch.rs  |  2 +-
 4 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/radicle-cli/src/commands/patch.rs b/radicle-cli/src/commands/patch.rs
index 0b825887..b9282b1c 100644
--- a/radicle-cli/src/commands/patch.rs
+++ b/radicle-cli/src/commands/patch.rs
@@ -98,20 +98,25 @@ impl Args for Options {
 
                 // Options.
                 Long("message") | Short('m') => {
-                    let txt: String = parser.value()?.to_string_lossy().into();
-                    message.append(&txt);
+                    if message != Comment::Blank {
+                        // We skip this code when `no-message` is specified.
+                        let txt: String = parser.value()?.to_string_lossy().into();
+                        message.append(&txt);
+                    }
                 }
                 Long("no-message") => {
                     message = Comment::Blank;
                 }
                 Long("sync") => {
-                    sync = true;
+                    // By default it is already true, so
+                    // the only case where this is false,
+                    // is the case where `no-sync` is specified.
                 }
                 Long("no-sync") => {
                     sync = false;
                 }
                 Long("push") => {
-                    push = true;
+                    // Skip for the same reason as `sync`.
                 }
                 Long("no-push") => {
                     push = false;
diff --git a/radicle-cli/src/commands/push.rs b/radicle-cli/src/commands/push.rs
index 0400079b..0239f896 100644
--- a/radicle-cli/src/commands/push.rs
+++ b/radicle-cli/src/commands/push.rs
@@ -51,7 +51,7 @@ impl Args for Options {
         let mut verbose = false;
         let mut force = false;
         let mut all = false;
-        let mut sync = false;
+        let mut sync = None;
         let mut set_upstream = false;
 
         while let Some(arg) = parser.next()? {
@@ -69,10 +69,14 @@ impl Args for Options {
                     set_upstream = true;
                 }
                 Long("sync") => {
-                    sync = true;
+                    // Falls back to `--no-sync` in case of ambiguity.
+                    // eg. `rad push --no-sync --sync`
+                    if sync.is_none() {
+                        sync = Some(true);
+                    }
                 }
                 Long("no-sync") => {
-                    sync = false;
+                    sync = Some(false);
                 }
                 Long("force") | Short('f') => {
                     force = true;
@@ -88,7 +92,7 @@ impl Args for Options {
                 force,
                 all,
                 set_upstream,
-                sync,
+                sync: sync.unwrap_or_default(),
                 verbose,
             },
             vec![],
diff --git a/radicle-cli/src/commands/review.rs b/radicle-cli/src/commands/review.rs
index bdfc32a4..5ab59202 100644
--- a/radicle-cli/src/commands/review.rs
+++ b/radicle-cli/src/commands/review.rs
@@ -80,14 +80,16 @@ impl Args for Options {
                     revision = Some(id);
                 }
                 Long("sync") => {
-                    sync = true;
+                    // Skipping due the `no-sync` flag precedence.
                 }
                 Long("no-sync") => {
                     sync = false;
                 }
                 Long("message") | Short('m') => {
-                    let txt: String = parser.value()?.to_string_lossy().into();
-                    message.append(&txt);
+                    if message != Comment::Blank {
+                        let txt: String = parser.value()?.to_string_lossy().into();
+                        message.append(&txt);
+                    }
                 }
                 Long("no-message") => {
                     message = Comment::Blank;
diff --git a/radicle-cli/src/terminal/patch.rs b/radicle-cli/src/terminal/patch.rs
index 0be15282..e1fe73a0 100644
--- a/radicle-cli/src/terminal/patch.rs
+++ b/radicle-cli/src/terminal/patch.rs
@@ -3,7 +3,7 @@ use radicle::git;
 use crate::terminal as term;
 
 /// How a comment is to be supplied by the user for a patch or issue on the terminal.
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub enum Comment {
     /// Prompt user to write comment in editor.
     Edit,