diff --git a/packages/app/modules/paseo-diff-prototype/.gitignore b/packages/app/modules/paseo-diff-prototype/.gitignore new file mode 100644 index 0000000000..d80969eb27 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/.gitignore @@ -0,0 +1,2 @@ +/android/.cxx/ +/android/build/ diff --git a/packages/app/modules/paseo-diff-prototype/README.md b/packages/app/modules/paseo-diff-prototype/README.md new file mode 100644 index 0000000000..f872033889 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/README.md @@ -0,0 +1,35 @@ +# Native diff layout experiment + +Android-only Expo module for evaluating background wrapped-text layout. The normal +diff surface does not call this module; it still uses the JavaScript model and +renderer. This is a checkpoint of the layout experiment, not a replacement renderer. + +The C++ core owns cell text and wrapped grapheme geometry. Kotlin/JNI supplies the +Android font manager and dispatches preparation through Expo's background queue. +It links the Skia libraries shipped by the installed React Native Skia package. +An iOS binding, renderer integration, and shipping library ownership are unfinished. + +## Diagnostic interface + +Access the module through `expo.modules.PaseoDiffPrototype` in a development build: + +- `create()` returns a numeric handle. +- `prepare(handle, texts, widths, family, fontSize)` asynchronously builds geometry + and returns counts, copy/layout timings, and a partial geometry allocation estimate. + Text and width arrays must have matching lengths. Input is copied; it is not zero-copy. +- `read(handle, startCell, count)` synchronously returns packed numerical geometry + for at most 256 cells. This exists for parity checks, not whole-model transfer. +- `release(handle)` cancels pending work and releases ownership. Release handles + after success or failure; module destruction also cancels and releases all handles. + +Each packed cell begins with its index and fragment count. Each fragment contains +UTF-16 start/end offsets, width, and grapheme count, followed by each grapheme's +UTF-16 start/end offsets and width. The caller retains source text. + +The experiment accepts normalized cell inputs. It does not parse wire payloads, +construct file/header/review models, apply highlighting, or draw pixels. A renderer +integration must preserve native ownership and request bounded visible data rather +than reconstructing the entire JavaScript graph. + +Grapheme property ranges are generated at build time from the installed JavaScript +dependency so segmentation uses the same data as the reference implementation. diff --git a/packages/app/modules/paseo-diff-prototype/android/CMakeLists.txt b/packages/app/modules/paseo-diff-prototype/android/CMakeLists.txt new file mode 100644 index 0000000000..1230d875a2 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/android/CMakeLists.txt @@ -0,0 +1,15 @@ +cmake_minimum_required(VERSION 3.22) +project(paseo_diff_prototype) +set(CMAKE_CXX_STANDARD 20) +execute_process(COMMAND node "${CMAKE_CURRENT_SOURCE_DIR}/../cpp/generate-graphemes.mjs" "${CMAKE_CURRENT_BINARY_DIR}/graphemes.h" COMMAND_ERROR_IS_FATAL ANY) + +add_library(paseo_diff_prototype SHARED ../cpp/layout.cpp ../cpp/jni.cpp) +target_include_directories(paseo_diff_prototype PRIVATE "${SKIA_PACKAGE}/cpp/skia" "${CMAKE_CURRENT_BINARY_DIR}") +target_compile_definitions(paseo_diff_prototype PRIVATE SK_BUILD_FOR_ANDROID) +target_compile_options(paseo_diff_prototype PRIVATE -O2 -Wall -Wextra) +target_link_options(paseo_diff_prototype PRIVATE "-Wl,-z,max-page-size=16384") +foreach(lib skparagraph skshaper skunicode_core skunicode_icu skia) + add_library(${lib} STATIC IMPORTED) + set_target_properties(${lib} PROPERTIES IMPORTED_LOCATION "${SKIA_PACKAGE}/libs/android/${ANDROID_ABI}/lib${lib}.a") +endforeach() +target_link_libraries(paseo_diff_prototype PRIVATE skparagraph skshaper skunicode_core skunicode_icu skia android log EGL GLESv2) diff --git a/packages/app/modules/paseo-diff-prototype/android/build.gradle b/packages/app/modules/paseo-diff-prototype/android/build.gradle new file mode 100644 index 0000000000..f218c03ef8 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/android/build.gradle @@ -0,0 +1,27 @@ +apply plugin: 'com.android.library' + +group = 'sh.paseo.diffprototype' +version = '0.1.0' + +def expoModulesCorePlugin = new File(project(":expo-modules-core").projectDir.absolutePath, "ExpoModulesCorePlugin.gradle") +apply from: expoModulesCorePlugin +applyKotlinExpoModulesCorePlugin() +useCoreDependencies() +useDefaultAndroidSdkVersions() + +def skiaPackage = new File(["node", "--print", "require.resolve('@shopify/react-native-skia/package.json')"].execute(null, projectDir).text.trim()).parentFile + +android { + namespace "sh.paseo.diffprototype" + defaultConfig { + externalNativeBuild { + cmake { + arguments "-DSKIA_PACKAGE=${skiaPackage.absolutePath}" + cppFlags "-std=c++20 -O2 -fexceptions -frtti" + } + } + } + externalNativeBuild { + cmake { path "CMakeLists.txt" } + } +} diff --git a/packages/app/modules/paseo-diff-prototype/android/src/main/AndroidManifest.xml b/packages/app/modules/paseo-diff-prototype/android/src/main/AndroidManifest.xml new file mode 100644 index 0000000000..94cbbcfc39 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/android/src/main/AndroidManifest.xml @@ -0,0 +1 @@ + diff --git a/packages/app/modules/paseo-diff-prototype/android/src/main/java/sh/paseo/diffprototype/PaseoDiffPrototypeModule.kt b/packages/app/modules/paseo-diff-prototype/android/src/main/java/sh/paseo/diffprototype/PaseoDiffPrototypeModule.kt new file mode 100644 index 0000000000..d542f0b17b --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/android/src/main/java/sh/paseo/diffprototype/PaseoDiffPrototypeModule.kt @@ -0,0 +1,34 @@ +package sh.paseo.diffprototype + +import expo.modules.kotlin.modules.Module +import expo.modules.kotlin.modules.ModuleDefinition + +// Experimental binding only. The layout engine has no JVM or platform dependency. +class PaseoDiffPrototypeModule : Module() { + companion object { + init { System.loadLibrary("paseo_diff_prototype") } + } + + private external fun createDocument(): Double + private external fun prepareDocument(id: Double, texts: Array, widths: DoubleArray, family: String, size: Double): DoubleArray + private external fun readDocument(id: Double, start: Int, count: Int): DoubleArray + private external fun releaseDocument(id: Double) + private external fun releaseAllDocuments() + + override fun definition() = ModuleDefinition { + Name("PaseoDiffPrototype") + + Function("create") { createDocument() } + // Expo dispatches this non-suspending AsyncFunction on its background queue. + AsyncFunction("prepare") { id: Double, texts: List, widths: List, family: String, size: Double -> + val stats = prepareDocument(id, texts.toTypedArray(), widths.toDoubleArray(), family, size) + mapOf( + "cells" to stats[0], "fragments" to stats[1], "graphemes" to stats[2], + "copyMs" to stats[3], "layoutMs" to stats[4], "geometryBytes" to stats[5] + ) + } + Function("read") { id: Double, start: Int, count: Int -> readDocument(id, start, count) } + Function("release") { id: Double -> releaseDocument(id) } + OnDestroy { releaseAllDocuments() } + } +} diff --git a/packages/app/modules/paseo-diff-prototype/cpp/generate-graphemes.mjs b/packages/app/modules/paseo-diff-prototype/cpp/generate-graphemes.mjs new file mode 100644 index 0000000000..70422df3ce --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/cpp/generate-graphemes.mjs @@ -0,0 +1,18 @@ +import { writeFileSync } from "node:fs"; +import { isExtendingChar } from "@marijn/find-cluster-break"; + +// Use the installed JS engine's Unicode property data, not a different ICU version. +const ranges = []; +let start = null; +for (let code = 0; code <= 0x110000; code++) { + const extending = code < 0x110000 && isExtendingChar(code); + if (extending && start === null) start = code; + if (!extending && start !== null) { + ranges.push(`{${start},${code}}`); + start = null; + } +} +writeFileSync( + process.argv[2], + `#pragma once\n#include \n#include \ninline constexpr std::array,${ranges.length}> extendingRanges = {{${ranges.join(",")}}};\n`, +); diff --git a/packages/app/modules/paseo-diff-prototype/cpp/jni.cpp b/packages/app/modules/paseo-diff-prototype/cpp/jni.cpp new file mode 100644 index 0000000000..a8f25fac54 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/cpp/jni.cpp @@ -0,0 +1,136 @@ +#include "layout.h" +#include +#include +#include +#include +#include +#include +#include +#include "include/ports/SkFontMgr_android.h" +#include "modules/skunicode/include/SkUnicode.h" + +namespace { +using Clock = std::chrono::steady_clock; +using paseo::diff::Document; +struct Job { + std::atomic cancelled{false}; + std::atomic started{false}; + std::shared_ptr document; +}; +std::mutex mutex; +std::unordered_map> jobs; +uint64_t nextId = 0; + +double elapsed(Clock::time_point start) { + return std::chrono::duration(Clock::now() - start).count(); +} +std::shared_ptr findJob(double id) { + std::lock_guard lock(mutex); + const auto found = jobs.find(static_cast(id)); + if (found == jobs.end()) throw std::runtime_error("Unknown or released layout"); + return found->second; +} +std::u16string copyString(JNIEnv* env, jstring source) { + if (!source) throw std::runtime_error("Missing text"); + const jsize length = env->GetStringLength(source); + const jchar* chars = env->GetStringChars(source, nullptr); + if (!chars) throw std::runtime_error("Could not copy text"); + std::u16string result(reinterpret_cast(chars), length); + env->ReleaseStringChars(source, chars); + return result; +} +jdoubleArray numbers(JNIEnv* env, const std::vector& values) { + auto result = env->NewDoubleArray(values.size()); + if (result) env->SetDoubleArrayRegion(result, 0, values.size(), values.data()); + return result; +} +void fail(JNIEnv* env, const std::exception& error) { + env->ThrowNew(env->FindClass("java/lang/IllegalStateException"), error.what()); +} +} // namespace + +#define METHOD(name) Java_sh_paseo_diffprototype_PaseoDiffPrototypeModule_##name + +extern "C" JNIEXPORT jdouble JNICALL METHOD(createDocument)(JNIEnv*, jobject) { + std::lock_guard lock(mutex); + const auto id = ++nextId; + jobs.emplace(id, std::make_shared()); + return id; +} + +extern "C" JNIEXPORT jdoubleArray JNICALL METHOD(prepareDocument)( + JNIEnv* env, jobject, jdouble id, jobjectArray texts, jdoubleArray widths, + jstring family, jdouble size) { + try { + auto job = findJob(id); + if (job->started.exchange(true)) throw std::runtime_error("Layout already started"); + const auto copyStart = Clock::now(); + const jsize count = env->GetArrayLength(texts); + if (count > 100000 || env->GetArrayLength(widths) != count || !std::isfinite(size) || size <= 0 || size > 100) + throw std::runtime_error("Invalid layout input"); + std::vector available(count); + env->GetDoubleArrayRegion(widths, 0, count, available.data()); + auto document = std::make_shared(); + document->cells.reserve(count); + size_t units = 0; + for (jsize i = 0; i < count; i++) { + if (job->cancelled.load()) throw std::runtime_error("Layout cancelled"); + if (!std::isfinite(available[i]) || available[i] <= 0) throw std::runtime_error("Invalid width"); + auto text = static_cast(env->GetObjectArrayElement(texts, i)); + auto copied = copyString(env, text); + env->DeleteLocalRef(text); + units += copied.size(); + if (units > 16000000) throw std::runtime_error("Prototype input limit exceeded"); + document->cells.push_back({std::move(copied), available[i], {}}); + } + const auto fontFamily = SkUnicode::convertUtf16ToUtf8(copyString(env, family)); + const double copyMs = elapsed(copyStart); + const auto layoutStart = Clock::now(); + paseo::diff::prepare(*document, SkFontMgr_New_Android(nullptr), fontFamily.c_str(), size, job->cancelled); + const double layoutMs = elapsed(layoutStart); + { + std::lock_guard lock(mutex); + if (job->cancelled.load()) throw std::runtime_error("Layout cancelled"); + job->document = document; + } + return numbers(env, {static_cast(document->cells.size()), + static_cast(document->fragmentCount), static_cast(document->graphemeCount), + copyMs, layoutMs, static_cast(document->geometryBytes)}); + } catch (const std::exception& error) { + fail(env, error); + return nullptr; + } +} + +extern "C" JNIEXPORT jdoubleArray JNICALL METHOD(readDocument)( + JNIEnv* env, jobject, jdouble id, jint start, jint count) { + try { + const auto job = findJob(id); + std::shared_ptr document; + { + std::lock_guard lock(mutex); + document = job->document; + } + if (!document) throw std::runtime_error("Layout is not ready"); + if (start < 0 || count < 0 || count > 256 || static_cast(start) > document->cells.size()) + throw std::runtime_error("Invalid viewport range"); + return numbers(env, paseo::diff::read(*document, start, count)); + } catch (const std::exception& error) { + fail(env, error); + return nullptr; + } +} + +extern "C" JNIEXPORT void JNICALL METHOD(releaseDocument)(JNIEnv*, jobject, jdouble id) { + std::lock_guard lock(mutex); + const auto found = jobs.find(static_cast(id)); + if (found == jobs.end()) return; + found->second->cancelled.store(true); + jobs.erase(found); +} + +extern "C" JNIEXPORT void JNICALL METHOD(releaseAllDocuments)(JNIEnv*, jobject) { + std::lock_guard lock(mutex); + for (auto& [id, job] : jobs) job->cancelled.store(true); + jobs.clear(); +} diff --git a/packages/app/modules/paseo-diff-prototype/cpp/layout.cpp b/packages/app/modules/paseo-diff-prototype/cpp/layout.cpp new file mode 100644 index 0000000000..e5f7d0f8a8 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/cpp/layout.cpp @@ -0,0 +1,277 @@ +#include "layout.h" +#include "graphemes.h" + +#include +#include +#include +#include +#include +#include + +#include "include/core/SkFont.h" +#include "modules/skparagraph/include/FontCollection.h" +#include "modules/skparagraph/include/Paragraph.h" +#include "modules/skparagraph/include/ParagraphBuilder.h" +#include "modules/skunicode/include/SkUnicode.h" + +namespace paseo::diff { +namespace { +namespace para = skia::textlayout; + +int codePoint(const std::u16string& text, size_t pos) { + int code = text[pos]; + if (code >= 0xd800 && code < 0xdc00 && pos + 1 < text.size()) { + int low = text[pos + 1]; + if (low >= 0xdc00 && low < 0xe000) return ((code - 0xd800) << 10) + low - 0xdc00 + 0x10000; + } + return code; +} +bool extending(int code) { + const auto found = std::upper_bound(extendingRanges.begin(), extendingRanges.end(), code, + [](int value, const auto& range) { return value < range.first; }); + return found != extendingRanges.begin() && code < std::prev(found)->second; +} +bool regional(int code) { return code >= 0x1f1e6 && code <= 0x1f1ff; } + +// Same forward-cluster rules as @marijn/find-cluster-break, including ZWJ/RI behavior. +std::vector segment(const std::u16string& text) { + std::vector result; + size_t pos = 0; + size_t column = 0; + while (pos < text.size()) { + const auto start = pos; + int previous = codePoint(text, pos); + pos += previous < 0x10000 ? 1 : 2; + while (pos < text.size()) { + const int next = codePoint(text, pos); + if (previous == 0x200d || next == 0x200d || extending(next)) { + pos += next < 0x10000 ? 1 : 2; + previous = next; + } else if (regional(next)) { + size_t count = 0; + for (int index = static_cast(pos) - 2; index >= 0 && regional(codePoint(text, index)); index -= 2) count++; + if (count % 2 == 0) break; + pos += 2; + } else { + break; + } + } + auto display = text.substr(start, pos - start); + const bool tab = display == u"\t"; + const size_t spaces = tab ? 4 - column % 4 : 0; + if (tab) display.assign(spaces, u' '); + result.push_back({static_cast(start), static_cast(pos), std::move(display)}); + column += tab ? spaces : 1; + } + return result; +} + +bool needsShaping(const std::u16string& text) { + for (const auto c : text) if ((c < 0x20 || c > 0x7e) && c != u'\t') return true; + for (const auto* pair : {u"==", u"!=", u"=>", u"<=", u">=", u"->", u"::", u"++", u"--"}) { + if (text.find(pair) != std::u16string::npos) return true; + } + return false; +} +bool cleanBoundary(const Grapheme& grapheme) { + for (const auto c : grapheme.text) { + if (c < 0x20 || c > 0x7e || std::u16string_view(u"=!<>+-:&|*/~^?.").find(c) != std::u16string_view::npos) return false; + } + return true; +} +size_t chunkEnd(const std::vector& graphemes, size_t start, size_t end) { + const size_t limit = start + 64; + if (limit >= end) return end; + const size_t floor = std::max(start + 1, limit - 16); + for (size_t seam = limit; seam > floor; seam--) { + if (cleanBoundary(graphemes[seam - 1]) && cleanBoundary(graphemes[seam])) return seam; + } + return limit; +} + +class Measurer { + public: + Measurer(sk_sp manager, const std::string& family, float size, + const std::atomic& cancelled) : cancelled_(cancelled) { + families_ = {SkString(family), SkString("Menlo"), SkString("SF Mono"), SkString("monospace"), SkString("System")}; + std::unordered_set available; + for (int i = 0; i < manager->countFamilies(); i++) { + SkString name; + manager->getFamilyName(i, &name); + available.insert(name.c_str()); + } + std::string primary = "System"; + for (const auto& name : families_) { + if (available.contains(name.c_str())) { primary = name.c_str(); break; } + } + font_ = SkFont(manager->matchFamilyStyle(primary.c_str(), SkFontStyle()), size); + fonts_ = sk_make_sp(); + fonts_->setDefaultFontManager(manager); + fonts_->enableFontFallback(); + para::StrutStyle strut; + strut.setStrutEnabled(true); + strut.setForceStrutHeight(true); + strut.setFontFamilies(families_); + strut.setFontSize(size); + strut.setHeight(std::round(size * 1.5) / size); + strut.setHeightOverride(true); + strut.setHalfLeading(true); + style_.setMaxLines(1); + style_.setStrutStyle(strut); + textStyle_.setFontFamilies(families_); + textStyle_.setFontSize(size); + textStyle_.setColor(SK_ColorBLACK); + } + + double measure(const std::vector& graphemes, size_t start, size_t end, + std::vector* advances = nullptr) { + double base = 0; + for (size_t chunkStart = start; chunkStart < end;) { + if (cancelled_.load()) throw std::runtime_error("Layout cancelled"); + const size_t chunkLimit = chunkEnd(graphemes, chunkStart, end); + std::u16string text; + for (size_t i = chunkStart; i < chunkLimit; i++) text += graphemes[i].text; + double width = 0; + if (!needsShaping(text) && hasEveryGlyph(text)) { + for (size_t i = chunkStart; i < chunkLimit; i++) { + width += additiveWidth(graphemes[i].text); + if (advances) advances->push_back(base + width); + } + } else { + auto paragraph = shape(text); + if (advances) { + size_t offset = 0; + for (size_t i = chunkStart; i < chunkLimit; i++) { + offset += graphemes[i].text.size(); + width = paragraphWidth(*paragraph, offset); + advances->push_back(base + width); + } + } else { + width = paragraphWidth(*paragraph, text.size()); + } + } + base += width; + chunkStart = chunkLimit; + } + return base; + } + + private: + bool hasEveryGlyph(const std::u16string& text) { + for (const auto c : text) { + auto [it, inserted] = glyphs_.try_emplace(c, false); + if (inserted) it->second = font_.unicharToGlyph(c) != 0; + if (!it->second) return false; + } + return true; + } + double additiveWidth(const std::u16string& text) { + auto [it, inserted] = widths_.try_emplace(text, 0); + if (inserted) { + std::vector glyphs(font_.countText(text.data(), text.size() * 2, SkTextEncoding::kUTF16)); + font_.textToGlyphs(text.data(), text.size() * 2, SkTextEncoding::kUTF16, SkSpan(glyphs)); + std::vector widths(glyphs.size()); + font_.getWidthsBounds(SkSpan(glyphs), SkSpan(widths), {}, nullptr); + // Match RNSkia 2.2.12 getTextWidth's integer accumulator exactly. + it->second = std::accumulate(widths.begin(), widths.end(), 0); + } + return it->second; + } + std::unique_ptr shape(const std::u16string& text) { + auto builder = para::ParagraphBuilder::make(style_, fonts_); + builder->pushStyle(textStyle_); + const auto utf8 = SkUnicode::convertUtf16ToUtf8(text); + builder->addText(utf8.c_str(), utf8.size()); + builder->pop(); + auto paragraph = builder->Build(); + paragraph->layout(100000); + return paragraph; + } + static double paragraphWidth(para::Paragraph& paragraph, size_t end) { + double width = 0; + for (const auto& box : paragraph.getRectsForRange(0, end, para::RectHeightStyle::kTight, para::RectWidthStyle::kTight)) { + // JS adds the two float-valued properties in double precision. + width = std::max(width, static_cast(box.rect.x()) + box.rect.width()); + } + return width; + } + SkFont font_; + std::vector families_; + sk_sp fonts_; + para::ParagraphStyle style_; + para::TextStyle textStyle_; + std::unordered_map glyphs_; + std::unordered_map widths_; + const std::atomic& cancelled_; +}; +} // namespace + +void prepare(Document& document, sk_sp fonts, const std::string& family, float size, + const std::atomic& cancelled) { + Measurer measure(std::move(fonts), family, size, cancelled); + for (auto& cell : document.cells) { + if (cancelled.load()) throw std::runtime_error("Layout cancelled"); + auto graphemes = segment(cell.text); + const bool bounded = !needsShaping(cell.text); + size_t start = 0; + do { + size_t fitting = graphemes.size(); + if (!graphemes.empty()) { + size_t low = start + 1; + size_t high = bounded ? std::min(graphemes.size(), start + 64) : graphemes.size(); + fitting = low; + if (bounded) { + while (measure.measure(graphemes, start, high) <= cell.availableWidth) { + fitting = high; + low = high + 1; + if (high == graphemes.size()) break; + high = std::min(graphemes.size(), start + (high - start) * 2); + } + } + while (low <= high) { + const size_t middle = (low + high) / 2; + const double width = measure.measure(graphemes, start, middle); + if (width <= cell.availableWidth || middle == start + 1) { + fitting = middle; + low = middle + 1; + } else { + high = middle - 1; + } + } + } + std::vector advances; + advances.reserve(fitting - start); + const double width = measure.measure(graphemes, start, fitting, &advances); + Fragment fragment{start < graphemes.size() ? graphemes[start].start : 0, + fitting > start ? graphemes[fitting - 1].end : 0, width, {}}; + fragment.graphemes.reserve(fitting - start); + for (size_t i = start; i < fitting; i++) { + auto grapheme = graphemes[i]; + grapheme.width = advances[i - start] - (i > start ? advances[i - start - 1] : 0); + fragment.graphemes.push_back(std::move(grapheme)); + } + document.fragmentCount++; + document.graphemeCount += fragment.graphemes.size(); + document.geometryBytes += sizeof(Fragment) + fragment.graphemes.capacity() * sizeof(Grapheme); + cell.fragments.push_back(std::move(fragment)); + start = fitting; + } while (start < graphemes.size()); + } +} + +std::vector read(const Document& document, size_t start, size_t count) { + std::vector output; + const size_t end = start + std::min(count, document.cells.size() - start); + for (size_t i = start; i < end; i++) { + const auto& cell = document.cells[i]; + output.insert(output.end(), {static_cast(i), static_cast(cell.fragments.size())}); + for (const auto& fragment : cell.fragments) { + output.insert(output.end(), {static_cast(fragment.start), static_cast(fragment.end), fragment.width, static_cast(fragment.graphemes.size())}); + for (const auto& grapheme : fragment.graphemes) { + output.insert(output.end(), {static_cast(grapheme.start), static_cast(grapheme.end), grapheme.width}); + } + } + } + return output; +} +} // namespace paseo::diff diff --git a/packages/app/modules/paseo-diff-prototype/cpp/layout.h b/packages/app/modules/paseo-diff-prototype/cpp/layout.h new file mode 100644 index 0000000000..7de6f00fe2 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/cpp/layout.h @@ -0,0 +1,37 @@ +#pragma once + +#include +#include +#include +#include +#include "include/core/SkFontMgr.h" + +namespace paseo::diff { +struct Grapheme { + int start; + int end; + std::u16string text; + double width = 0; +}; +struct Fragment { + int start; + int end; + double width; + std::vector graphemes; +}; +struct Cell { + std::u16string text; + double availableWidth; + std::vector fragments; +}; +struct Document { + std::vector cells; + size_t fragmentCount = 0; + size_t graphemeCount = 0; + size_t geometryBytes = 0; +}; + +void prepare(Document& document, sk_sp fonts, const std::string& family, float fontSize, + const std::atomic& cancelled); +std::vector read(const Document& document, size_t start, size_t count); +} // namespace paseo::diff diff --git a/packages/app/modules/paseo-diff-prototype/expo-module.config.json b/packages/app/modules/paseo-diff-prototype/expo-module.config.json new file mode 100644 index 0000000000..2a4587d5f8 --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/expo-module.config.json @@ -0,0 +1,6 @@ +{ + "platforms": ["android"], + "android": { + "modules": ["sh.paseo.diffprototype.PaseoDiffPrototypeModule"] + } +} diff --git a/packages/app/modules/paseo-diff-prototype/package.json b/packages/app/modules/paseo-diff-prototype/package.json new file mode 100644 index 0000000000..9c775282ed --- /dev/null +++ b/packages/app/modules/paseo-diff-prototype/package.json @@ -0,0 +1,5 @@ +{ + "name": "paseo-diff-prototype", + "version": "0.1.0", + "private": true +} diff --git a/packages/app/src/git/diff-document/measurement-cost.test.ts b/packages/app/src/git/diff-document/measurement-cost.test.ts index ef2f6231df..b870fbe9cb 100644 --- a/packages/app/src/git/diff-document/measurement-cost.test.ts +++ b/packages/app/src/git/diff-document/measurement-cost.test.ts @@ -1,6 +1,6 @@ import type { ParsedDiffFile } from "@getpaseo/protocol/messages"; import { describe, expect, it } from "vitest"; -import { buildDiffDocumentModel } from "./model"; +import { buildDiffDocumentModel, measureFragments } from "./model"; import type { BuildDiffDocumentModelInput, TextMeasurer } from "./types"; /** @@ -92,6 +92,33 @@ function shapedCharactersFor(options: { lineCount: number; lineLength: number }) } describe("diff document measurement cost", () => { + it("keeps additive wrapped search work proportional to long source lines", () => { + function measuredGraphemes(length: number): number { + let count = 0; + measureFragments({ + text: "a".repeat(length), + availableWidth: 200, + wrapLines: true, + lineHeight: 18, + measureText: { + measure: (text) => text.length * 10, + measureWidth(graphemes) { + count += graphemes.length; + return graphemes.length * 10; + }, + measureAdvances(graphemes) { + count += graphemes.length; + return graphemes.map((_, index) => (index + 1) * 10); + }, + }, + }); + return count; + } + const short = measuredGraphemes(100); + const long = measuredGraphemes(1600); + expect(long / short).toBeLessThan(32); + }); + it("shapes a bounded number of characters per character of text", () => { // The shape of the CHANGELOG.md working diff that froze the renderer: 42 // lines, longest 597 characters. diff --git a/packages/app/src/git/diff-document/model.test.ts b/packages/app/src/git/diff-document/model.test.ts index 1aa78ab3c6..c2b6d86c9c 100644 --- a/packages/app/src/git/diff-document/model.test.ts +++ b/packages/app/src/git/diff-document/model.test.ts @@ -71,6 +71,78 @@ function input(overrides: Partial = {}): BuildDiffD } describe("diff document model", () => { + it("preserves shaped wrap breaks when joining makes a longer prefix narrower", () => { + const measure = (text: string) => { + if (text === "لا") return 30; + if (text === "لام") return 15; + return text.length * 10; + }; + const fragments = measureFragments({ + text: "لام", + availableWidth: 20, + wrapLines: true, + lineHeight: 18, + measureText: { measure, measureWidth: (graphemes) => measure(graphemes.join("")) }, + }); + expect(fragments.map((fragment) => fragment.text)).toEqual(["ل", "ام"]); + expect(fragments.map((fragment) => fragment.width)).toEqual([10, 20]); + }); + + it("does not search wrap boundaries when the remaining text already fits", () => { + let widthQueries = 0; + const fragments = measureFragments({ + text: "const answer = value + 1;", + availableWidth: 300, + wrapLines: true, + lineHeight: 18, + measureText: { + measure: measurer.measure, + measureWidth(graphemes) { + widthQueries++; + return graphemes.length * 10; + }, + }, + }); + expect(fragments).toHaveLength(1); + expect(fragments[0]?.width).toBe(250); + expect(widthQueries).toBe(1); + }); + + it("builds character geometry only for displayed wrapped fragments", () => { + let advancedGraphemes = 0; + const measureWidth = (graphemes: readonly string[]) => graphemes.length * 10; + const measureText = { + // Whole-string widths need not match the chunked advance contract. + measure: () => 999, + measureWidth, + measureAdvances(graphemes: readonly string[]) { + advancedGraphemes += graphemes.length; + return graphemes.map((_, index) => (index + 1) * 10); + }, + }; + const text = "const next = (value) => value + 1;".repeat(4); + const fragments = measureFragments({ + text, + availableWidth: 80, + wrapLines: true, + lineHeight: 18, + measureText, + }); + + expect(fragments.map((fragment) => fragment.text).join("")).toBe(text); + expect(fragments.every((fragment) => fragment.width <= 80)).toBe(true); + expect(fragments).toEqual( + measureFragments({ + text, + availableWidth: 80, + wrapLines: true, + lineHeight: 18, + measureText: { measure: measurer.measure }, + }), + ); + expect(advancedGraphemes).toBe(text.length); + }); + it("materializes only visible rows of a large file while preserving full horizontal extent", () => { const source = file("large.ts"); source.hunks[0]!.lines = Array.from({ length: 1000 }, (_, index) => ({ diff --git a/packages/app/src/git/diff-document/model.ts b/packages/app/src/git/diff-document/model.ts index a8b2baed90..fb2b9a04f0 100644 --- a/packages/app/src/git/diff-document/model.ts +++ b/packages/app/src/git/diff-document/model.ts @@ -8,7 +8,7 @@ import { } from "@/utils/diff-layout"; import { compactHighlightTokens } from "@/utils/diff-rendering"; import { getInlineReviewThreadState, getSplitInlineReviewThreadState } from "@/review/geometry"; -import { advancesFor } from "./text-measurement"; +import { advancesFor, requiresShaping } from "./text-measurement"; import type { BuildDiffDocumentModelInput, DiffCell, @@ -571,14 +571,31 @@ export function measureFragments(input: { return [createFragment(graphemes, 0, graphemes.length, 0, input.lineHeight, input.measureText)]; } const fragments: DiffFragment[] = []; + const displayGraphemes = graphemes.map((grapheme) => grapheme.text); + const measureWidth = + input.measureText.measureWidth?.bind(input.measureText) ?? + ((slice: readonly string[]) => advancesFor(input.measureText)(slice).at(-1) ?? 0); + // Joining/ligatures can make a longer prefix narrower. Preserve the existing + // search order for those runs; changing the probes can change their breaks. + const canBoundSearch = !requiresShaping(input.text); let graphemeIndex = 0; while (graphemeIndex < graphemes.length) { let low = graphemeIndex + 1; - let high = graphemes.length; + // A small first probe finishes short lines in one query. Grow it only when + // it fits, so narrow wrapping never repeatedly measures a huge line's tail. + let high = canBoundSearch ? Math.min(graphemes.length, graphemeIndex + 64) : graphemes.length; let fitting = low; + if (canBoundSearch) { + while (measureWidth(displayGraphemes.slice(graphemeIndex, high)) <= input.availableWidth) { + fitting = high; + low = high + 1; + if (high === graphemes.length) break; + high = Math.min(graphemes.length, graphemeIndex + (high - graphemeIndex) * 2); + } + } while (low <= high) { const middle = (low + high) >>> 1; - const width = measureGraphemeSlice(graphemes, graphemeIndex, middle, input.measureText).width; + const width = measureWidth(displayGraphemes.slice(graphemeIndex, middle)); if (width <= input.availableWidth || middle === graphemeIndex + 1) { fitting = middle; low = middle + 1; diff --git a/packages/app/src/git/diff-document/text-measurement.test.ts b/packages/app/src/git/diff-document/text-measurement.test.ts index 144676e550..414054e76c 100644 --- a/packages/app/src/git/diff-document/text-measurement.test.ts +++ b/packages/app/src/git/diff-document/text-measurement.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { createCachedAsciiTextMetrics, + createChunkedWidthMeasurer, createFallbackAwareTextMeasurer, createMeasuredAdvances, requiresShaping, @@ -56,6 +57,7 @@ describe("cached ASCII text metrics", () => { expect(metrics.measureAdvances(["a", "b", "a"])).toEqual([10, 20, 30]); expect(metrics.measureAdvances(["b", "a"])).toEqual([10, 20]); expect(metrics.measure("abba")).toBe(40); + expect(metrics.measureWidth(["a", "b", "b", "a"])).toBe(40); expect(measured).toEqual(["a", "b"]); }); @@ -105,6 +107,27 @@ describe("chunked advance measurement", () => { return Array.from(text, (_, index) => shapingMeasure(text.slice(0, index + 1))); } + it.each([ + "", + "a => b ", + `${"a".repeat(63)}=>${"b".repeat(70)}`, + "=>".repeat(100), + "中文 é 👩‍💻 مرحبا".repeat(10), + ])("keeps width-only measurement equal to the last shaped advance: %s", (text) => { + const graphemes = Array.from(text); + const width = createChunkedWidthMeasurer((chunk) => shapingMeasure(chunk.join(""))); + expect(width(graphemes)).toBe(createMeasuredAdvances(shapingMeasure)(graphemes).at(-1) ?? 0); + }); + + it("keeps expanded tabs on the same additive width path", () => { + const metrics = createCachedAsciiTextMetrics({ + glyphIds: (text) => Array.from(text, () => 1), + measure: (text) => text.length * 3.7, + }); + const graphemes = ["a", " ", "b", " "]; + expect(metrics.measureWidth(graphemes)).toBe(metrics.measureAdvances(graphemes).at(-1)); + }); + it("measures short runs in one shaping context", () => { expect(advancesOf("a => b")).toEqual(expected("a => b")); }); diff --git a/packages/app/src/git/diff-document/text-measurement.ts b/packages/app/src/git/diff-document/text-measurement.ts index a141d170a7..905a8915d1 100644 --- a/packages/app/src/git/diff-document/text-measurement.ts +++ b/packages/app/src/git/diff-document/text-measurement.ts @@ -10,6 +10,7 @@ export interface CachedAsciiTextMetrics { measure(text: string): number; hasEveryGlyph(text: string): boolean; measureAdvances(graphemes: readonly string[]): number[]; + measureWidth(graphemes: readonly string[]): number; } const CODE_LIGATURE_CANDIDATE = /(?:===?|!==?|=>|<=|>=|->|::|\+\+|--)/; @@ -68,6 +69,22 @@ export function createChunkedAdvanceMeasurer( }; } +/** Preserve advance chunk boundaries when only the final width is needed. */ +export function createChunkedWidthMeasurer( + measure: (graphemes: readonly string[]) => number, +): (graphemes: readonly string[]) => number { + return (graphemes) => { + let width = 0; + let start = 0; + while (start < graphemes.length) { + const end = chunkEnd(graphemes, start); + width += measure(graphemes.slice(start, end)); + start = end; + } + return width; + }; +} + /** * Advance measurement for a backend that can only measure a whole string, which * is every backend except a native paragraph shaper. @@ -144,6 +161,11 @@ export function createCachedAsciiTextMetrics(primary: PrimaryTextFace): CachedAs return true; }, measureAdvances: additiveAdvances(measure), + measureWidth(graphemes) { + let width = 0; + for (const grapheme of graphemes) width += measure(grapheme); + return width; + }, }; } diff --git a/packages/app/src/git/diff-document/text.native.ts b/packages/app/src/git/diff-document/text.native.ts index 0c6f16bb5d..c482bed1a2 100644 --- a/packages/app/src/git/diff-document/text.native.ts +++ b/packages/app/src/git/diff-document/text.native.ts @@ -10,6 +10,7 @@ import { codeTextColor } from "./palette"; import { createCachedAsciiTextMetrics, createChunkedAdvanceMeasurer, + createChunkedWidthMeasurer, requiresShaping, type CachedAsciiTextMetrics, } from "./text-measurement"; @@ -202,43 +203,40 @@ export function createNativeTextMeasurer(input: { }; const asciiMetrics = createCachedAsciiTextMetrics(primary); const black = Skia.Color("black"); - const measureParagraph = (text: string) => { - const paragraph = createParagraph({ + const shapeParagraph = (text: string) => + createParagraph({ text, families, fontSize: input.fontSize, lineHeight: Math.round(input.fontSize * 1.5), color: black, }); - const width = paragraph.getLongestLine(); - paragraph.dispose(); - return width; - }; return { measure(text) { - return requiresRetainedParagraph(text, asciiMetrics) - ? measureParagraph(text) - : asciiMetrics.measure(text); + if (!requiresRetainedParagraph(text, asciiMetrics)) return asciiMetrics.measure(text); + const paragraph = shapeParagraph(text); + const width = paragraph.getLongestLine(); + paragraph.dispose(); + return width; }, + measureWidth: createChunkedWidthMeasurer((graphemes) => { + const text = graphemes.join(""); + if (!requiresRetainedParagraph(text, asciiMetrics)) + return asciiMetrics.measureWidth(graphemes); + const paragraph = shapeParagraph(text); + const width = paragraphAdvance(paragraph, text.length); + paragraph.dispose(); + return width; + }), measureAdvances: createChunkedAdvanceMeasurer({ requiresShaping: (text) => requiresRetainedParagraph(text, asciiMetrics), measureAdditive: (graphemes) => asciiMetrics.measureAdvances(graphemes), measureShaped(graphemes) { - const paragraph = createParagraph({ - text: graphemes.join(""), - families, - fontSize: input.fontSize, - lineHeight: Math.round(input.fontSize * 1.5), - color: black, - }); + const paragraph = shapeParagraph(graphemes.join("")); let end = 0; const advances = graphemes.map((grapheme) => { end += grapheme.length; - const rectangles = paragraph.getRectsForRange(0, end); - return rectangles.reduce( - (right, rectangle) => Math.max(right, rectangle.x + rectangle.width), - 0, - ); + return paragraphAdvance(paragraph, end); }); paragraph.dispose(); return advances; @@ -247,6 +245,12 @@ export function createNativeTextMeasurer(input: { }; } +function paragraphAdvance(paragraph: SkParagraph, end: number): number { + return paragraph + .getRectsForRange(0, end) + .reduce((right, rectangle) => Math.max(right, rectangle.x + rectangle.width), 0); +} + function requiresRetainedParagraph(text: string, asciiMetrics: CachedAsciiTextMetrics): boolean { return requiresShaping(text) || !asciiMetrics.hasEveryGlyph(text); } diff --git a/packages/app/src/git/diff-document/types.ts b/packages/app/src/git/diff-document/types.ts index c307628d0c..4cb7aaff80 100644 --- a/packages/app/src/git/diff-document/types.ts +++ b/packages/app/src/git/diff-document/types.ts @@ -167,6 +167,8 @@ export interface DiffDocumentModel { export interface TextMeasurer { measure(text: string, weight?: "regular" | "semibold"): number; measureAdvances?(graphemes: readonly string[]): number[]; + /** Same width as the last cumulative advance, without computing every position. */ + measureWidth?(graphemes: readonly string[]): number; } export interface BuildDiffDocumentModelInput { diff --git a/packages/server/CLAUDE.md b/packages/server/CLAUDE.md index 1324b3842c..34f99a3943 100644 --- a/packages/server/CLAUDE.md +++ b/packages/server/CLAUDE.md @@ -84,7 +84,7 @@ npm run cli -- daemon status # Check daemon status ### Imports -- Use path alias `@server/*` in server package (maps to `./src/`) +- Use relative `.js` paths for imports within shipped server code, including type imports. TypeScript preserves `@server/*` aliases in JavaScript and declarations, and production Node has no alias resolver. Vitest tests support the alias. - No barrel `index.ts` re-exports — they create unnecessary indirection ### Naming diff --git a/packages/server/src/server/checkout-diff-cache.test.ts b/packages/server/src/server/checkout-diff-cache.test.ts new file mode 100644 index 0000000000..e2ac4f31ba --- /dev/null +++ b/packages/server/src/server/checkout-diff-cache.test.ts @@ -0,0 +1,116 @@ +import { expect, test } from "vitest"; +import { CheckoutDiffCache } from "@server/server/checkout-diff-cache.js"; + +function createSequencedLoader(first: Promise, second: Promise) { + let calls = 0; + return { + load: () => (++calls === 1 ? first : second), + get calls() { + return calls; + }, + }; +} + +async function fillCompletedCache(cache: CheckoutDiffCache) { + for (let i = 0; i < 70; i++) { + await cache.read({ + cwd: String(i), + compare: { mode: "base" }, + load: async () => ({ diff: "" }), + }); + } +} + +test("forced reads after a mutation share a fresh build after the active build finishes", async () => { + const cache = new CheckoutDiffCache(() => 0); + const first = Promise.withResolvers<{ diff: string }>(); + const second = Promise.withResolvers<{ diff: string }>(); + const loader = createSequencedLoader(first.promise, second.promise); + const { load } = loader; + const pending = cache.read({ cwd: "repo", compare: { mode: "base" }, load }); + await Promise.resolve(); + const options = { force: true, reason: "repository-mutation" }; + const fresh = cache.read({ cwd: "repo", compare: { mode: "base" }, ...options, load }); + const another = cache.read({ cwd: "repo", compare: { mode: "base" }, ...options, load }); + expect(loader.calls).toBe(1); + first.resolve({ diff: "before-mutation" }); + expect(await pending).toEqual({ diff: "before-mutation" }); + await Promise.resolve(); + await Promise.resolve(); + expect(loader.calls).toBe(2); + second.resolve({ diff: "after-mutation" }); + expect(await fresh).toEqual({ diff: "after-mutation" }); + expect(await another).toEqual({ diff: "after-mutation" }); + expect(await cache.read({ cwd: "repo", compare: { mode: "base" }, load })).toEqual({ + diff: "after-mutation", + }); + expect(loader.calls).toBe(2); +}); + +test("reads after invalidation wait for fresh work without overlapping the original read", async () => { + const cache = new CheckoutDiffCache(() => 0); + const first = Promise.withResolvers<{ diff: string }>(); + const second = Promise.withResolvers<{ diff: string }>(); + const loader = createSequencedLoader(first.promise, second.promise); + const { load } = loader; + const read = () => cache.read({ cwd: "repo", compare: { mode: "base" }, load }); + const pending = read(); + await Promise.resolve(); + cache.invalidate("repo", "base"); + cache.invalidate("repo", "base"); + const fresh = read(); + const another = read(); + expect(loader.calls).toBe(1); + first.resolve({ diff: "old" }); + expect(await pending).toEqual({ diff: "old" }); + await Promise.resolve(); + await Promise.resolve(); + expect(loader.calls).toBe(2); + second.resolve({ diff: "new" }); + expect(await fresh).toEqual({ diff: "new" }); + expect(await another).toEqual({ diff: "new" }); + expect(await read()).toEqual({ diff: "new" }); + expect(loader.calls).toBe(2); +}); + +test("edits during a read do not delay its caller or cache its outdated snapshot", async () => { + const cache = new CheckoutDiffCache(() => 0); + const deferred = Promise.withResolvers<{ diff: string }>(); + let calls = 0; + const load = () => { + calls += 1; + return deferred.promise; + }; + const pending = cache.read({ cwd: "repo", compare: { mode: "uncommitted" }, load }); + await Promise.resolve(); + cache.invalidate("repo", "uncommitted"); + deferred.resolve({ diff: "snapshot" }); + expect(await pending).toEqual({ diff: "snapshot" }); + expect(calls).toBe(1); + expect( + await cache.read({ + cwd: "repo", + compare: { mode: "uncommitted" }, + load: async () => ({ diff: "fresh" }), + }), + ).toEqual({ diff: "fresh" }); +}); + +test("active reads survive completed-payload eviction and failed reads can retry", async () => { + const cache = new CheckoutDiffCache(() => 0); + const deferred = Promise.withResolvers<{ diff: string }>(); + const load = () => deferred.promise; + const first = cache.read({ cwd: "active", compare: { mode: "uncommitted" }, load }); + const rejected = expect(first).rejects.toThrow("read failed"); + await fillCompletedCache(cache); + expect(cache.read({ cwd: "active", compare: { mode: "uncommitted" }, load })).toBe(first); + deferred.reject(new Error("read failed")); + await rejected; + expect( + await cache.read({ + cwd: "active", + compare: { mode: "uncommitted" }, + load: async () => ({ diff: "retry" }), + }), + ).toEqual({ diff: "retry" }); +}); diff --git a/packages/server/src/server/checkout-diff-cache.ts b/packages/server/src/server/checkout-diff-cache.ts new file mode 100644 index 0000000000..38675bea37 --- /dev/null +++ b/packages/server/src/server/checkout-diff-cache.ts @@ -0,0 +1,92 @@ +import { LRUCache } from "lru-cache"; +import type { CheckoutDiffCompare, CheckoutDiffResult } from "../utils/checkout-git.js"; + +interface ActiveRead { + promise: Promise; + invalidated: boolean; +} + +interface CachedDiff { + value: CheckoutDiffResult; + loadedAt: number; + lastReadStarted: number; +} + +interface CheckoutDiffReadInput { + cwd: string; + compare: CheckoutDiffCompare; + force?: boolean; + reason?: string; + load: () => Promise; +} + +/** Completed payloads are bounded; active work survives invalidation and eviction. */ +export class CheckoutDiffCache { + private readonly values = new LRUCache({ + max: 64, + }); + private readonly active = new Map(); + + constructor(private readonly now: () => number) {} + + read(input: CheckoutDiffReadInput): Promise { + const { cwd, compare, force, reason, load } = input; + if (force && !reason) throw new Error("WorkspaceGitService forced read requires a reason"); + const key = JSON.stringify([ + cwd, + compare.mode, + compare.mode === "base" ? (compare.baseRef ?? null) : null, + compare.ignoreWhitespace === true, + compare.includeStructured === true, + ]); + const cached = this.values.get(key); + const now = this.now(); + if ( + !force && + cached && + (now - cached.loadedAt <= 15_000 || now - cached.lastReadStarted < 2_000) + ) + return Promise.resolve(cached.value); + const pending = this.active.get(key); + if (pending) { + if (force) { + pending.invalidated = true; + this.values.delete(key); + } + if (!pending.invalidated) return pending.promise; + // A caller arriving after a change needs a fresh read, once the old one + // finishes. The original caller can still receive its snapshot promptly. + // The older result cannot enter the cache. Drop force when retrying so + // callers waiting on that same result share the next fresh build. + const reload = () => this.read({ ...input, force: false }); + return pending.promise.then(reload, reload); + } + if (cached) cached.lastReadStarted = now; + + // Defer load until ownership is installed, including synchronous invalidations. + const read: ActiveRead = { + invalidated: false, + promise: Promise.resolve() + .then(load) + .then((value) => { + // Never repopulate the cache with a snapshot invalidated during its build. + if (!read.invalidated) + this.values.set(key, { value, loadedAt: this.now(), lastReadStarted: now }); + return value; + }) + .finally(() => this.active.delete(key)), + }; + this.active.set(key, read); + return read.promise; + } + + invalidate(cwd: string, mode: CheckoutDiffCompare["mode"]): void { + for (const key of new Set([...this.values.keys(), ...this.active.keys()])) { + const [cachedCwd, cachedMode] = JSON.parse(key) as string[]; + if (cachedCwd !== cwd || cachedMode !== mode) continue; + this.values.delete(key); + const pending = this.active.get(key); + if (pending) pending.invalidated = true; + } + } +} diff --git a/packages/server/src/server/daemon-e2e/checkout-diff-subscription.e2e.test.ts b/packages/server/src/server/daemon-e2e/checkout-diff-subscription.e2e.test.ts index 7ce8cbb3a3..fac7d01a3f 100644 --- a/packages/server/src/server/daemon-e2e/checkout-diff-subscription.e2e.test.ts +++ b/packages/server/src/server/daemon-e2e/checkout-diff-subscription.e2e.test.ts @@ -146,7 +146,9 @@ test("keeps the socket usable after rejecting an oversized structured diff", asy initGitRepo(cwd); commitFile(cwd, "large-a.js", "const value = 0;\n"); commitFile(cwd, "large-b.js", "const value = 0;\n"); - const denseExpression = `const value = ${"a+".repeat(450_000)}a;\n`; + // Short lines stay eligible for highlighting; a giant single line is skipped + // before token expansion and cannot exercise the structured-payload limit. + const denseExpression = `const value = ${"a+".repeat(100)}a;\n`.repeat(4_000); writeFileSync(path.join(cwd, "large-a.js"), denseExpression); writeFileSync(path.join(cwd, "large-b.js"), denseExpression); diff --git a/packages/server/src/server/utils/diff-highlighter.ts b/packages/server/src/server/utils/diff-highlighter.ts index 4f7f800dc9..daa1ebbb3d 100644 --- a/packages/server/src/server/utils/diff-highlighter.ts +++ b/packages/server/src/server/utils/diff-highlighter.ts @@ -332,30 +332,30 @@ export async function highlightDiffWithFileContent( return file; } - const reconstructedTokens = buildReconstructedTokenLookups(file); - let newTokensByLine = reconstructedTokens.newTokensByLine; - let oldTokensByLine = reconstructedTokens.oldTokensByLine; - - if (typeof options.oldFileContent === "string") { - oldTokensByLine = - buildFullFileTokenLookup(options.oldFileContent, file.path) ?? oldTokensByLine; - } - + let newTokensByLine: Map | null = null; + const oldTokensByLine = + typeof options.oldFileContent === "string" + ? buildFullFileTokenLookup(options.oldFileContent, file.path) + : null; if (typeof options.newFileContent === "string") { - newTokensByLine = - buildFullFileTokenLookup(options.newFileContent, file.path) ?? newTokensByLine; + newTokensByLine = buildFullFileTokenLookup(options.newFileContent, file.path); + } else { + try { + const fileContent = await readFile(resolve(cwd, file.path), "utf-8"); + newTokensByLine = buildFullFileTokenLookup(fileContent, file.path); + } catch { + // Deleted or unavailable files use reconstructed tokens below. + } + } + + if (newTokensByLine && oldTokensByLine) return applyTokensToHunks(file, newTokensByLine, oldTokensByLine); - } - - const filePath = resolve(cwd, file.path); - try { - const fileContent = await readFile(filePath, "utf-8"); - newTokensByLine = buildFullFileTokenLookup(fileContent, file.path) ?? newTokensByLine; - } catch { - // If file read fails (deleted file, etc.), fall back to reconstructed new-side tokens. - } - - return applyTokensToHunks(file, newTokensByLine, oldTokensByLine); + const reconstructedTokens = buildReconstructedTokenLookups(file); + return applyTokensToHunks( + file, + newTokensByLine ?? reconstructedTokens.newTokensByLine, + oldTokensByLine ?? reconstructedTokens.oldTokensByLine, + ); } function applyTokensToHunks( diff --git a/packages/server/src/server/workspace-git-service.observation.test.ts b/packages/server/src/server/workspace-git-service.observation.test.ts index c55faa2bb5..36c50769c8 100644 --- a/packages/server/src/server/workspace-git-service.observation.test.ts +++ b/packages/server/src/server/workspace-git-service.observation.test.ts @@ -216,6 +216,35 @@ describe("WorkspaceGitService checkout observation", () => { vi.useRealTimers(); }); + test("waits for the initial watcher inventory before building a cold diff", async () => { + const watcher = createWatcherHarness(); + const inventoryFinished = createDeferred(); + let builds = 0; + const service = createService(watcher, { + subscribe: async (...args: Parameters) => { + await inventoryFinished.promise; + return watcher.subscribe(...args); + }, + getCheckoutDiff: async () => { + builds += 1; + return { diff: "", structured: [] }; + }, + }); + const subscription = service.registerWorkspace({ cwd: REPO_CWD }, () => {}); + try { + const read = service.getCheckoutDiff(REPO_CWD, { mode: "uncommitted" }); + await flushPromises(); + expect(builds).toBe(0); + inventoryFinished.resolve(); + await read; + expect(builds).toBe(1); + } finally { + inventoryFinished.resolve(); + subscription.unsubscribe(); + await service.dispose(); + } + }); + test("dispose waits for file observation to finish closing", async () => { const watcher = createWatcherHarness(); const closeFinished = createDeferred(); diff --git a/packages/server/src/server/workspace-git-service.ts b/packages/server/src/server/workspace-git-service.ts index fc1d211f33..524360678b 100644 --- a/packages/server/src/server/workspace-git-service.ts +++ b/packages/server/src/server/workspace-git-service.ts @@ -2,6 +2,7 @@ import { createHash } from "node:crypto"; import { readFile } from "node:fs/promises"; import { basename, join, resolve } from "node:path"; import { LRUCache } from "lru-cache"; +import { CheckoutDiffCache } from "./checkout-diff-cache.js"; import pLimit from "p-limit"; import type pino from "pino"; import type { ProjectCheckoutLitePayload } from "@getpaseo/protocol/messages"; @@ -94,8 +95,6 @@ const WATCH_RECOVERY_MAX_ATTEMPTS = 3; const WORKSPACE_GIT_AUXILIARY_READ_TTL_MS = 15_000; // Non-forced refresh triggers share this minimum gap to absorb watcher/self-heal bursts; force bypasses it. const WORKSPACE_GIT_INTERNAL_MIN_GAP_MS = 2_000; -// Heavy values (multi-MB highlighted diffs); cap aggressively. Ephemeral worktree cwds would otherwise pile up forever. -const WORKSPACE_GIT_CHECKOUT_DIFF_CACHE_MAX = 64; // Small values (booleans, short strings, small arrays); generous cap. const WORKSPACE_GIT_AUXILIARY_CACHE_MAX = 256; @@ -566,10 +565,7 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { string, WorkspaceGitAuxiliaryReadCacheEntry >({ max: WORKSPACE_GIT_AUXILIARY_CACHE_MAX }); - private readonly checkoutDiffCache = new LRUCache< - string, - WorkspaceGitAuxiliaryReadCacheEntry - >({ max: WORKSPACE_GIT_CHECKOUT_DIFF_CACHE_MAX }); + private readonly checkoutDiffCache = new CheckoutDiffCache(() => this.deps.now().getTime()); private watcherErrorCallbackCount = 0; constructor(options: WorkspaceGitServiceOptions) { this.logger = options.logger.child({ module: "workspace-git-service" }); @@ -726,7 +722,7 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { return this.workspaceTargets.get(cwd)?.latestSnapshot ?? null; } - getCheckoutDiff( + async getCheckoutDiff( cwd: string, options: CheckoutDiffCompare, readOptions?: WorkspaceGitReadOptions, @@ -734,13 +730,20 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { this.assertNotDisposed(); const normalizedCwd = resolve(cwd); const normalizedOptions = this.normalizeCheckoutDiffOptions(options); - const key = this.buildCheckoutDiffCacheKey(normalizedCwd, normalizedOptions); - return this.readAuxiliaryCache(this.checkoutDiffCache, key, readOptions, () => - this.deps.getCheckoutDiff(normalizedCwd, normalizedOptions, { - paseoHome: this.paseoHome, - worktreesRoot: this.worktreesRoot, - }), - ); + // Initial inventory events cover the watcher setup gap. Process them before + // starting a cold diff, rather than invalidating that build halfway through. + await this.workspaceTargets.get(normalizedCwd)?.observationSetupPromise; + this.assertNotDisposed(); + return this.checkoutDiffCache.read({ + cwd: normalizedCwd, + compare: normalizedOptions, + ...readOptions, + load: () => + this.deps.getCheckoutDiff(normalizedCwd, normalizedOptions, { + paseoHome: this.paseoHome, + worktreesRoot: this.worktreesRoot, + }), + }); } private normalizeCheckoutDiffOptions(options: CheckoutDiffCompare): CheckoutDiffCompare { @@ -754,26 +757,8 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { }; } - private buildCheckoutDiffCacheKey(cwd: string, options: CheckoutDiffCompare): string { - // Diff content varies by compare signature. Keep the cache per exact diff read shape so - // hot diff panes coalesce while base refs and rendering options never share stale patches. - return JSON.stringify([ - "checkout-diff", - cwd, - options.mode, - options.mode === "base" ? (options.baseRef ?? null) : null, - options.ignoreWhitespace === true, - options.includeStructured === true, - ]); - } - private invalidateCheckoutDiffCache(cwd: string, mode: CheckoutDiffCompare["mode"]): void { - for (const key of this.checkoutDiffCache.keys()) { - const [kind, cachedCwd, cachedMode] = JSON.parse(key) as unknown[]; - if (kind === "checkout-diff" && cachedCwd === cwd && cachedMode === mode) { - this.checkoutDiffCache.delete(key); - } - } + this.checkoutDiffCache.invalidate(cwd, mode); } validateBranchRef( diff --git a/packages/server/src/utils/checkout-git-batching.test.ts b/packages/server/src/utils/checkout-git-batching.test.ts index 85bea20c49..18b84bd0c3 100644 --- a/packages/server/src/utils/checkout-git-batching.test.ts +++ b/packages/server/src/utils/checkout-git-batching.test.ts @@ -2,39 +2,8 @@ import { execFileSync } from "node:child_process"; import { mkdirSync, mkdtempSync, rmSync, writeFileSync, realpathSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { beforeEach, afterEach, describe, expect, it, vi } from "vitest"; - -const spawnCounters = vi.hoisted(() => ({ - trackedTextDiffCalls: 0, -})); - -vi.mock("child_process", async () => { - const actual = await vi.importActual("child_process"); - return { - ...actual, - spawn: (...args: Parameters) => { - const [command, commandArgs] = args; - if (command === "git" && Array.isArray(commandArgs)) { - const normalizedArgs = commandArgs.map((arg) => String(arg)); - // `runGitCommand` always prepends its two config overrides; skip them - // to find the actual git subcommand. - const subcommandIndex = - normalizedArgs[0] === "-c" && normalizedArgs[1] === "core.quotepath=false" ? 4 : 0; - const isTrackedTextDiff = - normalizedArgs[subcommandIndex] === "diff" && - normalizedArgs.includes("HEAD") && - !normalizedArgs.includes("--numstat") && - !normalizedArgs.includes("--no-index") && - !normalizedArgs.includes("--shortstat") && - !normalizedArgs.includes("--name-status"); - if (isTrackedTextDiff) { - spawnCounters.trackedTextDiffCalls += 1; - } - } - return actual.spawn(...args); - }, - }; -}); +import { beforeEach, afterEach, describe, expect, it } from "vitest"; +import { startGitCommandMetrics, stopGitCommandMetrics } from "@server/utils/run-git-command.js"; import { getCheckoutDiff } from "./checkout-git.js"; @@ -70,14 +39,14 @@ describe("checkout git diff batching", () => { const setup = initRepoWithTrackedChanges(20); tempDir = setup.tempDir; repoDir = setup.repoDir; - spawnCounters.trackedTextDiffCalls = 0; }); afterEach(() => { rmSync(tempDir, { recursive: true, force: true }); }); - it("uses per-file tracked git diff commands for tracked file diffs", async () => { + it("collects ordinary tracked patches with one bounded Git command", async () => { + startGitCommandMetrics(); const result = await getCheckoutDiff(repoDir, { mode: "uncommitted", includeStructured: false, @@ -85,6 +54,44 @@ describe("checkout git diff batching", () => { expect(result.diff).toContain("file-0.txt"); expect(result.diff).toContain("file-19.txt"); - expect(spawnCounters.trackedTextDiffCalls).toBe(20); + const metrics = stopGitCommandMetrics(); + expect( + metrics.commands.filter(({ args }) => args[0] === "diff" && args.includes("--")), + ).toHaveLength(1); + }); + + it("batches committed file contents and preserves the highlighted diff", async () => { + execFileSync("git", ["checkout", "-b", "feature"], { cwd: repoDir }); + execFileSync("git", ["add", "."], { cwd: repoDir }); + execFileSync("git", ["-c", "commit.gpgsign=false", "commit", "-m", "change"], { cwd: repoDir }); + startGitCommandMetrics(); + const result = await getCheckoutDiff(repoDir, { + mode: "base", + baseRef: "main", + includeStructured: true, + }); + const metrics = stopGitCommandMetrics(); + expect(result.structured).toHaveLength(20); + expect(result.structured?.every((file) => file.additions === 1 && file.deletions === 1)).toBe( + true, + ); + expect(metrics.commands.filter(({ args }) => args[0] === "show")).toHaveLength(0); + expect(metrics.commands.filter(({ args }) => args[0] === "cat-file")).toHaveLength(1); + }); + + it("keeps small neighbors when a patch exceeds the entire batch budget", async () => { + writeFileSync(join(repoDir, "file-0.txt"), "x".repeat(9 * 1024 * 1024) + "\n"); + const result = await getCheckoutDiff(repoDir, { + mode: "uncommitted", + includeStructured: true, + }); + expect(result.structured).toHaveLength(20); + expect(result.structured?.find((file) => file.path === "file-0.txt")).toMatchObject({ + status: "too_large", + hunks: [], + }); + expect(result.structured?.filter((file) => file.status === "ok")).toHaveLength(19); + expect(result.diff).toContain("+after-19"); + expect(result.diff).not.toContain("x".repeat(1_000)); }); }); diff --git a/packages/server/src/utils/checkout-git.test.ts b/packages/server/src/utils/checkout-git.test.ts index 45c0aff7c1..f83ee46ae5 100644 --- a/packages/server/src/utils/checkout-git.test.ts +++ b/packages/server/src/utils/checkout-git.test.ts @@ -1507,8 +1507,7 @@ const x = 1; expect(diff.diff).toContain(`-export const value = "old";`); expect(diff.diff).toContain(`+export const value = "new";`); expect(commands).toContain("diff --numstat HEAD"); - expect(commands).toContain("diff HEAD -- generated.js"); - expect(commands).toContain("diff HEAD -- small.ts"); + expect(commands).toContain("diff HEAD -- :(literal)generated.js :(literal)small.ts"); expect(metrics.maxConcurrent).toBeLessThanOrEqual(8); }); diff --git a/packages/server/src/utils/checkout-git.ts b/packages/server/src/utils/checkout-git.ts index d20a41616e..8e2a89f0bd 100644 --- a/packages/server/src/utils/checkout-git.ts +++ b/packages/server/src/utils/checkout-git.ts @@ -1,6 +1,7 @@ import { resolve, dirname, basename } from "path"; import { existsSync, realpathSync } from "fs"; import { open as openFile, readFile, stat as statFile } from "fs/promises"; +import { setImmediate } from "node:timers/promises"; import { TTLCache } from "@isaacs/ttlcache"; import type { CheckoutCommit, CheckoutCommitFile } from "@getpaseo/protocol/messages"; import { parseGitHubRemoteIdentity, parseGitRemoteLocation } from "@getpaseo/protocol/git-remote"; @@ -29,6 +30,7 @@ import { } from "../services/forge-cli-command.js"; import { parseGitRevParsePath, resolveGitRevParsePath } from "./git-rev-parse-path.js"; import { runGitCommand, type RunGitCommand } from "./run-git-command.js"; +import { readGitFileContents } from "./git-file-contents.js"; import { isPaseoOwnedWorktreeCwd, resolvePaseoWorktreesBaseRoot } from "./worktree.js"; import { branchNameFromRef, @@ -639,7 +641,7 @@ function buildGitDiffArgs(args: { ignoreWhitespace?: boolean; extra: string[] }) } const TRACKED_DIFF_NUMSTAT_MAX_BYTES = 2 * 1024 * 1024; // 2MB -const TRACKED_DIFF_BATCH_SIZE = 8; +const TRACKED_DIFF_BATCH_SIZE = 64; const EMPTY_TREE_OBJECT_ID = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"; function isUnbornHeadDiffError(error: unknown): boolean { @@ -716,7 +718,7 @@ async function getTrackedDiffTextForPath(input: { const result = await runGitCommand( buildGitDiffArgs({ ignoreWhitespace: input.ignoreWhitespace, - extra: [...getCheckoutDiffRefArgs(input.refsForDiff), "--", input.path], + extra: [...getCheckoutDiffRefArgs(input.refsForDiff), "--", `:(literal)${input.path}`], }), { cwd: input.cwd, @@ -732,6 +734,56 @@ async function getTrackedDiffTextForPath(input: { }; } +async function getTrackedDiffTexts(input: { + cwd: string; + refsForDiff: CheckoutDiffRefs; + paths: string[]; + ignoreWhitespace: boolean; +}): Promise> { + if (input.paths.length === 0) return []; + if (input.paths.length === 1) + return [await getTrackedDiffTextForPath({ ...input, path: input.paths[0] })]; + const result = await runGitCommand( + buildGitDiffArgs({ + ignoreWhitespace: input.ignoreWhitespace, + extra: [ + ...getCheckoutDiffRefArgs(input.refsForDiff), + "--", + ...input.paths.map((path) => `:(literal)${path}`), + ], + }), + { cwd: input.cwd, envOverlay: READ_ONLY_GIT_ENV, maxOutputBytes: 8 * PER_FILE_DIFF_MAX_BYTES }, + ); + const files = new Map(); + if (!result.truncated) { + for (const text of result.stdout.split(/(?=^diff --git )/m).filter(Boolean)) { + // Only identify the file here. Oversized bodies must not reach the parser. + const hunkStart = text.indexOf("\n@@ "); + const file = parseDiff(hunkStart < 0 ? text : text.slice(0, hunkStart))[0]; + if (!file || !input.paths.includes(file.path)) break; + files.set(file.path, { + path: file.path, + text, + truncated: Buffer.byteLength(text) > PER_FILE_DIFF_MAX_BYTES, + }); + } + // Empty patches are valid when whitespace is ignored. Every emitted section + // must be accounted for before accepting a combined command. + if ( + [...files.values()].reduce((length, file) => length + file.text.length, 0) === + result.stdout.length + ) { + return input.paths.map((path) => files.get(path) ?? { path, text: "", truncated: false }); + } + } + // Bound oversized output without letting one large file hide its neighbors. + const middle = Math.ceil(input.paths.length / 2); + return [ + ...(await getTrackedDiffTexts({ ...input, paths: input.paths.slice(0, middle) })), + ...(await getTrackedDiffTexts({ ...input, paths: input.paths.slice(middle) })), + ]; +} + export class NotGitRepoError extends Error { readonly cwd: string; readonly code = "NOT_GIT_REPO"; @@ -2978,13 +3030,23 @@ async function buildHighlightedTrackedDiffFile(input: { change: CheckoutFileChange; parsedFile: ParsedDiffFile; refsForDiff: CheckoutDiffRefs; + contents?: Map; }): Promise { const { cwd, change, parsedFile, refsForDiff } = input; const refPath = change.oldPath ?? change.path; - const [oldFileContent, newFileContent] = await Promise.all([ - change.isNew ? null : readGitFileContentAtRef(cwd, refsForDiff.baseRef, refPath), - refsForDiff.targetRef ? readGitFileContentAtRef(cwd, refsForDiff.targetRef, change.path) : null, - ]); + const [oldFileContent, newFileContent] = input.contents + ? [ + change.isNew ? null : input.contents.get(`${refsForDiff.baseRef}:${refPath}`), + refsForDiff.targetRef + ? input.contents.get(`${refsForDiff.targetRef}:${change.path}`) + : null, + ] + : await Promise.all([ + change.isNew ? null : readGitFileContentAtRef(cwd, refsForDiff.baseRef, refPath), + refsForDiff.targetRef + ? readGitFileContentAtRef(cwd, refsForDiff.targetRef, change.path) + : null, + ]); const highlightedFile = await highlightDiffWithFileContent(parsedFile, cwd, { oldFileContent, newFileContent, @@ -3012,6 +3074,19 @@ function isWhitespaceOnlyTrackedChange(input: { ); } +function readTrackedDiffContents( + cwd: string, + changes: CheckoutFileChange[], + refs: CheckoutDiffRefs, +): Promise | null> { + const specs: string[] = []; + for (const change of changes) { + if (!change.isNew) specs.push(`${refs.baseRef}:${change.oldPath ?? change.path}`); + if (refs.targetRef) specs.push(`${refs.targetRef}:${change.path}`); + } + return readGitFileContents(cwd, specs); +} + async function appendStructuredTrackedDiffs( input: AppendStructuredTrackedDiffsInput, ): Promise { @@ -3030,54 +3105,68 @@ async function appendStructuredTrackedDiffs( const parsedTrackedFiles = trackedDiffText.length > 0 ? parseDiff(trackedDiffText) : []; const parsedTrackedByPath = new Map(parsedTrackedFiles.map((file) => [file.path, file])); - for (const change of trackedChanges) { - const placeholder = trackedPlaceholderByPath.get(change.path); - if (placeholder) { - const file = buildPlaceholderParsedDiffFile(change, { - status: placeholder.status, - stat: placeholder.stat, - }); + for (let start = 0; start < trackedChanges.length; start += TRACKED_DIFF_BATCH_SIZE) { + const changes = trackedChanges.slice(start, start + TRACKED_DIFF_BATCH_SIZE); + const contents = await readTrackedDiffContents( + cwd, + changes.filter( + (change) => + !trackedPlaceholderByPath.has(change.path) && parsedTrackedByPath.has(change.path), + ), + refsForDiff, + ); + for (const change of changes) { + const placeholder = trackedPlaceholderByPath.get(change.path); + if (placeholder) { + const file = buildPlaceholderParsedDiffFile(change, { + status: placeholder.status, + stat: placeholder.stat, + }); + if (!appendStructuredFile(structured, file)) { + return false; + } + appendTrackedPlaceholderComment(change, placeholder.status); + continue; + } + + const stat = trackedNumstatByPath.get(change.path) ?? null; + const parsedFile = parsedTrackedByPath.get(change.path); + if (parsedFile) { + const file = await buildHighlightedTrackedDiffFile({ + cwd, + change, + parsedFile, + refsForDiff, + contents: contents ?? undefined, + }); + if (!appendStructuredFile(structured, file)) { + return false; + } + // Batched blobs remove the per-file I/O pause; keep serving other daemon work. + await setImmediate(); + continue; + } + + // `git diff -w --name-status` can still report a modified path even when the + // whitespace-filtered patch and numstat are both empty. Skip emitting a + // structured placeholder in that case so whitespace-only edits truly disappear. + if (isWhitespaceOnlyTrackedChange({ change, stat, ignoreWhitespace })) { + continue; + } + + const file = { + path: change.path, + ...(change.oldPath ? { oldPath: change.oldPath } : {}), + isNew: change.isNew, + isDeleted: change.isDeleted, + additions: stat?.additions ?? 0, + deletions: stat?.deletions ?? 0, + hunks: [], + status: "ok", + } satisfies ParsedDiffFile; if (!appendStructuredFile(structured, file)) { return false; } - appendTrackedPlaceholderComment(change, placeholder.status); - continue; - } - - const stat = trackedNumstatByPath.get(change.path) ?? null; - const parsedFile = parsedTrackedByPath.get(change.path); - if (parsedFile) { - const file = await buildHighlightedTrackedDiffFile({ - cwd, - change, - parsedFile, - refsForDiff, - }); - if (!appendStructuredFile(structured, file)) { - return false; - } - continue; - } - - // `git diff -w --name-status` can still report a modified path even when the - // whitespace-filtered patch and numstat are both empty. Skip emitting a - // structured placeholder in that case so whitespace-only edits truly disappear. - if (isWhitespaceOnlyTrackedChange({ change, stat, ignoreWhitespace })) { - continue; - } - - const file = { - path: change.path, - ...(change.oldPath ? { oldPath: change.oldPath } : {}), - isNew: change.isNew, - isDeleted: change.isDeleted, - additions: stat?.additions ?? 0, - deletions: stat?.deletions ?? 0, - hunks: [], - status: "ok", - } satisfies ParsedDiffFile; - if (!appendStructuredFile(structured, file)) { - return false; } } @@ -3196,18 +3285,32 @@ async function processTrackedChanges( let trackedDiffText = ""; let trackedDiffBytes = 0; + const renamedPaths = new Set( + trackedChanges.filter((change) => change.oldPath).map((change) => change.path), + ); for (let start = 0; start < trackedDiffPaths.length; start += TRACKED_DIFF_BATCH_SIZE) { const paths = trackedDiffPaths.slice(start, start + TRACKED_DIFF_BATCH_SIZE); - const trackedDiffs = await Promise.all( - paths.map((path) => - getTrackedDiffTextForPath({ - cwd, - refsForDiff, - path, - ignoreWhitespace, - }), - ), + // Keep rename/copy path filtering identical to the existing per-file command. + const ordinary = await getTrackedDiffTexts({ + cwd, + refsForDiff, + paths: paths.filter((path) => !renamedPaths.has(path)), + ignoreWhitespace, + }); + const renamed = await Promise.all( + paths + .filter((path) => renamedPaths.has(path)) + .map((path) => + getTrackedDiffTextForPath({ + cwd, + refsForDiff, + path, + ignoreWhitespace, + }), + ), ); + const byPath = new Map([...ordinary, ...renamed].map((file) => [file.path, file])); + const trackedDiffs = paths.map((path) => byPath.get(path)!); for (const fileDiff of trackedDiffs) { if (fileDiff.truncated) { diff --git a/packages/server/src/utils/git-file-contents.test.ts b/packages/server/src/utils/git-file-contents.test.ts new file mode 100644 index 0000000000..7b25686e65 --- /dev/null +++ b/packages/server/src/utils/git-file-contents.test.ts @@ -0,0 +1,53 @@ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, expect, test } from "vitest"; +import { readGitFileContents } from "@server/utils/git-file-contents.js"; +import { runGitCommand, runGitCommandBytes } from "@server/utils/run-git-command.js"; + +let cwd: string; +beforeEach(() => { + cwd = mkdtempSync(join(tmpdir(), "git-contents-")); + execFileSync("git", ["init", "-q"], { cwd }); +}); +afterEach(() => rmSync(cwd, { recursive: true, force: true })); + +test("batch byte framing survives Unicode, invalid UTF-8, and missing objects", async () => { + const text = Buffer.concat([Buffer.from("中文👩‍💻\n"), Buffer.from([0xff]), Buffer.from("\n")]); + const { stdout: oid } = await runGitCommand(["hash-object", "-w", "--stdin"], { + cwd, + input: text, + }); + const ref = oid.trim(); + const empty = execFileSync("git", ["hash-object", "-w", "--stdin"], { cwd, input: "" }) + .toString() + .trim(); + const contents = await readGitFileContents(cwd, [ref, "HEAD:missing", empty, ref]); + expect(contents).toEqual( + new Map([ + [ref, text.toString("utf8")], + ["HEAD:missing", null], + [empty, ""], + ]), + ); + const raw = await runGitCommandBytes(["cat-file", "blob", ref], { cwd }); + expect(raw.stdout.equals(text)).toBe(true); +}); + +test("oversized output stays bounded and requests the existing per-file path", async () => { + writeFileSync(join(cwd, "big"), Buffer.alloc(20 * 1024 * 1024, 97)); + const oid = execFileSync("git", ["hash-object", "-w", "big"], { cwd }).toString().trim(); + expect(await readGitFileContents(cwd, [oid])).toBeNull(); + const limited = await runGitCommandBytes(["cat-file", "--batch"], { + cwd, + input: oid + "\n", + maxOutputBytes: 100, + }); + expect(limited.truncated).toBe(true); + expect(limited.stdout.length).toBe(100); +}); + +test("newline paths do not become extra batch requests", async () => { + expect(await readGitFileContents(cwd, ["HEAD:with\nnewline"])).toBeNull(); +}); diff --git a/packages/server/src/utils/git-file-contents.ts b/packages/server/src/utils/git-file-contents.ts new file mode 100644 index 0000000000..e42d3c6c83 --- /dev/null +++ b/packages/server/src/utils/git-file-contents.ts @@ -0,0 +1,48 @@ +import { runGitCommandBytes } from "./run-git-command.js"; + +// Match the existing git-show ceiling. Oversized batches use bounded per-file reads. +const MAX_CONTENT_BYTES = 20 * 1024 * 1024; + +/** Null means the batch cannot preserve per-file read semantics within its budget. */ +export async function readGitFileContents( + cwd: string, + specs: string[], +): Promise | null> { + const unique = [...new Set(specs)]; + if (unique.length === 0) return new Map(); + // The line-based batch protocol cannot represent embedded newlines. + if (unique.some((spec) => spec.includes("\n"))) return null; + const result = await runGitCommandBytes(["cat-file", "--batch"], { + cwd, + input: unique.join("\n") + "\n", + envOverlay: { GIT_OPTIONAL_LOCKS: "0", LC_ALL: "C" }, + maxOutputBytes: MAX_CONTENT_BYTES, + }); + if (result.truncated) return null; + const contents = new Map(); + let offset = 0; + for (const spec of unique) { + const end = result.stdout.indexOf(10, offset); + if (end < 0) throw new Error("Incomplete git object header"); + const header = result.stdout.subarray(offset, end).toString("utf8"); + offset = end + 1; + if (header === `${spec} missing`) { + contents.set(spec, null); + continue; + } + const match = /^[a-f0-9]+ blob (\d+)$/.exec(header); + // Submodules and other non-blob objects retain git-show behavior. + if (!match) return null; + const size = Number(match[1]); + if ( + !Number.isSafeInteger(size) || + offset + size >= result.stdout.length || + result.stdout[offset + size] !== 10 + ) + throw new Error("Invalid git object length"); + contents.set(spec, result.stdout.subarray(offset, offset + size).toString("utf8")); + offset += size + 1; + } + if (offset !== result.stdout.length) throw new Error("Unexpected git object output"); + return contents; +} diff --git a/packages/server/src/utils/run-git-command.ts b/packages/server/src/utils/run-git-command.ts index aca69555ee..bdf7bc1f32 100644 --- a/packages/server/src/utils/run-git-command.ts +++ b/packages/server/src/utils/run-git-command.ts @@ -46,6 +46,7 @@ export function configureGitProcessPolicy(policy: GitProcessPolicy): void { export interface GitCommandOptions { cwd: string; + input?: string | Buffer; env?: ProcessEnvRecord; envOverlay?: ProcessEnvRecord; logger?: Pick; @@ -54,8 +55,8 @@ export interface GitCommandOptions { acceptExitCodes?: number[]; } -export interface GitCommandResult { - stdout: string; +export interface GitCommandResult { + stdout: Output; stderr: string; truncated: boolean; exitCode: number | null; @@ -263,6 +264,23 @@ function runGitCommandWithProvenance( options: GitCommandOptions, provenance?: string, ): Promise { + return executeGitCommand(args, options, (output) => output.toString("utf8"), provenance); +} + +/** Binary stdout preserves cat-file byte framing before text decoding. */ +export function runGitCommandBytes( + args: string[], + options: GitCommandOptions, +): Promise> { + return executeGitCommand(args, options, (output) => output); +} + +function executeGitCommand( + args: string[], + options: GitCommandOptions, + decode: (output: Buffer) => Output, + provenance?: string, +): Promise> { const metricsState = submitGitCommandMetric(args, options.cwd); const commandTrace = submitGitCommandTrace(args, options.cwd, { active: gitProcessScheduler.activeCount, @@ -274,7 +292,7 @@ function runGitCommandWithProvenance( const exited = new Promise((resolve) => { releaseProcessSlot = resolve; }); - const resultPromise = new Promise((resolve, reject) => { + const resultPromise = new Promise>((resolve, reject) => { startGitCommandTrace(commandTrace, { active: gitProcessScheduler.activeCount, pending: gitProcessScheduler.pendingCount, @@ -385,7 +403,7 @@ function runGitCommandWithProvenance( cwd: options.cwd, envOverlay, shell: false, - stdio: ["ignore", "pipe", "pipe"], + stdio: [options.input === undefined ? "ignore" : "pipe", "pipe", "pipe"], }, ); spawnGitCommandTrace(commandTrace, child.pid); @@ -465,12 +483,23 @@ function runGitCommandWithProvenance( } }); + if (options.input !== undefined) { + child.stdin!.on("error", (error: NodeJS.ErrnoException) => { + // A command may close stdin when it exits or reaches the output limit. + if (error.code !== "EPIPE") { + processError = error; + child.kill("SIGKILL"); + } + }); + child.stdin!.end(options.input); + } + child.on("exit", markProcessExited); child.on("close", (exitCode, signal) => { markProcessExited(exitCode, signal); - const result: GitCommandResult = { - stdout: Buffer.concat(stdoutChunks).toString("utf8"), + const result: GitCommandResult = { + stdout: decode(Buffer.concat(stdoutChunks)), stderr: Buffer.concat(stderrChunks).toString("utf8"), truncated, exitCode,