D229: phabricator: update diff property even if we choose not to create a new diff

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

D229: phabricator: update diff property even if we choose not to create a new diff

dsp (David Soria Parra)
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The diff property contains metadata like "HG Node". Previously we skip
  uploading a new diff if we are sure that the old patch and new patch have a
  same content. That has issues when a pusher adds an obsmarker using the old
  "HG Node" stored in the old diff.
 
  This patch adds logic to update the diff property so "HG Node" gets updated
  to prevent that issue.

REPOSITORY
  rHG Mercurial

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

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
@@ -255,7 +255,7 @@
     callconduit(ctx.repo(), 'differential.setdiffproperty', params)
 
 def createdifferentialrevision(ctx, revid=None, parentrevid=None, oldnode=None,
-                               actions=None):
+                               olddiff=None, actions=None):
     """create or update a Differential Revision
 
     If revid is None, create a new Differential Revision, otherwise update
@@ -279,6 +279,13 @@
         diff = creatediff(ctx)
         writediffproperties(ctx, diff)
         transactions.append({'type': 'update', 'value': diff[r'phid']})
+    else:
+        # Even if we don't need to upload a new diff because the patch content
+        # does not change. We might still need to update its metadata so
+        # pushers could know the correct node metadata.
+        assert olddiff
+        diff = olddiff
+    writediffproperties(ctx, diff)
 
     # Use a temporary summary to set dependency. There might be better ways but
     # I cannot find them for now. But do not do that if we are updating an
@@ -368,7 +375,7 @@
         if oldnode != ctx.node():
             # Create or update Differential Revision
             revision = createdifferentialrevision(ctx, revid, lastrevid,
-                                                  oldnode, actions)
+                                                  oldnode, olddiff, actions)
             newrevid = int(revision[r'object'][r'id'])
             if revid:
                 action = _('updated')



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

D229: phabricator: update diff property even if we choose not to create a new diff

dsp (David Soria Parra)
This revision was automatically updated to reflect the committed changes.
Closed by commit rHGf100354cce52: phabricator: update diff property even if we choose not to create a new diff (authored by quark).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D229?vs=552&id=566

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

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
@@ -255,7 +255,7 @@
     callconduit(ctx.repo(), 'differential.setdiffproperty', params)
 
 def createdifferentialrevision(ctx, revid=None, parentrevid=None, oldnode=None,
-                               actions=None):
+                               olddiff=None, actions=None):
     """create or update a Differential Revision
 
     If revid is None, create a new Differential Revision, otherwise update
@@ -279,6 +279,13 @@
         diff = creatediff(ctx)
         writediffproperties(ctx, diff)
         transactions.append({'type': 'update', 'value': diff[r'phid']})
+    else:
+        # Even if we don't need to upload a new diff because the patch content
+        # does not change. We might still need to update its metadata so
+        # pushers could know the correct node metadata.
+        assert olddiff
+        diff = olddiff
+    writediffproperties(ctx, diff)
 
     # Use a temporary summary to set dependency. There might be better ways but
     # I cannot find them for now. But do not do that if we are updating an
@@ -383,7 +390,7 @@
         if oldnode != ctx.node():
             # Create or update Differential Revision
             revision = createdifferentialrevision(ctx, revid, lastrevid,
-                                                  oldnode, actions)
+                                                  oldnode, olddiff, actions)
             newrevid = int(revision[r'object'][r'id'])
             if revid:
                 action = _('updated')



To: quark, #hg-reviewers, durin42
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

D229: phabricator: update diff property even if we choose not to create a new diff

dsp (David Soria Parra)
In reply to this post by dsp (David Soria Parra)
yuja added inline comments.

INLINE COMMENTS

> phabricator.py:280
>          diff = creatediff(ctx)
>          writediffproperties(ctx, diff)
>          transactions.append({'type': 'update', 'value': diff[r'phid']})

`writediffproperties()` is called twice. Perhaps one of them should be deleted.

REPOSITORY
  rHG Mercurial

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

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