Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works
On Wed, May 29, 2013 at 11:42 AM, Junio C Hamano wrote:
> Felipe Contreras writes:
>
>> Junio C Hamano wrote:
>>> Felipe Contreras writes:
>>>
>>> > +wanted = get_config('remote-bzr.branches').rstrip().split(', ')
>>>
>>> Two minor nits and one design suggestion:
>>>
>>> - Why rstrip() not strip()?
>>
>> The purpose of the strip is to remove the _single_ "\n" at the end that
>> subprocess communicate. Maybe get_config() should do that.
>>
>>> It appears that this only is helping
>>>an end-user "mistake" like this:
>>>
>>> git config remote-bzr.branches 'trunk, devel, test '
>>>
>>>without helping people who have done this:
>>>
>>> git config remote-bzr.branches 'trunk, devel, test'
>>
>> No, that's tnot it.
>
> Yes, rstrip() will also lose LF at the end.
>
> But it also is true that your code also removes the trailing extra
> SP in the first example above, while not losing the extra SP in the
> middle in the second example, no?
>
> So where does "that's tnot it" come from? Is it true or false that
> the former is helped while the latter is not?
You said it is *only* helping the end-user with mistakes, but that's
not true, it _also_ gets rid of the new line, which is not only
helping, but it's required for the code to work, and it's actually the
purpose behind the code. The side-effect of removing extra spaces if
the user makes mistakes is irrelevant.
>>> - Is
>>>
>>> git config remote-bzr.branches trunk,devel,test
>>>
>>>a grave sin?
>>>
>>>In other words, wouldn't we want something like this instead?
>>>
>>> map(lambda s: s.strip(), get_config('...').split(','))
>>
>> Yeah, that might make sense.
>
> If you go that route, you do not even have to even say "stupid
> python". You can write a more meaningful list comprehension, e.g.
>
> wanted = [s.strip() for s in get_config('...').split(',')]
>
> without an unsightly lambda in it.
Python would still do the stupid thing if there's no such configuration:
['']
But we can add 'if s' at the end, so the code to fix python's
stupidness is much smaller.
I wonder what 's' means. In C I use 'i', because that's what everybody
uses, but in more functional-like code we don't use an index, we
iterate directly with the element of an enumerable, so I say 'e' for
short.
>>> - Doesn't allowing multi-valued variable, e.g.
>>>
>>> [remote-bzr]
>>> branches = trunk
>>> branches = devel
>>> branches = test
>>>
>>>make it easier for the user to manage this configuration by
>>>e.g. selectively removing or adding tracked branches?
>>
>> How would the 'git config' command look like?
>>
>> Either way, that's orthogonal to this patch.
>
> Yeah, I notice that "single variable, split at comma" comes way
> before this series anyway, so it is too late (and it is not worth)
> to fix it using multi-valued variables. It was just an "if we were
> designing this from scratch" kind of suggestion.
It might be worth, I'm not sure, I'm not familiar with those, and I
don't know what the user would have to type, but my guess is that it
won't be as user friendly as 'git config foo one,two,three'.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works
Felipe Contreras writes:
> Junio C Hamano wrote:
>> Felipe Contreras writes:
>>
>> > +wanted = get_config('remote-bzr.branches').rstrip().split(', ')
>>
>> Two minor nits and one design suggestion:
>>
>> - Why rstrip() not strip()?
>
> The purpose of the strip is to remove the _single_ "\n" at the end that
> subprocess communicate. Maybe get_config() should do that.
>
>> It appears that this only is helping
>>an end-user "mistake" like this:
>>
>> git config remote-bzr.branches 'trunk, devel, test '
>>
>>without helping people who have done this:
>>
>> git config remote-bzr.branches 'trunk, devel, test'
>
> No, that's tnot it.
Yes, rstrip() will also lose LF at the end.
But it also is true that your code also removes the trailing extra
SP in the first example above, while not losing the extra SP in the
middle in the second example, no?
So where does "that's tnot it" come from? Is it true or false that
the former is helped while the latter is not?
>> - Is
>>
>> git config remote-bzr.branches trunk,devel,test
>>
>>a grave sin?
>>
>>In other words, wouldn't we want something like this instead?
>>
>> map(lambda s: s.strip(), get_config('...').split(','))
>
> Yeah, that might make sense.
If you go that route, you do not even have to even say "stupid
python". You can write a more meaningful list comprehension, e.g.
wanted = [s.strip() for s in get_config('...').split(',')]
without an unsightly lambda in it.
>> - Doesn't allowing multi-valued variable, e.g.
>>
>> [remote-bzr]
>> branches = trunk
>> branches = devel
>> branches = test
>>
>>make it easier for the user to manage this configuration by
>>e.g. selectively removing or adding tracked branches?
>
> How would the 'git config' command look like?
>
> Either way, that's orthogonal to this patch.
Yeah, I notice that "single variable, split at comma" comes way
before this series anyway, so it is too late (and it is not worth)
to fix it using multi-valued variables. It was just an "if we were
designing this from scratch" kind of suggestion.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works
Junio C Hamano wrote:
> Felipe Contreras writes:
>
> > +wanted = get_config('remote-bzr.branches').rstrip().split(', ')
>
> Two minor nits and one design suggestion:
>
> - Why rstrip() not strip()?
The purpose of the strip is to remove the _single_ "\n" at the end that
subprocess communicate. Maybe get_config() should do that.
> It appears that this only is helping
>an end-user "mistake" like this:
>
> git config remote-bzr.branches 'trunk, devel, test '
>
>without helping people who have done this:
>
> git config remote-bzr.branches 'trunk, devel, test'
No, that's tnot it.
> - Is
>
> git config remote-bzr.branches trunk,devel,test
>
>a grave sin?
>
>In other words, wouldn't we want something like this instead?
>
> map(lambda s: s.strip(), get_config('...').split(','))
Yeah, that might make sense.
> - Doesn't allowing multi-valued variable, e.g.
>
> [remote-bzr]
> branches = trunk
> branches = devel
> branches = test
>
>make it easier for the user to manage this configuration by
>e.g. selectively removing or adding tracked branches?
How would the 'git config' command look like?
Either way, that's orthogonal to this patch.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works
Felipe Contreras writes:
> +wanted = get_config('remote-bzr.branches').rstrip().split(', ')
Two minor nits and one design suggestion:
- Why rstrip() not strip()? It appears that this only is helping
an end-user "mistake" like this:
git config remote-bzr.branches 'trunk, devel, test '
without helping people who have done this:
git config remote-bzr.branches 'trunk, devel, test'
- Is
git config remote-bzr.branches trunk,devel,test
a grave sin?
In other words, wouldn't we want something like this instead?
map(lambda s: s.strip(), get_config('...').split(','))
- Doesn't allowing multi-valued variable, e.g.
[remote-bzr]
branches = trunk
branches = devel
branches = test
make it easier for the user to manage this configuration by
e.g. selectively removing or adding tracked branches?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works
If the user specified a list of branches, we ignore what the remote
repository lists, and simply use the branches directly. Since some
remotes don't report the branches correctly, this is useful.
Otherwise either fetch the repo, or the branch.
Signed-off-by: Felipe Contreras
---
contrib/remote-helpers/git-remote-bzr | 58 ---
1 file changed, 26 insertions(+), 32 deletions(-)
diff --git a/contrib/remote-helpers/git-remote-bzr
b/contrib/remote-helpers/git-remote-bzr
index 34025c3..3248586 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -795,7 +795,7 @@ def get_remote_branch(name):
return branch
-def find_branches(repo, wanted):
+def find_branches(repo):
transport = repo.bzrdir.root_transport
for fn in transport.iter_files_recursive():
@@ -806,9 +806,6 @@ def find_branches(repo, wanted):
name = name if name != '' else 'master'
name = name.replace('/', '+')
-if wanted and not name in wanted:
-continue
-
try:
cur = transport.clone(subdir)
branch = bzrlib.branch.Branch.open_from_transport(cur)
@@ -848,38 +845,35 @@ def get_repo(url, alias):
except bzrlib.errors.NoRepositoryPresent:
pass
-try:
-repo = origin.open_repository()
-if not repo.user_transport.listable():
-# this repository is not usable for us
-raise bzrlib.errors.NoRepositoryPresent(repo.bzrdir)
-except bzrlib.errors.NoRepositoryPresent:
-# branch
-
-name = 'master'
-branch = origin.open_branch().base
-
-if not is_local:
-peers[name] = branch
+wanted = get_config('remote-bzr.branches').rstrip().split(', ')
+# stupid python
+wanted = [e for e in wanted if e]
-branches[name] = branch
-
-return origin
+if not wanted:
+try:
+repo = origin.open_repository()
+if not repo.user_transport.listable():
+# this repository is not usable for us
+raise bzrlib.errors.NoRepositoryPresent(repo.bzrdir)
+except bzrlib.errors.NoRepositoryPresent:
+wanted = ['master']
+
+if wanted:
+def list_wanted(url, wanted):
+for name in wanted:
+subdir = name if name != 'master' else ''
+yield name, bzrlib.urlutils.join(url, subdir)
+
+branch_list = list_wanted(url, wanted)
else:
-# repository
-
-wanted = get_config('remote-bzr.branches').rstrip().split(', ')
-# stupid python
-wanted = [e for e in wanted if e]
+branch_list = find_branches(repo)
-for name, branch in find_branches(repo, wanted):
-
-if not is_local:
-peers[name] = branch
-
-branches[name] = branch
+for name, url in branch_list:
+if not is_local:
+peers[name] = url
+branches[name] = url
-return origin
+return origin
def fix_path(alias, orig_url):
url = urlparse.urlparse(orig_url, 'file')
--
1.8.3.rc3.312.g47657de
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
