flyby refactors#13786
Conversation
The question is where it gets the idea to fetch a certain template from in the first place. But from an API perspective, the function is hard to use.
It's so easy to miss these debugs, in all the noise from the other debugs, that also are traces now.
|
This is a CC for @estib-vega due to the PR template change. The question is if it's a problem if it ends up empty. The root issue though is that it thinks there is one in the first place, I couldn't figure out where it has it from, maybe stored state somewhere. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ea4ec07e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR improves diagnosability around branch pushing and review/PR template handling by increasing visibility of push-skip reasons and making template reads more tolerant of missing files, alongside a few tracing/dependency cleanups.
Changes:
- Emit
info-level logs when stack branches are skipped during push to make “why didn’t it push?” reasons visible. - Adjust PR/review template reading to avoid hard failures when template content is missing.
- Reduce instrumentation span verbosity (debug → trace) for some high-frequency operations and tidy an optional dependency entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
crates/gitbutler-branch-actions/src/stack.rs |
Promotes skipped-branch push logs from debug to info so users can see why a branch wasn’t pushed. |
crates/but-graph/src/projection/workspace/init.rs |
Lowers Graph::into_workspace span level to trace to reduce default log noise. |
crates/but-forge/Cargo.toml |
Fixes placement of the optional but-schemars dependency so the export-schema feature can reference it cleanly. |
crates/but-db/src/handle.rs |
Lowers DB handle construction instrumentation to trace. |
crates/but-db/src/cache/handle.rs |
Lowers cache handle construction instrumentation to trace. |
crates/but-api/src/legacy/forge.rs |
Changes template content extraction to default empty content instead of erroring on None. |
Comments suppressed due to low confidence (1)
crates/but-api/src/legacy/forge.rs:120
- In
review_template,unwrap_or_default()will returnSome(ReviewTemplateInfo { content: "" })even when the configured template file is missing (missing files yieldFileInfo::default()withsize=None). That makes it hard for callers to distinguish “missing template file” from a legitimately empty template, and it also hides invalid/binary content (wheresize=Somebutcontent=None). Consider branching onFileInfo.size/content: returnOk(None)when the file is missing, and error (or otherwise signal) when the file exists but isn’t valid UTF-8 text.
let content = ctx
.read_file_from_workspace(&path)?
.content
.unwrap_or_default();
Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
…jectId as input and output With these one can avoid to re-traverse the commit graph from scratch when asking for merge-bases. After all, we have all that information already. Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
crates/but-api/src/legacy/forge.rs:122
- Same as
pr_template(): a review template path that resolves to an image will produce base64 content andmime_type=Some(...), but will currently be treated as valid text because onlyis_invalid_utf8()is checked. Consider bailing whenfile.mime_type.is_some()to avoid returning base64 blobs as template text.
let file = ctx.read_file_from_workspace(&path)?;
if file.is_invalid_utf8() {
anyhow::bail!("PR template was not valid UTF-8");
}
let content = file.content.unwrap_or_default();
- Don't accidentally interpret Not-Found as UTF8 errors. Co-authored-by: GPT 5.5 <codex@openai.com>
This happened while trying to push a branch and it seemingly silently skipped doing that and I was very confused.
These changes make it easier to detect the issue.
Also somehow it wanted to pick up a PR template that didn't exist and failed hard because it didn't correctly interpret the return value of a function.
I fixed that, but maybe it causes other issues later if an empty PR template is an issue.
This PR also adds helpers to
but-graphto compute merge-bases directly, passing object-ids instead of segment ids. This helps to avoid redoing expensive computations on the Repository later.