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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: May 29, 2013, 17:03 UTC
Message-ID: <CAMP44s2UCZQBfSkhc5jg-ipWKMy3M4R-YVODR4=OuAUbpgkD8A@mail.gmail.com>
In-Reply-To: <7v38t5wxee.fsf@alter.siamese.dyndns.org>

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