Re: What's cooking in git.git (Dec 2012, #03; Wed, 12)

[Felipe Contreras <felipe.contreras@gmail.com>]

On Thu, Dec 13, 2012 at 1:31 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Dec 13, 2012 at 2:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>>>>  New remote helper for bzr (v3).  With minor fixes, this may be ready
>>>>>  for 'next'.
>>>>
>>>> What minor fixes?
>>>
>>> Lookng at the above (fixup), $gmane/210744 comes to mind
>>
>> That doesn't matter. The code and the tests would work just fine.
>
> One of us must be very confused.  Perhaps you were looking at a
> wrong message (or I quoted a wrong one?).
>
>   ... goes and double checks ...
>
> One of the review points were about this piece in the test:
>
>     > +cmd=<<EOF
>     > +import bzrlib
>     > +bzrlib.initialize()
>     > +import bzrlib.plugin
>     > +bzrlib.plugin.load_plugins()
>     > +import bzrlib.plugins.fastimport
>     > +EOF
>     > +if ! "$PYTHON_PATH" -c "$cmd"; then
>
>     I cannot see how this could have ever worked.
>
> And I still don't see how your "would work just fine" can be true.

As I have explained, all this code is the equivalent of python -c '',
or rather, it's a no-op. It works in the sense that it doesn't break
anything.

The purpose of the code is to check for the fastimport plug-in, but
that plug-in is not used any more, it's vestigial code, it doesn't
matter if the check works or not, as long as it doesn't fail.

>>> but there may be others.  It is the responsibility of a contributor to keep
>>> track of review comments others give to his or her patches and
>>> reroll them, so I do not recall every minor details, sorry.
>
> There may be others, but $gmane/210745 is also relevant, I think.

I'll comment on the patch, but I don't think it really prevents the merge.

>> There is nothing that prevents remote-bzr from being merged.
>
> What we merge may not be perfect.  Bugs and misdesigns are often
> identified long after a topic is merged and it is perfectly normal
> we fix things up in-tree.  There are even times where we say "OK, it
> is known to break if the user does something that pokes this and
> that obscure corners of this code, but the benefit of merging this
> 99% working code to help users that do not exercise the rare cases
> is greater than having them wait for getting the remaining 1% right,
> so let's merge it with known breakage documentation".
>
> But it is totally a different matter to merge a crap with known
> breakage that is one easy fix away from the get-go.  Allowing that
> means that all the times we spend on reviewing patches here go
> wasted, discouraging reviewers.

There is no breakage.

> If you want others to take your patches with respect, please take
> reviewers' comments with the same respect you expect to be paid by
> others.

I don't need others to take my patches with respect, my patches are
not people, they don't have feelings.

That being said, I don't think I have disrespected any of your
comments. Yes, you are right that the above code is wrong and doesn't
do anything, what part of agreeing is disrespectful? But I don't agree
that it is a merge blocker. Disagreeing is not disrespecting.

This code was ready for 1.8.1, but it's not going to be there, so, I
don't see any hurry. As I said, I think the code is ready, and these
minor details can wait.

Cheers.

-- 
Felipe Contreras