jj sometimes generates conflicts where Git (GitHub) doesn't

[chooglen]

Description

Steps to Reproduce the Problem

When I pushed https://github.com/martinvonz/jj/actions/runs/3492153286/jobs/5845616235, GitHub created this merge commit https://github.com/martinvonz/jj/commit/0b2077598d9639f1ed4e24791008a0829f29f689 and didn't complain of merge conflicts.

Expected Behavior

Doing the same merge in jj doesn't conflict.

Actual Behavior

Doing the same merge in jj results in a conflict:

jj new 2dc4f2fee11cd3c5f8d390b4e44b9daf514820ac f9f4f8b52040b7d42c2c9fa2c6b2735d97ece3d6;

The conflicted hunks are:

1. Odd, because there's no - side.

   
   <<<<<<<
   %%%%%%%
   +    pub fn finalize_writes(&mut self) {
   +        if let UiOutput::Paged {
   +            mut child,
   +            child_stdin,
   +        } = mem::replace(&mut self.output, UiOutput::new_terminal())
   +        {
   +            drop(child_stdin);
   +            child
   +                .wait()
   +                // It's possible (though unlikely) that this write fails, but
   +                // this function gets called so late that there's not much we
   +                // can do about it.
   +                .map_err(|e| self.write_error(&format!("Failed to wait on pager '{}'", e)))
   +                .ok();
   +        }
   +    }
   +
   +++++++
   pub fn prompt(&mut self, prompt: &str) -> io::Result<String> {
       if !atty::is(Stream::Stdout) {
           return Err(io::Error::new(
               io::ErrorKind::Unsupported,
               "Cannot prompt for input since the output is not connected to a terminal",
           ));
       }
       write!(self, "{}: ", prompt)?;
       self.flush()?;
       let mut buf = String::new();
       io::stdin().read_line(&mut buf)?;
       Ok(buf)
   }
   
   pub fn prompt_password(&mut self, prompt: &str) -> io::Result<String> {
       if !atty::is(Stream::Stdout) {
           return Err(io::Error::new(
               io::ErrorKind::Unsupported,
               "Cannot prompt for input since the output is not connected to a terminal",
           ));
       }
       rpassword::prompt_password(&format!("{}: ", prompt))
   }

2. jj seems to get confused by the single } line, so it doesn't place fn output_guard before and enum UiOutputPair after (which won't conflict).

   
   <<<<<<<
   %%%%%%%
   -enum UiOutputPair {
   -    Terminal { stdout: Stdout, stderr: Stderr },
   +enum UiOutput {
   +    Terminal {
   +        stdout: Stdout,
   +        stderr: Stderr,
   +    },
   +    Paged {
   +        child: Child,
   +        child_stdin: ChildStdin,
   +    },
   +++++++
   /// Construct a guard object which writes `data` when dropped. Useful for
   /// restoring terminal state.
   pub fn output_guard(&self, text: String) -> OutputGuard {
       OutputGuard {
           text,
           output: match self.output_pair {
               UiOutputPair::Terminal { .. } => io::stdout(),
           },
       }
   }
   }

enum UiOutputPair { Terminal { stdout: Stdout, stderr: Stderr },

}

3. Also no -

   
   <<<<<<<
   %%%%%%%
   +
   +pub struct OutputGuard {
   +    text: String,
   +    output: Stdout,
   +}
   +
   +impl Drop for OutputGuard {
   +    fn drop(&mut self) {
   +        _ = self.output.write_all(self.text.as_bytes());
   +    }
   +}
   +++++++

impl UiOutput { fn new_terminal() -> UiOutput { UiOutput::Terminal { stdout: io::stdout(), stderr: io::stderr(), } }

fn new_paged_else_terminal(settings: &UserSettings) -> UiOutput {
    let pager_cmd = pager_setting(settings);
    let child_result = Command::new(pager_cmd).stdin(Stdio::piped()).spawn();
    match child_result {
        Ok(mut child) => {
            let child_stdin = child.stdin.take().unwrap();
            UiOutput::Paged { child, child_stdin }
        }
        Err(e) => {
            io::stderr()
                .write_fmt(format_args!("Failed to spawn pager: '{}'", e))
                .ok();
            UiOutput::new_terminal()
        }
    }
}

}

Specifications

• Platform: MacOS 12.6.1
• Version: jj 0.5.1

[martinvonz]

1. Odd, because there's no - side.

That's the normal case :) See https://github.com/martinvonz/jj/blob/main/docs/conflicts.md#conflict-markers

[martinvonz]

Well, depends on what you mean by "no - side". It's normal that there's no ------- separator. That there are no lines in the diff prefixed by - just means that one side added those + lines without also removing anything.

[martinvonz]

To make this reproducible, can you share the base commit for 2dc4f2fee11cd3c5f8d390b4e44b9daf514820ac? By "base commit", I mean the common ancestor with the commit you merged with.

[martinvonz]

Oh, can you also try merging that again with git merge --diff-algorithm=histogram? I suspect the difference lies in the diff algorithm.

[martinvonz]

I just remembered that the ort strategy always uses histogram, and since ort is now the default, telling git merge to use histogram diff would make any difference.

After applying the changes in you conflicts onto bd7f72ff0348, I was able to reproduce this. I think the difference is in the diff algorithm, but a difference in my histogram implementation compared to Git's, which likely means a bug in my implementation :) I may have a fix for this in an old commit from when I initially implemented the histogram diff.

[arxanas]

I think the difference is in the diff algorithm, but a difference in my histogram implementation compared to Git's

Have you considered writing a test that to compare Git's histogram diff implementation against your own on some set of commits? This is how I validated patch application in git-branchless: https://github.com/arxanas/git-branchless/blob/6efd6de3bd6f201cc1d511fb6c0bbbf48ea4596a/git-branchless-lib/bin/testing/regression_test_cherry_pick.rs. I ran it against gecko-dev, but any other large repo should work, too. It found some number of bugs.

[martinvonz]

Good idea! I did lots of tests like that back when I wrote this code, but that might have been before I even added the --git format, so I think I only compared with git diff --color-words. Now that we have --git, it makes sense to compare exact output. We don't have to produce exactly the same output, of course, but it's a good idea to at least compare them.