On 12/06/2016 05:41 PM, Mateusz Kwapich wrote:
# HG changeset patch
# User Mateusz Kwapich <mitran...@fb.com>
# 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

Reply via email to