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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

[Junio C Hamano]

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.

>
> 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(-)

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

[Felipe Contreras]

On Mon, May 13, 2013 at 8:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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

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

[Felipe Contreras]

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.

Cheers.

-- 
Felipe Contreras

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

[Junio C Hamano]

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?

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?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...  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).

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

[Junio C Hamano]

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.

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

[Felipe Contreras]

On Tue, May 14, 2013 at 1:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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.

> 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.

> 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

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

[Felipe Contreras]

On Tue, May 14, 2013 at 1:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Felipe Contreras]

This is a trivial cleanup, cannot cause regressions.

Felipe Contreras wrote:
> 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

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

[Felipe Contreras]

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:
> 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

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

[Felipe Contreras]

No regression here either, we simply give more power to the user.

Felipe Contreras wrote:
> 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

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

[Felipe Contreras]

This is simply refactoring code, functionally they are the same.

Felipe Contreras wrote:
> 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

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

[Felipe Contreras]

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:
> 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

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

[Felipe Contreras]

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:
> 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

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

[Felipe Contreras]

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

Felipe Contreras wrote:
> 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

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

[Felipe Contreras]

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

Felipe Contreras wrote:
> 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

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

[Felipe Contreras]

No-brainer; improve one test.

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

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

[Felipe Contreras]

Another no-brainer; simply shuffling some code.

Felipe Contreras wrote:
> 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

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

[Junio C Hamano]

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.

> 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

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

[Felipe Contreras]

On Tue, May 14, 2013 at 4:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

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.

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

[Felipe Contreras]

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.

> 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*.

> "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.

> 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.

> 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.

> 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

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

[Junio C Hamano]

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.

> 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?

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

[Felipe Contreras]

On Tue, May 14, 2013 at 5:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>> 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

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

[Junio C Hamano]

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.

>> 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.

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

[Felipe Contreras]

On Tue, May 14, 2013 at 5:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>>> 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.

> 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

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

[Junio C Hamano]

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.

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

[Felipe Contreras]

On Tue, May 14, 2013 at 6:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Junio C Hamano]

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.

>> 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

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

[Felipe Contreras]

On Wed, May 15, 2013 at 2:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>     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