threads / patch / 33813

v3, 10 partsremote-hg: fixes and cleanups

Subject: [PATCH v3 00/10] remote-hg: fixes and cleanups

## tl;dr

41 messages between May 13, 2013 and May 15, 2013. Diffs are folded; open one to read it.

replies: 40people: 2as markdown or json

Felipe Contreras· May 13, 2013, 23:11 UTC · lore
Hi,
Since the last series is not merged to master yet, I decided to add more cleanups.
Felipe Contreras (10):
  remote-hg: trivial cleanups
  remote-hg: get rid of unused exception checks
  remote-hg: enable track-branches in hg-git mode
  remote-hg: add new get_config_bool() helper
  remote-hg: fix new branch creation
  remote-hg: disable forced push by default
  remote-hg: don't push fake 'master' bookmark
  remote-hg: update bookmarks when pulling
  remote-hg: test: be a little more quiet
  remote-hg: trivial reorganization
 contrib/remote-helpers/git-remote-hg     | 47 ++++++++++++++++----------------
 contrib/remote-helpers/test-hg-hg-git.sh |  3 +-
 contrib/remote-helpers/test-hg.sh        |  4 +--
 3 files changed, 26 insertions(+), 28 deletions(-)
-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 01/10] remote-hg: trivial cleanups

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg     | 2 +-
 contrib/remote-helpers/test-hg-hg-git.sh | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)
Show changes to 2 files +1 −3

contrib/remote-helpers/git-remote-hg, contrib/remote-helpers/test-hg-hg-git.sh

diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 96ad30d..d33c7ba 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -538,7 +538,7 @@ def list_head(repo, cur):
     g_head = (head, node)
 
 def do_list(parser):
-    global branches, bmarks, mode, track_branches
+    global branches, bmarks, track_branches
 
     repo = parser.repo
     for bmark, node in bookmarks.listbookmarks(repo).iteritems():
diff --git a/contrib/remote-helpers/test-hg-hg-git.sh b/contrib/remote-helpers/test-hg-hg-git.sh
index 8440341..0c36573 100755
--- a/contrib/remote-helpers/test-hg-hg-git.sh
+++ b/contrib/remote-helpers/test-hg-hg-git.sh
@@ -455,8 +455,6 @@ test_expect_success 'hg author' '
 		git_log gitrepo-$x > git-log-$x
 	done &&
 
-	test_cmp git-log-hg git-log-git &&
-
 	test_cmp hg-log-hg hg-log-git &&
 	test_cmp git-log-hg git-log-git
 '

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:12 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 01/10] remote-hg: trivial cleanups

This is a trivial cleanup, cannot cause regressions.
Felipe Contreras wrote:
Show 34 quoted lines
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg     | 2 +-
>  contrib/remote-helpers/test-hg-hg-git.sh | 2 --
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 96ad30d..d33c7ba 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -538,7 +538,7 @@ def list_head(repo, cur):
>      g_head = (head, node)
>  
>  def do_list(parser):
> -    global branches, bmarks, mode, track_branches
> +    global branches, bmarks, track_branches
>  
>      repo = parser.repo
>      for bmark, node in bookmarks.listbookmarks(repo).iteritems():
> diff --git a/contrib/remote-helpers/test-hg-hg-git.sh b/contrib/remote-helpers/test-hg-hg-git.sh
> index 8440341..0c36573 100755
> --- a/contrib/remote-helpers/test-hg-hg-git.sh
> +++ b/contrib/remote-helpers/test-hg-hg-git.sh
> @@ -455,8 +455,6 @@ test_expect_success 'hg author' '
>  		git_log gitrepo-$x > git-log-$x
>  	done &&
>  
> -	test_cmp git-log-hg git-log-git &&
> -
>  	test_cmp hg-log-hg hg-log-git &&
>  	test_cmp git-log-hg git-log-git
>  '
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 02/10] remote-hg: get rid of unused exception checks

We are not calling check_output() anymore.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)
Show changes to contrib/remote-helpers/git-remote-hg +9 −15
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index d33c7ba..9d6940b 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -327,11 +327,8 @@ def get_repo(url, alias):
     myui.setconfig('ui', 'interactive', 'off')
     myui.fout = sys.stderr
 
-    try:
-        if get_config('remote-hg.insecure') == 'true\n':
-            myui.setconfig('web', 'cacerts', '')
-    except subprocess.CalledProcessError:
-        pass
+    if get_config('remote-hg.insecure') == 'true\n':
+        myui.setconfig('web', 'cacerts', '')
 
     try:
         mod = extensions.load(myui, 'hgext.schemes', None)
@@ -910,16 +907,13 @@ def main(args):
     track_branches = True
     force_push = True
 
-    try:
-        if get_config('remote-hg.hg-git-compat') == 'true\n':
-            hg_git_compat = True
-            track_branches = False
-        if get_config('remote-hg.track-branches') == 'false\n':
-            track_branches = False
-        if get_config('remote-hg.force-push') == 'false\n':
-            force_push = False
-    except subprocess.CalledProcessError:
-        pass
+    if get_config('remote-hg.hg-git-compat') == 'true\n':
+        hg_git_compat = True
+        track_branches = False
+    if get_config('remote-hg.track-branches') == 'false\n':
+        track_branches = False
+    if get_config('remote-hg.force-push') == 'false\n':
+        force_push = False
 
     if hg_git_compat:
         mode = 'hg'

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:16 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 02/10] remote-hg: get rid of unused exception checks

This is removing an exception check and since that exception is thrown by check_output() but not Popen(), this doesn't change anything.

Felipe Contreras wrote:
Show 51 quoted lines
> We are not calling check_output() anymore.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index d33c7ba..9d6940b 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -327,11 +327,8 @@ def get_repo(url, alias):
>      myui.setconfig('ui', 'interactive', 'off')
>      myui.fout = sys.stderr
>  
> -    try:
> -        if get_config('remote-hg.insecure') == 'true\n':
> -            myui.setconfig('web', 'cacerts', '')
> -    except subprocess.CalledProcessError:
> -        pass
> +    if get_config('remote-hg.insecure') == 'true\n':
> +        myui.setconfig('web', 'cacerts', '')
>  
>      try:
>          mod = extensions.load(myui, 'hgext.schemes', None)
> @@ -910,16 +907,13 @@ def main(args):
>      track_branches = True
>      force_push = True
>  
> -    try:
> -        if get_config('remote-hg.hg-git-compat') == 'true\n':
> -            hg_git_compat = True
> -            track_branches = False
> -        if get_config('remote-hg.track-branches') == 'false\n':
> -            track_branches = False
> -        if get_config('remote-hg.force-push') == 'false\n':
> -            force_push = False
> -    except subprocess.CalledProcessError:
> -        pass
> +    if get_config('remote-hg.hg-git-compat') == 'true\n':
> +        hg_git_compat = True
> +        track_branches = False
> +    if get_config('remote-hg.track-branches') == 'false\n':
> +        track_branches = False
> +    if get_config('remote-hg.force-push') == 'false\n':
> +        force_push = False
>  
>      if hg_git_compat:
>          mode = 'hg'
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 03/10] remote-hg: enable track-branches in hg-git mode

The user can turn this off.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg     | 1 -
 contrib/remote-helpers/test-hg-hg-git.sh | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)
Show changes to 2 files +1 −1

contrib/remote-helpers/git-remote-hg, contrib/remote-helpers/test-hg-hg-git.sh

diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 9d6940b..de3a96e 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -909,7 +909,6 @@ def main(args):
 
     if get_config('remote-hg.hg-git-compat') == 'true\n':
         hg_git_compat = True
-        track_branches = False
     if get_config('remote-hg.track-branches') == 'false\n':
         track_branches = False
     if get_config('remote-hg.force-push') == 'false\n':
diff --git a/contrib/remote-helpers/test-hg-hg-git.sh b/contrib/remote-helpers/test-hg-hg-git.sh
index 0c36573..7f579c8 100755
--- a/contrib/remote-helpers/test-hg-hg-git.sh
+++ b/contrib/remote-helpers/test-hg-hg-git.sh
@@ -102,6 +102,7 @@ setup () {
 	) >> "$HOME"/.hgrc &&
 	git config --global receive.denycurrentbranch warn
 	git config --global remote-hg.hg-git-compat true
+	git config --global remote-hg.track-branches false
 
 	HGEDITOR=/usr/bin/true
 

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:17 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 03/10] remote-hg: enable track-branches in hg-git mode

No regression here either, we simply give more power to the user.
Felipe Contreras wrote:
Show 34 quoted lines
> The user can turn this off.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg     | 1 -
>  contrib/remote-helpers/test-hg-hg-git.sh | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 9d6940b..de3a96e 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -909,7 +909,6 @@ def main(args):
>  
>      if get_config('remote-hg.hg-git-compat') == 'true\n':
>          hg_git_compat = True
> -        track_branches = False
>      if get_config('remote-hg.track-branches') == 'false\n':
>          track_branches = False
>      if get_config('remote-hg.force-push') == 'false\n':
> diff --git a/contrib/remote-helpers/test-hg-hg-git.sh b/contrib/remote-helpers/test-hg-hg-git.sh
> index 0c36573..7f579c8 100755
> --- a/contrib/remote-helpers/test-hg-hg-git.sh
> +++ b/contrib/remote-helpers/test-hg-hg-git.sh
> @@ -102,6 +102,7 @@ setup () {
>  	) >> "$HOME"/.hgrc &&
>  	git config --global receive.denycurrentbranch warn
>  	git config --global remote-hg.hg-git-compat true
> +	git config --global remote-hg.track-branches false
>  
>  	HGEDITOR=/usr/bin/true
>  
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 04/10] remote-hg: add new get_config_bool() helper

No functional changes.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)
Show changes to contrib/remote-helpers/git-remote-hg +13 −11
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index de3a96e..4a5c72f 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -87,6 +87,15 @@ def get_config(config):
     output, _ = process.communicate()
     return output
 
+def get_config_bool(config, default=False):
+    value = get_config(config).rstrip('\n')
+    if value == "true":
+        return True
+    elif value == "false":
+        return False
+    else:
+        return default
+
 class Marks:
 
     def __init__(self, path):
@@ -327,7 +336,7 @@ def get_repo(url, alias):
     myui.setconfig('ui', 'interactive', 'off')
     myui.fout = sys.stderr
 
-    if get_config('remote-hg.insecure') == 'true\n':
+    if get_config_bool('remote-hg.insecure'):
         myui.setconfig('web', 'cacerts', '')
 
     try:
@@ -903,16 +912,9 @@ def main(args):
     url = args[2]
     peer = None
 
-    hg_git_compat = False
-    track_branches = True
-    force_push = True
-
-    if get_config('remote-hg.hg-git-compat') == 'true\n':
-        hg_git_compat = True
-    if get_config('remote-hg.track-branches') == 'false\n':
-        track_branches = False
-    if get_config('remote-hg.force-push') == 'false\n':
-        force_push = False
+    hg_git_compat = get_config_bool('remote-hg.hg-git-compat')
+    track_branches = get_config_bool('remote-hg.track-branches', True)
+    force_push = get_config_bool('remote-hg.force-push', True)
 
     if hg_git_compat:
         mode = 'hg'

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:18 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 04/10] remote-hg: add new get_config_bool() helper

This is simply refactoring code, functionally they are the same.
Felipe Contreras wrote:
Show 58 quoted lines
> No functional changes.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index de3a96e..4a5c72f 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -87,6 +87,15 @@ def get_config(config):
>      output, _ = process.communicate()
>      return output
>  
> +def get_config_bool(config, default=False):
> +    value = get_config(config).rstrip('\n')
> +    if value == "true":
> +        return True
> +    elif value == "false":
> +        return False
> +    else:
> +        return default
> +
>  class Marks:
>  
>      def __init__(self, path):
> @@ -327,7 +336,7 @@ def get_repo(url, alias):
>      myui.setconfig('ui', 'interactive', 'off')
>      myui.fout = sys.stderr
>  
> -    if get_config('remote-hg.insecure') == 'true\n':
> +    if get_config_bool('remote-hg.insecure'):
>          myui.setconfig('web', 'cacerts', '')
>  
>      try:
> @@ -903,16 +912,9 @@ def main(args):
>      url = args[2]
>      peer = None
>  
> -    hg_git_compat = False
> -    track_branches = True
> -    force_push = True
> -
> -    if get_config('remote-hg.hg-git-compat') == 'true\n':
> -        hg_git_compat = True
> -    if get_config('remote-hg.track-branches') == 'false\n':
> -        track_branches = False
> -    if get_config('remote-hg.force-push') == 'false\n':
> -        force_push = False
> +    hg_git_compat = get_config_bool('remote-hg.hg-git-compat')
> +    track_branches = get_config_bool('remote-hg.track-branches', True)
> +    force_push = get_config_bool('remote-hg.force-push', True)
>  
>      if hg_git_compat:
>          mode = 'hg'
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 05/10] remote-hg: fix new branch creation

When force_push is disabled, we need to turn the argument to True.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/remote-helpers/git-remote-hg +1 −1
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 4a5c72f..3cf9b4c 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -856,7 +856,7 @@ def do_export(parser):
             continue
 
     if peer:
-        parser.repo.push(peer, force=force_push)
+        parser.repo.push(peer, force=force_push, newbranch=True)
 
     # handle bookmarks
     for bmark, node in p_bmarks:

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:19 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 05/10] remote-hg: fix new branch creation

This is the first fix, but it's obvious this is what we want: if a user creates a new branch with git:

 % git checkout -b branches/devel
And then pushes this branch
 % git push origin branches/devel
(which is the way to push new mercurial branches)

We obviously want to create a branch, but the command would fail, and the fix is simple: tell the push that we might create new branches.

This only matters when foce_push=False.

Can't possibly introduce regressions, unless you think of the ability to push new branches as a regression.

Felipe Contreras wrote:
Show 22 quoted lines
> When force_push is disabled, we need to turn the argument to True.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 4a5c72f..3cf9b4c 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -856,7 +856,7 @@ def do_export(parser):
>              continue
>  
>      if peer:
> -        parser.repo.push(peer, force=force_push)
> +        parser.repo.push(peer, force=force_push, newbranch=True)
>  
>      # handle bookmarks
>      for bmark, node in p_bmarks:
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Junio C Hamano· May 15, 2013, 19:40 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 05/10] remote-hg: fix new branch creation

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Felipe Contreras wrote:
>> When force_push is disabled, we need to turn the argument to True.

With your follow-up clarification, here is what ended up in the log message:

    remote-hg: fix new branch creation
    When a user creates a new branch with git:
      $ git checkout -b branches/devel
    and then pushes this branch
      $ git push origin branches/devel
    which is the way to push new mercurial branches, we do want to
    create a branch, but the command would fail without newbranch=True.
    This only matters when force_push=False, but setting newbranch=True
    unconditionally does not hurt.

The only part that I came up with on my own is "but ... does not hurt" at the end. If that is incorrect, please supply an update.

Thanks.
Show 20 quoted lines
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  contrib/remote-helpers/git-remote-hg | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
>> index 4a5c72f..3cf9b4c 100755
>> --- a/contrib/remote-helpers/git-remote-hg
>> +++ b/contrib/remote-helpers/git-remote-hg
>> @@ -856,7 +856,7 @@ def do_export(parser):
>>              continue
>>  
>>      if peer:
>> -        parser.repo.push(peer, force=force_push)
>> +        parser.repo.push(peer, force=force_push, newbranch=True)
>>  
>>      # handle bookmarks
>>      for bmark, node in p_bmarks:
>> -- 
>> 1.8.3.rc1.579.g184e698
Felipe Contreras· May 15, 2013, 19:45 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 05/10] remote-hg: fix new branch creation

On Wed, May 15, 2013 at 2:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Felipe Contreras wrote:
>>> When force_push is disabled, we need to turn the argument to True.
>
> With your follow-up clarification, here is what ended up in the log
> message:
>
>     remote-hg: fix new branch creation
>
>     When a user creates a new branch with git:
>
>       $ git checkout -b branches/devel
>
>     and then pushes this branch
>
>       $ git push origin branches/devel
>
>     which is the way to push new mercurial branches,

I don't like this part. This is not documentation, this is a commit message. You don't explain how git works in every commit message. It's not relevant how to create Mercurial branches, it could be done through a totally different way and it wouldn't affect this patch. The only thing that is relevant is that a new Mercurial branch is created somehow.

But since you never, *ever*, agree that a piece of information in the commit message is not useful, I realize this is wasted breath.

Show 8 quoted lines
>     we do want to
>     create a branch, but the command would fail without newbranch=True.
>
>     This only matters when force_push=False, but setting newbranch=True
>     unconditionally does not hurt.
>
> The only part that I came up with on my own is "but ... does not
> hurt" at the end.  If that is incorrect, please supply an update.
It's correct.
-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 06/10] remote-hg: disable forced push by default

In certain situations we might end up pushing garbage revisions (e.g. in a rebase), and the patches to deal with that haven't been merged yet.

So let's disable forced pushes by default.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/remote-helpers/git-remote-hg +1 −1
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 3cf9b4c..53412dd 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -914,7 +914,7 @@ def main(args):
 
     hg_git_compat = get_config_bool('remote-hg.hg-git-compat')
     track_branches = get_config_bool('remote-hg.track-branches', True)
-    force_push = get_config_bool('remote-hg.force-push', True)
+    force_push = get_config_bool('remote-hg.force-push')
 
     if hg_git_compat:
         mode = 'hg'

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:23 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 06/10] remote-hg: disable forced push by default

And here is the important fix. We are essentially reverting back to the old v1.8.2 behavior, to minimize the possibility of regressions, but in a way the user can configure.

The cleanups before made it so this patch eas easy and simple.

And the fix before this makes it so the new default force_push=False still is able to push new branches, so we don't disable other v1.8.3 features.

Felipe Contreras wrote:
Show 25 quoted lines
> In certain situations we might end up pushing garbage revisions (e.g. in
> a rebase), and the patches to deal with that haven't been merged yet.
> 
> So let's disable forced pushes by default.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 3cf9b4c..53412dd 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -914,7 +914,7 @@ def main(args):
>  
>      hg_git_compat = get_config_bool('remote-hg.hg-git-compat')
>      track_branches = get_config_bool('remote-hg.track-branches', True)
> -    force_push = get_config_bool('remote-hg.force-push', True)
> +    force_push = get_config_bool('remote-hg.force-push')
>  
>      if hg_git_compat:
>          mode = 'hg'
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 07/10] remote-hg: don't push fake 'master' bookmark

We skip it locally, but not for the remote, so let's do so.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to contrib/remote-helpers/git-remote-hg +2 −1
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 53412dd..beb864b 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -873,7 +873,8 @@ def do_export(parser):
 
         if bmark == 'master' and 'master' not in parser.repo._bookmarks:
             # fake bookmark
-            pass
+            print "ok %s" % ref
+            continue
         elif bookmarks.pushbookmark(parser.repo, bmark, old, new):
             # updated locally
             pass

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:25 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 07/10] remote-hg: don't push fake 'master' bookmark

We obviously don't want to push our fake 'master' bookmark to the remote. This is an obvious good change.

Felipe Contreras wrote:
Show 23 quoted lines
> We skip it locally, but not for the remote, so let's do so.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 53412dd..beb864b 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -873,7 +873,8 @@ def do_export(parser):
>  
>          if bmark == 'master' and 'master' not in parser.repo._bookmarks:
>              # fake bookmark
> -            pass
> +            print "ok %s" % ref
> +            continue
>          elif bookmarks.pushbookmark(parser.repo, bmark, old, new):
>              # updated locally
>              pass
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 08/10] remote-hg: update bookmarks when pulling

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 3 +++
 1 file changed, 3 insertions(+)
Show changes to contrib/remote-helpers/git-remote-hg +3 −0
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index beb864b..dc276af 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -363,6 +363,9 @@ def get_repo(url, alias):
                 die('Repository error')
             repo.pull(peer, heads=None, force=True)
 
+        rb = peer.listkeys('bookmarks')
+        bookmarks.updatefromremote(myui, repo, rb, url)
+
     return repo
 
 def rev_to_mark(rev):

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:26 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

Without this fix, the user would never ever see new bookmarks, only the ones that (s)he initially cloned.

Felipe Contreras wrote:
Show 21 quoted lines
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index beb864b..dc276af 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -363,6 +363,9 @@ def get_repo(url, alias):
>                  die('Repository error')
>              repo.pull(peer, heads=None, force=True)
>  
> +        rb = peer.listkeys('bookmarks')
> +        bookmarks.updatefromremote(myui, repo, rb, url)
> +
>      return repo
>  
>  def rev_to_mark(rev):
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Junio C Hamano· May 14, 2013, 21:59 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Without this fix, the user would never ever see new bookmarks, only the ones
> that (s)he initially cloned.

Now, think again and realize how long it took you (the original author) to discover issues and come up with these fixes and explanation since the series was merged before -rc0.

Are we giving the users enough time to discover and complain issues that these 10 patches may introduce before the final release? "You can see these patches are so trivially correct" is not a valid argument. The original patches would also have been looked correct when they were sent to the list. Things take time and actual use by the users to mature.

Having said that, the impression I am getting is that whatever we pushed out in 1.8.2 and 1.8.3-rc0 was far from ready for real use, and with your explanations (by the way, I found that many of them deserve to be in the log message), the end result of applying these patches, up to 8/10, will still not be as it is very likely that you and users will discover issues at a similar rate, but at least it will be much closer to be ready than they currently are.

In other words, it still seems to be in "something is better than nothing, newer is better than older" stage before stabilization.

And that is to be expected for a contrib/ material; nothing for us to be ashamed of. So I changed my mind. As long as it is clearly marked as "still experimental, possibly with rough edges", I think it is better to ship 1.8.3 with these 8 patches than without.

I am unhappy with 3/10, though. It is spreading existing mistake by adding another configuration variable with dash in its name, which goes against the recommended practice, and making it more cumbersome to eventually fix them, because we would need to break end user's configuration.

Things like 1/10 and 2/10 that can be characterized as:
> This is a trivial cleanup, cannot cause regressions.

may probably be a good clean-up to build the next development cycle on top, but are not at all urgent for it to deserve to be included in the upcoming release. But it seems that 3-8 textually depend on at least 2, so I'll queue the first eight for 1.8.3 and exclude the rest for now. If these are identical to the early part of the 47-patch series (I didn't check; they are for the next cycle), it would make the next cycle shorter by 8 patches.

Felipe Contreras· May 14, 2013, 22:14 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

On Tue, May 14, 2013 at 4:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Without this fix, the user would never ever see new bookmarks, only the ones
>> that (s)he initially cloned.
>
> Now, think again and realize how long it took you (the original
> author) to discover issues and come up with these fixes and
> explanation since the series was merged before -rc0.
This issue has *always* been there, it's not a regression.
> Are we giving the users enough time to discover and complain issues
> that these 10 patches may introduce before the final release?
Yes, because the time needed is *zero*.
Show 5 quoted lines
> "You
> can see these patches are so trivially correct" is not a valid
> argument. The original patches would also have been looked correct
> when they were sent to the list. Things take time and actual use by
> the users to mature.

There was no original patches that introduced this regression, because it's not a regression.

When I say these are trivially correct, I mean it; the regressions you talk about were on patches that were marked as *not* trivially correct, and potentially dangerous.

Show 7 quoted lines
> Having said that, the impression I am getting is that whatever we
> pushed out in 1.8.2 and 1.8.3-rc0 was far from ready for real use,
> and with your explanations (by the way, I found that many of them
> deserve to be in the log message), the end result of applying these
> patches, up to 8/10, will still not be as it is very likely that you
> and users will discover issues at a similar rate, but at least it
> will be much closer to be ready than they currently are.
Define "ready". It's probably more "ready" than any other bridge tool out there.
> In other words, it still seems to be in "something is better than
> nothing, newer is better than older" stage before stabilization.

remote-hg is already stable, this patch has nothing to do with stabilization, neither do any of the 47 patches I sent to the list.

Show 10 quoted lines
> And that is to be expected for a contrib/ material; nothing for us
> to be ashamed of.  So I changed my mind.  As long as it is clearly
> marked as "still experimental, possibly with rough edges", I think
> it is better to ship 1.8.3 with these 8 patches than without.
>
> I am unhappy with 3/10, though.  It is spreading existing mistake by
> adding another configuration variable with dash in its name, which
> goes against the recommended practice, and making it more cumbersome
> to eventually fix them, because we would need to break end user's
> configuration.

It's not adding any configuration; remote-hg.track-branches is already present, even in v1.8.2.

Show 11 quoted lines
> Things like 1/10 and 2/10 that can be characterized as:
>
>> This is a trivial cleanup, cannot cause regressions.
>
> may probably be a good clean-up to build the next development cycle
> on top, but are not at all urgent for it to deserve to be included
> in the upcoming release.  But it seems that 3-8 textually depend on
> at least 2, so I'll queue the first eight for 1.8.3 and exclude the
> rest for now.  If these are identical to the early part of the
> 47-patch series (I didn't check; they are for the next cycle), it
> would make the next cycle shorter by 8 patches.

Indeed, they are exactly the same as the first 10 patches of the 47-patch series.

I think this makes sense.
-- 
Felipe Contreras
Junio C Hamano· May 14, 2013, 22:25 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 11 quoted lines
> On Tue, May 14, 2013 at 4:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Without this fix, the user would never ever see new bookmarks, only the ones
>>> that (s)he initially cloned.
>>
>> Now, think again and realize how long it took you (the original
>> author) to discover issues and come up with these fixes and
>> explanation since the series was merged before -rc0.
>
> This issue has *always* been there, it's not a regression.
Then why are you rushing it?  -rc is a "regression-fix-only" period.
> Define "ready". It's probably more "ready" than any other bridge
> tool out there.

Anything that needs "oh, we need to push these ten patches to avoid regressions at the last minute" is not mature enough to be relied upon by end users for everyday use. That is what I meant.

But now you are saying these are not regression fixes, in which case they have to wait because the users have known about the limitations that existed for a long time and learned to live with them. That is a sign of mature (not "not ready") software.

You cannot be both.  Which is it?
Felipe Contreras· May 14, 2013, 22:39 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

On Tue, May 14, 2013 at 5:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Tue, May 14, 2013 at 4:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> Without this fix, the user would never ever see new bookmarks, only the ones
>>>> that (s)he initially cloned.
>>>
>>> Now, think again and realize how long it took you (the original
>>> author) to discover issues and come up with these fixes and
>>> explanation since the series was merged before -rc0.
>>
>> This issue has *always* been there, it's not a regression.
>
> Then why are you rushing it?  -rc is a "regression-fix-only" period.

I'm not rushing this patch, I'm rushing the regression fix. I added the cleanup patches because they help the fix, and I added this patch because it's obviously good.

Show 6 quoted lines
>> Define "ready". It's probably more "ready" than any other bridge
>> tool out there.
>
> Anything that needs "oh, we need to push these ten patches to avoid
> regressions at the last minute" is not mature enough to be relied
> upon by end users for everyday use.  That is what I meant.

Only a single patch is needed to fix the regression and I sent that patch standalone in v2 of this series, but you didn't pick it, so I sent the cleanups as well.

> But now you are saying these are not regression fixes, in which case
> they have to wait because the users have known about the limitations
> that existed for a long time and learned to live with them.  That is
> a sign of mature (not "not ready") software.

No, they don't have to wait. And we don't have to mindlessly apply guidelines as dogma.

The reason for the "only regression" period is to avoid more regressions. If you show me how any of the fixes I sent in this series could potentially cause a regression, I would agree with you, even if remote, the possibility would be there, therefore the patch wouldn't be material for 1.8.3.

But the fact of the matter is that the possibility is not there, we are not decreasing the possibility of regressions, we are simply removing a feature users could enjoy for no gain at all.

> You cannot be both.  Which is it?

I marked the patch that fix a regression as such, I marked the patches that are obvious fixes with no possibility of regressions as such, and I marked the trivial cleanups with no possibility of regressions as such.

-- 
Felipe Contreras
Junio C Hamano· May 14, 2013, 22:49 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

Felipe Contreras <felipe.contreras@gmail.com> writes:
> The reason for the "only regression" period is to avoid more
> regressions. If you show me how any of the fixes I sent in this series
> could potentially cause a regression,

I already said that "You can see these patches are so trivially correct" is not a valid argument. The original patches would also have been looked correct when they were sent to the list. Things take time and actual use by the users to mature.

Show 6 quoted lines
>> You cannot be both.  Which is it?
>
> I marked the patch that fix a regression as such, I marked the patches
> that are obvious fixes with no possibility of regressions as such, and
> I marked the trivial cleanups with no possibility of regressions as
> such.

I think you mean 6/10 by "the patch that fix a regression", but if that is the case, please send only the regression fix that cleanly apply to the tip of 'master', without any other dependencies, with a proper description of what breaks and how it fixes.

We know you can do better than "certain" and "might".
> In certain situations we might end up pushing garbage revisions (e.g. in
> a rebase), and the patches to deal with that haven't been merged yet.
> 
> So let's disable forced pushes by default.
Thanks.
Felipe Contreras· May 14, 2013, 23:11 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

On Tue, May 14, 2013 at 5:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> The reason for the "only regression" period is to avoid more
>> regressions. If you show me how any of the fixes I sent in this series
>> could potentially cause a regression,
>
> I already said that "You can see these patches are so trivially
> correct" is not a valid argument.
You can say so, it doesn't make it so.
> The original patches would also
> have been looked correct when they were sent to the list.

No, they didn't look correct, and I made it clear when I sent the patches to be *tested*.

> Things
> take time and actual use by the users to mature.
Some things, not all things. That's a hasty generalization fallacy.
Show 11 quoted lines
>>> You cannot be both.  Which is it?
>>
>> I marked the patch that fix a regression as such, I marked the patches
>> that are obvious fixes with no possibility of regressions as such, and
>> I marked the trivial cleanups with no possibility of regressions as
>> such.
>
> I think you mean 6/10 by "the patch that fix a regression", but if
> that is the case, please send only the regression fix that cleanly
> apply to the tip of 'master', without any other dependencies, with a
> proper description of what breaks and how it fixes.
I did that for v2, but you didn't merge that.
Show 6 quoted lines
> We know you can do better than "certain" and "might".
>
>> In certain situations we might end up pushing garbage revisions (e.g. in
>> a rebase), and the patches to deal with that haven't been merged yet.
>>
>> So let's disable forced pushes by default.

I have done more than enough. I'm not going to write tests for this, and I'm not going to find out for sure. There is a *potential* that there's a regression, and that's reason enough to apply this patch as it is.

I have done way more than my fair share already. I'm 98% certain that this patch series as it is would not introduce a regression for v1.8.3, and that's good enough for me. If anybody has any problem with that, they can pick this series and do whatever they want with them.

If I'm supposed to be the owner of contrib/remote-helpers, it certainly doesn't feel that way. If I am the owner, but you are worried, why don't you let me make the decision, and if something blows in v1.8.3, you can tell me; "see? you are not ready to have the full maintainership of this". After all, this is in the contrib area, so if there's a time for a possible future maintainer of a core part of git to make mistakes, it would be now. But of course, this would *not* happen, because the patches are obviously correct and I stand by that claim.

As things currently stand, v1.8.3 *might* introduce a regression.
-- 
Felipe Contreras
Junio C Hamano· May 14, 2013, 23:32 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

Felipe Contreras <felipe.contreras@gmail.com> writes:
> ... After all, this is in the contrib area,
> so if there's a time for a possible future maintainer of a core part
> of git to make mistakes, it would be now.
That sounds reasonable.

Incidentally, before I had to stop working in order to respond to your endless arguments, I already queued the 8 patches to 'master' (also remote-bzr one is in below that).

I had no time checking other topics in flight nor merging the result up to 'next' and 'pu' yet, and it will take a while for the result to be published, but we'll see these in 1.8.3.

Felipe Contreras· May 14, 2013, 23:37 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling

On Tue, May 14, 2013 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> ... After all, this is in the contrib area,
>> so if there's a time for a possible future maintainer of a core part
>> of git to make mistakes, it would be now.
>
> That sounds reasonable.
>
> Incidentally, before I had to stop working in order to respond to
> your endless arguments, I already queued the 8 patches to 'master'
> (also remote-bzr one is in below that).
Cool. But FTR, the "endless arguments" come from both sides.
> I had no time checking other topics in flight nor merging the result
> up to 'next' and 'pu' yet, and it will take a while for the result
> to be published, but we'll see these in 1.8.3.
Well, the priority is 1.8.3.
Cheers.
-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:11 UTC · re: Felipe Contreras · lore

[PATCH v3 09/10] remote-hg: test: be a little more quiet

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/test-hg.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/remote-helpers/test-hg.sh +2 −2
diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh
index 8de2aa7..f8d1f9e 100755
--- a/contrib/remote-helpers/test-hg.sh
+++ b/contrib/remote-helpers/test-hg.sh
@@ -109,10 +109,10 @@ test_expect_success 'update bookmark' '
   (
   git clone "hg::$PWD/hgrepo" gitrepo &&
   cd gitrepo &&
-  git checkout devel &&
+  git checkout --quiet devel &&
   echo devel > content &&
   git commit -a -m devel &&
-  git push
+  git push --quiet
   ) &&
 
   hg -R hgrepo bookmarks | egrep "devel[	 ]+3:"

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:27 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 09/10] remote-hg: test: be a little more quiet

No-brainer; improve one test.
Felipe Contreras wrote:
Show 24 quoted lines
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/test-hg.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh
> index 8de2aa7..f8d1f9e 100755
> --- a/contrib/remote-helpers/test-hg.sh
> +++ b/contrib/remote-helpers/test-hg.sh
> @@ -109,10 +109,10 @@ test_expect_success 'update bookmark' '
>    (
>    git clone "hg::$PWD/hgrepo" gitrepo &&
>    cd gitrepo &&
> -  git checkout devel &&
> +  git checkout --quiet devel &&
>    echo devel > content &&
>    git commit -a -m devel &&
> -  git push
> +  git push --quiet
>    ) &&
>  
>    hg -R hgrepo bookmarks | egrep "devel[	 ]+3:"
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Junio C Hamano· May 14, 2013, 21:40 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 09/10] remote-hg: test: be a little more quiet

Felipe Contreras <felipe.contreras@gmail.com> writes:
> No-brainer; improve one test.

In general, unless we are taking the output from commands to a file and grepping in it, we prefer not to have --quiet (unless you are testing the --quiet feature of the command, of course). Running the tests without "-v" option will not show them and when running with "-v" to debug the tests, the extra output will help to figure out which step failed.

Show 25 quoted lines
> Felipe Contreras wrote:
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  contrib/remote-helpers/test-hg.sh | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh
>> index 8de2aa7..f8d1f9e 100755
>> --- a/contrib/remote-helpers/test-hg.sh
>> +++ b/contrib/remote-helpers/test-hg.sh
>> @@ -109,10 +109,10 @@ test_expect_success 'update bookmark' '
>>    (
>>    git clone "hg::$PWD/hgrepo" gitrepo &&
>>    cd gitrepo &&
>> -  git checkout devel &&
>> +  git checkout --quiet devel &&
>>    echo devel > content &&
>>    git commit -a -m devel &&
>> -  git push
>> +  git push --quiet
>>    ) &&
>>  
>>    hg -R hgrepo bookmarks | egrep "devel[	 ]+3:"
>> -- 
>> 1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 21:52 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 09/10] remote-hg: test: be a little more quiet

On Tue, May 14, 2013 at 4:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> No-brainer; improve one test.
>
> In general, unless we are taking the output from commands to a file
> and grepping in it, we prefer not to have --quiet (unless you are
> testing the --quiet feature of the command, of course).  Running the
> tests without "-v" option will not show them and when running with
> "-v" to debug the tests, the extra output will help to figure out
> which step failed.

Yeah, but I spent a long time looking at the output of these tests and grew tired of all the irrelevant noise. In fact, I'm even tempted to set push.default because of that annoying message all over them. Maybe the --quiet for the push shouldn't be there, but the --quiet for checkout definitely. Either way, I don't see much value in changing this patch at this point.

-- 
Felipe Contreras
Felipe Contreras· May 13, 2013, 23:12 UTC · re: Felipe Contreras · lore

[PATCH v3 10/10] remote-hg: trivial reorganization

We only need to get the remote dict once.
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/remote-helpers/git-remote-hg +2 −2
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index dc276af..96bed8d 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -860,6 +860,7 @@ def do_export(parser):
 
     if peer:
         parser.repo.push(peer, force=force_push, newbranch=True)
+        remote_bmarks = peer.listkeys('bookmarks')
 
     # handle bookmarks
     for bmark, node in p_bmarks:
@@ -886,8 +887,7 @@ def do_export(parser):
             continue
 
         if peer:
-            rb = peer.listkeys('bookmarks')
-            old = rb.get(bmark, '')
+            old = remote_bmarks.get(bmark, '')
             if not peer.pushkey('bookmarks', bmark, old, new):
                 print "error %s" % ref
                 continue

-- 
1.8.3.rc1.579.g184e698
Felipe Contreras· May 14, 2013, 20:27 UTC · re: Felipe Contreras · lore

RE: [PATCH v3 10/10] remote-hg: trivial reorganization

Another no-brainer; simply shuffling some code.
Felipe Contreras wrote:
Show 31 quoted lines
> We only need to get the remote dict once.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  contrib/remote-helpers/git-remote-hg | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index dc276af..96bed8d 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -860,6 +860,7 @@ def do_export(parser):
>  
>      if peer:
>          parser.repo.push(peer, force=force_push, newbranch=True)
> +        remote_bmarks = peer.listkeys('bookmarks')
>  
>      # handle bookmarks
>      for bmark, node in p_bmarks:
> @@ -886,8 +887,7 @@ def do_export(parser):
>              continue
>  
>          if peer:
> -            rb = peer.listkeys('bookmarks')
> -            old = rb.get(bmark, '')
> +            old = remote_bmarks.get(bmark, '')
>              if not peer.pushkey('bookmarks', bmark, old, new):
>                  print "error %s" % ref
>                  continue
> -- 
> 1.8.3.rc1.579.g184e698
-- 
Felipe Contreras
Junio C Hamano· May 14, 2013, 01:08 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 00/10] remote-hg: fixes and cleanups

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Since the last series is not merged to master yet, I decided to add more cleanups.

Because nothing new will go to 'master' past -rc1 by default, unless you are working on fixing or finding 1.8.3 regressions, this is a good time to polish things that are meant for the next cycle.

Folks interested in working remote-hg, please try it out, so that we can have a polished one soon after 1.8.3 ships (I am not saying this round is not polished---I haven't even looked at the patches).

And others, please spend time on testing the 1.8.3-rc2 to make sure what we are going to ship is free of embarrassing regressions.

Thanks.
Show 17 quoted lines
>
> Felipe Contreras (10):
>   remote-hg: trivial cleanups
>   remote-hg: get rid of unused exception checks
>   remote-hg: enable track-branches in hg-git mode
>   remote-hg: add new get_config_bool() helper
>   remote-hg: fix new branch creation
>   remote-hg: disable forced push by default
>   remote-hg: don't push fake 'master' bookmark
>   remote-hg: update bookmarks when pulling
>   remote-hg: test: be a little more quiet
>   remote-hg: trivial reorganization
>
>  contrib/remote-helpers/git-remote-hg     | 47 ++++++++++++++++----------------
>  contrib/remote-helpers/test-hg-hg-git.sh |  3 +-
>  contrib/remote-helpers/test-hg.sh        |  4 +--
>  3 files changed, 26 insertions(+), 28 deletions(-)
Felipe Contreras· May 14, 2013, 03:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 00/10] remote-hg: fixes and cleanups

On Mon, May 13, 2013 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Since the last series is not merged to master yet, I decided to add more cleanups.
>
> Because nothing new will go to 'master' past -rc1 by default, unless
> you are working on fixing or finding 1.8.3 regressions, this is a
> good time to polish things that are meant for the next cycle.

I know, I've been polishing a bunch of patches for the next cycle for a long time.

Show 6 quoted lines
> Folks interested in working remote-hg, please try it out, so that we
> can have a polished one soon after 1.8.3 ships (I am not saying this
> round is not polished---I haven't even looked at the patches).
>
> And others, please spend time on testing the 1.8.3-rc2 to make sure
> what we are going to ship is free of embarrassing regressions.

The whole purpose of this series is to avoid regressions, that's why I sent them for 1.8.3. I thought I made it clear[1] that we would want these patches in, to avoid regressions, unless it's desirable that we end up pushing garbage to a remote repository.

Cheers.
[1] http://article.gmane.org/gmane.comp.version-control.git/224224
-- 
Felipe Contreras
Junio C Hamano· May 14, 2013, 18:21 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 00/10] remote-hg: fixes and cleanups

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 6 quoted lines
> On Mon, May 13, 2013 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> And others, please spend time on testing the 1.8.3-rc2 to make sure
>> what we are going to ship is free of embarrassing regressions.
>
> The whole purpose of this series is to avoid regressions, that's why I
> sent them for 1.8.3.

We agreed to make an exception to let remote-bzr updates go in even after v1.8.3-rc1, because it is too young and you can afford to check that the updated code will give only gains to its userbase that matters without hurting them by checking with Emacs and other projects.

I do not recall us doing a similar exception for remote-hg.  Did we?

If we didn't, then a 10-patch series at this point in the cycle is way too late for me to be comfortable taking.

We merged the 21-patch remote-hg series from you on Apr 21st, a week before -rc0, and it has been 3 weeks. Back then you thought it made things better without regression, and I expected that loose ends could be fixed by -rc1 with enough time to cook them in 'next' (meaning tying such loose ends would be just the matter of a couple of trivial patches at most). But now you are saying they regress things and need 6 (in 'next') + 10 + 47 patches to fix on top, in order not to hurt existing users?

What is going on?

People make mistakes and the initial submission being buggy is *not* a problem per-se, but what quality assurance do the 10-patch and/or the follow up 47-patch series have over these 21 patches to assure us that they do not introduce more problems, when we are this close to the final, way less than the 3 weeks we had?

The best we could do to avoid regressions (if there are reported ones) is to revert specific changes that cause the regression that are already in v1.8.3-rc2. Which one(s) should we be reverting, and do you have a regression report that says the commit(s) in question breaks things for a specific project, and reverting it(them) makes things work again?

Junio C Hamano· May 14, 2013, 18:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 00/10] remote-hg: fixes and cleanups

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> ...  But now you are saying they regress
> things and need 6 (in 'next') + 10 + 47 patches to fix on top, in
> order not to hurt existing users?
>
> What is going on?

Ahh, OK, I miscounted. The 10 were supposed to replace 6 and then 47 in turn are supposed to replace the whole thing, while these are still *not* in 'next'.

OK, will replace fc/remote-hg that is currently on 'pu' with the latest (v4 */47).

Felipe Contreras· May 14, 2013, 20:02 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 00/10] remote-hg: fixes and cleanups

On Tue, May 14, 2013 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Mon, May 13, 2013 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> And others, please spend time on testing the 1.8.3-rc2 to make sure
>>> what we are going to ship is free of embarrassing regressions.
>>
>> The whole purpose of this series is to avoid regressions, that's why I
>> sent them for 1.8.3.
>
> We agreed to make an exception to let remote-bzr updates go in even
> after v1.8.3-rc1, because it is too young and you can afford to
> check that the updated code will give only gains to its userbase
> that matters without hurting them by checking with Emacs and other
> projects.
>
> I do not recall us doing a similar exception for remote-hg.  Did we?

The exception was for massive changes, theses are not massive changes, they are no-brainer fixes that would possibly fix regressions.

> If we didn't, then a 10-patch series at this point in the cycle is
> way too late for me to be comfortable taking.
Well, don't blame me if users hit regressions then.
Show 10 quoted lines
> We merged the 21-patch remote-hg series from you on Apr 21st, a week
> before -rc0, and it has been 3 weeks.  Back then you thought it made
> things better without regression, and I expected that loose ends
> could be fixed by -rc1 with enough time to cook them in 'next'
> (meaning tying such loose ends would be just the matter of a couple
> of trivial patches at most).  But now you are saying they regress
> things and need 6 (in 'next') + 10 + 47 patches to fix on top, in
> order not to hurt existing users?
>
> What is going on?

No, I sent *four* patches for 'master', which I eventually increased to ten, because the increased ones are all simple cleanups.

And the fixes are obvious and can't possibly introduce regressions, specially since the important change re-introduces the same behavior we had in v1.8.2.

The 47 patches I sent are for 'next', and are clearly marked as so. I included the same 10 fixes I sent for 'master', because they never landed on master.

Show 5 quoted lines
> People make mistakes and the initial submission being buggy is *not*
> a problem per-se, but what quality assurance do the 10-patch and/or
> the follow up 47-patch series have over these 21 patches to assure
> us that they do not introduce more problems, when we are this close
> to the final, way less than the 3 weeks we had?
Read the patches and you would see.
Show 6 quoted lines
> The best we could do to avoid regressions (if there are reported
> ones) is to revert specific changes that cause the regression that
> are already in v1.8.3-rc2.  Which one(s) should we be reverting, and
> do you have a regression report that says the commit(s) in question
> breaks things for a specific project, and reverting it(them) makes
> things work again?
I am going to go one by one to show you the patches make sense for 'master'.
-- 
Felipe Contreras
Felipe Contreras· May 14, 2013, 04:47 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 00/10] remote-hg: fixes and cleanups

On Mon, May 13, 2013 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> Folks interested in working remote-hg, please try it out, so that we
> can have a polished one soon after 1.8.3 ships (I am not saying this
> round is not polished---I haven't even looked at the patches).
>
> And others, please spend time on testing the 1.8.3-rc2 to make sure
> what we are going to ship is free of embarrassing regressions.

If we want folks to test something, it should be the patches I prepared for 'next' which I just sent.

Cheers.
-- 
Felipe Contreras
Junio C Hamano· May 14, 2013, 18:59 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 00/10] remote-hg: fixes and cleanups

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 11 quoted lines
> On Mon, May 13, 2013 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Folks interested in working remote-hg, please try it out, so that we
>> can have a polished one soon after 1.8.3 ships (I am not saying this
>> round is not polished---I haven't even looked at the patches).
>>
>> And others, please spend time on testing the 1.8.3-rc2 to make sure
>> what we are going to ship is free of embarrassing regressions.
>
> If we want folks to test something, it should be the patches I
> prepared for 'next' which I just sent.

Yeah, but that is for the release _after_ 1.8.3; I'd rather see folks help to make sure we have a solid 1.8.3 relaese.

Felipe Contreras· May 14, 2013, 20:08 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 00/10] remote-hg: fixes and cleanups

On Tue, May 14, 2013 at 1:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Mon, May 13, 2013 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> Folks interested in working remote-hg, please try it out, so that we
>>> can have a polished one soon after 1.8.3 ships (I am not saying this
>>> round is not polished---I haven't even looked at the patches).
>>>
>>> And others, please spend time on testing the 1.8.3-rc2 to make sure
>>> what we are going to ship is free of embarrassing regressions.
>>
>> If we want folks to test something, it should be the patches I
>> prepared for 'next' which I just sent.
>
> Yeah, but that is for the release _after_ 1.8.3; I'd rather see
> folks help to make sure we have a solid 1.8.3 relaese.

That's the intention of the ten patches I sent for 1.8.3. But you said you are not going to put them in 'master', but in 'next', so I sent the ones I think should go into next.

So now you have the patches that I think should go into 'master' (10), and the ones for 'next' (47, including the previous unmerged 10).

-- 
Felipe Contreras

← back to recent threads