Skip to content

Conversation

@zeripath
Copy link
Contributor

@zeripath zeripath commented Aug 6, 2019

This PR does a number of things.

  • makes it possible to run the test suite on git 1.7.2 and should make gitea work again on git 1.7.2.
  • makes it possible to run the test suite on git 2.1.2 making gitea work on git 2.1.2.
  • standardise the base, tracking and staging branches in the merge to simply be base, tracking and staging - thus getting rid of a potential branchname conflict (albeit an unlikely one.)

Fixes #7862
Fixes #7863

@zeripath zeripath added type/bug issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change backport/v1.9 labels Aug 6, 2019
@zeripath zeripath added this to the 1.10.0 milestone Aug 6, 2019
@zeripath zeripath changed the title Fix #7754: Make merge work on 1.7.2 Fix #7754: Make merge work on git 1.7.2 Aug 6, 2019
@zeripath
Copy link
Contributor Author

zeripath commented Aug 6, 2019

Damn! That breaks the build.

Ok what needs to happen is that the temporary repo just fetches the base and head branches into standard named branches. Perhaps these could simply be master and tracking to keep with the names the system uses internally. That way there is no change that there can be a collision - at present there is a small chance of this.

I'm out of time to do this now so I'll have to leave this broken here for now.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 6, 2019
@lafriks
Copy link
Member

lafriks commented Aug 6, 2019

Great job tracking this one down! 🎉

@lunny
Copy link
Member

lunny commented Aug 7, 2019

How about extract merge bug to a separate PR?

@zeripath
Copy link
Contributor Author

zeripath commented Aug 7, 2019

Hey @lunny, so feel free to extract out the second commit separately (which should fix #7754) - my thought process was that I couldn't replicate the problem until I had TestGit working again on my local machine and so I worked from there. Stupidly I forgot to run the full set of tests before pushing.

I won't be able to get to look again at fixing this PR until the evening UK time. The first three commits should be separately extractable - the problem lies in the fourth.

@jdehaan
Copy link

jdehaan commented Aug 7, 2019

I think this is definitively solving a couple of issues, the whitelist problem is overcome. But in the following steps there are now issues.

Noticeable is the double lines in the logging... Maybe a hint, see following comment from @sharpSteff

We try to increase the logging settings in order to provide more information. The settings were not touched since ages and there was a deeper change in 1.9. That explains why we have so less information especially about "ROUTER"

@sharpSteff
Copy link

got problems with rebase and merge in 1.10.0+dev-144-gb5b7b67

2019/08/07 06:31:25 routers/repo/pull.go:629:MergePullRequest() [E] Merge: git checkout: From /srv/git/yyy/xxxx.git
	 * [new branch]      bug/6327-TargetArrayType -> head_repo/bug/6327-TargetArrayType
	 * [new branch]      bug/6327-TargetArrayType -> head_repo/bug/6327-TargetArrayType
	warning: refname 'head_repo/bug/6327-TargetArrayType' is ambiguous.
	warning: refname 'head_repo/bug/6327-TargetArrayType' is ambiguous.
	fatal: Ambiguous object name: 'head_repo/bug/6327-TargetArrayType'.

However squash and merge does the job

@zeripath
Copy link
Contributor Author

zeripath commented Aug 7, 2019

@jdehaan if you can see if just the second commit solves the issue that would be good to know. The extra commits are to resolve issues with git <1.9

@jdehaan
Copy link

jdehaan commented Aug 7, 2019

I understand. I will checkout & build 7ca5dd06688b85483eb42b1c702fc2152225c147 and report. I could take some time I got some urgent tasks on my desk. But I will definitively follow up this topic till the end ;-)

@jdehaan
Copy link

jdehaan commented Aug 7, 2019

@sharpSteff I deployed the version mentioned above (2nd commit of this PR). Could you please try out once again and report the result (at time of your convenience).

@sharpSteff
Copy link

@zeripath @jdehaan

looks good so far....
rebase works
merge works
push with/without protection works as expected

@codecov-io
Copy link

codecov-io commented Aug 7, 2019

Codecov Report

Merging #7775 into master will increase coverage by <.01%.
The diff coverage is 39.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7775      +/-   ##
==========================================
+ Coverage   41.84%   41.84%   +<.01%     
==========================================
  Files         497      497              
  Lines       65745    65807      +62     
==========================================
+ Hits        27510    27537      +27     
- Misses      34707    34733      +26     
- Partials     3528     3537       +9
Impacted Files Coverage Δ
modules/git/repo_tree.go 36.61% <0%> (-4.66%) ⬇️
modules/repofiles/upload.go 0% <0%> (ø) ⬆️
services/mirror/mirror.go 18.93% <0%> (ø) ⬆️
modules/process/manager.go 78.37% <100%> (+1.56%) ⬆️
models/repo.go 48.01% <100%> (ø) ⬆️
modules/git/repo_branch.go 78.57% <100%> (ø) ⬆️
modules/repofiles/update.go 38.75% <27.27%> (+0.33%) ⬆️
services/pull/merge.go 35.29% <27.27%> (-1.62%) ⬇️
modules/repofiles/temp_repo.go 70.44% <75.86%> (-0.07%) ⬇️
modules/git/repo.go 56.5% <0%> (-3%) ⬇️
... and 4 more

Continue to review full report at Codecov.

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

@zeripath
Copy link
Contributor Author

zeripath commented Aug 7, 2019

OK so there are number of other issues with running old git versions. (Unfortunately most of these are probably my fault.) I will open a PR to track fixes for old git versions.

@zeripath zeripath changed the title Fix #7754: Make merge work on git 1.7.2 Fix #7754: Make merge work on early gits Aug 7, 2019
@lunny lunny mentioned this pull request Aug 8, 2019
7 tasks
@zeripath zeripath changed the title Fix #7754: Make merge work on early gits Restore functionality for early gits and fix #7754 Aug 8, 2019
@zeripath zeripath force-pushed the fix-7754-Make-merge-work-on-1.7.2 branch from 7887ace to 04c67ed Compare August 14, 2019 12:01
@zeripath
Copy link
Contributor Author

zeripath commented Oct 9, 2019

Ok, so if you try to run Gitea and the test suite with git 1.7.2 on Gitea master it will fail.

Similarly for 1.8.

If you try to run Giteas integration test suite with lfs switched off it will currently fail.

I think there's a few other versions that I test for in the pr.

There's another slight efficiency change in that we won't get tags when doing pr merges and branch name conflicts are also now completely prevented during merges too.

@guillep2k
Copy link
Member

@zeripath Sorry, I was looking for something more specific, like whether a specific breaking changes in git or so. For example, I see the usage of "git remove" vs. "git rm" is one of them. But now I realize my question was kinda silly. 😄

@zeripath
Copy link
Contributor Author

zeripath commented Oct 9, 2019

No worries. It was a very boring PR finding when each option was added and if git required the option to be not present if it wasn't supported

(For example git fetch --no-tags seems to be simply ignored by gits too old to have that support.)

@zeripath
Copy link
Contributor Author

zeripath commented Oct 9, 2019

The more interesting things are in services/pull/merge.go and that's where the potential breakages are. I will take another look at those tomorrow myself.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 10, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 11, 2019
@lunny
Copy link
Member

lunny commented Oct 12, 2019

make L-G-T-M work

@lunny lunny merged commit 5e759b6 into go-gitea:master Oct 12, 2019
@lafriks
Copy link
Member

lafriks commented Oct 12, 2019

Please send backport

@zeripath zeripath deleted the fix-7754-Make-merge-work-on-1.7.2 branch October 12, 2019 06:59
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot run integration tests on git 1.7.2, 1.8.0, 2.1.2+ Gitea no longer works on git 1.7.2

8 participants