Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print a notice when delta panics #1917

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

th1000s
Copy link
Collaborator

@th1000s th1000s commented Nov 26, 2024

  • Print a notice when delta panics

Setting DELTA_DEBUG_LOGFILE=crash.log and repeating the command
writes all information needed to reproduce the crash into that file.

  • Add --test-panic

To test the debug handler


The notice reads:

delta panicked and crashed, sorry :(
To quickly repeat the previous command without delta, run 'export GIT_PAGER=less' first. If you want
to report the crash and it is easy to repeat, do so after 'export DELTA_DEBUG_LOGFILE=crash.log'
then submit the logfile to github at <https://github.com/dandavison/delta/issues>. Thank you!

!! Make sure there is NO sensitive information in the log file, it will contain the entire diff !!

Comment on lines +127 to +135
unsafe {
std::env::set_var(RUST_BACKTRACE, "1");
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preparing for the 2024 edition :)

while let Some(Ok(raw_line_bytes)) = lines.next() {
debug_helper.write(raw_line_bytes);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So IIUC, the pattern we're using is

Create the debug_helper = RecordDeltaCall::new(); This causes some default diagnostics to be written.

...
Append additional arbitrary diagnostics via debug_helper.write
...
Terminate the file in the Drop impl for RecordDeltaCall.

But if the DELTA_DEBUG_LOGFILE is not set, then, although we do a few function calls, they are all no-ops.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, write() will usually return after an an easily branch-predicted if.

src/cli.rs Outdated
*/
#[arg(hide = true, long = "test-panic")]
pub test_panic: bool,

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether this commit (Add --test-panic) is more a development commit that we don't need to merge?

Copy link
Owner

@dandavison dandavison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main commit LGTM (PR needs rebasing). I'm inclined to think that we don't really need the second commit in main -- save a little complexity from our code readers's eyes.

Copy link
Collaborator Author

@th1000s th1000s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped the second commit, can be merged.

while let Some(Ok(raw_line_bytes)) = lines.next() {
debug_helper.write(raw_line_bytes);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, write() will usually return after an an easily branch-predicted if.

Comment on lines +76 to +81
write("git config values:\n".into())?;
cfg.for_each(".*", |entry, value: Option<&str>| {
if !(entry.starts_with("user.")
|| entry.starts_with("remote.")
|| entry.starts_with("branch.")
|| entry.starts_with("gui."))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I inverted the logic, now printing all git configs (see git config --get-regexp '.*') except a those listed here.


write(
"<details>\n\
<summary>Input which caused delta to crash</summary>\n```\n\n"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<summary>Input which caused delta to crash</summary>\n```\n\n"
<summary>Input which caused delta to crash</summary>\n\n```\n\n"

In practice I believe the GitHub markdown renderer requires one blank line before the triple-backtick. E.g. compare

Input which caused delta to crash
hello

with

Input which caused delta to crash ``` hello ```

Setting DELTA_DEBUG_LOGFILE=crash.log and repeating the command
writes all information needed to reproduce the crash into that file.
@th1000s
Copy link
Collaborator Author

th1000s commented Jan 12, 2025

The raw lines of course contain the usual escape characters, pasting those into an issue isn't that helpful:

�[33mcommit d10095bc732a7675295d3389152675ccc41e8240�[m�[33m (�[m�[1;31morigin/main�[m�[33m)�[m

This turns the escape chars into there. Can they be recovered by quoting someone? But they might not even be valid UTF-8.

A crash.log and a crash.log.raw might be an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants