-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
File history for renamed files, with '--follow' equivalent to show the complete history #34686
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the PR.
Although it needs more improvements to follow Gitea's code guideline, the idea is good enough.
I can help to make future improvements, before making more changes, I have some questions, see below.
modules/git/commit.go
Outdated
} | ||
|
||
// CommitsCount returns number of total commits of until given revision. | ||
func CommitsCount(ctx context.Context, opts CommitsCountOptions) (int64, error) { | ||
cmd := NewCommand("rev-list", "--count") | ||
cmd := NewCommand("--no-pager", "log", "--pretty=format:%H") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC "rev-list --count" was used intentionally because "log" is much slower (especially on a file with many commits)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I indeed saw commit 58a4407 which made the change, but its comment says --follow is slow, and not git log on its own, so I assumed git log was OK. If not, I'll gladly use rev-list by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If "rev-list --count" works, please use it as much as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modules/git/repo_commit.go
Outdated
@@ -253,7 +264,7 @@ func (repo *Repository) CommitsByFileAndRange(opts CommitsByFileAndRangeOptions) | |||
Stdout: stdoutWriter, | |||
Stderr: &stderr, | |||
}) | |||
if err != nil { | |||
if err != nil && err != io.ErrUnexpectedEOF { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why checking ErrUnexpectedEOF
? When it would happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
git log --pretty=format:%H output lacks a newline at the end, whereas git rev-list doesn't. So that's the fix I found to avoid error 500, please tell me if something better is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think ErrUnexpectedEOF is right, ErrUnexpectedEOF only means something wrong happens, so it needs a clear fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the core issue is in modules/git/repo_commit.go:279-281 :
length := objectFormat.FullLength()
commits := []*Commit{}
shaline := make([]byte, length+1)
From what I understand, shaline is expected to have a length of the hash's length + 1, presumably for the newline. So at line 283 (n, err := io.ReadFull(stdoutReader, shaline)
), when the last output line is reached, only length bytes are read, instead of the expected length + 1 bytes, so it triggers ErrUnexpectedEOF.
If an EOF happens after reading some but not all the bytes, ReadFull returns ErrUnexpectedEOF.
https://pkg.go.dev/io#ReadFull
Co-authored-by: silverwind <[email protected]>
Co-authored-by: silverwind <[email protected]>
The commit history became a mess. Please follow git's manual to maintain the commits, and then force-push after fixing the problems. And one more thing, when review starts and there is no special requirement, please avoid rebase&force-push. |
Fixes #28253
Added a checkbox in the file history, so that we can toggle following renames.
I made a test repo, with a first commit creating a file, and a second one renaming it.
Here is what the history looks.
Rename following disabled :

Rename following enabled :

As a reminder, here's what it looks like now :

However, I have a question: I inserted a small spacing between the text and the checkbox, and as 1em and 1rem looked too big, I put 5px instead. Is it OK to use an absolute size in this context ?