fix: avoid duplicate checkout when returning from Tix travel

Reuse an available symbolic HEAD pin as the checkout target when returning to its branch, avoiding an intermediate detached checkout and its extra worktree traversal. Complete pending rebases using only the affected rewritten path so unrelated history cannot delay or block time travel.

Codex committed 2 months ago

Commit 32eb3eb1 parent c9875ff2 files · +149 −45Browse files

Unified · Split

2 files

gix-tix/spec.md +7 −4
gix-tix/src/edit/time_travel.rs +142 −41

gix-tix/spec.md+7−4

 - When tix detaches an attached local branch, it records that branch symbolically
   in the singleton `refs/worktree/tix/pins/HEAD` ref. Git stores this HEAD pin
   privately for the current worktree, and later branch advances move its tip.
-  Further detached travel preserves the singleton. After each successful tix
+  Further detached travel preserves the singleton. Landing on an available
-  checkout, landing on the remembered branch tip reattaches `HEAD` to that branch
+  remembered branch tip uses that branch as the checkout target directly,
-  and removes the HEAD pin; explicitly attaching another branch also removes it.
+  reattaches `HEAD` in one checkout, and removes the HEAD pin; explicitly
+  attaching another branch also removes it.
   Attach is the exception: it deliberately retains the symbolic HEAD pin
   after attaching its remembered branch.
   A failed automatic reattachment leaves both detached `HEAD` and the HEAD pin
   ancestry through that destination. Every later descendant is reparented and
   becomes or remains lazy and unsigned, including ordinary commits created above
   pending history; traveling toward a non-pending ancestor leaves the entire
-  pending region untouched. A conflict retains the ours tree, exact merge-result
+  pending region untouched. A completed final replay does not reload history;
+  another pass loads only the rewritten path and never unrelated references.
+  A conflict retains the ours tree, exact merge-result
   tree, conflict stages, prepared commits, and in-memory objects without changing
   the repository. The actual conflicting row is selected and centered with normal
   history-boundary clamping and shows a blinking red `C`; `<enter>` persists

gix-tix/src/edit/time_travel.rs+142−41

         return Ok((None, Vec::new()));
     }
     let mut ref_changes = Vec::new();
-    let destination_pin = selected_pin(&repository, selected)?;
+    let pins = history::all_pins(&repository)?;
-    let checkout_detaches = reference.is_none()
+    let destination_pin = selected_pin(&pins, selected);
-        && destination_pin
+    let direct_head_pin = if reference.is_none() {
-            .as_ref()
+        pins.into_iter().find(|pin| {
-            .is_none_or(|pin| pin.target.try_name().is_none());
+            pin.is_head()
+                && pin.id == selected
+                && pin.target.try_name().is_some_and(|branch| {
+                    branch.as_bstr().starts_with(b"refs/heads/")
+                        && ensure_branch_is_available(&repository, branch).is_ok()
+                })
+        })
+    } else {
+        None
+    };
+    let pin_for_checkout = direct_head_pin.as_ref().or(destination_pin.as_ref());
+    let checkout_detaches = reference.is_none() && pin_for_checkout.is_none_or(|pin| pin.target.try_name().is_none());
     let head_pin = match head_ref
         .as_ref()
         .filter(|name| checkout_detaches && name.as_bstr().starts_with(b"refs/heads/"))
         None => None,
     };
     let head_after_checkout = reference.map_or_else(
-        || match destination_pin.as_ref().map(|pin| &pin.target) {
+        || match pin_for_checkout.map(|pin| &pin.target) {
             Some(Target::Symbolic(name)) => super::undo::State::Symbolic(name.clone()),
             Some(Target::Object(id)) => super::undo::State::Object(*id),
             None => super::undo::State::Object(selected),
         |name| super::undo::State::Symbolic(name.clone()),
     );
     drop(repository);
-    let checkout = match (reference, &destination_pin) {
+    let checkout = match (reference, pin_for_checkout) {
         (Some(reference), _) if reference.as_bstr().starts_with(b"refs/heads/") => {
             checkout_branch(&workdir, reference.as_ref())
         }
             after: head_after_checkout,
         });
     }
-    let mut notice = reference.map_or_else(
+    let mut notice = match (reference, direct_head_pin.as_ref(), destination_pin.as_ref()) {
-        || {
+        (Some(reference), _, _) => format!("checked out {}", reference.shorten()),
-            destination_pin.as_ref().map_or_else(
+        (None, Some(pin), _) => format!(
-                || format!("time-travelled to {}", selected.to_hex_with_len(7)),
+            "returned to {}",
-                |pin| format!("returned from {}", pin_label(pin)),
+            pin.target
-            )
+                .try_name()
-        },
+                .expect("a direct HEAD return is symbolic")
-        |reference| format!("checked out {}", reference.shorten()),
+                .shorten()
-    );
+        ),
+        (None, None, Some(pin)) => format!("returned from {}", pin_label(pin)),
+        (None, None, None) => format!("time-travelled to {}", selected.to_hex_with_len(7)),
+    };
     let repository = match open_repository(repository_path, bare, false) {
         Ok(repository) => repository,
         Err(err) => {
     let remembered = remembered_branch(&repository)?;
     validate_attach(&repository, head_id, &remembered)?;
-    let destination_pin = selected_pin(&repository, head_id)?;
+    let pins = history::all_pins(&repository)?;
+    let destination_pin = selected_pin(&pins, head_id);
     let mut ref_changes = Vec::new();
     let provisional = if remembered.branch_tip != head_id && !contains(&repository, remembered.branch_tip, head_id) {
         let (pin, created, mut changes) = create_or_reuse_pin_reporting(
     {
         anyhow::bail!("cannot time-travel with unresolved index conflicts");
     }
+    let source_review = review_tree(&repository, graph, review_roots, head_id)?;
+    let destination_review = review_tree(&repository, graph, review_roots, selected)?;
+    let crosses_review_boundary =
+        source_review.as_ref().map(|review| review.root) != destination_review.as_ref().map(|review| review.root);
     let mut completed_graph = None;
     let mut original_ids = HashMap::new();
-    let mut review_roots = review_roots.to_vec();
     let mut ref_rewrites = Vec::new();
     let mut ref_changes = Vec::new();
-    while let Some(base) = pending_base(&repository, selected)? {
+    let mut pending = pending_base(&repository, selected)?;
+    while let Some(base) = pending {
+        let graph = completed_graph.as_ref().unwrap_or(graph);
         let mut rebased = Vec::new();
         let outcome = super::rebase::perform_reporting_rebased(
             &repository,
-            completed_graph.as_ref().unwrap_or(graph),
+            graph,
             super::rebase::Edit::Repeat {
                 base,
                 checkout: selected,
         };
         ref_rewrites.extend(outcome.ref_rewrites.iter().cloned());
         ref_changes.extend(outcome.ref_changes.iter().cloned());
-        for (old, original) in rebased {
+        for &(old, original) in &rebased {
             if let Some(new) = outcome.map(old) {
                 original_ids.insert(new, original);
             }
         head_id = outcome
             .map(head_id)
             .context("HEAD disappeared while completing its rebase")?;
-        review_roots = review_roots.into_iter().filter_map(|id| outcome.map(id)).collect();
         repository = open_repository(repository_path, bare, false)
             .context("could not reopen repository after completing a pending rebase")?;
-        completed_graph = Some(super::loaded_graph(&repository)?);
+        pending = pending_base(&repository, selected)?;
+        if pending.is_some() {
+            let affected = rebased
+                .into_iter()
+                .map(|(id, _original)| {
+                    outcome
+                        .map(id)
+                        .context("a pending rebase commit disappeared while completing time-travel")
+                })
+                .collect::<Result<Vec<_>>>()?;
+            completed_graph = Some(history::HistoryGraph::for_commits(&repository, &affected)?);
+        }
     }
-    let graph = completed_graph.as_ref().unwrap_or(graph);
-    let source_review = review_tree(&repository, graph, &review_roots, head_id)?;
-    let destination_review = review_tree(&repository, graph, &review_roots, selected)?;
-    let crosses_review_boundary =
-        source_review.as_ref().map(|review| review.root) != destination_review.as_ref().map(|review| review.root);
     let workdir = repository
         .workdir()
         .context("time-travel requires a worktree")?
     Ok(base)
 }
-fn selected_pin(repository: &gix::Repository, selected: ObjectId) -> Result<Option<history::Pin>> {
+fn selected_pin(pins: &[history::Pin], selected: ObjectId) -> Option<history::Pin> {
-    let mut pins: Vec<_> = history::all_pins(repository)?
+    pins.iter()
-        .into_iter()
         .filter(|pin| !pin.is_head() && pin.id == selected)
-        .collect();
+        .min_by(|a, b| {
-    pins.sort_by(|a, b| {
+            a.target
-        a.target
+                .try_name()
-            .try_name()
+                .is_none()
-            .is_none()
+                .cmp(&b.target.try_name().is_none())
-            .cmp(&b.target.try_name().is_none())
+                .then_with(|| a.name.cmp(&b.name))
-            .then_with(|| a.name.cmp(&b.name))
+        })
-    });
+        .cloned()
-    Ok(pins.into_iter().next())
 }
 #[cfg(test)]
             .set_target_id(topic, "advance pinned branch")?;
         let advanced = history::snapshot(&repository, &[], &[], false)?;
         assert!(advanced.view_tips.contains(&topic), "a symbolic pin follows its branch");
+        create_pin(&repository, Target::Object(topic), topic, "overlap with the HEAD pin")?;
+        assert_eq!(history::all_pins(&repository)?.len(), 2, "both return paths coexist");
         drop(repository);
+        let reflog_entries = git(fixture.path(), &["reflog", "show", "--format=%H", "HEAD"])?
+            .split(|byte| *byte == b'\n')
+            .filter(|line| !line.is_empty())
+            .count();
         let returned = perform(&repository_path, false, topic, &graph, &[], &[], false)?;
         let Perform::Complete { ref_changes, .. } = returned else {
             return Err("returning through the HEAD pin must complete".into());
         };
+        let returned_reflog_entries = git(fixture.path(), &["reflog", "show", "--format=%H", "HEAD"])?
+            .split(|byte| *byte == b'\n')
+            .filter(|line| !line.is_empty())
+            .count();
+        assert_eq!(
+            returned_reflog_entries,
+            reflog_entries + 1,
+            "returning through the HEAD pin performs one checkout"
+        );
         let repository = crate::test_repository::open(fixture.path())?;
         assert_eq!(
             repository.head_name()?.map(|name| name.as_bstr().to_owned()),
             .apply_with_worktrees(&repository)?;
         assert!(repository.head()?.is_detached(), "undo restores the detached HEAD");
         assert_eq!(repository.head_id()?.detach(), middle);
+        let pins = history::all_pins(&repository)?;
+        assert_eq!(pins.len(), 2, "undo restores both return paths");
         assert!(
-            history::all_pins(&repository)?.iter().any(history::Pin::is_head),
+            pins.iter().any(history::Pin::is_head),
             "undo restores the symbolic HEAD pin"
         );
         super::super::undo::plan_redo(&repository)?
         Ok(())
     }
+    #[test]
+    fn pending_time_travel_does_not_load_unrelated_ref_history() -> gix_testtools::Result {
+        let fixture = gix_testtools::scripted_fixture_writable("rebase_edit.sh")?;
+        let repository = crate::test_repository::open(fixture.path())?;
+        let repository_path = repository.git_dir().to_owned();
+        let graph = super::super::loaded_graph(&repository)?;
+        let middle = repository.rev_parse_single("HEAD~1")?.detach();
+        let old_tip = repository.head_id()?.detach();
+        let mut commit = repository.find_commit(middle)?.decode()?.into_owned()?;
+        commit.tree = repository
+            .find_commit(repository.rev_parse_single("HEAD~2")?)?
+            .tree_id()?
+            .detach();
+        let marked = super::super::rebase::perform(
+            &repository,
+            &graph,
+            super::super::rebase::Edit::Replace { target: middle, commit },
+            super::super::rebase::Signature::InvalidateExisting,
+            super::super::rebase::Tree::LeaveAsIsAndMark,
+        )?
+        .complete()?;
+        let pending_tip = marked.map(old_tip).context("the marked tip is retained")?;
+        assert!(super::super::rebase::is_pending(
+            &repository.find_commit(pending_tip)?.decode()?.into_owned()?
+        ));
+        let missing_parent = ObjectId::Sha1([0x42; 20]);
+        let mut unrelated = repository.find_commit(pending_tip)?.decode()?.into_owned()?;
+        unrelated.parents = [missing_parent].into_iter().collect();
+        unrelated.message = "unrelated incomplete history".into();
+        let unrelated = repository.write_object(&unrelated)?.detach();
+        drop(repository);
+        git(
+            fixture.path(),
+            &["update-ref", "refs/heads/unrelated", &unrelated.to_string()],
+        )?;
+        let repository = crate::test_repository::open(fixture.path())?;
+        let graph = loaded_graph(&repository, &[OsString::from("main")])?;
+        drop(repository);
+        let mut rebased = Vec::new();
+        let outcome = perform_reporting_rebased(&repository_path, false, pending_tip, &graph, &[], &[], false, |id| {
+            rebased.push(id);
+        })?;
+        let Perform::Complete { selected, .. } = outcome else {
+            return Err("the pending rebase must complete".into());
+        };
+        assert_eq!(rebased, [pending_tip], "only the selected pending path is replayed");
+        let repository = crate::test_repository::open(fixture.path())?;
+        assert!(!super::super::rebase::is_pending(
+            &repository.find_commit(selected)?.decode()?.into_owned()?
+        ));
+        assert_eq!(
+            repository.find_reference("refs/heads/unrelated")?.id().detach(),
+            unrelated,
+            "time-travel leaves the unrelated incomplete history untouched"
+        );
+        Ok(())
+    }
     #[test]
     fn time_travel_materializes_only_the_pending_path_to_the_destination() -> gix_testtools::Result {
         if !gix_testtools::signature::program_available("ssh-keygen") {

fix: avoid duplicate checkout when returning from Tix travel

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code