Re: [PATCH 1 of 8 evolve-ext, V2] metaedit: add a helper function for just metadata rewrites

2016-12-20 Thread Pierre-Yves David 

On 12/06/2016 05:41 PM, Mateusz Kwapich wrote:

# HG changeset patch
# User Mateusz Kwapich 
# Date 1481028829 28800
#  Tue Dec 06 04:53:49 2016 -0800
# Branch stable
# Node ID 78b75ed14103cee05ed13948025310919adde559
# Parent  727c7211c810d304ebf92b32db7ecf697ce46ac6
metaedit: add a helper function for just metadata rewrites

It will be used by metaedit.


I've eventually came to review this. Sorry for the delay.

The overall approach seems right, and I'm happy to see the "multiple 
revs" case implemented. However, I'm not thrilled with the amount of 
code duplicated. I think we can share more codepath here. I was not too 
sure of where to explain how, so you get a wall of text in patch 1. 
Happy reading.


About rewrite
-

The very venerable 'rewrite' function have been in this repository since 
september 19th 2011. And was actually written by Peter Arrenbrecht in 
spring 2011 (I just imported it in). It was originally written to handle 
'amend'. But is no longer used for this purpose.


Its signature is a bit strange.

  def rewrite(repo, old, updates, head, newbases, commitopts):

If will effectively fold "old + updates" and "rebase" the result on 
"newbases" without actually merging anything. so "updates" MUST be 
linear direct descendant of "old", and "newbases" must have the same 
manifest content as the old's parents.


"head" seems to just be 'heads(old + updates)'
The meta-data of "old" are reused for the resulting commit and mixed 
with "commitopts"



It looks like it could use some cleanup if you feel brave enough.

About metarewrite vs rewrite
--

metarewrite seems to be doing the same things as rewrite but:
1) For only a single changesets
2) In the case we know we can reuse the manifest revision (and so we do).

We can probably detect that directly within 'rewrite' and take a fast 
path in that case.


* We can detect (1) if 'updates' is empty (and therefore head is old),
* we check if the manifest is reusable by checking if the manifest 
nodeid for 'old.p1()' and 'old.p2()' match the one in 'newbases'. If we 
detect these two conditions, we can skip the expensive 'files' 
computation in rewrite ctx, and just call memlightctx to create the 
changeset.


These two checkes + small amount of conditional branching seems small 
enough to me that I don't think we should duplicate the code and just 
have rewrite be a bit smarter. This would avoid the two code bases to 
slowly diverge for no good reason.
In addition that would benefit other command that use 'rewrite' in 
compatible situation (eg: fold).


What do you think ?

About the core fold code


The same apply to the code duplication in the body of the metaedit 
function. We get two entirely new "codebase" for each code path (fold 
and not fold) while they still have a lot in common. This lead to slow 
but dangerous code drift. We already see it in your series where fold is 
properly handling phase preservation while not-fold lost it.


So in my opinion we could have a single code base using the fact that:

* Having only 1 folding action can still be expressed in a list of actions
* Not folding changesets together can be expressed with folding a set of 
only one changeset.


At that point, I'll move the rest into the reply to patch 3. for clarity.

Cheers


diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -907,6 +907,13 @@ def rewrite(repo, old, updates, head, ne
 finally:
 lockmod.release(tr, lock, wlock)

+def metarewrite(repo, old, newbases, commitopts):
+'''Like rewrite but affects only the changeset metadata.'''
+# TODO: reuse the manifest for speed
+newid, created = rewrite(repo, old, [old], old, newbases,
+ commitopts=commitopts)
+return newid, created
+
 class MergeFailure(error.Abort):
 pass

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel



--
Pierre-Yves David
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[PATCH 1 of 8 evolve-ext, V2] metaedit: add a helper function for just metadata rewrites

2016-12-06 Thread Mateusz Kwapich 
# HG changeset patch
# User Mateusz Kwapich 
# Date 1481028829 28800
#  Tue Dec 06 04:53:49 2016 -0800
# Branch stable
# Node ID 78b75ed14103cee05ed13948025310919adde559
# Parent  727c7211c810d304ebf92b32db7ecf697ce46ac6
metaedit: add a helper function for just metadata rewrites

It will be used by metaedit.

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -907,6 +907,13 @@ def rewrite(repo, old, updates, head, ne
 finally:
 lockmod.release(tr, lock, wlock)
 
+def metarewrite(repo, old, newbases, commitopts):
+'''Like rewrite but affects only the changeset metadata.'''
+# TODO: reuse the manifest for speed
+newid, created = rewrite(repo, old, [old], old, newbases,
+ commitopts=commitopts)
+return newid, created
+
 class MergeFailure(error.Abort):
 pass
 
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel