Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the test for mergeability #6417

Closed
MarkusAmshove opened this issue Mar 23, 2019 · 3 comments · Fixed by #18004
Closed

Improve the test for mergeability #6417

MarkusAmshove opened this issue Mar 23, 2019 · 3 comments · Fixed by #18004
Labels
type/enhancement An improvement of existing functionality type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@MarkusAmshove
Copy link
Contributor

MarkusAmshove commented Mar 23, 2019

Description

In our workflow we have two branches with continuous deployment: master and qa (production and quality assurance).

Because both deploy on new pushes, they're both branch protected and require a PR with a review.

We always branch off new features from master and the first target after development is qa for acceptance testing.
We often seem to have conflicts when targeting a feature to qa, because the qa has more features that are currently in testing and are not released into master yet.
What we do then is manually resolve the issues so that git is able to merge it locally.
Merging qa into the feature is no option, because then the feature is dependent on other features that might not be ready for production.

Now to the Gitea part :-)

Have a look at this PR which emulates a conflict that is shown in Gitea but can be merged locally.
Gitea isn't able to merge it and just says it is conflicted.

However, you can do the following:

git clone https://try.gitea.io/conflicttest/noconflict
cd noconflict
git checkout qs
git merge --no-ff feature

Git is happy to merge it.

I've also tested what Github would show, here is the result: MarkusAmshove/Conflict#1

The problem seems to be that Gitea "just" tries to apply the branch as a patch with, which can be reproduced locally:

[10:29] markus@suse-markus ~/scm/conflicttest (qs|✔) $ git format-patch feature --stdout | git apply --check --cached
error: Anwendung des Patches fehlgeschlagen: B:3
error: B: Patch konnte nicht angewendet werden

So, what could Gitea do better in that regard?

I think that instead of doing the patch approach, Gitea should try the different merge strategies it supports (which can be configured at the repositoy settings page) and see if the PR is actually mergeable.

We only have --no-ff enabled as a possibility and the exit code would be an indicator:

[10:39] markus@suse-markus ~/scm/conflicttest (qs|✔) $ git merge feature --no-ff --no-edit
Merge made by the 'recursive' strategy.
[10:39] markus@suse-markus ~/scm/conflicttest (qs↑ 3|✔) $ echo $status
0

In another repository where there are indeed conflicts:

[10:40] markus@suse-markus ~/scm/conflict (qa|✔) $ git merge feature2 --no-ff --no-edit
automatischer Merge von README.md
KONFLIKT (Inhalt): Merge-Konflikt in README.md
Automatischer Merge fehlgeschlagen; beheben Sie die Konflikte und committen Sie dann das Ergebnis.
[10:40] markus@suse-markus ~/scm/conflict (qa|MERGING|✖1⚡1) $ echo $status
1

Proposal

When testing for mergeability of a PR, try the different merge strategies configured in Gitea (for the repository) to see if the PR can be merged.

Bonus points if, when there are multiple strategies configured, Gitea "grays out" strategies that aren't possible.

@MarkusAmshove
Copy link
Contributor Author

MarkusAmshove commented Mar 23, 2019

Please note that this is a constructed example to show the limitations of the current check.

Having the same contents also results in Gitea showing 404 on the files tab:
https://try.gitea.io/conflicttest/conflict/pulls/1/files

While Github still shows the changes:
https://github.com/MarkusAmshove/Conflict/pull/1/files

I haven't investigated what Github could do different for the file diffs.

Edit: Don't mind the 404, I linked the wrong repository 😆

@lunny lunny added type/enhancement An improvement of existing functionality type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Mar 24, 2019
@ollien
Copy link

ollien commented Mar 24, 2019

Did you mean to post https://try.gitea.io/conflicttest/noconflict/pulls/1? Your other links 404

@MarkusAmshove
Copy link
Contributor Author

You're right, I messed up the repository names :-)
I'll update the issue

6543 pushed a commit that referenced this issue Dec 19, 2021
…-file functionality (#18004)

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 #13679
Fix #6417

Signed-off-by: Andrew Thornton <art27@cantab.net>
Chianina pushed a commit to Chianina/gitea that referenced this issue Mar 28, 2022
…-file functionality (go-gitea#18004)

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 go-gitea#13679
Fix go-gitea#6417

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/enhancement An improvement of existing functionality type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
3 participants