Patchwork D5410: merge: allow to merge non-conflicting changes outside narrowspec

login
register
mail settings
Submitter phabricator
Date Dec. 11, 2018, 11:22 a.m.
Message ID <differential-rev-PHID-DREV-eww5yqo3g4spjnmvcrrr-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/37106/
State New
Headers show

Comments

phabricator - Dec. 11, 2018, 11:22 a.m.
pulkit created this revision.
Herald added a reviewer: durin42.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch allows merging of non-conflicting changes outside narrowspec.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5410

AFFECTED FILES
  mercurial/merge.py
  tests/test-narrow-merge.t

CHANGE DETAILS




To: pulkit, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 11, 2018, 11:25 a.m.
pulkit added a subscriber: martinvonz.
pulkit added a comment.


  I am dubiuos that my fix is correct. I went through the history and didn't find any explanation why we don't allow merging non-conflicting changes outside narrowspec except TODO's. @martinvonz do you know why we don't allow merging of non-conflicting changes here?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5410

To: pulkit, durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Dec. 11, 2018, 2:47 p.m.
martinvonz requested changes to this revision.
martinvonz added a comment.
This revision now requires changes to proceed.


  I'm pretty sure this doesn't actually perform the merge, it just drops the changes outside the narrowspec. On commit, we need to record that outside/ has the new nodeid that we got from the side we merged in. To do that, we need to remember what that nodeid is, from the time of `hg merge` to the time of `hg commit`. That probably means storing the nodeid in the dirstate (like git does), or maybe in the merge state.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5410

To: pulkit, durin42, #hg-reviewers, martinvonz
Cc: martinvonz, mercurial-devel
phabricator - Feb. 4, 2019, 1:48 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D5410#80207, @martinvonz wrote:
  
  > I'm pretty sure this doesn't actually perform the merge, it just drops the changes outside the narrowspec. On commit, we need to record that outside/ has the new nodeid that we got from the side we merged in. To do that, we need to remember what that nodeid is, from the time of `hg merge` to the time of `hg commit`. That probably means storing the nodeid in the dirstate (like git does), or maybe in the merge state.
  
  
  IIUC, dirstate does not contains files outside narrowspec. Maybe we need to do this in merge state?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5410

To: pulkit, durin42, #hg-reviewers, martinvonz
Cc: martinvonz, mercurial-devel
phabricator - Feb. 4, 2019, 4:24 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D5410#85232, @pulkit wrote:
  
  > In https://phab.mercurial-scm.org/D5410#80207, @martinvonz wrote:
  >
  > > I'm pretty sure this doesn't actually perform the merge, it just drops the changes outside the narrowspec. On commit, we need to record that outside/ has the new nodeid that we got from the side we merged in. To do that, we need to remember what that nodeid is, from the time of `hg merge` to the time of `hg commit`. That probably means storing the nodeid in the dirstate (like git does), or maybe in the merge state.
  >
  >
  > IIUC, dirstate does not contains files outside narrowspec. Maybe we need to do this in merge state?
  
  
  I think neither of them currently contains files outside narrows, so we'd need to extend whichever we decide. It's probably easier to extend the merge state. I'm not sure which I think is cleaner to extend. There's precedent for populating the commit based on the dirstate (index) in git.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5410

To: pulkit, durin42, #hg-reviewers, martinvonz
Cc: martinvonz, mercurial-devel
phabricator - Feb. 21, 2019, 4:58 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D5410#85244, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/D5410#85232, @pulkit wrote:
  >
  > > In https://phab.mercurial-scm.org/D5410#80207, @martinvonz wrote:
  > >
  > > > I'm pretty sure this doesn't actually perform the merge, it just drops the changes outside the narrowspec. On commit, we need to record that outside/ has the new nodeid that we got from the side we merged in. To do that, we need to remember what that nodeid is, from the time of `hg merge` to the time of `hg commit`. That probably means storing the nodeid in the dirstate (like git does), or maybe in the merge state.
  > >
  > >
  > > IIUC, dirstate does not contains files outside narrowspec. Maybe we need to do this in merge state?
  >
  >
  > I think neither of them currently contains files outside narrows, so we'd need to extend whichever we decide. It's probably easier to extend the merge state. I'm not sure which I think is cleaner to extend. There's precedent for populating the commit based on the dirstate (index) in git.
  
  
  Or not touch any of them and introduce a new file?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5410

To: pulkit, durin42, #hg-reviewers, martinvonz
Cc: martinvonz, mercurial-devel
phabricator - Feb. 22, 2019, 1:36 a.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D5410#87526, @pulkit wrote:
  
  > In https://phab.mercurial-scm.org/D5410#85244, @martinvonz wrote:
  >
  > > In https://phab.mercurial-scm.org/D5410#85232, @pulkit wrote:
  > >
  > > > In https://phab.mercurial-scm.org/D5410#80207, @martinvonz wrote:
  > > >
  > > > > I'm pretty sure this doesn't actually perform the merge, it just drops the changes outside the narrowspec. On commit, we need to record that outside/ has the new nodeid that we got from the side we merged in. To do that, we need to remember what that nodeid is, from the time of `hg merge` to the time of `hg commit`. That probably means storing the nodeid in the dirstate (like git does), or maybe in the merge state.
  > > >
  > > >
  > > > IIUC, dirstate does not contains files outside narrowspec. Maybe we need to do this in merge state?
  > >
  > >
  > > I think neither of them currently contains files outside narrows, so we'd need to extend whichever we decide. It's probably easier to extend the merge state. I'm not sure which I think is cleaner to extend. There's precedent for populating the commit based on the dirstate (index) in git.
  >
  >
  > Or not touch any of them and introduce a new file?
  
  
  Sure, that's another option. It might be the easiest one since it won't involve making the format compatible (although I think that's pretty easy for the merge state).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5410

To: pulkit, durin42, #hg-reviewers, martinvonz
Cc: martinvonz, mercurial-devel

Patch

diff --git a/tests/test-narrow-merge.t b/tests/test-narrow-merge.t
--- a/tests/test-narrow-merge.t
+++ b/tests/test-narrow-merge.t
@@ -80,16 +80,14 @@ 
   (no more unresolved files)
   $ hg commit -m 'merge inside/f1'
 
-TODO: Can merge non-conflicting changes outside narrow spec
+Can merge non-conflicting changes outside narrow spec
 
   $ hg update -q 'desc("modify inside/f1")'
   $ hg merge 'desc("modify outside/f1")'
-  abort: merge affects file 'outside/f1' outside narrow, which is not yet supported (flat !)
-  abort: merge affects file 'outside/' outside narrow, which is not yet supported (tree !)
-  (merging in the other direction may work)
-  [255]
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
 
-  $ hg update -q 'desc("modify outside/f1")'
+  $ hg update -q 'desc("modify outside/f1")' -C
   $ hg merge 'desc("modify inside/f1")'
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1109,10 +1109,7 @@ 
         elif action[0] in nooptypes:
             del actions[f] # merge does not affect file
         elif action[0] in nonconflicttypes:
-            raise error.Abort(_('merge affects file \'%s\' outside narrow, '
-                                'which is not yet supported') % f,
-                              hint=_('merging in the other direction '
-                                     'may work'))
+            del actions[f]
         else:
             raise error.Abort(_('conflict in file \'%s\' is outside '
                                 'narrow clone') % f)