Patchwork patchbomb: check that targets exist at the publicurl

login
register
mail settings
Submitter Pierre-Yves David
Date Oct. 12, 2015, 4:11 p.m.
Message ID <004264d03eca64967c92.1444666295@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/10962/
State Superseded
Commit dca161728dc9a2df8de293a7df600606cdd1b208
Headers show

Comments

Pierre-Yves David - Oct. 12, 2015, 4:11 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1444626783 25200
#      Sun Oct 11 22:13:03 2015 -0700
# Node ID 004264d03eca64967c9288559cfc18be0bcab630
# Parent  9ca13d10881d7044b79d903ad64653f6541591f1
# EXP-Topic pb.publicurl
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 004264d03eca
patchbomb: check that targets exist at the publicurl

Advertising that the patch are available to be pulled requires that to be true.
So we check revision availability on the remote before sending any email.
Augie Fackler - Oct. 12, 2015, 5:14 p.m.
On Mon, Oct 12, 2015 at 09:11:35AM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1444626783 25200
> #      Sun Oct 11 22:13:03 2015 -0700
> # Node ID 004264d03eca64967c9288559cfc18be0bcab630
> # Parent  9ca13d10881d7044b79d903ad64653f6541591f1
> # EXP-Topic pb.publicurl
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 004264d03eca
> patchbomb: check that targets exist at the publicurl
>
> Advertising that the patch are available to be pulled requires that to be true.
> So we check revision availability on the remote before sending any email.
>
> diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py
> --- a/hgext/patchbomb.py
> +++ b/hgext/patchbomb.py
> @@ -525,10 +525,39 @@ def patchbomb(ui, repo, *revs, **opts):
>      if outgoing:
>          revs = _getoutgoing(repo, dest, revs)
>      if bundle:
>          opts['revs'] = [str(r) for r in revs]
>
> +    # check if revision exist on the public destination
> +    publicurl = repo.ui.config('patchbomb', 'publicurl')
> +    if publicurl is not None:
> +        repo.ui.debug('checking that revision exist in the public repo')
> +        try:
> +            publicpeer = hg.peer(repo, {}, publicurl)
> +        except error.RepoError:
> +            repo.ui.write_err(_('unable to access public repo: %s\n')
> +                              % publicurl)
> +            raise
> +        if not publicpeer.capable('known'):
> +            repo.ui.debug('skipping existence checks: public repo too old')
> +        else:
> +            out = [repo[r] for r in revs]
> +            known = publicpeer.known(h.node() for h in out)
> +            missing = []
> +            for idx, h in enumerate(out):
> +                if not known[idx]:
> +                    missing.append(h)
> +            if missing:
> +                if 1 < len(missing):
> +                    msg = _('public "%s" is missing %s and %i others')
> +                    msg %= (publicurl, missing[0], len(missing) - 1)
> +                else:
> +                    msg = _('public url %s is missing %s')
> +                    msg %= (publicurl, missing[0])
> +                hint = _('use "hg push %s -r %s"') % (publicurl, missing[0])

shouldn't the hint be something like ' '.join('-r ' + node for node in heads(missing))?

> +                raise error.Abort(msg, hint=hint)
> +
>      # start
>      if date:
>          start_time = util.parsedate(date)
>      else:
>          start_time = util.makedate()
> diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
> --- a/tests/test-patchbomb.t
> +++ b/tests/test-patchbomb.t
> @@ -2843,17 +2843,36 @@ single rev
>    +d
>
>  Test pull url header
>  =================================
>
> +basic version
> +
>    $ echo 'intro=auto' >> $HGRCPATH
> -  $ echo 'publicurl=http://example.com/myrepo/' >> $HGRCPATH
> +  $ echo "publicurl=$TESTTMP/t2" >> $HGRCPATH
>    $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10' | grep '^#'
> -  # HG changeset patch
> -  # User test
> -  # Date 5 0
> -  #      Thu Jan 01 00:00:05 1970 +0000
> -  # Branch test
> -  # Node ID 3b6f1ec9dde933a40a115a7990f8b320477231af
> -  # Parent  2f9fa9b998c5fe3ac2bd9a2b14bfcbeecbc7c268
> -  # Available At http://example.com/myrepo/
> -  #              hg pull http://example.com/myrepo/ -r 3b6f1ec9dde9
> +  abort: public url $TESTTMP/t2 is missing 3b6f1ec9dde9
> +  (use "hg push $TESTTMP/t2 -r 3b6f1ec9dde9")
> +  [1]
> +
> +remote missing
> +
> +  $ echo 'publicurl=$TESTTMP/missing' >> $HGRCPATH
> +  $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10'
> +  unable to access public repo: $TESTTMP/missing
> +  abort: repository $TESTTMP/missing not found!
> +  [255]
> +
> +node missing at remote
> +
> +  $ hg clone -r '9' . ../t3
> +  adding changesets
> +  adding manifests
> +  adding file changes
> +  added 3 changesets with 3 changes to 3 files
> +  updating to branch test
> +  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ echo 'publicurl=$TESTTMP/t3' >> $HGRCPATH
> +  $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10'
> +  abort: public url $TESTTMP/t3 is missing 3b6f1ec9dde9

Aborting seems heavy-handed here. For trivial patches I might want to
continue anyway, so maybe prompt?

> +  (use "hg push $TESTTMP/t3 -r 3b6f1ec9dde9")
> +  [255]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Oct. 12, 2015, 5:18 p.m.
On 10/12/2015 10:14 AM, Augie Fackler wrote:
> On Mon, Oct 12, 2015 at 09:11:35AM -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1444626783 25200
>> #      Sun Oct 11 22:13:03 2015 -0700
>> # Node ID 004264d03eca64967c9288559cfc18be0bcab630
>> # Parent  9ca13d10881d7044b79d903ad64653f6541591f1
>> # EXP-Topic pb.publicurl
>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 004264d03eca
>> patchbomb: check that targets exist at the publicurl
>>
>> Advertising that the patch are available to be pulled requires that to be true.
>> So we check revision availability on the remote before sending any email.
>>
>> diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py
>> --- a/hgext/patchbomb.py
>> +++ b/hgext/patchbomb.py
>> @@ -525,10 +525,39 @@ def patchbomb(ui, repo, *revs, **opts):
>>       if outgoing:
>>           revs = _getoutgoing(repo, dest, revs)
>>       if bundle:
>>           opts['revs'] = [str(r) for r in revs]
>>
>> +    # check if revision exist on the public destination
>> +    publicurl = repo.ui.config('patchbomb', 'publicurl')
>> +    if publicurl is not None:
>> +        repo.ui.debug('checking that revision exist in the public repo')
>> +        try:
>> +            publicpeer = hg.peer(repo, {}, publicurl)
>> +        except error.RepoError:
>> +            repo.ui.write_err(_('unable to access public repo: %s\n')
>> +                              % publicurl)
>> +            raise
>> +        if not publicpeer.capable('known'):
>> +            repo.ui.debug('skipping existence checks: public repo too old')
>> +        else:
>> +            out = [repo[r] for r in revs]
>> +            known = publicpeer.known(h.node() for h in out)
>> +            missing = []
>> +            for idx, h in enumerate(out):
>> +                if not known[idx]:
>> +                    missing.append(h)
>> +            if missing:
>> +                if 1 < len(missing):
>> +                    msg = _('public "%s" is missing %s and %i others')
>> +                    msg %= (publicurl, missing[0], len(missing) - 1)
>> +                else:
>> +                    msg = _('public url %s is missing %s')
>> +                    msg %= (publicurl, missing[0])
>> +                hint = _('use "hg push %s -r %s"') % (publicurl, missing[0])
>
> shouldn't the hint be something like ' '.join('-r ' + node for node in heads(missing))?

There might be multiple heads, but we should probably hint for pushing 
heads yes.

>> +                raise error.Abort(msg, hint=hint)
>> +
>>       # start
>>       if date:
>>           start_time = util.parsedate(date)
>>       else:
>>           start_time = util.makedate()
>> diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
>> --- a/tests/test-patchbomb.t
>> +++ b/tests/test-patchbomb.t
>> @@ -2843,17 +2843,36 @@ single rev
>>     +d
>>
>>   Test pull url header
>>   =================================
>>
>> +basic version
>> +
>>     $ echo 'intro=auto' >> $HGRCPATH
>> -  $ echo 'publicurl=http://example.com/myrepo/' >> $HGRCPATH
>> +  $ echo "publicurl=$TESTTMP/t2" >> $HGRCPATH
>>     $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10' | grep '^#'
>> -  # HG changeset patch
>> -  # User test
>> -  # Date 5 0
>> -  #      Thu Jan 01 00:00:05 1970 +0000
>> -  # Branch test
>> -  # Node ID 3b6f1ec9dde933a40a115a7990f8b320477231af
>> -  # Parent  2f9fa9b998c5fe3ac2bd9a2b14bfcbeecbc7c268
>> -  # Available At http://example.com/myrepo/
>> -  #              hg pull http://example.com/myrepo/ -r 3b6f1ec9dde9
>> +  abort: public url $TESTTMP/t2 is missing 3b6f1ec9dde9
>> +  (use "hg push $TESTTMP/t2 -r 3b6f1ec9dde9")
>> +  [1]
>> +
>> +remote missing
>> +
>> +  $ echo 'publicurl=$TESTTMP/missing' >> $HGRCPATH
>> +  $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10'
>> +  unable to access public repo: $TESTTMP/missing
>> +  abort: repository $TESTTMP/missing not found!
>> +  [255]
>> +
>> +node missing at remote
>> +
>> +  $ hg clone -r '9' . ../t3
>> +  adding changesets
>> +  adding manifests
>> +  adding file changes
>> +  added 3 changesets with 3 changes to 3 files
>> +  updating to branch test
>> +  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
>> +  $ echo 'publicurl=$TESTTMP/t3' >> $HGRCPATH
>> +  $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10'
>> +  abort: public url $TESTTMP/t3 is missing 3b6f1ec9dde9
>
> Aborting seems heavy-handed here. For trivial patches I might want to
> continue anyway, so maybe prompt?

Meh, broken pull url will be really annoying on the other end. The way 
to go is probably more to allow automatic push on patchbomb.

In the mean-time, this is still an experimental featureso I would says 
it is fine.
Augie Fackler - Oct. 12, 2015, 5:20 p.m.
On Mon, Oct 12, 2015 at 1:18 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 10/12/2015 10:14 AM, Augie Fackler wrote:
>>
>> On Mon, Oct 12, 2015 at 09:11:35AM -0700, Pierre-Yves David wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1444626783 25200
>>> #      Sun Oct 11 22:13:03 2015 -0700
>>> # Node ID 004264d03eca64967c9288559cfc18be0bcab630
>>> # Parent  9ca13d10881d7044b79d903ad64653f6541591f1
>>> # EXP-Topic pb.publicurl
>>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
>>> 004264d03eca
>>> patchbomb: check that targets exist at the publicurl
>>>
>>> Advertising that the patch are available to be pulled requires that to be
>>> true.
>>> So we check revision availability on the remote before sending any email.
>>>
>>> diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py
>>> --- a/hgext/patchbomb.py
>>> +++ b/hgext/patchbomb.py
>>> @@ -525,10 +525,39 @@ def patchbomb(ui, repo, *revs, **opts):
>>>       if outgoing:
>>>           revs = _getoutgoing(repo, dest, revs)
>>>       if bundle:
>>>           opts['revs'] = [str(r) for r in revs]
>>>
>>> +    # check if revision exist on the public destination
>>> +    publicurl = repo.ui.config('patchbomb', 'publicurl')
>>> +    if publicurl is not None:
>>> +        repo.ui.debug('checking that revision exist in the public repo')
>>> +        try:
>>> +            publicpeer = hg.peer(repo, {}, publicurl)
>>> +        except error.RepoError:
>>> +            repo.ui.write_err(_('unable to access public repo: %s\n')
>>> +                              % publicurl)
>>> +            raise
>>> +        if not publicpeer.capable('known'):
>>> +            repo.ui.debug('skipping existence checks: public repo too
>>> old')
>>> +        else:
>>> +            out = [repo[r] for r in revs]
>>> +            known = publicpeer.known(h.node() for h in out)
>>> +            missing = []
>>> +            for idx, h in enumerate(out):
>>> +                if not known[idx]:
>>> +                    missing.append(h)
>>> +            if missing:
>>> +                if 1 < len(missing):
>>> +                    msg = _('public "%s" is missing %s and %i others')
>>> +                    msg %= (publicurl, missing[0], len(missing) - 1)
>>> +                else:
>>> +                    msg = _('public url %s is missing %s')
>>> +                    msg %= (publicurl, missing[0])
>>> +                hint = _('use "hg push %s -r %s"') % (publicurl,
>>> missing[0])
>>
>>
>> shouldn't the hint be something like ' '.join('-r ' + node for node in
>> heads(missing))?
>
>
> There might be multiple heads, but we should probably hint for pushing heads
> yes.
>
>
>>> +                raise error.Abort(msg, hint=hint)
>>> +
>>>       # start
>>>       if date:
>>>           start_time = util.parsedate(date)
>>>       else:
>>>           start_time = util.makedate()
>>> diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
>>> --- a/tests/test-patchbomb.t
>>> +++ b/tests/test-patchbomb.t
>>> @@ -2843,17 +2843,36 @@ single rev
>>>     +d
>>>
>>>   Test pull url header
>>>   =================================
>>>
>>> +basic version
>>> +
>>>     $ echo 'intro=auto' >> $HGRCPATH
>>> -  $ echo 'publicurl=http://example.com/myrepo/' >> $HGRCPATH
>>> +  $ echo "publicurl=$TESTTMP/t2" >> $HGRCPATH
>>>     $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10' | grep
>>> '^#'
>>> -  # HG changeset patch
>>> -  # User test
>>> -  # Date 5 0
>>> -  #      Thu Jan 01 00:00:05 1970 +0000
>>> -  # Branch test
>>> -  # Node ID 3b6f1ec9dde933a40a115a7990f8b320477231af
>>> -  # Parent  2f9fa9b998c5fe3ac2bd9a2b14bfcbeecbc7c268
>>> -  # Available At http://example.com/myrepo/
>>> -  #              hg pull http://example.com/myrepo/ -r 3b6f1ec9dde9
>>> +  abort: public url $TESTTMP/t2 is missing 3b6f1ec9dde9
>>> +  (use "hg push $TESTTMP/t2 -r 3b6f1ec9dde9")
>>> +  [1]
>>> +
>>> +remote missing
>>> +
>>> +  $ echo 'publicurl=$TESTTMP/missing' >> $HGRCPATH
>>> +  $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10'
>>> +  unable to access public repo: $TESTTMP/missing
>>> +  abort: repository $TESTTMP/missing not found!
>>> +  [255]
>>> +
>>> +node missing at remote
>>> +
>>> +  $ hg clone -r '9' . ../t3
>>> +  adding changesets
>>> +  adding manifests
>>> +  adding file changes
>>> +  added 3 changesets with 3 changes to 3 files
>>> +  updating to branch test
>>> +  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
>>> +  $ echo 'publicurl=$TESTTMP/t3' >> $HGRCPATH
>>> +  $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10'
>>> +  abort: public url $TESTTMP/t3 is missing 3b6f1ec9dde9
>>
>>
>> Aborting seems heavy-handed here. For trivial patches I might want to
>> continue anyway, so maybe prompt?
>
>
> Meh, broken pull url will be really annoying on the other end. The way to go
> is probably more to allow automatic push on patchbomb.

My (unstated, I should have mentioned it) assumption would be that the
pull URL would be omitted if it was broken.

> In the mean-time, this is still an experimental featureso I would says it is
> fine.

Fix the hint for multiple heads and I'll take it.

>
> --
> Pierre-Yves David

Patch

diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py
--- a/hgext/patchbomb.py
+++ b/hgext/patchbomb.py
@@ -525,10 +525,39 @@  def patchbomb(ui, repo, *revs, **opts):
     if outgoing:
         revs = _getoutgoing(repo, dest, revs)
     if bundle:
         opts['revs'] = [str(r) for r in revs]
 
+    # check if revision exist on the public destination
+    publicurl = repo.ui.config('patchbomb', 'publicurl')
+    if publicurl is not None:
+        repo.ui.debug('checking that revision exist in the public repo')
+        try:
+            publicpeer = hg.peer(repo, {}, publicurl)
+        except error.RepoError:
+            repo.ui.write_err(_('unable to access public repo: %s\n')
+                              % publicurl)
+            raise
+        if not publicpeer.capable('known'):
+            repo.ui.debug('skipping existence checks: public repo too old')
+        else:
+            out = [repo[r] for r in revs]
+            known = publicpeer.known(h.node() for h in out)
+            missing = []
+            for idx, h in enumerate(out):
+                if not known[idx]:
+                    missing.append(h)
+            if missing:
+                if 1 < len(missing):
+                    msg = _('public "%s" is missing %s and %i others')
+                    msg %= (publicurl, missing[0], len(missing) - 1)
+                else:
+                    msg = _('public url %s is missing %s')
+                    msg %= (publicurl, missing[0])
+                hint = _('use "hg push %s -r %s"') % (publicurl, missing[0])
+                raise error.Abort(msg, hint=hint)
+
     # start
     if date:
         start_time = util.parsedate(date)
     else:
         start_time = util.makedate()
diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
--- a/tests/test-patchbomb.t
+++ b/tests/test-patchbomb.t
@@ -2843,17 +2843,36 @@  single rev
   +d
   
 Test pull url header
 =================================
 
+basic version
+
   $ echo 'intro=auto' >> $HGRCPATH
-  $ echo 'publicurl=http://example.com/myrepo/' >> $HGRCPATH
+  $ echo "publicurl=$TESTTMP/t2" >> $HGRCPATH
   $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10' | grep '^#'
-  # HG changeset patch
-  # User test
-  # Date 5 0
-  #      Thu Jan 01 00:00:05 1970 +0000
-  # Branch test
-  # Node ID 3b6f1ec9dde933a40a115a7990f8b320477231af
-  # Parent  2f9fa9b998c5fe3ac2bd9a2b14bfcbeecbc7c268
-  # Available At http://example.com/myrepo/
-  #              hg pull http://example.com/myrepo/ -r 3b6f1ec9dde9
+  abort: public url $TESTTMP/t2 is missing 3b6f1ec9dde9
+  (use "hg push $TESTTMP/t2 -r 3b6f1ec9dde9")
+  [1]
+
+remote missing
+
+  $ echo 'publicurl=$TESTTMP/missing' >> $HGRCPATH
+  $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10'
+  unable to access public repo: $TESTTMP/missing
+  abort: repository $TESTTMP/missing not found!
+  [255]
+
+node missing at remote
+
+  $ hg clone -r '9' . ../t3
+  adding changesets
+  adding manifests
+  adding file changes
+  added 3 changesets with 3 changes to 3 files
+  updating to branch test
+  3 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ echo 'publicurl=$TESTTMP/t3' >> $HGRCPATH
+  $ hg email --date '1980-1-1 0:1' -n -t foo -s test -r '10'
+  abort: public url $TESTTMP/t3 is missing 3b6f1ec9dde9
+  (use "hg push $TESTTMP/t3 -r 3b6f1ec9dde9")
+  [255]