Merge pull request #5068 from gitbutlerapp/Fix-ordering-and-conflict-viewing-and-diffing

Make edit mode fantastic

apps/desktop/src/lib/components/EditMode.svelte

@@ -3,7 +3,11 @@
 	import { CommitService } from '$lib/commits/service';
 	import { editor } from '$lib/editorLink/editorLink';
 	import FileContextMenu from '$lib/file/FileContextMenu.svelte';
-	import { ModeService, type EditModeMetadata } from '$lib/modes/service';
+	import {
+		ModeService,
+		type ConflictEntryPresence,
+		type EditModeMetadata
+	} from '$lib/modes/service';
 	import ScrollableContainer from '$lib/scroll/ScrollableContainer.svelte';
 	import { UncommitedFilesWatcher } from '$lib/uncommitedFiles/watcher';
 	import { getContext } from '$lib/utils/context';
@@ -33,7 +37,7 @@
 	let modeServiceAborting = $state<'inert' | 'loading' | 'completed'>('inert');
 	let modeServiceSaving = $state<'inert' | 'loading' | 'completed'>('inert');
 
-	let initialFiles = $state<RemoteFile[]>([]);
+	let initialFiles = $state<[RemoteFile, ConflictEntryPresence | undefined][]>([]);
 	let commit = $state<Commit | undefined>(undefined);
 
 	let filesList = $state<HTMLDivElement | undefined>(undefined);
@@ -55,9 +59,34 @@
 		name: string;
 		path: string;
 		conflicted: boolean;
+		conflictHint?: string;
 		status?: FileStatus;
 	}
 
+	function conflictEntryHint(presence: ConflictEntryPresence): string {
+		let defaultVerb = 'added';
+
+		if (presence.ancestor) {
+			defaultVerb = 'modified';
+		}
+
+		let oursVerb = defaultVerb;
+
+		if (!presence.ours) {
+			oursVerb = 'deleted';
+		}
+
+		let theirsVerb = defaultVerb;
+
+		if (!presence.theirs) {
+			theirsVerb = 'deleted';
+		}
+
+		let output = `You have ${theirsVerb} this file, They have ${oursVerb} this file.`;
+
+		return output;
+	}
+
 	const files = $derived.by(() => {
 		const initialFileMap = new Map<string, RemoteFile>();
 		const uncommitedFileMap = new Map<string, RemoteFile>();
@@ -65,7 +94,7 @@
 
 		// Build maps of files
 		{
-			initialFiles.forEach((initialFile) => {
+			initialFiles.forEach(([initialFile]) => {
 				initialFileMap.set(initialFile.path, initialFile);
 			});
 
@@ -76,14 +105,21 @@
 
 		// Create output
 		{
-			initialFiles.forEach((initialFile) => {
+			initialFiles.forEach(([initialFile, conflictEntryPresence]) => {
 				const isDeleted = uncommitedFileMap.has(initialFile.path);
 
+				if (conflictEntryPresence) {
+					console.log(initialFile.path, conflictEntryPresence);
+				}
+
 				outputMap.set(initialFile.path, {
 					name: initialFile.filename,
 					path: initialFile.path,
-					conflicted: initialFile.looksConflicted,
+					conflicted: !!conflictEntryPresence,
-					status: isDeleted ? undefined : 'D'
+					conflictHint: conflictEntryPresence
+						? conflictEntryHint(conflictEntryPresence)
+						: undefined,
+					status: isDeleted || !!conflictEntryPresence ? undefined : 'D'
 				});
 			});
 
@@ -95,18 +131,13 @@
 						(hunk) => !uncommitedFile.hunks.map((hunk) => hunk.diff).includes(hunk.diff)
 					);
 
-					if (fileChanged && !uncommitedFile.looksConflicted) {
+					if (fileChanged) {
 						// All initial entries should have been added to the map,
 						// so we can safely assert that it will be present
 						const outputFile = outputMap.get(uncommitedFile.path)!;
-						outputFile.status = 'M';
+						if (!outputFile.conflicted) {
-						outputFile.conflicted = false;
+							outputFile.status = 'M';
-						return;
+						}
-					}
-
-					if (uncommitedFile.looksConflicted) {
-						const outputFile = outputMap.get(uncommitedFile.path)!;
-						outputFile.conflicted = true;
 						return;
 					}
 
@@ -122,8 +153,8 @@
 			});
 		}
 
-		const files = Array.from(outputMap.values());
+		const orderedOutput = Array.from(outputMap.values());
-		files.sort((a, b) => {
+		orderedOutput.sort((a, b) => {
 			// Float conflicted files to the top
 			if (a.conflicted && !b.conflicted) {
 				return -1;
@@ -134,7 +165,7 @@
 			return a.path.localeCompare(b.path);
 		});
 
-		return files;
+		return orderedOutput;
 	});
 
 	const conflictedFiles = $derived(files.filter((file) => file.conflicted));
@@ -201,12 +232,13 @@
 				<Badge label={files.length} />
 			</div>
 			<ScrollableContainer>
-				{#each files as file}
+				{#each files as file (file.path)}
 					<div class="file">
 						<FileListItem
 							filePath={file.path}
 							fileStatus={file.status}
 							conflicted={file.conflicted}
+							conflictHint={file.conflictHint}
 							fileStatusStyle={file.status === 'M' ? 'full' : 'dot'}
 							onclick={(e) => {
 								contextMenu?.open(e, { files: [file] });

apps/desktop/src/lib/modes/service.ts

@@ -8,6 +8,12 @@ export interface EditModeMetadata {
 	branchReference: string;
 }
 
+export interface ConflictEntryPresence {
+	ours: boolean;
+	theirs: boolean;
+	ancestor: boolean;
+}
+
 type Mode =
 	| { type: 'OpenWorkspace' }
 	| { type: 'OutsideWorkspace' }
@@ -64,10 +70,13 @@ export class ModeService {
 	}
 
 	async getInitialIndexState() {
-		return plainToInstance(
+		const rawOutput = await invoke<unknown[][]>('edit_initial_index_state', {
-			RemoteFile,
+			projectId: this.projectId
-			await invoke<unknown[]>('edit_initial_index_state', { projectId: this.projectId })
+		});
-		);
+
+		return rawOutput.map((entry) => {
+			return [plainToInstance(RemoteFile, entry[0]), entry[1] as ConflictEntryPresence | undefined];
+		}) as [RemoteFile, ConflictEntryPresence | undefined][];
 	}
 }
 

apps/desktop/src/lib/vbranches/types.ts

@@ -84,10 +84,6 @@ export class LocalFile {
 			.filter(notNull)
 			.filter(isDefined);
 	}
-
-	get looksConflicted(): boolean {
-		return fileLooksConflicted(this);
-	}
 }
 
 export class SkippedFile {
@@ -312,22 +308,6 @@ export class RemoteFile {
 	get locked(): boolean {
 		return false;
 	}
-
-	get looksConflicted(): boolean {
-		return fileLooksConflicted(this);
-	}
-}
-
-function fileLooksConflicted(file: AnyFile) {
-	const hasStartingMarker = file.hunks.some((hunk) =>
-		hunk.diff.split('\n').some((line) => line.startsWith('>>>>>>> theirs', 1))
-	);
-
-	const hasEndingMarker = file.hunks.some((hunk) =>
-		hunk.diff.split('\n').some((line) => line.startsWith('<<<<<<< ours', 1))
-	);
-
-	return hasStartingMarker && hasEndingMarker;
 }
 
 export interface Author {

crates/gitbutler-edit-mode/src/commands.rs

@@ -9,6 +9,8 @@ use gitbutler_oplog::{
 use gitbutler_project::{access::WriteWorkspaceGuard, Project};
 use gitbutler_reference::ReferenceName;
 
+use crate::ConflictEntryPresence;
+
 pub fn enter_edit_mode(
     project: &Project,
     commit_oid: git2::Oid,
@@ -61,7 +63,9 @@ pub fn abort_and_return_to_workspace(project: &Project) -> Result<()> {
     crate::abort_and_return_to_workspace(&ctx, guard.write_permission())
 }
 
-pub fn starting_index_state(project: &Project) -> Result<Vec<RemoteBranchFile>> {
+pub fn starting_index_state(
+    project: &Project,
+) -> Result<Vec<(RemoteBranchFile, Option<ConflictEntryPresence>)>> {
     let (ctx, guard) = open_with_permission(project)?;
 
     assure_edit_mode(&ctx)?;

crates/gitbutler-edit-mode/src/lib.rs

@@ -1,3 +1,5 @@
+use std::collections::HashMap;
+use std::path::PathBuf;
 use std::str::FromStr;
 
 use anyhow::{bail, Context, Result};
@@ -25,6 +27,7 @@ use gitbutler_reference::{ReferenceName, Refname};
 use gitbutler_repo::{rebase::cherry_rebase, RepositoryExt};
 use gitbutler_stack::{Stack, VirtualBranchesHandle};
 use gitbutler_stack_api::StackExt;
+use serde::Serialize;
 
 pub mod commands;
 
@@ -76,7 +79,7 @@ fn checkout_edit_branch(ctx: &CommandContext, commit: &git2::Commit) -> Result<(
 
     // Checkout commits's parent
     let commit_parent = if commit.is_conflicted() {
-        let base_tree = repository.find_real_tree(commit, ConflictedTreeKey::Base)?;
+        let base_tree = repository.find_real_tree(commit, ConflictedTreeKey::Ours)?;
 
         let base = repository.commit(
             None,
@@ -243,10 +246,18 @@ pub(crate) fn save_and_return_to_workspace(
     Ok(())
 }
 
+#[derive(Serialize, Debug, Clone)]
+#[serde(rename_all = "camelCase")]
+pub struct ConflictEntryPresence {
+    pub ours: bool,
+    pub theirs: bool,
+    pub ancestor: bool,
+}
+
 pub(crate) fn starting_index_state(
     ctx: &CommandContext,
     _perm: &WorktreeReadPermission,
-) -> Result<Vec<RemoteBranchFile>> {
+) -> Result<Vec<(RemoteBranchFile, Option<ConflictEntryPresence>)>> {
     let OperatingMode::Edit(metadata) = operating_mode(ctx) else {
         bail!("Starting index state can only be fetched while in edit mode")
     };
@@ -254,22 +265,55 @@ pub(crate) fn starting_index_state(
     let repository = ctx.repository();
 
     let commit = repository.find_commit(metadata.commit_oid)?;
-    let commit_parent = commit.parent(0)?;
+    let commit_parent_tree = if commit.is_conflicted() {
-    let commit_parent_tree = repository.find_real_tree(&commit_parent, Default::default())?;
+        repository.find_real_tree(&commit, ConflictedTreeKey::Ours)?
+    } else {
+        commit.parent(0)?.tree()?
+    };
 
     let index = get_commit_index(repository, &commit)?;
 
+    let conflicts = index
+        .conflicts()?
+        .filter_map(|conflict| {
+            let Ok(conflict) = conflict else {
+                return None;
+            };
+
+            let path = conflict
+                .ancestor
+                .as_ref()
+                .or(conflict.our.as_ref())
+                .or(conflict.their.as_ref())
+                .map(|entry| PathBuf::from(entry.path.to_str_lossy().to_string()))?;
+
+            Some((
+                path,
+                ConflictEntryPresence {
+                    ours: conflict.our.is_some(),
+                    theirs: conflict.their.is_some(),
+                    ancestor: conflict.ancestor.is_some(),
+                },
+            ))
+        })
+        .collect::<HashMap<PathBuf, ConflictEntryPresence>>();
+
+    dbg!(&conflicts);
+
     let diff = repository.diff_tree_to_index(Some(&commit_parent_tree), Some(&index), None)?;
 
     let diff_files = hunks_by_filepath(Some(repository), &diff)?
         .into_iter()
         .map(|(path, file)| {
             let binary = file.hunks.iter().any(|h| h.binary);
-            RemoteBranchFile {
+            (
-                path,
+                RemoteBranchFile {
-                hunks: file.hunks,
+                    path: path.clone(),
-                binary,
+                    hunks: file.hunks,
-            }
+                    binary,
+                },
+                conflicts.get(&path).cloned(),
+            )
         })
         .collect();
 

crates/gitbutler-tauri/src/modes.rs

@@ -1,5 +1,6 @@
 use anyhow::Context;
 use gitbutler_branch_actions::RemoteBranchFile;
+use gitbutler_edit_mode::ConflictEntryPresence;
 use gitbutler_operating_modes::EditModeMetadata;
 use gitbutler_operating_modes::OperatingMode;
 use gitbutler_project::Controller;
@@ -62,7 +63,7 @@ pub fn save_edit_and_return_to_workspace(
 pub fn edit_initial_index_state(
     projects: State<'_, Controller>,
     project_id: ProjectId,
-) -> Result<Vec<RemoteBranchFile>, Error> {
+) -> Result<Vec<(RemoteBranchFile, Option<ConflictEntryPresence>)>, Error> {
     let project = projects.get(project_id)?;
 
     gitbutler_edit_mode::commands::starting_index_state(&project).map_err(Into::into)

packages/ui/src/lib/file/FileListItem.svelte

@@ -20,6 +20,7 @@
 		checked?: boolean;
 		indeterminate?: boolean;
 		conflicted?: boolean;
+		conflictHint?: string;
 		locked?: boolean;
 		lockText?: string;
 		stacking?: boolean;
@@ -47,6 +48,7 @@
 		checked = $bindable(),
 		indeterminate,
 		conflicted,
+		conflictHint,
 		locked,
 		lockText,
 		stacking = false,
@@ -57,7 +59,7 @@
 		oncontextmenu
 	}: Props = $props();
 
-	const fileInfo = splitFilePath(filePath);
+	const fileInfo = $derived(splitFilePath(filePath));
 </script>
 
 <div
@@ -109,9 +111,11 @@
 		{/if}
 
 		{#if conflicted}
-			<div class="conflicted">
+			<Tooltip text={conflictHint}>
-				<Icon name="warning-small" color="error" />
+				<div class="conflicted">
-			</div>
+					<Icon name="warning-small" color="error" />
+				</div>
+			</Tooltip>
 		{/if}
 
 		{#if fileStatus}