Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#1088) post-rewrite hook does not track intermediate commits from an interactive rebase #1098

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions git-branchless-hook/tests/test_hook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ fn test_rebase_no_process_new_commits_until_conclusion() -> eyre::Result<()> {
insta::assert_snapshot!(stdout, @"");
}

git.commit_file("test4", 4)?;
{
let (stdout, stderr) = git.run(&["rebase", "--continue"])?;
insta::assert_snapshot!(stderr, @r###"
Expand Down Expand Up @@ -133,8 +132,6 @@ fn test_rebase_no_process_new_commits_until_conclusion() -> eyre::Result<()> {
O f777ecc (master) create initial.txt
|\
| o 047b7ad create test1.txt
| |
| o ecab41f create test4.txt
|
x 62fc20d (rewritten as 047b7ad7) create test1.txt
|
Expand Down
32 changes: 2 additions & 30 deletions git-branchless-lib/src/core/rewrite/rewrite_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,9 @@
use std::collections::{HashMap, HashSet};

use std::fmt::Write;
use std::fs::{self, File};
use std::io::{self, stdin, BufRead, BufReader, Read, Write as WriteIo};
use std::fs::File;
use std::io::{stdin, BufRead, BufReader, Read, Write as WriteIo};
use std::path::{Path, PathBuf};
use std::str::FromStr;
use std::time::SystemTime;

use console::style;
Expand Down Expand Up @@ -44,20 +43,6 @@ 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,
Expand Down Expand Up @@ -148,19 +133,6 @@ pub fn hook_post_rewrite(
let event_log_db = EventLogDb::new(&conn)?;
let event_tx_id = event_log_db.make_transaction_id(now, "hook-post-rewrite")?;

{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there code that also writes the deferred commits that is now also obsolete? If so, we should delete it too. Or, alternatively, keep all the code but comment out the add_events line with a note, as we might need similar code in the future to fix certain other bugs.

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
Expand Down
40 changes: 36 additions & 4 deletions git-branchless-lib/src/testing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -498,14 +498,15 @@ then you can only run tests in the main `git-branchless` and \
/// used to set the commit timestamp, which is factored into the commit
/// hash. The filename is always appended to the message prefix.
#[instrument]
pub fn commit_file_with_contents_and_message(
pub fn commit_file_with_contents_and_message_and_file_name(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather keep the API smaller. Can you revert this change and call write_file followed by commit_file instead? (Or feel free to use git.run(&["commit", ...]); the timestamp should still be stable for tests.)

&self,
name: &str,
time: isize,
contents: &str,
message_prefix: &str,
file_name: &str,
) -> eyre::Result<NonZeroOid> {
let message = format!("{message_prefix} {name}.txt");
let message = format!("{message_prefix} {file_name}");
self.write_file_txt(name, contents)?;
self.run(&["add", "."])?;
self.run_with_options(
Expand All @@ -524,6 +525,26 @@ then you can only run tests in the main `git-branchless` and \
Ok(oid)
}

/// Commit a file with given contents and message. The `time` argument is
/// used to set the commit timestamp, which is factored into the commit
/// hash. The filename is always appended to the message prefix.
#[instrument]
pub fn commit_file_with_contents_and_message(
&self,
name: &str,
time: isize,
contents: &str,
message_prefix: &str,
) -> eyre::Result<NonZeroOid> {
self.commit_file_with_contents_and_message_and_file_name(
name,
time,
contents,
message_prefix,
format!("{name}.txt").as_str(),
)
}

/// Commit a file with given contents and a default message. The `time`
/// argument is used to set the commit timestamp, which is factored into the
/// commit hash.
Expand Down Expand Up @@ -877,6 +898,18 @@ pub mod pty {
branchless_subcommand: &str,
args: &[&str],
inputs: &[PtyAction],
) -> eyre::Result<ExitStatus> {
// add "branchless" to subcommand list
run_in_pty_with_command(git, &["branchless", branchless_subcommand], args, inputs)
}

/// Run the provided script in the context of a virtual terminal.
#[track_caller]
pub fn run_in_pty_with_command(
git: &Git,
command: &[&str],
args: &[&str],
inputs: &[PtyAction],
) -> eyre::Result<ExitStatus> {
// Use the native pty implementation for the system
let pty_system = native_pty_system();
Expand All @@ -896,8 +929,7 @@ pub mod pty {
cmd.env(k, v);
}
cmd.env("TERM", "xterm");
cmd.arg("branchless");
cmd.arg(branchless_subcommand);
cmd.args(command);
cmd.args(args);
cmd.cwd(&git.repo_path);

Expand Down
54 changes: 54 additions & 0 deletions git-branchless/tests/test_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ use lib::core::eventlog::{Event, EventLogDb, EventReplayer};
use lib::core::formatting::Glyphs;
use lib::git::GitVersion;
use lib::testing::make_git;
use lib::testing::pty::{run_in_pty_with_command, PtyAction};
use lib::util::get_sh;
use std::process::Command;

const CARRIAGE_RETURN: &str = "\r";

#[test]
fn test_abandoned_commit_message() -> eyre::Result<()> {
let git = make_git()?;
Expand Down Expand Up @@ -385,3 +388,54 @@ fn test_git_am_recorded() -> eyre::Result<()> {

Ok(())
}

#[cfg(unix)]
#[test]
fn test_git_rebase_multiple_fixup_does_not_strand_commits() -> eyre::Result<()> {
let git = make_git()?;
git.init_repo()?;

git.detach_head()?;
git.commit_file_with_contents_and_message_and_file_name(
"test1",
1,
"bleh",
"create",
"test1.txt",
)?;
git.commit_file_with_contents_and_message_and_file_name(
"test2",
2,
"bleh",
"fixup! create",
"test1.txt",
)?;
git.commit_file_with_contents_and_message_and_file_name(
"test3",
3,
"bleh",
"fixup! create",
"test1.txt",
)?;

run_in_pty_with_command(
&git,
&["rebase"],
&["-i", "--autosquash", "master"],
&[
PtyAction::WaitUntilContains(" "),
PtyAction::Write(CARRIAGE_RETURN),
],
)?;
Comment on lines +421 to +429
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just write git.run(&["rebase", "--autosquash", "master"])? It seems to work in my testing. If so, do that and revert the changes to run_in_pty.


{
let stdout = git.smartlog()?;
insta::assert_snapshot!(stdout, @r###"
O f777ecc (master) create initial.txt
|
@ 916a41f create test1.txt
"###);
}

Ok(())
}
Loading