Patchwork D2108: infinitepush: drop the `--to` flag to push and use `-B` instead

login
register
mail settings
Submitter phabricator
Date Feb. 9, 2018, 11:47 a.m.
Message ID <differential-rev-PHID-DREV-m3hduhjmtitcnblc2a5v-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27510/
State Superseded
Headers show

Comments

phabricator - Feb. 9, 2018, 11:47 a.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The extension added a `--to` flag to specify the bookmark to which revs should
  be pushed. This patch deletes that flag and instead uses the `-B` flag. After
  this patch, bookmark passed as `-B` is parsed and if it matches the infinitepush
  bookmark pattern, we consider that push as infinitepush.
  
  This is still not the best of what we can do. Later patches in the series will
  drop the use of `-B` flag and will instead handle things at bookmark bundle2
  part. Plugging these logic to bookmark bundle2 part will also get rid of the
  scratchbranchparttype bundle2 part.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/infinitepush/__init__.py
  tests/test-infinitepush-bundlestore.t
  tests/test-infinitepush.t
  tests/test-issue4074.t

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 9, 2018, 12:03 p.m.
pulkit added a subscriber: durham.
pulkit added a comment.


  There is still some work required to make this extension better. I didn't want to grow my stack too long and hence send upto here. Few things which will be coming for sure:
  
  - removal of wrapping of `push` command on client side
  - using the bookmarks bundle2 part instead of scratchbranchparttype on both client and server side
  - drop the scratchbranchparttype bundle2 part
  - dropping the debugfillinfinitepushmetadata command defined in infinitepushcommands.py
  
  There are things which I am not sure whether to keep or not:
  
  - the --bundle-store flag to push command
  - functionality to pull from bundlestore using `hg pull`
  - functionality to pull changesets from bundlestore if a changeset is not found locally on `hg update`
  - logic around sql store
  - interaction with the hoisting functionality of remotenames extension which is also being moved to core
  
  Once  these things are figured out, then I will try to add support for old clients and the clients on which we have no control. Also I will like to get these reviews less painful as much possible, so if there is any another way which can make the review easy, I will be happy to follow.
  
  cc: @durham

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: durham, mercurial-devel
phabricator - Feb. 10, 2018, 3:15 a.m.
indygreg accepted this revision.
indygreg added a comment.
This revision is now accepted and ready to land.


  I'm find with you sending more parts to this series. Especially if they delete code: if they delete code then my review of the import will be a review of the final state of the code post deletions.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, durham, mercurial-devel
phabricator - Feb. 12, 2018, 9:43 p.m.
durham added a comment.


  > There are things which I am not sure whether to keep or not:
  > 
  > - the --bundle-store flag to push command
  
  This is useful for scripts or tools that want to upload a commit to the cloud without having to give it a name. For instance, you can use it to push a commit then send that commit hash to some build service which can checkout the commit without having to worry about a bookmark name.  But this could always be added back later, so it's probably fine to drop it if there's not an immediate need in Mozilla's use case.
  
  > - functionality to pull from bundlestore using hg pull
  
  Similar to the points above and below, this is useful for automation that already passes hashes around.  Not having to pass around bookmark names as well means it's easier for that automation to migrate to infinitepush.
  
  > - functionality to pull changesets from bundlestore if a changeset is not found locally on hg update
  
  This is a bit of magic that user's really like.  When combined with automatic backup pushes, it makes it feel like everyone is using the same repository.  I'd highly recommend keeping this just for the eventual PR of saying "I can just hg commit, and my friend can do hg checkout HASH"
  
  > - logic around sql store
  
  Without this, would the server always store data in the filesystem?  The sql store seems like an important bit of making this robust in enterprise usage.
  
  > - interaction with the hoisting functionality of remotenames extension which is also being moved to core
  
  I'm not familiar with how infinitepush plays into hoisting, but I just wanted to make sure users never have to type 'remote/' or 'default/'.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, durham, mercurial-devel
phabricator - Feb. 12, 2018, 9:57 p.m.
indygreg added a comment.


  In https://phab.mercurial-scm.org/D2108#36335, @durham wrote:
  
  > > There are things which I am not sure whether to keep or not:
  > > 
  > > - the --bundle-store flag to push command
  >
  > This is useful for scripts or tools that want to upload a commit to the cloud without having to give it a name. For instance, you can use it to push a commit then send that commit hash to some build service which can checkout the commit without having to worry about a bookmark name.  But this could always be added back later, so it's probably fine to drop it if there's not an immediate need in Mozilla's use case.
  
  
  To be clear, Mozilla has 2 use cases where infinitepush could be useful:
  
  1. For our Try repository. Upload a nameless bundle somewhere and CI consumes it.
  2. For user repositories (basically forks of the main repos).

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, durham, mercurial-devel
phabricator - Feb. 15, 2018, 10:18 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2108#35150, @indygreg wrote:
  
  > I'm find with you sending more parts to this series. Especially if they delete code: if they delete code then my review of the import will be a review of the final state of the code post deletions.
  
  
  With discussions on https://phab.mercurial-scm.org/D2096 and this patch, understanding your requirements, I think the wrapping of push command, the scratchbranchparttype bundle2 part etc. will be useful. So, can you review the current series with this patch as the last one?

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers, indygreg
Cc: indygreg, durham, mercurial-devel

Patch

diff --git a/tests/test-issue4074.t b/tests/test-issue4074.t
--- a/tests/test-issue4074.t
+++ b/tests/test-issue4074.t
@@ -26,4 +26,3 @@ 
 Time a check-in, should never take more than 10 seconds user time:
 
   $ hg ci --time -m1
-  time: real .* secs .user [0-9][.].* sys .* (re)
diff --git a/tests/test-infinitepush.t b/tests/test-infinitepush.t
--- a/tests/test-infinitepush.t
+++ b/tests/test-infinitepush.t
@@ -49,7 +49,7 @@ 
   $ hg ci -Am "scratchfirstpart"
   adding scratchfirstpart
   created new head
-  $ hg push -r . --to scratch/firstpart
+  $ hg push -r . -B scratch/firstpart
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 1 commit:
@@ -60,7 +60,7 @@ 
   $ hg ci -Am "scratchsecondpart"
   adding scratchsecondpart
   created new head
-  $ hg push -r . --to scratch/secondpart
+  $ hg push -r . -B scratch/secondpart
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 1 commit:
@@ -96,7 +96,7 @@ 
   $ hg log -r '.' -T '{node}\n' > ../testpullbycommithash1
   $ echo testpullbycommithash2 > testpullbycommithash2
   $ hg ci -Aqm "testpullbycommithash2"
-  $ hg push -r . --to scratch/mybranch -q
+  $ hg push -r . -B scratch/mybranch -q
 
 Create third client and pull by commit hash.
 Make sure testpullbycommithash2 has not fetched
@@ -144,7 +144,7 @@ 
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   $ echo scratchontopofpublic > scratchontopofpublic
   $ hg ci -Aqm "scratchontopofpublic"
-  $ hg push -r . --to scratch/scratchontopofpublic
+  $ hg push -r . -B scratch/scratchontopofpublic
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 1 commit:
@@ -300,7 +300,7 @@ 
   $ hg ci -Aqm "tofillmetadata"
   $ hg log -r . -T '{node}\n'
   d2b0410d4da084bc534b1d90df0de9eb21583496
-  $ hg push -r . --to scratch/fillmetadata/fill
+  $ hg push -r . -B scratch/fillmetadata/fill
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 6 commits:
diff --git a/tests/test-infinitepush-bundlestore.t b/tests/test-infinitepush-bundlestore.t
--- a/tests/test-infinitepush-bundlestore.t
+++ b/tests/test-infinitepush-bundlestore.t
@@ -26,7 +26,7 @@ 
   remote: adding file changes
   remote: added 1 changesets with 1 changes to 1 files
   $ mkcommit scratchcommit
-  $ hg push -r . --to scratch/mybranch
+  $ hg push -r . -B scratch/mybranch
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 1 commit:
@@ -119,7 +119,7 @@ 
   $ cd ../client2
   $ hg up -q scratch/mybranch
   $ mkcommit 'new scratch commit'
-  $ hg push -r . --to scratch/mybranch
+  $ hg push -r . -B scratch/mybranch
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 2 commits:
@@ -140,7 +140,7 @@ 
   scratch/mybranch 1de1d7d92f8965260391d0513fe8a8d5973d3042
 
 Push scratch bookmark with no new revs
-  $ hg push -r . --to scratch/anotherbranch
+  $ hg push -r . -B scratch/anotherbranch
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 2 commits:
@@ -216,7 +216,7 @@ 
 
 Test with pushrebase
   $ mkcommit scratchcommitwithpushrebase
-  $ hg push -r . --to scratch/mybranch
+  $ hg push -r . -B scratch/mybranch
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 4 commits:
@@ -237,7 +237,7 @@ 
 
 Change the order of pushrebase and infinitepush
   $ mkcommit scratchcommitwithpushrebase2
-  $ hg push -r . --to scratch/mybranch
+  $ hg push -r . -B scratch/mybranch
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 5 commits:
@@ -300,15 +300,15 @@ 
   $ scratchbookmarks
   scratch/anotherbranch 1de1d7d92f8965260391d0513fe8a8d5973d3042
   scratch/mybranch 6c10d49fe92751666c40263f96721b918170d3da
-  $ hg push -r . --to scratch/mybranch
+  $ hg push -r . -B scratch/mybranch
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: non-forward push
   remote: (use --non-forward-move to override)
   abort: push failed on remote
   [255]
 
-  $ hg push -r . --to scratch/mybranch --non-forward-move
+  $ hg push -r . -B scratch/mybranch --non-forward-move
   pushing to ssh://user@dummy/repo
   searching for changes
   remote: pushing 5 commits:
diff --git a/hgext/infinitepush/__init__.py b/hgext/infinitepush/__init__.py
--- a/hgext/infinitepush/__init__.py
+++ b/hgext/infinitepush/__init__.py
@@ -315,9 +315,6 @@ 
 
 def clientextsetup(ui):
     entry = extensions.wrapcommand(commands.table, 'push', _push)
-    # Don't add the 'to' arg if it already exists
-    if not any(a for a in entry[1] if a[1] == 'to'):
-        entry[1].append(('', 'to', '', _('push revs to this bookmark')))
 
     if not any(a for a in entry[1] if a[1] == 'non-forward-move'):
         entry[1].append(('', 'non-forward-move', None,
@@ -743,7 +740,13 @@ 
     return common, True, remoteheads
 
 def _push(orig, ui, repo, dest=None, *args, **opts):
-    bookmark = opts.get('to') or ''
+
+    bookmark = opts.get('bookmark')
+    # we only support pushing one infinitepush bookmark at once
+    if len(bookmark) == 1:
+        bookmark = bookmark[0]
+    else:
+        bookmark = ''
 
     oldphasemove = None
     overrides = {(experimental, configbookmark): bookmark}
@@ -759,6 +762,9 @@ 
         ui.setconfig(experimental, confignonforwardmove,
                      opts.get('non_forward_move'), '--non-forward-move')
         if scratchpush:
+            # this is an infinitepush, we don't want the bookmark to be applied
+            # rather that should be stored in the bundlestore
+            opts['bookmark'] = []
             ui.setconfig(experimental, configscratchpush, True)
             oldphasemove = extensions.wrapfunction(exchange,
                                                    '_localphasemove',