More Phabricator forge for git submit

git-branchless-hook/Cargo.toml

@@ -21,3 +21,4 @@ git-branchless-opts = { version = "0.7.0", path = "../git-branchless-opts" }
 
 [dev-dependencies]
 git-branchless-testing = { version = "0.7.0", path = "../git-branchless-testing" }
+insta = "1.28.0"

git-branchless-hook/src/lib.rs

@@ -17,17 +17,21 @@
 #![allow(clippy::too_many_arguments, clippy::blocks_in_if_conditions)]
 
 use std::fmt::Write;
+use std::fs::File;
 use std::io::{stdin, BufRead};
 use std::time::SystemTime;
 
 use eyre::Context;
 use git_branchless_invoke::CommandContext;
 use git_branchless_opts::{HookArgs, HookSubcommand};
 use itertools::Itertools;
+use lib::core::dag::Dag;
+use lib::core::repo_ext::RepoExt;
+use lib::core::rewrite::rewrite_hooks::get_deferred_commits_path;
 use lib::util::ExitCode;
 use tracing::{error, instrument, warn};
 
-use lib::core::eventlog::{should_ignore_ref_updates, Event, EventLogDb};
+use lib::core::eventlog::{should_ignore_ref_updates, Event, EventLogDb, EventReplayer};
 use lib::core::formatting::{Glyphs, Pluralize};
 use lib::core::gc::{gc, mark_commit_reachable};
 use lib::git::{CategorizedReferenceName, MaybeZeroOid, NonZeroOid, ReferenceName, Repo};
@@ -102,6 +106,32 @@ fn hook_post_commit_common(effects: &Effects, hook_name: &str) -> eyre::Result<(
     mark_commit_reachable(&repo, commit_oid)
         .wrap_err("Marking commit as reachable for GC purposes")?;
 
+    let event_replayer = EventReplayer::from_event_log_db(effects, &repo, &event_log_db)?;
+    let event_cursor = event_replayer.make_default_cursor();
+    let references_snapshot = repo.get_references_snapshot()?;
+    Dag::open_and_sync(
+        effects,
+        &repo,
+        &event_replayer,
+        event_cursor,
+        &references_snapshot,
+    )?;
+
+    if repo.is_rebase_underway()? {
+        let deferred_commits_path = get_deferred_commits_path(&repo);
+        let mut deferred_commits_file = File::options()
+            .create(true)
+            .append(true)
+            .open(&deferred_commits_path)
+            .with_context(|| {
+                format!("Opening deferred commits file at {deferred_commits_path:?}")
+            })?;
+
+        use std::io::Write;
+        writeln!(deferred_commits_file, "{commit_oid}")?;
+        return Ok(());
+    }
+
     let timestamp = commit.get_time().to_system_time()?;
 
     // Potentially lossy conversion. The semantics are to round to the nearest

git-branchless-hook/tests/test_hook.rs

@@ -22,3 +22,129 @@ fn test_is_rebase_underway() -> eyre::Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_rebase_no_process_new_commits_until_conclusion() -> eyre::Result<()> {
+    let git = make_git()?;
+
+    if !git.supports_reference_transactions()? {
+        return Ok(());
+    }
+    git.init_repo()?;
+
+    git.detach_head()?;
+    git.commit_file("test1", 1)?;
+    let test2_oid = git.commit_file("test2", 2)?;
+
+    // Ensure commits aren't preserved if the rebase is aborted.
+    {
+        git.run_with_options(
+            &["rebase", "master", "--force", "--exec", "exit 1"],
+            &GitRunOptions {
+                expected_exit_code: 1,
+                ..Default::default()
+            },
+        )?;
+        git.run(&[
+            "commit",
+            "--amend",
+            "--message",
+            "this commit shouldn't show up in the smartlog",
+        ])?;
+        git.commit_file("test3", 3)?;
+        git.run(&["rebase", "--abort"])?;
+
+        {
+            let stdout = git.smartlog()?;
+            insta::assert_snapshot!(stdout, @r###"
+        O f777ecc (master) create initial.txt
+        |
+        o 62fc20d create test1.txt
+        |
+        @ 96d1c37 create test2.txt
+        "###);
+        }
+    }
+
+    // Ensure commits are preserved if the rebase succeeds.
+    {
+        git.run(&["checkout", "HEAD^"])?;
+
+        {
+            let (stdout, stderr) = git.run_with_options(
+                &["rebase", "master", "--force", "--exec", "exit 1"],
+                &GitRunOptions {
+                    expected_exit_code: 1,
+                    ..Default::default()
+                },
+            )?;
+
+            // As of `38c541ce94048cf72aa4f465be9314423a57f445` (Git >=v2.36.0),
+            // `git checkout` is called in fewer cases, which affects the stderr
+            // output for the test.
+            let stderr: String = stderr
+                .lines()
+                .filter_map(|line| {
+                    if line.starts_with("branchless:") {
+                        None
+                    } else {
+                        Some(format!("{line}\n"))
+                    }
+                })
+                .collect();
+
+            insta::assert_snapshot!(stderr, @r###"
+            Executing: exit 1
+            warning: execution failed: exit 1
+            You can fix the problem, and then run
+
+              git rebase --continue
+
+
+            "###);
+            insta::assert_snapshot!(stdout, @"");
+        }
+
+        git.commit_file("test4", 4)?;
+        {
+            let (stdout, stderr) = git.run(&["rebase", "--continue"])?;
+            insta::assert_snapshot!(stderr, @r###"
+            branchless: processing 1 rewritten commit
+            branchless: This operation abandoned 1 commit!
+            branchless: Consider running one of the following:
+            branchless:   - git restack: re-apply the abandoned commits/branches
+            branchless:     (this is most likely what you want to do)
+            branchless:   - git smartlog: assess the situation
+            branchless:   - git hide [<commit>...]: hide the commits from the smartlog
+            branchless:   - git undo: undo the operation
+            hint: disable this hint by running: git config --global branchless.hint.restackWarnAbandoned false
+            Successfully rebased and updated detached HEAD.
+            "###);
+            insta::assert_snapshot!(stdout, @"");
+        }
+
+        // Switch away to make sure that the new commit isn't visible just
+        // because it's reachable from `HEAD`.
+        git.run(&["checkout", &test2_oid.to_string()])?;
+
+        {
+            let stdout = git.smartlog()?;
+            insta::assert_snapshot!(stdout, @r###"
+            O f777ecc (master) create initial.txt
+            |\
+            | o 047b7ad create test1.txt
+            | |
+            | o ecab41f create test4.txt
+            |
+            x 62fc20d (rewritten as 047b7ad7) create test1.txt
+            |
+            @ 96d1c37 create test2.txt
+            hint: there is 1 abandoned commit in your commit graph
+            hint: to fix this, run: git restack
+            hint: disable this hint by running: git config --global branchless.hint.smartlogFixAbandoned false
+            "###);
+        }
+    }
+
+    Ok(())
+}

git-branchless-lib/src/core/dag.rs

@@ -358,9 +358,13 @@ impl Dag {
 
     /// Wrapper around DAG method.
     #[instrument]
-    pub fn sort(&self, commit_set: &CommitSet) -> eyre::Result<CommitSet> {
-        let result = self.run_blocking(self.inner.sort(commit_set))?;
-        Ok(result)
+    pub fn sort(&self, commit_set: &CommitSet) -> eyre::Result<Vec<NonZeroOid>> {
+        let commit_set = self.run_blocking(self.inner.sort(commit_set))?;
+        let commit_oids = self.commit_set_to_vec(&commit_set)?;
+
+        // `.sort` seems to sort it such that the child-most commits are first?
+        // In all current use-cases, we want to start with the parent commits.
+        Ok(commit_oids.into_iter().rev().collect())
     }
 
     /// Eagerly convert a `CommitSet` into a `Vec<NonZeroOid>` by iterating over it, preserving order.

git-branchless-lib/src/core/rewrite/rewrite_hooks.rs

@@ -3,9 +3,10 @@
 use std::collections::{HashMap, HashSet};
 
 use std::fmt::Write;
-use std::fs::File;
-use std::io::{stdin, BufRead, BufReader, Read, Write as WriteIo};
-use std::path::Path;
+use std::fs::{self, File};
+use std::io::{self, stdin, BufRead, BufReader, Read, Write as WriteIo};
+use std::path::{Path, PathBuf};
+use std::str::FromStr;
 use std::time::SystemTime;
 
 use console::style;
@@ -29,6 +30,34 @@ use crate::git::{
 use super::execute::check_out_updated_head;
 use super::{find_abandoned_children, move_branches};
 
+/// Get the path to the file which stores the list of "deferred commits".
+///
+/// During a rebase, we make new commits, but if we abort the rebase, we don't
+/// want those new commits to persist in the smartlog, etc. To address this, we
+/// instead queue up the list of created commits and only confirm them once the
+/// rebase has completed.
+///
+/// Note that this has the effect that if the user manually creates a commit
+/// during a rebase, and then aborts the rebase, the commit will not be
+/// available in the event log anywhere. This is probably acceptable.
+pub fn get_deferred_commits_path(repo: &Repo) -> PathBuf {
+    repo.get_rebase_state_dir_path().join("deferred-commits")
+}
+
+fn read_deferred_commits(repo: &Repo) -> eyre::Result<Vec<NonZeroOid>> {
+    let deferred_commits_path = get_deferred_commits_path(repo);
+    let contents = match fs::read_to_string(&deferred_commits_path) {
+        Ok(contents) => contents,
+        Err(err) if err.kind() == io::ErrorKind::NotFound => Default::default(),
+        Err(err) => {
+            return Err(err)
+                .with_context(|| format!("Reading deferred commits at {deferred_commits_path:?}"))
+        }
+    };
+    let commit_oids = contents.lines().map(NonZeroOid::from_str).try_collect()?;
+    Ok(commit_oids)
+}
+
 #[instrument(skip(stream))]
 fn read_rewritten_list_entries(
     stream: &mut impl Read,
@@ -110,11 +139,29 @@ pub fn hook_post_rewrite(
     let timestamp = now.duration_since(SystemTime::UNIX_EPOCH)?.as_secs_f64();
 
     let repo = Repo::from_current_dir()?;
+    let is_spurious_event = rewrite_type == "amend" && repo.is_rebase_underway()?;
+    if is_spurious_event {
+        return Ok(());
+    }
+
     let conn = repo.get_db_conn()?;
     let event_log_db = EventLogDb::new(&conn)?;
     let event_tx_id = event_log_db.make_transaction_id(now, "hook-post-rewrite")?;
 
-    let (rewritten_oids, events) = {
+    {
+        let deferred_commit_oids = read_deferred_commits(&repo)?;
+        let commit_events = deferred_commit_oids
+            .into_iter()
+            .map(|commit_oid| Event::CommitEvent {
+                timestamp,
+                event_tx_id,
+                commit_oid,
+            })
+            .collect_vec();
+        event_log_db.add_events(commit_events)?;
+    }
+
+    let (rewritten_oids, rewrite_events) = {
         let rewritten_oids = read_rewritten_list_entries(&mut stdin().lock())?;
         let events = rewritten_oids
             .iter()
@@ -131,21 +178,17 @@ pub fn hook_post_rewrite(
         (rewritten_oids_map, events)
     };
 
-    let is_spurious_event = rewrite_type == "amend" && repo.is_rebase_underway()?;
-    if !is_spurious_event {
-        let message_rewritten_commits = Pluralize {
-            determiner: None,
-            amount: rewritten_oids.len(),
-            unit: ("rewritten commit", "rewritten commits"),
-        }
-        .to_string();
-        writeln!(
-            effects.get_output_stream(),
-            "branchless: processing {message_rewritten_commits}"
-        )?;
+    let message_rewritten_commits = Pluralize {
+        determiner: None,
+        amount: rewritten_oids.len(),
+        unit: ("rewritten commit", "rewritten commits"),
     }
-
-    event_log_db.add_events(events)?;
+    .to_string();
+    writeln!(
+        effects.get_output_stream(),
+        "branchless: processing {message_rewritten_commits}"
+    )?;
+    event_log_db.add_events(rewrite_events)?;
 
     if repo
         .get_rebase_state_dir_path()

git-branchless-lib/src/util.rs

@@ -1,13 +1,20 @@
 //! Utility functions.
 
+use std::num::TryFromIntError;
 use std::path::PathBuf;
+use std::process::ExitStatus;
 
 /// Represents the code to exit the process with.
 #[must_use]
 #[derive(Copy, Clone, Debug)]
 pub struct ExitCode(pub isize);
 
 impl ExitCode {
+    /// Return an exit code corresponding to success.
+    pub fn success() -> Self {
+        Self(0)
+    }
+
     /// Determine whether or not this exit code represents a successful
     /// termination.
     pub fn is_success(&self) -> bool {
@@ -18,6 +25,15 @@ impl ExitCode {
     }
 }
 
+impl TryFrom<ExitStatus> for ExitCode {
+    type Error = TryFromIntError;
+
+    fn try_from(status: ExitStatus) -> Result<Self, Self::Error> {
+        let exit_code = status.code().unwrap_or(1);
+        Ok(Self(exit_code.try_into()?))
+    }
+}
+
 /// Returns a path for a given file, searching through PATH to find it.
 pub fn get_from_path(exe_name: &str) -> Option<PathBuf> {
     std::env::var_os("PATH").and_then(|paths| {

git-branchless-opts/src/lib.rs

@@ -346,6 +346,20 @@ pub struct SmartlogArgs {
     pub resolve_revset_options: ResolveRevsetOptions,
 }
 
+/// The Git hosting provider to use, called a "forge".
+#[derive(Clone, Debug, ValueEnum)]
+pub enum ForgeKind {
+    /// Force-push branches to the default push remote.
+    Branch,
+
+    /// Force-push branches to the remote and create a pull request for each
+    /// branch using the `gh` command-line tool.
+    Github,
+
+    /// Submit code reviews to Phabricator using the `arc` command-line tool.
+    Phabricator,
+}
+
 /// Push commits to a remote.
 #[derive(Debug, Parser)]
 pub struct SubmitArgs {
@@ -374,6 +388,10 @@ pub struct SubmitArgs {
     /// Options for resolving revset expressions.
     #[clap(flatten)]
     pub resolve_revset_options: ResolveRevsetOptions,
+
+    /// The Git hosting provider to use.
+    #[clap(short = 'F', long = "forge")]
+    pub forge: Option<ForgeKind>,
 }
 
 /// Run a command on each commit in a given set and aggregate the results.