Skip to content

Commit a7b4278

Browse files
arxanasarxanas
committed
fix(submit:phabricator): do not let events escape git test worktrees
`arc diff` will amend the current commit as part of the submit process (primarily to create the change ID/embed the review URL in the commit message), but we don't want those changes to be available in the regular event log. My solution is to create a special event transaction ID that indicates that events should not be added to the event log. Another solution could be to allow events to created in a transaction, and then roll back the transaction rather than commit it. (Currently, "transactions" only refer to groups of events, and are always committed.) Normally, SQLite (which we use for the event log) would expose this kind of transactional functionality, but we can't use database-level transactions because the events span multiple process calls (such as Git hooks that are invoked as child processes), so we can't use the normal database mechanisms for this. It would be a lot of work to implement our own transaction system, so it just seems simpler to suppress all events immediately. One consequence is that users who create or rewrite commits inside `git test` calls will not have the events be preserved, which hopefully does not happen often in practice. Note that `git test fix` would still look at the resulting commits and apply the tree-level fixes, even though the original rewrite events in the worktrees wouldn't be preserved.
1 parent d518148 commit a7b4278

File tree

9 files changed

+147
-96
lines changed

9 files changed

+147
-96
lines changed

‎git-branchless-lib/src/core/eventlog.rs‎

Lines changed: 69 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -54,21 +54,33 @@ struct Row {
5454
/// Unlike in a database, there is no specific guarantee that an event
5555
/// transaction is an atomic unit of work.
5656
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
57-
pub struct EventTransactionId(isize);
57+
pub enum EventTransactionId {
58+
/// A normal transaction ID.
59+
Id(isize),
60+
61+
/// A value indicating that the no events should actually be added for this transaction.
62+
Suppressed,
63+
}
5864

5965
impl ToString for EventTransactionId {
6066
fn to_string(&self) -> String {
61-
let EventTransactionId(event_id) = self;
62-
event_id.to_string()
67+
match self {
68+
EventTransactionId::Id(event_id) => event_id.to_string(),
69+
EventTransactionId::Suppressed => "SUPPRESSED".to_string(),
70+
}
6371
}
6472
}
6573

6674
impl FromStr for EventTransactionId {
6775
type Err = <isize as FromStr>::Err;
6876

6977
fn from_str(s: &str) -> Result<Self, Self::Err> {
70-
let event_id = s.parse()?;
71-
Ok(EventTransactionId(event_id))
78+
if s == "SUPPRESSED" {
79+
Ok(EventTransactionId::Suppressed)
80+
} else {
81+
let event_id = s.parse()?;
82+
Ok(EventTransactionId::Id(event_id))
83+
}
7284
}
7385
}
7486

@@ -215,12 +227,39 @@ impl Event {
215227
}
216228
}
217229

218-
impl From<Event> for Row {
219-
fn from(event: Event) -> Row {
220-
match event {
230+
impl TryFrom<Event> for Row {
231+
type Error = ();
232+
233+
fn try_from(event: Event) -> Result<Self, Self::Error> {
234+
let row = match event {
235+
Event::RewriteEvent {
236+
event_tx_id: EventTransactionId::Suppressed,
237+
..
238+
}
239+
| Event::RefUpdateEvent {
240+
event_tx_id: EventTransactionId::Suppressed,
241+
..
242+
}
243+
| Event::CommitEvent {
244+
event_tx_id: EventTransactionId::Suppressed,
245+
..
246+
}
247+
| Event::ObsoleteEvent {
248+
event_tx_id: EventTransactionId::Suppressed,
249+
..
250+
}
251+
| Event::UnobsoleteEvent {
252+
event_tx_id: EventTransactionId::Suppressed,
253+
..
254+
}
255+
| Event::WorkingCopySnapshot {
256+
event_tx_id: EventTransactionId::Suppressed,
257+
..
258+
} => return Err(()),
259+
221260
Event::RewriteEvent {
222261
timestamp,
223-
event_tx_id: EventTransactionId(event_tx_id),
262+
event_tx_id: EventTransactionId::Id(event_tx_id),
224263
old_commit_oid,
225264
new_commit_oid,
226265
} => Row {
@@ -235,7 +274,7 @@ impl From<Event> for Row {
235274

236275
Event::RefUpdateEvent {
237276
timestamp,
238-
event_tx_id: EventTransactionId(event_tx_id),
277+
event_tx_id: EventTransactionId::Id(event_tx_id),
239278
ref_name,
240279
old_oid,
241280
new_oid,
@@ -252,7 +291,7 @@ impl From<Event> for Row {
252291

253292
Event::CommitEvent {
254293
timestamp,
255-
event_tx_id: EventTransactionId(event_tx_id),
294+
event_tx_id: EventTransactionId::Id(event_tx_id),
256295
commit_oid,
257296
} => Row {
258297
timestamp,
@@ -266,7 +305,7 @@ impl From<Event> for Row {
266305

267306
Event::ObsoleteEvent {
268307
timestamp,
269-
event_tx_id: EventTransactionId(event_tx_id),
308+
event_tx_id: EventTransactionId::Id(event_tx_id),
270309
commit_oid,
271310
} => Row {
272311
timestamp,
@@ -280,7 +319,7 @@ impl From<Event> for Row {
280319

281320
Event::UnobsoleteEvent {
282321
timestamp,
283-
event_tx_id: EventTransactionId(event_tx_id),
322+
event_tx_id: EventTransactionId::Id(event_tx_id),
284323
commit_oid,
285324
} => Row {
286325
timestamp,
@@ -294,7 +333,7 @@ impl From<Event> for Row {
294333

295334
Event::WorkingCopySnapshot {
296335
timestamp,
297-
event_tx_id: EventTransactionId(event_tx_id),
336+
event_tx_id: EventTransactionId::Id(event_tx_id),
298337
head_oid,
299338
commit_oid,
300339
ref_name,
@@ -307,7 +346,8 @@ impl From<Event> for Row {
307346
ref_name,
308347
message: None,
309348
},
310-
}
349+
};
350+
Ok(row)
311351
}
312352
}
313353

@@ -322,7 +362,7 @@ fn try_from_row_helper(row: &Row) -> Result<Event, eyre::Error> {
322362
ref2,
323363
message,
324364
} = row;
325-
let event_tx_id = EventTransactionId(event_tx_id);
365+
let event_tx_id = EventTransactionId::Id(event_tx_id);
326366

327367
let get_oid =
328368
|reference_name: &Option<ReferenceName>, oid_name: &str| -> eyre::Result<MaybeZeroOid> {
@@ -490,6 +530,10 @@ impl<'conn> EventLogDb<'conn> {
490530
pub fn add_events(&self, events: Vec<Event>) -> eyre::Result<()> {
491531
let tx = self.conn.unchecked_transaction()?;
492532
for event in events {
533+
let row = match Row::try_from(event) {
534+
Ok(row) => row,
535+
Err(()) => continue,
536+
};
493537
let Row {
494538
timestamp,
495539
type_,
@@ -498,7 +542,7 @@ impl<'conn> EventLogDb<'conn> {
498542
ref2,
499543
ref_name,
500544
message,
501-
} = Row::from(event);
545+
} = row;
502546

503547
let ref1 = ref1.as_ref().map(|x| x.as_str());
504548
let ref2 = ref2.as_ref().map(|x| x.as_str());
@@ -608,7 +652,7 @@ ORDER BY rowid ASC
608652
// SQLite connection.
609653
let event_tx_id: isize = self.conn.last_insert_rowid().try_into()?;
610654
tx.commit()?;
611-
Ok(EventTransactionId(event_tx_id))
655+
Ok(EventTransactionId::Id(event_tx_id))
612656
}
613657

614658
/// Create a new event transaction ID to be used to insert subsequent
@@ -623,7 +667,12 @@ ORDER BY rowid ASC
623667

624668
/// Get the message associated with the given transaction.
625669
pub fn get_transaction_message(&self, event_tx_id: EventTransactionId) -> eyre::Result<String> {
626-
let EventTransactionId(event_tx_id) = event_tx_id;
670+
let event_tx_id = match event_tx_id {
671+
EventTransactionId::Id(event_tx_id) => event_tx_id,
672+
EventTransactionId::Suppressed => {
673+
eyre::bail!("No message available for suppressed transaction ID")
674+
}
675+
};
627676
let mut stmt = self.conn.prepare(
628677
"
629678
SELECT message
@@ -1352,7 +1401,7 @@ pub mod testing {
13521401

13531402
/// Create a new transaction ID, for testing.
13541403
pub fn new_event_transaction_id(id: isize) -> EventTransactionId {
1355-
EventTransactionId(id)
1404+
EventTransactionId::Id(id)
13561405
}
13571406

13581407
/// Create a new event cursor, for testing.

‎git-branchless-lib/tests/test_eventlog.rs‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,21 +58,21 @@ fn test_different_event_transaction_ids() -> eyre::Result<()> {
5858
events.iter().map(|event| event.get_event_tx_id()).collect();
5959
if git.supports_reference_transactions()? {
6060
insta::assert_debug_snapshot!(event_tx_ids, @r###"
61-
[
62-
EventTransactionId(
63-
1,
64-
),
65-
EventTransactionId(
66-
1,
67-
),
68-
EventTransactionId(
69-
2,
70-
),
71-
EventTransactionId(
72-
3,
73-
),
74-
]
75-
"###);
61+
[
62+
Id(
63+
1,
64+
),
65+
Id(
66+
1,
67+
),
68+
Id(
69+
2,
70+
),
71+
Id(
72+
3,
73+
),
74+
]
75+
"###);
7676
} else {
7777
insta::assert_debug_snapshot!(event_tx_ids, @r###"
7878
[

‎git-branchless-submit/src/phabricator.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -389,7 +389,6 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
389389
self.dag,
390390
self.repo,
391391
self.event_log_db,
392-
event_tx_id,
393392
self.revset,
394393
&commits,
395394
&ResolvedTestOptions {
@@ -644,7 +643,6 @@ Differential Revision: https://phabricator.example.com/D000$(git rev-list --coun
644643
self.dag,
645644
self.repo,
646645
self.event_log_db,
647-
event_tx_id,
648646
self.revset,
649647
&commits,
650648
&test_options,

‎git-branchless-submit/tests/test_phabricator_forge.rs‎

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -101,9 +101,19 @@ fn test_submit_phabricator_strategy_worktree() -> eyre::Result<()> {
101101
git.detach_head()?;
102102
git.commit_file("test1", 1)?;
103103
git.commit_file("test2", 2)?;
104+
{
105+
let stdout = git.smartlog()?;
106+
insta::assert_snapshot!(stdout, @r###"
107+
O f777ecc (master) create initial.txt
108+
|
109+
o 62fc20d create test1.txt
110+
|
111+
@ 96d1c37 create test2.txt
112+
"###);
113+
}
104114

105115
{
106-
let (stdout, _stderr) = git.branchless_with_options(
116+
let (stdout, stderr) = git.branchless_with_options(
107117
"submit",
108118
&[
109119
"--create",
@@ -117,20 +127,19 @@ fn test_submit_phabricator_strategy_worktree() -> eyre::Result<()> {
117127
..Default::default()
118128
},
119129
)?;
130+
insta::assert_snapshot!(stderr, @r###"
131+
branchless: creating working copy snapshot
132+
Previous HEAD position was 96d1c37 create test2.txt
133+
branchless: processing 1 update: ref HEAD
134+
HEAD is now at ccb7fd5 create test2.txt
135+
branchless: processing checkout
136+
"###);
120137
insta::assert_snapshot!(stdout, @r###"
121138
Using command execution strategy: worktree
122139
Attempting rebase in-memory...
123140
[1/2] Committed as: 55af3db create test1.txt
124141
[2/2] Committed as: ccb7fd5 create test2.txt
125142
branchless: processing 2 rewritten commits
126-
branchless: This operation abandoned 1 commit!
127-
branchless: Consider running one of the following:
128-
branchless: - git restack: re-apply the abandoned commits/branches
129-
branchless: (this is most likely what you want to do)
130-
branchless: - git smartlog: assess the situation
131-
branchless: - git hide [<commit>...]: hide the commits from the smartlog
132-
branchless: - git undo: undo the operation
133-
hint: disable this hint by running: git config --global branchless.hint.restackWarnAbandoned false
134143
branchless: running command: <git-executable> checkout ccb7fd5d90c1888bea906a41c197e9215d6b9bb3
135144
In-memory rebase succeeded.
136145
Setting D0002 as stack root (no dependencies)
@@ -144,17 +153,10 @@ fn test_submit_phabricator_strategy_worktree() -> eyre::Result<()> {
144153
let stdout = git.smartlog()?;
145154
insta::assert_snapshot!(stdout, @r###"
146155
O f777ecc (master) create initial.txt
147-
|\
148-
| o 55af3db (D0002) D0002 create test1.txt
149-
| |
150-
| @ ccb7fd5 (D0003) D0003 create test2.txt
151156
|
152-
x 62fc20d (rewritten as 55af3dba) create test1.txt
157+
o 55af3db (D0002) D0002 create test1.txt
153158
|
154-
o 1ea08db D0003 create test2.txt
155-
hint: there is 1 abandoned commit in your commit graph
156-
hint: to fix this, run: git restack
157-
hint: disable this hint by running: git config --global branchless.hint.smartlogFixAbandoned false
159+
@ ccb7fd5 (D0003) D0003 create test2.txt
158160
"###);
159161
}
160162

‎git-branchless-test/src/lib.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,9 @@ use lib::core::config::{
4141
};
4242
use lib::core::dag::{sorted_commit_set, CommitSet, Dag};
4343
use lib::core::effects::{icons, Effects, OperationIcon, OperationType};
44-
use lib::core::eventlog::{EventLogDb, EventReplayer, EventTransactionId};
44+
use lib::core::eventlog::{
45+
EventLogDb, EventReplayer, EventTransactionId, BRANCHLESS_TRANSACTION_ID_ENV_VAR,
46+
};
4547
use lib::core::formatting::{Glyphs, Pluralize, StyledStringBuilder};
4648
use lib::core::repo_ext::RepoExt;
4749
use lib::core::rewrite::{
@@ -619,7 +621,6 @@ fn subcommand_run(
619621
&dag,
620622
&repo,
621623
&event_log_db,
622-
event_tx_id,
623624
&revset,
624625
&commits,
625626
&options,
@@ -1207,11 +1208,11 @@ pub fn run_tests<'a>(
12071208
dag: &Dag,
12081209
repo: &Repo,
12091210
event_log_db: &EventLogDb,
1210-
event_tx_id: EventTransactionId,
12111211
revset: &Revset,
12121212
commits: &[Commit],
12131213
options: &ResolvedTestOptions,
12141214
) -> EyreExitOr<TestResults> {
1215+
let event_tx_id = EventTransactionId::Suppressed;
12151216
let abort_trap = match set_abort_trap(
12161217
now,
12171218
effects,
@@ -2626,6 +2627,7 @@ fn test_commit(
26262627
.arg("-c")
26272628
.arg(options.command.to_string())
26282629
.current_dir(working_directory)
2630+
.env(BRANCHLESS_TRANSACTION_ID_ENV_VAR, event_tx_id.to_string())
26292631
.env("BRANCHLESS_TEST_COMMIT", commit.get_oid().to_string())
26302632
.env("BRANCHLESS_TEST_COMMAND", options.command.to_string());
26312633

‎git-branchless/tests/test_bug_report.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,30 +126,30 @@ fn test_bug_report() -> eyre::Result<()> {
126126
127127
##### Event ID: 6, transaction ID: 4 (message: post-commit)
128128
129-
1. `CommitEvent { timestamp: <redacted for test>, event_tx_id: EventTransactionId(4), commit_oid: NonZeroOid(96d1c37a3d4363611c49f7e52186e189a04c531f) }`
129+
1. `CommitEvent { timestamp: <redacted for test>, event_tx_id: Id(4), commit_oid: NonZeroOid(96d1c37a3d4363611c49f7e52186e189a04c531f) }`
130130
```
131131
:
132132
@ 96d1c37 (> master) xxxxxx xxxxxxxxx
133133
```
134134
##### Event ID: 4, transaction ID: 3 (message: reference-transaction)
135135
136-
1. `RefUpdateEvent { timestamp: <redacted for test>, event_tx_id: EventTransactionId(3), ref_name: ReferenceName("HEAD"), old_oid: 62fc20d2a290daea0d52bdc2ed2ad4be6491010e, new_oid: 96d1c37a3d4363611c49f7e52186e189a04c531f, message: None }`
137-
1. `RefUpdateEvent { timestamp: <redacted for test>, event_tx_id: EventTransactionId(3), ref_name: ReferenceName("refs/heads/master"), old_oid: 62fc20d2a290daea0d52bdc2ed2ad4be6491010e, new_oid: 96d1c37a3d4363611c49f7e52186e189a04c531f, message: None }`
136+
1. `RefUpdateEvent { timestamp: <redacted for test>, event_tx_id: Id(3), ref_name: ReferenceName("HEAD"), old_oid: 62fc20d2a290daea0d52bdc2ed2ad4be6491010e, new_oid: 96d1c37a3d4363611c49f7e52186e189a04c531f, message: None }`
137+
1. `RefUpdateEvent { timestamp: <redacted for test>, event_tx_id: Id(3), ref_name: ReferenceName("refs/heads/master"), old_oid: 62fc20d2a290daea0d52bdc2ed2ad4be6491010e, new_oid: 96d1c37a3d4363611c49f7e52186e189a04c531f, message: None }`
138138
```
139139
:
140140
@ 96d1c37 (> master) xxxxxx xxxxxxxxx
141141
```
142142
##### Event ID: 3, transaction ID: 2 (message: post-commit)
143143
144-
1. `CommitEvent { timestamp: <redacted for test>, event_tx_id: EventTransactionId(2), commit_oid: NonZeroOid(62fc20d2a290daea0d52bdc2ed2ad4be6491010e) }`
144+
1. `CommitEvent { timestamp: <redacted for test>, event_tx_id: Id(2), commit_oid: NonZeroOid(62fc20d2a290daea0d52bdc2ed2ad4be6491010e) }`
145145
```
146146
:
147147
@ 96d1c37 (> master) xxxxxx xxxxxxxxx
148148
```
149149
##### Event ID: 1, transaction ID: 1 (message: reference-transaction)
150150
151-
1. `RefUpdateEvent { timestamp: <redacted for test>, event_tx_id: EventTransactionId(1), ref_name: ReferenceName("HEAD"), old_oid: f777ecc9b0db5ed372b2615695191a8a17f79f24, new_oid: 62fc20d2a290daea0d52bdc2ed2ad4be6491010e, message: None }`
152-
1. `RefUpdateEvent { timestamp: <redacted for test>, event_tx_id: EventTransactionId(1), ref_name: ReferenceName("refs/heads/master"), old_oid: f777ecc9b0db5ed372b2615695191a8a17f79f24, new_oid: 62fc20d2a290daea0d52bdc2ed2ad4be6491010e, message: None }`
151+
1. `RefUpdateEvent { timestamp: <redacted for test>, event_tx_id: Id(1), ref_name: ReferenceName("HEAD"), old_oid: f777ecc9b0db5ed372b2615695191a8a17f79f24, new_oid: 62fc20d2a290daea0d52bdc2ed2ad4be6491010e, message: None }`
152+
1. `RefUpdateEvent { timestamp: <redacted for test>, event_tx_id: Id(1), ref_name: ReferenceName("refs/heads/master"), old_oid: f777ecc9b0db5ed372b2615695191a8a17f79f24, new_oid: 62fc20d2a290daea0d52bdc2ed2ad4be6491010e, message: None }`
153153
```
154154
:
155155
@ 96d1c37 (> master) xxxxxx xxxxxxxxx

0 commit comments

Comments
 (0)