The merge and update branch code was previously a little tangled and had
some very long functions. The functions were not very clear in their
reasoning and there were deficiencies in their logging and at least one
bug in the handling of LFS for update by rebase.
This PR substantially refactors this code and splits things out to into
separate functions. It also attempts to tidy up the calls by wrapping
things in "context"s. There are also attempts to improve logging when
there are errors.
Signed-off-by: Andrew Thornton <art27@cantab.net>
---------
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: delvh <dev.lh@web.de>
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 <xiaolunwen@gmail.com>
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 <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
This PR introduce glob match for protected branch name. The separator is
`/` and you can use `*` matching non-separator chars and use `**` across
separator.
It also supports input an exist or non-exist branch name as matching
condition and branch name condition has high priority than glob rule.
Should fix#2529 and #15705
screenshots
<img width="1160" alt="image"
src="https://user-images.githubusercontent.com/81045/205651179-ebb5492a-4ade-4bb4-a13c-965e8c927063.png">
Co-authored-by: zeripath <art27@cantab.net>
For a long time Gitea has tested PR patches using a git apply --check
method, and in fact prior to the introduction of a read-tree assisted
three-way merge in #18004, this was the only way of checking patches.
Since #18004, the git apply --check method has been a fallback method,
only used when the read-tree three-way merge method has detected a
conflict. The read-tree assisted three-way merge method is much faster
and less resource intensive method of detecting conflicts. #18004 kept
the git apply method around because it was thought possible that this
fallback might be able to rectify conflicts that the read-tree three-way
merge detected. I am not certain if this could ever be the case.
Given the uncertainty here and the now relative stability of the
read-tree method - this PR makes using this fallback optional and
disables it by default. The hope is that users will not notice any
significant difference in conflict detection and we will be able to
remove the git apply fallback in future, and/or improve the read-tree
three-way merge method to catch any conflicts that git apply method
might have been able to fix.
An additional benefit is that patch checking should be significantly
less resource intensive and much quicker.
(See
https://github.com/go-gitea/gitea/issues/22083\#issuecomment-1347961737)
Ref #22083
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Moved files in a patch will result in git apply returning:
```
error: {filename}: No such file or directory
```
This wasn't handled by the git apply patch code. This PR adds handling
for this.
Fix#22083
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Change all license headers to comply with REUSE specification.
Fix#16132
Co-authored-by: flynnnnnnnnnn <flynnnnnnnnnn@github>
Co-authored-by: John Olheiser <john.olheiser@gmail.com>
This PR adds a context parameter to a bunch of methods. Some helper
`xxxCtx()` methods got replaced with the normal name now.
Co-authored-by: delvh <dev.lh@web.de>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
There is a subtle bug in the code relating to collating the results of
`git ls-files -u -z` in `unmergedFiles()`. The code here makes the
mistake of assuming that every unmerged file will always have a stage 1
conflict, and this results in conflicts that occur in stage 3 only being
dropped.
This PR simply adjusts this code to ensure that any empty unmergedFile
will always be passed down the channel.
The PR also adds a lot of Trace commands to attempt to help find future
bugs in this code.
Fix#19527
Signed-off-by: Andrew Thornton <art27@cantab.net>
* Fixes issue #19603 (Not able to merge commit in PR when branches content is same, but different commit id)
* fill HeadCommitID in PullRequest
* compare real commits ID as check for merging
* based on @zeripath patch in #19738
* Move access and repo permission to models/perm/access
* fix test
* fix git test
* Move functions sequence
* Some improvements per @KN4CK3R and @delvh
* Move issues related code to models/issues
* Move some issues related sub package
* Merge
* Fix test
* Fix test
* Fix test
* Fix test
* Rename some files
* Set correct PR status on 3way on conflict checking
- When 3-way merge is enabled for conflict checking, it has a new
interesting behavior that it doesn't return any error when it found a
conflict, so we change the condition to not check for the error, but
instead check if conflictedfiles is populated, this fixes a issue
whereby PR status wasn't correctly on conflicted PR's.
- Refactor the mergeable property(which was incorrectly set and lead me this
bug) to be more maintainable.
- Add a dedicated test for conflicting checking, so it should prevent
future issues with this.
* Fix linter
Follows #19266, #8553, Close#18553, now there are only three `Run..(&RunOpts{})` functions.
* before: `stdout, err := RunInDir(path)`
* now: `stdout, _, err := RunStdString(&git.RunOpts{Dir:path})`
There is yet another problem with conflicted files not being reset when
the test patch resolves them.
This PR adjusts the code for checkConflicts to reset the ConflictedFiles
field immediately at the top. It also adds a reset to conflictedFiles
for the manuallyMerged and a shortcut for the empty status in
protectedfiles.
Signed-off-by: Andrew Thornton <art27@cantab.net>
This code adds a simple endpoint to apply patches to repositories and
branches on gitea. This is then used along with the conflicting checking
code in #18004 to provide a basic implementation of cherry-pick revert.
Now because the buttons necessary for cherry-pick and revert have
required us to create a dropdown next to the Browse Source button
I've also implemented Create Branch and Create Tag operations.
Fix#3880Fix#17986
Signed-off-by: Andrew Thornton <art27@cantab.net>
* Detect conflicts with 3way merge
Unforunately git apply --3way reports conflicts differently than standard patches
resulting in conflicts being missed.
Adjust the conflict detection code to account for this different error reporting.
Fix#18514
Signed-off-by: Andrew Thornton <art27@cantab.net>
* and three-way failed
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
This PR continues the work in #17125 by progressively ensuring that git
commands run within the request context.
This now means that the if there is a git repo already open in the context it will be used instead of reopening it.
Signed-off-by: Andrew Thornton <art27@cantab.net>
Although #18004 will seriously reduce the likelihood of finding
conflicts in the first place - one bug was introduced whereby the
conflicted files status was not being reset properly. This leads to
conflicted PRs remaining conflicted when the conflict has been resolved.
Signed-off-by: Andrew Thornton <art27@cantab.net>
The current TestPatch conflict code uses a plain git apply which does not properly
account for 3-way merging. However, we can improve things using `git read-tree -m` to
do a three-way merge then follow the algorithm used in merge-one-file. We can also use
`--patience` and/or `--histogram` to generate a nicer diff for applying patches too.
Fix#13679Fix#6417
Signed-off-by: Andrew Thornton <art27@cantab.net>
The io/ioutil package has been deprecated as of Go 1.16, see
https://golang.org/doc/go1.16#ioutil. This commit replaces the existing
io/ioutil functions with their new definitions in io and os packages.
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Fixes#16381
Note that changes to unprotected files via the web editor still cannot be pushed directly to the protected branch. I could easily add such support for edits and deletes if needed. But for adding, uploading or renaming unprotected files, it is not trivial.
* Extract & Move GetAffectedFiles to modules/git
* Make auto check manual merge as a chooseable mod and add manual merge way on ui
as title, Before this pr, we use same way with GH to check manually merge.
It good, but in some special cases, misjudgments can occur. and it's hard
to fix this bug. So I add option to allow repo manager block "auto check manual merge"
function, Then it will have same style like gitlab(allow empty pr). and to compensate for
not being able to detect THE PR merge automatically, I added a manual approach.
Signed-off-by: a1012112796 <1012112796@qq.com>
* make swager
* api support
* ping ci
* fix TestPullCreate_EmptyChangesWithCommits
* Apply suggestions from code review
Co-authored-by: zeripath <art27@cantab.net>
* Apply review suggestions and add test
* Apply suggestions from code review
Co-authored-by: zeripath <art27@cantab.net>
* fix build
* test error message
* make fmt
* Fix indentation issues identified by @silverwind
Co-authored-by: silverwind <me@silverwind.io>
* Fix tests and make manually merged disabled error on API the same
Signed-off-by: Andrew Thornton <art27@cantab.net>
* a small nit
* fix wrong commit id error
* fix bug
* simple test
* fix test
Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
* [Enhancement] Allow admin to merge pr with protected file changes
As tilte, show protected message in diff page and merge box.
Signed-off-by: a1012112796 <1012112796@qq.com>
* remove unused ver
* Update options/locale/locale_en-US.ini
Co-authored-by: Cirno the Strongest <1447794+CirnoT@users.noreply.github.com>
* Add TrN
* Apply suggestions from code review
* fix lint
* Update options/locale/locale_en-US.ini
Co-authored-by: zeripath <art27@cantab.net>
* Apply suggestions from code review
* move pr proteced files check to TestPatch
* Call TestPatch when protected branches settings changed
* Apply review suggestion @CirnoT
* move to service @lunny
* slightly restructure routers/private/hook.go
Adds a lot of comments and simplifies the logic
Signed-off-by: Andrew Thornton <art27@cantab.net>
* placate lint
Signed-off-by: Andrew Thornton <art27@cantab.net>
* skip duplicate protected files check
* fix check logic
* slight refactor of TestPatch
Signed-off-by: Andrew Thornton <art27@cantab.net>
* When checking for protected files changes in TestPatch use the temporary repository
Signed-off-by: Andrew Thornton <art27@cantab.net>
* fix introduced issue with hook
Signed-off-by: Andrew Thornton <art27@cantab.net>
* Remove the check on PR index being greater than 0 as it unnecessary
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: techknowlogick <matti@mdranta.net>
Co-authored-by: Cirno the Strongest <1447794+CirnoT@users.noreply.github.com>
Co-authored-by: zeripath <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Replace all calls to os.Remove/os.RemoveAll by retrying util.Remove/util.RemoveAll and remove circular dependencies from util.
Fix#12339
Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: silverwind <me@silverwind.io>
* Generate Diff and Patch direct from Pull head
Fix#10932
Also fix "Empty Diff/Patch File when pull is merged"
Closes#10934
Signed-off-by: Andrew Thornton <art27@cantab.net>
* Add tests to ensure that diff does not change
Signed-off-by: Andrew Thornton <art27@cantab.net>
* Ensure diffs and pulls pages work if head branch is deleted too
Signed-off-by: Andrew Thornton <art27@cantab.net>
* Add require signed commit for protected branch
* Fix fmt
* Make editor show if they will be signed
* bugfix
* Add basic merge check and better information for CRUD
* linting comment
* Add descriptors to merge signing
* Slight refactor
* Slight improvement to appearances
* Handle Merge API
* manage CRUD API
* Move error to error.go
* Remove fix to delete.go
* prep for merge
* need to tolerate \r\n in message
* check protected branch before trying to load it
* Apply suggestions from code review
Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>
* fix commit-reader
Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>
* Save patches to temporary files
* Remove SavePatch and generate patches on the fly
* Use ioutil.TempDir
* fixup! Use ioutil.TempDir
* fixup! fixup! Use ioutil.TempDir
* RemoveAll LocalCopyPath() in initIntergrationTest
* Default to status checking on PR creation
* Remove unnecessary set to StatusChecking
* Protect against unable to load repo
* Handle conflicts
* Restore original conflict setting
* In TestPullRequests update status to StatusChecking before running TestPatch
