Add a button to merge requests to merge the branch on the server-side. Of course, don't do it if there are conflicts. Also need to consider server-side filesystem permissions and make sure install docs are updated if needed, etc.
The code changes look good, I think this is a good direction. I haven't tested it yet though.
After fetching the downstream repo into the upstream repo, I wonder if that information will be transferred to checkouts of the upstream repo. Not necessarily a problem, but we should be aware of any side effects.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
As far as I can tell, nothing visible to the end user will be transferred, because we're not creating named branch for the branch we fetch from downstream repo. Commits will, obviously, be there, but not attached to any specific branch, so if you checkout upstream repo, and then run git branch -a you see only branches that was actually pushed to that repo.
It might be slightly better to move commit detection to the downstream repo, because upstream commits will be present there already most of the time, and when they are not, we still will need to fetch them for [#7836]
Either downstream or upstream remotes is fine with me, whatever works out best for both this ticket and [#7836]. Perhaps it would be slightly simpler to manage one upstream remote on each fork, compared to many downstream remotes on the one main repo?
We do not actually adding a remote but just fetching branch that we need using repo url (git & mercurial allow you to do that). I'll look into that during mercurial and [#7836] tickets more closely, though.
To detect merge conflicts for mercurial, I'm cloning repo and trying to actually merge. Seems like there is no other way to do it without messing up the repo. It must be fast in most of the cases, since mercurial uses FS hard links when cloning from the same file system. However it can take a long time if repo has subrepos, which all will be fetched when we're trying to merge (e.g. https://bitbucket.org/sjl/dotfiles/src)
We may want to move can_merge to a background task and make an AJAX call from merge request page to determine if request can be merged, but I think we can create another ticket for that.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
This is going to be very nice :) I did find some things to improve though:
clone_url isn't good to use since it may be overridden with a external_checkout_url setting. Moreover, we should clone from the filesystem rather than through a full checkout url (unnecessary overhead). Development configuration has a filesystem path for the checkout url, but on a real site they will be git:// and http:// urls. I think full_fs_path is what we need probably.
We could cache if hash->hash is merge-able, probably right on the merge request document. If source or destination is updated, the cache would not be valid of course. That might be useful for big repos where can_merge takes a while, but not sure how necessary it is yet.
Merge conflict was not detected in the case of a MR deleting a file and upstream modifying the file. Same for the reverse (MR modifies file, upstream removes it) and different renames. The merge error handling worked, but it'd be better if can_merge detected this properly.
Can we change the commit author from the default Author: Allura user <allura@my.host.name>? I think using the current user's full name and no email <> would be good. Changing the commit message might be really nice too, the current Merge commit 'LONG-COMMIT-HASH' [into BRANCH] is ok, but the commit hash is pretty long and not too useful. Ideally we'd want the message to specify the downstream repo and branch, or maybe the merge request URL? (Too bad we don't have an artifact shortlink for merge requests yet). It seems the way to do set those fields on a merge is to run it with --no-commit and then commit manually with a specified author and message.
Have not tested hg yet.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
clone_url: yes, I stumbled upon it yesterday, while working on [#7836] and full_fs_path should work good. I'll change that.
Caching: I had the same approach in mind, but I think we don't need to complicate things yet, and should see how it works without it.
Conflicts: I'll look into that. Don't sure if it is possible to detect such conflicts with current approach, though. Maybe we'll need something like hg does, which is trying to merge and see if it fails, but it will be slower, so we'll need to move conflict detection to background task.
With current approach we can't detect those git conflicts you mentioned above. Maybe we can do smarter parsing of output of merge-tree, but it seems pretty complicated and error-prone.
We can't use 'removed in' as indicator of conflict, since different files can be removed in upstream and downstream and in this case there's no conflict. See output examples here.
I think most reliable approach is to try run merge in temporary repository clone and see if it fails (as hg does). As I mentioned earlier it would require background task + ajax monitoring on user page. What do you think?
I've fixed other issues you mentioned.
Also take a look at my comment on [#7836]. It can affect this ticket too.
Updated allura:ib/7830 (force-push).
No new merge request for ForgeHg yet. I found some issues with it, so work in progress.
Closed #729.
ib/7830I've implemented only Git part, but I think review would be helpful before I can proceed.
Created #733 for mercurial.
The code changes look good, I think this is a good direction. I haven't tested it yet though.
After fetching the downstream repo into the upstream repo, I wonder if that information will be transferred to checkouts of the upstream repo. Not necessarily a problem, but we should be aware of any side effects.
As far as I can tell, nothing visible to the end user will be transferred, because we're not creating named branch for the branch we fetch from downstream repo. Commits will, obviously, be there, but not attached to any specific branch, so if you checkout upstream repo, and then run
git branch -ayou see only branches that was actually pushed to that repo.It might be slightly better to move commit detection to the downstream repo, because upstream commits will be present there already most of the time, and when they are not, we still will need to fetch them for [#7836]
Related
Tickets:
#7836Last edit: Igor Bondarenko 2015-02-24
Either downstream or upstream remotes is fine with me, whatever works out best for both this ticket and [#7836]. Perhaps it would be slightly simpler to manage one upstream remote on each fork, compared to many downstream remotes on the one main repo?
Related
Tickets:
#7836We do not actually adding a remote but just fetching branch that we need using repo url (git & mercurial allow you to do that). I'll look into that during mercurial and [#7836] tickets more closely, though.
Related
Tickets:
#7836Closed #733. Force-pushed
ib/7830Merge request for ForgeHg is here.
To detect merge conflicts for mercurial, I'm cloning repo and trying to actually merge. Seems like there is no other way to do it without messing up the repo. It must be fast in most of the cases, since mercurial uses FS hard links when cloning from the same file system. However it can take a long time if repo has subrepos, which all will be fetched when we're trying to merge (e.g. https://bitbucket.org/sjl/dotfiles/src)
We may want to move
can_mergeto a background task and make an AJAX call from merge request page to determine if request can be merged, but I think we can create another ticket for that.This is going to be very nice :) I did find some things to improve though:
clone_urlisn't good to use since it may be overridden with aexternal_checkout_urlsetting. Moreover, we should clone from the filesystem rather than through a full checkout url (unnecessary overhead). Development configuration has a filesystem path for the checkout url, but on a real site they will be git:// and http:// urls. I thinkfull_fs_pathis what we need probably.We could cache if hash->hash is merge-able, probably right on the merge request document. If source or destination is updated, the cache would not be valid of course. That might be useful for big repos where
can_mergetakes a while, but not sure how necessary it is yet.Merge conflict was not detected in the case of a MR deleting a file and upstream modifying the file. Same for the reverse (MR modifies file, upstream removes it) and different renames. The merge error handling worked, but it'd be better if
can_mergedetected this properly.Can we change the commit author from the default
Author: Allura user <allura@my.host.name>? I think using the current user's full name and no email<>would be good. Changing the commit message might be really nice too, the currentMerge commit 'LONG-COMMIT-HASH' [into BRANCH]is ok, but the commit hash is pretty long and not too useful. Ideally we'd want the message to specify the downstream repo and branch, or maybe the merge request URL? (Too bad we don't have an artifact shortlink for merge requests yet). It seems the way to do set those fields on a merge is to run it with--no-commitand then commit manually with a specified author and message.Have not tested hg yet.
clone_url: yes, I stumbled upon it yesterday, while working on [#7836] andfull_fs_pathshould work good. I'll change that.Caching: I had the same approach in mind, but I think we don't need to complicate things yet, and should see how it works without it.
Conflicts: I'll look into that. Don't sure if it is possible to detect such conflicts with current approach, though. Maybe we'll need something like hg does, which is trying to merge and see if it fails, but it will be slower, so we'll need to move conflict detection to background task.
Commit message: will do.
Related
Tickets:
#7836With current approach we can't detect those git conflicts you mentioned above. Maybe we can do smarter parsing of output of
merge-tree, but it seems pretty complicated and error-prone.We can't use 'removed in' as indicator of conflict, since different files can be removed in upstream and downstream and in this case there's no conflict. See output examples here.
I think most reliable approach is to try run merge in temporary repository clone and see if it fails (as hg does). As I mentioned earlier it would require background task + ajax monitoring on user page. What do you think?
I've fixed other issues you mentioned.
Also take a look at my comment on [#7836]. It can affect this ticket too.
Updated
allura:ib/7830(force-push).No new merge request for ForgeHg yet. I found some issues with it, so work in progress.
Related
Tickets:
#7836ForgeHG merge request https://sourceforge.net/p/forgehg/code/merge-requests/3/
See also my comment above.
I think this is good. If we want to improve the conflict detection in the future, your suggestion makes sense.