{"thread":{"id":"33923","subject":"[PATCH v2 0/8] remote-bzr: patches for next","startedAt":"2013-05-25T02:24:18Z","lastAt":"2013-05-29T17:03:30Z","messageCount":13,"participants":["Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":8},"messages":[{"id":"218420","messageId":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":null,"subject":"[PATCH v2 0/8] remote-bzr: patches for next","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:18Z","receivedAt":"2013-05-25T02:24:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nMinor fixes since last time.\n\nThese patches have been cooking in my github repository, and improve the\nsituation when bzr servers don't support repositories properly.\n\nFelipe Contreras (8):\n  remote-bzr: recover from failed clones\n  remote-bzr: fix for files with spaces\n  remote-bzr: simplify get_remote_branch()\n  remote-bzr: delay cloning/pulling\n  remote-bzr: change global repo\n  remote-bzr: trivial cleanups\n  remote-bzr: reorganize the way 'wanted' works\n  remote-bzr: add fallback check for a partial clone\n\n contrib/remote-helpers/git-remote-bzr | 112 +++++++++++++++++-----------------\n 1 file changed, 55 insertions(+), 57 deletions(-)\n\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218421","messageId":"1369448666-17515-2-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 1/8] remote-bzr: recover from failed clones","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:19Z","receivedAt":"2013-05-25T02:24:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/remote-helpers/git-remote-bzr | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr\nindex 10300c6..8a4df51 100755\n--- a/contrib/remote-helpers/git-remote-bzr\n+++ b/contrib/remote-helpers/git-remote-bzr\n@@ -769,22 +769,24 @@ def get_remote_branch(origin, remote_branch, name):\n     global dirname, peers\n \n     branch_path = os.path.join(dirname, 'clone', name)\n-    if os.path.exists(branch_path):\n-        # pull\n+\n+    try:\n         d = bzrlib.bzrdir.BzrDir.open(branch_path)\n         branch = d.open_branch()\n-        try:\n-            branch.pull(remote_branch, [], None, False)\n-        except bzrlib.errors.DivergedBranches:\n-            # use remote branch for now\n-            return remote_branch\n-    else:\n+    except bzrlib.errors.NotBranchError:\n         # clone\n         d = origin.sprout(branch_path, None,\n                 hardlink=True, create_tree_if_local=False,\n                 force_new_repo=False,\n                 source_branch=remote_branch)\n         branch = d.open_branch()\n+    else:\n+        # pull\n+        try:\n+            branch.pull(remote_branch, [], None, False)\n+        except bzrlib.errors.DivergedBranches:\n+            # use remote branch for now\n+            return remote_branch\n \n     return branch\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218423","messageId":"1369448666-17515-3-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 2/8] remote-bzr: fix for files with spaces","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:20Z","receivedAt":"2013-05-25T02:24:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Set the maximum number of splits to make when dividing the diff stat\nlines based on space characters.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/remote-helpers/git-remote-bzr | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr\nindex 8a4df51..35664c6 100755\n--- a/contrib/remote-helpers/git-remote-bzr\n+++ b/contrib/remote-helpers/git-remote-bzr\n@@ -621,7 +621,7 @@ def parse_commit(parser):\n             mark = int(mark_ref[1:])\n             f = { 'mode' : m, 'mark' : mark }\n         elif parser.check('D'):\n-            t, path = line.split(' ')\n+            t, path = line.split(' ', 1)\n             f = { 'deleted' : True }\n         else:\n             die('Unknown file command: %s' % line)\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218422","messageId":"1369448666-17515-4-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 3/8] remote-bzr: simplify get_remote_branch()","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:21Z","receivedAt":"2013-05-25T02:24:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"No need for 'origin', it's only needed for the bzrdir 'sprout' method,\nwhich can be greatly simplified.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/remote-helpers/git-remote-bzr | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr\nindex 35664c6..5c4201a 100755\n--- a/contrib/remote-helpers/git-remote-bzr\n+++ b/contrib/remote-helpers/git-remote-bzr\n@@ -765,25 +765,26 @@ def do_list(parser):\n     print \"@refs/heads/%s HEAD\" % master_branch\n     print\n \n-def get_remote_branch(origin, remote_branch, name):\n+def clone(path, remote_branch):\n+    bdir = bzrlib.bzrdir.BzrDir.create(path)\n+    repo = bdir.find_repository()\n+    repo.fetch(remote_branch.repository)\n+    return remote_branch.sprout(bdir, repository=repo)\n+\n+def get_remote_branch(remote_branch, name):\n     global dirname, peers\n \n     branch_path = os.path.join(dirname, 'clone', name)\n \n     try:\n-        d = bzrlib.bzrdir.BzrDir.open(branch_path)\n-        branch = d.open_branch()\n+        branch = bzrlib.branch.Branch.open(branch_path)\n     except bzrlib.errors.NotBranchError:\n         # clone\n-        d = origin.sprout(branch_path, None,\n-                hardlink=True, create_tree_if_local=False,\n-                force_new_repo=False,\n-                source_branch=remote_branch)\n-        branch = d.open_branch()\n+        branch = clone(branch_path, remote_branch)\n     else:\n         # pull\n         try:\n-            branch.pull(remote_branch, [], None, False)\n+            branch.pull(remote_branch, overwrite=True)\n         except bzrlib.errors.DivergedBranches:\n             # use remote branch for now\n             return remote_branch\n@@ -856,7 +857,7 @@ def get_repo(url, alias):\n \n         if not is_local:\n             peers[name] = remote_branch.base\n-            branch = get_remote_branch(origin, remote_branch, name)\n+            branch = get_remote_branch(remote_branch, name)\n         else:\n             branch = remote_branch\n \n@@ -874,7 +875,7 @@ def get_repo(url, alias):\n \n             if not is_local:\n                 peers[name] = remote_branch.base\n-                branch = get_remote_branch(origin, remote_branch, name)\n+                branch = get_remote_branch(remote_branch, name)\n             else:\n                 branch = remote_branch\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218424","messageId":"1369448666-17515-5-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 4/8] remote-bzr: delay cloning/pulling","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:22Z","receivedAt":"2013-05-25T02:24:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Until the branch is actually going to be used.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/remote-helpers/git-remote-bzr | 28 +++++++++++++---------------\n 1 file changed, 13 insertions(+), 15 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr\nindex 5c4201a..202a4f7 100755\n--- a/contrib/remote-helpers/git-remote-bzr\n+++ b/contrib/remote-helpers/git-remote-bzr\n@@ -278,7 +278,7 @@ def export_branch(repo, name):\n     ref = '%s/heads/%s' % (prefix, name)\n     tip = marks.get_tip(name)\n \n-    branch = bzrlib.branch.Branch.open(branches[name])\n+    branch = get_remote_branch(name)\n     repo = branch.repository\n \n     branch.lock_read()\n@@ -590,7 +590,7 @@ def parse_commit(parser):\n \n     if ref.startswith('refs/heads/'):\n         name = ref[len('refs/heads/'):]\n-        branch = bzrlib.branch.Branch.open(branches[name])\n+        branch = get_remote_branch(name)\n     else:\n         die('unknown ref')\n \n@@ -692,7 +692,7 @@ def do_export(parser):\n     for ref, revid in parsed_refs.iteritems():\n         if ref.startswith('refs/heads/'):\n             name = ref[len('refs/heads/'):]\n-            branch = bzrlib.branch.Branch.open(branches[name])\n+            branch = get_remote_branch(name)\n             branch.generate_revision_history(revid, marks.get_tip(name))\n \n             if name in peers:\n@@ -749,7 +749,7 @@ def do_list(parser):\n             master_branch = name\n         print \"? refs/heads/%s\" % name\n \n-    branch = bzrlib.branch.Branch.open(branches[master_branch])\n+    branch = get_remote_branch(master_branch)\n     branch.lock_read()\n     for tag, revid in branch.tags.get_tag_dict().items():\n         try:\n@@ -771,8 +771,12 @@ def clone(path, remote_branch):\n     repo.fetch(remote_branch.repository)\n     return remote_branch.sprout(bdir, repository=repo)\n \n-def get_remote_branch(remote_branch, name):\n-    global dirname, peers\n+def get_remote_branch(name):\n+    global dirname, branches\n+\n+    remote_branch = bzrlib.branch.Branch.open(branches[name])\n+    if isinstance(remote_branch.user_transport, bzrlib.transport.local.LocalTransport):\n+        return remote_branch\n \n     branch_path = os.path.join(dirname, 'clone', name)\n \n@@ -857,13 +861,10 @@ def get_repo(url, alias):\n \n         if not is_local:\n             peers[name] = remote_branch.base\n-            branch = get_remote_branch(remote_branch, name)\n-        else:\n-            branch = remote_branch\n \n-        branches[name] = branch.base\n+        branches[name] = remote_branch.base\n \n-        return branch.repository\n+        return remote_branch.repository\n     else:\n         # repository\n \n@@ -875,11 +876,8 @@ def get_repo(url, alias):\n \n             if not is_local:\n                 peers[name] = remote_branch.base\n-                branch = get_remote_branch(remote_branch, name)\n-            else:\n-                branch = remote_branch\n \n-            branches[name] = branch.base\n+            branches[name] = remote_branch.base\n \n         return repo\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218427","messageId":"1369448666-17515-6-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 5/8] remote-bzr: change global repo","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:23Z","receivedAt":"2013-05-25T02:24:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It's not used anyway.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/remote-helpers/git-remote-bzr | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr\nindex 202a4f7..80ed59f 100755\n--- a/contrib/remote-helpers/git-remote-bzr\n+++ b/contrib/remote-helpers/git-remote-bzr\n@@ -864,7 +864,7 @@ def get_repo(url, alias):\n \n         branches[name] = remote_branch.base\n \n-        return remote_branch.repository\n+        return origin\n     else:\n         # repository\n \n@@ -879,7 +879,7 @@ def get_repo(url, alias):\n \n             branches[name] = remote_branch.base\n \n-        return repo\n+        return origin\n \n def fix_path(alias, orig_url):\n     url = urlparse.urlparse(orig_url, 'file')\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218425","messageId":"1369448666-17515-7-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 6/8] remote-bzr: trivial cleanups","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:24Z","receivedAt":"2013-05-25T02:24:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/remote-helpers/git-remote-bzr | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr\nindex 80ed59f..34025c3 100755\n--- a/contrib/remote-helpers/git-remote-bzr\n+++ b/contrib/remote-helpers/git-remote-bzr\n@@ -815,7 +815,7 @@ def find_branches(repo, wanted):\n         except bzrlib.errors.NotBranchError:\n             continue\n         else:\n-            yield name, branch\n+            yield name, branch.base\n \n def get_repo(url, alias):\n     global dirname, peer, branches\n@@ -857,12 +857,12 @@ def get_repo(url, alias):\n         # branch\n \n         name = 'master'\n-        remote_branch = origin.open_branch()\n+        branch = origin.open_branch().base\n \n         if not is_local:\n-            peers[name] = remote_branch.base\n+            peers[name] = branch\n \n-        branches[name] = remote_branch.base\n+        branches[name] = branch\n \n         return origin\n     else:\n@@ -872,12 +872,12 @@ def get_repo(url, alias):\n         # stupid python\n         wanted = [e for e in wanted if e]\n \n-        for name, remote_branch in find_branches(repo, wanted):\n+        for name, branch in find_branches(repo, wanted):\n \n             if not is_local:\n-                peers[name] = remote_branch.base\n+                peers[name] = branch\n \n-            branches[name] = remote_branch.base\n+            branches[name] = branch\n \n         return origin\n \n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218426","messageId":"1369448666-17515-8-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:25Z","receivedAt":"2013-05-25T02:24:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"If the user specified a list of branches, we ignore what the remote\nrepository lists, and simply use the branches directly. Since some\nremotes don't report the branches correctly, this is useful.\n\nOtherwise either fetch the repo, or the branch.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/remote-helpers/git-remote-bzr | 58 ++++++++++++++++-------------------\n 1 file changed, 26 insertions(+), 32 deletions(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr\nindex 34025c3..3248586 100755\n--- a/contrib/remote-helpers/git-remote-bzr\n+++ b/contrib/remote-helpers/git-remote-bzr\n@@ -795,7 +795,7 @@ def get_remote_branch(name):\n \n     return branch\n \n-def find_branches(repo, wanted):\n+def find_branches(repo):\n     transport = repo.bzrdir.root_transport\n \n     for fn in transport.iter_files_recursive():\n@@ -806,9 +806,6 @@ def find_branches(repo, wanted):\n         name = name if name != '' else 'master'\n         name = name.replace('/', '+')\n \n-        if wanted and not name in wanted:\n-            continue\n-\n         try:\n             cur = transport.clone(subdir)\n             branch = bzrlib.branch.Branch.open_from_transport(cur)\n@@ -848,38 +845,35 @@ def get_repo(url, alias):\n             except bzrlib.errors.NoRepositoryPresent:\n                 pass\n \n-    try:\n-        repo = origin.open_repository()\n-        if not repo.user_transport.listable():\n-            # this repository is not usable for us\n-            raise bzrlib.errors.NoRepositoryPresent(repo.bzrdir)\n-    except bzrlib.errors.NoRepositoryPresent:\n-        # branch\n-\n-        name = 'master'\n-        branch = origin.open_branch().base\n-\n-        if not is_local:\n-            peers[name] = branch\n+    wanted = get_config('remote-bzr.branches').rstrip().split(', ')\n+    # stupid python\n+    wanted = [e for e in wanted if e]\n \n-        branches[name] = branch\n-\n-        return origin\n+    if not wanted:\n+        try:\n+            repo = origin.open_repository()\n+            if not repo.user_transport.listable():\n+                # this repository is not usable for us\n+                raise bzrlib.errors.NoRepositoryPresent(repo.bzrdir)\n+        except bzrlib.errors.NoRepositoryPresent:\n+            wanted = ['master']\n+\n+    if wanted:\n+        def list_wanted(url, wanted):\n+            for name in wanted:\n+                subdir = name if name != 'master' else ''\n+                yield name, bzrlib.urlutils.join(url, subdir)\n+\n+        branch_list = list_wanted(url, wanted)\n     else:\n-        # repository\n-\n-        wanted = get_config('remote-bzr.branches').rstrip().split(', ')\n-        # stupid python\n-        wanted = [e for e in wanted if e]\n+        branch_list = find_branches(repo)\n \n-        for name, branch in find_branches(repo, wanted):\n-\n-            if not is_local:\n-                peers[name] = branch\n-\n-            branches[name] = branch\n+    for name, url in branch_list:\n+        if not is_local:\n+            peers[name] = url\n+        branches[name] = url\n \n-        return origin\n+    return origin\n \n def fix_path(alias, orig_url):\n     url = urlparse.urlparse(orig_url, 'file')\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218428","messageId":"1369448666-17515-9-git-send-email-felipe.contreras@gmail.com","threadId":"33923","inReplyTo":"1369448666-17515-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 8/8] remote-bzr: add fallback check for a partial clone","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-25T02:24:26Z","receivedAt":"2013-05-25T02:24:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n contrib/remote-helpers/git-remote-bzr | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr\nindex 3248586..3cd6572 100755\n--- a/contrib/remote-helpers/git-remote-bzr\n+++ b/contrib/remote-helpers/git-remote-bzr\n@@ -766,7 +766,10 @@ def do_list(parser):\n     print\n \n def clone(path, remote_branch):\n-    bdir = bzrlib.bzrdir.BzrDir.create(path)\n+    try:\n+        bdir = bzrlib.bzrdir.BzrDir.create(path)\n+    except bzrlib.errors.AlreadyControlDirError:\n+        bdir = bzrlib.bzrdir.BzrDir.open(path)\n     repo = bdir.find_repository()\n     repo.fetch(remote_branch.repository)\n     return remote_branch.sprout(bdir, repository=repo)\n-- \n1.8.3.rc3.312.g47657de\n"},{"id":"218675","messageId":"7vvc632g1s.fsf@alter.siamese.dyndns.org","threadId":"33923","inReplyTo":"1369448666-17515-8-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-28T17:02:55Z","receivedAt":"2013-05-28T17:02:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> +    wanted = get_config('remote-bzr.branches').rstrip().split(', ')\n\nTwo minor nits and one design suggestion:\n\n - Why rstrip() not strip()?  It appears that this only is helping\n   an end-user \"mistake\" like this:\n\n\tgit config remote-bzr.branches 'trunk, devel, test '\n\n   without helping people who have done this:\n\n\tgit config remote-bzr.branches 'trunk,  devel, test'\n\n - Is\n\n     git config remote-bzr.branches trunk,devel,test\n\n   a grave sin?\n\n   In other words, wouldn't we want something like this instead?\n\n\tmap(lambda s: s.strip(), get_config('...').split(','))\n\n - Doesn't allowing multi-valued variable, e.g.\n\n\t[remote-bzr]\n            branches = trunk\n            branches = devel\n            branches = test\n\n   make it easier for the user to manage this configuration by\n   e.g. selectively removing or adding tracked branches?\n"},{"id":"218711","messageId":"51a5686ba7f2c_807b33e189951a@nysa.mail","threadId":"33923","inReplyTo":"7vvc632g1s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T02:31:07Z","receivedAt":"2013-05-29T02:31:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n> \n> > +    wanted = get_config('remote-bzr.branches').rstrip().split(', ')\n> \n> Two minor nits and one design suggestion:\n> \n>  - Why rstrip() not strip()?\n\nThe purpose of the strip is to remove the _single_ \"\\n\" at the end that\nsubprocess communicate. Maybe get_config() should do that.\n\n> It appears that this only is helping\n>    an end-user \"mistake\" like this:\n> \n> \tgit config remote-bzr.branches 'trunk, devel, test '\n> \n>    without helping people who have done this:\n> \n> \tgit config remote-bzr.branches 'trunk,  devel, test'\n\nNo, that's tnot it.\n\n>  - Is\n> \n>      git config remote-bzr.branches trunk,devel,test\n> \n>    a grave sin?\n> \n>    In other words, wouldn't we want something like this instead?\n> \n> \tmap(lambda s: s.strip(), get_config('...').split(','))\n\nYeah, that might make sense.\n\n>  - Doesn't allowing multi-valued variable, e.g.\n> \n> \t[remote-bzr]\n>             branches = trunk\n>             branches = devel\n>             branches = test\n> \n>    make it easier for the user to manage this configuration by\n>    e.g. selectively removing or adding tracked branches?\n\nHow would the 'git config' command look like?\n\nEither way, that's orthogonal to this patch.\n\n-- \nFelipe Contreras\n"},{"id":"218851","messageId":"7v38t5wxee.fsf@alter.siamese.dyndns.org","threadId":"33923","inReplyTo":"51a5686ba7f2c_807b33e189951a@nysa.mail","subject":"Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-29T16:42:17Z","receivedAt":"2013-05-29T16:42:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>> \n>> > +    wanted = get_config('remote-bzr.branches').rstrip().split(', ')\n>> \n>> Two minor nits and one design suggestion:\n>> \n>>  - Why rstrip() not strip()?\n>\n> The purpose of the strip is to remove the _single_ \"\\n\" at the end that\n> subprocess communicate. Maybe get_config() should do that.\n>\n>> It appears that this only is helping\n>>    an end-user \"mistake\" like this:\n>> \n>> \tgit config remote-bzr.branches 'trunk, devel, test '\n>> \n>>    without helping people who have done this:\n>> \n>> \tgit config remote-bzr.branches 'trunk,  devel, test'\n>\n> No, that's tnot it.\n\nYes, rstrip() will also lose LF at the end.\n\nBut it also is true that your code also removes the trailing extra\nSP in the first example above, while not losing the extra SP in the\nmiddle in the second example, no?\n\nSo where does \"that's tnot it\" come from?  Is it true or false that\nthe former is helped while the latter is not?\n\n>>  - Is\n>> \n>>      git config remote-bzr.branches trunk,devel,test\n>> \n>>    a grave sin?\n>> \n>>    In other words, wouldn't we want something like this instead?\n>> \n>> \tmap(lambda s: s.strip(), get_config('...').split(','))\n>\n> Yeah, that might make sense.\n\nIf you go that route, you do not even have to even say \"stupid\npython\".  You can write a more meaningful list comprehension, e.g.\n\n\twanted = [s.strip() for s in get_config('...').split(',')]\n\nwithout an unsightly lambda in it.\n\n>>  - Doesn't allowing multi-valued variable, e.g.\n>> \n>> \t[remote-bzr]\n>>             branches = trunk\n>>             branches = devel\n>>             branches = test\n>> \n>>    make it easier for the user to manage this configuration by\n>>    e.g. selectively removing or adding tracked branches?\n>\n> How would the 'git config' command look like?\n>\n> Either way, that's orthogonal to this patch.\n\nYeah, I notice that \"single variable, split at comma\" comes way\nbefore this series anyway, so it is too late (and it is not worth)\nto fix it using multi-valued variables.  It was just an \"if we were\ndesigning this from scratch\" kind of suggestion.\n"},{"id":"218855","messageId":"CAMP44s2UCZQBfSkhc5jg-ipWKMy3M4R-YVODR4=OuAUbpgkD8A@mail.gmail.com","threadId":"33923","inReplyTo":"7v38t5wxee.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-05-29T17:03:30Z","receivedAt":"2013-05-29T17:03:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, May 29, 2013 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> wrote:\n>>> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>>>\n>>> > +    wanted = get_config('remote-bzr.branches').rstrip().split(', ')\n>>>\n>>> Two minor nits and one design suggestion:\n>>>\n>>>  - Why rstrip() not strip()?\n>>\n>> The purpose of the strip is to remove the _single_ \"\\n\" at the end that\n>> subprocess communicate. Maybe get_config() should do that.\n>>\n>>> It appears that this only is helping\n>>>    an end-user \"mistake\" like this:\n>>>\n>>>      git config remote-bzr.branches 'trunk, devel, test '\n>>>\n>>>    without helping people who have done this:\n>>>\n>>>      git config remote-bzr.branches 'trunk,  devel, test'\n>>\n>> No, that's tnot it.\n>\n> Yes, rstrip() will also lose LF at the end.\n>\n> But it also is true that your code also removes the trailing extra\n> SP in the first example above, while not losing the extra SP in the\n> middle in the second example, no?\n>\n> So where does \"that's tnot it\" come from?  Is it true or false that\n> the former is helped while the latter is not?\n\nYou said it is *only* helping the end-user with mistakes, but that's\nnot true, it _also_  gets rid of the new line, which is not only\nhelping, but it's required for the code to work, and it's actually the\npurpose behind the code. The side-effect of removing extra spaces if\nthe user makes mistakes is irrelevant.\n\n>>>  - Is\n>>>\n>>>      git config remote-bzr.branches trunk,devel,test\n>>>\n>>>    a grave sin?\n>>>\n>>>    In other words, wouldn't we want something like this instead?\n>>>\n>>>      map(lambda s: s.strip(), get_config('...').split(','))\n>>\n>> Yeah, that might make sense.\n>\n> If you go that route, you do not even have to even say \"stupid\n> python\".  You can write a more meaningful list comprehension, e.g.\n>\n>         wanted = [s.strip() for s in get_config('...').split(',')]\n>\n> without an unsightly lambda in it.\n\nPython would still do the stupid thing if there's no such configuration:\n\n['']\n\nBut we can add 'if s' at the end, so the code to fix python's\nstupidness is much smaller.\n\nI wonder what 's' means. In C I use 'i', because that's what everybody\nuses, but in more functional-like code we don't use an index, we\niterate directly with the element of an enumerable, so I say 'e' for\nshort.\n\n>>>  - Doesn't allowing multi-valued variable, e.g.\n>>>\n>>>      [remote-bzr]\n>>>             branches = trunk\n>>>             branches = devel\n>>>             branches = test\n>>>\n>>>    make it easier for the user to manage this configuration by\n>>>    e.g. selectively removing or adding tracked branches?\n>>\n>> How would the 'git config' command look like?\n>>\n>> Either way, that's orthogonal to this patch.\n>\n> Yeah, I notice that \"single variable, split at comma\" comes way\n> before this series anyway, so it is too late (and it is not worth)\n> to fix it using multi-valued variables.  It was just an \"if we were\n> designing this from scratch\" kind of suggestion.\n\nIt might be worth, I'm not sure, I'm not familiar with those, and I\ndon't know what the user would have to type, but my guess is that it\nwon't be as user friendly as 'git config foo one,two,three'.\n\n-- \nFelipe Contreras\n"}]}