Re: [PATCH v2 7/8] remote-bzr: reorganize the way 'wanted' works
On Wed, May 29, 2013 at 11:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 33 quoted lines
> 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?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.
Show 18 quoted lines
>>> - 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.
Show 18 quoted lines
>>> - 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
Felipe Contreras