Abstract hash function usage #28138

AdamMajer · 2023-11-20T15:47:00Z

Refactor Hash interfaces and centralize hash function. This will allow easier introduction of different hash function later on.

This forms the "no-op" part of the SHA256 enablement patch.

delvh · 2023-11-21T00:20:00Z

"no-op".
I mean, it does change the database.
As such, we need a database migration.

cmd/hook.go

AdamMajer · 2023-11-21T12:35:04Z

"no-op". I mean, it does change the database. As such, we need a database migration.

Let's keep a true no-op. We can change database in other commits. I've now pushed a change here that doesn't require a change in database. If you see anything that does, please point it out and I will move it out.

cmd/hook.go

models/git/commit_status.go

models/git/branch_test.go

modules/git/blame.go

AdamMajer · 2023-11-24T14:09:49Z

I've gone over the patch and renamed things as requested and did some further refactoring. Please let me know if I've missed something..

Locally, I've run integration unit tests against mssql container and regular backend unit tests, so hopefully things will pass against other backends.

AdamMajer · 2023-11-24T15:30:11Z

Just out of curiosity, is the test build failure because of some desync on the test end or something else? I can build locally with TAGS="gogit sqlite ..." and also TAGS="nogogit sqlite .." while some of the bots are getting compilation errors. I'm confused.

wxiaoguang · 2023-11-25T03:09:11Z

I can build locally with TAGS="gogit sqlite ..." and also TAGS="nogogit sqlite .." while some of the bots are getting compilation errors. I'm confused.

How do you build locally? What if you introduce some explict errors in the "gogit" files?

The CI still says that the errors are related to gogit.

modules/git/hash.go

services/pull/check.go

lunny · 2023-12-13T07:54:00Z

Wow, what a big refactor. I think https://github.com/go-gitea/gitea/pull/28138/files#r1424969756 is the only one I want to know.

Refactor SHA1 interfaces and centralize the hash function. This will allow easier introduction of different hash function later on.

wxiaoguang · 2023-12-13T10:41:22Z

https://github.com/go-gitea/gitea/blob/main/CONTRIBUTING.md#maintaining-open-prs

The moment you create a non-draft PR or the moment you convert a draft PR to a non-draft PR is the moment code review starts for it.
Once that happens, do not rebase or squash your branch anymore as it makes it difficult to review the new changes.
Merge the base branch into your branch only when you really need to, i.e. because of conflicting changes in the mean time.
This reduces unnecessary CI runs.
Don't worry about merge commits messing up your commit history as every PR will be squash merged.
This means that all changes are joined into a single new commit whose message is as described below.

AdamMajer · 2023-12-13T10:42:05Z

Wow, what a big refactor. I think https://github.com/go-gitea/gitea/pull/28138/files#r1424969756 is the only one I want to know.

This issue should be fixed now. There are plenty of other, tangential refactors that can be done with these changes but these can be done at a smaller pace.

AdamMajer · 2023-12-13T10:47:26Z

Thanks @wxiaoguang , seems I was doing it wrong and re-basing on top of main while I should be adding new commits and merges (when conflicts arise) instead.

* giteaofficial/main: Abstract hash function usage (go-gitea#28138) Add endpoint for not implemented Docker auth (go-gitea#28457) docs: Update group membership fields to match application. (go-gitea#28175)

wxiaoguang · 2023-12-14T05:30:01Z

modules/git/object_format.go

+type ObjectFormatID int
+
+const (
+	Sha1 ObjectFormatID = iota


Why it needs "integer" number?

I think type ObjectFormat string and Sha1 = "sha1" work better?

I think so.

There is another function that does this from string already, at the bottom of the file.

git.ObjectFormatFromString()

Basically, there is no need for this to be int or string in particular, but I wanted to move at least temporarily away from string to catch errors like passing "SHA1" vs. "sha1".

Finally, maybe my prejudice shows, coming from C and C++ world shows, where enums are ints 😄

Refactor Hash interfaces and centralize hash function. This will allow easier introduction of different hash function later on. This forms the "no-op" part of the SHA256 enablement patch.

Currently only SHA1 repositories are supported by Gitea. This adds support for alternate SHA256 with the additional aim of easier support for additional hash types in the future. Fixes: #13794 Limited by: go-git/go-git#899 Depend on: #28138 <img width="776" alt="图片" src="https://github.com/go-gitea/gitea/assets/81045/5448c9a7-608e-4341-a149-5dd0069c9447"> --------- Co-authored-by: Lunny Xiao <[email protected]> Co-authored-by: 6543 <[email protected]>

Refactor Hash interfaces and centralize hash function. This will allow easier introduction of different hash function later on. This forms the "no-op" part of the SHA256 enablement patch.

Currently only SHA1 repositories are supported by Gitea. This adds support for alternate SHA256 with the additional aim of easier support for additional hash types in the future. Fixes: go-gitea#13794 Limited by: go-git/go-git#899 Depend on: go-gitea#28138 <img width="776" alt="图片" src="https://github.com/go-gitea/gitea/assets/81045/5448c9a7-608e-4341-a149-5dd0069c9447"> --------- Co-authored-by: Lunny Xiao <[email protected]> Co-authored-by: 6543 <[email protected]>

Refactor Hash interfaces and centralize hash function. This will allow easier introduction of different hash function later on. This forms the "no-op" part of the SHA256 enablement patch.

Currently only SHA1 repositories are supported by Gitea. This adds support for alternate SHA256 with the additional aim of easier support for additional hash types in the future. Fixes: go-gitea#13794 Limited by: go-git/go-git#899 Depend on: go-gitea#28138 <img width="776" alt="图片" src="https://github.com/go-gitea/gitea/assets/81045/5448c9a7-608e-4341-a149-5dd0069c9447"> --------- Co-authored-by: Lunny Xiao <[email protected]> Co-authored-by: 6543 <[email protected]>

@lunny

There's a miss in #28138: ![image](https://github.com/go-gitea/gitea/assets/18380374/b1e0c5fc-0e6e-44ab-9f6e-34bc8ffbe1cc) https://github.com/go-gitea/gitea/pull/28138/files#diff-2556e62ad7204a230c91927a3f2115e25a2b688240d0ee1de6d34f0277f37dfeR162 @lunny Not sure about the impact of this, but it will only effect 1.22, and maybe we should fix it ASAP. Co-authored-by: KN4CK3R <[email protected]>

* giteaofficial/main: Fix mail template error (go-gitea#29410) Document our issue locking policy (go-gitea#29433) Fix htmx rendering the login page in frame on session logout (go-gitea#29405) Ignore empty repo for CreateRepository in action notifier (go-gitea#29416) Fix incorrect tree path value for patch editor (go-gitea#29377) Fix logic error from go-gitea#28138 (go-gitea#29417)

wxiaoguang · 2024-04-26T01:28:08Z

modules/git/object_id.go

+	_, _ = h.Write([]byte(" "))
+	_, _ = h.Write([]byte(strconv.FormatInt(int64(len(content)), 10)))
+	_, _ = h.Write([]byte{0})
+	return h.HashSum()


Here and below, a regression ..... the "content" is not written into the hasher 😭

-> Fix incorrect object id hash function #30708

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 20, 2023

pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 20, 2023

github-actions bot added modifies/api This PR adds API routes or modifies them modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin labels Nov 20, 2023

AdamMajer force-pushed the sha-agnostic branch from 77b2a5c to a31fba9 Compare November 20, 2023 15:49

AdamMajer mentioned this pull request Nov 20, 2023

Add support for sha256 repositories #23894

Merged

AdamMajer force-pushed the sha-agnostic branch from a31fba9 to 3e22d34 Compare November 20, 2023 16:06

delvh added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Nov 21, 2023

wxiaoguang reviewed Nov 21, 2023

View reviewed changes

cmd/hook.go Outdated Show resolved Hide resolved

wxiaoguang reviewed Nov 21, 2023

View reviewed changes

cmd/hook.go Outdated Show resolved Hide resolved

6543 self-requested a review November 21, 2023 02:48

AdamMajer force-pushed the sha-agnostic branch from 3e22d34 to 38807ae Compare November 21, 2023 12:32

AdamMajer force-pushed the sha-agnostic branch 3 times, most recently from 44bd4fc to 05a23b8 Compare November 21, 2023 15:08

6543 added this to the 1.22.0 milestone Nov 21, 2023

wxiaoguang reviewed Nov 22, 2023

View reviewed changes

cmd/hook.go Show resolved Hide resolved

wxiaoguang reviewed Nov 22, 2023

View reviewed changes

models/git/commit_status.go Outdated Show resolved Hide resolved

wxiaoguang reviewed Nov 22, 2023

View reviewed changes

models/git/branch_test.go Outdated Show resolved Hide resolved

wxiaoguang reviewed Nov 22, 2023

View reviewed changes

modules/git/blame.go Outdated Show resolved Hide resolved

AdamMajer force-pushed the sha-agnostic branch from 05a23b8 to bf9dc4c Compare November 24, 2023 13:58

AdamMajer requested review from wxiaoguang and lunny November 24, 2023 14:04

AdamMajer force-pushed the sha-agnostic branch from bf9dc4c to 29250ba Compare November 24, 2023 14:05

lunny reviewed Nov 25, 2023

View reviewed changes

modules/git/hash.go Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Dec 12, 2023

lunny reviewed Dec 13, 2023

View reviewed changes

services/pull/check.go Outdated Show resolved Hide resolved

Abstract SHA1 function usage

fd090c8

Refactor SHA1 interfaces and centralize the hash function. This will allow easier introduction of different hash function later on.

AdamMajer force-pushed the sha-agnostic branch from c0140fa to fd090c8 Compare December 13, 2023 10:37

lunny approved these changes Dec 13, 2023

View reviewed changes

GiteaBot 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 Dec 13, 2023

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 13, 2023

lunny enabled auto-merge (squash) December 13, 2023 12:51

Merge branch 'main' into sha-agnostic

07323fc

lunny merged commit cbf923e into go-gitea:main Dec 13, 2023
25 checks passed

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 13, 2023

wxiaoguang reviewed Dec 14, 2023

View reviewed changes

yp05327 mentioned this pull request Feb 26, 2024

Fix logic error from #28138 #29417

Merged

go-gitea locked as resolved and limited conversation to collaborators Mar 12, 2024

wxiaoguang reviewed Apr 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract hash function usage #28138

Abstract hash function usage #28138

AdamMajer commented Nov 20, 2023

delvh commented Nov 21, 2023

AdamMajer commented Nov 21, 2023

AdamMajer commented Nov 24, 2023

AdamMajer commented Nov 24, 2023

wxiaoguang commented Nov 25, 2023

lunny commented Dec 13, 2023

wxiaoguang commented Dec 13, 2023

AdamMajer commented Dec 13, 2023

AdamMajer commented Dec 13, 2023

wxiaoguang Dec 14, 2023 •

edited

Loading

lunny Dec 14, 2023

AdamMajer Dec 14, 2023

lunny Dec 14, 2023

wxiaoguang Apr 26, 2024

Abstract hash function usage #28138

Abstract hash function usage #28138

Conversation

AdamMajer commented Nov 20, 2023

delvh commented Nov 21, 2023

AdamMajer commented Nov 21, 2023

AdamMajer commented Nov 24, 2023

AdamMajer commented Nov 24, 2023

wxiaoguang commented Nov 25, 2023

lunny commented Dec 13, 2023

wxiaoguang commented Dec 13, 2023

AdamMajer commented Dec 13, 2023

AdamMajer commented Dec 13, 2023

wxiaoguang Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

lunny Dec 14, 2023

Choose a reason for hiding this comment

AdamMajer Dec 14, 2023

Choose a reason for hiding this comment

lunny Dec 14, 2023

Choose a reason for hiding this comment

wxiaoguang Apr 26, 2024

Choose a reason for hiding this comment

wxiaoguang Dec 14, 2023 •

edited

Loading