On Thu, Dec 13, 2012 at 1:31 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Felipe Contreras <felipe.contreras@xxxxxxxxx> writes: > >> On Thu, Dec 13, 2012 at 2:11 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: >>> Felipe Contreras <felipe.contreras@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html