| indygreg |
| hg-reviewers |
Earlier, manifestmerge() and calculateupdates() returns a tuple of three things.
I wanted to add one more thing to return value.
Introducing a special class which represents results of a merge will help
understand better and also ease adding new return values.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
This whole series aims to improve the merge code by introducing a mergeresult object which represents result of a merge and using it in various merge related functions. Right now all of this done using actions dict which is also different in different functions. Having a dedicated object will make things easier to understand and do some optimizations.
The series however long consists of small obvious patches.
| mercurial/merge.py | ||
|---|---|---|
| 563 | I'm generally not a fan of having trivial @property wrappers for internal attributes like this, as I don't think it adds any value. Especially since there is code mutating the internal state, which is a bit janky. But I'll approve this anyway. If the code remains unchanged after the completion of the series, my vote is to remove the @property wrappers and access attributes directly. | |
| Path | Packages | |||
|---|---|---|---|---|
| M | hgext/convert/hg.py (4 lines) | |||
| M | hgext/largefiles/overrides.py (50 lines) | |||
| M | mercurial/merge.py (86 lines) |
| Status | Author | Revision | |
|---|---|---|---|
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit | ||
| Closed | pulkit |
| and p2, given that the merge is coming from the given source. | and p2, given that the merge is coming from the given source. | ||||
| This prevents us from losing files that only exist in the target p2 and | This prevents us from losing files that only exist in the target p2 and | ||||
| that don't come from the source repo (like if you're merging multiple | that don't come from the source repo (like if you're merging multiple | ||||
| repositories together). | repositories together). | ||||
| """ | """ | ||||
| anc = [p1ctx.ancestor(p2ctx)] | anc = [p1ctx.ancestor(p2ctx)] | ||||
| # Calculate what files are coming from p2 | # Calculate what files are coming from p2 | ||||
| actions, diverge, rename = mergemod.calculateupdates( | mresult = mergemod.calculateupdates( | ||||
| self.repo, | self.repo, | ||||
| p1ctx, | p1ctx, | ||||
| p2ctx, | p2ctx, | ||||
| anc, | anc, | ||||
| branchmerge=True, | branchmerge=True, | ||||
| force=True, | force=True, | ||||
| acceptremote=False, | acceptremote=False, | ||||
| followcopies=False, | followcopies=False, | ||||
| ) | ) | ||||
| for file, (action, info, msg) in pycompat.iteritems(actions): | for file, (action, info, msg) in pycompat.iteritems(mresult.actions): | ||||
| if source.targetfilebelongstosource(file): | if source.targetfilebelongstosource(file): | ||||
| # If the file belongs to the source repo, ignore the p2 | # If the file belongs to the source repo, ignore the p2 | ||||
| # since it will be covered by the existing fileset. | # since it will be covered by the existing fileset. | ||||
| continue | continue | ||||
| # If the file requires actual merging, abort. We don't have enough | # If the file requires actual merging, abort. We don't have enough | ||||
| # context to resolve merges correctly. | # context to resolve merges correctly. | ||||
| if action in [b'm', b'dm', b'cd', b'dc']: | if action in [b'm', b'dm', b'cd', b'dc']: | ||||
| # Finally, the merge.applyupdates function will then take care of | # Finally, the merge.applyupdates function will then take care of | ||||
| # writing the files into the working copy and lfcommands.updatelfiles | # writing the files into the working copy and lfcommands.updatelfiles | ||||
| # will update the largefiles. | # will update the largefiles. | ||||
| @eh.wrapfunction(merge, b'calculateupdates') | @eh.wrapfunction(merge, b'calculateupdates') | ||||
| def overridecalculateupdates( | def overridecalculateupdates( | ||||
| origfn, repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs | origfn, repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs | ||||
| ): | ): | ||||
| overwrite = force and not branchmerge | overwrite = force and not branchmerge | ||||
| actions, diverge, renamedelete = origfn( | mresult = origfn( | ||||
| repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs | repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs | ||||
| ) | ) | ||||
| if overwrite: | if overwrite: | ||||
| return actions, diverge, renamedelete | return mresult | ||||
| # Convert to dictionary with filename as key and action as value. | # Convert to dictionary with filename as key and action as value. | ||||
| lfiles = set() | lfiles = set() | ||||
| for f in actions: | for f in mresult.actions: | ||||
| splitstandin = lfutil.splitstandin(f) | splitstandin = lfutil.splitstandin(f) | ||||
| if splitstandin is not None and splitstandin in p1: | if splitstandin is not None and splitstandin in p1: | ||||
| lfiles.add(splitstandin) | lfiles.add(splitstandin) | ||||
| elif lfutil.standin(f) in p1: | elif lfutil.standin(f) in p1: | ||||
| lfiles.add(f) | lfiles.add(f) | ||||
| for lfile in sorted(lfiles): | for lfile in sorted(lfiles): | ||||
| standin = lfutil.standin(lfile) | standin = lfutil.standin(lfile) | ||||
| (lm, largs, lmsg) = actions.get(lfile, (None, None, None)) | (lm, largs, lmsg) = mresult.actions.get(lfile, (None, None, None)) | ||||
| (sm, sargs, smsg) = actions.get(standin, (None, None, None)) | (sm, sargs, smsg) = mresult.actions.get(standin, (None, None, None)) | ||||
| if sm in (b'g', b'dc') and lm != b'r': | if sm in (b'g', b'dc') and lm != b'r': | ||||
| if sm == b'dc': | if sm == b'dc': | ||||
| f1, f2, fa, move, anc = sargs | f1, f2, fa, move, anc = sargs | ||||
| sargs = (p2[f2].flags(), False) | sargs = (p2[f2].flags(), False) | ||||
| # Case 1: normal file in the working copy, largefile in | # Case 1: normal file in the working copy, largefile in | ||||
| # the second parent | # the second parent | ||||
| usermsg = ( | usermsg = ( | ||||
| _( | _( | ||||
| b'remote turned local normal file %s into a largefile\n' | b'remote turned local normal file %s into a largefile\n' | ||||
| b'use (l)argefile or keep (n)ormal file?' | b'use (l)argefile or keep (n)ormal file?' | ||||
| b'$$ &Largefile $$ &Normal file' | b'$$ &Largefile $$ &Normal file' | ||||
| ) | ) | ||||
| % lfile | % lfile | ||||
| ) | ) | ||||
| if repo.ui.promptchoice(usermsg, 0) == 0: # pick remote largefile | if repo.ui.promptchoice(usermsg, 0) == 0: # pick remote largefile | ||||
| actions[lfile] = (b'r', None, b'replaced by standin') | mresult.actions[lfile] = (b'r', None, b'replaced by standin') | ||||
| actions[standin] = (b'g', sargs, b'replaces standin') | mresult.actions[standin] = (b'g', sargs, b'replaces standin') | ||||
| else: # keep local normal file | else: # keep local normal file | ||||
| actions[lfile] = (b'k', None, b'replaces standin') | mresult.actions[lfile] = (b'k', None, b'replaces standin') | ||||
| if branchmerge: | if branchmerge: | ||||
| actions[standin] = (b'k', None, b'replaced by non-standin') | mresult.actions[standin] = ( | ||||
| b'k', | |||||
| None, | |||||
| b'replaced by non-standin', | |||||
| ) | |||||
| else: | else: | ||||
| actions[standin] = (b'r', None, b'replaced by non-standin') | mresult.actions[standin] = ( | ||||
| b'r', | |||||
| None, | |||||
| b'replaced by non-standin', | |||||
| ) | |||||
| elif lm in (b'g', b'dc') and sm != b'r': | elif lm in (b'g', b'dc') and sm != b'r': | ||||
| if lm == b'dc': | if lm == b'dc': | ||||
| f1, f2, fa, move, anc = largs | f1, f2, fa, move, anc = largs | ||||
| largs = (p2[f2].flags(), False) | largs = (p2[f2].flags(), False) | ||||
| # Case 2: largefile in the working copy, normal file in | # Case 2: largefile in the working copy, normal file in | ||||
| # the second parent | # the second parent | ||||
| usermsg = ( | usermsg = ( | ||||
| _( | _( | ||||
| b'remote turned local largefile %s into a normal file\n' | b'remote turned local largefile %s into a normal file\n' | ||||
| b'keep (l)argefile or use (n)ormal file?' | b'keep (l)argefile or use (n)ormal file?' | ||||
| b'$$ &Largefile $$ &Normal file' | b'$$ &Largefile $$ &Normal file' | ||||
| ) | ) | ||||
| % lfile | % lfile | ||||
| ) | ) | ||||
| if repo.ui.promptchoice(usermsg, 0) == 0: # keep local largefile | if repo.ui.promptchoice(usermsg, 0) == 0: # keep local largefile | ||||
| if branchmerge: | if branchmerge: | ||||
| # largefile can be restored from standin safely | # largefile can be restored from standin safely | ||||
| actions[lfile] = (b'k', None, b'replaced by standin') | mresult.actions[lfile] = ( | ||||
| actions[standin] = (b'k', None, b'replaces standin') | b'k', | ||||
| None, | |||||
| b'replaced by standin', | |||||
| ) | |||||
| mresult.actions[standin] = (b'k', None, b'replaces standin') | |||||
| else: | else: | ||||
| # "lfile" should be marked as "removed" without | # "lfile" should be marked as "removed" without | ||||
| # removal of itself | # removal of itself | ||||
| actions[lfile] = ( | mresult.actions[lfile] = ( | ||||
| b'lfmr', | b'lfmr', | ||||
| None, | None, | ||||
| b'forget non-standin largefile', | b'forget non-standin largefile', | ||||
| ) | ) | ||||
| # linear-merge should treat this largefile as 're-added' | # linear-merge should treat this largefile as 're-added' | ||||
| actions[standin] = (b'a', None, b'keep standin') | mresult.actions[standin] = (b'a', None, b'keep standin') | ||||
| else: # pick remote normal file | else: # pick remote normal file | ||||
| actions[lfile] = (b'g', largs, b'replaces standin') | mresult.actions[lfile] = (b'g', largs, b'replaces standin') | ||||
| actions[standin] = (b'r', None, b'replaced by non-standin') | mresult.actions[standin] = ( | ||||
| b'r', | |||||
| None, | |||||
| b'replaced by non-standin', | |||||
| ) | |||||
| return actions, diverge, renamedelete | return mresult | ||||
| @eh.wrapfunction(mergestatemod, b'recordupdates') | @eh.wrapfunction(mergestatemod, b'recordupdates') | ||||
| def mergerecordupdates(orig, repo, actions, branchmerge, getfiledata): | def mergerecordupdates(orig, repo, actions, branchmerge, getfiledata): | ||||
| if b'lfmr' in actions: | if b'lfmr' in actions: | ||||
| lfdirstate = lfutil.openlfdirstate(repo.ui, repo) | lfdirstate = lfutil.openlfdirstate(repo.ui, repo) | ||||
| for lfile, args, msg in actions[b'lfmr']: | for lfile, args, msg in actions[b'lfmr']: | ||||
| # this should be executed before 'orig', to execute 'remove' | # this should be executed before 'orig', to execute 'remove' | ||||
I'm generally not a fan of having trivial @property wrappers for internal attributes like this, as I don't think it adds any value. Especially since there is code mutating the internal state, which is a bit janky.
But I'll approve this anyway. If the code remains unchanged after the completion of the series, my vote is to remove the @property wrappers and access attributes directly.