commands: cross-platform log formatting to files #2178
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
technoweenie
approved these changes
Apr 25, 2017
Contributor
technoweenie
left a comment
technoweenie
left a comment
There was a problem hiding this comment.
Looks good. Great description of the fix :)
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Feb 28, 2025
In PR git-lfs#1466 we introduced our custom "errors" package as a replacement for the "errutil" package used previously to provide various error handling functions and specialized error types. As part of this PR, in commit 8624a87 we defined the StackTrace() function in the new "errors" package, and updated the logPanicToWriter() function in our "commands" package to call it when generating an error log file. We write these log files to the .git/lfs/logs directory whenever a Git LFS command invokes one of our error reporting utility functions such as LoggedError() or Panic(). Later, in commit 377366d of PR git-lfs#2178, we revised the logPanicToWriter() function to use the "%+v" format string specifier when writing an error's details to the log file, and dropped the call to the StackTrace() function of our "errors" package. The StackTrace() call was deemed no longer necessary because we expect all of our errors to be created using our "errors" package's functions, which instantiate error structures defined by the legacy "github.com/pkg/errors" package. This package defines its own Format() methods for its structures so that when the "%+v" format string specifier is used with them, they output both the message and stack trace associated with each error. As a consequence of this change in PR git-lfs#2178, the StackTrace() function in our own custom "errors" package has no remaining callers, so we can remove it now.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Mar 5, 2025
In PR git-lfs#1466 we introduced our custom "errors" package as a replacement for the "errutil" package used previously to provide various error handling functions and specialized error types. As part of this PR, in commit 8624a87 we defined the StackTrace() function in the new "errors" package, and updated the logPanicToWriter() function in our "commands" package to call it when generating an error log file. We write these log files to the .git/lfs/logs directory whenever a Git LFS command invokes one of our error reporting utility functions such as LoggedError() or Panic(). Later, in commit 377366d of PR git-lfs#2178, we revised the logPanicToWriter() function to use the "%+v" format string specifier when writing an error's details to the log file, and dropped the call to the StackTrace() function of our "errors" package. The StackTrace() call was deemed no longer necessary because we expect all of our errors to be created using our "errors" package's functions, which instantiate error structures defined by the legacy "github.com/pkg/errors" package. This package defines its own Format() methods for its structures so that when the "%+v" format string specifier is used with them, they output both the message and stack trace associated with each error. As a consequence of this change in PR git-lfs#2178, the StackTrace() function in our own custom "errors" package has no remaining callers, so we can remove it now.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request addresses the second and third points of #2076:
This one required some investigation. I originally thought that the goal here was to replace all instances of printing LF (
\n) characters sent to the terminal, but the actual fix is to just print platform-specific line ending sequences to files on disk, not the terminal.To determine this, I wrote two files to my
~/github/sharedirectory, which is accessible via my Windows VM. Each had the words "foo" and "bar". One was separated by a LF character, and the other by a CRLF, as follows:On Windows, they render in the terminal as expected independent of the line ending sequences (tested via cmd.exe, and reproducible in Git Bash):
So the fix in 43ec8e9 involved:
lineEndingargument to thelogPanicToWriter()func. This function is used to print to bothos.Stderr, as well as*os.Fileinstances, so it needs to be able to handle eitherLForCRLFs.gitLineEndingfunc, which prefers yourcore.autocrlfsetting, or defaults to the platform correct sequence instead.This is a larger problem with my (perhaps) excessive use of error-wrapping, but is easily solved by formatting errors with
%+vinstead of%s. Here's a before and after:%s(old):%+v(new):Closes: #2076.
/cc @git-lfs/core @JarrettR #2076