Patchwork [remotenames] push: improve hgsvn compatibility

login
register
mail settings
Submitter Jun Wu
Date Sept. 14, 2016, 12:53 p.m.
Message ID <495a2dc683e529b8744f.1473857613@x1c>
Download mbox | patch
Permalink /patch/16621/
State Rejected
Delegated to: Ryan McElroy
Headers show

Comments

Jun Wu - Sept. 14, 2016, 12:53 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1473854964 -3600
#      Wed Sep 14 13:09:24 2016 +0100
# Node ID 495a2dc683e529b8744f422509fbca37b232263e
# Parent  7a6c5ff76f225c8ebe9baef9d5ef753da915aa8b
push: improve hgsvn compatibility

This patch improves the hgsubversion compatibility handling to resolve an
issue that "hg push" does not work with both hgsvn and remotenames enabled:

  File "remotenames.py", line 835, in expushcmd
    return orig(ui, repo, dest, opargs=opargs, **opts)
  ....
  File "hgsubversion/wrappers.py", line 383, in exchangepush
    pushop.cgresult = push(repo, remote, force, revs)
  File "hgsubversion/wrappers.py", line 185, in push
    assert not revs, 'designated revisions for push remains unimplemented.'
  AssertionError: designated revisions for push remains unimplemented.

and avoids some potential unwanted KeyError handling by removing the try
block.
Ryan McElroy - Sept. 14, 2016, 2:46 p.m.
On 9/14/2016 1:53 PM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1473854964 -3600
> #      Wed Sep 14 13:09:24 2016 +0100
> # Node ID 495a2dc683e529b8744f422509fbca37b232263e
> # Parent  7a6c5ff76f225c8ebe9baef9d5ef753da915aa8b
> push: improve hgsvn compatibility
>
> This patch improves the hgsubversion compatibility handling to resolve an
> issue that "hg push" does not work with both hgsvn and remotenames enabled:
>
>    File "remotenames.py", line 835, in expushcmd
>      return orig(ui, repo, dest, opargs=opargs, **opts)
>    ....
>    File "hgsubversion/wrappers.py", line 383, in exchangepush
>      pushop.cgresult = push(repo, remote, force, revs)
>    File "hgsubversion/wrappers.py", line 185, in push
>      assert not revs, 'designated revisions for push remains unimplemented.'
>    AssertionError: designated revisions for push remains unimplemented.
>
> and avoids some potential unwanted KeyError handling by removing the try
> block.
>
> diff --git a/remotenames.py b/remotenames.py
> --- a/remotenames.py
> +++ b/remotenames.py
> @@ -815,12 +815,9 @@ def expushcmd(orig, ui, repo, dest=None,
>           dest = 'default-push'
>   
> -    try:
> -        # hgsubversion and hggit do funcky things on push. Just call it
> -        # directly
> -        path = paths[dest]
> -        if path.startswith('svn+') or path.startswith('git+'):
> -            return orig(ui, repo, dest, opargs=opargs, **opts)
> -    except KeyError:
> -        pass
> +    # hgsubversion and hggit do funcky things on push. Just call it
> +    # directly
> +    path = paths.get(dest or 'default')
> +    if path and (path.startswith('svn+') or path.startswith('git+')):
> +        return orig(ui, repo, origdest, opargs=opargs, **opts)
>   
>       if not opargs['to']:

This patch leaves me unconvinced. The failure is because revs are being 
passed in, but this patch only changes the destination, which is not 
related to revs passed in.

Furthermore, there is no test here that failed previously. How do I know 
this fixes anything?

Just a minute ago, I pushed from a repo with hgsubversion and 
remotenames both enabled and it worked with no errors.

It seems that something else is going on here.
Augie Fackler - Sept. 14, 2016, 2:49 p.m.
> On Sep 14, 2016, at 08:53, Jun Wu <quark@fb.com> wrote:
> 
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1473854964 -3600
> #      Wed Sep 14 13:09:24 2016 +0100
> # Node ID 495a2dc683e529b8744f422509fbca37b232263e
> # Parent  7a6c5ff76f225c8ebe9baef9d5ef753da915aa8b
> push: improve hgsvn compatibility
> 
> This patch improves the hgsubversion compatibility handling to resolve an
> issue that "hg push" does not work with both hgsvn and remotenames enabled:
> 
>  File "remotenames.py", line 835, in expushcmd
>    return orig(ui, repo, dest, opargs=opargs, **opts)
>  ....
>  File "hgsubversion/wrappers.py", line 383, in exchangepush
>    pushop.cgresult = push(repo, remote, force, revs)
>  File "hgsubversion/wrappers.py", line 185, in push
>    assert not revs, 'designated revisions for push remains unimplemented.'
>  AssertionError: designated revisions for push remains unimplemented.
> 
> and avoids some potential unwanted KeyError handling by removing the try
> block.
> 
> diff --git a/remotenames.py b/remotenames.py
> --- a/remotenames.py
> +++ b/remotenames.py
> @@ -815,12 +815,9 @@ def expushcmd(orig, ui, repo, dest=None,
>         dest = 'default-push'
> 
> -    try:
> -        # hgsubversion and hggit do funcky things on push. Just call it
> -        # directly
> -        path = paths[dest]
> -        if path.startswith('svn+') or path.startswith('git+'):
> -            return orig(ui, repo, dest, opargs=opargs, **opts)

I can't remember what dest is here, but if it's a peer you might be able to get away with some capability checks for 'svn' instead of url-format-sniffing (which would help in the bare-https-url case). Also, you should also sniff for svn:// if you want to do url sniffing.

> -    except KeyError:
> -        pass
> +    # hgsubversion and hggit do funcky things on push. Just call it
> +    # directly
> +    path = paths.get(dest or 'default')
> +    if path and (path.startswith('svn+') or path.startswith('git+')):
> +        return orig(ui, repo, origdest, opargs=opargs, **opts)
> 
>     if not opargs['to']:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Ryan McElroy - Sept. 14, 2016, 3:19 p.m.
On 9/14/2016 3:49 PM, Augie Fackler wrote:
>> On Sep 14, 2016, at 08:53, Jun Wu <quark@fb.com> wrote:
>>
>> # HG changeset patch
>> # User Jun Wu <quark@fb.com>
>> # Date 1473854964 -3600
>> #      Wed Sep 14 13:09:24 2016 +0100
>> # Node ID 495a2dc683e529b8744f422509fbca37b232263e
>> # Parent  7a6c5ff76f225c8ebe9baef9d5ef753da915aa8b
>> push: improve hgsvn compatibility
>>
>> This patch improves the hgsubversion compatibility handling to resolve an
>> issue that "hg push" does not work with both hgsvn and remotenames enabled:
>>
>>   File "remotenames.py", line 835, in expushcmd
>>     return orig(ui, repo, dest, opargs=opargs, **opts)
>>   ....
>>   File "hgsubversion/wrappers.py", line 383, in exchangepush
>>     pushop.cgresult = push(repo, remote, force, revs)
>>   File "hgsubversion/wrappers.py", line 185, in push
>>     assert not revs, 'designated revisions for push remains unimplemented.'
>>   AssertionError: designated revisions for push remains unimplemented.
>>
>> and avoids some potential unwanted KeyError handling by removing the try
>> block.
>>
>> diff --git a/remotenames.py b/remotenames.py
>> --- a/remotenames.py
>> +++ b/remotenames.py
>> @@ -815,12 +815,9 @@ def expushcmd(orig, ui, repo, dest=None,
>>          dest = 'default-push'
>>
>> -    try:
>> -        # hgsubversion and hggit do funcky things on push. Just call it
>> -        # directly
>> -        path = paths[dest]
>> -        if path.startswith('svn+') or path.startswith('git+'):
>> -            return orig(ui, repo, dest, opargs=opargs, **opts)
> I can't remember what dest is here, but if it's a peer you might be able to get away with some capability checks for 'svn' instead of url-format-sniffing (which would help in the bare-https-url case). Also, you should also sniff for svn:// if you want to do url sniffing.

Capability checks sounds like network (correct me if I'm wrong), which 
we want to avoid at this stage probably. It would also be a much bigger 
change.

Instead, I think we'll use your suggestion to broaden the check to be 
more inclusive and thus more correct hopefully. I'll do that in a second 
patch.

>> -    except KeyError:
>> -        pass
>> +    # hgsubversion and hggit do funcky things on push. Just call it
>> +    # directly
>> +    path = paths.get(dest or 'default')
>> +    if path and (path.startswith('svn+') or path.startswith('git+')):
>> +        return orig(ui, repo, origdest, opargs=opargs, **opts)
>>
>>      if not opargs['to']:

Patch

diff --git a/remotenames.py b/remotenames.py
--- a/remotenames.py
+++ b/remotenames.py
@@ -815,12 +815,9 @@  def expushcmd(orig, ui, repo, dest=None,
         dest = 'default-push'
 
-    try:
-        # hgsubversion and hggit do funcky things on push. Just call it
-        # directly
-        path = paths[dest]
-        if path.startswith('svn+') or path.startswith('git+'):
-            return orig(ui, repo, dest, opargs=opargs, **opts)
-    except KeyError:
-        pass
+    # hgsubversion and hggit do funcky things on push. Just call it
+    # directly
+    path = paths.get(dest or 'default')
+    if path and (path.startswith('svn+') or path.startswith('git+')):
+        return orig(ui, repo, origdest, opargs=opargs, **opts)
 
     if not opargs['to']: