refactor(DisplaySettings): use Options for default fields (#161)

Previously, we needed to specify the default values in multiple places:
- the two `default{,_compact}` constructors
- in the binary, as `default_*_name`
- also in the binary, as the default values for flags

This commit removes all those and uses `None`s in there, instead adding methods called `DisplaySettings::<field_name>_or_default` that return `Some` values if they exist, or the _actual_ defaults in case of `None`.

There was another approach that came to mind, but was seen as too cumbersome: make a newtype for each such field, and implement `Default` for them -- this'd requier too much boilerplate and could require exposing the newtypes in a lot of places (most importantly, in the binary).

Ideally, of course, we'd use default field values, but those aren't stabilzed yet.[^1]

This also gets rid of the TODO in `mergiraf solve`, which was addressed back in #74

[^1]: rust-lang/rust#132162

Reviewed-on: https://codeberg.org/mergiraf/mergiraf/pulls/161
Reviewed-by: Antonin Delpeuch <wetneb@noreply.codeberg.org>
Co-authored-by: Ada Alakbarova <ada.alakbarova@proton.me>
Co-committed-by: Ada Alakbarova <ada.alakbarova@proton.me>

src/bin/mergiraf.rs

@@ -50,8 +50,8 @@ enum CliCommand {
         #[clap(long)]
         fast: bool,
         /// Display compact conflicts, breaking down lines
-        #[arg(short, long, default_value_t = false)]
-        compact: bool,
+        #[arg(short, long)]
+        compact: Option<bool>,
         /// Behave as a git merge driver: overwrite the left revision
         #[clap(short, long)]
         git: bool,
@@ -80,8 +80,8 @@ enum CliCommand {
         /// Path to a file containing merge conflicts
         conflicts: String,
         /// Display compact conflicts, breaking down lines
-        #[clap(short, long, default_value_t = false)]
-        compact: bool,
+        #[clap(short, long)]
+        compact: Option<bool>,
         /// Keep file untouched and show the results of resolution on standard output instead
         #[clap(short, long)]
         keep: bool,
@@ -122,10 +122,6 @@ fn real_main(args: CliArgs) -> Result<i32, String> {
         .init()
         .unwrap();
 
-    let default_base_name = "base";
-    let default_left_name = "left";
-    let default_right_name = "right";
-
     let return_code = match args.command {
         CliCommand::Merge {
             base,
@@ -144,20 +140,21 @@ fn real_main(args: CliArgs) -> Result<i32, String> {
 
             let settings = DisplaySettings {
                 compact,
+                conflict_marker_size: None, // TODO: get as flag
                 base_revision_name: match base_name.as_deref() {
-                    Some("%S") => default_base_name,
-                    Some(name) => name,
-                    None => &base,
+                    Some("%S") => None,
+                    Some(name) => Some(name),
+                    None => Some(&base),
                 },
                 left_revision_name: match left_name.as_deref() {
-                    Some("%X") => default_left_name,
-                    Some(name) => name,
-                    None => &left,
+                    Some("%X") => None,
+                    Some(name) => Some(name),
+                    None => Some(&left),
                 },
                 right_revision_name: match right_name.as_deref() {
-                    Some("%Y") => default_right_name,
-                    Some(name) => name,
-                    None => &right,
+                    Some("%Y") => None,
+                    Some(name) => Some(name),
+                    None => Some(&right),
                 },
                 ..Default::default()
             };
@@ -224,9 +221,11 @@ fn real_main(args: CliArgs) -> Result<i32, String> {
         } => {
             let settings = DisplaySettings {
                 compact,
-                base_revision_name: default_base_name, // TODO detect from file
-                left_revision_name: default_left_name,
-                right_revision_name: default_right_name,
+                // NOTE: the names will be recognized in `resolve_merge_cascading` (if possible)
+                base_revision_name: None,
+                left_revision_name: None,
+                right_revision_name: None,
+                conflict_marker_size: None, // TODO: get as flag
                 ..Default::default()
             };
 
@@ -313,13 +312,21 @@ fn fallback_to_git_merge_file(
     if !git {
         command.arg("-p");
     }
+    if let (Some(base_rev_name), Some(left_rev_name), Some(right_rev_name)) = (
+        settings.base_revision_name,
+        settings.left_revision_name,
+        settings.right_revision_name,
+    ) {
+        command
+            .arg("-L")
+            .arg(left_rev_name)
+            .arg("-L")
+            .arg(base_rev_name)
+            .arg("-L")
+            .arg(right_rev_name);
+    };
+
     command
-        .arg("-L")
-        .arg(settings.left_revision_name)
-        .arg("-L")
-        .arg(settings.base_revision_name)
-        .arg("-L")
-        .arg(settings.right_revision_name)
         .arg(left)
         .arg(base)
         .arg(right)

src/line_based.rs

@@ -15,7 +15,7 @@ pub(crate) fn line_based_merge(
     settings: &DisplaySettings,
 ) -> MergeResult {
     let merge_options = MergeOptions {
-        conflict_marker_length: settings.conflict_marker_size,
+        conflict_marker_length: settings.conflict_marker_size_or_default(),
         style: if settings.diff3 {
             ConflictStyle::Diff3
         } else {

src/merged_text.rs

@@ -123,7 +123,7 @@ impl<'a> MergedText<'a> {
 
     /// Renders the full file according to the supplied [`DisplaySettings`]
     pub(crate) fn render(&self, settings: &DisplaySettings) -> String {
-        if settings.compact {
+        if settings.compact_or_default() {
             self.render_compact(settings)
         } else {
             self.render_full_lines(settings)
@@ -223,14 +223,14 @@ impl<'a> MergedText<'a> {
         output: &mut String,
     ) {
         Self::maybe_add_newline(output);
-        output.push_str(&settings.left_marker());
+        output.push_str(&settings.left_marker_or_default());
         output.push('\n');
         if !left.trim().is_empty() {
             output.push_str(left);
         }
         if settings.diff3 {
             Self::maybe_add_newline(output);
-            output.push_str(&settings.base_marker());
+            output.push_str(&settings.base_marker_or_default());
             output.push('\n');
             if !base.trim().is_empty() {
                 output.push_str(base);
@@ -243,7 +243,7 @@ impl<'a> MergedText<'a> {
             output.push_str(right);
         }
         Self::maybe_add_newline(output);
-        output.push_str(&settings.right_marker());
+        output.push_str(&settings.right_marker_or_default());
         output.push('\n');
     }
 

src/parsed_merge.rs

@@ -288,18 +288,18 @@ impl<'a> ParsedMerge<'a> {
                 MergedChunk::Conflict {
                     left, base, right, ..
                 } => {
-                    result.push_str(&settings.left_marker());
+                    result.push_str(&settings.left_marker_or_default());
                     result.push('\n');
                     result.push_str(left);
                     if settings.diff3 {
-                        result.push_str(&settings.base_marker());
+                        result.push_str(&settings.base_marker_or_default());
                         result.push('\n');
                         result.push_str(base);
                     }
                     result.push_str(&settings.middle_marker());
                     result.push('\n');
                     result.push_str(right);
-                    result.push_str(&settings.right_marker());
+                    result.push_str(&settings.right_marker_or_default());
                     result.push('\n');
                 }
             }
@@ -633,9 +633,9 @@ mod tests {
         assert_eq!(
             enriched_settings,
             DisplaySettings {
-                left_revision_name: "my_left",
-                base_revision_name: "my_base",
-                right_revision_name: "my_right",
+                left_revision_name: Some("my_left"),
+                base_revision_name: Some("my_base"),
+                right_revision_name: Some("my_right"),
                 ..initial_settings
             }
         );

src/settings.rs

@@ -6,62 +6,87 @@ pub struct DisplaySettings<'a> {
     /// Whether to show the base revision in the conflicts (true by default)
     pub diff3: bool,
     /// Whether to show compact conflicts or to expand them to fill an entire line
-    pub compact: bool,
+    pub compact: Option<bool>,
     /// The number of characters for conflict markers (7 by default)
-    pub conflict_marker_size: usize,
+    pub conflict_marker_size: Option<usize>,
     /// The string that identifies the left revision in conflict markers
-    pub left_revision_name: &'a str,
+    pub left_revision_name: Option<&'a str>,
     /// The string that identifies the base revision in conflict markers
-    pub base_revision_name: &'a str,
+    pub base_revision_name: Option<&'a str>,
     /// The string that identifies the right revision in conflict markers
-    pub right_revision_name: &'a str,
+    pub right_revision_name: Option<&'a str>,
 }
 
 impl<'a> DisplaySettings<'a> {
+    /// The value of `compact` if set, the default value otherwise
+    pub fn compact_or_default(&self) -> bool {
+        self.compact.unwrap_or(false)
+    }
+
+    /// The value of `conflict_marker_size` if set, the default value otherwise
+    pub fn conflict_marker_size_or_default(&self) -> usize {
+        self.conflict_marker_size.unwrap_or(7)
+    }
+
+    /// The value of `left_revision_name` if set, the default value otherwise
+    pub fn left_revision_name_or_default(&self) -> &str {
+        self.left_revision_name.unwrap_or("LEFT")
+    }
+
+    /// The value of `base_revision_name` if set, the default value otherwise
+    pub fn base_revision_name_or_default(&self) -> &str {
+        self.base_revision_name.unwrap_or("BASE")
+    }
+
+    /// The value of `right_revision_name` if set, the default value otherwise
+    pub fn right_revision_name_or_default(&self) -> &str {
+        self.right_revision_name.unwrap_or("RIGHT")
+    }
+
     /// The marker at the beginning of the "left" (first) part of a conflict.
     /// It does not contain any newline character.
-    pub fn left_marker(&self) -> String {
+    /// Uses the default values of `conflict_marker_size` and `left_revision_name` if not set
+    pub fn left_marker_or_default(&self) -> String {
         format!(
             "{} {}",
-            "<".repeat(self.conflict_marker_size),
-            self.left_revision_name
+            "<".repeat(self.conflict_marker_size_or_default()),
+            self.left_revision_name_or_default()
         )
     }
 
     /// The marker at the beginning of the "base" part of a conflict.
     /// It does not contain any newline character.
-    pub fn base_marker(&self) -> String {
+    /// Uses the default values of `conflict_marker_size` and `base_revision_name` if not set
+    pub fn base_marker_or_default(&self) -> String {
         format!(
             "{} {}",
-            "|".repeat(self.conflict_marker_size),
-            self.base_revision_name
+            "|".repeat(self.conflict_marker_size_or_default()),
+            self.base_revision_name_or_default()
         )
     }
 
     /// The marker at the end of the "right" (last) part of a conflict.
     /// It does not contain any newline character.
-    pub fn right_marker(&self) -> String {
+    /// Uses the default values of `conflict_marker_size` and `right_revision_name` if not set
+    pub fn right_marker_or_default(&self) -> String {
         format!(
             "{} {}",
-            ">".repeat(self.conflict_marker_size),
-            self.right_revision_name
+            ">".repeat(self.conflict_marker_size_or_default()),
+            self.right_revision_name_or_default(),
         )
     }
 
     /// The marker before the beginning of "right" (last) part of a conflict.
     /// It does not contain any newline character.
+    /// Uses the default values of `conflict_marker_size` if not set
     pub fn middle_marker(&self) -> String {
-        "=".repeat(self.conflict_marker_size)
+        "=".repeat(self.conflict_marker_size_or_default())
     }
 
     pub fn default_compact() -> Self {
         Self {
-            diff3: true,
-            compact: true,
-            conflict_marker_size: 7,
-            left_revision_name: "LEFT",
-            base_revision_name: "BASE",
-            right_revision_name: "RIGHT",
+            compact: Some(true),
+            ..Default::default()
         }
     }
 
@@ -79,9 +104,9 @@ impl<'a> DisplaySettings<'a> {
             Some((left_name, base_name, right_name))
                 if !left_name.is_empty() && !base_name.is_empty() && !right_name.is_empty() =>
             {
-                self.left_revision_name = left_name;
-                self.base_revision_name = base_name;
-                self.right_revision_name = right_name;
+                self.left_revision_name = Some(left_name);
+                self.base_revision_name = Some(base_name);
+                self.right_revision_name = Some(right_name);
             }
             _ => {}
         }
@@ -92,11 +117,11 @@ impl Default for DisplaySettings<'_> {
     fn default() -> Self {
         Self {
             diff3: true,
-            compact: false,
-            conflict_marker_size: 7,
-            left_revision_name: "LEFT",
-            base_revision_name: "BASE",
-            right_revision_name: "RIGHT",
+            compact: Some(false),
+            conflict_marker_size: None,
+            left_revision_name: None,
+            base_revision_name: None,
+            right_revision_name: None,
         }
     }
 }