Re: D122: phabricator: add --amend option to phabsend

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously `hg phabsend` was imitating `hg email` and won't mutate
  changesets. That works fine with reviewer-push workflow, reviewers run
  `phabread`, `import`.
 
  However, it does not work well with author-push workflow. Namely, the author
  needs to run extra commands to get the right commit message, and remove the
  local tag after push.
 
  This patch solves those issues by adding the `--amend` option, so local
  changesets will have the right commit message, and tags become unnecessary.

TEST PLAN
  Given the following DAG:
 
    o  17
    o  16
    | o  15
    | @  14
    |/
    o  13
    o  12
 
  Run `hg phabsend '(13::)-17'  --amend`, check the new DAG looks like:
 
    o  21
    | o  20
    | @  19
    |/
    o  18
    | o  17
    | x  16
    | x  13
    |/
    o  12
 
  And commit messages are updated to contain the `Differential Revision` lines.
 
  Also check `phabsend .` followed by a `phabsend . --amend`, the commit
  message will be updated and the tag will be removed.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -38,6 +38,8 @@
 from mercurial.node import bin, nullid
 from mercurial.i18n import _
 from mercurial import (
+    cmdutil,
+    context,
     encoding,
     error,
     mdiff,
@@ -325,6 +327,7 @@
 
 @command('phabsend',
          [('r', 'rev', [], _('revisions to send'), _('REV')),
+          ('', 'amend', False, _('update commit messages')),
           ('', 'reviewer', [], _('specify reviewers'))],
          _('REV [OPTIONS]'))
 def phabsend(ui, repo, *revs, **opts):
@@ -334,16 +337,21 @@
     with a linear dependencies relationship using the order specified by the
     revset.
 
-    For the first time uploading changesets, local tags will be created to
-    maintain the association. After the first time, phabsend will check
-    obsstore and tags information so it can figure out whether to update an
-    existing Differential Revision, or create a new one.
+    If --amend is set, update commit messages so they have the
+    ``Differential Revision`` URL, remove related tags. This is similar to what
+    arcanist will do, and is more desired in author-push workflows. Otherwise,
+    use local tags to record the ``Differential Revision`` association.
+
+    phabsend will check obsstore and the above association to decide whether to
+    update an existing Differential Revision, or create a new one.
     """
     revs = list(revs) + opts.get('rev', [])
     revs = scmutil.revrange(repo, revs)
 
     if not revs:
         raise error.Abort(_('phabsend requires at least one changeset'))
+    if opts.get('amend'):
+        cmdutil.checkunfinished(repo)
 
     actions = []
     reviewers = opts.get('reviewer', [])
@@ -355,6 +363,7 @@
 
     # Send patches one by one so we know their Differential Revision IDs and
     # can provide dependency relationship
+    drevids = []
     lastrevid = None
     for rev in revs:
         ui.debug('sending rev %d\n' % rev)
@@ -372,20 +381,55 @@
             else:
                 action = _('created')
 
-            # Create a local tag to note the association
-            tagname = 'D%d' % newrevid
-            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
-                     date=None, local=True)
+            # Create a local tag to note the association, if commit message
+            # does not have it already
+            m = _differentialrevisiondescre.search(ctx.description())
+            if not m or int(m.group(1)) != newrevid:
+                tagname = 'D%d' % newrevid
+                tags.tag(repo, tagname, ctx.node(), message=None, user=None,
+                         date=None, local=True)
         else:
             # Nothing changed. But still set "newrevid" so the next revision
             # could depend on this one.
             newrevid = revid
             action = _('skipped')
 
         ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
                                             ctx.description().split('\n')[0]))
+        drevids.append(newrevid)
         lastrevid = newrevid
 
+    # Update commit messages and remove tags
+    if opts.get('amend'):
+        unfi = repo.unfiltered()
+        drevs = callconduit(repo, 'differential.query', {'ids': drevids})
+        with repo.wlock(), repo.lock(), repo.transaction('phabsend'):
+            wnode = unfi['.'].node()
+            mapping = {} # {oldnode: newnode}
+            for i, rev in enumerate(revs):
+                old = unfi[rev]
+                drevid = drevids[i]
+                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
+                newdesc = getdescfromdrev(drev)
+                # Make sure commit message contain "Differential Revision"
+                if old.description() != newdesc:
+                    parents = [
+                        mapping.get(old.p1().node(), (old.p1(),))[0],
+                        mapping.get(old.p2().node(), (old.p2(),))[0],
+                    ]
+                    new = context.metadataonlyctx(
+                        repo, old, parents=parents, text=newdesc,
+                        user=old.user(), date=old.date(), extra=old.extra())
+                    mapping[old.node()] = [new.commit()]
+                # Remove local tags since it's no longer necessary
+                tagname = 'D%d' % drevid
+                if tagname in repo.tags():
+                    tags.tag(repo, tagname, nullid, message=None, user=None,
+                             date=None, local=True)
+            scmutil.cleanupnodes(repo, mapping, 'phabsend')
+            if wnode in mapping:
+                unfi.setparents(mapping[wnode][0])
+
 # Map from "hg:meta" keys to header understood by "hg import". The order is
 # consistent with "hg export" output.
 _metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'),



EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: quark, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
mitrandir accepted this revision.
mitrandir added inline comments.

INLINE COMMENTS

> phabricator.py:413
> +                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
> +                newdesc = getdescfromdrev(drev)
> +                # Make sure commit message contain "Differential Revision"

I think it would be better to use differential.getcommitmessage for this. For example "reviewed by" field is a really nice thing to have in the commit message.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: quark, #hg-reviewers, mitrandir
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
In reply to this post by phillco (Phil Cohen)
quark added inline comments.

INLINE COMMENTS

> mitrandir wrote in phabricator.py:413
> I think it would be better to use differential.getcommitmessage for this. For example "reviewed by" field is a really nice thing to have in the commit message.

I was trying to make it behave closer to `hg email`. IIRC reviewers have some other plans to show "who accepts" information (that works for both Phabricator and email submits) on hgweb, so I didn't pay too much attention on "Reviewed By" here.

REPOSITORY
  rHG Mercurial

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

EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: quark, #hg-reviewers, mitrandir
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
In reply to this post by phillco (Phil Cohen)
quark updated this revision to Diff 485.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D122?vs=233&id=485

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

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -38,6 +38,8 @@
 from mercurial.node import bin, nullid
 from mercurial.i18n import _
 from mercurial import (
+    cmdutil,
+    context,
     encoding,
     error,
     mdiff,
@@ -324,6 +326,7 @@
 
 @command('phabsend',
          [('r', 'rev', [], _('revisions to send'), _('REV')),
+          ('', 'amend', False, _('update commit messages')),
           ('', 'reviewer', [], _('specify reviewers'))],
          _('REV [OPTIONS]'))
 def phabsend(ui, repo, *revs, **opts):
@@ -333,16 +336,21 @@
     with a linear dependencies relationship using the order specified by the
     revset.
 
-    For the first time uploading changesets, local tags will be created to
-    maintain the association. After the first time, phabsend will check
-    obsstore and tags information so it can figure out whether to update an
-    existing Differential Revision, or create a new one.
+    If --amend is set, update commit messages so they have the
+    ``Differential Revision`` URL, remove related tags. This is similar to what
+    arcanist will do, and is more desired in author-push workflows. Otherwise,
+    use local tags to record the ``Differential Revision`` association.
+
+    phabsend will check obsstore and the above association to decide whether to
+    update an existing Differential Revision, or create a new one.
     """
     revs = list(revs) + opts.get('rev', [])
     revs = scmutil.revrange(repo, revs)
 
     if not revs:
         raise error.Abort(_('phabsend requires at least one changeset'))
+    if opts.get('amend'):
+        cmdutil.checkunfinished(repo)
 
     actions = []
     reviewers = opts.get('reviewer', [])
@@ -354,6 +362,7 @@
 
     # Send patches one by one so we know their Differential Revision IDs and
     # can provide dependency relationship
+    drevids = []
     lastrevid = None
     for rev in revs:
         ui.debug('sending rev %d\n' % rev)
@@ -371,20 +380,55 @@
             else:
                 action = _('created')
 
-            # Create a local tag to note the association
-            tagname = 'D%d' % newrevid
-            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
-                     date=None, local=True)
+            # Create a local tag to note the association, if commit message
+            # does not have it already
+            m = _differentialrevisiondescre.search(ctx.description())
+            if not m or int(m.group(1)) != newrevid:
+                tagname = 'D%d' % newrevid
+                tags.tag(repo, tagname, ctx.node(), message=None, user=None,
+                         date=None, local=True)
         else:
             # Nothing changed. But still set "newrevid" so the next revision
             # could depend on this one.
             newrevid = revid
             action = _('skipped')
 
         ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
                                             ctx.description().split('\n')[0]))
+        drevids.append(newrevid)
         lastrevid = newrevid
 
+    # Update commit messages and remove tags
+    if opts.get('amend'):
+        unfi = repo.unfiltered()
+        drevs = callconduit(repo, 'differential.query', {'ids': drevids})
+        with repo.wlock(), repo.lock(), repo.transaction('phabsend'):
+            wnode = unfi['.'].node()
+            mapping = {} # {oldnode: newnode}
+            for i, rev in enumerate(revs):
+                old = unfi[rev]
+                drevid = drevids[i]
+                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
+                newdesc = getdescfromdrev(drev)
+                # Make sure commit message contain "Differential Revision"
+                if old.description() != newdesc:
+                    parents = [
+                        mapping.get(old.p1().node(), (old.p1(),))[0],
+                        mapping.get(old.p2().node(), (old.p2(),))[0],
+                    ]
+                    new = context.metadataonlyctx(
+                        repo, old, parents=parents, text=newdesc,
+                        user=old.user(), date=old.date(), extra=old.extra())
+                    mapping[old.node()] = [new.commit()]
+                # Remove local tags since it's no longer necessary
+                tagname = 'D%d' % drevid
+                if tagname in repo.tags():
+                    tags.tag(repo, tagname, nullid, message=None, user=None,
+                             date=None, local=True)
+            scmutil.cleanupnodes(repo, mapping, 'phabsend')
+            if wnode in mapping:
+                unfi.setparents(mapping[wnode][0])
+
 # Map from "hg:meta" keys to header understood by "hg import". The order is
 # consistent with "hg export" output.
 _metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'),



To: quark, #hg-reviewers, mitrandir
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
In reply to this post by phillco (Phil Cohen)
quark updated this revision to Diff 553.
quark edited the test plan for this revision.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D122?vs=485&id=553

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

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -38,6 +38,8 @@
 from mercurial.node import bin, nullid
 from mercurial.i18n import _
 from mercurial import (
+    cmdutil,
+    context,
     encoding,
     error,
     mdiff,
@@ -316,7 +318,7 @@
     if not revision:
         raise error.Abort(_('cannot create revision for %s') % ctx)
 
-    return revision
+    return revision, diff
 
 def userphids(repo, names):
     """convert user names to PHIDs"""
@@ -334,6 +336,7 @@
 
 @command('phabsend',
          [('r', 'rev', [], _('revisions to send'), _('REV')),
+          ('', 'amend', False, _('update commit messages')),
           ('', 'reviewer', [], _('specify reviewers'))],
          _('REV [OPTIONS]'))
 def phabsend(ui, repo, *revs, **opts):
@@ -343,16 +346,21 @@
     with a linear dependencies relationship using the order specified by the
     revset.
 
-    For the first time uploading changesets, local tags will be created to
-    maintain the association. After the first time, phabsend will check
-    obsstore and tags information so it can figure out whether to update an
-    existing Differential Revision, or create a new one.
+    If --amend is set, update commit messages so they have the
+    ``Differential Revision`` URL, remove related tags. This is similar to what
+    arcanist will do, and is more desired in author-push workflows. Otherwise,
+    use local tags to record the ``Differential Revision`` association.
+
+    phabsend will check obsstore and the above association to decide whether to
+    update an existing Differential Revision, or create a new one.
     """
     revs = list(revs) + opts.get('rev', [])
     revs = scmutil.revrange(repo, revs)
 
     if not revs:
         raise error.Abort(_('phabsend requires at least one changeset'))
+    if opts.get('amend'):
+        cmdutil.checkunfinished(repo)
 
     actions = []
     reviewers = opts.get('reviewer', [])
@@ -363,6 +371,9 @@
     # {newnode: (oldnode, olddiff, olddrev}
     oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs])
 
+    drevids = [] # [int]
+    diffmap = {} # {newnode: diff}
+
     # Send patches one by one so we know their Differential Revision IDs and
     # can provide dependency relationship
     lastrevid = None
@@ -374,28 +385,67 @@
         oldnode, olddiff, revid = oldmap.get(ctx.node(), (None, None, None))
         if oldnode != ctx.node():
             # Create or update Differential Revision
-            revision = createdifferentialrevision(ctx, revid, lastrevid,
-                                                  oldnode, olddiff, actions)
+            revision, diff = createdifferentialrevision(
+                ctx, revid, lastrevid, oldnode, olddiff, actions)
+            diffmap[ctx.node()] = diff
             newrevid = int(revision[r'object'][r'id'])
             if revid:
                 action = _('updated')
             else:
                 action = _('created')
 
-            # Create a local tag to note the association
-            tagname = 'D%d' % newrevid
-            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
-                     date=None, local=True)
+            # Create a local tag to note the association, if commit message
+            # does not have it already
+            m = _differentialrevisiondescre.search(ctx.description())
+            if not m or int(m.group(1)) != newrevid:
+                tagname = 'D%d' % newrevid
+                tags.tag(repo, tagname, ctx.node(), message=None, user=None,
+                         date=None, local=True)
         else:
             # Nothing changed. But still set "newrevid" so the next revision
             # could depend on this one.
             newrevid = revid
             action = _('skipped')
 
         ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
                                             ctx.description().split('\n')[0]))
+        drevids.append(newrevid)
         lastrevid = newrevid
 
+    # Update commit messages and remove tags
+    if opts.get('amend'):
+        unfi = repo.unfiltered()
+        drevs = callconduit(repo, 'differential.query', {'ids': drevids})
+        with repo.wlock(), repo.lock(), repo.transaction('phabsend'):
+            wnode = unfi['.'].node()
+            mapping = {} # {oldnode: [newnode]}
+            for i, rev in enumerate(revs):
+                old = unfi[rev]
+                drevid = drevids[i]
+                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
+                newdesc = getdescfromdrev(drev)
+                # Make sure commit message contain "Differential Revision"
+                if old.description() != newdesc:
+                    parents = [
+                        mapping.get(old.p1().node(), (old.p1(),))[0],
+                        mapping.get(old.p2().node(), (old.p2(),))[0],
+                    ]
+                    new = context.metadataonlyctx(
+                        repo, old, parents=parents, text=newdesc,
+                        user=old.user(), date=old.date(), extra=old.extra())
+                    newnode = new.commit()
+                    mapping[old.node()] = [newnode]
+                    # Update diff property
+                    writediffproperties(unfi[newnode], diffmap[old.node()])
+                # Remove local tags since it's no longer necessary
+                tagname = 'D%d' % drevid
+                if tagname in repo.tags():
+                    tags.tag(repo, tagname, nullid, message=None, user=None,
+                             date=None, local=True)
+            scmutil.cleanupnodes(repo, mapping, 'phabsend')
+            if wnode in mapping:
+                unfi.setparents(mapping[wnode][0])
+
 # Map from "hg:meta" keys to header understood by "hg import". The order is
 # consistent with "hg export" output.
 _metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'),



To: quark, #hg-reviewers, mitrandir
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
In reply to this post by phillco (Phil Cohen)
quark updated this revision to Diff 568.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D122?vs=553&id=568

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

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -38,6 +38,8 @@
 from mercurial.node import bin, nullid
 from mercurial.i18n import _
 from mercurial import (
+    cmdutil,
+    context,
     encoding,
     error,
     mdiff,
@@ -316,7 +318,7 @@
     if not revision:
         raise error.Abort(_('cannot create revision for %s') % ctx)
 
-    return revision
+    return revision, diff
 
 def userphids(repo, names):
     """convert user names to PHIDs"""
@@ -334,6 +336,7 @@
 
 @command('phabsend',
          [('r', 'rev', [], _('revisions to send'), _('REV')),
+          ('', 'amend', False, _('update commit messages')),
           ('', 'reviewer', [], _('specify reviewers')),
           ('', 'confirm', None, _('ask for confirmation before sending'))],
          _('REV [OPTIONS]'))
@@ -349,18 +352,28 @@
     obsstore and tags information so it can figure out whether to update an
     existing Differential Revision, or create a new one.
 
+    If --amend is set, update commit messages so they have the
+    ``Differential Revision`` URL, remove related tags. This is similar to what
+    arcanist will do, and is more desired in author-push workflows. Otherwise,
+    use local tags to record the ``Differential Revision`` association.
+
     The --confirm option lets you confirm changesets before sending them. You
     can also add following to your configuration file to make it default
     behaviour.
 
     [phabsend]
     confirm = true
+
+    phabsend will check obsstore and the above association to decide whether to
+    update an existing Differential Revision, or create a new one.
     """
     revs = list(revs) + opts.get('rev', [])
     revs = scmutil.revrange(repo, revs)
 
     if not revs:
         raise error.Abort(_('phabsend requires at least one changeset'))
+    if opts.get('amend'):
+        cmdutil.checkunfinished(repo)
 
     confirm = ui.configbool('phabsend', 'confirm')
     confirm |= bool(opts.get('confirm'))
@@ -378,6 +391,9 @@
     # {newnode: (oldnode, olddiff, olddrev}
     oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs])
 
+    drevids = [] # [int]
+    diffmap = {} # {newnode: diff}
+
     # Send patches one by one so we know their Differential Revision IDs and
     # can provide dependency relationship
     lastrevid = None
@@ -389,28 +405,67 @@
         oldnode, olddiff, revid = oldmap.get(ctx.node(), (None, None, None))
         if oldnode != ctx.node():
             # Create or update Differential Revision
-            revision = createdifferentialrevision(ctx, revid, lastrevid,
-                                                  oldnode, olddiff, actions)
+            revision, diff = createdifferentialrevision(
+                ctx, revid, lastrevid, oldnode, olddiff, actions)
+            diffmap[ctx.node()] = diff
             newrevid = int(revision[r'object'][r'id'])
             if revid:
                 action = _('updated')
             else:
                 action = _('created')
 
-            # Create a local tag to note the association
-            tagname = 'D%d' % newrevid
-            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
-                     date=None, local=True)
+            # Create a local tag to note the association, if commit message
+            # does not have it already
+            m = _differentialrevisiondescre.search(ctx.description())
+            if not m or int(m.group(1)) != newrevid:
+                tagname = 'D%d' % newrevid
+                tags.tag(repo, tagname, ctx.node(), message=None, user=None,
+                         date=None, local=True)
         else:
             # Nothing changed. But still set "newrevid" so the next revision
             # could depend on this one.
             newrevid = revid
             action = _('skipped')
 
         ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
                                             ctx.description().split('\n')[0]))
+        drevids.append(newrevid)
         lastrevid = newrevid
 
+    # Update commit messages and remove tags
+    if opts.get('amend'):
+        unfi = repo.unfiltered()
+        drevs = callconduit(repo, 'differential.query', {'ids': drevids})
+        with repo.wlock(), repo.lock(), repo.transaction('phabsend'):
+            wnode = unfi['.'].node()
+            mapping = {} # {oldnode: [newnode]}
+            for i, rev in enumerate(revs):
+                old = unfi[rev]
+                drevid = drevids[i]
+                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
+                newdesc = getdescfromdrev(drev)
+                # Make sure commit message contain "Differential Revision"
+                if old.description() != newdesc:
+                    parents = [
+                        mapping.get(old.p1().node(), (old.p1(),))[0],
+                        mapping.get(old.p2().node(), (old.p2(),))[0],
+                    ]
+                    new = context.metadataonlyctx(
+                        repo, old, parents=parents, text=newdesc,
+                        user=old.user(), date=old.date(), extra=old.extra())
+                    newnode = new.commit()
+                    mapping[old.node()] = [newnode]
+                    # Update diff property
+                    writediffproperties(unfi[newnode], diffmap[old.node()])
+                # Remove local tags since it's no longer necessary
+                tagname = 'D%d' % drevid
+                if tagname in repo.tags():
+                    tags.tag(repo, tagname, nullid, message=None, user=None,
+                             date=None, local=True)
+            scmutil.cleanupnodes(repo, mapping, 'phabsend')
+            if wnode in mapping:
+                unfi.setparents(mapping[wnode][0])
+
 # Map from "hg:meta" keys to header understood by "hg import". The order is
 # consistent with "hg export" output.
 _metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'),



To: quark, #hg-reviewers, mitrandir
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
In reply to this post by phillco (Phil Cohen)
quark updated this revision to Diff 789.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D122?vs=568&id=789

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

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -38,6 +38,8 @@
 from mercurial.node import bin, nullid
 from mercurial.i18n import _
 from mercurial import (
+    cmdutil,
+    context,
     encoding,
     error,
     mdiff,
@@ -316,7 +318,7 @@
     if not revision:
         raise error.Abort(_('cannot create revision for %s') % ctx)
 
-    return revision
+    return revision, diff
 
 def userphids(repo, names):
     """convert user names to PHIDs"""
@@ -334,6 +336,7 @@
 
 @command('phabsend',
          [('r', 'rev', [], _('revisions to send'), _('REV')),
+          ('', 'amend', False, _('update commit messages')),
           ('', 'reviewer', [], _('specify reviewers')),
           ('', 'confirm', None, _('ask for confirmation before sending'))],
          _('REV [OPTIONS]'))
@@ -349,18 +352,28 @@
     obsstore and tags information so it can figure out whether to update an
     existing Differential Revision, or create a new one.
 
+    If --amend is set, update commit messages so they have the
+    ``Differential Revision`` URL, remove related tags. This is similar to what
+    arcanist will do, and is more desired in author-push workflows. Otherwise,
+    use local tags to record the ``Differential Revision`` association.
+
     The --confirm option lets you confirm changesets before sending them. You
     can also add following to your configuration file to make it default
     behaviour.
 
     [phabsend]
     confirm = true
+
+    phabsend will check obsstore and the above association to decide whether to
+    update an existing Differential Revision, or create a new one.
     """
     revs = list(revs) + opts.get('rev', [])
     revs = scmutil.revrange(repo, revs)
 
     if not revs:
         raise error.Abort(_('phabsend requires at least one changeset'))
+    if opts.get('amend'):
+        cmdutil.checkunfinished(repo)
 
     confirm = ui.configbool('phabsend', 'confirm')
     confirm |= bool(opts.get('confirm'))
@@ -378,6 +391,9 @@
     # {newnode: (oldnode, olddiff, olddrev}
     oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs])
 
+    drevids = [] # [int]
+    diffmap = {} # {newnode: diff}
+
     # Send patches one by one so we know their Differential Revision IDs and
     # can provide dependency relationship
     lastrevid = None
@@ -387,30 +403,69 @@
 
         # Get Differential Revision ID
         oldnode, olddiff, revid = oldmap.get(ctx.node(), (None, None, None))
-        if oldnode != ctx.node():
+        if oldnode != ctx.node() or opts.get('amend'):
             # Create or update Differential Revision
-            revision = createdifferentialrevision(ctx, revid, lastrevid,
-                                                  oldnode, olddiff, actions)
+            revision, diff = createdifferentialrevision(
+                ctx, revid, lastrevid, oldnode, olddiff, actions)
+            diffmap[ctx.node()] = diff
             newrevid = int(revision[r'object'][r'id'])
             if revid:
                 action = _('updated')
             else:
                 action = _('created')
 
-            # Create a local tag to note the association
-            tagname = 'D%d' % newrevid
-            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
-                     date=None, local=True)
+            # Create a local tag to note the association, if commit message
+            # does not have it already
+            m = _differentialrevisiondescre.search(ctx.description())
+            if not m or int(m.group(1)) != newrevid:
+                tagname = 'D%d' % newrevid
+                tags.tag(repo, tagname, ctx.node(), message=None, user=None,
+                         date=None, local=True)
         else:
             # Nothing changed. But still set "newrevid" so the next revision
             # could depend on this one.
             newrevid = revid
             action = _('skipped')
 
         ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
                                             ctx.description().split('\n')[0]))
+        drevids.append(newrevid)
         lastrevid = newrevid
 
+    # Update commit messages and remove tags
+    if opts.get('amend'):
+        unfi = repo.unfiltered()
+        drevs = callconduit(repo, 'differential.query', {'ids': drevids})
+        with repo.wlock(), repo.lock(), repo.transaction('phabsend'):
+            wnode = unfi['.'].node()
+            mapping = {} # {oldnode: [newnode]}
+            for i, rev in enumerate(revs):
+                old = unfi[rev]
+                drevid = drevids[i]
+                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
+                newdesc = getdescfromdrev(drev)
+                # Make sure commit message contain "Differential Revision"
+                if old.description() != newdesc:
+                    parents = [
+                        mapping.get(old.p1().node(), (old.p1(),))[0],
+                        mapping.get(old.p2().node(), (old.p2(),))[0],
+                    ]
+                    new = context.metadataonlyctx(
+                        repo, old, parents=parents, text=newdesc,
+                        user=old.user(), date=old.date(), extra=old.extra())
+                    newnode = new.commit()
+                    mapping[old.node()] = [newnode]
+                    # Update diff property
+                    writediffproperties(unfi[newnode], diffmap[old.node()])
+                # Remove local tags since it's no longer necessary
+                tagname = 'D%d' % drevid
+                if tagname in repo.tags():
+                    tags.tag(repo, tagname, nullid, message=None, user=None,
+                             date=None, local=True)
+            scmutil.cleanupnodes(repo, mapping, 'phabsend')
+            if wnode in mapping:
+                unfi.setparents(mapping[wnode][0])
+
 # Map from "hg:meta" keys to header understood by "hg import". The order is
 # consistent with "hg export" output.
 _metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'),



To: quark, #hg-reviewers, mitrandir
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
In reply to this post by phillco (Phil Cohen)
quark updated this revision to Diff 838.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D122?vs=789&id=838

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

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -38,6 +38,8 @@
 from mercurial.node import bin, nullid
 from mercurial.i18n import _
 from mercurial import (
+    cmdutil,
+    context,
     encoding,
     error,
     mdiff,
@@ -315,7 +317,7 @@
     if not revision:
         raise error.Abort(_('cannot create revision for %s') % ctx)
 
-    return revision
+    return revision, diff
 
 def userphids(repo, names):
     """convert user names to PHIDs"""
@@ -333,6 +335,7 @@
 
 @command('phabsend',
          [('r', 'rev', [], _('revisions to send'), _('REV')),
+          ('', 'amend', False, _('update commit messages')),
           ('', 'reviewer', [], _('specify reviewers')),
           ('', 'confirm', None, _('ask for confirmation before sending'))],
          _('REV [OPTIONS]'))
@@ -348,18 +351,28 @@
     obsstore and tags information so it can figure out whether to update an
     existing Differential Revision, or create a new one.
 
+    If --amend is set, update commit messages so they have the
+    ``Differential Revision`` URL, remove related tags. This is similar to what
+    arcanist will do, and is more desired in author-push workflows. Otherwise,
+    use local tags to record the ``Differential Revision`` association.
+
     The --confirm option lets you confirm changesets before sending them. You
     can also add following to your configuration file to make it default
     behaviour.
 
     [phabsend]
     confirm = true
+
+    phabsend will check obsstore and the above association to decide whether to
+    update an existing Differential Revision, or create a new one.
     """
     revs = list(revs) + opts.get('rev', [])
     revs = scmutil.revrange(repo, revs)
 
     if not revs:
         raise error.Abort(_('phabsend requires at least one changeset'))
+    if opts.get('amend'):
+        cmdutil.checkunfinished(repo)
 
     confirm = ui.configbool('phabsend', 'confirm')
     confirm |= bool(opts.get('confirm'))
@@ -377,6 +390,9 @@
     # {newnode: (oldnode, olddiff, olddrev}
     oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs])
 
+    drevids = [] # [int]
+    diffmap = {} # {newnode: diff}
+
     # Send patches one by one so we know their Differential Revision IDs and
     # can provide dependency relationship
     lastrevid = None
@@ -386,30 +402,69 @@
 
         # Get Differential Revision ID
         oldnode, olddiff, revid = oldmap.get(ctx.node(), (None, None, None))
-        if oldnode != ctx.node():
+        if oldnode != ctx.node() or opts.get('amend'):
             # Create or update Differential Revision
-            revision = createdifferentialrevision(ctx, revid, lastrevid,
-                                                  oldnode, olddiff, actions)
+            revision, diff = createdifferentialrevision(
+                ctx, revid, lastrevid, oldnode, olddiff, actions)
+            diffmap[ctx.node()] = diff
             newrevid = int(revision[r'object'][r'id'])
             if revid:
                 action = _('updated')
             else:
                 action = _('created')
 
-            # Create a local tag to note the association
-            tagname = 'D%d' % newrevid
-            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
-                     date=None, local=True)
+            # Create a local tag to note the association, if commit message
+            # does not have it already
+            m = _differentialrevisiondescre.search(ctx.description())
+            if not m or int(m.group(1)) != newrevid:
+                tagname = 'D%d' % newrevid
+                tags.tag(repo, tagname, ctx.node(), message=None, user=None,
+                         date=None, local=True)
         else:
             # Nothing changed. But still set "newrevid" so the next revision
             # could depend on this one.
             newrevid = revid
             action = _('skipped')
 
         ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
                                             ctx.description().split('\n')[0]))
+        drevids.append(newrevid)
         lastrevid = newrevid
 
+    # Update commit messages and remove tags
+    if opts.get('amend'):
+        unfi = repo.unfiltered()
+        drevs = callconduit(repo, 'differential.query', {'ids': drevids})
+        with repo.wlock(), repo.lock(), repo.transaction('phabsend'):
+            wnode = unfi['.'].node()
+            mapping = {} # {oldnode: [newnode]}
+            for i, rev in enumerate(revs):
+                old = unfi[rev]
+                drevid = drevids[i]
+                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
+                newdesc = getdescfromdrev(drev)
+                # Make sure commit message contain "Differential Revision"
+                if old.description() != newdesc:
+                    parents = [
+                        mapping.get(old.p1().node(), (old.p1(),))[0],
+                        mapping.get(old.p2().node(), (old.p2(),))[0],
+                    ]
+                    new = context.metadataonlyctx(
+                        repo, old, parents=parents, text=newdesc,
+                        user=old.user(), date=old.date(), extra=old.extra())
+                    newnode = new.commit()
+                    mapping[old.node()] = [newnode]
+                    # Update diff property
+                    writediffproperties(unfi[newnode], diffmap[old.node()])
+                # Remove local tags since it's no longer necessary
+                tagname = 'D%d' % drevid
+                if tagname in repo.tags():
+                    tags.tag(repo, tagname, nullid, message=None, user=None,
+                             date=None, local=True)
+            scmutil.cleanupnodes(repo, mapping, 'phabsend')
+            if wnode in mapping:
+                unfi.setparents(mapping[wnode][0])
+
 # Map from "hg:meta" keys to header understood by "hg import". The order is
 # consistent with "hg export" output.
 _metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'),



To: quark, #hg-reviewers, mitrandir
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D122: phabricator: add --amend option to phabsend

phillco (Phil Cohen)
In reply to this post by phillco (Phil Cohen)
This revision was automatically updated to reflect the committed changes.
Closed by commit rHGfa3aa6c98bb7: phabricator: add --amend option to phabsend (authored by quark).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D122?vs=838&id=873

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

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -38,6 +38,8 @@
 from mercurial.node import bin, nullid
 from mercurial.i18n import _
 from mercurial import (
+    cmdutil,
+    context,
     encoding,
     error,
     mdiff,
@@ -315,7 +317,7 @@
     if not revision:
         raise error.Abort(_('cannot create revision for %s') % ctx)
 
-    return revision
+    return revision, diff
 
 def userphids(repo, names):
     """convert user names to PHIDs"""
@@ -333,6 +335,7 @@
 
 @command('phabsend',
          [('r', 'rev', [], _('revisions to send'), _('REV')),
+          ('', 'amend', False, _('update commit messages')),
           ('', 'reviewer', [], _('specify reviewers')),
           ('', 'confirm', None, _('ask for confirmation before sending'))],
          _('REV [OPTIONS]'))
@@ -348,18 +351,28 @@
     obsstore and tags information so it can figure out whether to update an
     existing Differential Revision, or create a new one.
 
+    If --amend is set, update commit messages so they have the
+    ``Differential Revision`` URL, remove related tags. This is similar to what
+    arcanist will do, and is more desired in author-push workflows. Otherwise,
+    use local tags to record the ``Differential Revision`` association.
+
     The --confirm option lets you confirm changesets before sending them. You
     can also add following to your configuration file to make it default
     behaviour.
 
     [phabsend]
     confirm = true
+
+    phabsend will check obsstore and the above association to decide whether to
+    update an existing Differential Revision, or create a new one.
     """
     revs = list(revs) + opts.get('rev', [])
     revs = scmutil.revrange(repo, revs)
 
     if not revs:
         raise error.Abort(_('phabsend requires at least one changeset'))
+    if opts.get('amend'):
+        cmdutil.checkunfinished(repo)
 
     confirm = ui.configbool('phabsend', 'confirm')
     confirm |= bool(opts.get('confirm'))
@@ -377,6 +390,9 @@
     # {newnode: (oldnode, olddiff, olddrev}
     oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs])
 
+    drevids = [] # [int]
+    diffmap = {} # {newnode: diff}
+
     # Send patches one by one so we know their Differential Revision IDs and
     # can provide dependency relationship
     lastrevid = None
@@ -386,30 +402,69 @@
 
         # Get Differential Revision ID
         oldnode, olddiff, revid = oldmap.get(ctx.node(), (None, None, None))
-        if oldnode != ctx.node():
+        if oldnode != ctx.node() or opts.get('amend'):
             # Create or update Differential Revision
-            revision = createdifferentialrevision(ctx, revid, lastrevid,
-                                                  oldnode, olddiff, actions)
+            revision, diff = createdifferentialrevision(
+                ctx, revid, lastrevid, oldnode, olddiff, actions)
+            diffmap[ctx.node()] = diff
             newrevid = int(revision[r'object'][r'id'])
             if revid:
                 action = _('updated')
             else:
                 action = _('created')
 
-            # Create a local tag to note the association
-            tagname = 'D%d' % newrevid
-            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
-                     date=None, local=True)
+            # Create a local tag to note the association, if commit message
+            # does not have it already
+            m = _differentialrevisiondescre.search(ctx.description())
+            if not m or int(m.group(1)) != newrevid:
+                tagname = 'D%d' % newrevid
+                tags.tag(repo, tagname, ctx.node(), message=None, user=None,
+                         date=None, local=True)
         else:
             # Nothing changed. But still set "newrevid" so the next revision
             # could depend on this one.
             newrevid = revid
             action = _('skipped')
 
         ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
                                             ctx.description().split('\n')[0]))
+        drevids.append(newrevid)
         lastrevid = newrevid
 
+    # Update commit messages and remove tags
+    if opts.get('amend'):
+        unfi = repo.unfiltered()
+        drevs = callconduit(repo, 'differential.query', {'ids': drevids})
+        with repo.wlock(), repo.lock(), repo.transaction('phabsend'):
+            wnode = unfi['.'].node()
+            mapping = {} # {oldnode: [newnode]}
+            for i, rev in enumerate(revs):
+                old = unfi[rev]
+                drevid = drevids[i]
+                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
+                newdesc = getdescfromdrev(drev)
+                # Make sure commit message contain "Differential Revision"
+                if old.description() != newdesc:
+                    parents = [
+                        mapping.get(old.p1().node(), (old.p1(),))[0],
+                        mapping.get(old.p2().node(), (old.p2(),))[0],
+                    ]
+                    new = context.metadataonlyctx(
+                        repo, old, parents=parents, text=newdesc,
+                        user=old.user(), date=old.date(), extra=old.extra())
+                    newnode = new.commit()
+                    mapping[old.node()] = [newnode]
+                    # Update diff property
+                    writediffproperties(unfi[newnode], diffmap[old.node()])
+                # Remove local tags since it's no longer necessary
+                tagname = 'D%d' % drevid
+                if tagname in repo.tags():
+                    tags.tag(repo, tagname, nullid, message=None, user=None,
+                             date=None, local=True)
+            scmutil.cleanupnodes(repo, mapping, 'phabsend')
+            if wnode in mapping:
+                unfi.setparents(mapping[wnode][0])
+
 # Map from "hg:meta" keys to header understood by "hg import". The order is
 # consistent with "hg export" output.
 _metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'),



To: quark, #hg-reviewers, mitrandir
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Loading...