GNOME Bugzilla – Bug 690469
Automatic 3 way merge for conflicts
Last modified: 2013-03-29 20:07:06 UTC
Created attachment 231852 [details] [review] Patch to add CONFLICT types, and get_path_for_conflict methods to bzr and git. Also updates bzr to have get_path_for_repo_file When opening a file which is listed by VC as conflicted, that it is opened in a 3 way merge as OTHER/MERGE HEAD | merged | THIS/HEAD Patches attached.
Created attachment 231853 [details] [review] Patch to update svn to use 3 way diff. Attached patch updates svn to use a 3 way diff. It does involve significant changes to how it's cached tree is handled, but brings it in to line with how git and bzr handle the cached tree. This is needed so vcview can make assumptions on what is returned when the below is called: self.vc._get_tree_cache(path).get(path, None)
Created attachment 231855 [details] [review] Patch to add kwargs to create-diff emit Patch to add kwargs to create-diff emit Also adds merge_output kwarg to append_filemerge and append_diff, so we can specify where auto_merge output goes from meldwindow
Created attachment 231856 [details] [review] patch to open conflicted files in 3 way auto merge Patch to open conflicted files in 3 way auto merge. It's possible to change this to use the content of 'MERGED' instead of BASE simply by changing file2's conflict kwarg to CONFLICT_MERGED and removing the kwargs['auto_merge'] = True line. There has been discussion on mailing list as to if we should use merged, or do the merge ourselves using diff3 markers inside meld.
Review of attachment 231852 [details] [review]: This patch is the most important of the series to me. I'd like to get this in, and then think about the remainder. ::: meld/vc/_vc.py @@ +36,3 @@ STATE_MISSING, STATE_NONEXIST, STATE_MAX = list(range(13)) +# Order is important here, matches up with git show numbers Could we please instead have a line in git.py making sure that these numbers line up, rather than requiring it here? It feels wrong to have that restriction in _vc. @@ +43,3 @@ +CONFLICT_THIS = CONFLICT_LOCAL + +conflicts = _("Merged:Base:Local:Remote").split(':') I know we do this elsewhere in the codebase, but please don't. While translators *can* work with it, I'd rather they didn't have to deal. @@ +44,3 @@ + +conflicts = _("Merged:Base:Local:Remote").split(':') +# These names are used by BZR, and are logically identical. Missing space around operator. ::: meld/vc/bzr.py @@ +175,3 @@ + with tempfile.NamedTemporaryFile(prefix='meld-tmp', delete=False) as f: + shutil.copyfileobj(vc_file, f) + args.append("-r%s" % commit) I'd really appreciate it if you split this get_path_for_repo_file() addition out into a separate patch. There's no reason that I can see that these should be linked. @@ +182,3 @@ + raise _vc.InvalidVCPath(self, path, "Path not in repository") + + vc_file = process.stdout Is there no less-dodgy way of getting conflicting files in bzr via the command line? The git code for example actually queries the repo state to get the files... I don't know whether bzr (or svn) have such a facility, but if they do, I'd very much prefer to use it. ::: meld/vc/git.py @@ +81,3 @@ return [self.CMD,"checkout"] + def get_path_for_conflict(self, path, conflict=None): None doesn't seem to be a sensible default value here. Shouldn't we default to 0 or something?
Review of attachment 231853 [details] [review]: I'll need you to explain the point of the large SVN changes. Are they required in some way by the conflict part of the patch (which is mostly fine), or is this just a 'cleanup'? Either way, it looks like this borrows heavily from the Git version, but doesn't fix any of the PEP8 violations that are there. ::: meld/vc/svn.py @@ +106,3 @@ + + return "%s%s" % (path, self.conflict_map[conflict]) + Same comment as for the bzr change; can we get this via the command line, rather than just hoping that the files are there?
Review of attachment 231852 [details] [review]: Commented stuff that needs it, other changes made, about to upload the new patch. ::: meld/vc/bzr.py @@ +182,3 @@ + raise _vc.InvalidVCPath(self, path, "Path not in repository") + + vc_file = process.stdout From what I can see, there is no easy way. It IS possible to get BASE and LOCAL/THIS via cli (bzr cat -r ancestor <file>), but the no such luck for MERGED or REMOTE/OTHER directly. It is possible by doing a bzr info, getting the parent branch path, then bzr cat -d <parent branch> <file> but if the parent branch has been updated since you started the merge this could fail miserably. Seems safer to use the files created during the original merge unfortunately. ::: meld/vc/git.py @@ +81,3 @@ return [self.CMD,"checkout"] + def get_path_for_conflict(self, path, conflict=None): Indeed, it shouldn't even be a keyword, changed it to a required parameter instead.
Created attachment 238083 [details] [review] Adds conflict methods to bzr and git, and conflict types to VC Stripped down base patch, and fixed comments as per review.
Review of attachment 231853 [details] [review]: Because of the way the code in vcview.py works, if the path list inside a vc class doesn't conform with the newer types uses in bzr/git etc. it instead just queues a diff of the file by itself and won't check for conflicts. (look ~ line 465 in current HEAD), therefore it won't ever call get_path_for_conflict as in the other patch if this is the case. It seemed easier to bring svn up to use a more current method than to change the core functionality here. I'm actually tempted to leave svn as is to be honest - merging is generally simpler as there's only ever a single parent repo, but will give it a crack if we do want it as well. ::: meld/vc/svn.py @@ +106,3 @@ + + return "%s%s" % (path, self.conflict_map[conflict]) + I've had further look at this, and my code is actually broken. Instead we have to do ugly globs... for the highest versions of <file>.r* svn only has revision spec to get 'HEAD' latest in repository 'BASE' base rev of item's working copy 'COMMITTED' last commit at or before BASE 'PREV' revision just before COMMITTED So again, no way to get the current changes of the working copy except by getting the .mine file.
Review of attachment 231856 [details] [review]: ::: meld/vcview.py @@ +473,3 @@ + path, conflict=tree.CONFLICT_BASE) + file3 = self.vc.get_path_for_conflict( + # the current file. Just a placeholder for now, that conflict will no longer be a keyword argument now.
Review of attachment 231856 [details] [review]: Changed status on patch to needs-work
Review of attachment 231855 [details] [review]: Just noting a section which conflicts with current HEAD ::: meld/meldwindow.py @@ +629,3 @@ page.connect("label-changed", self.on_notebook_label_changed) page.connect("file-changed", self.on_file_changed) + page.connect("create-diff", lambda obj, arg, kwargs: This section conflicts with HEAD atm.
Created attachment 238266 [details] [review] Patch to add kwargs to create-diff emit Updated patch for current git head.
Created attachment 238267 [details] [review] patch to open conflicted files in 3 way auto merge Updated arguments for get_path_for_conflict Also defaulted to use the MERGED output from VCS, until we sort out the auto-merge bar prompt.
(In reply to comment #8) > Review of attachment 231853 [details] [review]: > > Because of the way the code in vcview.py works, if the path list inside a vc > class doesn't conform with the newer types uses in bzr/git etc. it instead just > queues a diff of the file by itself and won't check for conflicts. (look ~ line > 465 in current HEAD), therefore it won't ever call get_path_for_conflict as in > the other patch if this is the case. > > It seemed easier to bring svn up to use a more current method than to change > the core functionality here. > > I'm actually tempted to leave svn as is to be honest - merging is generally > simpler as there's only ever a single parent repo, but will give it a crack if > we do want it as well. > > ::: meld/vc/svn.py > @@ +106,3 @@ > + > + return "%s%s" % (path, self.conflict_map[conflict]) > + > > I've had further look at this, and my code is actually broken. > Instead we have to do ugly globs... for the highest versions of <file>.r* > > > svn only has revision spec to get > 'HEAD' latest in repository > 'BASE' base rev of item's working copy > 'COMMITTED' last commit at or before BASE > 'PREV' revision just before COMMITTED > So again, no way to get the current changes of the working copy except by > getting the .mine file. Given the number of people sadly still on subversion, I think it would be nice to do this. Having said that, I have no problem with only supporting it for subversion >1.7, which should reduce the required work and make life easier. If you're still interested in making this happen, then we can keep this bug open, or open another one to track SVN specifically.
Created attachment 239320 [details] [review] Add conflict map to git
Created attachment 239321 [details] [review] Use merged by default, fix permission changes
Will move SVN patch to new bug. Two new patches needed for HEAD - as mentioned on patch descriptions.
Second patch is all good. The first one is mostly good, but the CONFLICT_MERGED handling is a bit odd. Can't we just *not* add in the CONFLICT_MERGED state in the dict, and 'return path' if we hit the CONFLICT_MERGED state? I mean that's basically what we're doing anyway... or have I misread something?
Created attachment 239661 [details] [review] Add conflict map to git Entirely correct! I think I was playing with a few things and forgot to clean up entirely before submitting this... Updated, should be fine now.
Excellent, thanks. I've pushed those patches so... are we good to close this bug? The subversion stuff is being tracked elsewhere, and I'll have to spend some quality time with that before pushing, just because of the significance of the changes for SVN <1.7.
Yeah good to close unless we want to keep it open for the auto-merge part, but that can probably be a separate bug (and not something I'm dying for urgently, most of the merging can (and probably should) be done by the VCS generally.
Created attachment 239857 [details] [review] Add is_temp to get_path_for_conflict Then I forgot that I hadn't uploaded this patch yet... just to remove some error messages when non-temp files are removed (As discussed in mailing list)
Looks good. I actually went through and removed comments and such; the meaning is relatively clear, and while the docstrings would be nice, they really belong in a new superclass that doesn't yet exist. Anyway, thanks very much for following through on all of these; this will all make a very nice addition to 1.7.2.
Created attachment 239905 [details] [review] Fix for is_temp being passed to the diff emit I'm annoyed at myself for missing this. diffs needs to only have the path names, as it's passed on to the actual create-diff emit.
I changed your patch around a bit and pushed; I thought it was clearer to just re-create what we needed with a second comprehension. Thanks.