#7830 One-click merge

v1.3.0
closed
SCM
2015-08-20
2015-02-09
No

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.

Related

Tickets: #7836
Tickets: #7848

Discussion

  • Dave Brondsema

    Dave Brondsema - 2015-02-09
    • labels: --> sf-current, sf-4
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-02-13
    • labels: sf-current, sf-4 --> sf-current, sf-4, 42cc
    • assigned_to: Igor Bondarenko
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-02-13
    • status: open --> in-progress
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-02-19
    • status: in-progress --> review
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-02-19

    Closed #729. ib/7830

    I've implemented only Git part, but I think review would be helpful before I can proceed.

    Created #733 for mercurial.

     
  • Dave Brondsema

    Dave Brondsema - 2015-02-23

    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.

     
    • Igor Bondarenko

      Igor Bondarenko - 2015-02-24

      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]

       

      Related

      Tickets: #7836


      Last edit: Igor Bondarenko 2015-02-24
      • Dave Brondsema

        Dave Brondsema - 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: #7836

        • Igor Bondarenko

          Igor Bondarenko - 2015-02-25

          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.

           

          Related

          Tickets: #7836

  • Dave Brondsema

    Dave Brondsema - 2015-02-23
    • status: review --> in-progress
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-02-27
    • status: in-progress --> review
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-02-27

    Closed #733. Force-pushed ib/7830

    Merge 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_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.

     
  • Dave Brondsema

    Dave Brondsema - 2015-03-05
    • status: review --> in-progress
    • Reviewer: Dave Brondsema
     
  • Dave Brondsema

    Dave Brondsema - 2015-03-05

    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.

     
    • Igor Bondarenko

      Igor Bondarenko - 2015-03-06

      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.

      Commit message: will do.

       

      Related

      Tickets: #7836

  • Igor Bondarenko

    Igor Bondarenko - 2015-03-06

    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.

     

    Related

    Tickets: #7836

  • Igor Bondarenko

    Igor Bondarenko - 2015-03-18
    • status: in-progress --> review
     
  • Dave Brondsema

    Dave Brondsema - 2015-03-20
    • status: review --> closed
     
  • Dave Brondsema

    Dave Brondsema - 2015-03-20

    I think this is good. If we want to improve the conflict detection in the future, your suggestion makes sense.

     
  • Dave Brondsema

    Dave Brondsema - 2015-03-23
    • labels: sf-current, sf-4, 42cc --> sf-4, 42cc
     
  • Igor Bondarenko

    Igor Bondarenko - 2015-06-18
    • Milestone: unreleased --> asf_release_1.3.0
     

Log in to post a comment.