From 5d9c64b3feeab32678a983d4256272c010a0e057 Mon Sep 17 00:00:00 2001 From: crystal <71373843+CrystalCommunication@users.noreply.github.com> Date: Wed, 1 Feb 2023 21:51:02 -0700 Subject: [PATCH 01/23] Fix line spacing for plaintext previews (#22699) Adding `
` between each line is not necessary since the entire file is rendered inside a `
`

fixes https://codeberg.org/Codeberg/Community/issues/915
---
 modules/charset/escape.go | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/modules/charset/escape.go b/modules/charset/escape.go
index 3b1c20697793b..5608836a4510e 100644
--- a/modules/charset/escape.go
+++ b/modules/charset/escape.go
@@ -44,7 +44,7 @@ func EscapeControlReader(reader io.Reader, writer io.Writer, locale translation.
 	return streamer.escaped, err
 }
 
-// EscapeControlStringReader escapes the unicode control sequences in a provided reader of string content and writer in a locale and returns the findings as an EscapeStatus and the escaped []byte
+// EscapeControlStringReader escapes the unicode control sequences in a provided reader of string content and writer in a locale and returns the findings as an EscapeStatus and the escaped []byte. HTML line breaks are not inserted after every newline by this method.
 func EscapeControlStringReader(reader io.Reader, writer io.Writer, locale translation.Locale, allowed ...rune) (escaped *EscapeStatus, err error) {
 	bufRd := bufio.NewReader(reader)
 	outputStream := &HTMLStreamerWriter{Writer: writer}
@@ -65,10 +65,6 @@ func EscapeControlStringReader(reader io.Reader, writer io.Writer, locale transl
 			}
 			break
 		}
-		if err := streamer.SelfClosingTag("br"); err != nil {
-			streamer.escaped.HasError = true
-			return streamer.escaped, err
-		}
 	}
 	return streamer.escaped, err
 }

From c46f53a627393766b6a7a4efc10218cbb792e7e3 Mon Sep 17 00:00:00 2001
From: wxiaoguang 
Date: Thu, 2 Feb 2023 13:39:55 +0800
Subject: [PATCH 02/23] Fix diff UI for unexpandable items (#22700)

Follows #21094

Before:

There are 2 problems:

1. Sometimes, the header starts with a number, sometimes, it starts with
an icon button. It makes the UI look like misaligned.
2. The second item's bottom border is too thick (actually, that's an
empty element with border, which should be hidden as well)
3. (An old problem) the number is not mono-font


![image](https://user-images.githubusercontent.com/2114189/215935944-003fe2d3-69bf-413c-bbae-0a4668a508c3.png)


After:

Fix above problems.


![image](https://user-images.githubusercontent.com/2114189/215944811-b867a20c-110c-47a2-aa52-572a8162a44d.png)

---------

Co-authored-by: Lunny Xiao 
Co-authored-by: John Olheiser 
---
 templates/repo/diff/box.tmpl | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl
index f74714499ae54..798a7eae14d1b 100644
--- a/templates/repo/diff/box.tmpl
+++ b/templates/repo/diff/box.tmpl
@@ -76,19 +76,18 @@
 						{{$isImage := or (call $.IsBlobAnImage $blobBase) (call $.IsBlobAnImage $blobHead)}}
 						{{$isCsv := (call $.IsCsvFile $file)}}
 						{{$showFileViewToggle := or $isImage (and (not $file.IsIncomplete) $isCsv)}}
-						
+ {{$isExpandable := or (gt $file.Addition 0) (gt $file.Deletion 0) $file.IsBin}} +

- {{if or (gt $file.Addition 0) (gt $file.Deletion 0) $file.IsBin}} - - {{if $file.ShouldBeHidden}} - {{svg "octicon-chevron-right" 18}} - {{else}} - {{svg "octicon-chevron-down" 18}} - {{end}} - - {{end}} -
+ + {{if $file.ShouldBeHidden}} + {{svg "octicon-chevron-right" 18}} + {{else}} + {{svg "octicon-chevron-down" 18}} + {{end}} + +
{{if $file.IsBin}} {{$.locale.Tr "repo.diff.bin"}} From 9ef8bfb69bea6de663b1fbc595375a1dd78e7e35 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Thu, 2 Feb 2023 15:53:14 +0900 Subject: [PATCH 03/23] set user dashboard org visibility to basic (#22706) Same to https://github.com/go-gitea/gitea/pull/22674 and https://github.com/go-gitea/gitea/pull/22605 Sorry to create 3 PR to fix this. I checked all span with class `org-visibility`, i think this is the last one :) And I found that private/limited user has no private/limited tag in dashboard. but org does. If it is ok i will add this feature in another pr. Co-authored-by: 6543 <6543@obermui.de> Co-authored-by: Lunny Xiao --- templates/user/dashboard/navbar.tmpl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/templates/user/dashboard/navbar.tmpl b/templates/user/dashboard/navbar.tmpl index 4b20bf52680f2..ab3d5029d145d 100644 --- a/templates/user/dashboard/navbar.tmpl +++ b/templates/user/dashboard/navbar.tmpl @@ -7,8 +7,8 @@ {{.ContextUser.ShortName 40}} {{if .ContextUser.IsOrganization}} - {{if .ContextUser.Visibility.IsLimited}}
{{.locale.Tr "org.settings.visibility.limited_shortname"}}
{{end}} - {{if .ContextUser.Visibility.IsPrivate}}
{{.locale.Tr "org.settings.visibility.private_shortname"}}
{{end}} + {{if .ContextUser.Visibility.IsLimited}}
{{.locale.Tr "org.settings.visibility.limited_shortname"}}
{{end}} + {{if .ContextUser.Visibility.IsPrivate}}
{{.locale.Tr "org.settings.visibility.private_shortname"}}
{{end}}
{{end}} {{svg "octicon-triangle-down" 14 "dropdown icon"}} @@ -27,8 +27,8 @@ {{avatar .}} {{.ShortName 40}} - {{if .Visibility.IsLimited}}
{{$.locale.Tr "org.settings.visibility.limited_shortname"}}
{{end}} - {{if .Visibility.IsPrivate}}
{{$.locale.Tr "org.settings.visibility.private_shortname"}}
{{end}} + {{if .Visibility.IsLimited}}
{{$.locale.Tr "org.settings.visibility.limited_shortname"}}
{{end}} + {{if .Visibility.IsPrivate}}
{{$.locale.Tr "org.settings.visibility.private_shortname"}}
{{end}}
{{end}} From 98770d3db8198714789c3182c14950d365ae4fb1 Mon Sep 17 00:00:00 2001 From: Pavel Ezhov Date: Thu, 2 Feb 2023 10:45:00 +0300 Subject: [PATCH 04/23] Fix group filter for ldap source sync (#22506) There are 2 separate flows of creating a user: authentication and source sync. When a group filter is defined, source sync ignores group filter, while authentication respects it. With this PR I've fixed this behavior, so both flows now apply this filter when searching users in LDAP in a unified way. - Unified LDAP group membership lookup for authentication and source sync flows - Replaced custom group membership lookup (used for authentication flow) with an existing listLdapGroupMemberships method (used for source sync flow) - Modified listLdapGroupMemberships and getUserAttributeListedInGroup in a way group lookup could be called separately - Added user filtering based on a group membership for a source sync - Added tests to cover this logic Co-authored-by: Pavel Ezhov Co-authored-by: Lunny Xiao --- services/auth/source/ldap/source_search.go | 137 +++++++++++---------- tests/integration/auth_ldap_test.go | 99 +++++++++++++-- 2 files changed, 159 insertions(+), 77 deletions(-) diff --git a/services/auth/source/ldap/source_search.go b/services/auth/source/ldap/source_search.go index 68ebba391769d..16f13029f9259 100644 --- a/services/auth/source/ldap/source_search.go +++ b/services/auth/source/ldap/source_search.go @@ -196,22 +196,39 @@ func checkRestricted(l *ldap.Conn, ls *Source, userDN string) bool { } // List all group memberships of a user -func (source *Source) listLdapGroupMemberships(l *ldap.Conn, uid string) []string { +func (source *Source) listLdapGroupMemberships(l *ldap.Conn, uid string, applyGroupFilter bool) []string { var ldapGroups []string - groupFilter := fmt.Sprintf("(%s=%s)", source.GroupMemberUID, ldap.EscapeFilter(uid)) + var searchFilter string + + groupFilter, ok := source.sanitizedGroupFilter(source.GroupFilter) + if !ok { + return ldapGroups + } + + groupDN, ok := source.sanitizedGroupDN(source.GroupDN) + if !ok { + return ldapGroups + } + + if applyGroupFilter { + searchFilter = fmt.Sprintf("(&(%s)(%s=%s))", groupFilter, source.GroupMemberUID, ldap.EscapeFilter(uid)) + } else { + searchFilter = fmt.Sprintf("(%s=%s)", source.GroupMemberUID, ldap.EscapeFilter(uid)) + } + result, err := l.Search(ldap.NewSearchRequest( - source.GroupDN, + groupDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, - groupFilter, + searchFilter, []string{}, nil, )) if err != nil { - log.Error("Failed group search using filter[%s]: %v", groupFilter, err) + log.Error("Failed group search in LDAP with filter [%s]: %v", searchFilter, err) return ldapGroups } @@ -238,9 +255,7 @@ func (source *Source) mapLdapGroupsToTeams() map[string]map[string][]string { } // getMappedMemberships : returns the organizations and teams to modify the users membership -func (source *Source) getMappedMemberships(l *ldap.Conn, uid string) (map[string][]string, map[string][]string) { - // get all LDAP group memberships for user - usersLdapGroups := source.listLdapGroupMemberships(l, uid) +func (source *Source) getMappedMemberships(usersLdapGroups []string, uid string) (map[string][]string, map[string][]string) { // unmarshall LDAP group team map from configs ldapGroupsToTeams := source.mapLdapGroupsToTeams() membershipsToAdd := map[string][]string{} @@ -260,6 +275,14 @@ func (source *Source) getMappedMemberships(l *ldap.Conn, uid string) (map[string return membershipsToAdd, membershipsToRemove } +func (source *Source) getUserAttributeListedInGroup(entry *ldap.Entry) string { + if strings.ToLower(source.UserUID) == "dn" { + return entry.DN + } + + return entry.GetAttributeValue(source.UserUID) +} + // SearchEntry : search an LDAP source if an entry (name, passwd) is valid and in the specific filter func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchResult { // See https://tools.ietf.org/search/rfc4513#section-5.1.2 @@ -375,58 +398,30 @@ func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchR firstname := sr.Entries[0].GetAttributeValue(source.AttributeName) surname := sr.Entries[0].GetAttributeValue(source.AttributeSurname) mail := sr.Entries[0].GetAttributeValue(source.AttributeMail) - uid := sr.Entries[0].GetAttributeValue(source.UserUID) - if source.UserUID == "dn" || source.UserUID == "DN" { - uid = sr.Entries[0].DN - } - // Check group membership - if source.GroupsEnabled && source.GroupFilter != "" { - groupFilter, ok := source.sanitizedGroupFilter(source.GroupFilter) - if !ok { - return nil - } - groupDN, ok := source.sanitizedGroupDN(source.GroupDN) - if !ok { - return nil - } + teamsToAdd := make(map[string][]string) + teamsToRemove := make(map[string][]string) - log.Trace("Fetching groups '%v' with filter '%s' and base '%s'", source.GroupMemberUID, groupFilter, groupDN) - groupSearch := ldap.NewSearchRequest( - groupDN, ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false, groupFilter, - []string{source.GroupMemberUID}, - nil) + // Check group membership + if source.GroupsEnabled { + userAttributeListedInGroup := source.getUserAttributeListedInGroup(sr.Entries[0]) + usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, true) - srg, err := l.Search(groupSearch) - if err != nil { - log.Error("LDAP group search failed: %v", err) - return nil - } else if len(srg.Entries) < 1 { - log.Error("LDAP group search failed: 0 entries") + if source.GroupFilter != "" && len(usersLdapGroups) == 0 { return nil } - isMember := false - Entries: - for _, group := range srg.Entries { - for _, member := range group.GetAttributeValues(source.GroupMemberUID) { - if (source.UserUID == "dn" && member == sr.Entries[0].DN) || member == uid { - isMember = true - break Entries - } - } - } - - if !isMember { - log.Error("LDAP group membership test failed") - return nil + if source.GroupTeamMap != "" || source.GroupTeamMapRemoval { + teamsToAdd, teamsToRemove = source.getMappedMemberships(usersLdapGroups, userAttributeListedInGroup) } } if isAttributeSSHPublicKeySet { sshPublicKey = sr.Entries[0].GetAttributeValues(source.AttributeSSHPublicKey) } + isAdmin := checkAdmin(l, source, userDN) + var isRestricted bool if !isAdmin { isRestricted = checkRestricted(l, source, userDN) @@ -436,12 +431,6 @@ func (source *Source) SearchEntry(name, passwd string, directBind bool) *SearchR Avatar = sr.Entries[0].GetRawAttributeValue(source.AttributeAvatar) } - teamsToAdd := make(map[string][]string) - teamsToRemove := make(map[string][]string) - if source.GroupsEnabled && (source.GroupTeamMap != "" || source.GroupTeamMapRemoval) { - teamsToAdd, teamsToRemove = source.getMappedMemberships(l, uid) - } - if !directBind && source.AttributesInBind { // binds user (checking password) after looking-up attributes in BindDN context err = bindUser(l, userDN, passwd) @@ -520,19 +509,29 @@ func (source *Source) SearchEntries() ([]*SearchResult, error) { return nil, err } - result := make([]*SearchResult, len(sr.Entries)) + result := make([]*SearchResult, 0, len(sr.Entries)) - for i, v := range sr.Entries { + for _, v := range sr.Entries { teamsToAdd := make(map[string][]string) teamsToRemove := make(map[string][]string) - if source.GroupsEnabled && (source.GroupTeamMap != "" || source.GroupTeamMapRemoval) { - userAttributeListedInGroup := v.GetAttributeValue(source.UserUID) - if source.UserUID == "dn" || source.UserUID == "DN" { - userAttributeListedInGroup = v.DN + + if source.GroupsEnabled { + userAttributeListedInGroup := source.getUserAttributeListedInGroup(v) + + if source.GroupFilter != "" { + usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, true) + if len(usersLdapGroups) == 0 { + continue + } + } + + if source.GroupTeamMap != "" || source.GroupTeamMapRemoval { + usersLdapGroups := source.listLdapGroupMemberships(l, userAttributeListedInGroup, false) + teamsToAdd, teamsToRemove = source.getMappedMemberships(usersLdapGroups, userAttributeListedInGroup) } - teamsToAdd, teamsToRemove = source.getMappedMemberships(l, userAttributeListedInGroup) } - result[i] = &SearchResult{ + + user := &SearchResult{ Username: v.GetAttributeValue(source.AttributeUsername), Name: v.GetAttributeValue(source.AttributeName), Surname: v.GetAttributeValue(source.AttributeSurname), @@ -541,16 +540,22 @@ func (source *Source) SearchEntries() ([]*SearchResult, error) { LdapTeamAdd: teamsToAdd, LdapTeamRemove: teamsToRemove, } - if !result[i].IsAdmin { - result[i].IsRestricted = checkRestricted(l, source, v.DN) + + if !user.IsAdmin { + user.IsRestricted = checkRestricted(l, source, v.DN) } + if isAttributeSSHPublicKeySet { - result[i].SSHPublicKey = v.GetAttributeValues(source.AttributeSSHPublicKey) + user.SSHPublicKey = v.GetAttributeValues(source.AttributeSSHPublicKey) } + if isAtributeAvatarSet { - result[i].Avatar = v.GetRawAttributeValue(source.AttributeAvatar) + user.Avatar = v.GetRawAttributeValue(source.AttributeAvatar) } - result[i].LowerName = strings.ToLower(result[i].Username) + + user.LowerName = strings.ToLower(user.Username) + + result = append(result, user) } return result, nil diff --git a/tests/integration/auth_ldap_test.go b/tests/integration/auth_ldap_test.go index 88571ecd614e4..9dfa4c73d83ea 100644 --- a/tests/integration/auth_ldap_test.go +++ b/tests/integration/auth_ldap_test.go @@ -11,12 +11,14 @@ import ( "testing" "code.gitea.io/gitea/models" + auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/translation" "code.gitea.io/gitea/services/auth" + "code.gitea.io/gitea/services/auth/source/ldap" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -102,13 +104,28 @@ func getLDAPServerHost() string { return host } -func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string, groupMapParams ...string) { +func getLDAPServerPort() string { + port := os.Getenv("TEST_LDAP_PORT") + if len(port) == 0 { + port = "389" + } + return port +} + +func addAuthSourceLDAP(t *testing.T, sshKeyAttribute, groupFilter string, groupMapParams ...string) { groupTeamMapRemoval := "off" groupTeamMap := "" if len(groupMapParams) == 2 { groupTeamMapRemoval = groupMapParams[0] groupTeamMap = groupMapParams[1] } + + // Modify user filter to test group filter explicitly + userFilter := "(&(objectClass=inetOrgPerson)(memberOf=cn=git,ou=people,dc=planetexpress,dc=com)(uid=%s))" + if groupFilter != "" { + userFilter = "(&(objectClass=inetOrgPerson)(uid=%s))" + } + session := loginUser(t, "user1") csrf := GetCSRF(t, session, "/admin/auths/new") req := NewRequestWithValues(t, "POST", "/admin/auths/new", map[string]string{ @@ -116,11 +133,11 @@ func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string, groupMapParams ...s "type": "2", "name": "ldap", "host": getLDAPServerHost(), - "port": "389", + "port": getLDAPServerPort(), "bind_dn": "uid=gitea,ou=service,dc=planetexpress,dc=com", "bind_password": "password", "user_base": "ou=people,dc=planetexpress,dc=com", - "filter": "(&(objectClass=inetOrgPerson)(memberOf=cn=git,ou=people,dc=planetexpress,dc=com)(uid=%s))", + "filter": userFilter, "admin_filter": "(memberOf=cn=admin_staff,ou=people,dc=planetexpress,dc=com)", "restricted_filter": "(uid=leela)", "attribute_username": "uid", @@ -133,6 +150,7 @@ func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string, groupMapParams ...s "groups_enabled": "on", "group_dn": "ou=people,dc=planetexpress,dc=com", "group_member_uid": "member", + "group_filter": groupFilter, "group_team_map": groupTeamMap, "group_team_map_removal": groupTeamMapRemoval, "user_uid": "DN", @@ -146,7 +164,7 @@ func TestLDAPUserSignin(t *testing.T) { return } defer tests.PrepareTestEnv(t)() - addAuthSourceLDAP(t, "") + addAuthSourceLDAP(t, "", "") u := gitLDAPUsers[0] @@ -163,7 +181,7 @@ func TestLDAPUserSignin(t *testing.T) { func TestLDAPAuthChange(t *testing.T) { defer tests.PrepareTestEnv(t)() - addAuthSourceLDAP(t, "") + addAuthSourceLDAP(t, "", "") session := loginUser(t, "user1") req := NewRequest(t, "GET", "/admin/auths") @@ -221,7 +239,7 @@ func TestLDAPUserSync(t *testing.T) { return } defer tests.PrepareTestEnv(t)() - addAuthSourceLDAP(t, "") + addAuthSourceLDAP(t, "", "") auth.SyncExternalUsers(context.Background(), true) session := loginUser(t, "user1") @@ -266,13 +284,72 @@ func TestLDAPUserSync(t *testing.T) { } } +func TestLDAPUserSyncWithGroupFilter(t *testing.T) { + if skipLDAPTests() { + t.Skip() + return + } + defer tests.PrepareTestEnv(t)() + addAuthSourceLDAP(t, "", "(cn=git)") + + // Assert a user not a member of the LDAP group "cn=git" cannot login + // This test may look like TestLDAPUserSigninFailed but it is not. + // The later test uses user filter containing group membership filter (memberOf) + // This test is for the case when LDAP user records may not be linked with + // all groups the user is a member of, the user filter is modified accordingly inside + // the addAuthSourceLDAP based on the value of the groupFilter + u := otherLDAPUsers[0] + testLoginFailed(t, u.UserName, u.Password, translation.NewLocale("en-US").Tr("form.username_password_incorrect")) + + auth.SyncExternalUsers(context.Background(), true) + + // Assert members of LDAP group "cn=git" are added + for _, gitLDAPUser := range gitLDAPUsers { + unittest.BeanExists(t, &user_model.User{ + Name: gitLDAPUser.UserName, + }) + } + + // Assert everyone else is not added + for _, gitLDAPUser := range otherLDAPUsers { + unittest.AssertNotExistsBean(t, &user_model.User{ + Name: gitLDAPUser.UserName, + }) + } + + ldapSource := unittest.AssertExistsAndLoadBean(t, &auth_model.Source{ + Name: "ldap", + }) + ldapConfig := ldapSource.Cfg.(*ldap.Source) + ldapConfig.GroupFilter = "(cn=ship_crew)" + auth_model.UpdateSource(ldapSource) + + auth.SyncExternalUsers(context.Background(), true) + + for _, gitLDAPUser := range gitLDAPUsers { + if gitLDAPUser.UserName == "fry" || gitLDAPUser.UserName == "leela" || gitLDAPUser.UserName == "bender" { + // Assert members of the LDAP group "cn-ship_crew" are still active + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ + Name: gitLDAPUser.UserName, + }) + assert.True(t, user.IsActive, "User %s should be active", gitLDAPUser.UserName) + } else { + // Assert everyone else is inactive + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ + Name: gitLDAPUser.UserName, + }) + assert.False(t, user.IsActive, "User %s should be inactive", gitLDAPUser.UserName) + } + } +} + func TestLDAPUserSigninFailed(t *testing.T) { if skipLDAPTests() { t.Skip() return } defer tests.PrepareTestEnv(t)() - addAuthSourceLDAP(t, "") + addAuthSourceLDAP(t, "", "") u := otherLDAPUsers[0] testLoginFailed(t, u.UserName, u.Password, translation.NewLocale("en-US").Tr("form.username_password_incorrect")) @@ -284,7 +361,7 @@ func TestLDAPUserSSHKeySync(t *testing.T) { return } defer tests.PrepareTestEnv(t)() - addAuthSourceLDAP(t, "sshPublicKey") + addAuthSourceLDAP(t, "sshPublicKey", "") auth.SyncExternalUsers(context.Background(), true) @@ -317,7 +394,7 @@ func TestLDAPGroupTeamSyncAddMember(t *testing.T) { return } defer tests.PrepareTestEnv(t)() - addAuthSourceLDAP(t, "", "on", `{"cn=ship_crew,ou=people,dc=planetexpress,dc=com":{"org26": ["team11"]},"cn=admin_staff,ou=people,dc=planetexpress,dc=com": {"non-existent": ["non-existent"]}}`) + addAuthSourceLDAP(t, "", "", "on", `{"cn=ship_crew,ou=people,dc=planetexpress,dc=com":{"org26": ["team11"]},"cn=admin_staff,ou=people,dc=planetexpress,dc=com": {"non-existent": ["non-existent"]}}`) org, err := organization.GetOrgByName("org26") assert.NoError(t, err) team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11") @@ -362,7 +439,7 @@ func TestLDAPGroupTeamSyncRemoveMember(t *testing.T) { return } defer tests.PrepareTestEnv(t)() - addAuthSourceLDAP(t, "", "on", `{"cn=dispatch,ou=people,dc=planetexpress,dc=com": {"org26": ["team11"]}}`) + addAuthSourceLDAP(t, "", "", "on", `{"cn=dispatch,ou=people,dc=planetexpress,dc=com": {"org26": ["team11"]}}`) org, err := organization.GetOrgByName("org26") assert.NoError(t, err) team, err := organization.GetTeam(db.DefaultContext, org.ID, "team11") @@ -398,7 +475,7 @@ func TestBrokenLDAPMapUserSignin(t *testing.T) { return } defer tests.PrepareTestEnv(t)() - addAuthSourceLDAP(t, "", "on", `{"NOT_A_VALID_JSON"["MISSING_DOUBLE_POINT"]}`) + addAuthSourceLDAP(t, "", "", "on", `{"NOT_A_VALID_JSON"["MISSING_DOUBLE_POINT"]}`) u := gitLDAPUsers[0] From 368d43643f8f8ed1bfb1462d5cae586c90e93383 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 2 Feb 2023 20:40:08 +0800 Subject: [PATCH 05/23] Fix actions workflow branches match bug (#22724) caused by #22680 `pushPayload.Ref` and `prPayload.PullRequest.Base.Ref` have the format like `refs/heads/`, so we need to trim the prefix before comparing. --- modules/actions/workflows.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index c1c8e71f53b5c..7f0e6e456436b 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -75,7 +75,6 @@ func DetectWorkflows(commit *git.Commit, triggedEvent webhook_module.HookEventTy if evt.Name != triggedEvent.Event() { continue } - if detectMatched(commit, triggedEvent, payload, evt) { workflows[entry.Name()] = content } @@ -105,8 +104,9 @@ func detectMatched(commit *git.Commit, triggedEvent webhook_module.HookEventType for cond, vals := range evt.Acts { switch cond { case "branches", "tags": + refShortName := git.RefName(pushPayload.Ref).ShortName() for _, val := range vals { - if glob.MustCompile(val, '/').Match(pushPayload.Ref) { + if glob.MustCompile(val, '/').Match(refShortName) { matchTimes++ break } @@ -160,8 +160,9 @@ func detectMatched(commit *git.Commit, triggedEvent webhook_module.HookEventType } } case "branches": + refShortName := git.RefName(prPayload.PullRequest.Base.Ref).ShortName() for _, val := range vals { - if glob.MustCompile(val, '/').Match(prPayload.PullRequest.Base.Ref) { + if glob.MustCompile(val, '/').Match(refShortName) { matchTimes++ break } From ccb38512818dd3ee86f7960ed6cdf34754e4d09f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 3 Feb 2023 01:39:38 +0800 Subject: [PATCH 06/23] Add some comments for recent code (#22725) When using the main branch, I found that some changed code didn't have comments. This PR adds some comments. --- models/dbfs/dbfs.go | 29 +++++++++++++++++++++++++++++ modules/httpcache/httpcache.go | 2 ++ tests/mysql.ini.tmpl | 1 + 3 files changed, 32 insertions(+) diff --git a/models/dbfs/dbfs.go b/models/dbfs/dbfs.go index 89d3dc7cbc911..6b5b3beeb2745 100644 --- a/models/dbfs/dbfs.go +++ b/models/dbfs/dbfs.go @@ -10,6 +10,35 @@ import ( "code.gitea.io/gitea/models/db" ) +/* +The reasons behind the DBFS (database-filesystem) package: +When a Gitea action is running, the Gitea action server should collect and store all the logs. + +The requirements are: +* The running logs must be stored across the cluster if the Gitea servers are deployed as a cluster. +* The logs will be archived to Object Storage (S3/MinIO, etc.) after a period of time. +* The Gitea action UI should be able to render the running logs and the archived logs. + +Some possible solutions for the running logs: +* [Not ideal] Using local temp file: it can not be shared across the cluster. +* [Not ideal] Using shared file in the filesystem of git repository: although at the moment, the Gitea cluster's + git repositories must be stored in a shared filesystem, in the future, Gitea may need a dedicated Git Service Server + to decouple the shared filesystem. Then the action logs will become a blocker. +* [Not ideal] Record the logs in a database table line by line: it has a couple of problems: + - It's difficult to make multiple increasing sequence (log line number) for different databases. + - The database table will have a lot of rows and be affected by the big-table performance problem. + - It's difficult to load logs by using the same interface as other storages. + - It's difficult to calculate the size of the logs. + +The DBFS solution: +* It can be used in a cluster. +* It can share the same interface (Read/Write/Seek) as other storages. +* It's very friendly to database because it only needs to store much fewer rows than the log-line solution. +* In the future, when Gitea action needs to limit the log size (other CI/CD services also do so), it's easier to calculate the log file size. +* Even sometimes the UI needs to render the tailing lines, the tailing lines can be found be counting the "\n" from the end of the file by seek. + The seeking and finding is not the fastest way, but it's still acceptable and won't affect the performance too much. +*/ + type dbfsMeta struct { ID int64 `xorm:"pk autoincr"` FullPath string `xorm:"VARCHAR(500) UNIQUE NOT NULL"` diff --git a/modules/httpcache/httpcache.go b/modules/httpcache/httpcache.go index d7d9ce0b7ebf9..f0caa30eb82b3 100644 --- a/modules/httpcache/httpcache.go +++ b/modules/httpcache/httpcache.go @@ -19,6 +19,8 @@ import ( func AddCacheControlToHeader(h http.Header, maxAge time.Duration, additionalDirectives ...string) { directives := make([]string, 0, 2+len(additionalDirectives)) + // "max-age=0 + must-revalidate" (aka "no-cache") is preferred instead of "no-store" + // because browsers may restore some input fields after navigate-back / reload a page. if setting.IsProd { if maxAge == 0 { directives = append(directives, "max-age=0", "private", "must-revalidate") diff --git a/tests/mysql.ini.tmpl b/tests/mysql.ini.tmpl index b7ed98c34e91c..1dd7bfab2a49c 100644 --- a/tests/mysql.ini.tmpl +++ b/tests/mysql.ini.tmpl @@ -126,6 +126,7 @@ INTERNAL_TOKEN = eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYmYiOjE0OTU1NTE2MTh9.h ENABLED = true [email.incoming] +; temporarily disabled because the incoming mail tests are flaky due to the IMAP server (during integration tests) couldn't be not ready in time sometimes. ENABLED = false HOST = smtpimap PORT = 993 From 2914c5299b37c3f98997fc923b0b715c9b3f750a Mon Sep 17 00:00:00 2001 From: zeripath Date: Thu, 2 Feb 2023 18:25:54 +0000 Subject: [PATCH 07/23] Improve error report when user passes a private key (#22726) The error reported when a user passes a private ssh key as their ssh public key is not very nice. This PR improves this slightly. Ref #22693 Signed-off-by: Andrew Thornton Co-authored-by: delvh --- models/asymkey/error.go | 3 +++ models/asymkey/ssh_key_parse.go | 3 +++ options/locale/locale_en-US.ini | 1 + routers/web/repo/setting.go | 4 ++++ routers/web/user/setting/keys.go | 2 ++ 5 files changed, 13 insertions(+) diff --git a/models/asymkey/error.go b/models/asymkey/error.go index 1d486082f4610..03bc82302f100 100644 --- a/models/asymkey/error.go +++ b/models/asymkey/error.go @@ -24,6 +24,9 @@ func (err ErrKeyUnableVerify) Error() string { return fmt.Sprintf("Unable to verify key content [result: %s]", err.Result) } +// ErrKeyIsPrivate is returned when the provided key is a private key not a public key +var ErrKeyIsPrivate = util.NewSilentWrapErrorf(util.ErrInvalidArgument, "the provided key is a private key") + // ErrKeyNotExist represents a "KeyNotExist" kind of error. type ErrKeyNotExist struct { ID int64 diff --git a/models/asymkey/ssh_key_parse.go b/models/asymkey/ssh_key_parse.go index 1df6db6fa7219..8693c87e76b2d 100644 --- a/models/asymkey/ssh_key_parse.go +++ b/models/asymkey/ssh_key_parse.go @@ -96,6 +96,9 @@ func parseKeyString(content string) (string, error) { if block == nil { return "", fmt.Errorf("failed to parse PEM block containing the public key") } + if strings.Contains(block.Type, "PRIVATE") { + return "", ErrKeyIsPrivate + } pub, err := x509.ParsePKIXPublicKey(block.Bytes) if err != nil { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 8465660cc0756..26217293a5efa 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -518,6 +518,7 @@ organization_leave_success = You have successfully left the organization %s. invalid_ssh_key = Cannot verify your SSH key: %s invalid_gpg_key = Cannot verify your GPG key: %s invalid_ssh_principal = Invalid principal: %s +must_use_public_key = The key you provided is a private key. Please do not upload your private key anywhere. Use your public key instead. unable_verify_ssh_key = "Cannot verify the SSH key; double-check it for mistakes." auth_failed = Authentication failed: %v diff --git a/routers/web/repo/setting.go b/routers/web/repo/setting.go index da52957548be8..2cc263e5bbfd3 100644 --- a/routers/web/repo/setting.go +++ b/routers/web/repo/setting.go @@ -1158,6 +1158,10 @@ func DeployKeysPost(ctx *context.Context) { ctx.Flash.Info(ctx.Tr("settings.ssh_disabled")) } else if asymkey_model.IsErrKeyUnableVerify(err) { ctx.Flash.Info(ctx.Tr("form.unable_verify_ssh_key")) + } else if err == asymkey_model.ErrKeyIsPrivate { + ctx.Data["HasError"] = true + ctx.Data["Err_Content"] = true + ctx.Flash.Error(ctx.Tr("form.must_use_public_key")) } else { ctx.Data["HasError"] = true ctx.Data["Err_Content"] = true diff --git a/routers/web/user/setting/keys.go b/routers/web/user/setting/keys.go index 0ecc39ecd17ed..6debf95bbce06 100644 --- a/routers/web/user/setting/keys.go +++ b/routers/web/user/setting/keys.go @@ -159,6 +159,8 @@ func KeysPost(ctx *context.Context) { ctx.Flash.Info(ctx.Tr("settings.ssh_disabled")) } else if asymkey_model.IsErrKeyUnableVerify(err) { ctx.Flash.Info(ctx.Tr("form.unable_verify_ssh_key")) + } else if err == asymkey_model.ErrKeyIsPrivate { + ctx.Flash.Error(ctx.Tr("form.must_use_public_key")) } else { ctx.Flash.Error(ctx.Tr("form.invalid_ssh_key", err.Error())) } From 82728a7cecfe4d254890545fea49d6afde4f40db Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 3 Feb 2023 04:48:48 +0800 Subject: [PATCH 08/23] Do not overwrite empty DefaultBranch (#22708) Fix #21994. And fix #19470. While generating new repo from a template, it does something like "commit to git repo, re-fetch repo model from DB, and update default branch if it's empty". https://github.com/go-gitea/gitea/blob/19d5b2f922c2defde579a935fbedb680eb8fff18/modules/repository/generate.go#L241-L253 Unfortunately, when load repo from DB, the default branch will be set to `setting.Repository.DefaultBranch` if it's empty: https://github.com/go-gitea/gitea/blob/19d5b2f922c2defde579a935fbedb680eb8fff18/models/repo/repo.go#L228-L233 I believe it's a very old temporary patch but has been kept for many years, see: [2d2d85bb](https://github.com/go-gitea/gitea/commit/2d2d85bb#diff-1851799b06733db4df3ec74385c1e8850ee5aedee70b8b55366910d22725eea8) I know it's a risk to delete it, may lead to potential behavioral changes, but we cannot keep the outdated `FIXME` forever. On the other hand, an empty `DefaultBranch` does make sense: an empty repo doesn't have one conceptually (actually, Gitea will still set it to `setting.Repository.DefaultBranch` to make it safer). --- models/fixtures/repository.yml | 26 ++++++++++++++++++++++++++ models/repo/repo.go | 5 ----- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/models/fixtures/repository.yml b/models/fixtures/repository.yml index 5d4781ad44f7c..ef3cfbbbec7c5 100644 --- a/models/fixtures/repository.yml +++ b/models/fixtures/repository.yml @@ -4,6 +4,7 @@ owner_name: user2 lower_name: repo1 name: repo1 + default_branch: master num_watches: 4 num_stars: 0 num_forks: 0 @@ -34,6 +35,7 @@ owner_name: user2 lower_name: repo2 name: repo2 + default_branch: master num_watches: 0 num_stars: 1 num_forks: 0 @@ -64,6 +66,7 @@ owner_name: user3 lower_name: repo3 name: repo3 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -94,6 +97,7 @@ owner_name: user5 lower_name: repo4 name: repo4 + default_branch: master num_watches: 0 num_stars: 1 num_forks: 0 @@ -274,6 +278,7 @@ owner_name: user12 lower_name: repo10 name: repo10 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 1 @@ -304,6 +309,7 @@ owner_name: user13 lower_name: repo11 name: repo11 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -425,6 +431,7 @@ owner_name: user2 lower_name: repo15 name: repo15 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -455,6 +462,7 @@ owner_name: user2 lower_name: repo16 name: repo16 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -905,6 +913,7 @@ owner_name: user2 lower_name: repo20 name: repo20 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -965,6 +974,7 @@ owner_name: user2 lower_name: utf8 name: utf8 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1055,6 +1065,7 @@ owner_name: user2 lower_name: commits_search_test name: commits_search_test + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1085,6 +1096,7 @@ owner_name: user2 lower_name: git_hooks_test name: git_hooks_test + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1115,6 +1127,7 @@ owner_name: limited_org lower_name: public_repo_on_limited_org name: public_repo_on_limited_org + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1145,6 +1158,7 @@ owner_name: limited_org lower_name: private_repo_on_limited_org name: private_repo_on_limited_org + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1175,6 +1189,7 @@ owner_name: privated_org lower_name: public_repo_on_private_org name: public_repo_on_private_org + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1205,6 +1220,7 @@ owner_name: privated_org lower_name: private_repo_on_private_org name: private_repo_on_private_org + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1235,6 +1251,7 @@ owner_name: user2 lower_name: glob name: glob + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1295,6 +1312,7 @@ owner_name: user27 lower_name: template1 name: template1 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1355,6 +1373,7 @@ owner_name: org26 lower_name: repo_external_tracker name: repo_external_tracker + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1385,6 +1404,7 @@ owner_name: org26 lower_name: repo_external_tracker_numeric name: repo_external_tracker_numeric + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1415,6 +1435,7 @@ owner_name: org26 lower_name: repo_external_tracker_alpha name: repo_external_tracker_alpha + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1445,6 +1466,7 @@ owner_name: user27 lower_name: repo49 name: repo49 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1475,6 +1497,7 @@ owner_name: user30 lower_name: repo50 name: repo50 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1505,6 +1528,7 @@ owner_name: user30 lower_name: repo51 name: repo51 + default_branch: master num_watches: 0 num_stars: 0 num_forks: 0 @@ -1565,6 +1589,7 @@ owner_name: user30 lower_name: renderer name: renderer + default_branch: master is_archived: false is_empty: false is_private: false @@ -1592,6 +1617,7 @@ owner_name: user2 lower_name: lfs name: lfs + default_branch: master is_empty: false is_archived: false is_private: true diff --git a/models/repo/repo.go b/models/repo/repo.go index 831eb22dc5a50..06ec34ed631aa 100644 --- a/models/repo/repo.go +++ b/models/repo/repo.go @@ -227,11 +227,6 @@ func (repo *Repository) IsBroken() bool { // AfterLoad is invoked from XORM after setting the values of all fields of this object. func (repo *Repository) AfterLoad() { - // FIXME: use models migration to solve all at once. - if len(repo.DefaultBranch) == 0 { - repo.DefaultBranch = setting.Repository.DefaultBranch - } - repo.NumOpenIssues = repo.NumIssues - repo.NumClosedIssues repo.NumOpenPulls = repo.NumPulls - repo.NumClosedPulls repo.NumOpenMilestones = repo.NumMilestones - repo.NumClosedMilestones From 891391689a26e0bc3dcb1558512d3c2b6857232d Mon Sep 17 00:00:00 2001 From: jladbrook Date: Fri, 3 Feb 2023 06:24:45 +0000 Subject: [PATCH 09/23] Update button is shown when a Pull Request is marked WIP - Issue #21740 (#22683) Fix #21740. Updated the Pull Request template so that the 'Update branch by merge' button is visible for WIP PR's. Making the behaviour match a non WIP-PR. Previous WIP page with changes pending on the branch: ![image](https://user-images.githubusercontent.com/1656302/215738307-e68a2f92-5ff8-4f48-a541-35ca81d1f1a4.png) Updated UI adding the update button: ![image](https://user-images.githubusercontent.com/1656302/215737872-e0e9d712-b7aa-4b90-b7ed-6a92a14fc182.png) ## Notes * have not removed the **$canAutoMerge** variable from the pull.tmpl on this [line](https://github.com/go-gitea/gitea/blob/36dc11869d0401b796a7a3f74627fec842a4a89a/templates/repo/issue/view_content/pull.tmpl#L131) - doesn't appear to be used elsewhere but wasn't sure * In order to avoid duplicating code corresponding UI code was added to a new tmpl file, ```update_branch_by_merge.tmpl``` and is called in two places from ```pull.tmpl```. --------- Signed-off-by: Andrew Thornton Co-authored-by: Andrew Thornton Co-authored-by: Lunny Xiao --- templates/repo/issue/view_content/pull.tmpl | 40 +------------------ .../view_content/update_branch_by_merge.tmpl | 39 ++++++++++++++++++ 2 files changed, 41 insertions(+), 38 deletions(-) create mode 100644 templates/repo/issue/view_content/update_branch_by_merge.tmpl diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 1f94001db06eb..dc671eb6d6706 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -190,6 +190,7 @@ {{end}}
+ {{template "repo/issue/view_content/update_branch_by_merge" (dict "locale" .locale "Issue" .Issue "UpdateAllowed" .UpdateAllowed "UpdateByRebaseAllowed" .UpdateByRebaseAllowed "Link" .Link)}} {{else if .Issue.PullRequest.IsChecking}}
{{svg "octicon-sync"}} @@ -282,44 +283,7 @@
{{end}} {{end}} - {{if and (gt .Issue.PullRequest.CommitsBehind 0) (not .Issue.IsClosed) (not .Issue.PullRequest.IsChecking) (not .IsPullFilesConflicted) (not .IsPullRequestBroken) (not $canAutoMerge)}} -
-
-
- {{svg "octicon-alert"}} - {{$.locale.Tr "repo.pulls.outdated_with_base_branch"}} -
-
- {{if and .UpdateAllowed .UpdateByRebaseAllowed}} -
-
- - - -
-
- {{end}} - {{if and .UpdateAllowed (not .UpdateByRebaseAllowed)}} -
- {{.CsrfTokenHtml}} - -
- {{end}} -
-
- {{end}} + {{template "repo/issue/view_content/update_branch_by_merge" (dict "locale" .locale "Issue" .Issue "UpdateAllowed" .UpdateAllowed "UpdateByRebaseAllowed" .UpdateByRebaseAllowed "Link" .Link)}} {{if .Issue.PullRequest.IsEmpty}}
diff --git a/templates/repo/issue/view_content/update_branch_by_merge.tmpl b/templates/repo/issue/view_content/update_branch_by_merge.tmpl new file mode 100644 index 0000000000000..3bc8dcca9752e --- /dev/null +++ b/templates/repo/issue/view_content/update_branch_by_merge.tmpl @@ -0,0 +1,39 @@ +{{$canAutoMerge := false}} +{{if and (gt .Issue.PullRequest.CommitsBehind 0) (not .Issue.IsClosed) (not .Issue.PullRequest.IsChecking) (not .IsPullFilesConflicted) (not .IsPullRequestBroken) (not $canAutoMerge)}} +
+
+
+ {{svg "octicon-alert"}} + {{$.locale.Tr "repo.pulls.outdated_with_base_branch"}} +
+
+ {{if and .UpdateAllowed .UpdateByRebaseAllowed}} +
+
+ + + +
+
+ {{end}} + {{if and .UpdateAllowed (not .UpdateByRebaseAllowed)}} +
+ {{.CsrfTokenHtml}} + +
+ {{end}} +
+
+{{end}} From 1410e13dc51030340e280b4637aeafa52defb359 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 3 Feb 2023 18:37:25 +0800 Subject: [PATCH 10/23] Add missed reverse proxy authentication documentation (#22250) Co-authored-by: KN4CK3R Co-authored-by: Jason Song --- .../doc/features/authentication.en-us.md | 19 +++++++++++++++++++ .../doc/features/authentication.zh-cn.md | 19 ++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/docs/content/doc/features/authentication.en-us.md b/docs/content/doc/features/authentication.en-us.md index f25065d9c48db..c27a09b00bf32 100644 --- a/docs/content/doc/features/authentication.en-us.md +++ b/docs/content/doc/features/authentication.en-us.md @@ -329,3 +329,22 @@ Before activating SSPI single sign-on authentication (SSO) you have to prepare y - You have added the URL of the web app to the `Local intranet zone` - The clocks of the server and client should not differ with more than 5 minutes (depends on group policy) - `Integrated Windows Authentication` should be enabled in Internet Explorer (under `Advanced settings`) + +## Reverse Proxy + +Gitea supports Reverse Proxy Header authentication, it will read headers as a trusted login user name or user email address. This hasn't been enabled by default, you can enable it with + +```ini +[service] +ENABLE_REVERSE_PROXY_AUTHENTICATION = true +``` + +The default login user name is in the `X-WEBAUTH-USER` header, you can change it via changing `REVERSE_PROXY_AUTHENTICATION_USER` in app.ini. If the user doesn't exist, you can enable automatic registration with `ENABLE_REVERSE_PROXY_AUTO_REGISTRATION=true`. + +The default login user email is `X-WEBAUTH-EMAIL`, you can change it via changing `REVERSE_PROXY_AUTHENTICATION_EMAIL` in app.ini, this could also be disabled with `ENABLE_REVERSE_PROXY_EMAIL` + +If set `ENABLE_REVERSE_PROXY_FULL_NAME=true`, a user full name expected in `X-WEBAUTH-FULLNAME` will be assigned to the user when auto creating the user. You can also change the header name with `REVERSE_PROXY_AUTHENTICATION_FULL_NAME`. + +You can also limit the reverse proxy's IP address range with `REVERSE_PROXY_TRUSTED_PROXIES` which default value is `127.0.0.0/8,::1/128`. By `REVERSE_PROXY_LIMIT`, you can limit trusted proxies level. + +Notice: Reverse Proxy Auth doesn't support the API. You still need an access token or basic auth to make API requests. diff --git a/docs/content/doc/features/authentication.zh-cn.md b/docs/content/doc/features/authentication.zh-cn.md index 481e33441b5b7..aeb099f760b8e 100644 --- a/docs/content/doc/features/authentication.zh-cn.md +++ b/docs/content/doc/features/authentication.zh-cn.md @@ -15,4 +15,21 @@ menu: # 认证 -## TBD +## 反向代理认证 + +Gitea 支持通过读取反向代理传递的 HTTP 头中的登录名或者 email 地址来支持反向代理来认证。默认是不启用的,你可以用以下配置启用。 + +```ini +[service] +ENABLE_REVERSE_PROXY_AUTHENTICATION = true +``` + +默认的登录用户名的 HTTP 头是 `X-WEBAUTH-USER`,你可以通过修改 `REVERSE_PROXY_AUTHENTICATION_USER` 来变更它。如果用户不存在,可以自动创建用户,当然你需要修改 `ENABLE_REVERSE_PROXY_AUTO_REGISTRATION=true` 来启用它。 + +默认的登录用户 Email 的 HTTP 头是 `X-WEBAUTH-EMAIL`,你可以通过修改 `REVERSE_PROXY_AUTHENTICATION_EMAIL` 来变更它。如果用户不存在,可以自动创建用户,当然你需要修改 `ENABLE_REVERSE_PROXY_AUTO_REGISTRATION=true` 来启用它。你也可以通过修改 `ENABLE_REVERSE_PROXY_EMAIL` 来启用或停用这个 HTTP 头。 + +如果设置了 `ENABLE_REVERSE_PROXY_FULL_NAME=true`,则用户的全名会从 `X-WEBAUTH-FULLNAME` 读取,这样在自动创建用户时将使用这个字段作为用户全名,你也可以通过修改 `REVERSE_PROXY_AUTHENTICATION_FULL_NAME` 来变更 HTTP 头。 + +你也可以通过修改 `REVERSE_PROXY_TRUSTED_PROXIES` 来设置反向代理的IP地址范围,加强安全性,默认值是 `127.0.0.0/8,::1/128`。 通过 `REVERSE_PROXY_LIMIT`, 可以设置最多信任几级反向代理。 + +注意:反向代理认证不支持认证 API,API 仍旧需要用 access token 来进行认证。 From cfb1cb1168726a3a4c13aeafaa9728d82e7e84fe Mon Sep 17 00:00:00 2001 From: techknowlogick Date: Fri, 3 Feb 2023 11:23:52 -0500 Subject: [PATCH 11/23] update to build with go1.20 (#22732) as title --------- Co-authored-by: Lauris BH --- .drone.yml | 32 +++++++++++++-------------- .golangci.yml | 4 ++-- Dockerfile | 2 +- Dockerfile.rootless | 2 +- Makefile | 4 ++-- docs/config.yaml | 4 ++-- go.mod | 2 +- models/db/sql_postgres_with_schema.go | 7 ++---- 8 files changed, 27 insertions(+), 30 deletions(-) diff --git a/.drone.yml b/.drone.yml index a8fa7eba3687b..f9da8f9743807 100644 --- a/.drone.yml +++ b/.drone.yml @@ -25,7 +25,7 @@ steps: - make deps-frontend - name: deps-backend - image: golang:1.19 + image: golang:1.20 pull: always commands: - make deps-backend @@ -88,7 +88,7 @@ steps: depends_on: [deps-frontend] - name: checks-backend - image: golang:1.19 + image: golang:1.20 commands: - make --always-make checks-backend # ensure the 'go-licenses' make target runs depends_on: [deps-backend] @@ -109,7 +109,7 @@ steps: depends_on: [deps-frontend] - name: build-backend-no-gcc - image: golang:1.18 # this step is kept as the lowest version of golang that we support + image: golang:1.19 # this step is kept as the lowest version of golang that we support pull: always environment: GO111MODULE: on @@ -122,7 +122,7 @@ steps: path: /go - name: build-backend-arm64 - image: golang:1.19 + image: golang:1.20 environment: GO111MODULE: on GOPROXY: https://goproxy.io @@ -138,7 +138,7 @@ steps: path: /go - name: build-backend-windows - image: golang:1.19 + image: golang:1.20 environment: GO111MODULE: on GOPROXY: https://goproxy.io @@ -153,7 +153,7 @@ steps: path: /go - name: build-backend-386 - image: golang:1.19 + image: golang:1.20 environment: GO111MODULE: on GOPROXY: https://goproxy.io @@ -247,7 +247,7 @@ steps: - pull_request - name: deps-backend - image: golang:1.19 + image: golang:1.20 pull: always commands: - make deps-backend @@ -364,7 +364,7 @@ steps: path: /go - name: generate-coverage - image: golang:1.19 + image: golang:1.20 commands: - make coverage environment: @@ -440,7 +440,7 @@ steps: - pull_request - name: deps-backend - image: golang:1.19 + image: golang:1.20 pull: always commands: - make deps-backend @@ -557,7 +557,7 @@ steps: - name: test-e2e image: mcr.microsoft.com/playwright:v1.29.2-focal commands: - - curl -sLO https://go.dev/dl/go1.19.linux-amd64.tar.gz && tar -C /usr/local -xzf go1.19.linux-amd64.tar.gz + - curl -sLO https://go.dev/dl/go1.20.linux-amd64.tar.gz && tar -C /usr/local -xzf go1.20.linux-amd64.tar.gz - groupadd --gid 1001 gitea && useradd -m --gid 1001 --uid 1001 gitea - apt-get -qq update && apt-get -qqy install build-essential - export TEST_PGSQL_SCHEMA='' @@ -656,7 +656,7 @@ trigger: steps: - name: download - image: golang:1.19 + image: golang:1.20 pull: always commands: - timeout -s ABRT 40m make generate-license generate-gitignore @@ -720,7 +720,7 @@ steps: - make deps-frontend - name: deps-backend - image: golang:1.19 + image: golang:1.20 pull: always commands: - make deps-backend @@ -729,7 +729,7 @@ steps: path: /go - name: static - image: techknowlogick/xgo:go-1.19.x + image: techknowlogick/xgo:go-1.20.x pull: always commands: # Upgrade to node 18 once https://github.com/techknowlogick/xgo/issues/163 is resolved @@ -841,7 +841,7 @@ steps: - make deps-frontend - name: deps-backend - image: golang:1.19 + image: golang:1.20 pull: always commands: - make deps-backend @@ -850,7 +850,7 @@ steps: path: /go - name: static - image: techknowlogick/xgo:go-1.19.x + image: techknowlogick/xgo:go-1.20.x pull: always commands: # Upgrade to node 18 once https://github.com/techknowlogick/xgo/issues/163 is resolved @@ -932,7 +932,7 @@ trigger: steps: - name: build-docs - image: golang:1.19 + image: golang:1.20 commands: - cd docs - make trans-copy clean build diff --git a/.golangci.yml b/.golangci.yml index 7635e83a37260..caa237370e375 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -28,7 +28,7 @@ linters: fast: false run: - go: 1.19 + go: 1.20 timeout: 10m skip-dirs: - node_modules @@ -74,7 +74,7 @@ linters-settings: - name: modifies-value-receiver gofumpt: extra-rules: true - lang-version: "1.19" + lang-version: "1.20" depguard: list-type: denylist # Check the list against standard lib. diff --git a/Dockerfile b/Dockerfile index 3ee474bb3425b..89f000882c57a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,5 +1,5 @@ #Build stage -FROM golang:1.19-alpine3.17 AS build-env +FROM golang:1.20-alpine3.17 AS build-env ARG GOPROXY ENV GOPROXY ${GOPROXY:-direct} diff --git a/Dockerfile.rootless b/Dockerfile.rootless index a43a63fa10c78..e92ce857d3cd3 100644 --- a/Dockerfile.rootless +++ b/Dockerfile.rootless @@ -1,5 +1,5 @@ #Build stage -FROM golang:1.19-alpine3.17 AS build-env +FROM golang:1.20-alpine3.17 AS build-env ARG GOPROXY ENV GOPROXY ${GOPROXY:-direct} diff --git a/Makefile b/Makefile index 4d7c507875529..717d7cafc660b 100644 --- a/Makefile +++ b/Makefile @@ -23,13 +23,13 @@ SHASUM ?= shasum -a 256 HAS_GO = $(shell hash $(GO) > /dev/null 2>&1 && echo "GO" || echo "NOGO" ) COMMA := , -XGO_VERSION := go-1.19.x +XGO_VERSION := go-1.20.x AIR_PACKAGE ?= github.com/cosmtrek/air@v1.40.4 EDITORCONFIG_CHECKER_PACKAGE ?= github.com/editorconfig-checker/editorconfig-checker/cmd/editorconfig-checker@2.6.0 ERRCHECK_PACKAGE ?= github.com/kisielk/errcheck@v1.6.2 GOFUMPT_PACKAGE ?= mvdan.cc/gofumpt@v0.4.0 -GOLANGCI_LINT_PACKAGE ?= github.com/golangci/golangci-lint/cmd/golangci-lint@v1.50.1 +GOLANGCI_LINT_PACKAGE ?= github.com/golangci/golangci-lint/cmd/golangci-lint@v1.51.0 GXZ_PAGAGE ?= github.com/ulikunitz/xz/cmd/gxz@v0.5.10 MISSPELL_PACKAGE ?= github.com/client9/misspell/cmd/misspell@v0.3.4 SWAGGER_PACKAGE ?= github.com/go-swagger/go-swagger/cmd/swagger@v0.30.3 diff --git a/docs/config.yaml b/docs/config.yaml index 0b47c0ffd83f5..0a6d5d13f23a9 100644 --- a/docs/config.yaml +++ b/docs/config.yaml @@ -19,8 +19,8 @@ params: author: The Gitea Authors website: https://docs.gitea.io version: 1.18.1 - minGoVersion: 1.18 - goVersion: 1.19 + minGoVersion: 1.19 + goVersion: 1.20 minNodeVersion: 16 search: nav repo: "https://github.com/go-gitea/gitea" diff --git a/go.mod b/go.mod index a929508e0de0d..eb23bd9e32a17 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module code.gitea.io/gitea -go 1.18 +go 1.19 require ( code.gitea.io/actions-proto-go v0.2.0 diff --git a/models/db/sql_postgres_with_schema.go b/models/db/sql_postgres_with_schema.go index ec63447f6f6f3..c2694b37bb934 100644 --- a/models/db/sql_postgres_with_schema.go +++ b/models/db/sql_postgres_with_schema.go @@ -37,9 +37,7 @@ func (d *postgresSchemaDriver) Open(name string) (driver.Conn, error) { } schemaValue, _ := driver.String.ConvertValue(setting.Database.Schema) - // golangci lint is incorrect here - there is no benefit to using driver.ExecerContext here - // and in any case pq does not implement it - if execer, ok := conn.(driver.Execer); ok { //nolint + if execer, ok := conn.(driver.Execer); ok { _, err := execer.Exec(`SELECT set_config( 'search_path', $1 || ',' || current_setting('search_path'), @@ -63,8 +61,7 @@ func (d *postgresSchemaDriver) Open(name string) (driver.Conn, error) { // driver.String.ConvertValue will never return err for string - // golangci lint is incorrect here - there is no benefit to using stmt.ExecWithContext here - _, err = stmt.Exec([]driver.Value{schemaValue}) //nolint + _, err = stmt.Exec([]driver.Value{schemaValue}) if err != nil { _ = conn.Close() return nil, err From ce4fd95233b0012fa5510bd3f2d049a0edac7903 Mon Sep 17 00:00:00 2001 From: Yarden Shoham Date: Fri, 3 Feb 2023 19:22:11 +0200 Subject: [PATCH 12/23] Use native error checking with `exec.ErrDot` (#22735) This was meant to land in #22073 but was blocked until #22732 was merged Signed-off-by: Yarden Shoham --- modules/setting/setting.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/setting/setting.go b/modules/setting/setting.go index f591727ca3421..afd7a40150d25 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -6,6 +6,7 @@ package setting import ( "encoding/base64" + "errors" "fmt" "math" "net" @@ -466,8 +467,7 @@ func getAppPath() (string, error) { } if err != nil { - // FIXME: Once we switch to go 1.19 use !errors.Is(err, exec.ErrDot) - if !strings.Contains(err.Error(), "cannot run executable found relative to current directory") { + if !errors.Is(err, exec.ErrDot) { return "", err } appPath, err = filepath.Abs(os.Args[0]) From 01f082287d7957ed63a0865b26e04ad23382c715 Mon Sep 17 00:00:00 2001 From: Francesco Siddi Date: Fri, 3 Feb 2023 23:25:55 +0100 Subject: [PATCH 13/23] Remove 'primary' class from tab counter labels (#22687) Using the primary color for each label counter makes the use of color redundant, as well as suggesting this is a call to action. Use the base grey color instead. ![grey_lables](https://user-images.githubusercontent.com/451841/215778889-0d5dddad-353f-4703-a48f-1540080dee26.jpg) --- templates/repo/header.tmpl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/templates/repo/header.tmpl b/templates/repo/header.tmpl index 0e50169486b24..b860c1d26ce31 100644 --- a/templates/repo/header.tmpl +++ b/templates/repo/header.tmpl @@ -163,7 +163,7 @@ {{svg "octicon-issue-opened"}} {{.locale.Tr "repo.issues"}} {{if .Repository.NumOpenIssues}} - {{CountFmt .Repository.NumOpenIssues}} + {{CountFmt .Repository.NumOpenIssues}} {{end}} {{end}} @@ -178,7 +178,7 @@ {{svg "octicon-git-pull-request"}} {{.locale.Tr "repo.pulls"}} {{if .Repository.NumOpenPulls}} - {{CountFmt .Repository.NumOpenPulls}} + {{CountFmt .Repository.NumOpenPulls}} {{end}} {{end}} @@ -187,7 +187,7 @@ {{svg "octicon-play"}} {{.locale.Tr "actions.actions"}} {{if .Repository.NumOpenActionRuns}} - {{CountFmt .Repository.NumOpenActionRuns}} + {{CountFmt .Repository.NumOpenActionRuns}} {{end}} {{end}} @@ -202,7 +202,7 @@ {{svg "octicon-project"}} {{.locale.Tr "repo.project_board"}} {{if .Repository.NumOpenProjects}} - {{CountFmt .Repository.NumOpenProjects}} + {{CountFmt .Repository.NumOpenProjects}} {{end}} {{end}} @@ -211,7 +211,7 @@ {{svg "octicon-tag"}} {{.locale.Tr "repo.releases"}} {{if .NumReleases}} - {{CountFmt .NumReleases}} + {{CountFmt .NumReleases}} {{end}} {{end}} From 3c5655ce18056277917092d370bbdfbcdaaa8ae6 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 3 Feb 2023 23:11:48 +0000 Subject: [PATCH 14/23] Improve trace logging for pulls and processes (#22633) Our trace logging is far from perfect and is difficult to follow. This PR: * Add trace logging for process manager add and remove. * Fixes an errant read file for git refs in getMergeCommit * Brings in the pullrequest `String` and `ColorFormat` methods introduced in #22568 * Adds a lot more logging in to testPR etc. Ref #22578 --------- Signed-off-by: Andrew Thornton Co-authored-by: Lunny Xiao Co-authored-by: delvh Co-authored-by: 6543 <6543@obermui.de> Co-authored-by: techknowlogick --- models/issues/pull.go | 67 +++++++++++- modules/log/colors.go | 7 ++ modules/log/log.go | 11 ++ modules/process/manager.go | 22 ++-- modules/process/manager_test.go | 2 +- routers/web/repo/pull.go | 51 ++++----- services/automerge/automerge.go | 26 ++--- services/pull/check.go | 188 ++++++++++++++++---------------- services/pull/patch.go | 2 +- 9 files changed, 230 insertions(+), 146 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 6ff6502e4eb56..044fb5fa04d65 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -8,6 +8,7 @@ import ( "context" "fmt" "io" + "strconv" "strings" "code.gitea.io/gitea/models/db" @@ -132,6 +133,27 @@ const ( PullRequestStatusAncestor ) +func (status PullRequestStatus) String() string { + switch status { + case PullRequestStatusConflict: + return "CONFLICT" + case PullRequestStatusChecking: + return "CHECKING" + case PullRequestStatusMergeable: + return "MERGEABLE" + case PullRequestStatusManuallyMerged: + return "MANUALLY_MERGED" + case PullRequestStatusError: + return "ERROR" + case PullRequestStatusEmpty: + return "EMPTY" + case PullRequestStatusAncestor: + return "ANCESTOR" + default: + return strconv.Itoa(int(status)) + } +} + // PullRequestFlow the flow of pull request type PullRequestFlow int @@ -202,6 +224,42 @@ func DeletePullsByBaseRepoID(ctx context.Context, repoID int64) error { return err } +// ColorFormat writes a colored string to identify this struct +func (pr *PullRequest) ColorFormat(s fmt.State) { + if pr == nil { + log.ColorFprintf(s, "PR[%d]%s#%d[%s...%s:%s]", + log.NewColoredIDValue(0), + log.NewColoredValue("/"), + log.NewColoredIDValue(0), + log.NewColoredValue(""), + log.NewColoredValue("/"), + log.NewColoredValue(""), + ) + return + } + + log.ColorFprintf(s, "PR[%d]", log.NewColoredIDValue(pr.ID)) + if pr.BaseRepo != nil { + log.ColorFprintf(s, "%s#%d[%s...", log.NewColoredValue(pr.BaseRepo.FullName()), + log.NewColoredIDValue(pr.Index), log.NewColoredValue(pr.BaseBranch)) + } else { + log.ColorFprintf(s, "Repo[%d]#%d[%s...", log.NewColoredIDValue(pr.BaseRepoID), + log.NewColoredIDValue(pr.Index), log.NewColoredValue(pr.BaseBranch)) + } + if pr.HeadRepoID == pr.BaseRepoID { + log.ColorFprintf(s, "%s]", log.NewColoredValue(pr.HeadBranch)) + } else if pr.HeadRepo != nil { + log.ColorFprintf(s, "%s:%s]", log.NewColoredValue(pr.HeadRepo.FullName()), log.NewColoredValue(pr.HeadBranch)) + } else { + log.ColorFprintf(s, "Repo[%d]:%s]", log.NewColoredIDValue(pr.HeadRepoID), log.NewColoredValue(pr.HeadBranch)) + } +} + +// String represents the pr as a simple string +func (pr *PullRequest) String() string { + return log.ColorFormatAsString(pr) +} + // MustHeadUserName returns the HeadRepo's username if failed return blank func (pr *PullRequest) MustHeadUserName(ctx context.Context) string { if err := pr.LoadHeadRepo(ctx); err != nil { @@ -234,7 +292,8 @@ func (pr *PullRequest) LoadAttributes(ctx context.Context) (err error) { return nil } -// LoadHeadRepo loads the head repository +// LoadHeadRepo loads the head repository, pr.HeadRepo will remain nil if it does not exist +// and thus ErrRepoNotExist will never be returned func (pr *PullRequest) LoadHeadRepo(ctx context.Context) (err error) { if !pr.isHeadRepoLoaded && pr.HeadRepo == nil && pr.HeadRepoID > 0 { if pr.HeadRepoID == pr.BaseRepoID { @@ -249,14 +308,14 @@ func (pr *PullRequest) LoadHeadRepo(ctx context.Context) (err error) { pr.HeadRepo, err = repo_model.GetRepositoryByID(ctx, pr.HeadRepoID) if err != nil && !repo_model.IsErrRepoNotExist(err) { // Head repo maybe deleted, but it should still work - return fmt.Errorf("GetRepositoryByID(head): %w", err) + return fmt.Errorf("pr[%d].LoadHeadRepo[%d]: %w", pr.ID, pr.HeadRepoID, err) } pr.isHeadRepoLoaded = true } return nil } -// LoadBaseRepo loads the target repository +// LoadBaseRepo loads the target repository. ErrRepoNotExist may be returned. func (pr *PullRequest) LoadBaseRepo(ctx context.Context) (err error) { if pr.BaseRepo != nil { return nil @@ -274,7 +333,7 @@ func (pr *PullRequest) LoadBaseRepo(ctx context.Context) (err error) { pr.BaseRepo, err = repo_model.GetRepositoryByID(ctx, pr.BaseRepoID) if err != nil { - return fmt.Errorf("repo_model.GetRepositoryByID(base): %w", err) + return fmt.Errorf("pr[%d].LoadBaseRepo[%d]: %w", pr.ID, pr.BaseRepoID, err) } return nil } diff --git a/modules/log/colors.go b/modules/log/colors.go index 02781afe84ebd..85e205cb6764d 100644 --- a/modules/log/colors.go +++ b/modules/log/colors.go @@ -383,6 +383,13 @@ func (cv *ColoredValue) Format(s fmt.State, c rune) { s.Write(*cv.resetBytes) } +// ColorFormatAsString returns the result of the ColorFormat without the color +func ColorFormatAsString(colorVal ColorFormatted) string { + s := new(strings.Builder) + _, _ = ColorFprintf(&protectedANSIWriter{w: s, mode: removeColor}, "%-v", colorVal) + return s.String() +} + // SetColorBytes will allow a user to set the colorBytes of a colored value func (cv *ColoredValue) SetColorBytes(colorBytes []byte) { cv.colorBytes = &colorBytes diff --git a/modules/log/log.go b/modules/log/log.go index 73f908dfab55a..9057cda37e6b1 100644 --- a/modules/log/log.go +++ b/modules/log/log.go @@ -9,6 +9,8 @@ import ( "runtime" "strings" "sync" + + "code.gitea.io/gitea/modules/process" ) type loggerMap struct { @@ -285,6 +287,15 @@ func (l *LoggerAsWriter) Log(msg string) { } func init() { + process.Trace = func(start bool, pid process.IDType, description string, parentPID process.IDType, typ string) { + if start && parentPID != "" { + Log(1, TRACE, "Start %s: %s (from %s) (%s)", NewColoredValue(pid, FgHiYellow), description, NewColoredValue(parentPID, FgYellow), NewColoredValue(typ, Reset)) + } else if start { + Log(1, TRACE, "Start %s: %s (%s)", NewColoredValue(pid, FgHiYellow), description, NewColoredValue(typ, Reset)) + } else { + Log(1, TRACE, "Done %s: %s", NewColoredValue(pid, FgHiYellow), NewColoredValue(description, Reset)) + } + } _, filename, _, _ := runtime.Caller(0) prefix = strings.TrimSuffix(filename, "modules/log/log.go") if prefix == filename { diff --git a/modules/process/manager.go b/modules/process/manager.go index 1272510067ffa..fdfca3db7a159 100644 --- a/modules/process/manager.go +++ b/modules/process/manager.go @@ -6,6 +6,7 @@ package process import ( "context" + "log" "runtime/pprof" "strconv" "sync" @@ -43,6 +44,18 @@ type IDType string // - it is simply an alias for context.CancelFunc and is only for documentary purposes type FinishedFunc = context.CancelFunc +var Trace = defaultTrace // this global can be overridden by particular logging packages - thus avoiding import cycles + +func defaultTrace(start bool, pid IDType, description string, parentPID IDType, typ string) { + if start && parentPID != "" { + log.Printf("start process %s: %s (from %s) (%s)", pid, description, parentPID, typ) + } else if start { + log.Printf("start process %s: %s (%s)", pid, description, typ) + } else { + log.Printf("end process %s: %s", pid, description) + } +} + // Manager manages all processes and counts PIDs. type Manager struct { mutex sync.Mutex @@ -154,6 +167,7 @@ func (pm *Manager) Add(ctx context.Context, description string, cancel context.C pm.processMap[pid] = process pm.mutex.Unlock() + Trace(true, pid, description, parentPID, processType) pprofCtx := pprof.WithLabels(ctx, pprof.Labels(DescriptionPProfLabel, description, PPIDPProfLabel, string(parentPID), PIDPProfLabel, string(pid), ProcessTypePProfLabel, processType)) if currentlyRunning { @@ -185,18 +199,12 @@ func (pm *Manager) nextPID() (start time.Time, pid IDType) { return start, pid } -// Remove a process from the ProcessManager. -func (pm *Manager) Remove(pid IDType) { - pm.mutex.Lock() - delete(pm.processMap, pid) - pm.mutex.Unlock() -} - func (pm *Manager) remove(process *process) { pm.mutex.Lock() defer pm.mutex.Unlock() if p := pm.processMap[process.PID]; p == process { delete(pm.processMap, process.PID) + Trace(false, process.PID, process.Description, process.ParentPID, process.Type) } } diff --git a/modules/process/manager_test.go b/modules/process/manager_test.go index 527072713ffa4..2e2e35d24a0bf 100644 --- a/modules/process/manager_test.go +++ b/modules/process/manager_test.go @@ -82,7 +82,7 @@ func TestManager_Remove(t *testing.T) { assert.NotEqual(t, GetContext(p1Ctx).GetPID(), GetContext(p2Ctx).GetPID(), "expected to get different pids got %s == %s", GetContext(p2Ctx).GetPID(), GetContext(p1Ctx).GetPID()) - pm.Remove(GetPID(p2Ctx)) + finished() _, exists := pm.processMap[GetPID(p2Ctx)] assert.False(t, exists, "PID %d is in the list but shouldn't", GetPID(p2Ctx)) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 71bc98d13d07c..11d336d4ecaae 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -926,59 +926,54 @@ func MergePullRequest(ctx *context.Context) { pr := issue.PullRequest pr.Issue = issue pr.Issue.Repo = ctx.Repo.Repository - manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged + manualMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged forceMerge := form.ForceMerge != nil && *form.ForceMerge // start with merging by checking - if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, forceMerge); err != nil { - if errors.Is(err, pull_service.ErrIsClosed) { + if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manualMerge, forceMerge); err != nil { + switch { + case errors.Is(err, pull_service.ErrIsClosed): if issue.IsPull { ctx.Flash.Error(ctx.Tr("repo.pulls.is_closed")) - ctx.Redirect(issue.Link()) } else { ctx.Flash.Error(ctx.Tr("repo.issues.closed_title")) - ctx.Redirect(issue.Link()) } - } else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) { + case errors.Is(err, pull_service.ErrUserNotAllowedToMerge): ctx.Flash.Error(ctx.Tr("repo.pulls.update_not_allowed")) - ctx.Redirect(issue.Link()) - } else if errors.Is(err, pull_service.ErrHasMerged) { + case errors.Is(err, pull_service.ErrHasMerged): ctx.Flash.Error(ctx.Tr("repo.pulls.has_merged")) - ctx.Redirect(issue.Link()) - } else if errors.Is(err, pull_service.ErrIsWorkInProgress) { + case errors.Is(err, pull_service.ErrIsWorkInProgress): ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_wip")) - ctx.Redirect(issue.Link()) - } else if errors.Is(err, pull_service.ErrNotMergableState) { + case errors.Is(err, pull_service.ErrNotMergableState): ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready")) - ctx.Redirect(issue.Link()) - } else if models.IsErrDisallowedToMerge(err) { + case models.IsErrDisallowedToMerge(err): ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_not_ready")) - ctx.Redirect(issue.Link()) - } else if asymkey_service.IsErrWontSign(err) { - ctx.Flash.Error(err.Error()) // has not translation ... - ctx.Redirect(issue.Link()) - } else if errors.Is(err, pull_service.ErrDependenciesLeft) { + case asymkey_service.IsErrWontSign(err): + ctx.Flash.Error(err.Error()) // has no translation ... + case errors.Is(err, pull_service.ErrDependenciesLeft): ctx.Flash.Error(ctx.Tr("repo.issues.dependency.pr_close_blocked")) - ctx.Redirect(issue.Link()) - } else { + default: ctx.ServerError("WebCheck", err) + return } + + ctx.Redirect(issue.Link()) return } // handle manually-merged mark - if manuallMerge { + if manualMerge { if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil { - if models.IsErrInvalidMergeStyle(err) { + switch { + + case models.IsErrInvalidMergeStyle(err): ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option")) - ctx.Redirect(issue.Link()) - } else if strings.Contains(err.Error(), "Wrong commit ID") { + case strings.Contains(err.Error(), "Wrong commit ID"): ctx.Flash.Error(ctx.Tr("repo.pulls.wrong_commit_id")) - ctx.Redirect(issue.Link()) - } else { + default: ctx.ServerError("MergedManually", err) + return } - return } ctx.Redirect(issue.Link()) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 15d94e79209eb..74cfb8da8fdf4 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -164,7 +164,7 @@ func getPullRequestsByHeadSHA(ctx context.Context, sha string, repo *repo_model. func handlePull(pullID int64, sha string) { ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), - fmt.Sprintf("Handle AutoMerge of pull[%d] with sha[%s]", pullID, sha)) + fmt.Sprintf("Handle AutoMerge of PR[%d] with sha[%s]", pullID, sha)) defer finished() pr, err := issues_model.GetPullRequestByID(ctx, pullID) @@ -176,7 +176,7 @@ func handlePull(pullID int64, sha string) { // Check if there is a scheduled pr in the db exists, scheduledPRM, err := pull_model.GetScheduledMergeByPullID(ctx, pr.ID) if err != nil { - log.Error("pull[%d] GetScheduledMergeByPullID: %v", pr.ID, err) + log.Error("%-v GetScheduledMergeByPullID: %v", pr, err) return } if !exists { @@ -188,13 +188,13 @@ func handlePull(pullID int64, sha string) { // did not succeed or was not finished yet. if err = pr.LoadHeadRepo(ctx); err != nil { - log.Error("pull[%d] LoadHeadRepo: %v", pr.ID, err) + log.Error("%-v LoadHeadRepo: %v", pr, err) return } headGitRepo, err := git.OpenRepository(ctx, pr.HeadRepo.RepoPath()) if err != nil { - log.Error("OpenRepository: %v", err) + log.Error("OpenRepository %-v: %v", pr.HeadRepo, err) return } defer headGitRepo.Close() @@ -202,40 +202,40 @@ func handlePull(pullID int64, sha string) { headBranchExist := headGitRepo.IsBranchExist(pr.HeadBranch) if pr.HeadRepo == nil || !headBranchExist { - log.Warn("Head branch of auto merge pr does not exist [HeadRepoID: %d, Branch: %s, PR ID: %d]", pr.HeadRepoID, pr.HeadBranch, pr.ID) + log.Warn("Head branch of auto merge %-v does not exist [HeadRepoID: %d, Branch: %s]", pr, pr.HeadRepoID, pr.HeadBranch) return } // Check if all checks succeeded pass, err := pull_service.IsPullCommitStatusPass(ctx, pr) if err != nil { - log.Error("IsPullCommitStatusPass: %v", err) + log.Error("%-v IsPullCommitStatusPass: %v", pr, err) return } if !pass { - log.Info("Scheduled auto merge pr has unsuccessful status checks [PullID: %d]", pr.ID) + log.Info("Scheduled auto merge %-v has unsuccessful status checks", pr) return } // Merge if all checks succeeded doer, err := user_model.GetUserByID(ctx, scheduledPRM.DoerID) if err != nil { - log.Error("GetUserByIDCtx: %v", err) + log.Error("Unable to get scheduled User[%d]: %v", scheduledPRM.DoerID, err) return } perm, err := access_model.GetUserRepoPermission(ctx, pr.HeadRepo, doer) if err != nil { - log.Error("GetUserRepoPermission: %v", err) + log.Error("GetUserRepoPermission %-v: %v", pr.HeadRepo, err) return } if err := pull_service.CheckPullMergable(ctx, doer, &perm, pr, false, false); err != nil { if errors.Is(pull_service.ErrUserNotAllowedToMerge, err) { - log.Info("PR %d was scheduled to automerge by an unauthorized user", pr.ID) + log.Info("%-v was scheduled to automerge by an unauthorized user", pr) return } - log.Error("pull[%d] CheckPullMergable: %v", pr.ID, err) + log.Error("%-v CheckPullMergable: %v", pr, err) return } @@ -244,13 +244,13 @@ func handlePull(pullID int64, sha string) { baseGitRepo = headGitRepo } else { if err = pr.LoadBaseRepo(ctx); err != nil { - log.Error("LoadBaseRepo: %v", err) + log.Error("%-v LoadBaseRepo: %v", pr, err) return } baseGitRepo, err = git.OpenRepository(ctx, pr.BaseRepo.RepoPath()) if err != nil { - log.Error("OpenRepository: %v", err) + log.Error("OpenRepository %-v: %v", pr.BaseRepo, err) return } defer baseGitRepo.Close() diff --git a/services/pull/check.go b/services/pull/check.go index db86378909c49..481491c73bb09 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -8,7 +8,6 @@ import ( "context" "errors" "fmt" - "os" "strconv" "strings" @@ -27,7 +26,6 @@ import ( "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/timeutil" - "code.gitea.io/gitea/modules/util" asymkey_service "code.gitea.io/gitea/services/asymkey" ) @@ -50,14 +48,14 @@ func AddToTaskQueue(pr *issues_model.PullRequest) { pr.Status = issues_model.PullRequestStatusChecking err := pr.UpdateColsIfNotMerged(db.DefaultContext, "status") if err != nil { - log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err) + log.Error("AddToTaskQueue(%-v).UpdateCols.(add to queue): %v", pr, err) } else { - log.Trace("Adding PR ID: %d to the test pull requests queue", pr.ID) + log.Trace("Adding %-v to the test pull requests queue", pr) } return err }) if err != nil && err != queue.ErrAlreadyInQueue { - log.Error("Error adding prID %d to the test pull requests queue: %v", pr.ID, err) + log.Error("Error adding %-v to the test pull requests queue: %v", pr, err) } } @@ -69,12 +67,14 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce } if err := pr.LoadIssue(ctx); err != nil { + log.Error("Unable to load issue[%d] for %-v: %v", pr.IssueID, pr, err) return err } else if pr.Issue.IsClosed { return ErrIsClosed } if allowedMerge, err := IsUserAllowedToMerge(ctx, pr, *perm, doer); err != nil { + log.Error("Error whilst checking if %-v is allowed to merge %-v: %v", doer, pr, err) return err } else if !allowedMerge { return ErrUserNotAllowedToMerge @@ -98,15 +98,19 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce } if err := CheckPullBranchProtections(ctx, pr, false); err != nil { - if models.IsErrDisallowedToMerge(err) { - if force { - if isRepoAdmin, err2 := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); err2 != nil { - return err2 - } else if !isRepoAdmin { - return err - } - } - } else { + if !models.IsErrDisallowedToMerge(err) { + log.Error("Error whilst checking pull branch protection for %-v: %v", pr, err) + return err + } + + if !force { + return err + } + + if isRepoAdmin, err2 := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); err2 != nil { + log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, err2) + return err2 + } else if !isRepoAdmin { return err } } @@ -144,7 +148,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer // checkAndUpdateStatus checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { - // Status is not changed to conflict means mergeable. + // If status has not been changed to conflict by testPatch then we are mergeable if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable } @@ -152,79 +156,69 @@ func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { // Make sure there is no waiting test to process before leaving the checking status. has, err := prPatchCheckerQueue.Has(strconv.FormatInt(pr.ID, 10)) if err != nil { - log.Error("Unable to check if the queue is waiting to reprocess pr.ID %d. Error: %v", pr.ID, err) + log.Error("Unable to check if the queue is waiting to reprocess %-v. Error: %v", pr, err) } - if !has { - if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { - log.Error("Update[%d]: %v", pr.ID, err) - } + if has { + log.Trace("Not updating status for %-v as it is due to be rechecked", pr) + return + } + + if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { + log.Error("Update[%-v]: %v", pr, err) } } -// getMergeCommit checks if a pull request got merged +// getMergeCommit checks if a pull request has been merged // Returns the git.Commit of the pull request if merged func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Commit, error) { - if pr.BaseRepo == nil { - var err error - pr.BaseRepo, err = repo_model.GetRepositoryByID(ctx, pr.BaseRepoID) - if err != nil { - return nil, fmt.Errorf("GetRepositoryByID: %w", err) - } - } - - indexTmpPath, err := os.MkdirTemp(os.TempDir(), "gitea-"+pr.BaseRepo.Name) - if err != nil { - return nil, fmt.Errorf("Failed to create temp dir for repository %s: %w", pr.BaseRepo.RepoPath(), err) + if err := pr.LoadBaseRepo(ctx); err != nil { + return nil, fmt.Errorf("unable to load base repo for %s: %w", pr, err) } - defer func() { - if err := util.RemoveAll(indexTmpPath); err != nil { - log.Warn("Unable to remove temporary index path: %s: Error: %v", indexTmpPath, err) - } - }() - headFile := pr.GetGitRefName() + prHeadRef := pr.GetGitRefName() - // Check if a pull request is merged into BaseBranch - _, _, err = git.NewCommand(ctx, "merge-base", "--is-ancestor").AddDynamicArguments(headFile, pr.BaseBranch). - RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath(), Env: []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}}) - if err != nil { - // Errors are signaled by a non-zero status that is not 1 + // Check if the pull request is merged into BaseBranch + if _, _, err := git.NewCommand(ctx, "merge-base", "--is-ancestor"). + AddDynamicArguments(prHeadRef, pr.BaseBranch). + RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath()}); err != nil { if strings.Contains(err.Error(), "exit status 1") { + // prHeadRef is not an ancestor of the base branch return nil, nil } - return nil, fmt.Errorf("git merge-base --is-ancestor: %w", err) + // Errors are signaled by a non-zero status that is not 1 + return nil, fmt.Errorf("%-v git merge-base --is-ancestor: %w", pr, err) } - commitIDBytes, err := os.ReadFile(pr.BaseRepo.RepoPath() + "/" + headFile) + // If merge-base successfully exits then prHeadRef is an ancestor of pr.BaseBranch + + // Find the head commit id + prHeadCommitID, err := git.GetFullCommitID(ctx, pr.BaseRepo.RepoPath(), prHeadRef) if err != nil { - return nil, fmt.Errorf("ReadFile(%s): %w", headFile, err) + return nil, fmt.Errorf("GetFullCommitID(%s) in %s: %w", prHeadRef, pr.BaseRepo.FullName(), err) } - commitID := string(commitIDBytes) - if len(commitID) < git.SHAFullLength { - return nil, fmt.Errorf(`ReadFile(%s): invalid commit-ID "%s"`, headFile, commitID) - } - cmd := commitID[:git.SHAFullLength] + ".." + pr.BaseBranch // Get the commit from BaseBranch where the pull request got merged - mergeCommit, _, err := git.NewCommand(ctx, "rev-list", "--ancestry-path", "--merges", "--reverse").AddDynamicArguments(cmd). - RunStdString(&git.RunOpts{Dir: "", Env: []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}}) + mergeCommit, _, err := git.NewCommand(ctx, "rev-list", "--ancestry-path", "--merges", "--reverse"). + AddDynamicArguments(prHeadCommitID + ".." + pr.BaseBranch). + RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) if err != nil { return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %w", err) } else if len(mergeCommit) < git.SHAFullLength { // PR was maybe fast-forwarded, so just use last commit of PR - mergeCommit = commitID[:git.SHAFullLength] + mergeCommit = prHeadCommitID } + mergeCommit = strings.TrimSpace(mergeCommit) gitRepo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath()) if err != nil { - return nil, fmt.Errorf("OpenRepository: %w", err) + return nil, fmt.Errorf("%-v OpenRepository: %w", pr.BaseRepo, err) } defer gitRepo.Close() - commit, err := gitRepo.GetCommit(mergeCommit[:git.SHAFullLength]) + commit, err := gitRepo.GetCommit(mergeCommit) if err != nil { - return nil, fmt.Errorf("GetMergeCommit[%v]: %w", mergeCommit[:git.SHAFullLength], err) + return nil, fmt.Errorf("GetMergeCommit[%s]: %w", mergeCommit, err) } return commit, nil @@ -234,7 +228,7 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com // When a pull request got manually merged mark the pull request as merged func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { if err := pr.LoadBaseRepo(ctx); err != nil { - log.Error("PullRequest[%d].LoadBaseRepo: %v", pr.ID, err) + log.Error("%-v LoadBaseRepo: %v", pr, err) return false } @@ -244,47 +238,50 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { return false } } else { - log.Error("PullRequest[%d].BaseRepo.GetUnit(unit.TypePullRequests): %v", pr.ID, err) + log.Error("%-v BaseRepo.GetUnit(unit.TypePullRequests): %v", pr, err) return false } commit, err := getMergeCommit(ctx, pr) if err != nil { - log.Error("PullRequest[%d].getMergeCommit: %v", pr.ID, err) + log.Error("%-v getMergeCommit: %v", pr, err) + return false + } + + if commit == nil { + // no merge commit found return false } - if commit != nil { - pr.MergedCommitID = commit.ID.String() - pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) - pr.Status = issues_model.PullRequestStatusManuallyMerged - merger, _ := user_model.GetUserByEmail(commit.Author.Email) - - // When the commit author is unknown set the BaseRepo owner as merger - if merger == nil { - if pr.BaseRepo.Owner == nil { - if err = pr.BaseRepo.GetOwner(ctx); err != nil { - log.Error("BaseRepo.GetOwner[%d]: %v", pr.ID, err) - return false - } + + pr.MergedCommitID = commit.ID.String() + pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) + pr.Status = issues_model.PullRequestStatusManuallyMerged + merger, _ := user_model.GetUserByEmail(commit.Author.Email) + + // When the commit author is unknown set the BaseRepo owner as merger + if merger == nil { + if pr.BaseRepo.Owner == nil { + if err = pr.BaseRepo.GetOwner(ctx); err != nil { + log.Error("%-v BaseRepo.GetOwner: %v", pr, err) + return false } - merger = pr.BaseRepo.Owner } - pr.Merger = merger - pr.MergerID = merger.ID + merger = pr.BaseRepo.Owner + } + pr.Merger = merger + pr.MergerID = merger.ID - if merged, err := pr.SetMerged(ctx); err != nil { - log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) - return false - } else if !merged { - return false - } + if merged, err := pr.SetMerged(ctx); err != nil { + log.Error("%-v setMerged : %v", pr, err) + return false + } else if !merged { + return false + } - notification.NotifyMergePullRequest(ctx, merger, pr) + notification.NotifyMergePullRequest(ctx, merger, pr) - log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String()) - return true - } - return false + log.Info("manuallyMerged[%-v]: Marked as manually merged into %s/%s by commit id: %s", pr, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String()) + return true } // InitializePullRequests checks and tests untested patches of pull requests. @@ -300,10 +297,10 @@ func InitializePullRequests(ctx context.Context) { return default: if err := prPatchCheckerQueue.PushFunc(strconv.FormatInt(prID, 10), func() error { - log.Trace("Adding PR ID: %d to the pull requests patch checking queue", prID) + log.Trace("Adding PR[%d] to the pull requests patch checking queue", prID) return nil }); err != nil { - log.Error("Error adding prID: %s to the pull requests patch checking queue %v", prID, err) + log.Error("Error adding PR[%d] to the pull requests patch checking queue %v", prID, err) } } } @@ -327,23 +324,30 @@ func testPR(id int64) { pr, err := issues_model.GetPullRequestByID(ctx, id) if err != nil { - log.Error("GetPullRequestByID[%d]: %v", id, err) + log.Error("Unable to GetPullRequestByID[%d] for testPR: %v", id, err) return } + log.Trace("Testing %-v", pr) + defer func() { + log.Trace("Done testing %-v (status: %s)", pr, pr.Status) + }() + if pr.HasMerged { + log.Trace("%-v is already merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) return } if manuallyMerged(ctx, pr) { + log.Trace("%-v is manually merged (status: %s, merge commit: %s)", pr, pr.Status, pr.MergedCommitID) return } if err := TestPatch(pr); err != nil { - log.Error("testPatch[%d]: %v", pr.ID, err) + log.Error("testPatch[%-v]: %v", pr, err) pr.Status = issues_model.PullRequestStatusError if err := pr.UpdateCols("status"); err != nil { - log.Error("update pr [%d] status to PullRequestStatusError failed: %v", pr.ID, err) + log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err) } return } diff --git a/services/pull/patch.go b/services/pull/patch.go index 26a72a7371bf1..a3084196bbc90 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -60,7 +60,7 @@ var patchErrorSuffices = []string{ // TestPatch will test whether a simple patch will apply func TestPatch(pr *issues_model.PullRequest) error { - ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: Repo[%d]#%d", pr.BaseRepoID, pr.Index)) + ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr)) defer finished() // Clone base repo. From 6bc3079c0036a54d1b8ab04c5a6e14111c719c9a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 4 Feb 2023 10:30:43 +0800 Subject: [PATCH 15/23] Refactor git command package to improve security and maintainability (#22678) This PR follows #21535 (and replace #22592) ## Review without space diff https://github.com/go-gitea/gitea/pull/22678/files?diff=split&w=1 ## Purpose of this PR 1. Make git module command completely safe (risky user inputs won't be passed as argument option anymore) 2. Avoid low-level mistakes like https://github.com/go-gitea/gitea/pull/22098#discussion_r1045234918 3. Remove deprecated and dirty `CmdArgCheck` function, hide the `CmdArg` type 4. Simplify code when using git command ## The main idea of this PR * Move the `git.CmdArg` to the `internal` package, then no other package except `git` could use it. Then developers could never do `AddArguments(git.CmdArg(userInput))` any more. * Introduce `git.ToTrustedCmdArgs`, it's for user-provided and already trusted arguments. It's only used in a few cases, for example: use git arguments from config file, help unit test with some arguments. * Introduce `AddOptionValues` and `AddOptionFormat`, they make code more clear and simple: * Before: `AddArguments("-m").AddDynamicArguments(message)` * After: `AddOptionValues("-m", message)` * - * Before: `AddArguments(git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email)))` * After: `AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email)` ## FAQ ### Why these changes were not done in #21535 ? #21535 is mainly a search&replace, it did its best to not change too much logic. Making the framework better needs a lot of changes, so this separate PR is needed as the second step. ### The naming of `AddOptionXxx` According to git's manual, the `--xxx` part is called `option`. ### How can it guarantee that `internal.CmdArg` won't be not misused? Go's specification guarantees that. Trying to access other package's internal package causes compilation error. And, `golangci-lint` also denies the git/internal package. Only the `git/command.go` can use it carefully. ### There is still a `ToTrustedCmdArgs`, will it still allow developers to make mistakes and pass untrusted arguments? Generally speaking, no. Because when using `ToTrustedCmdArgs`, the code will be very complex (see the changes for examples). Then developers and reviewers can know that something might be unreasonable. ### Why there was a `CmdArgCheck` and why it's removed? At the moment of #21535, to reduce unnecessary changes, `CmdArgCheck` was introduced as a hacky patch. Now, almost all code could be written as `cmd := NewCommand(); cmd.AddXxx(...)`, then there is no need for `CmdArgCheck` anymore. ### Why many codes for `signArg == ""` is deleted? Because in the old code, `signArg` could never be empty string, it's either `-S[key-id]` or `--no-gpg-sign`. So the `signArg == ""` is just dead code. --------- Co-authored-by: Lunny Xiao --- .golangci.yml | 1 + modules/git/command.go | 97 ++++++++++---- modules/git/command_test.go | 11 ++ modules/git/commit.go | 18 +-- modules/git/git.go | 2 +- modules/git/internal/cmdarg.go | 9 ++ modules/git/repo.go | 2 +- modules/git/repo_archive.go | 4 +- modules/git/repo_attribute.go | 40 +++--- modules/git/repo_blame.go | 8 +- modules/git/repo_branch_gogit.go | 4 +- modules/git/repo_branch_nogogit.go | 18 +-- modules/git/repo_commit.go | 90 ++++++------- modules/git/repo_compare.go | 14 +- modules/git/repo_stats.go | 4 +- modules/git/repo_tag.go | 4 +- modules/git/repo_tag_nogogit.go | 2 +- modules/git/repo_tree.go | 3 +- modules/git/tree_nogogit.go | 13 +- modules/gitgraph/graph.go | 10 +- modules/repository/init.go | 9 +- routers/web/repo/blame.go | 2 +- routers/web/repo/compare.go | 2 +- routers/web/repo/http.go | 6 +- routers/web/repo/lfs.go | 2 +- routers/web/repo/view.go | 2 +- services/cron/tasks_basic.go | 6 +- services/cron/tasks_extended.go | 6 +- services/gitdiff/gitdiff.go | 80 +++++------- services/gitdiff/gitdiff_test.go | 2 +- services/mirror/mirror_pull.go | 10 +- services/mirror/mirror_push.go | 4 +- services/pull/merge.go | 92 +++++-------- services/pull/patch.go | 121 +++++++++--------- services/repository/check.go | 13 +- services/repository/files/patch.go | 8 +- services/repository/files/temp_repo.go | 14 +- services/repository/files/update.go | 2 +- services/repository/files/upload.go | 2 +- .../git_helper_for_declarative_test.go | 20 +-- tests/integration/git_test.go | 2 +- tests/integration/pull_merge_test.go | 2 +- 42 files changed, 379 insertions(+), 382 deletions(-) create mode 100644 modules/git/internal/cmdarg.go diff --git a/.golangci.yml b/.golangci.yml index caa237370e375..aa04e0a8efd31 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -84,6 +84,7 @@ linters-settings: - github.com/unknwon/com: "use gitea's util and replacements" - io/ioutil: "use os or io instead" - golang.org/x/exp: "it's experimental and unreliable." + - code.gitea.io/gitea/modules/git/internal: "do not use the internal package, use AddXxx function instead" issues: max-issues-per-linter: 0 diff --git a/modules/git/command.go b/modules/git/command.go index d88fcd1a8c807..0bc8103116a0b 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -16,14 +16,20 @@ import ( "time" "unsafe" + "code.gitea.io/gitea/modules/git/internal" //nolint:depguard // only this file can use the internal type CmdArg, other files and packages should use AddXxx functions "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/util" ) +// TrustedCmdArgs returns the trusted arguments for git command. +// It's mainly for passing user-provided and trusted arguments to git command +// In most cases, it shouldn't be used. Use AddXxx function instead +type TrustedCmdArgs []internal.CmdArg + var ( // globalCommandArgs global command args for external package setting - globalCommandArgs []CmdArg + globalCommandArgs TrustedCmdArgs // defaultCommandExecutionTimeout default command execution timeout duration defaultCommandExecutionTimeout = 360 * time.Second @@ -42,8 +48,6 @@ type Command struct { brokenArgs []string } -type CmdArg string - func (c *Command) String() string { if len(c.args) == 0 { return c.name @@ -53,7 +57,7 @@ func (c *Command) String() string { // NewCommand creates and returns a new Git Command based on given command and arguments. // Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead. -func NewCommand(ctx context.Context, args ...CmdArg) *Command { +func NewCommand(ctx context.Context, args ...internal.CmdArg) *Command { // Make an explicit copy of globalCommandArgs, otherwise append might overwrite it cargs := make([]string, 0, len(globalCommandArgs)+len(args)) for _, arg := range globalCommandArgs { @@ -70,15 +74,9 @@ func NewCommand(ctx context.Context, args ...CmdArg) *Command { } } -// NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args -// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead. -func NewCommandNoGlobals(args ...CmdArg) *Command { - return NewCommandContextNoGlobals(DefaultContext, args...) -} - // NewCommandContextNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args // Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead. -func NewCommandContextNoGlobals(ctx context.Context, args ...CmdArg) *Command { +func NewCommandContextNoGlobals(ctx context.Context, args ...internal.CmdArg) *Command { cargs := make([]string, 0, len(args)) for _, arg := range args { cargs = append(cargs, string(arg)) @@ -96,27 +94,70 @@ func (c *Command) SetParentContext(ctx context.Context) *Command { return c } -// SetDescription sets the description for this command which be returned on -// c.String() +// SetDescription sets the description for this command which be returned on c.String() func (c *Command) SetDescription(desc string) *Command { c.desc = desc return c } -// AddArguments adds new git argument(s) to the command. Each argument must be safe to be trusted. -// User-provided arguments should be passed to AddDynamicArguments instead. -func (c *Command) AddArguments(args ...CmdArg) *Command { +// isSafeArgumentValue checks if the argument is safe to be used as a value (not an option) +func isSafeArgumentValue(s string) bool { + return s == "" || s[0] != '-' +} + +// isValidArgumentOption checks if the argument is a valid option (starting with '-'). +// It doesn't check whether the option is supported or not +func isValidArgumentOption(s string) bool { + return s != "" && s[0] == '-' +} + +// AddArguments adds new git arguments (option/value) to the command. It only accepts string literals, or trusted CmdArg. +// Type CmdArg is in the internal package, so it can not be used outside of this package directly, +// it makes sure that user-provided arguments won't cause RCE risks. +// User-provided arguments should be passed by other AddXxx functions +func (c *Command) AddArguments(args ...internal.CmdArg) *Command { for _, arg := range args { c.args = append(c.args, string(arg)) } return c } -// AddDynamicArguments adds new dynamic argument(s) to the command. -// The arguments may come from user input and can not be trusted, so no leading '-' is allowed to avoid passing options +// AddOptionValues adds a new option with a list of non-option values +// For example: AddOptionValues("--opt", val) means 2 arguments: {"--opt", val}. +// The values are treated as dynamic argument values. It equals to: AddArguments("--opt") then AddDynamicArguments(val). +func (c *Command) AddOptionValues(opt internal.CmdArg, args ...string) *Command { + if !isValidArgumentOption(string(opt)) { + c.brokenArgs = append(c.brokenArgs, string(opt)) + return c + } + c.args = append(c.args, string(opt)) + c.AddDynamicArguments(args...) + return c +} + +// AddOptionFormat adds a new option with a format string and arguments +// For example: AddOptionFormat("--opt=%s %s", val1, val2) means 1 argument: {"--opt=val1 val2"}. +func (c *Command) AddOptionFormat(opt string, args ...any) *Command { + if !isValidArgumentOption(opt) { + c.brokenArgs = append(c.brokenArgs, opt) + return c + } + // a quick check to make sure the format string matches the number of arguments, to find low-level mistakes ASAP + if strings.Count(strings.ReplaceAll(opt, "%%", ""), "%") != len(args) { + c.brokenArgs = append(c.brokenArgs, opt) + return c + } + s := fmt.Sprintf(opt, args...) + c.args = append(c.args, s) + return c +} + +// AddDynamicArguments adds new dynamic argument values to the command. +// The arguments may come from user input and can not be trusted, so no leading '-' is allowed to avoid passing options. +// TODO: in the future, this function can be renamed to AddArgumentValues func (c *Command) AddDynamicArguments(args ...string) *Command { for _, arg := range args { - if arg != "" && arg[0] == '-' { + if !isSafeArgumentValue(arg) { c.brokenArgs = append(c.brokenArgs, arg) } } @@ -137,14 +178,14 @@ func (c *Command) AddDashesAndList(list ...string) *Command { return c } -// CmdArgCheck checks whether the string is safe to be used as a dynamic argument. -// It panics if the check fails. Usually it should not be used, it's just for refactoring purpose -// deprecated -func CmdArgCheck(s string) CmdArg { - if s != "" && s[0] == '-' { - panic("invalid git cmd argument: " + s) +// ToTrustedCmdArgs converts a list of strings (trusted as argument) to TrustedCmdArgs +// In most cases, it shouldn't be used. Use AddXxx function instead +func ToTrustedCmdArgs(args []string) TrustedCmdArgs { + ret := make(TrustedCmdArgs, len(args)) + for i, arg := range args { + ret[i] = internal.CmdArg(arg) } - return CmdArg(s) + return ret } // RunOpts represents parameters to run the command. If UseContextTimeout is specified, then Timeout is ignored. @@ -364,9 +405,9 @@ func (c *Command) RunStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunS } // AllowLFSFiltersArgs return globalCommandArgs with lfs filter, it should only be used for tests -func AllowLFSFiltersArgs() []CmdArg { +func AllowLFSFiltersArgs() TrustedCmdArgs { // Now here we should explicitly allow lfs filters to run - filteredLFSGlobalArgs := make([]CmdArg, len(globalCommandArgs)) + filteredLFSGlobalArgs := make(TrustedCmdArgs, len(globalCommandArgs)) j := 0 for _, arg := range globalCommandArgs { if strings.Contains(string(arg), "lfs") { diff --git a/modules/git/command_test.go b/modules/git/command_test.go index 2dca2d0d344d0..4e5f991d31125 100644 --- a/modules/git/command_test.go +++ b/modules/git/command_test.go @@ -41,3 +41,14 @@ func TestRunWithContextStd(t *testing.T) { assert.Empty(t, stderr) assert.Contains(t, stdout, "git version") } + +func TestGitArgument(t *testing.T) { + assert.True(t, isValidArgumentOption("-x")) + assert.True(t, isValidArgumentOption("--xx")) + assert.False(t, isValidArgumentOption("")) + assert.False(t, isValidArgumentOption("x")) + + assert.True(t, isSafeArgumentValue("")) + assert.True(t, isSafeArgumentValue("x")) + assert.False(t, isSafeArgumentValue("-x")) +} diff --git a/modules/git/commit.go b/modules/git/commit.go index 14710de612153..1f6289ed02224 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -9,7 +9,6 @@ import ( "bytes" "context" "errors" - "fmt" "io" "os/exec" "strconv" @@ -91,8 +90,8 @@ func AddChanges(repoPath string, all bool, files ...string) error { } // AddChangesWithArgs marks local changes to be ready for commit. -func AddChangesWithArgs(repoPath string, globalArgs []CmdArg, all bool, files ...string) error { - cmd := NewCommandNoGlobals(append(globalArgs, "add")...) +func AddChangesWithArgs(repoPath string, globalArgs TrustedCmdArgs, all bool, files ...string) error { + cmd := NewCommandContextNoGlobals(DefaultContext, globalArgs...).AddArguments("add") if all { cmd.AddArguments("--all") } @@ -111,17 +110,18 @@ type CommitChangesOptions struct { // CommitChanges commits local changes with given committer, author and message. // If author is nil, it will be the same as committer. func CommitChanges(repoPath string, opts CommitChangesOptions) error { - cargs := make([]CmdArg, len(globalCommandArgs)) + cargs := make(TrustedCmdArgs, len(globalCommandArgs)) copy(cargs, globalCommandArgs) return CommitChangesWithArgs(repoPath, cargs, opts) } // CommitChangesWithArgs commits local changes with given committer, author and message. // If author is nil, it will be the same as committer. -func CommitChangesWithArgs(repoPath string, args []CmdArg, opts CommitChangesOptions) error { - cmd := NewCommandNoGlobals(args...) +func CommitChangesWithArgs(repoPath string, args TrustedCmdArgs, opts CommitChangesOptions) error { + cmd := NewCommandContextNoGlobals(DefaultContext, args...) if opts.Committer != nil { - cmd.AddArguments("-c", CmdArg("user.name="+opts.Committer.Name), "-c", CmdArg("user.email="+opts.Committer.Email)) + cmd.AddOptionValues("-c", "user.name="+opts.Committer.Name) + cmd.AddOptionValues("-c", "user.email="+opts.Committer.Email) } cmd.AddArguments("commit") @@ -129,9 +129,9 @@ func CommitChangesWithArgs(repoPath string, args []CmdArg, opts CommitChangesOpt opts.Author = opts.Committer } if opts.Author != nil { - cmd.AddArguments(CmdArg(fmt.Sprintf("--author='%s <%s>'", opts.Author.Name, opts.Author.Email))) + cmd.AddOptionFormat("--author='%s <%s>'", opts.Author.Name, opts.Author.Email) } - cmd.AddArguments("-m").AddDynamicArguments(opts.Message) + cmd.AddOptionValues("-m", opts.Message) _, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath}) // No stderr but exit status 1 means nothing to commit. diff --git a/modules/git/git.go b/modules/git/git.go index f5919d82dcae1..2feb242ac5dd0 100644 --- a/modules/git/git.go +++ b/modules/git/git.go @@ -383,6 +383,6 @@ func configUnsetAll(key, value string) error { } // Fsck verifies the connectivity and validity of the objects in the database -func Fsck(ctx context.Context, repoPath string, timeout time.Duration, args ...CmdArg) error { +func Fsck(ctx context.Context, repoPath string, timeout time.Duration, args TrustedCmdArgs) error { return NewCommand(ctx, "fsck").AddArguments(args...).Run(&RunOpts{Timeout: timeout, Dir: repoPath}) } diff --git a/modules/git/internal/cmdarg.go b/modules/git/internal/cmdarg.go new file mode 100644 index 0000000000000..f8f3c2011ea9c --- /dev/null +++ b/modules/git/internal/cmdarg.go @@ -0,0 +1,9 @@ +// Copyright 2023 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package internal + +// CmdArg represents a command argument for git command, and it will be used for the git command directly without any further processing. +// In most cases, you should use the "AddXxx" functions to add arguments, but not use this type directly. +// Casting a risky (user-provided) string to CmdArg would cause security issues if it's injected with a "--xxx" argument. +type CmdArg string diff --git a/modules/git/repo.go b/modules/git/repo.go index 4ba40d20af0d5..e77a3a6ad8e39 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -115,7 +115,7 @@ func Clone(ctx context.Context, from, to string, opts CloneRepoOptions) error { } // CloneWithArgs original repository to target path. -func CloneWithArgs(ctx context.Context, args []CmdArg, from, to string, opts CloneRepoOptions) (err error) { +func CloneWithArgs(ctx context.Context, args TrustedCmdArgs, from, to string, opts CloneRepoOptions) (err error) { toDir := path.Dir(to) if err = os.MkdirAll(toDir, os.ModePerm); err != nil { return err diff --git a/modules/git/repo_archive.go b/modules/git/repo_archive.go index cff9724f00543..2b45a50f191b6 100644 --- a/modules/git/repo_archive.go +++ b/modules/git/repo_archive.go @@ -57,9 +57,9 @@ func (repo *Repository) CreateArchive(ctx context.Context, format ArchiveType, t cmd := NewCommand(ctx, "archive") if usePrefix { - cmd.AddArguments(CmdArg("--prefix=" + filepath.Base(strings.TrimSuffix(repo.Path, ".git")) + "/")) + cmd.AddOptionFormat("--prefix=%s", filepath.Base(strings.TrimSuffix(repo.Path, ".git"))+"/") } - cmd.AddArguments(CmdArg("--format=" + format.String())) + cmd.AddOptionFormat("--format=%s", format.String()) cmd.AddDynamicArguments(commitID) var stderr strings.Builder diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go index 404d9e502c042..e7d5fb68068bb 100644 --- a/modules/git/repo_attribute.go +++ b/modules/git/repo_attribute.go @@ -17,7 +17,7 @@ import ( type CheckAttributeOpts struct { CachedOnly bool AllAttributes bool - Attributes []CmdArg + Attributes []string Filenames []string IndexFile string WorkTree string @@ -48,7 +48,7 @@ func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[ } else { for _, attribute := range opts.Attributes { if attribute != "" { - cmd.AddArguments(attribute) + cmd.AddDynamicArguments(attribute) } } } @@ -95,7 +95,7 @@ func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[ // CheckAttributeReader provides a reader for check-attribute content that can be long running type CheckAttributeReader struct { // params - Attributes []CmdArg + Attributes []string Repo *Repository IndexFile string WorkTree string @@ -111,19 +111,6 @@ type CheckAttributeReader struct { // Init initializes the CheckAttributeReader func (c *CheckAttributeReader) Init(ctx context.Context) error { - cmdArgs := []CmdArg{"check-attr", "--stdin", "-z"} - - if len(c.IndexFile) > 0 { - cmdArgs = append(cmdArgs, "--cached") - c.env = append(c.env, "GIT_INDEX_FILE="+c.IndexFile) - } - - if len(c.WorkTree) > 0 { - c.env = append(c.env, "GIT_WORK_TREE="+c.WorkTree) - } - - c.env = append(c.env, "GIT_FLUSH=1") - if len(c.Attributes) == 0 { lw := new(nulSeparatedAttributeWriter) lw.attributes = make(chan attributeTriple) @@ -134,11 +121,22 @@ func (c *CheckAttributeReader) Init(ctx context.Context) error { return fmt.Errorf("no provided Attributes to check") } - cmdArgs = append(cmdArgs, c.Attributes...) - cmdArgs = append(cmdArgs, "--") - c.ctx, c.cancel = context.WithCancel(ctx) - c.cmd = NewCommand(c.ctx, cmdArgs...) + c.cmd = NewCommand(c.ctx, "check-attr", "--stdin", "-z") + + if len(c.IndexFile) > 0 { + c.cmd.AddArguments("--cached") + c.env = append(c.env, "GIT_INDEX_FILE="+c.IndexFile) + } + + if len(c.WorkTree) > 0 { + c.env = append(c.env, "GIT_WORK_TREE="+c.WorkTree) + } + + c.env = append(c.env, "GIT_FLUSH=1") + + // The empty "--" comes from #16773 , and it seems unnecessary because nothing else would be added later. + c.cmd.AddDynamicArguments(c.Attributes...).AddArguments("--") var err error @@ -294,7 +292,7 @@ func (repo *Repository) CheckAttributeReader(commitID string) (*CheckAttributeRe } checker := &CheckAttributeReader{ - Attributes: []CmdArg{"linguist-vendored", "linguist-generated", "linguist-language", "gitlab-language"}, + Attributes: []string{"linguist-vendored", "linguist-generated", "linguist-language", "gitlab-language"}, Repo: repo, IndexFile: indexFilename, WorkTree: worktree, diff --git a/modules/git/repo_blame.go b/modules/git/repo_blame.go index 7f44735f9f907..e25efa2d3bfa5 100644 --- a/modules/git/repo_blame.go +++ b/modules/git/repo_blame.go @@ -3,7 +3,9 @@ package git -import "fmt" +import ( + "fmt" +) // FileBlame return the Blame object of file func (repo *Repository) FileBlame(revision, path, file string) ([]byte, error) { @@ -14,8 +16,8 @@ func (repo *Repository) FileBlame(revision, path, file string) ([]byte, error) { // LineBlame returns the latest commit at the given line func (repo *Repository) LineBlame(revision, path, file string, line uint) (*Commit, error) { res, _, err := NewCommand(repo.Ctx, "blame"). - AddArguments(CmdArg(fmt.Sprintf("-L %d,%d", line, line))). - AddArguments("-p").AddDynamicArguments(revision). + AddOptionFormat("-L %d,%d", line, line). + AddOptionValues("-p", revision). AddDashesAndList(file).RunStdString(&RunOpts{Dir: path}) if err != nil { return nil, err diff --git a/modules/git/repo_branch_gogit.go b/modules/git/repo_branch_gogit.go index ca19d3827e586..f9896a7a09047 100644 --- a/modules/git/repo_branch_gogit.go +++ b/modules/git/repo_branch_gogit.go @@ -50,8 +50,8 @@ func (repo *Repository) IsBranchExist(name string) bool { return reference.Type() != plumbing.InvalidReference } -// GetBranches returns branches from the repository, skipping skip initial branches and -// returning at most limit branches, or all branches if limit is 0. +// GetBranches returns branches from the repository, skipping "skip" initial branches and +// returning at most "limit" branches, or all branches if "limit" is 0. func (repo *Repository) GetBranchNames(skip, limit int) ([]string, int, error) { var branchNames []string diff --git a/modules/git/repo_branch_nogogit.go b/modules/git/repo_branch_nogogit.go index 7559513c9b433..b1e7c8b73e640 100644 --- a/modules/git/repo_branch_nogogit.go +++ b/modules/git/repo_branch_nogogit.go @@ -59,10 +59,10 @@ func (repo *Repository) IsBranchExist(name string) bool { return repo.IsReferenceExist(BranchPrefix + name) } -// GetBranchNames returns branches from the repository, skipping skip initial branches and -// returning at most limit branches, or all branches if limit is 0. +// GetBranchNames returns branches from the repository, skipping "skip" initial branches and +// returning at most "limit" branches, or all branches if "limit" is 0. func (repo *Repository) GetBranchNames(skip, limit int) ([]string, int, error) { - return callShowRef(repo.Ctx, repo.Path, BranchPrefix, []CmdArg{BranchPrefix, "--sort=-committerdate"}, skip, limit) + return callShowRef(repo.Ctx, repo.Path, BranchPrefix, TrustedCmdArgs{BranchPrefix, "--sort=-committerdate"}, skip, limit) } // WalkReferences walks all the references from the repository @@ -73,19 +73,19 @@ func WalkReferences(ctx context.Context, repoPath string, walkfn func(sha1, refn // WalkReferences walks all the references from the repository // refType should be empty, ObjectTag or ObjectBranch. All other values are equivalent to empty. func (repo *Repository) WalkReferences(refType ObjectType, skip, limit int, walkfn func(sha1, refname string) error) (int, error) { - var args []CmdArg + var args TrustedCmdArgs switch refType { case ObjectTag: - args = []CmdArg{TagPrefix, "--sort=-taggerdate"} + args = TrustedCmdArgs{TagPrefix, "--sort=-taggerdate"} case ObjectBranch: - args = []CmdArg{BranchPrefix, "--sort=-committerdate"} + args = TrustedCmdArgs{BranchPrefix, "--sort=-committerdate"} } return walkShowRef(repo.Ctx, repo.Path, args, skip, limit, walkfn) } // callShowRef return refs, if limit = 0 it will not limit -func callShowRef(ctx context.Context, repoPath, trimPrefix string, extraArgs []CmdArg, skip, limit int) (branchNames []string, countAll int, err error) { +func callShowRef(ctx context.Context, repoPath, trimPrefix string, extraArgs TrustedCmdArgs, skip, limit int) (branchNames []string, countAll int, err error) { countAll, err = walkShowRef(ctx, repoPath, extraArgs, skip, limit, func(_, branchName string) error { branchName = strings.TrimPrefix(branchName, trimPrefix) branchNames = append(branchNames, branchName) @@ -95,7 +95,7 @@ func callShowRef(ctx context.Context, repoPath, trimPrefix string, extraArgs []C return branchNames, countAll, err } -func walkShowRef(ctx context.Context, repoPath string, extraArgs []CmdArg, skip, limit int, walkfn func(sha1, refname string) error) (countAll int, err error) { +func walkShowRef(ctx context.Context, repoPath string, extraArgs TrustedCmdArgs, skip, limit int, walkfn func(sha1, refname string) error) (countAll int, err error) { stdoutReader, stdoutWriter := io.Pipe() defer func() { _ = stdoutReader.Close() @@ -104,7 +104,7 @@ func walkShowRef(ctx context.Context, repoPath string, extraArgs []CmdArg, skip, go func() { stderrBuilder := &strings.Builder{} - args := []CmdArg{"for-each-ref", "--format=%(objectname) %(refname)"} + args := TrustedCmdArgs{"for-each-ref", "--format=%(objectname) %(refname)"} args = append(args, extraArgs...) err := NewCommand(ctx, args...).Run(&RunOpts{ Dir: repoPath, diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 8343e34843534..a62e0670fedca 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -89,7 +89,7 @@ func (repo *Repository) GetCommitByPath(relpath string) (*Commit, error) { func (repo *Repository) commitsByRange(id SHA1, page, pageSize int) ([]*Commit, error) { stdout, _, err := NewCommand(repo.Ctx, "log"). - AddArguments(CmdArg("--skip="+strconv.Itoa((page-1)*pageSize)), CmdArg("--max-count="+strconv.Itoa(pageSize)), prettyLogFormat). + AddOptionFormat("--skip=%d", (page-1)*pageSize).AddOptionFormat("--max-count=%d", pageSize).AddArguments(prettyLogFormat). AddDynamicArguments(id.String()). RunStdBytes(&RunOpts{Dir: repo.Path}) if err != nil { @@ -99,32 +99,36 @@ func (repo *Repository) commitsByRange(id SHA1, page, pageSize int) ([]*Commit, } func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Commit, error) { - // create new git log command with limit of 100 commis - cmd := NewCommand(repo.Ctx, "log", "-100", prettyLogFormat).AddDynamicArguments(id.String()) - // ignore case - args := []CmdArg{"-i"} + // add common arguments to git command + addCommonSearchArgs := func(c *Command) { + // ignore case + c.AddArguments("-i") + + // add authors if present in search query + if len(opts.Authors) > 0 { + for _, v := range opts.Authors { + c.AddOptionFormat("--author=%s", v) + } + } - // add authors if present in search query - if len(opts.Authors) > 0 { - for _, v := range opts.Authors { - args = append(args, CmdArg("--author="+v)) + // add committers if present in search query + if len(opts.Committers) > 0 { + for _, v := range opts.Committers { + c.AddOptionFormat("--committer=%s", v) + } } - } - // add committers if present in search query - if len(opts.Committers) > 0 { - for _, v := range opts.Committers { - args = append(args, CmdArg("--committer="+v)) + // add time constraints if present in search query + if len(opts.After) > 0 { + c.AddOptionFormat("--after=%s", opts.After) + } + if len(opts.Before) > 0 { + c.AddOptionFormat("--before=%s", opts.Before) } } - // add time constraints if present in search query - if len(opts.After) > 0 { - args = append(args, CmdArg("--after="+opts.After)) - } - if len(opts.Before) > 0 { - args = append(args, CmdArg("--before="+opts.Before)) - } + // create new git log command with limit of 100 commits + cmd := NewCommand(repo.Ctx, "log", "-100", prettyLogFormat).AddDynamicArguments(id.String()) // pretend that all refs along with HEAD were listed on command line as // https://git-scm.com/docs/git-log#Documentation/git-log.txt---all @@ -137,12 +141,12 @@ func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Co // note this is done only for command created above if len(opts.Keywords) > 0 { for _, v := range opts.Keywords { - cmd.AddArguments(CmdArg("--grep=" + v)) + cmd.AddOptionFormat("--grep=%s", v) } } // search for commits matching given constraints and keywords in commit msg - cmd.AddArguments(args...) + addCommonSearchArgs(cmd) stdout, _, err := cmd.RunStdBytes(&RunOpts{Dir: repo.Path}) if err != nil { return nil, err @@ -160,7 +164,7 @@ func (repo *Repository) searchCommits(id SHA1, opts SearchCommitsOptions) ([]*Co // create new git log command with 1 commit limit hashCmd := NewCommand(repo.Ctx, "log", "-1", prettyLogFormat) // add previous arguments except for --grep and --all - hashCmd.AddArguments(args...) + addCommonSearchArgs(hashCmd) // add keyword as hashCmd.AddDynamicArguments(v) @@ -213,8 +217,8 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) ( go func() { stderr := strings.Builder{} gitCmd := NewCommand(repo.Ctx, "rev-list"). - AddArguments(CmdArg("--max-count=" + strconv.Itoa(setting.Git.CommitsRangeSize*page))). - AddArguments(CmdArg("--skip=" + strconv.Itoa(skip))) + AddOptionFormat("--max-count=%d", setting.Git.CommitsRangeSize*page). + AddOptionFormat("--skip=%d", skip) gitCmd.AddDynamicArguments(revision) gitCmd.AddDashesAndList(file) err := gitCmd.Run(&RunOpts{ @@ -295,21 +299,21 @@ func (repo *Repository) CommitsBetweenLimit(last, before *Commit, limit, skip in var stdout []byte var err error if before == nil { - stdout, _, err = NewCommand(repo.Ctx, "rev-list", - "--max-count", CmdArg(strconv.Itoa(limit)), - "--skip", CmdArg(strconv.Itoa(skip))). + stdout, _, err = NewCommand(repo.Ctx, "rev-list"). + AddOptionValues("--max-count", strconv.Itoa(limit)). + AddOptionValues("--skip", strconv.Itoa(skip)). AddDynamicArguments(last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path}) } else { - stdout, _, err = NewCommand(repo.Ctx, "rev-list", - "--max-count", CmdArg(strconv.Itoa(limit)), - "--skip", CmdArg(strconv.Itoa(skip))). + stdout, _, err = NewCommand(repo.Ctx, "rev-list"). + AddOptionValues("--max-count", strconv.Itoa(limit)). + AddOptionValues("--skip", strconv.Itoa(skip)). AddDynamicArguments(before.ID.String() + ".." + last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path}) if err != nil && strings.Contains(err.Error(), "no merge base") { // future versions of git >= 2.28 are likely to return an error if before and last have become unrelated. // previously it would return the results of git rev-list --max-count n before last so let's try that... - stdout, _, err = NewCommand(repo.Ctx, "rev-list", - "--max-count", CmdArg(strconv.Itoa(limit)), - "--skip", CmdArg(strconv.Itoa(skip))). + stdout, _, err = NewCommand(repo.Ctx, "rev-list"). + AddOptionValues("--max-count", strconv.Itoa(limit)). + AddOptionValues("--skip", strconv.Itoa(skip)). AddDynamicArguments(before.ID.String(), last.ID.String()).RunStdBytes(&RunOpts{Dir: repo.Path}) } } @@ -349,12 +353,11 @@ func (repo *Repository) CommitsCountBetween(start, end string) (int64, error) { // commitsBefore the limit is depth, not total number of returned commits. func (repo *Repository) commitsBefore(id SHA1, limit int) ([]*Commit, error) { - cmd := NewCommand(repo.Ctx, "log") + cmd := NewCommand(repo.Ctx, "log", prettyLogFormat) if limit > 0 { - cmd.AddArguments(CmdArg("-"+strconv.Itoa(limit)), prettyLogFormat).AddDynamicArguments(id.String()) - } else { - cmd.AddArguments(prettyLogFormat).AddDynamicArguments(id.String()) + cmd.AddOptionFormat("-%d", limit) } + cmd.AddDynamicArguments(id.String()) stdout, _, runErr := cmd.RunStdBytes(&RunOpts{Dir: repo.Path}) if runErr != nil { @@ -393,10 +396,9 @@ func (repo *Repository) getCommitsBeforeLimit(id SHA1, num int) ([]*Commit, erro func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error) { if CheckGitVersionAtLeast("2.7.0") == nil { - stdout, _, err := NewCommand(repo.Ctx, "for-each-ref", - CmdArg("--count="+strconv.Itoa(limit)), - "--format=%(refname:strip=2)", "--contains"). - AddDynamicArguments(commit.ID.String(), BranchPrefix). + stdout, _, err := NewCommand(repo.Ctx, "for-each-ref", "--format=%(refname:strip=2)"). + AddOptionFormat("--count=%d", limit). + AddOptionValues("--contains", commit.ID.String(), BranchPrefix). RunStdString(&RunOpts{Dir: repo.Path}) if err != nil { return nil, err @@ -406,7 +408,7 @@ func (repo *Repository) getBranches(commit *Commit, limit int) ([]string, error) return branches, nil } - stdout, _, err := NewCommand(repo.Ctx, "branch", "--contains").AddDynamicArguments(commit.ID.String()).RunStdString(&RunOpts{Dir: repo.Path}) + stdout, _, err := NewCommand(repo.Ctx, "branch").AddOptionValues("--contains", commit.ID.String()).RunStdString(&RunOpts{Dir: repo.Path}) if err != nil { return nil, err } diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index b1b55c88a4446..9a4d66f2f521b 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -172,25 +172,21 @@ func (repo *Repository) GetDiffNumChangedFiles(base, head string, directComparis // GetDiffShortStat counts number of changed files, number of additions and deletions func (repo *Repository) GetDiffShortStat(base, head string) (numFiles, totalAdditions, totalDeletions int, err error) { - numFiles, totalAdditions, totalDeletions, err = GetDiffShortStat(repo.Ctx, repo.Path, CmdArgCheck(base+"..."+head)) + numFiles, totalAdditions, totalDeletions, err = GetDiffShortStat(repo.Ctx, repo.Path, nil, base+"..."+head) if err != nil && strings.Contains(err.Error(), "no merge base") { - return GetDiffShortStat(repo.Ctx, repo.Path, CmdArgCheck(base), CmdArgCheck(head)) + return GetDiffShortStat(repo.Ctx, repo.Path, nil, base, head) } return numFiles, totalAdditions, totalDeletions, err } // GetDiffShortStat counts number of changed files, number of additions and deletions -func GetDiffShortStat(ctx context.Context, repoPath string, args ...CmdArg) (numFiles, totalAdditions, totalDeletions int, err error) { +func GetDiffShortStat(ctx context.Context, repoPath string, trustedArgs TrustedCmdArgs, dynamicArgs ...string) (numFiles, totalAdditions, totalDeletions int, err error) { // Now if we call: // $ git diff --shortstat 1ebb35b98889ff77299f24d82da426b434b0cca0...788b8b1440462d477f45b0088875 // we get: // " 9902 files changed, 2034198 insertions(+), 298800 deletions(-)\n" - args = append([]CmdArg{ - "diff", - "--shortstat", - }, args...) - - stdout, _, err := NewCommand(ctx, args...).RunStdString(&RunOpts{Dir: repoPath}) + cmd := NewCommand(ctx, "diff", "--shortstat").AddArguments(trustedArgs...).AddDynamicArguments(dynamicArgs...) + stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath}) if err != nil { return 0, 0, 0, err } diff --git a/modules/git/repo_stats.go b/modules/git/repo_stats.go index d6e91f25a9a60..41f94e24f99ca 100644 --- a/modules/git/repo_stats.go +++ b/modules/git/repo_stats.go @@ -40,7 +40,7 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) since := fromTime.Format(time.RFC3339) - stdout, _, runErr := NewCommand(repo.Ctx, "rev-list", "--count", "--no-merges", "--branches=*", "--date=iso", CmdArg(fmt.Sprintf("--since='%s'", since))).RunStdString(&RunOpts{Dir: repo.Path}) + stdout, _, runErr := NewCommand(repo.Ctx, "rev-list", "--count", "--no-merges", "--branches=*", "--date=iso").AddOptionFormat("--since='%s'", since).RunStdString(&RunOpts{Dir: repo.Path}) if runErr != nil { return nil, runErr } @@ -60,7 +60,7 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) _ = stdoutWriter.Close() }() - gitCmd := NewCommand(repo.Ctx, "log", "--numstat", "--no-merges", "--pretty=format:---%n%h%n%aN%n%aE%n", "--date=iso", CmdArg(fmt.Sprintf("--since='%s'", since))) + gitCmd := NewCommand(repo.Ctx, "log", "--numstat", "--no-merges", "--pretty=format:---%n%h%n%aN%n%aE%n", "--date=iso").AddOptionFormat("--since='%s'", since) if len(branch) == 0 { gitCmd.AddArguments("--branches=*") } else { diff --git a/modules/git/repo_tag.go b/modules/git/repo_tag.go index 8aa06545d4cb1..ae877f0211002 100644 --- a/modules/git/repo_tag.go +++ b/modules/git/repo_tag.go @@ -121,7 +121,9 @@ func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) { rc := &RunOpts{Dir: repo.Path, Stdout: stdoutWriter, Stderr: &stderr} go func() { - err := NewCommand(repo.Ctx, "for-each-ref", CmdArg("--format="+forEachRefFmt.Flag()), "--sort", "-*creatordate", "refs/tags").Run(rc) + err := NewCommand(repo.Ctx, "for-each-ref"). + AddOptionFormat("--format=%s", forEachRefFmt.Flag()). + AddArguments("--sort", "-*creatordate", "refs/tags").Run(rc) if err != nil { _ = stdoutWriter.CloseWithError(ConcatenateError(err, stderr.String())) } else { diff --git a/modules/git/repo_tag_nogogit.go b/modules/git/repo_tag_nogogit.go index d3331cf9b73e1..9080ffcfd7820 100644 --- a/modules/git/repo_tag_nogogit.go +++ b/modules/git/repo_tag_nogogit.go @@ -25,7 +25,7 @@ func (repo *Repository) IsTagExist(name string) bool { // GetTags returns all tags of the repository. // returning at most limit tags, or all if limit is 0. func (repo *Repository) GetTags(skip, limit int) (tags []string, err error) { - tags, _, err = callShowRef(repo.Ctx, repo.Path, TagPrefix, []CmdArg{TagPrefix, "--sort=-taggerdate"}, skip, limit) + tags, _, err = callShowRef(repo.Ctx, repo.Path, TagPrefix, TrustedCmdArgs{TagPrefix, "--sort=-taggerdate"}, skip, limit) return tags, err } diff --git a/modules/git/repo_tree.go b/modules/git/repo_tree.go index 5fea5c0aea4bd..63c33379bf5dc 100644 --- a/modules/git/repo_tree.go +++ b/modules/git/repo_tree.go @@ -6,7 +6,6 @@ package git import ( "bytes" - "fmt" "os" "strings" "time" @@ -45,7 +44,7 @@ func (repo *Repository) CommitTree(author, committer *Signature, tree *Tree, opt _, _ = messageBytes.WriteString("\n") if opts.KeyID != "" || opts.AlwaysSign { - cmd.AddArguments(CmdArg(fmt.Sprintf("-S%s", opts.KeyID))) + cmd.AddOptionFormat("-S%s", opts.KeyID) } if opts.NoGPGSign { diff --git a/modules/git/tree_nogogit.go b/modules/git/tree_nogogit.go index 185317e7a7096..ef598d7e91327 100644 --- a/modules/git/tree_nogogit.go +++ b/modules/git/tree_nogogit.go @@ -100,14 +100,15 @@ func (t *Tree) ListEntries() (Entries, error) { // listEntriesRecursive returns all entries of current tree recursively including all subtrees // extraArgs could be "-l" to get the size, which is slower -func (t *Tree) listEntriesRecursive(extraArgs ...CmdArg) (Entries, error) { +func (t *Tree) listEntriesRecursive(extraArgs TrustedCmdArgs) (Entries, error) { if t.entriesRecursiveParsed { return t.entriesRecursive, nil } - args := append([]CmdArg{"ls-tree", "-t", "-r"}, extraArgs...) - args = append(args, CmdArg(t.ID.String())) - stdout, _, runErr := NewCommand(t.repo.Ctx, args...).RunStdBytes(&RunOpts{Dir: t.repo.Path}) + stdout, _, runErr := NewCommand(t.repo.Ctx, "ls-tree", "-t", "-r"). + AddArguments(extraArgs...). + AddDynamicArguments(t.ID.String()). + RunStdBytes(&RunOpts{Dir: t.repo.Path}) if runErr != nil { return nil, runErr } @@ -123,10 +124,10 @@ func (t *Tree) listEntriesRecursive(extraArgs ...CmdArg) (Entries, error) { // ListEntriesRecursiveFast returns all entries of current tree recursively including all subtrees, no size func (t *Tree) ListEntriesRecursiveFast() (Entries, error) { - return t.listEntriesRecursive() + return t.listEntriesRecursive(nil) } // ListEntriesRecursiveWithSize returns all entries of current tree recursively including all subtrees, with size func (t *Tree) ListEntriesRecursiveWithSize() (Entries, error) { - return t.listEntriesRecursive("--long") + return t.listEntriesRecursive(TrustedCmdArgs{"--long"}) } diff --git a/modules/gitgraph/graph.go b/modules/gitgraph/graph.go index baedfe5980632..331ad6b218de9 100644 --- a/modules/gitgraph/graph.go +++ b/modules/gitgraph/graph.go @@ -7,7 +7,6 @@ import ( "bufio" "bytes" "context" - "fmt" "os" "strings" @@ -33,12 +32,9 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo graphCmd.AddArguments("--all") } - graphCmd.AddArguments( - "-C", - "-M", - git.CmdArg(fmt.Sprintf("-n %d", setting.UI.GraphMaxCommitNum*page)), - "--date=iso", - git.CmdArg(fmt.Sprintf("--pretty=format:%s", format))) + graphCmd.AddArguments("-C", "-M", "--date=iso"). + AddOptionFormat("-n %d", setting.UI.GraphMaxCommitNum*page). + AddOptionFormat("--pretty=format:%s", format) if len(branches) > 0 { graphCmd.AddDynamicArguments(branches...) diff --git a/modules/repository/init.go b/modules/repository/init.go index 2b0d0be7bc99c..5705fe5b998e4 100644 --- a/modules/repository/init.go +++ b/modules/repository/init.go @@ -316,14 +316,13 @@ func initRepoCommit(ctx context.Context, tmpPath string, repo *repo_model.Reposi return fmt.Errorf("git add --all: %w", err) } - cmd := git.NewCommand(ctx, - "commit", git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email)), - "-m", "Initial commit", - ) + cmd := git.NewCommand(ctx, "commit"). + AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email). + AddOptionValues("-m", "Initial commit") sign, keyID, signer, _ := asymkey_service.SignInitialCommit(ctx, tmpPath, u) if sign { - cmd.AddArguments(git.CmdArg("-S" + keyID)) + cmd.AddOptionFormat("-S%s", keyID) if repo.GetTrustModel() == repo_model.CommitterTrustModel || repo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel { // need to set the committer to the KeyID owner diff --git a/routers/web/repo/blame.go b/routers/web/repo/blame.go index 50bfa9d3bd289..def7cfcadbe6c 100644 --- a/routers/web/repo/blame.go +++ b/routers/web/repo/blame.go @@ -217,7 +217,7 @@ func renderBlame(ctx *context.Context, blameParts []git.BlamePart, commitNames m filename2attribute2info, err := ctx.Repo.GitRepo.CheckAttribute(git.CheckAttributeOpts{ CachedOnly: true, - Attributes: []git.CmdArg{"linguist-language", "gitlab-language"}, + Attributes: []string{"linguist-language", "gitlab-language"}, Filenames: []string{ctx.Repo.TreePath}, IndexFile: indexFilename, WorkTree: worktree, diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 35f923d561ea5..c4b8c814e6756 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -560,7 +560,7 @@ func ParseCompareInfo(ctx *context.Context) *CompareInfo { func PrepareCompareDiff( ctx *context.Context, ci *CompareInfo, - whitespaceBehavior git.CmdArg, + whitespaceBehavior git.TrustedCmdArgs, ) bool { var ( repo = ctx.Repo.Repository diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go index 2c91ed6178e05..89c86e764e3cf 100644 --- a/routers/web/repo/http.go +++ b/routers/web/repo/http.go @@ -498,7 +498,8 @@ func serviceRPC(ctx gocontext.Context, h serviceHandler, service string) { } var stderr bytes.Buffer - cmd := git.NewCommand(h.r.Context(), git.CmdArgCheck(service), "--stateless-rpc").AddDynamicArguments(h.dir) + // the service is generated by ourselves, so it's safe to trust it + cmd := git.NewCommand(h.r.Context(), git.ToTrustedCmdArgs([]string{service})...).AddArguments("--stateless-rpc").AddDynamicArguments(h.dir) cmd.SetDescription(fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir)) if err := cmd.Run(&git.RunOpts{ Dir: h.dir, @@ -570,7 +571,8 @@ func GetInfoRefs(ctx *context.Context) { } h.environ = append(os.Environ(), h.environ...) - refs, _, err := git.NewCommand(ctx, git.CmdArgCheck(service), "--stateless-rpc", "--advertise-refs", ".").RunStdBytes(&git.RunOpts{Env: h.environ, Dir: h.dir}) + // the service is generated by ourselves, so we can trust it + refs, _, err := git.NewCommand(ctx, git.ToTrustedCmdArgs([]string{service})...).AddArguments("--stateless-rpc", "--advertise-refs", ".").RunStdBytes(&git.RunOpts{Env: h.environ, Dir: h.dir}) if err != nil { log.Error(fmt.Sprintf("%v - %s", err, string(refs))) } diff --git a/routers/web/repo/lfs.go b/routers/web/repo/lfs.go index 41d5edcdd8ca0..869a69c377219 100644 --- a/routers/web/repo/lfs.go +++ b/routers/web/repo/lfs.go @@ -146,7 +146,7 @@ func LFSLocks(ctx *context.Context) { } name2attribute2info, err := gitRepo.CheckAttribute(git.CheckAttributeOpts{ - Attributes: []git.CmdArg{"lockable"}, + Attributes: []string{"lockable"}, Filenames: filenames, CachedOnly: true, }) diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index 769e4e895cd99..f314902374c38 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -509,7 +509,7 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry, treeLink, rawLink st filename2attribute2info, err := ctx.Repo.GitRepo.CheckAttribute(git.CheckAttributeOpts{ CachedOnly: true, - Attributes: []git.CmdArg{"linguist-language", "gitlab-language"}, + Attributes: []string{"linguist-language", "gitlab-language"}, Filenames: []string{ctx.Repo.TreePath}, IndexFile: indexFilename, WorkTree: worktree, diff --git a/services/cron/tasks_basic.go b/services/cron/tasks_basic.go index 05aef6623d4f7..aad0e395912ef 100644 --- a/services/cron/tasks_basic.go +++ b/services/cron/tasks_basic.go @@ -59,11 +59,7 @@ func registerRepoHealthCheck() { }, func(ctx context.Context, _ *user_model.User, config Config) error { rhcConfig := config.(*RepoHealthCheckConfig) // the git args are set by config, they can be safe to be trusted - args := make([]git.CmdArg, 0, len(rhcConfig.Args)) - for _, arg := range rhcConfig.Args { - args = append(args, git.CmdArg(arg)) - } - return repo_service.GitFsckRepos(ctx, rhcConfig.Timeout, args) + return repo_service.GitFsckRepos(ctx, rhcConfig.Timeout, git.ToTrustedCmdArgs(rhcConfig.Args)) }) } diff --git a/services/cron/tasks_extended.go b/services/cron/tasks_extended.go index 520d940edf3c5..3e0dbd132eb31 100644 --- a/services/cron/tasks_extended.go +++ b/services/cron/tasks_extended.go @@ -61,11 +61,7 @@ func registerGarbageCollectRepositories() { }, func(ctx context.Context, _ *user_model.User, config Config) error { rhcConfig := config.(*RepoHealthCheckConfig) // the git args are set by config, they can be safe to be trusted - args := make([]git.CmdArg, 0, len(rhcConfig.Args)) - for _, arg := range rhcConfig.Args { - args = append(args, git.CmdArg(arg)) - } - return repo_service.GitGcRepos(ctx, rhcConfig.Timeout, args...) + return repo_service.GitGcRepos(ctx, rhcConfig.Timeout, git.ToTrustedCmdArgs(rhcConfig.Args)) }) } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index d3ee93ec945b7..4a74c1a8944e5 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1056,7 +1056,7 @@ type DiffOptions struct { MaxLines int MaxLineCharacters int MaxFiles int - WhitespaceBehavior git.CmdArg + WhitespaceBehavior git.TrustedCmdArgs DirectComparison bool } @@ -1071,38 +1071,22 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff return nil, err } - argsLength := 6 - if len(opts.WhitespaceBehavior) > 0 { - argsLength++ - } - if len(opts.SkipTo) > 0 { - argsLength++ - } - if len(files) > 0 { - argsLength += len(files) + 1 - } - - diffArgs := make([]git.CmdArg, 0, argsLength) + cmdDiff := git.NewCommand(gitRepo.Ctx) if (len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == git.EmptySHA) && commit.ParentCount() == 0 { - diffArgs = append(diffArgs, "diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M") - if len(opts.WhitespaceBehavior) != 0 { - diffArgs = append(diffArgs, opts.WhitespaceBehavior) - } - // append empty tree ref - diffArgs = append(diffArgs, "4b825dc642cb6eb9a060e54bf8d69288fbee4904") - diffArgs = append(diffArgs, git.CmdArgCheck(opts.AfterCommitID)) + cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). + AddArguments(opts.WhitespaceBehavior...). + AddArguments("4b825dc642cb6eb9a060e54bf8d69288fbee4904"). // append empty tree ref + AddDynamicArguments(opts.AfterCommitID) } else { actualBeforeCommitID := opts.BeforeCommitID if len(actualBeforeCommitID) == 0 { parentCommit, _ := commit.Parent(0) actualBeforeCommitID = parentCommit.ID.String() } - diffArgs = append(diffArgs, "diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M") - if len(opts.WhitespaceBehavior) != 0 { - diffArgs = append(diffArgs, opts.WhitespaceBehavior) - } - diffArgs = append(diffArgs, git.CmdArgCheck(actualBeforeCommitID)) - diffArgs = append(diffArgs, git.CmdArgCheck(opts.AfterCommitID)) + + cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). + AddArguments(opts.WhitespaceBehavior...). + AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID) opts.BeforeCommitID = actualBeforeCommitID } @@ -1111,16 +1095,11 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff // the skipping for us parsePatchSkipToFile := opts.SkipTo if opts.SkipTo != "" && git.CheckGitVersionAtLeast("2.31") == nil { - diffArgs = append(diffArgs, git.CmdArg("--skip-to="+opts.SkipTo)) + cmdDiff.AddOptionFormat("--skip-to=%s", opts.SkipTo) parsePatchSkipToFile = "" } - if len(files) > 0 { - diffArgs = append(diffArgs, "--") - for _, file := range files { - diffArgs = append(diffArgs, git.CmdArg(file)) // it's safe to cast it to CmdArg because there is a "--" before - } - } + cmdDiff.AddDashesAndList(files...) reader, writer := io.Pipe() defer func() { @@ -1128,10 +1107,9 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff _ = writer.Close() }() - go func(ctx context.Context, diffArgs []git.CmdArg, repoPath string, writer *io.PipeWriter) { - cmd := git.NewCommand(ctx, diffArgs...) - cmd.SetDescription(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) - if err := cmd.Run(&git.RunOpts{ + go func() { + cmdDiff.SetDescription(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) + if err := cmdDiff.Run(&git.RunOpts{ Timeout: time.Duration(setting.Git.Timeout.Default) * time.Second, Dir: repoPath, Stderr: os.Stderr, @@ -1141,7 +1119,7 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff } _ = writer.Close() - }(gitRepo.Ctx, diffArgs, repoPath, writer) + }() diff, err := ParsePatch(opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) if err != nil { @@ -1201,16 +1179,16 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff separator = ".." } - shortstatArgs := []git.CmdArg{git.CmdArgCheck(opts.BeforeCommitID + separator + opts.AfterCommitID)} + diffPaths := []string{opts.BeforeCommitID + separator + opts.AfterCommitID} if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == git.EmptySHA { - shortstatArgs = []git.CmdArg{git.EmptyTreeSHA, git.CmdArgCheck(opts.AfterCommitID)} + diffPaths = []string{git.EmptyTreeSHA, opts.AfterCommitID} } - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, shortstatArgs...) + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) if err != nil && strings.Contains(err.Error(), "no merge base") { // git >= 2.28 now returns an error if base and head have become unrelated. // previously it would return the results of git diff --shortstat base head so let's try that... - shortstatArgs = []git.CmdArg{git.CmdArgCheck(opts.BeforeCommitID), git.CmdArgCheck(opts.AfterCommitID)} - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, shortstatArgs...) + diffPaths = []string{opts.BeforeCommitID, opts.AfterCommitID} + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) } if err != nil { return nil, err @@ -1324,17 +1302,17 @@ func CommentMustAsDiff(c *issues_model.Comment) *Diff { } // GetWhitespaceFlag returns git diff flag for treating whitespaces -func GetWhitespaceFlag(whitespaceBehavior string) git.CmdArg { - whitespaceFlags := map[string]string{ - "ignore-all": "-w", - "ignore-change": "-b", - "ignore-eol": "--ignore-space-at-eol", - "show-all": "", +func GetWhitespaceFlag(whitespaceBehavior string) git.TrustedCmdArgs { + whitespaceFlags := map[string]git.TrustedCmdArgs{ + "ignore-all": {"-w"}, + "ignore-change": {"-b"}, + "ignore-eol": {"--ignore-space-at-eol"}, + "show-all": nil, } if flag, ok := whitespaceFlags[whitespaceBehavior]; ok { - return git.CmdArg(flag) + return flag } log.Warn("unknown whitespace behavior: %q, default to 'show-all'", whitespaceBehavior) - return "" + return nil } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 267f0e4cff6ad..eb9ed862e82f4 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -626,7 +626,7 @@ func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) { return } defer gitRepo.Close() - for _, behavior := range []git.CmdArg{"-w", "--ignore-space-at-eol", "-b", ""} { + for _, behavior := range []git.TrustedCmdArgs{{"-w"}, {"--ignore-space-at-eol"}, {"-b"}, nil} { diffs, err := GetDiff(gitRepo, &DiffOptions{ AfterCommitID: "bd7063cc7c04689c4d082183d32a604ed27a24f9", diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index 98e8d122a5dfa..7dee90352ea22 100644 --- a/services/mirror/mirror_pull.go +++ b/services/mirror/mirror_pull.go @@ -203,11 +203,11 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo log.Trace("SyncMirrors [repo: %-v]: running git remote update...", m.Repo) - gitArgs := []git.CmdArg{"remote", "update"} + cmd := git.NewCommand(ctx, "remote", "update") if m.EnablePrune { - gitArgs = append(gitArgs, "--prune") + cmd.AddArguments("--prune") } - gitArgs = append(gitArgs, git.CmdArgCheck(m.GetRemoteName())) + cmd.AddDynamicArguments(m.GetRemoteName()) remoteURL, remoteErr := git.GetRemoteURL(ctx, repoPath, m.GetRemoteName()) if remoteErr != nil { @@ -217,7 +217,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo stdoutBuilder := strings.Builder{} stderrBuilder := strings.Builder{} - if err := git.NewCommand(ctx, gitArgs...). + if err := cmd. SetDescription(fmt.Sprintf("Mirror.runSync: %s", m.Repo.FullName())). Run(&git.RunOpts{ Timeout: timeout, @@ -243,7 +243,7 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo // Successful prune - reattempt mirror stderrBuilder.Reset() stdoutBuilder.Reset() - if err = git.NewCommand(ctx, gitArgs...). + if err = cmd. SetDescription(fmt.Sprintf("Mirror.runSync: %s", m.Repo.FullName())). Run(&git.RunOpts{ Timeout: timeout, diff --git a/services/mirror/mirror_push.go b/services/mirror/mirror_push.go index c0c68a3f54161..2c1b00b60c294 100644 --- a/services/mirror/mirror_push.go +++ b/services/mirror/mirror_push.go @@ -37,10 +37,10 @@ func AddPushMirrorRemote(ctx context.Context, m *repo_model.PushMirror, addr str if _, _, err := cmd.RunStdString(&git.RunOpts{Dir: path}); err != nil { return err } - if _, _, err := git.NewCommand(ctx, "config", "--add", git.CmdArg("remote."+m.RemoteName+".push"), "+refs/heads/*:refs/heads/*").RunStdString(&git.RunOpts{Dir: path}); err != nil { + if _, _, err := git.NewCommand(ctx, "config", "--add").AddDynamicArguments("remote."+m.RemoteName+".push", "+refs/heads/*:refs/heads/*").RunStdString(&git.RunOpts{Dir: path}); err != nil { return err } - if _, _, err := git.NewCommand(ctx, "config", "--add", git.CmdArg("remote."+m.RemoteName+".push"), "+refs/tags/*:refs/tags/*").RunStdString(&git.RunOpts{Dir: path}); err != nil { + if _, _, err := git.NewCommand(ctx, "config", "--add").AddDynamicArguments("remote."+m.RemoteName+".push", "+refs/tags/*:refs/tags/*").RunStdString(&git.RunOpts{Dir: path}); err != nil { return err } return nil diff --git a/services/pull/merge.go b/services/pull/merge.go index 7ffbdb78b0207..edd5b601da934 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -370,16 +370,16 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode sig := doer.NewGitSig() committer := sig - // Determine if we should sign - var signArg git.CmdArg - sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, pr, doer, tmpBasePath, "HEAD", trackingBranch) + // Determine if we should sign. If no signKeyID, use --no-gpg-sign to countermand the sign config (from gitconfig) + var signArgs git.TrustedCmdArgs + sign, signKeyID, signer, _ := asymkey_service.SignMerge(ctx, pr, doer, tmpBasePath, "HEAD", trackingBranch) if sign { - signArg = git.CmdArg("-S" + keyID) if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel { committer = signer } + signArgs = git.ToTrustedCmdArgs([]string{"-S" + signKeyID}) } else { - signArg = git.CmdArg("--no-gpg-sign") + signArgs = append(signArgs, "--no-gpg-sign") } commitTimeStr := time.Now().Format(time.RFC3339) @@ -403,7 +403,7 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode return "", err } - if err := commitAndSignNoAuthor(ctx, pr, message, signArg, tmpBasePath, env); err != nil { + if err := commitAndSignNoAuthor(ctx, pr, message, signArgs, tmpBasePath, env); err != nil { log.Error("Unable to make final commit: %v", err) return "", err } @@ -505,7 +505,7 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode return "", err } if mergeStyle == repo_model.MergeStyleRebaseMerge { - if err := commitAndSignNoAuthor(ctx, pr, message, signArg, tmpBasePath, env); err != nil { + if err := commitAndSignNoAuthor(ctx, pr, message, signArgs, tmpBasePath, env); err != nil { log.Error("Unable to make final commit: %v", err) return "", err } @@ -523,35 +523,22 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode return "", fmt.Errorf("LoadPoster: %w", err) } sig := pr.Issue.Poster.NewGitSig() - if signArg == "" { - if err := git.NewCommand(ctx, "commit", git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email)), "-m").AddDynamicArguments(message). - Run(&git.RunOpts{ - Env: env, - Dir: tmpBasePath, - Stdout: &outbuf, - Stderr: &errbuf, - }); err != nil { - log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - return "", fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - } - } else { - if setting.Repository.PullRequest.AddCoCommitterTrailers && committer.String() != sig.String() { - // add trailer - message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String()) - } - if err := git.NewCommand(ctx, "commit"). - AddArguments(signArg). - AddArguments(git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email))). - AddArguments("-m").AddDynamicArguments(message). - Run(&git.RunOpts{ - Env: env, - Dir: tmpBasePath, - Stdout: &outbuf, - Stderr: &errbuf, - }); err != nil { - log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - return "", fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - } + if setting.Repository.PullRequest.AddCoCommitterTrailers && committer.String() != sig.String() { + // add trailer + message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String()) + } + if err := git.NewCommand(ctx, "commit"). + AddArguments(signArgs...). + AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email). + AddOptionValues("-m", message). + Run(&git.RunOpts{ + Env: env, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { + log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return "", fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } outbuf.Reset() errbuf.Reset() @@ -649,30 +636,17 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode return mergeCommitID, nil } -func commitAndSignNoAuthor(ctx context.Context, pr *issues_model.PullRequest, message string, signArg git.CmdArg, tmpBasePath string, env []string) error { +func commitAndSignNoAuthor(ctx context.Context, pr *issues_model.PullRequest, message string, signArgs git.TrustedCmdArgs, tmpBasePath string, env []string) error { var outbuf, errbuf strings.Builder - if signArg == "" { - if err := git.NewCommand(ctx, "commit", "-m").AddDynamicArguments(message). - Run(&git.RunOpts{ - Env: env, - Dir: tmpBasePath, - Stdout: &outbuf, - Stderr: &errbuf, - }); err != nil { - log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - } - } else { - if err := git.NewCommand(ctx, "commit").AddArguments(signArg).AddArguments("-m").AddDynamicArguments(message). - Run(&git.RunOpts{ - Env: env, - Dir: tmpBasePath, - Stdout: &outbuf, - Stderr: &errbuf, - }); err != nil { - log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) - } + if err := git.NewCommand(ctx, "commit").AddArguments(signArgs...).AddOptionValues("-m", message). + Run(&git.RunOpts{ + Env: env, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { + log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } return nil } diff --git a/services/pull/patch.go b/services/pull/patch.go index a3084196bbc90..c2ccc75bdccdd 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -376,16 +376,16 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * prConfig := prUnit.PullRequestsConfig() // 6. Prepare the arguments to apply the patch against the index - args := []git.CmdArg{"apply", "--check", "--cached"} + cmdApply := git.NewCommand(gitRepo.Ctx, "apply", "--check", "--cached") if prConfig.IgnoreWhitespaceConflicts { - args = append(args, "--ignore-whitespace") + cmdApply.AddArguments("--ignore-whitespace") } is3way := false if git.CheckGitVersionAtLeast("2.32.0") == nil { - args = append(args, "--3way") + cmdApply.AddArguments("--3way") is3way = true } - args = append(args, git.CmdArgCheck(patchPath)) + cmdApply.AddDynamicArguments(patchPath) // 7. Prep the pipe: // - Here we could do the equivalent of: @@ -407,71 +407,70 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * // 8. Run the check command conflict = false - err = git.NewCommand(gitRepo.Ctx, args...). - Run(&git.RunOpts{ - Dir: tmpBasePath, - Stderr: stderrWriter, - PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { - // Close the writer end of the pipe to begin processing - _ = stderrWriter.Close() - defer func() { - // Close the reader on return to terminate the git command if necessary - _ = stderrReader.Close() - }() - - const prefix = "error: patch failed:" - const errorPrefix = "error: " - const threewayFailed = "Failed to perform three-way merge..." - const appliedPatchPrefix = "Applied patch to '" - const withConflicts = "' with conflicts." - - conflicts := make(container.Set[string]) - - // Now scan the output from the command - scanner := bufio.NewScanner(stderrReader) - for scanner.Scan() { - line := scanner.Text() - log.Trace("PullRequest[%d].testPatch: stderr: %s", pr.ID, line) - if strings.HasPrefix(line, prefix) { - conflict = true - filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) - conflicts.Add(filepath) - } else if is3way && line == threewayFailed { - conflict = true - } else if strings.HasPrefix(line, errorPrefix) { - conflict = true - for _, suffix := range patchErrorSuffices { - if strings.HasSuffix(line, suffix) { - filepath := strings.TrimSpace(strings.TrimSuffix(line[len(errorPrefix):], suffix)) - if filepath != "" { - conflicts.Add(filepath) - } - break + err = cmdApply.Run(&git.RunOpts{ + Dir: tmpBasePath, + Stderr: stderrWriter, + PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { + // Close the writer end of the pipe to begin processing + _ = stderrWriter.Close() + defer func() { + // Close the reader on return to terminate the git command if necessary + _ = stderrReader.Close() + }() + + const prefix = "error: patch failed:" + const errorPrefix = "error: " + const threewayFailed = "Failed to perform three-way merge..." + const appliedPatchPrefix = "Applied patch to '" + const withConflicts = "' with conflicts." + + conflicts := make(container.Set[string]) + + // Now scan the output from the command + scanner := bufio.NewScanner(stderrReader) + for scanner.Scan() { + line := scanner.Text() + log.Trace("PullRequest[%d].testPatch: stderr: %s", pr.ID, line) + if strings.HasPrefix(line, prefix) { + conflict = true + filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) + conflicts.Add(filepath) + } else if is3way && line == threewayFailed { + conflict = true + } else if strings.HasPrefix(line, errorPrefix) { + conflict = true + for _, suffix := range patchErrorSuffices { + if strings.HasSuffix(line, suffix) { + filepath := strings.TrimSpace(strings.TrimSuffix(line[len(errorPrefix):], suffix)) + if filepath != "" { + conflicts.Add(filepath) } - } - } else if is3way && strings.HasPrefix(line, appliedPatchPrefix) && strings.HasSuffix(line, withConflicts) { - conflict = true - filepath := strings.TrimPrefix(strings.TrimSuffix(line, withConflicts), appliedPatchPrefix) - if filepath != "" { - conflicts.Add(filepath) + break } } - // only list 10 conflicted files - if len(conflicts) >= 10 { - break + } else if is3way && strings.HasPrefix(line, appliedPatchPrefix) && strings.HasSuffix(line, withConflicts) { + conflict = true + filepath := strings.TrimPrefix(strings.TrimSuffix(line, withConflicts), appliedPatchPrefix) + if filepath != "" { + conflicts.Add(filepath) } } + // only list 10 conflicted files + if len(conflicts) >= 10 { + break + } + } - if len(conflicts) > 0 { - pr.ConflictedFiles = make([]string, 0, len(conflicts)) - for key := range conflicts { - pr.ConflictedFiles = append(pr.ConflictedFiles, key) - } + if len(conflicts) > 0 { + pr.ConflictedFiles = make([]string, 0, len(conflicts)) + for key := range conflicts { + pr.ConflictedFiles = append(pr.ConflictedFiles, key) } + } - return nil - }, - }) + return nil + }, + }) // 9. Check if the found conflictedfiles is non-zero, "err" could be non-nil, so we should ignore it if we found conflicts. // Note: `"err" could be non-nil` is due that if enable 3-way merge, it doesn't return any error on found conflicts. diff --git a/services/repository/check.go b/services/repository/check.go index e9d65aea4a8ce..3a1f0b7f306bd 100644 --- a/services/repository/check.go +++ b/services/repository/check.go @@ -23,7 +23,7 @@ import ( ) // GitFsckRepos calls 'git fsck' to check repository health. -func GitFsckRepos(ctx context.Context, timeout time.Duration, args []git.CmdArg) error { +func GitFsckRepos(ctx context.Context, timeout time.Duration, args git.TrustedCmdArgs) error { log.Trace("Doing: GitFsck") if err := db.Iterate( @@ -47,10 +47,10 @@ func GitFsckRepos(ctx context.Context, timeout time.Duration, args []git.CmdArg) } // GitFsckRepo calls 'git fsck' to check an individual repository's health. -func GitFsckRepo(ctx context.Context, repo *repo_model.Repository, timeout time.Duration, args []git.CmdArg) error { +func GitFsckRepo(ctx context.Context, repo *repo_model.Repository, timeout time.Duration, args git.TrustedCmdArgs) error { log.Trace("Running health check on repository %-v", repo) repoPath := repo.RepoPath() - if err := git.Fsck(ctx, repoPath, timeout, args...); err != nil { + if err := git.Fsck(ctx, repoPath, timeout, args); err != nil { log.Warn("Failed to health check repository (%-v): %v", repo, err) if err = system_model.CreateRepositoryNotice("Failed to health check repository (%s): %v", repo.FullName(), err); err != nil { log.Error("CreateRepositoryNotice: %v", err) @@ -60,9 +60,8 @@ func GitFsckRepo(ctx context.Context, repo *repo_model.Repository, timeout time. } // GitGcRepos calls 'git gc' to remove unnecessary files and optimize the local repository -func GitGcRepos(ctx context.Context, timeout time.Duration, args ...git.CmdArg) error { +func GitGcRepos(ctx context.Context, timeout time.Duration, args git.TrustedCmdArgs) error { log.Trace("Doing: GitGcRepos") - args = append([]git.CmdArg{"gc"}, args...) if err := db.Iterate( ctx, @@ -86,9 +85,9 @@ func GitGcRepos(ctx context.Context, timeout time.Duration, args ...git.CmdArg) } // GitGcRepo calls 'git gc' to remove unnecessary files and optimize the local repository -func GitGcRepo(ctx context.Context, repo *repo_model.Repository, timeout time.Duration, args []git.CmdArg) error { +func GitGcRepo(ctx context.Context, repo *repo_model.Repository, timeout time.Duration, args git.TrustedCmdArgs) error { log.Trace("Running git gc on %-v", repo) - command := git.NewCommand(ctx, args...). + command := git.NewCommand(ctx, "gc").AddArguments(args...). SetDescription(fmt.Sprintf("Repository Garbage Collection: %s", repo.FullName())) var stdout string var err error diff --git a/services/repository/files/patch.go b/services/repository/files/patch.go index 73ee0fa815e4c..f65199cfcb94e 100644 --- a/services/repository/files/patch.go +++ b/services/repository/files/patch.go @@ -141,14 +141,12 @@ func ApplyDiffPatch(ctx context.Context, repo *repo_model.Repository, doer *user stdout := &strings.Builder{} stderr := &strings.Builder{} - args := []git.CmdArg{"apply", "--index", "--recount", "--cached", "--ignore-whitespace", "--whitespace=fix", "--binary"} - + cmdApply := git.NewCommand(ctx, "apply", "--index", "--recount", "--cached", "--ignore-whitespace", "--whitespace=fix", "--binary") if git.CheckGitVersionAtLeast("2.32") == nil { - args = append(args, "-3") + cmdApply.AddArguments("-3") } - cmd := git.NewCommand(ctx, args...) - if err := cmd.Run(&git.RunOpts{ + if err := cmdApply.Run(&git.RunOpts{ Dir: t.basePath, Stdout: stdout, Stderr: stderr, diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index 1f3375cdcc83c..a086d15a4f99e 100644 --- a/services/repository/files/temp_repo.go +++ b/services/repository/files/temp_repo.go @@ -233,11 +233,9 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(parent string, author, co _, _ = messageBytes.WriteString(message) _, _ = messageBytes.WriteString("\n") - var args []git.CmdArg + cmdCommitTree := git.NewCommand(t.ctx, "commit-tree").AddDynamicArguments(treeHash) if parent != "" { - args = []git.CmdArg{"commit-tree", git.CmdArgCheck(treeHash), "-p", git.CmdArgCheck(parent)} - } else { - args = []git.CmdArg{"commit-tree", git.CmdArgCheck(treeHash)} + cmdCommitTree.AddOptionValues("-p", parent) } var sign bool @@ -249,7 +247,7 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(parent string, author, co sign, keyID, signer, _ = asymkey_service.SignInitialCommit(t.ctx, t.repo.RepoPath(), author) } if sign { - args = append(args, git.CmdArg("-S"+keyID)) + cmdCommitTree.AddOptionFormat("-S%s", keyID) if t.repo.GetTrustModel() == repo_model.CommitterTrustModel || t.repo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel { if committerSig.Name != authorSig.Name || committerSig.Email != authorSig.Email { // Add trailers @@ -264,7 +262,7 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(parent string, author, co committerSig = signer } } else { - args = append(args, "--no-gpg-sign") + cmdCommitTree.AddArguments("--no-gpg-sign") } if signoff { @@ -281,7 +279,7 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(parent string, author, co stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) - if err := git.NewCommand(t.ctx, args...). + if err := cmdCommitTree. Run(&git.RunOpts{ Env: env, Dir: t.basePath, @@ -364,7 +362,7 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) { t.repo.FullName(), err, stderr) } - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(t.ctx, t.basePath, "--cached", "HEAD") + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(t.ctx, t.basePath, git.TrustedCmdArgs{"--cached"}, "HEAD") if err != nil { return nil, err } diff --git a/services/repository/files/update.go b/services/repository/files/update.go index 58b7a5e082b58..45a469239665f 100644 --- a/services/repository/files/update.go +++ b/services/repository/files/update.go @@ -370,7 +370,7 @@ func CreateOrUpdateRepoFile(ctx context.Context, repo *repo_model.Repository, do if setting.LFS.StartServer && hasOldBranch { // Check there is no way this can return multiple infos filename2attribute2info, err := t.gitRepo.CheckAttribute(git.CheckAttributeOpts{ - Attributes: []git.CmdArg{"filter"}, + Attributes: []string{"filter"}, Filenames: []string{treePath}, CachedOnly: true, }) diff --git a/services/repository/files/upload.go b/services/repository/files/upload.go index e7289dd60d23b..cf2f7019b68c1 100644 --- a/services/repository/files/upload.go +++ b/services/repository/files/upload.go @@ -96,7 +96,7 @@ func UploadRepoFiles(ctx context.Context, repo *repo_model.Repository, doer *use var filename2attribute2info map[string]map[string]string if setting.LFS.StartServer { filename2attribute2info, err = t.gitRepo.CheckAttribute(git.CheckAttributeOpts{ - Attributes: []git.CmdArg{"filter"}, + Attributes: []string{"filter"}, Filenames: names, CachedOnly: true, }) diff --git a/tests/integration/git_helper_for_declarative_test.go b/tests/integration/git_helper_for_declarative_test.go index 9e3ff9c4484d3..1e99783096ee6 100644 --- a/tests/integration/git_helper_for_declarative_test.go +++ b/tests/integration/git_helper_for_declarative_test.go @@ -154,16 +154,16 @@ func doGitAddRemote(dstPath, remoteName string, u *url.URL) func(*testing.T) { } } -func doGitPushTestRepository(dstPath string, args ...git.CmdArg) func(*testing.T) { +func doGitPushTestRepository(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { - _, _, err := git.NewCommand(git.DefaultContext, append([]git.CmdArg{"push", "-u"}, args...)...).RunStdString(&git.RunOpts{Dir: dstPath}) + _, _, err := git.NewCommand(git.DefaultContext, "push", "-u").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } } -func doGitPushTestRepositoryFail(dstPath string, args ...git.CmdArg) func(*testing.T) { +func doGitPushTestRepositoryFail(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { - _, _, err := git.NewCommand(git.DefaultContext, append([]git.CmdArg{"push"}, args...)...).RunStdString(&git.RunOpts{Dir: dstPath}) + _, _, err := git.NewCommand(git.DefaultContext, "push").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.Error(t, err) } } @@ -175,23 +175,23 @@ func doGitCreateBranch(dstPath, branch string) func(*testing.T) { } } -func doGitCheckoutBranch(dstPath string, args ...git.CmdArg) func(*testing.T) { +func doGitCheckoutBranch(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { - _, _, err := git.NewCommandNoGlobals(append(append(git.AllowLFSFiltersArgs(), "checkout"), args...)...).RunStdString(&git.RunOpts{Dir: dstPath}) + _, _, err := git.NewCommandContextNoGlobals(git.DefaultContext, git.AllowLFSFiltersArgs()...).AddArguments("checkout").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } } -func doGitMerge(dstPath string, args ...git.CmdArg) func(*testing.T) { +func doGitMerge(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { - _, _, err := git.NewCommand(git.DefaultContext, append([]git.CmdArg{"merge"}, args...)...).RunStdString(&git.RunOpts{Dir: dstPath}) + _, _, err := git.NewCommand(git.DefaultContext, "merge").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } } -func doGitPull(dstPath string, args ...git.CmdArg) func(*testing.T) { +func doGitPull(dstPath string, args ...string) func(*testing.T) { return func(t *testing.T) { - _, _, err := git.NewCommandNoGlobals(append(append(git.AllowLFSFiltersArgs(), "pull"), args...)...).RunStdString(&git.RunOpts{Dir: dstPath}) + _, _, err := git.NewCommandContextNoGlobals(git.DefaultContext, git.AllowLFSFiltersArgs()...).AddArguments("pull").AddArguments(git.ToTrustedCmdArgs(args)...).RunStdString(&git.RunOpts{Dir: dstPath}) assert.NoError(t, err) } } diff --git a/tests/integration/git_test.go b/tests/integration/git_test.go index a11bad21b7b08..420a8676b9ed6 100644 --- a/tests/integration/git_test.go +++ b/tests/integration/git_test.go @@ -509,7 +509,7 @@ func doCreatePRAndSetManuallyMerged(ctx, baseCtx APITestContext, dstPath, baseBr })) t.Run("CreateHeadBranch", doGitCreateBranch(dstPath, headBranch)) - t.Run("PushToHeadBranch", doGitPushTestRepository(dstPath, "origin", git.CmdArgCheck(headBranch))) + t.Run("PushToHeadBranch", doGitPushTestRepository(dstPath, "origin", headBranch)) t.Run("CreateEmptyPullRequest", func(t *testing.T) { pr, err = doAPICreatePullRequest(ctx, baseCtx.Username, baseCtx.Reponame, baseBranch, headBranch)(t) assert.NoError(t, err) diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index e80822a7520af..090a27c39efbc 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -287,7 +287,7 @@ func TestCantMergeUnrelated(t *testing.T) { assert.NoError(t, err) sha := strings.TrimSpace(stdout.String()) - _, _, err = git.NewCommand(git.DefaultContext, "update-index", "--add", "--replace", "--cacheinfo", "100644", git.CmdArgCheck(sha), "somewher-over-the-rainbow").RunStdString(&git.RunOpts{Dir: path}) + _, _, err = git.NewCommand(git.DefaultContext, "update-index", "--add", "--replace", "--cacheinfo").AddDynamicArguments("100644", sha, "somewher-over-the-rainbow").RunStdString(&git.RunOpts{Dir: path}) assert.NoError(t, err) treeSha, _, err := git.NewCommand(git.DefaultContext, "write-tree").RunStdString(&git.RunOpts{Dir: path}) From c2774d9e80d9a436d9c2044960369c4db227e3a0 Mon Sep 17 00:00:00 2001 From: silverwind Date: Sat, 4 Feb 2023 04:17:43 +0100 Subject: [PATCH 16/23] Fix color of tertiary button on dark theme (#22739) Before: Screenshot 2023-02-03 at 14 07 34 After: Screenshot 2023-02-03 at 14 07 52 This is the only instance of such a button in all templates. --------- Co-authored-by: Lauris BH Co-authored-by: Lunny Xiao --- web_src/less/_base.less | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/web_src/less/_base.less b/web_src/less/_base.less index 26fc83785b79f..e58bf53f5df9f 100644 --- a/web_src/less/_base.less +++ b/web_src/less/_base.less @@ -2200,9 +2200,7 @@ a.ui.label:hover { border: 1px solid var(--color-light-border); color: var(--color-text); } -.ui.tertiary.button { - border: none; -} + .page-content .ui.button { box-shadow: none !important; } @@ -2319,6 +2317,15 @@ a.ui.label:hover { color: var(--color-secondary-dark-8) !important; } +.ui.tertiary.button { + color: var(--color-text-light); + border: none; +} + +.ui.tertiary.button:hover { + color: var(--color-text); +} + .ui.primary.label, .ui.primary.labels .label { background-color: var(--color-primary) !important; From 2741546bed546fbe9792b48119743cfaebfec6bf Mon Sep 17 00:00:00 2001 From: techknowlogick Date: Sat, 4 Feb 2023 01:48:38 -0500 Subject: [PATCH 17/23] Repositories: by default disable all units except code and pulls on forks (#22541) Most of the time forks are used for contributing code only, so not having issues, projects, release and packages is a better default for such cases. They can still be enabled in the settings. A new option `DEFAULT_FORK_REPO_UNITS` is added to configure the default units on forks. Also add missing `repo.packages` unit to documentation. code by: @brechtvl ## :warning: BREAKING :warning: When forking a repository, the fork will now have issues, projects, releases, packages and wiki disabled. These can be enabled in the repository settings afterwards. To change back to the previous default behavior, configure `DEFAULT_FORK_REPO_UNITS` to be the same value as `DEFAULT_REPO_UNITS`. Co-authored-by: Brecht Van Lommel --- custom/conf/app.example.ini | 10 +++- .../doc/advanced/config-cheat-sheet.en-us.md | 3 +- models/unit/unit.go | 58 ++++++++++++------- modules/repository/create.go | 12 ++-- modules/repository/generate.go | 2 +- modules/setting/repository.go | 2 + services/repository/adopt.go | 2 +- services/repository/fork.go | 2 +- 8 files changed, 60 insertions(+), 31 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 77efe1417ba96..b0260f21d1388 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -927,14 +927,18 @@ ROUTER = console ;USE_COMPAT_SSH_URI = false ;; ;; Close issues as long as a commit on any branch marks it as fixed -;; Comma separated list of globally disabled repo units. Allowed values: repo.issues, repo.ext_issues, repo.pulls, repo.wiki, repo.ext_wiki, repo.projects +;; Comma separated list of globally disabled repo units. Allowed values: repo.issues, repo.ext_issues, repo.pulls, repo.wiki, repo.ext_wiki, repo.projects, repo.packages ;DISABLED_REPO_UNITS = ;; -;; Comma separated list of default repo units. Allowed values: repo.code, repo.releases, repo.issues, repo.pulls, repo.wiki, repo.projects. +;; Comma separated list of default new repo units. Allowed values: repo.code, repo.releases, repo.issues, repo.pulls, repo.wiki, repo.projects, repo.packages. ;; Note: Code and Releases can currently not be deactivated. If you specify default repo units you should still list them for future compatibility. ;; External wiki and issue tracker can't be enabled by default as it requires additional settings. ;; Disabled repo units will not be added to new repositories regardless if it is in the default list. -;DEFAULT_REPO_UNITS = repo.code,repo.releases,repo.issues,repo.pulls,repo.wiki,repo.projects +;DEFAULT_REPO_UNITS = repo.code,repo.releases,repo.issues,repo.pulls,repo.wiki,repo.projects,repo.packages +;; +;; Comma separated list of default forked repo units. +;; The set of allowed values and rules are the same as DEFAULT_REPO_UNITS. +;DEFAULT_FORK_REPO_UNITS = repo.code,repo.pulls ;; ;; Prefix archive files by placing them in a directory named after the repository ;PREFIX_ARCHIVE_FILES = true diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 441bb824ad208..e12d1feeb9ba8 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -104,7 +104,8 @@ In addition there is _`StaticRootPath`_ which can be set as a built-in at build - `ENABLE_PUSH_CREATE_USER`: **false**: Allow users to push local repositories to Gitea and have them automatically created for a user. - `ENABLE_PUSH_CREATE_ORG`: **false**: Allow users to push local repositories to Gitea and have them automatically created for an org. - `DISABLED_REPO_UNITS`: **_empty_**: Comma separated list of globally disabled repo units. Allowed values: \[repo.issues, repo.ext_issues, repo.pulls, repo.wiki, repo.ext_wiki, repo.projects\] -- `DEFAULT_REPO_UNITS`: **repo.code,repo.releases,repo.issues,repo.pulls,repo.wiki,repo.projects**: Comma separated list of default repo units. Allowed values: \[repo.code, repo.releases, repo.issues, repo.pulls, repo.wiki, repo.projects\]. Note: Code and Releases can currently not be deactivated. If you specify default repo units you should still list them for future compatibility. External wiki and issue tracker can't be enabled by default as it requires additional settings. Disabled repo units will not be added to new repositories regardless if it is in the default list. +- `DEFAULT_REPO_UNITS`: **repo.code,repo.releases,repo.issues,repo.pulls,repo.wiki,repo.projects,repo.packages**: Comma separated list of default new repo units. Allowed values: \[repo.code, repo.releases, repo.issues, repo.pulls, repo.wiki, repo.projects\]. Note: Code and Releases can currently not be deactivated. If you specify default repo units you should still list them for future compatibility. External wiki and issue tracker can't be enabled by default as it requires additional settings. Disabled repo units will not be added to new repositories regardless if it is in the default list. +- `DEFAULT_FORK_REPO_UNITS`: **repo.code,repo.pulls**: Comma separated list of default forked repo units. The set of allowed values and rules is the same as `DEFAULT_REPO_UNITS`. - `PREFIX_ARCHIVE_FILES`: **true**: Prefix archive files by placing them in a directory named after the repository. - `DISABLE_MIGRATIONS`: **false**: Disable migrating feature. - `DISABLE_STARS`: **false**: Disable stars feature. diff --git a/models/unit/unit.go b/models/unit/unit.go index bcd0572ab9ecf..a2a9079dc25c7 100644 --- a/models/unit/unit.go +++ b/models/unit/unit.go @@ -94,6 +94,12 @@ var ( TypePackages, } + // ForkRepoUnits contains the default unit types for forks + DefaultForkRepoUnits = []Type{ + TypeCode, + TypePullRequests, + } + // NotAllowedDefaultRepoUnits contains units that can't be default NotAllowedDefaultRepoUnits = []Type{ TypeExternalWiki, @@ -110,26 +116,41 @@ var ( DisabledRepoUnits = []Type{} ) -// LoadUnitConfig load units from settings -func LoadUnitConfig() { - setDefaultRepoUnits := FindUnitTypes(setting.Repository.DefaultRepoUnits...) - // Default repo units set if setting is not empty - if len(setDefaultRepoUnits) > 0 { +// Get valid set of default repository units from settings +func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type { + units := defaultUnits + + // Use setting if not empty + if len(settingDefaultUnits) > 0 { // MustRepoUnits required as default - DefaultRepoUnits = make([]Type, len(MustRepoUnits)) - copy(DefaultRepoUnits, MustRepoUnits) - for _, defaultU := range setDefaultRepoUnits { - if !defaultU.CanBeDefault() { - log.Warn("Not allowed as default unit: %s", defaultU.String()) + units = make([]Type, len(MustRepoUnits)) + copy(units, MustRepoUnits) + for _, settingUnit := range settingDefaultUnits { + if !settingUnit.CanBeDefault() { + log.Warn("Not allowed as default unit: %s", settingUnit.String()) continue } // MustRepoUnits already added - if defaultU.CanDisable() { - DefaultRepoUnits = append(DefaultRepoUnits, defaultU) + if settingUnit.CanDisable() { + units = append(units, settingUnit) } } } + // Remove disabled units + for _, disabledUnit := range DisabledRepoUnits { + for i, unit := range units { + if unit == disabledUnit { + units = append(units[:i], units[i+1:]...) + } + } + } + + return units +} + +// LoadUnitConfig load units from settings +func LoadUnitConfig() { DisabledRepoUnits = FindUnitTypes(setting.Repository.DisabledRepoUnits...) // Check that must units are not disabled for i, disabledU := range DisabledRepoUnits { @@ -138,14 +159,11 @@ func LoadUnitConfig() { DisabledRepoUnits = append(DisabledRepoUnits[:i], DisabledRepoUnits[i+1:]...) } } - // Remove disabled units from default units - for _, disabledU := range DisabledRepoUnits { - for i, defaultU := range DefaultRepoUnits { - if defaultU == disabledU { - DefaultRepoUnits = append(DefaultRepoUnits[:i], DefaultRepoUnits[i+1:]...) - } - } - } + + setDefaultRepoUnits := FindUnitTypes(setting.Repository.DefaultRepoUnits...) + DefaultRepoUnits = validateDefaultRepoUnits(DefaultRepoUnits, setDefaultRepoUnits) + setDefaultForkRepoUnits := FindUnitTypes(setting.Repository.DefaultForkRepoUnits...) + DefaultForkRepoUnits = validateDefaultRepoUnits(DefaultForkRepoUnits, setDefaultForkRepoUnits) } // UnitGlobalDisabled checks if unit type is global disabled diff --git a/modules/repository/create.go b/modules/repository/create.go index 454a192ddbb52..7bcda0fe45fe1 100644 --- a/modules/repository/create.go +++ b/modules/repository/create.go @@ -30,7 +30,7 @@ import ( ) // CreateRepositoryByExample creates a repository for the user/organization. -func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository, overwriteOrAdopt bool) (err error) { +func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, repo *repo_model.Repository, overwriteOrAdopt, isFork bool) (err error) { if err = repo_model.IsUsableRepoName(repo.Name); err != nil { return err } @@ -67,8 +67,12 @@ func CreateRepositoryByExample(ctx context.Context, doer, u *user_model.User, re } // insert units for repo - units := make([]repo_model.RepoUnit, 0, len(unit.DefaultRepoUnits)) - for _, tp := range unit.DefaultRepoUnits { + defaultUnits := unit.DefaultRepoUnits + if isFork { + defaultUnits = unit.DefaultForkRepoUnits + } + units := make([]repo_model.RepoUnit, 0, len(defaultUnits)) + for _, tp := range defaultUnits { if tp == unit.TypeIssues { units = append(units, repo_model.RepoUnit{ RepoID: repo.ID, @@ -212,7 +216,7 @@ func CreateRepository(doer, u *user_model.User, opts CreateRepoOptions) (*repo_m var rollbackRepo *repo_model.Repository if err := db.WithTx(db.DefaultContext, func(ctx context.Context) error { - if err := CreateRepositoryByExample(ctx, doer, u, repo, false); err != nil { + if err := CreateRepositoryByExample(ctx, doer, u, repo, false, false); err != nil { return err } diff --git a/modules/repository/generate.go b/modules/repository/generate.go index b6a1d7b43ef09..31d5ebbb11f40 100644 --- a/modules/repository/generate.go +++ b/modules/repository/generate.go @@ -319,7 +319,7 @@ func GenerateRepository(ctx context.Context, doer, owner *user_model.User, templ TrustModel: templateRepo.TrustModel, } - if err = CreateRepositoryByExample(ctx, doer, owner, generateRepo, false); err != nil { + if err = CreateRepositoryByExample(ctx, doer, owner, generateRepo, false, false); err != nil { return nil, err } diff --git a/modules/setting/repository.go b/modules/setting/repository.go index d78b63a1f3f78..f53de17a4dd98 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -41,6 +41,7 @@ var ( EnablePushCreateOrg bool DisabledRepoUnits []string DefaultRepoUnits []string + DefaultForkRepoUnits []string PrefixArchiveFiles bool DisableMigrations bool DisableStars bool `ini:"DISABLE_STARS"` @@ -157,6 +158,7 @@ var ( EnablePushCreateOrg: false, DisabledRepoUnits: []string{}, DefaultRepoUnits: []string{}, + DefaultForkRepoUnits: []string{}, PrefixArchiveFiles: true, DisableMigrations: false, DisableStars: false, diff --git a/services/repository/adopt.go b/services/repository/adopt.go index 8ebf2b6a3e60f..280c4cc035a8f 100644 --- a/services/repository/adopt.go +++ b/services/repository/adopt.go @@ -67,7 +67,7 @@ func AdoptRepository(doer, u *user_model.User, opts repo_module.CreateRepoOption } } - if err := repo_module.CreateRepositoryByExample(ctx, doer, u, repo, true); err != nil { + if err := repo_module.CreateRepositoryByExample(ctx, doer, u, repo, true, false); err != nil { return err } if err := adoptRepository(ctx, repoPath, doer, repo, opts); err != nil { diff --git a/services/repository/fork.go b/services/repository/fork.go index ad534be887f1b..c3ca89e02e7f8 100644 --- a/services/repository/fork.go +++ b/services/repository/fork.go @@ -119,7 +119,7 @@ func ForkRepository(ctx context.Context, doer, owner *user_model.User, opts Fork }() err = db.WithTx(ctx, func(txCtx context.Context) error { - if err = repo_module.CreateRepositoryByExample(txCtx, doer, owner, repo, false); err != nil { + if err = repo_module.CreateRepositoryByExample(txCtx, doer, owner, repo, false, true); err != nil { return err } From ea13b23349ef98249deeb9469f6b1444de42abf5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 4 Feb 2023 18:30:55 +0800 Subject: [PATCH 18/23] Escape path for the file list (#22741) Fix #22740 --- web_src/js/features/repo-findfile.js | 6 +++++- web_src/js/features/repo-findfile.test.js | 7 ++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/web_src/js/features/repo-findfile.js b/web_src/js/features/repo-findfile.js index 7b8833e793c69..f279dfeeb73aa 100644 --- a/web_src/js/features/repo-findfile.js +++ b/web_src/js/features/repo-findfile.js @@ -72,6 +72,10 @@ export function filterRepoFilesWeighted(files, filter) { return filterResult; } +export function escapePath(s) { + return s.split('/').map(encodeURIComponent).join('/'); +} + function filterRepoFiles(filter) { const treeLink = $repoFindFileInput.attr('data-url-tree-link'); $repoFindFileTableBody.empty(); @@ -83,7 +87,7 @@ function filterRepoFiles(filter) { for (const r of filterResult) { const $row = $(tmplRow); const $a = $row.find('a'); - $a.attr('href', `${treeLink}/${r.matchResult.join('')}`); + $a.attr('href', `${treeLink}/${escapePath(r.matchResult.join(''))}`); const $octiconFile = $(svg('octicon-file')).addClass('mr-3'); $a.append($octiconFile); // if the target file path is "abc/xyz", to search "bx", then the matchResult is ['a', 'b', 'c/', 'x', 'yz'] diff --git a/web_src/js/features/repo-findfile.test.js b/web_src/js/features/repo-findfile.test.js index a90b0bf0a2cab..50321853960be 100644 --- a/web_src/js/features/repo-findfile.test.js +++ b/web_src/js/features/repo-findfile.test.js @@ -1,5 +1,5 @@ import {describe, expect, test} from 'vitest'; -import {strSubMatch, calcMatchedWeight, filterRepoFilesWeighted} from './repo-findfile.js'; +import {strSubMatch, calcMatchedWeight, filterRepoFilesWeighted, escapePath} from './repo-findfile.js'; describe('Repo Find Files', () => { test('strSubMatch', () => { @@ -32,4 +32,9 @@ describe('Repo Find Files', () => { expect(res).toHaveLength(2); expect(res[0].matchResult).toEqual(['', 'word', '.txt']); }); + + test('escapePath', () => { + expect(escapePath('a/b/c')).toEqual('a/b/c'); + expect(escapePath('a/b/ c')).toEqual('a/b/%20c'); + }); }); From 4d20a4a1baeb9ed1bb5b3ed7c44b6046f6387303 Mon Sep 17 00:00:00 2001 From: delvh Date: Sat, 4 Feb 2023 14:26:38 +0100 Subject: [PATCH 19/23] Remove ONLY_SHOW_RELEVANT_REPOS setting (#21962) Every user can already disable the filter manually, so the explicit setting is absolutely useless and only complicates the logic. Previously, there was also unexpected behavior when multiple query parameters were present. --------- Co-authored-by: zeripath Co-authored-by: Lunny Xiao --- custom/conf/app.example.ini | 4 ---- docs/content/doc/advanced/config-cheat-sheet.en-us.md | 2 -- models/repo/repo_list.go | 8 ++++---- modules/setting/setting.go | 2 -- routers/web/explore/repo.go | 11 +++++------ 5 files changed, 9 insertions(+), 18 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index b0260f21d1388..5a1edf9fbb791 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1222,10 +1222,6 @@ ROUTER = console ;; ;; Whether to enable a Service Worker to cache frontend assets ;USE_SERVICE_WORKER = false -;; -;; Whether to only show relevant repos on the explore page when no keyword is specified and default sorting is used. -;; A repo is considered irrelevant if it's a fork or if it has no metadata (no description, no icon, no topic). -;ONLY_SHOW_RELEVANT_REPOS = false ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index e12d1feeb9ba8..67ca7a5166208 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -231,8 +231,6 @@ The following configuration set `Content-Type: application/vnd.android.package-a - `DEFAULT_SHOW_FULL_NAME`: **false**: Whether the full name of the users should be shown where possible. If the full name isn't set, the username will be used. - `SEARCH_REPO_DESCRIPTION`: **true**: Whether to search within description at repository search on explore page. - `USE_SERVICE_WORKER`: **false**: Whether to enable a Service Worker to cache frontend assets. -- `ONLY_SHOW_RELEVANT_REPOS`: **false** Whether to only show relevant repos on the explore page when no keyword is specified and default sorting is used. - A repo is considered irrelevant if it's a fork or if it has no metadata (no description, no icon, no topic). ### UI - Admin (`ui.admin`) diff --git a/models/repo/repo_list.go b/models/repo/repo_list.go index c6e9a204d1330..d64368daa6b28 100644 --- a/models/repo/repo_list.go +++ b/models/repo/repo_list.go @@ -494,7 +494,7 @@ func SearchRepositoryCondition(opts *SearchRepoOptions) builder.Cond { } if opts.OnlyShowRelevant { - // Only show a repo that either has a topic or description. + // Only show a repo that has at least a topic, an icon, or a description subQueryCond := builder.NewCond() // Topic checking. Topics are present. @@ -504,13 +504,13 @@ func SearchRepositoryCondition(opts *SearchRepoOptions) builder.Cond { subQueryCond = subQueryCond.Or(builder.And(builder.Neq{"topics": "null"}, builder.Neq{"topics": "[]"})) } - // Description checking. Description not empty. + // Description checking. Description not empty subQueryCond = subQueryCond.Or(builder.Neq{"description": ""}) - // Repo has a avatar. + // Repo has a avatar subQueryCond = subQueryCond.Or(builder.Neq{"avatar": ""}) - // Always hide repo's that are empty. + // Always hide repo's that are empty subQueryCond = subQueryCond.And(builder.Eq{"is_empty": false}) cond = cond.And(subQueryCond) diff --git a/modules/setting/setting.go b/modules/setting/setting.go index afd7a40150d25..23cd90553eb33 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -241,7 +241,6 @@ var ( CustomEmojisMap map[string]string `ini:"-"` SearchRepoDescription bool UseServiceWorker bool - OnlyShowRelevantRepos bool Notification struct { MinTimeout time.Duration @@ -1123,7 +1122,6 @@ func loadFromConf(allowEmpty bool, extraConfig string) { UI.DefaultShowFullName = Cfg.Section("ui").Key("DEFAULT_SHOW_FULL_NAME").MustBool(false) UI.SearchRepoDescription = Cfg.Section("ui").Key("SEARCH_REPO_DESCRIPTION").MustBool(true) UI.UseServiceWorker = Cfg.Section("ui").Key("USE_SERVICE_WORKER").MustBool(false) - UI.OnlyShowRelevantRepos = Cfg.Section("ui").Key("ONLY_SHOW_RELEVANT_REPOS").MustBool(false) HasRobotsTxt, err = util.IsFile(path.Join(CustomPath, "robots.txt")) if err != nil { diff --git a/routers/web/explore/repo.go b/routers/web/explore/repo.go index 5271e39bbc91b..e9684dd2865d4 100644 --- a/routers/web/explore/repo.go +++ b/routers/web/explore/repo.go @@ -17,7 +17,8 @@ import ( const ( // tplExploreRepos explore repositories page template - tplExploreRepos base.TplName = "explore/repos" + tplExploreRepos base.TplName = "explore/repos" + relevantReposOnlyParam string = "no_filter" ) // RepoSearchOptions when calling search repositories @@ -81,13 +82,11 @@ func RenderRepoSearch(ctx *context.Context, opts *RepoSearchOptions) { default: ctx.Data["SortType"] = "recentupdate" orderBy = db.SearchOrderByRecentUpdated - onlyShowRelevant = setting.UI.OnlyShowRelevantRepos && !ctx.FormBool("no_filter") } + onlyShowRelevant = !ctx.FormBool(relevantReposOnlyParam) + keyword := ctx.FormTrim("q") - if keyword != "" { - onlyShowRelevant = false - } ctx.Data["OnlyShowRelevant"] = onlyShowRelevant @@ -139,7 +138,7 @@ func RenderRepoSearch(ctx *context.Context, opts *RepoSearchOptions) { pager.SetDefaultParams(ctx) pager.AddParam(ctx, "topic", "TopicOnly") pager.AddParam(ctx, "language", "Language") - pager.AddParamString("no_filter", ctx.FormString("no_filter")) + pager.AddParamString(relevantReposOnlyParam, ctx.FormString(relevantReposOnlyParam)) ctx.Data["Page"] = pager ctx.HTML(http.StatusOK, opts.TplName) From 8574a6433fab47b6f20997f024c176490dfad1c0 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 4 Feb 2023 22:35:08 +0800 Subject: [PATCH 20/23] Show all projects, not just repo projects and open/closed projects (#22640) This PR fixes two problems. One is when filter repository issues, only repository level projects are listed. Another is if you list open issues, only open projects will be displayed in filter options and if you list closed issues, only closed projects will be displayed in filter options. In this PR, both repository level and org/user level projects will be displayed in filter, and both open and closed projects will be listed as filter items. --------- Co-authored-by: John Olheiser Co-authored-by: zeripath Co-authored-by: delvh --- models/db/search.go | 6 ++++ models/issues/issue.go | 2 ++ options/locale/locale_en-US.ini | 3 +- routers/web/repo/issue.go | 10 ++---- templates/repo/issue/list.tmpl | 64 +++++++++++++++++++++++++++------ 5 files changed, 65 insertions(+), 20 deletions(-) diff --git a/models/db/search.go b/models/db/search.go index f5273cb6f6bfa..26e082756a51b 100644 --- a/models/db/search.go +++ b/models/db/search.go @@ -27,3 +27,9 @@ const ( SearchOrderByForks SearchOrderBy = "num_forks ASC" SearchOrderByForksReverse SearchOrderBy = "num_forks DESC" ) + +const ( + // Which means a condition to filter the records which don't match any id. + // It's different from zero which means the condition could be ignored. + NoneID = -1 +) diff --git a/models/issues/issue.go b/models/issues/issue.go index 78cac900523f9..3ddc799270962 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -1251,6 +1251,8 @@ func (opts *IssuesOptions) setupSessionNoLimit(sess *xorm.Session) { if opts.ProjectID > 0 { sess.Join("INNER", "project_issue", "issue.id = project_issue.issue_id"). And("project_issue.project_id=?", opts.ProjectID) + } else if opts.ProjectID == db.NoneID { // show those that are in no project + sess.And(builder.NotIn("issue.id", builder.Select("issue_id").From("project_issue"))) } if opts.ProjectBoardID != 0 { diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 26217293a5efa..f384056613c9c 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1306,7 +1306,8 @@ issues.filter_label_no_select = All labels issues.filter_milestone = Milestone issues.filter_milestone_no_select = All milestones issues.filter_project = Project -issues.filter_project_no_select = All projects +issues.filter_project_all = All projects +issues.filter_project_none = No project issues.filter_assignee = Assignee issues.filter_assginee_no_select = All assignees issues.filter_poster = Author diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 5bff9e67f340a..2193da5110efc 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -363,16 +363,10 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption uti return 0 } - projects, _, err := project_model.FindProjects(ctx, project_model.SearchOptions{ - RepoID: repo.ID, - Type: project_model.TypeRepository, - IsClosed: util.OptionalBoolOf(isShowClosed), - }) - if err != nil { - ctx.ServerError("FindProjects", err) + retrieveProjects(ctx, repo) + if ctx.Written() { return } - ctx.Data["Projects"] = projects ctx.Data["IssueStats"] = issueStats ctx.Data["SelLabelIDs"] = labelIDs diff --git a/templates/repo/issue/list.tmpl b/templates/repo/issue/list.tmpl index 23bcc60f94f79..cf2a2a6bbabaa 100644 --- a/templates/repo/issue/list.tmpl +++ b/templates/repo/issue/list.tmpl @@ -75,19 +75,41 @@
- -

diff --git a/templates/package/content/cargo.tmpl b/templates/package/content/cargo.tmpl new file mode 100644 index 0000000000000..54c40a5b0dc0a --- /dev/null +++ b/templates/package/content/cargo.tmpl @@ -0,0 +1,62 @@ +{{if eq .PackageDescriptor.Package.Type "cargo"}} +

{{.locale.Tr "packages.installation"}}

+
+
+
+ +
[registry]
+default = "gitea"
+
+[registries.gitea]
+index = "{{AppUrl}}{{.PackageDescriptor.Owner.Name}}/_cargo-index.git"
+
+[net]
+git-fetch-with-cli = true
+
+
+ +
cargo add {{.PackageDescriptor.Package.Name}}@{{.PackageDescriptor.Version.Version}}
+
+
+ +
+
+
+ + {{if or .PackageDescriptor.Metadata.Description .PackageDescriptor.Metadata.Readme}} +

{{.locale.Tr "packages.about"}}

+ {{if .PackageDescriptor.Metadata.Description}}
{{.PackageDescriptor.Metadata.Description}}
{{end}} + {{if .PackageDescriptor.Metadata.Readme}}
{{RenderMarkdownToHtml .PackageDescriptor.Metadata.Readme}}
{{end}} + {{end}} + + {{if .PackageDescriptor.Metadata.Dependencies}} +

{{.locale.Tr "packages.dependencies"}}

+
+ + + + + + + + + {{range .PackageDescriptor.Metadata.Dependencies}} + + + + + {{end}} + +
{{.locale.Tr "packages.dependency.id"}}{{.locale.Tr "packages.dependency.version"}}
{{.Name}}{{.Req}}
+
+ {{end}} + + {{if .PackageDescriptor.Metadata.Keywords}} +

{{.locale.Tr "packages.keywords"}}

+
+ {{range .PackageDescriptor.Metadata.Keywords}} + {{.}} + {{end}} +
+ {{end}} +{{end}} diff --git a/templates/package/metadata/cargo.tmpl b/templates/package/metadata/cargo.tmpl new file mode 100644 index 0000000000000..68232d48321c5 --- /dev/null +++ b/templates/package/metadata/cargo.tmpl @@ -0,0 +1,7 @@ +{{if eq .PackageDescriptor.Package.Type "cargo"}} + {{range .PackageDescriptor.Metadata.Authors}}
{{svg "octicon-person" 16 "mr-3"}} {{.}}
{{end}} + {{if .PackageDescriptor.Metadata.ProjectURL}}
{{svg "octicon-link-external" 16 "mr-3"}} {{.locale.Tr "packages.details.project_site"}}
{{end}} + {{if .PackageDescriptor.Metadata.RepositoryURL}}
{{svg "octicon-link-external" 16 "mr-3"}} {{.locale.Tr "packages.cargo.details.repository_site"}}
{{end}} + {{if .PackageDescriptor.Metadata.DocumentationURL}}
{{svg "octicon-link-external" 16 "mr-3"}} {{.locale.Tr "packages.cargo.details.documentation_site"}}
{{end}} + {{if .PackageDescriptor.Metadata.License}}
{{svg "octicon-law" 16 "mr-3"}} {{.PackageDescriptor.Metadata.License}}
{{end}} +{{end}} diff --git a/templates/package/shared/cargo.tmpl b/templates/package/shared/cargo.tmpl new file mode 100644 index 0000000000000..b9c2ef4d2ba43 --- /dev/null +++ b/templates/package/shared/cargo.tmpl @@ -0,0 +1,24 @@ +

+ {{.locale.Tr "packages.owner.settings.cargo.title"}} +

+
+
+
+ +
+
+ {{.CsrfTokenHtml}} + +
+
+ +
+
+ {{.CsrfTokenHtml}} + +
+
+ +
+
+
diff --git a/templates/package/view.tmpl b/templates/package/view.tmpl index 4611cdb8d840f..6209e6cb02e27 100644 --- a/templates/package/view.tmpl +++ b/templates/package/view.tmpl @@ -19,6 +19,7 @@
+ {{template "package/content/cargo" .}} {{template "package/content/composer" .}} {{template "package/content/conan" .}} {{template "package/content/conda" .}} @@ -43,6 +44,7 @@ {{end}}
{{svg "octicon-calendar" 16 "mr-3"}} {{TimeSinceUnix .PackageDescriptor.Version.CreatedUnix $.locale}}
{{svg "octicon-download" 16 "mr-3"}} {{.PackageDescriptor.Version.DownloadCount}}
+ {{template "package/metadata/cargo" .}} {{template "package/metadata/composer" .}} {{template "package/metadata/conan" .}} {{template "package/metadata/conda" .}} diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 5a3f2c2bb9e98..d6e3009b35b0d 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -2100,6 +2100,7 @@ }, { "enum": [ + "cargo", "composer", "conan", "conda", diff --git a/templates/user/settings/packages.tmpl b/templates/user/settings/packages.tmpl index 9aae14f934326..472d84327a729 100644 --- a/templates/user/settings/packages.tmpl +++ b/templates/user/settings/packages.tmpl @@ -4,6 +4,7 @@
{{template "base/alert" .}} {{template "package/shared/cleanup_rules/list" .}} + {{template "package/shared/cargo" .}}
{{template "base/footer" .}} diff --git a/tests/integration/api_packages_cargo_test.go b/tests/integration/api_packages_cargo_test.go new file mode 100644 index 0000000000000..0c542eaf1e857 --- /dev/null +++ b/tests/integration/api_packages_cargo_test.go @@ -0,0 +1,341 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package integration + +import ( + "bytes" + "encoding/binary" + "fmt" + "io" + "net/http" + neturl "net/url" + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/packages" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/json" + cargo_module "code.gitea.io/gitea/modules/packages/cargo" + "code.gitea.io/gitea/modules/setting" + cargo_router "code.gitea.io/gitea/routers/api/packages/cargo" + cargo_service "code.gitea.io/gitea/services/packages/cargo" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" +) + +func TestPackageCargo(t *testing.T) { + onGiteaRun(t, testPackageCargo) +} + +func testPackageCargo(t *testing.T, _ *neturl.URL) { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + + packageName := "cargo-package" + packageVersion := "1.0.3" + packageDescription := "Package Description" + packageAuthor := "KN4CK3R" + packageHomepage := "https://gitea.io/" + packageLicense := "MIT" + + createPackage := func(name, version string) io.Reader { + metadata := `{ + "name":"` + name + `", + "vers":"` + version + `", + "description":"` + packageDescription + `", + "authors": ["` + packageAuthor + `"], + "deps":[ + { + "name":"dep", + "version_req":"1.0", + "registry": "https://gitea.io/user/_cargo-index", + "kind": "normal", + "default_features": true + } + ], + "homepage":"` + packageHomepage + `", + "license":"` + packageLicense + `" +}` + + var buf bytes.Buffer + binary.Write(&buf, binary.LittleEndian, uint32(len(metadata))) + buf.WriteString(metadata) + binary.Write(&buf, binary.LittleEndian, uint32(4)) + buf.WriteString("test") + return &buf + } + + err := cargo_service.InitializeIndexRepository(db.DefaultContext, user, user) + assert.NoError(t, err) + + repo, err := repo_model.GetRepositoryByOwnerAndName(db.DefaultContext, user.Name, cargo_service.IndexRepositoryName) + assert.NotNil(t, repo) + assert.NoError(t, err) + + readGitContent := func(t *testing.T, path string) string { + gitRepo, err := git.OpenRepository(db.DefaultContext, repo.RepoPath()) + assert.NoError(t, err) + defer gitRepo.Close() + + commit, err := gitRepo.GetBranchCommit(repo.DefaultBranch) + assert.NoError(t, err) + + blob, err := commit.GetBlobByPath(path) + assert.NoError(t, err) + + content, err := blob.GetBlobContent() + assert.NoError(t, err) + + return content + } + + root := fmt.Sprintf("%sapi/packages/%s/cargo", setting.AppURL, user.Name) + url := fmt.Sprintf("%s/api/v1/crates", root) + + t.Run("Index", func(t *testing.T) { + t.Run("Config", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + content := readGitContent(t, cargo_service.ConfigFileName) + + var config cargo_service.Config + err := json.Unmarshal([]byte(content), &config) + assert.NoError(t, err) + + assert.Equal(t, url, config.DownloadURL) + assert.Equal(t, root, config.APIURL) + }) + }) + + t.Run("Upload", func(t *testing.T) { + t.Run("InvalidNameOrVersion", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + content := createPackage("0test", "1.0.0") + + req := NewRequestWithBody(t, "PUT", url+"/new", content) + req = AddBasicAuthHeader(req, user.Name) + resp := MakeRequest(t, req, http.StatusBadRequest) + + var status cargo_router.StatusResponse + DecodeJSON(t, resp, &status) + assert.False(t, status.OK) + + content = createPackage("test", "-1.0.0") + + req = NewRequestWithBody(t, "PUT", url+"/new", content) + req = AddBasicAuthHeader(req, user.Name) + resp = MakeRequest(t, req, http.StatusBadRequest) + + DecodeJSON(t, resp, &status) + assert.False(t, status.OK) + }) + + t.Run("InvalidContent", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + metadata := `{"name":"test","vers":"1.0.0"}` + + var buf bytes.Buffer + binary.Write(&buf, binary.LittleEndian, uint32(len(metadata))) + buf.WriteString(metadata) + binary.Write(&buf, binary.LittleEndian, uint32(4)) + buf.WriteString("te") + + req := NewRequestWithBody(t, "PUT", url+"/new", &buf) + req = AddBasicAuthHeader(req, user.Name) + MakeRequest(t, req, http.StatusBadRequest) + }) + + t.Run("Valid", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequestWithBody(t, "PUT", url+"/new", createPackage(packageName, packageVersion)) + MakeRequest(t, req, http.StatusUnauthorized) + + req = NewRequestWithBody(t, "PUT", url+"/new", createPackage(packageName, packageVersion)) + req = AddBasicAuthHeader(req, user.Name) + resp := MakeRequest(t, req, http.StatusOK) + + var status cargo_router.StatusResponse + DecodeJSON(t, resp, &status) + assert.True(t, status.OK) + + pvs, err := packages.GetVersionsByPackageType(db.DefaultContext, user.ID, packages.TypeCargo) + assert.NoError(t, err) + assert.Len(t, pvs, 1) + + pd, err := packages.GetPackageDescriptor(db.DefaultContext, pvs[0]) + assert.NoError(t, err) + assert.NotNil(t, pd.SemVer) + assert.IsType(t, &cargo_module.Metadata{}, pd.Metadata) + assert.Equal(t, packageName, pd.Package.Name) + assert.Equal(t, packageVersion, pd.Version.Version) + + pfs, err := packages.GetFilesByVersionID(db.DefaultContext, pvs[0].ID) + assert.NoError(t, err) + assert.Len(t, pfs, 1) + assert.Equal(t, fmt.Sprintf("%s-%s.crate", packageName, packageVersion), pfs[0].Name) + assert.True(t, pfs[0].IsLead) + + pb, err := packages.GetBlobByID(db.DefaultContext, pfs[0].BlobID) + assert.NoError(t, err) + assert.EqualValues(t, 4, pb.Size) + + req = NewRequestWithBody(t, "PUT", url+"/new", createPackage(packageName, packageVersion)) + req = AddBasicAuthHeader(req, user.Name) + MakeRequest(t, req, http.StatusConflict) + + t.Run("Index", func(t *testing.T) { + t.Run("Entry", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + content := readGitContent(t, cargo_service.BuildPackagePath(packageName)) + + var entry cargo_service.IndexVersionEntry + err := json.Unmarshal([]byte(content), &entry) + assert.NoError(t, err) + + assert.Equal(t, packageName, entry.Name) + assert.Equal(t, packageVersion, entry.Version) + assert.Equal(t, pb.HashSHA256, entry.FileChecksum) + assert.False(t, entry.Yanked) + assert.Len(t, entry.Dependencies, 1) + dep := entry.Dependencies[0] + assert.Equal(t, "dep", dep.Name) + assert.Equal(t, "1.0", dep.Req) + assert.Equal(t, "normal", dep.Kind) + assert.True(t, dep.DefaultFeatures) + assert.Empty(t, dep.Features) + assert.False(t, dep.Optional) + assert.Nil(t, dep.Target) + assert.NotNil(t, dep.Registry) + assert.Equal(t, "https://gitea.io/user/_cargo-index", *dep.Registry) + assert.Nil(t, dep.Package) + }) + + t.Run("Rebuild", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + err := cargo_service.RebuildIndex(db.DefaultContext, user, user) + assert.NoError(t, err) + + _ = readGitContent(t, cargo_service.BuildPackagePath(packageName)) + }) + }) + }) + }) + + t.Run("Download", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + pv, err := packages.GetVersionByNameAndVersion(db.DefaultContext, user.ID, packages.TypeCargo, packageName, packageVersion) + assert.NoError(t, err) + assert.EqualValues(t, 0, pv.DownloadCount) + + pfs, err := packages.GetFilesByVersionID(db.DefaultContext, pv.ID) + assert.NoError(t, err) + assert.Len(t, pfs, 1) + + req := NewRequest(t, "GET", fmt.Sprintf("%s/%s/%s/download", url, neturl.PathEscape(packageName), neturl.PathEscape(pv.Version))) + req = AddBasicAuthHeader(req, user.Name) + resp := MakeRequest(t, req, http.StatusOK) + + assert.Equal(t, "test", resp.Body.String()) + + pv, err = packages.GetVersionByNameAndVersion(db.DefaultContext, user.ID, packages.TypeCargo, packageName, packageVersion) + assert.NoError(t, err) + assert.EqualValues(t, 1, pv.DownloadCount) + }) + + t.Run("Search", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + cases := []struct { + Query string + Page int + PerPage int + ExpectedTotal int64 + ExpectedResults int + }{ + {"", 0, 0, 1, 1}, + {"", 1, 10, 1, 1}, + {"cargo", 1, 0, 1, 1}, + {"cargo", 1, 10, 1, 1}, + {"cargo", 2, 10, 1, 0}, + {"test", 0, 10, 0, 0}, + } + + for i, c := range cases { + req := NewRequest(t, "GET", fmt.Sprintf("%s?q=%s&page=%d&per_page=%d", url, c.Query, c.Page, c.PerPage)) + req = AddBasicAuthHeader(req, user.Name) + resp := MakeRequest(t, req, http.StatusOK) + + var result cargo_router.SearchResult + DecodeJSON(t, resp, &result) + + assert.Equal(t, c.ExpectedTotal, result.Meta.Total, "case %d: unexpected total hits", i) + assert.Len(t, result.Crates, c.ExpectedResults, "case %d: unexpected result count", i) + } + }) + + t.Run("Yank", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "DELETE", fmt.Sprintf("%s/%s/%s/yank", url, neturl.PathEscape(packageName), neturl.PathEscape(packageVersion))) + req = AddBasicAuthHeader(req, user.Name) + resp := MakeRequest(t, req, http.StatusOK) + + var status cargo_router.StatusResponse + DecodeJSON(t, resp, &status) + assert.True(t, status.OK) + + content := readGitContent(t, cargo_service.BuildPackagePath(packageName)) + + var entry cargo_service.IndexVersionEntry + err := json.Unmarshal([]byte(content), &entry) + assert.NoError(t, err) + + assert.True(t, entry.Yanked) + }) + + t.Run("Unyank", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "PUT", fmt.Sprintf("%s/%s/%s/unyank", url, neturl.PathEscape(packageName), neturl.PathEscape(packageVersion))) + req = AddBasicAuthHeader(req, user.Name) + resp := MakeRequest(t, req, http.StatusOK) + + var status cargo_router.StatusResponse + DecodeJSON(t, resp, &status) + assert.True(t, status.OK) + + content := readGitContent(t, cargo_service.BuildPackagePath(packageName)) + + var entry cargo_service.IndexVersionEntry + err := json.Unmarshal([]byte(content), &entry) + assert.NoError(t, err) + + assert.False(t, entry.Yanked) + }) + + t.Run("ListOwners", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + req := NewRequest(t, "GET", fmt.Sprintf("%s/%s/owners", url, neturl.PathEscape(packageName))) + resp := MakeRequest(t, req, http.StatusOK) + + var owners cargo_router.Owners + DecodeJSON(t, resp, &owners) + + assert.Len(t, owners.Users, 1) + assert.Equal(t, user.ID, owners.Users[0].ID) + assert.Equal(t, user.Name, owners.Users[0].Login) + assert.Equal(t, user.DisplayName(), owners.Users[0].Name) + }) +} diff --git a/tests/integration/api_packages_test.go b/tests/integration/api_packages_test.go index a6c335eb7e0ed..39852e212c387 100644 --- a/tests/integration/api_packages_test.go +++ b/tests/integration/api_packages_test.go @@ -21,6 +21,7 @@ import ( "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" packages_service "code.gitea.io/gitea/services/packages" + packages_cleanup_service "code.gitea.io/gitea/services/packages/cleanup" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" @@ -246,7 +247,7 @@ func TestPackageCleanup(t *testing.T) { _, err = packages_model.GetInternalVersionByNameAndVersion(db.DefaultContext, 2, packages_model.TypeContainer, "test", container_model.UploadVersion) assert.NoError(t, err) - err = packages_service.Cleanup(db.DefaultContext, duration) + err = packages_cleanup_service.Cleanup(db.DefaultContext, duration) assert.NoError(t, err) pbs, err = packages_model.FindExpiredUnreferencedBlobs(db.DefaultContext, duration) @@ -383,7 +384,7 @@ func TestPackageCleanup(t *testing.T) { pcr, err := packages_model.InsertCleanupRule(db.DefaultContext, c.Rule) assert.NoError(t, err) - err = packages_service.Cleanup(db.DefaultContext, duration) + err = packages_cleanup_service.Cleanup(db.DefaultContext, duration) assert.NoError(t, err) for _, v := range c.Versions { diff --git a/web_src/svg/gitea-cargo.svg b/web_src/svg/gitea-cargo.svg new file mode 100644 index 0000000000000..dbec107ad0dcf --- /dev/null +++ b/web_src/svg/gitea-cargo.svg @@ -0,0 +1,3 @@ + + + \ No newline at end of file