Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works

[Junio C Hamano <gitster@pobox.com>]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> 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.