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

Add checks for commits with missing author and time #2771

Merged
merged 3 commits into from
Oct 26, 2017

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented Oct 23, 2017

Can be reproduced in torvalds/linux repo where there are commits without author and/or time and tags that point not to commit but tree

@codecov-io
Copy link

codecov-io commented Oct 23, 2017

Codecov Report

Merging #2771 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2771      +/-   ##
==========================================
- Coverage   27.21%   27.18%   -0.03%     
==========================================
  Files          88       88              
  Lines       17348    17367      +19     
==========================================
  Hits         4721     4721              
- Misses      11942    11961      +19     
  Partials      685      685
Impacted Files Coverage Δ
models/user.go 21.94% <0%> (-0.17%) ⬇️
models/update.go 10.1% <0%> (-0.66%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc84110...483f895. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 23, 2017
@@ -30,7 +32,7 @@
</th>
<th class="nine wide">
</th>
<th class="three wide text grey right age">{{TimeSince .LatestCommit.Author.When $.Lang}}</th>
<th class="three wide text grey right age">{{if .LatestCommit.Author}}{{TimeSince .LatestCommit.Author.When $.Lang}}{{end}}</th>
Copy link
Member

Choose a reason for hiding this comment

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

Missing an "else" here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, if there is no time than we just don't show anything

Copy link
Member

Choose a reason for hiding this comment

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

or show - ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it's needed as it visually won't give any meaningful info

emails[c.Author.Email] = u
} else {
u = v
}
Copy link
Member

Choose a reason for hiding this comment

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

If c.Author is nil, then u will keep its value from the previous iteration. Should we set u to nil if c.Author is nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@lafriks
Copy link
Member Author

lafriks commented Oct 25, 2017

@ethantkoenig fixed

@ethantkoenig
Copy link
Member

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 25, 2017
@lunny
Copy link
Member

lunny commented Oct 26, 2017

How about give a ghost author struct which name is blank? I seached the whole repository, many place assumes commit.Author always not nil. So maybe you have to changed them all.

@lafriks
Copy link
Member Author

lafriks commented Oct 26, 2017

@lunny better not in this PR because it needs to be done than in git library code but that would need to be tested carefully if that does not brake anything

@lunny
Copy link
Member

lunny commented Oct 26, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 26, 2017
@lunny lunny merged commit 6a107e5 into go-gitea:master Oct 26, 2017
@lafriks lafriks deleted the fix/tags_without_author_time branch October 26, 2017 07:45
lafriks added a commit to lafriks-fork/gitea that referenced this pull request Oct 26, 2017
* Add checks for commits with missing author and time

* Fix validate commits with emails if it has no Author
@lafriks lafriks added the backport/done All backports for this PR have been created label Oct 26, 2017
lunny pushed a commit that referenced this pull request Oct 26, 2017
* Add checks for commits with missing author and time

* Fix validate commits with emails if it has no Author
vdbt pushed a commit to vdbt/gitea that referenced this pull request Oct 27, 2017
* Add checks for commits with missing author and time

* Fix validate commits with emails if it has no Author
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants