[PATCH 0/9] remote-helpers: fixes and cleanups

[PATCH 0/9] remote-helpers: fixes and cleanups

[Felipe Contreras]

Hi,

Here's a bunch of cleanups mostly to synchronize remote-bzr and remote-hg.

One of these might conflict with a series already in pu, if so, the code here
should be the prefered one.

Felipe Contreras (9):
  remote-bzr: trivial cleanups
  remote-hg: remove extra check
  remote-bzr: fix bad state issue
  remote-bzr: add support to push URLs
  remote-hg: use hashlib instead of hg sha1 util
  remote-bzr: store converted URL
  remote-hg: use python urlparse
  remote-bzr: tell bazaar to be quiet
  remote-bzr: strip extra newline

 contrib/remote-helpers/git-remote-bzr | 47 ++++++++++++++++++++++++++++++-----
 contrib/remote-helpers/git-remote-hg  | 17 ++++++-------
 2 files changed, 48 insertions(+), 16 deletions(-)

-- 
1.8.2.1

[PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index aa7bc97..82bf7c7 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -94,7 +94,7 @@ class Marks:
         return self.last_mark
 
     def is_marked(self, rev):
-        return self.marks.has_key(rev)
+        return rev in self.marks
 
     def new_mark(self, rev, mark):
         self.marks[rev] = mark
@@ -224,7 +224,7 @@ def export_files(tree, files):
             else:
                 mode = '100644'
 
-            # is the blog already exported?
+            # is the blob already exported?
             if h in filenodes:
                 mark = filenodes[h]
                 final.append((mode, mark, path))
@@ -521,7 +521,7 @@ def c_style_unescape(string):
     return string
 
 def parse_commit(parser):
-    global marks, blob_marks, bmarks, parsed_refs
+    global marks, blob_marks, parsed_refs
     global mode
 
     parents = []
@@ -555,7 +555,7 @@ def parse_commit(parser):
             mark = int(mark_ref[1:])
             f = { 'mode' : m, 'data' : blob_marks[mark] }
         elif parser.check('D'):
-            t, path = line.split(' ')
+            t, path = line.split(' ', 1)
             f = { 'deleted' : True }
         else:
             die('Unknown file command: %s' % line)
@@ -643,6 +643,7 @@ def do_export(parser):
                 wt = repo.bzrdir.open_workingtree()
                 wt.update()
         print "ok %s" % ref
+
     print
 
 def do_capabilities(parser):
-- 
1.8.2.1

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
> index aa7bc97..82bf7c7 100755
> --- a/contrib/remote-helpers/git-remote-bzr
> +++ b/contrib/remote-helpers/git-remote-bzr
> @@ -94,7 +94,7 @@ class Marks:
>          return self.last_mark
>
>      def is_marked(self, rev):
> -        return self.marks.has_key(rev)
> +        return rev in self.marks

Why?  Is the new form faster than the older one?

> @@ -224,7 +224,7 @@ def export_files(tree, files):
>              else:
>                  mode = '100644'
>
> -            # is the blog already exported?
> +            # is the blob already exported?

What is this?  Whitespace?

> @@ -521,7 +521,7 @@ def c_style_unescape(string):
>      return string
>
>  def parse_commit(parser):
> -    global marks, blob_marks, bmarks, parsed_refs
> +    global marks, blob_marks, parsed_refs

How is this trivial?  You just removed one argument.

> @@ -555,7 +555,7 @@ def parse_commit(parser):
>              mark = int(mark_ref[1:])
>              f = { 'mode' : m, 'data' : blob_marks[mark] }
>          elif parser.check('D'):
> -            t, path = line.split(' ')
> +            t, path = line.split(' ', 1)

How on earth is this trivial?  It changes the entire meaning!

> @@ -643,6 +643,7 @@ def do_export(parser):
>                  wt = repo.bzrdir.open_workingtree()
>                  wt.update()
>          print "ok %s" % ref
> +

Whitespace?

I'm outraged by this.  What kind of changes are you pushing to
remote-hg?  A "trivial cleanups" bundling miscellaneous changes, with
no commit message?  Why don't you just squash everything into one
"miscellaneous changes" patch?

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 1:19 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
>> index aa7bc97..82bf7c7 100755
>> --- a/contrib/remote-helpers/git-remote-bzr
>> +++ b/contrib/remote-helpers/git-remote-bzr
>> @@ -94,7 +94,7 @@ class Marks:
>>          return self.last_mark
>>
>>      def is_marked(self, rev):
>> -        return self.marks.has_key(rev)
>> +        return rev in self.marks
>
> Why?  Is the new form faster than the older one?

has_key is deprecated.

>> @@ -224,7 +224,7 @@ def export_files(tree, files):
>>              else:
>>                  mode = '100644'
>>
>> -            # is the blog already exported?
>> +            # is the blob already exported?
>
> What is this?  Whitespace?

s/blog/blob/

>> @@ -521,7 +521,7 @@ def c_style_unescape(string):
>>      return string
>>
>>  def parse_commit(parser):
>> -    global marks, blob_marks, bmarks, parsed_refs
>> +    global marks, blob_marks, parsed_refs
>
> How is this trivial?  You just removed one argument.

It's not an argument.

>> @@ -555,7 +555,7 @@ def parse_commit(parser):
>>              mark = int(mark_ref[1:])
>>              f = { 'mode' : m, 'data' : blob_marks[mark] }
>>          elif parser.check('D'):
>> -            t, path = line.split(' ')
>> +            t, path = line.split(' ', 1)
>
> How on earth is this trivial?  It changes the entire meaning!

And nobody has noticed any problem.

>> @@ -643,6 +643,7 @@ def do_export(parser):
>>                  wt = repo.bzrdir.open_workingtree()
>>                  wt.update()
>>          print "ok %s" % ref
>> +
>
> Whitespace?

Aka. trivial.

> I'm outraged by this.  What kind of changes are you pushing to
> remote-hg?  A "trivial cleanups" bundling miscellaneous changes, with
> no commit message?

There are no miscellaneous changes other than the *possible* fix for
deleted files. Which we don't know if it would actually fix anything,
but as far as we know if it's a bug, nobody has seen it, and if it
isn't, it's very unlikely that anybody is relying on the current
behavior.

Plus the change seems to be obviously correct, as it comes from
remote-hg, where somebody appeared to have found a bug.

That being said, I do remember writing an explanation for this in the
commit message:

--
commit ca8c02dc7ea6395b1c864296f2500b718892fab8
Reflog: HEAD@{143} (Felipe Contreras <felipe.contreras@gmail.com>)
Reflog message: rebase -i (fixup): remote-bzr: trivial cleanups
Author: Felipe Contreras <felipe.contreras@gmail.com>
Date:   Tue Apr 23 18:29:49 2013 -0500

    remote-bzr: trivial cleanups

    Mostly from remote-hg. It's possible that there's a fix to delete files
    with spaces.

    Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

Yeap, there it is. It was just squashed by mistake.

But I do not care that much really. The patch is good either way, if
you don't like it, you go ahead and fix it, because I won't. I have
174 remote-helper related patches in my queue, and nobody benefits
from rambling about a one liner that is obviously correct, not you,
not me, not the users, not the developers.

Junio of course might disagree and drop this patch, but then he would
need to deal with the fallout of possible conflicts. Or he can do the
sensible thing and pick the commit message above. I have real issues
to deal with, and I think the less-than-perfect commit messages in a
*contrib* script that is extremely recent is a small price to pay for
having nice and workable bzr and mercurial remote-helpers as soon as
possible; our users would thank us, and in fact, they already are.

In my hurry to reorganize all the commits of my fourteen remote-helper
branches, I squashed the commit message of a trivial fix into trivial
cleanups. Big whooping deal.

> Why don't you just squash everything into one
> "miscellaneous changes" patch?

Hyperbole much?

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Thomas Rast]

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

> But I do not care that much really. The patch is good either way, if
> you don't like it, you go ahead and fix it, because I won't. I have
> 174 remote-helper related patches in my queue, and nobody benefits
> from rambling about a one liner that is obviously correct, not you,
> not me, not the users, not the developers.

You don't stick to the rules of this project, which have been pointed
out already:

  The body should provide a meaningful commit message, which:

    . explains the problem the change tries to solve, iow, what is wrong
      with the current code without the change.

    . justifies the way the change solves the problem, iow, why the
      result with the change is better.

    . alternate solutions considered but discarded, if any.

Your project is moving too fast to put up with the established
procedures in this community.

In fact you are pretty much holding us hostage with a "take it or keep
it broken while causing more work" attitude:

> Junio of course might disagree and drop this patch, but then he would
> need to deal with the fallout of possible conflicts.

You did not respond well to reviews and criticism.  Even the
constructive fine-let's-do-the-work-for-him kind that Peff offered.

And on top of that, remote helpers are written against an interface that
was designed to allow working with external programs.

So why is this in git.git?

Why should we take any more contrib additions from you?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 3:30 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> But I do not care that much really. The patch is good either way, if
>> you don't like it, you go ahead and fix it, because I won't. I have
>> 174 remote-helper related patches in my queue, and nobody benefits
>> from rambling about a one liner that is obviously correct, not you,
>> not me, not the users, not the developers.
>
> You don't stick to the rules of this project, which have been pointed
> out already:

The rules of the contrib area are different from the ones of the rest
of the project.

> Your project is moving too fast to put up with the established
> procedures in this community.

That's one of the reasons it's in the contrib area.

> In fact you are pretty much holding us hostage with a "take it or keep
> it broken while causing more work" attitude:

I'm the maintainer of this code, so it's my call. If Junio has a
problem with that, I would gladly take my code somewhere else. I doubt
that's in the best interest of anyone.

But if the problem is this particular patch (reaally?), Junio could
just drop this particular patch. Are you seriously suggesting that the
whole contrib/remote-helpers should be dropped because this patch
introduces a one-liner fix without mentioning it in the commit
message? Really? I haven't seen anybody complain about *any* of the
other patches where I "held the project hostage" and refused to fix
the commit message or change the patch.

Other than this instance, show me where exactly did I do that.

>> Junio of course might disagree and drop this patch, but then he would
>> need to deal with the fallout of possible conflicts.
>
> You did not respond well to reviews and criticism.  Even the
> constructive fine-let's-do-the-work-for-him kind that Peff offered.

Define "respond well". If your idea to "respond well" is to say "Yes
sir!" to every criticism, then no, I didn't. OTOH, if it's to reply
and address the issues with objective reasoning and an open mind, I
did.

I don't understand this notion that every review criticism is valid
and correct. They are not, and it's OK to point that out.. really. If
they turn to be valid and correct, the reviewer can surely
counter-argue and substantiate his/her claims.

And I don't recall Peff ever doing this "constructive
fine-let's-do-the-work-for-him" on any contrib/remote-helpers stuff.

> So why is this in git.git?
>
> Why should we take any more contrib additions from you?

Because it's good for the users.

If you are seriously suggesting to drop contrib/remote-helpers, I
suggest that 1) don't do it in the review thread of a trivial patch 2)
start a new thread where you point multiple instances where the
maintainer of the code (me) failed to respond correctly to criticism
(of remote-helpers's code), 3) show how this affects negatively the
project, and 4) ask for new maintainers if the job of the current one
is not deemed up-to-par, and only if no maintainer steps up, drop the
code.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Junio C Hamano]

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

> On Thu, Apr 25, 2013 at 3:30 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> But I do not care that much really. The patch is good either way, if
>>> you don't like it, you go ahead and fix it, because I won't. I have
>>> 174 remote-helper related patches in my queue, and nobody benefits
>>> from rambling about a one liner that is obviously correct, not you,
>>> not me, not the users, not the developers.
>>
>> You don't stick to the rules of this project, which have been pointed
>> out already:
>
> The rules of the contrib area are different from the ones of the rest
> of the project.

Yes and no. 

A contrib/ material may not be held to the same high standard, but
that does not mean a contrib/ area maintainer has a blank check to
do anything there.

It would be pretty obvious to people observing what happens in the
area after a while, if the quality standard the area maintainer
enforces is too out of whack, and at that point the area maintainer
deserves to be ridiculed ;-)

> And I don't recall Peff ever doing this "constructive
> fine-let's-do-the-work-for-him" on any contrib/remote-helpers stuff.

I do not think Thomas was talking specific about contrib/ material
but your interaction in general with other developers.

Cf. http://thread.gmane.org/gmane.comp.version-control.git/220427/focus=220891

FWIW, I thought "that person was me" response from him was more than
reasonable, and I still do.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 4:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Apr 25, 2013 at 3:30 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> But I do not care that much really. The patch is good either way, if
>>>> you don't like it, you go ahead and fix it, because I won't. I have
>>>> 174 remote-helper related patches in my queue, and nobody benefits
>>>> from rambling about a one liner that is obviously correct, not you,
>>>> not me, not the users, not the developers.
>>>
>>> You don't stick to the rules of this project, which have been pointed
>>> out already:
>>
>> The rules of the contrib area are different from the ones of the rest
>> of the project.
>
> Yes and no.
>
> A contrib/ material may not be held to the same high standard, but
> that does not mean a contrib/ area maintainer has a blank check to
> do anything there.

Nor did I claim I had one.

> It would be pretty obvious to people observing what happens in the
> area after a while, if the quality standard the area maintainer
> enforces is too out of whack, and at that point the area maintainer
> deserves to be ridiculed ;-)

Of course, but the claim the rules are different still stands.

>> And I don't recall Peff ever doing this "constructive
>> fine-let's-do-the-work-for-him" on any contrib/remote-helpers stuff.
>
> I do not think Thomas was talking specific about contrib/ material
> but your interaction in general with other developers.
>
> Cf. http://thread.gmane.org/gmane.comp.version-control.git/220427/focus=220891

Yeah but how is that relevant in this context? We are talking about a
particular patch of remote-bzr. And he immediately used that claim as
ammunition to suggest the whole remote-helpers should be dropped. It
does not follow.

Any suggestion to drop remote-helpers should use facts and arguments
regarding remote-helpers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Junio C Hamano]

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

> But I do not care that much really. The patch is good either way, if
> you don't like it, you go ahead and fix it, because I won't. I have
> 174 remote-helper related patches in my queue, and nobody benefits
> from rambling about a one liner that is obviously correct, not you,
> not me, not the users, not the developers.

Three random points.

 * For this particular patch [1/9], especially because this would
   land close to the corresponding remote-hg fixes (e.g. "has_key is
   deprecated"), I think it is sufficient to say "port fixes from
   corresponding remote-hg patches" (you said it in 0/9 and didn't
   say it in 1/9, though) without going into individual details.
   Anybody who wonders what these changes were about will have a
   clue to check contemporary patches to remote-hg that way.

 * You may want to hold onto those 174 patches and polish their
   explanation up to save the list audiences' time by avoiding this
   kind of useless "why no explanation" exchanges.

 * If you do not want to keep a readable history, it would mean that
   nobody but you will fix problems discovered in the future in
   remote-hg, and there is no point carrying it in my tree for other
   Git developers to look at it.  The users are better off getting
   them from your tree and that will make it clear for them whom to
   ask help/fix for when they hit a snag.

> Junio of course might disagree and drop this patch, but then he would
> need to deal with the fallout of possible conflicts.

A much more sensible thing in such a case for me to do actually is
to drop the whole thing. I do not want to do that unless necessary.

> ... I think the less-than-perfect commit messages in a
> *contrib* script that is extremely recent is a small price to pay for
> having nice and workable bzr and mercurial remote-helpers as soon as
> possible

I do not share this view at all. The users survived without it long
enough; they can wait for a well maintained version.  On the other
hand, shipping something that will not be maintainable is not the
way to help end users. It is being irresponsive to them.

Helping other developers understand your code is a way to ensure
that your code that would help users will be kept maintained.  I do
not agree with Ram at all when he says that developers are more
important than users, and I agree with you that the project exists
for users, and not for developers.  But you need to help your fellow
developers anyway by spending effort to keep your history readable,
in order to help them help the users.

Do not take the "users matter" mantra to the extreme. You need other
developers to put users first.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 3:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> But I do not care that much really. The patch is good either way, if
>> you don't like it, you go ahead and fix it, because I won't. I have
>> 174 remote-helper related patches in my queue, and nobody benefits
>> from rambling about a one liner that is obviously correct, not you,
>> not me, not the users, not the developers.
>
> Three random points.
>
>  * For this particular patch [1/9], especially because this would
>    land close to the corresponding remote-hg fixes (e.g. "has_key is
>    deprecated"), I think it is sufficient to say "port fixes from
>    corresponding remote-hg patches" (you said it in 0/9 and didn't
>    say it in 1/9, though) without going into individual details.
>    Anybody who wonders what these changes were about will have a
>    clue to check contemporary patches to remote-hg that way.

I don't see the value of pointing that out in the particular commit,
since you are the only one that would do anything with that
information, and it seems the message came across.

If there's any issues with that, just drop the patch, and if there's
issues with the rest of the series, just drop them. I'll resend when
the stuff is merged to master.

>  * You may want to hold onto those 174 patches and polish their
>    explanation up to save the list audiences' time by avoiding this
>    kind of useless "why no explanation" exchanges.

That's exactly what I've been doing.

You are extrapolating from this particular patch, which I already
admitted I made a mistake, and it's not really important in any way.

>  * If you do not want to keep a readable history, it would mean that
>    nobody but you will fix problems discovered in the future in
>    remote-hg, and there is no point carrying it in my tree for other
>    Git developers to look at it.  The users are better off getting
>    them from your tree and that will make it clear for them whom to
>    ask help/fix for when they hit a snag.

The history *is* readable. If anybody has any problems with the commit
messages, the place to mention such problems is IN THE PATCH REVIEW.
Nobody has done that, because either nobody has any problems, or they
are not interested. Either way, there's nothing I can do about it.

*This* patch is an exception, and I'm not willing to waste time on
this extremely trivial patch. Drop it.

>> Junio of course might disagree and drop this patch, but then he would
>> need to deal with the fallout of possible conflicts.
>
> A much more sensible thing in such a case for me to do actually is
> to drop the whole thing. I do not want to do that unless necessary.

You want to drop the whole series because of a cleanup patch with a
less-than-perfect commit message? Even though there quite likely won't
be any conflicts if you drop the single patch. Fine, drop the whole
series.

>> ... I think the less-than-perfect commit messages in a
>> *contrib* script that is extremely recent is a small price to pay for
>> having nice and workable bzr and mercurial remote-helpers as soon as
>> possible
>
> I do not share this view at all. The users survived without it long
> enough; they can wait for a well maintained version.  On the other
> hand, shipping something that will not be maintainable is not the
> way to help end users. It is being irresponsive to them.

Are you saying that because *ONE PATCH*, introduces a fix without
mentioning it in the commit message, *THE WHOLE* project becomes
unmaintainable?

If not, then why are we discussing about something that is not happening?

> Helping other developers understand your code is a way to ensure
> that your code that would help users will be kept maintained.  I do
> not agree with Ram at all when he says that developers are more
> important than users, and I agree with you that the project exists
> for users, and not for developers.  But you need to help your fellow
> developers anyway by spending effort to keep your history readable,
> in order to help them help the users.

And I am. Because I made a mistake in this patch doesn't mean the same
happened in all the patches.

I am helping my fellow developers by replying to the comments they
make when I send the patches for review. Unfortunately, the only
developer other than you that has made any comment at all, Ramkumar
Ramachandra, did so in a bellicose tone, but I replied to all his
comments either way, which where invalid. The only comment where he is
right and I acknowledged making a small mistake, is trivial, does not
cause any issues, and can be easily dropped.

> Do not take the "users matter" mantra to the extreme. You need other
> developers to put users first.

No, I don't. It would be nice, yes, but not necessary.

Now, let's drop this pointless discussion and deal with the actual
issue. What do you want to do?

1) Drop this patch
2) Drop the whole series
3) I reroll without the change that was not described

Anything else, I'm not interested in doing. There's tasks with actual
value to do.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Junio C Hamano]

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

>> Three random points.
>>
>>  * For this particular patch [1/9], especially because this would
>>    land close to the corresponding remote-hg fixes (e.g. "has_key is
>>    deprecated"), I think it is sufficient to say "port fixes from
>>    corresponding remote-hg patches" (you said it in 0/9 and didn't
>>    say it in 1/9, though) without going into individual details.
>>    Anybody who wonders what these changes were about will have a
>>    clue to check contemporary patches to remote-hg that way.
>
> If there's any issues with that, just drop the patch,...
> ...
> 1) Drop this patch
> 2) Drop the whole series
> 3) I reroll without the change that was not described

Just in case you missed it, the first in the three-random-points was
"I personally think 1/9 that does not say anything about the minute
and irrelevant details Ram kibitzed about is fine".  So "Drop this
patch" is not something on the table in the first place.

 * After seeing that this change is a copy from recent remote-hg
   changes, a revier who did a little homework would easily find a
   change around has_key in recent patches.

 * A reviewer who did a little homework would know by reading a bit
   beyond the patch context to see that nobody uses "bmarks".

 * A reviewer who wondered how the two lines are different can stop
   staring at the screen, take a walk and come back with refreshed
   eyes to spot the difference between blog and blob very easily.

For these reasons, I personally do not think it is unreasonable to
throw comments like the ones on "has_key", "global bmarks", and
"blog vs blob" into "too obvious, not even deserve to be responded"
bin.

Having said that, I am more worried about wasting everybody's time
(and this includes your time) with the impedance mismatch between
you and the rest of us.

Our standard for explaining the change (either in the log or in the
comment) is to err on the descriptive side to be helpful even to
people new to the codebase.  We do not require or encourage to state
the obvious. The issue is the definition of "obviousness" varies
even among the rest of us and even for a single person depending on
how familiar that person is with the area of the code in question.
But the divide between you (alone) and the rest of us seems to be
far more vast than differences among the people other than you.

Especially the criteria I used in the above example for "bmarks"
need to be used carefully.  If a reviewer needs to follow a very
deep callchain to convince himself why a change does not break
things, it is no longer obvious and deserves to be explained.

So I dunno.  If you are not willing to change your ways and try to
be more descriptive to help others to understand what you are doing,
there is nothing I can do to help you.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 5:01 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Having said that, I am more worried about wasting everybody's time
> (and this includes your time) with the impedance mismatch between
> you and the rest of us.
>
> Our standard for explaining the change (either in the log or in the
> comment) is to err on the descriptive side to be helpful even to
> people new to the codebase.  We do not require or encourage to state
> the obvious. The issue is the definition of "obviousness" varies
> even among the rest of us and even for a single person depending on
> how familiar that person is with the area of the code in question.
> But the divide between you (alone) and the rest of us seems to be
> far more vast than differences among the people other than you.

You are missing my point, this is *ONE INSTANCE*. Show me another
instance where a reviewer complained about the lack of a descriptive
commit messages on *remote-helpers*.

> Especially the criteria I used in the above example for "bmarks"
> need to be used carefully.  If a reviewer needs to follow a very
> deep callchain to convince himself why a change does not break
> things, it is no longer obvious and deserves to be explained.

So if I'm not willing to describe every little trivial cleanup change
I do, what should I do then? Avoid those trivial changes?

If your true purpose of having descriptive commit messages is to
improve maintainability, then actually doing these cleanups should
have priority over a descriptive commit message, because doing the
cleanups improves the maintainability even without a detailed
description.

Clearly, your reasoning is incomplete.

> So I dunno.  If you are not willing to change your ways and try to
> be more descriptive to help others to understand what you are doing,
> there is nothing I can do to help you.

I'm willing to change my ways when there's reason to change my ways,
and so far, nobody has provided any evidence that my commit messages
are indeed lacking, only *opinions*.

Other people are perfectly fine with them:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/?qt=author&q=felipe.contreras

And the only reason we are wasting time, is that *you* make us waste
time. Any sensible reviewer would be context aware, notice that this
is a contrib patch, and focus on behavioral changes, notice the
mistake I made, and point that *one* of the changes was changing the
behavior, at which point I would agree and reroll either without that
change, or with the change in a separate commit (which I don't want to
do right now). The maintainer (you), wouldn't even have to reply at
all.

But the reviewer failed to do so, and other contributors went even
further, so the ball is in now in your court. IMO a sensible
maintainer would simply say "Guys, stay on topic, what do we do with
this patch?", but no, you allow people to suggest that not only the
whole series, but the whole sub-project be dropped, and to do so with
totally unrelated facts, and generalizing from *ONE INSTANCE* in the
actual sub-project, and generally from ad hominem arguments.

This doesn't help anybody.

Show me a systemic problem with the commit messages *in
remote-helpers*, and then perhaps it would be worth to start *a new
thread* to discuss them, but nobody has done so. We are still talking
about a *single patch*.

And if you really really don't like the patch, say "do X, or I drop
the patch", or the series, and there would be no need for other
reviewers to waste their time (if their comments were truly valid and
correct, which they are not). There's no need to say anything more.
And even if the reviewers were correct in their comments, allowing
suggestions such as that the whole sub-project should be dropped
because of one patch is going to waste people's times, no matter what.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Junio C Hamano]

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

> You are missing my point, this is *ONE INSTANCE*. Show me another
> instance where a reviewer complained about the lack of a descriptive
> commit messages on *remote-helpers*.

You are the one who is missing the point.  My message was about your
patches to _any_ part of our system, not limited to remote helpers.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 6:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> You are missing my point, this is *ONE INSTANCE*. Show me another
>> instance where a reviewer complained about the lack of a descriptive
>> commit messages on *remote-helpers*.
>
> You are the one who is missing the point.  My message was about your
> patches to _any_ part of our system, not limited to remote helpers.

I still see "Re: [PATCH 1/9] remote-bzr: trivial cleanups", if we are
talking about something else, let's do so and be clear in the subject,
but I don't see what this has to do with this trivial patch, the
series, or even remote-bzr in general (for which nobody has complained
bout commit messages before).

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> Any sensible reviewer would be context aware, notice that this
> is a contrib patch, and focus on behavioral changes, notice the
> mistake I made, and point that *one* of the changes was changing the
> behavior, at which point I would agree and reroll either without that
> change, or with the change in a separate commit (which I don't want to
> do right now). The maintainer (you), wouldn't even have to reply at
> all.

Personally, I think it is the job of the submitter to provide a really
helpful commit message and widen his review audience.  If I'm hitting
the git mailing list with my patches, I try to make sure that nearly
everyone on the list can understand what I've done and potentially
review it.  Why else would I want to hit their inboxes with my
patches?

Here's my solution to the problem: maintain your project outside
git.git and merge changes in every couple of months or so with a
simple email containing a pull URL, addressing Junio.  If Junio trusts
you enough to put the changes you send into contrib/ after a cursory
glance, we're done.  Start a separate mailing list for your project/
accept GitHub pull requests via which contributors can send you
changes.  No more fuss or drama on the git list about this.  You can
be as stubborn as you want, and we go back to our lives.  Everyone
wins.

If you want to submit patches to other parts of git, you seriously
need to change your ways.  Let's deal with that problem when it arises
next.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 7:19 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> Any sensible reviewer would be context aware, notice that this
>> is a contrib patch, and focus on behavioral changes, notice the
>> mistake I made, and point that *one* of the changes was changing the
>> behavior, at which point I would agree and reroll either without that
>> change, or with the change in a separate commit (which I don't want to
>> do right now). The maintainer (you), wouldn't even have to reply at
>> all.
>
> Personally, I think it is the job of the submitter to provide a really
> helpful commit message and widen his review audience.  If I'm hitting
> the git mailing list with my patches, I try to make sure that nearly
> everyone on the list can understand what I've done and potentially
> review it.  Why else would I want to hit their inboxes with my
> patches?

If you don't understand the reasoning and history behind remote-bzr,
you might be doing a disservice to everyone by commenting at all.

Bazaar is a dead project, and there are *real* users suffering as we
speak, bound to eternal SCM torment by evil dictators and political
non-speak. Even the worst of remote-bzr patches are a thousand times
better than what you see in bzr code itself.

To give you some perspective, one commit broke a branch in the emacs
project, and ever since then people are not able to clone that branch.
This bug has been known for years, and nobody fixes it. Every time
anybody tries to clone that branch, they need a special sequence of
commands.

They *need* something like remote-bzr to escape the horrendities of
bzr, and all you are doing complaining about a sneaked fix is a
disservice to everyone. Yes, doing such a thing on git.c would not be
particularly great, but wouldn't be horrific either, fortunately we
are not doing that!

Answer me, do you use bzr? No? Do you use remote-bzr? No? Then how in
hell could you possibly have any contextual information to make even a
guess as to what would be the impact of sneaking such a small fix? You
can't.

But why are we even speaking about this nonsense? This patch has been
dropped. You want to review something, go review PATCH v2 1/9. Stop
arguing about stubbornness and hypotheticals, when there's actual code
to review.

What is your objective, do you want to help this project move forward or not?

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> What is your objective, do you want to help this project move forward or not?

Forward, please.

I want a solution to this persistent problem of conflict though.  And
I presented one in my previous email:

Here's my solution to the problem: maintain your project outside
git.git and merge changes in every couple of months or so with a
simple email containing a pull URL, addressing Junio.  If Junio trusts
you enough to put the changes you send into contrib/ after a cursory
glance, we're done.  Start a separate mailing list for your project/
accept GitHub pull requests via which contributors can send you
changes.  No more fuss or drama on the git list about this.  You can
be as stubborn as you want, and we go back to our lives.  Everyone
wins.

I'll probably even contribute small patches once in a while.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 1:53 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> What is your objective, do you want to help this project move forward or not?
>
> Forward, please.
>
> I want a solution to this persistent problem of conflict though.  And
> I presented one in my previous email:
>
> Here's my solution to the problem: maintain your project outside
> git.git and merge changes in every couple of months or so with a
> simple email containing a pull URL, addressing Junio.  If Junio trusts
> you enough to put the changes you send into contrib/ after a cursory
> glance, we're done.  Start a separate mailing list for your project/
> accept GitHub pull requests via which contributors can send you
> changes.  No more fuss or drama on the git list about this.  You can
> be as stubborn as you want, and we go back to our lives.  Everyone
> wins.

I already maintain my own clone outside of git.git[1], and I do
already accept pull requests[2], and people have sent me patches
directly. The stuff I send to the git mailing list is what I think is
ready for merging. But there's only so much stuff I can catch.

We all benefit from these patches being reviewed in the git mailing
list, nobody has claimed otherwise. You are making the error of
assuming that your review was actionable, that I should have done
something, fix the commit message I suppose, but I don't think that's
important.

In contrast, this is how a constructive, valid and helpful review looks like:

http://article.gmane.org/gmane.comp.version-control.git/220034

Junio caught a problem I didn't see, I accepted the valid feedback,
and I resent with a fix. We all benefit from such interactions, both
users, and developers. What's wrong with that?

You just got angry that your review didn't turn out to be helpful, is
that it? Why do you want to steal helpful review from the users of
remote-{bzr,hg}? If that's not the case, please stop doing that. All
review is welcome, not all review should be acted upon.

Cheers.

[1] https://github.com/felipec/git
[2] https://github.com/felipec/git/pulls?direction=desc&page=1&sort=created&state=closed

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> We all benefit from these patches being reviewed in the git mailing
> list, nobody has claimed otherwise. You are making the error of
> assuming that your review was actionable, that I should have done
> something, fix the commit message I suppose, but I don't think that's
> important.

What I'm saying is that you can get more eyes.  A lot more eyes.  If
you just write a proper commit message!

Why are you hitting everyone's inboxes with such cryptic patches that
require either:
1. The reviewer to trust what you've done and move on.
2. The reviewer to do a lot of digging before the patch becomes
accessible to her.

> You just got angry that your review didn't turn out to be helpful, is
> that it? Why do you want to steal helpful review from the users of
> remote-{bzr,hg}? If that's not the case, please stop doing that. All
> review is welcome, not all review should be acted upon.

I'm not angry about anything, or trying to steal anything.

What happened:  New email.  Felipe's remote-hg fixes.  Okay, let's
look at this.  Part 1.  What?!  [I wrote down what I was thinking as I
was reading the email]

This is where you _should_ apply reason: justify everything you've
done in the patch in your commit message.  Why are you so stubborn
about not wanting to change your ways despite so many people telling
you?  Is it your pride*?

* Yes, I noticed that you have a huge ego.  I consider it an undesirable trait.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 2:56 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> We all benefit from these patches being reviewed in the git mailing
>> list, nobody has claimed otherwise. You are making the error of
>> assuming that your review was actionable, that I should have done
>> something, fix the commit message I suppose, but I don't think that's
>> important.
>
> What I'm saying is that you can get more eyes.  A lot more eyes.  If
> you just write a proper commit message!

It is a trivial obvious patch that changes very few lines of code, it
doesn't need more eyes in my opinion. For the patches that do need
eyes I do send descriptive commit messages. And some patches that I
don't think need eyes, but I think I could do wrong, I also write
descriptive messages.

So far, it looks like I was right in thinking this patch didn't need
more eyes. And I think my original commit description, which I deleted
by mistake was more than enough "Mostly from remote-hg. It's possible
that there's a fix to delete files with spaces". Because so far,
nobody has pointed out any actual issue, and it's not clear if this
patch would benefit from even more eyes... probably won't.

> Why are you hitting everyone's inboxes with such cryptic patches that
> require either:
> 1. The reviewer to trust what you've done and move on.
> 2. The reviewer to do a lot of digging before the patch becomes
> accessible to her.

Because there's no other way to get the changes into git.git. If you
wan't I can add "DO NOT REVIEW" in the title, but I think "trivial
cleanups" pretty much sums that what I feel, and actually I wouldn't
want people to _not_ review the patches, but rather to understand that
I think they are trivial, and shouldn't worry too much about them.

>> You just got angry that your review didn't turn out to be helpful, is
>> that it? Why do you want to steal helpful review from the users of
>> remote-{bzr,hg}? If that's not the case, please stop doing that. All
>> review is welcome, not all review should be acted upon.
>
> I'm not angry about anything, or trying to steal anything.

Good, so I'll keep sending the patches, because our users benefit from
the review.

> What happened:  New email.  Felipe's remote-hg fixes.  Okay, let's
> look at this.  Part 1.  What?!  [I wrote down what I was thinking as I
> was reading the email]

Write what you see, not what you feel. Your questions about the code
are fine, but making assumptions about what remote-bzr users must be
suffering by not having more descriptive commit messages are not. You
also assumed that I wanted to send that commit message, when that was
not true, I removed a chunk by mistake.

In general, you shouldn't make assumptions.

> This is where you _should_ apply reason: justify everything you've
> done in the patch in your commit message.  Why are you so stubborn
> about not wanting to change your ways despite so many people telling
> you?  Is it your pride*?

Stop asking these questions, I thought you already agreed this patch
was not worth discussing about. If you see *any other* patch that
doesn't have a good enough commit message, reply _there_. And if you
do want to pursue these questions irrespective of this patch, start a
new thread.

> * Yes, I noticed that you have a huge ego.  I consider it an undesirable trait.

I don't think so, but even if I did, it doesn't matter, all that
matters is that my arguments are sound and valid, you should
concentrate on the ball, not on the man. The fact that I believe my
arguments are valid and sound doesn't make me egotistic, it might be
that they are actually valid and sound, and I'm simply assessing them
correctly. I of course I'm willing to admit otherwise, based on
evidence, and reason.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Junio C Hamano]

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

> Good, so I'll keep sending the patches, because our users benefit from
> the review.

Just for the record, a patch sent to the list which nobody bothered
to read does not really count as reviewed.

You can either

 (1) pace yourself when people are otherwise busy; or

 (2) send them anyway but not claim "this was sent to the list two
     weeks ago, nobody complained, so it must be perfect" when it is
     not picked up after a few weeks.

Often (1) is a better strategy, as people who wanted to review but
otherwise were busy tend to declare patch bankruptcy after their
busy period ends.

Also, a reason that a patch goes uncommented is when it is difficult
to judge.  A patch with code change without sufficient explanation
behind the motivation to justify the change, a reviewer finds it
much harder to convince himself that the patch is a good change, and
it also is much harder to find which part of the change is wrong and
offer improvements, compared to a patch with the same change that is
justified properly.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Good, so I'll keep sending the patches, because our users benefit from
>> the review.
>
> Just for the record, a patch sent to the list which nobody bothered
> to read does not really count as reviewed.

No, but I did my part, which is sending them for review.

> You can either
>
>  (1) pace yourself when people are otherwise busy; or

I would, if there was a reason to.

>  (2) send them anyway but not claim "this was sent to the list two
>      weeks ago, nobody complained, so it must be perfect" when it is
>      not picked up after a few weeks.

When have I ever done that?

> Often (1) is a better strategy, as people who wanted to review but
> otherwise were busy tend to declare patch bankruptcy after their
> busy period ends.

Not for remote-{bzr,hg}. I've yet to see anybody claim they would
review the patches thoroughly, if only they were given time. I've yet
to see anybody claim they would review the patches thoroughly under
any circumstance at all. And by that I mean the patches that really
would benefit from reviewing.

> Also, a reason that a patch goes uncommented is when it is difficult
> to judge.  A patch with code change without sufficient explanation
> behind the motivation to justify the change, a reviewer finds it
> much harder to convince himself that the patch is a good change, and
> it also is much harder to find which part of the change is wrong and
> offer improvements, compared to a patch with the same change that is
> justified properly.

Yes, but is that the case *HERE*? And no, single line changes that are
trivial and obvious don't count. Show me an important patch that
surely would benefit from reviewing that doesn't have sufficient
explanation. Show me an important patch that anybody is not convinced
is a good patch. In the remote-{hg,bzr} context.

If there isn't any, I don't see why remote-{bzr,hg} should slow down.

For this patch, I don't care one iota.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> If you don't understand the reasoning and history behind remote-bzr,
> you might be doing a disservice to everyone by commenting at all.

Felipe, I'm trying to help.  If you think my review lacked context,
you can write me a paragraph/ link me to an email and I will read it.

It's not reviewers and submitters "attacking" each other.  It's
helping out other rational people in the community because you care
for their reviews.  Don't practice exclusivity and label some people
as "not eligible to review".  That's not a good way to develop.

> Bazaar is a dead project, and there are *real* users suffering as we
> speak, bound to eternal SCM torment by evil dictators and political
> non-speak. Even the worst of remote-bzr patches are a thousand times
> better than what you see in bzr code itself.
>
> To give you some perspective, one commit broke a branch in the emacs
> project, and ever since then people are not able to clone that branch.
> This bug has been known for years, and nobody fixes it. Every time
> anybody tries to clone that branch, they need a special sequence of
> commands.
>
> They *need* something like remote-bzr to escape the horrendities of
> bzr, and all you are doing complaining about a sneaked fix is a
> disservice to everyone. Yes, doing such a thing on git.c would not be
> particularly great, but wouldn't be horrific either, fortunately we
> are not doing that!

My God.  This is horror story.

> Answer me, do you use bzr? No? Do you use remote-bzr? No? Then how in
> hell could you possibly have any contextual information to make even a
> guess as to what would be the impact of sneaking such a small fix? You
> can't.

No Felipe, I don't use bzr.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> I am helping my fellow developers by replying to the comments they
> make when I send the patches for review. Unfortunately, the only
> developer other than you that has made any comment at all, Ramkumar
> Ramachandra, did so in a bellicose tone, but I replied to all his
> comments either way, which where invalid.

I've never wanted to pick fights with anyone, and I don't foresee
having a desire to do so in the future.  I was just saying what was on
my mind, which is along the lines of: you have written this patch with
the attitude "I know what I'm doing, my users will benefit, and nobody
else is going to look at this patch anyway"; I'm worried about what
your other patches look like if this is your attitude towards
development.  Junio is harping about the same thing: the impedance
mismatch between you and the rest of us.

> The history *is* readable. If anybody has any problems with the commit
> messages, the place to mention such problems is IN THE PATCH REVIEW.
> Nobody has done that, because either nobody has any problems, or they
> are not interested. Either way, there's nothing I can do about it.

That's what I've been trying to say over and over again: _why_ are
people not reviewing your patches?

0. Because nobody has any problems with them.

1. Because nobody on the git list cares about remote-hg.

2. Because you're stubborn as a mule, and the resulting thread often
results in long-winded discussions like this one (which wastes
everyone's time).  Therefore, the potential reviewer's reasoning is
that their review time is better spent elsewhere, where their review
is actually appreciated.

Hint: it's not (0).

If you're claiming that (1) is the case, then why are you posting to
the git list and hitting everyone's inboxes?  Maintain your project
outside git.

I'm claiming that it's (2).  In which case, it's you who needs changing.

> I'm willing to change my ways when there's reason to change my ways,
> and so far, nobody has provided any evidence that my commit messages
> are indeed lacking, only *opinions*.

You want a formal mathematical proof?  We operate on opinions, and
freeze what we think we all agree with into "community guidelines".

> Other people are perfectly fine with them:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/?qt=author&q=felipe.contreras

So you're now claiming that we're the ones at fault (Peff, Thomas,
Junio, and me, among others).  Okay, so why are you forcing your
changes and opinions down our throats?  You're in the wrong community:
join a community of people who are more like you (or start your own
project), and stop wasting our time.

Junio C Hamano wrote:
> I do
> not agree with Ram at all when he says that developers are more
> important than users, and I agree with you that the project exists
> for users, and not for developers.

On this.

If Peff were to suddenly stop working on git one day (because he was
frustrated with the community/ development practices), we'd all lose a
lot more than if one random end-user switches to using hg for his tiny
personal projects.  I'm _not_ claiming that there's a split between
users and users that are developers (we have one mailing list for
everyone, and I like that).  What I'm claiming is that we cannot (and
should not) pay equal attention to every user of git.  Some users are
more important than others.  Again, that does _not_ mean that we push
a change that benefits one important user but breaks everyone else's
setup.

Ofcourse the project exists for its users; we're not doing research.
However, we don't all have to write tutorials to keep in touch with
end-users who are completely detached from the development process
(our time is better spent elsewhere), or even have an
end-user-friendly bug tracker (where the SNR is very low).  We don't
have to consciously reach out to people we're not connected to
directly: if we're all sufficiently connected to the real world, the
itches/ bugs worth working on will always find their way to us.  We
live in a connected world.

Yes, I know.  You're going to respond to this email arguing about why
you're Right and why I (and everyone else) is Wrong, either by quoting
what Linus (TM) said and twisting it to mean what you want, belaboring
over what you've already said, or something similar.

I've given up on you, and I suspect a lot of other people have too.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 4:32 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> I am helping my fellow developers by replying to the comments they
>> make when I send the patches for review. Unfortunately, the only
>> developer other than you that has made any comment at all, Ramkumar
>> Ramachandra, did so in a bellicose tone, but I replied to all his
>> comments either way, which where invalid.
>
> I've never wanted to pick fights with anyone, and I don't foresee
> having a desire to do so in the future.  I was just saying what was on
> my mind, which is along the lines of: you have written this patch with
> the attitude "I know what I'm doing, my users will benefit, and nobody
> else is going to look at this patch anyway";

That is an assumption, it's wrong, and it's antagonistic. There's no
need for that.

> I'm worried about what
> your other patches look like if this is your attitude towards
> development.

More assumptions and hypotheticals. Why don't we limit ourselves to
facts and reality?

>> The history *is* readable. If anybody has any problems with the commit
>> messages, the place to mention such problems is IN THE PATCH REVIEW.
>> Nobody has done that, because either nobody has any problems, or they
>> are not interested. Either way, there's nothing I can do about it.
>
> That's what I've been trying to say over and over again: _why_ are
> people not reviewing your patches?
>
> 0. Because nobody has any problems with them.
>
> 1. Because nobody on the git list cares about remote-hg.
>
> 2. Because you're stubborn as a mule, and the resulting thread often
> results in long-winded discussions like this one (which wastes
> everyone's time).  Therefore, the potential reviewer's reasoning is
> that their review time is better spent elsewhere, where their review
> is actually appreciated.
>
> Hint: it's not (0).
>
> If you're claiming that (1) is the case, then why are you posting to
> the git list and hitting everyone's inboxes?  Maintain your project
> outside git.
>
> I'm claiming that it's (2).  In which case, it's you who needs changing.

This is the false dichotomy fallacy, why does it have to be only one
of these reasons? Couldn't it be a mixture of them? Maybe most people
don't care about remote-{bzr,hg}, maybe for the ones that do, most
don't see any problems in the patches, and maybe the ones that do see
problems in the patches don't bring them up, because of various
reasons, like for example, they don't see them as major, and would
rather fix them themselves later after investigation if they are
indeed import problems, or maybe they just don't have time to engage
in discussions.

Yes, there's a possibility that my stubbornness is a factor, and given
their possible lack of time, and possible lack of interested, they
choose to not engage.

But to claim that *everyone* is in (2), and that there are no other
factors that made them land in (2) but my stubbornness is disingenuous
at best.

What would you think of a scientist that says, oh, "I'm not going to
review that paper, because each time I do that, the reviewee defends
it, and we end up with long-winded discussions". This scientist
doesn't have the right spirit.

If you submit a review comment, you should be prepared to do defend
it, just like you do when you submit a patch. And you should be
prepared to accept that your patch has mistakes, just like you should
be prepared to accept that your review has mistakes. In this view,
stubbornness is a good thing, because it brings the best in the
reviewer, and in the reviewee, and in the end everyone benefits by
trying to achieve the higher quality possible. Just like stubbornness
is a good thing in science, if both reviwer and reviewee fight for
their point of view, only the best science wins, and we all benefit.

This is all of course if the stubbornness is warranted, but you
haven't claimed that it wasn't, simply that I was stubborn. Well, so
what? Isn't that good?

Show me where I was unreasonable, show me where I was wrong, not where
I was stubborn. One can be stubborn and be right, in fact, when one is
right, it's when one has all the more right to be stubborn.

If you are not prepared to defend your review, so are others, why to
you blame that on me? If you were right, you would be shown to be
right. Period.

>> I'm willing to change my ways when there's reason to change my ways,
>> and so far, nobody has provided any evidence that my commit messages
>> are indeed lacking, only *opinions*.
>
> You want a formal mathematical proof?  We operate on opinions, and
> freeze what we think we all agree with into "community guidelines".

No, we operate in evidence and reason, *not* opinion. Any reasonable
person would say "well, I *think* this commit message needs more
description, but I don't *know*, I don't have *evidence* for it, so
I'm not going to fight to the death, as if I had".

Any reasonable person would know the difference between an opinion,
and an objective fact. And react accordingly when another person
attacks an opinion, which is not a big deal, and when an objective
fact is attacked, which is a big deal.

>> Other people are perfectly fine with them:
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/?qt=author&q=felipe.contreras
>
> So you're now claiming that we're the ones at fault (Peff, Thomas,
> Junio, and me, among others).  Okay, so why are you forcing your
> changes and opinions down our throats?

Where am I doing that?

First of all, lets not act like bitching girlfriends arguing about who
didn't throw the trash three years ago. We are talking about
*remote-bzr*, in fact, the subject is "[PATCH 1/9] remote-bzr: trivial
cleanups". *NONE* of this discussion is relevant to that patch.

If we go one step above, to remote-helpers in general, nobody has
commented *ANYTHING* negative on those patches, you are the first to
do so. So how exactly did I managed to shove 71 patches down you
throat if everybody complained all the way?

Ultimately the decision to merge or not to merge comes to Junio, if
you don't like his decision, go complain to him, but I would be
prepared with points in time where people complained about these
patches, and there are no complains, so you have no ammunition at all
whatsoever.

If you are talking about something else, FFS, change the subject line.
If you want to argue about who didn't take out the trash three years
ago, fine, but lets do so clearly in another thread, not this one
about a *single trivial patch*.

> You're in the wrong community:
> join a community of people who are more like you (or start your own
> project), and stop wasting our time.

And this is how communities die. When everybody thinks the same, and
everyone who thinks differently is displaced. A monoculture, a place
full of yes-men where nobody criticizes anybody, a circlejerk where
everyone palms the back of everyone else. Eventually things go south,
and nobody around you understands why.

Diversity in a community is healthy. If you don't like people who
think differently, *you* have a problem. If you don't like standing up
and defending your ideas, *you* have a problem. If you don't like
discussing on the basis of evidence and reason, *you* have a problem.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

[completely off-topic; don't worry, we're just having a friendly chat]

Felipe Contreras wrote:
> If you are not prepared to defend your review, so are others, why to
> you blame that on me? If you were right, you would be shown to be
> right. Period.

Felipe, there are some things that are worth arguing about for a long
time (like the new revision spec I'm proposing in [1]), and others
that are not.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/222248/focus=222526

> No, we operate in evidence and reason, *not* opinion. Any reasonable
> person would say "well, I *think* this commit message needs more
> description, but I don't *know*, I don't have *evidence* for it, so
> I'm not going to fight to the death, as if I had".

Don't you think you're taking reason to an extreme here?  Reason is a
tool that I use when I want.  I don't want reason when I'm browsing
Google Art Project or listening to Gentle Giant.  Arguments like "is
this commit message large enough?" are really not worth the time and
effort.

> Ultimately the decision to merge or not to merge comes to Junio, if
> you don't like his decision, go complain to him, but I would be
> prepared with points in time where people complained about these
> patches, and there are no complains, so you have no ammunition at all
> whatsoever.

I have no desire to "attack" you, Felipe.  I respect you as a more
experienced developer than myself, and am trying to offer constructive
criticism.

I don't have an ego (or consider myself important to the community).
Whatever will happen will happen, with or without me.

> And this is how communities die. When everybody thinks the same, and
> everyone who thinks differently is displaced. A monoculture, a place
> full of yes-men where nobody criticizes anybody, a circlejerk where
> everyone palms the back of everyone else. Eventually things go south,
> and nobody around you understands why.
>
> Diversity in a community is healthy. If you don't like people who
> think differently, *you* have a problem. If you don't like standing up
> and defending your ideas, *you* have a problem. If you don't like
> discussing on the basis of evidence and reason, *you* have a problem.

Diversity is certainly healthy, and I it would be nice to have you in
the community.  We just have to find a way to keep the conflict down.

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> Diversity is certainly healthy, and I it would be nice to have you in
> the community.  We just have to find a way to keep the conflict down.

After all, what are we asking for?  Better commit messages.  Why are
you making such a big deal out of it?  You want diversity in "length/
form of commit messages"?

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 2:30 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> [completely off-topic; don't worry, we're just having a friendly chat]
>
> Felipe Contreras wrote:
>> If you are not prepared to defend your review, so are others, why to
>> you blame that on me? If you were right, you would be shown to be
>> right. Period.
>
> Felipe, there are some things that are worth arguing about for a long
> time (like the new revision spec I'm proposing in [1]), and others
> that are not.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/222248/focus=222526

I agree, and I have discussed about issues with diff in the past,
unfortunately didn't reach any conclusion. I've tried to follow that
thread, but I don't really know what is actually being proposed
anymore.

If you are so keen in receiving feedback from your fellow developers,
you should eventually send an email summarizing the issues and the
proposal for everyone to understand.

In contrast, I take it you agree this trivial patch is not worth
discussing, which I agreed in my first reply.

>> No, we operate in evidence and reason, *not* opinion. Any reasonable
>> person would say "well, I *think* this commit message needs more
>> description, but I don't *know*, I don't have *evidence* for it, so
>> I'm not going to fight to the death, as if I had".
>
> Don't you think you're taking reason to an extreme here?  Reason is a
> tool that I use when I want.  I don't want reason when I'm browsing
> Google Art Project or listening to Gentle Giant.  Arguments like "is
> this commit message large enough?" are really not worth the time and
> effort.

Reason is not a tool for appreciating art, reason is a tool for
discovering truth, and if when arguing you are not interested in what
is actually true, I'm not interested in arguing with you.

>> Ultimately the decision to merge or not to merge comes to Junio, if
>> you don't like his decision, go complain to him, but I would be
>> prepared with points in time where people complained about these
>> patches, and there are no complains, so you have no ammunition at all
>> whatsoever.
>
> I have no desire to "attack" you, Felipe.  I respect you as a more
> experienced developer than myself, and am trying to offer constructive
> criticism.
>
> I don't have an ego (or consider myself important to the community).
> Whatever will happen will happen, with or without me.

I appreciate your criticism, but that doesn't mean I must agree with
it. And if I do agree, that doesn't mean I must act upon it.

>> And this is how communities die. When everybody thinks the same, and
>> everyone who thinks differently is displaced. A monoculture, a place
>> full of yes-men where nobody criticizes anybody, a circlejerk where
>> everyone palms the back of everyone else. Eventually things go south,
>> and nobody around you understands why.
>>
>> Diversity in a community is healthy. If you don't like people who
>> think differently, *you* have a problem. If you don't like standing up
>> and defending your ideas, *you* have a problem. If you don't like
>> discussing on the basis of evidence and reason, *you* have a problem.
>
> Diversity is certainly healthy, and I it would be nice to have you in
> the community.  We just have to find a way to keep the conflict down.

A fine way to start is to not rattle away in trivial inconsequential patches.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> A fine way to start is to not rattle away in trivial inconsequential patches.

I have something from Linus (TM) this time :)

https://lkml.org/lkml/2004/12/20/255

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 3:03 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> A fine way to start is to not rattle away in trivial inconsequential patches.
>
> I have something from Linus (TM) this time :)
>
> https://lkml.org/lkml/2004/12/20/255

I happen to agree with that, specially in the context of the Linux
kernel, but I don't see how that applies here. Linus is talking about
trivial patches from an entry-level developer, who has much to learn,
and this is one the best ways to do that.

But in particular, he is talking about the fact that prominent kernel
developers don't spend too much time on these trivial patches from
these entry-level developers, and that can be frustrating for these
entry-level developers, which can be problematic.

Nothing at all related to what we are facing here.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> If you are so keen in receiving feedback from your fellow developers,
> you should eventually send an email summarizing the issues and the
> proposal for everyone to understand.

Thanks.  I'll do that in the morning.

> Reason is not a tool for appreciating art, reason is a tool for
> discovering truth, and if when arguing you are not interested in what
> is actually true, I'm not interested in arguing with you.

There is no great truth to be discovered by arguing about the length
of commit messages, Felipe.  There are some "guidelines" or "axioms"
upon which we build reason.  If you want to argue till everything
breaks down to Peano's Axioms, do Foundations of mathematics or
Analytical philosophy.  From personal experience, it's much more
satisfying than arguing with other humans (who aren't exact
creatures).

> I appreciate your criticism, but that doesn't mean I must agree with
> it. And if I do agree, that doesn't mean I must act upon it.

Why not?  Am I being unreasonable in asking you to justify your
changes, so I can understand what you've done with one quick reading?

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 3:28 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:

>> Reason is not a tool for appreciating art, reason is a tool for
>> discovering truth, and if when arguing you are not interested in what
>> is actually true, I'm not interested in arguing with you.
>
> There is no great truth to be discovered by arguing about the length
> of commit messages, Felipe.  There are some "guidelines" or "axioms"
> upon which we build reason.

And based on what do you build these guidelines and axioms if not
truth? Do you ask a computer to throw a number randomly from 0 to
infinite and that shall be the new axiom for what the perfect number
of words a commit message should have?

No, you determine that based on experience, and convenience. You find
a number that convenient to write, not hundreds of pages, and a number
that would help the reader if the patch in case a bug in the changes
is found on a later time, and that would help reviewers of code find
issues, and understand it. But most importantly, the number depends on
the complexity of the code changes. Note that I'm not saying on size,
because even one-liners can be extremely complex.

It's not arbitrary.

> If you want to argue till everything
> breaks down to Peano's Axioms, do Foundations of mathematics or
> Analytical philosophy.  From personal experience, it's much more
> satisfying than arguing with other humans (who aren't exact
> creatures).

I do not want to argue the fundamentals of logic and reason, but
unfortunately most people don't have a strong grasp on them.

So let me simply; truth matters, and how we find truth mattes. Which
is why the degree of certainty we have on certain facts matters; you
shouldn't act the same about a claim you are 10% sure it's true, than
with a claim you are 90% sure.

If you are 100% sure my commit messages are too short, then there's no
point in arguing with you. Nor if you think it doesn't matter if it's
90%, or 50%, or even 0%. Because it's an opinion, and an opinion
doesn't need any facts, or certainty, it just needs a person to hold
it, whatever unreasonable or unlikely it is.

>> I appreciate your criticism, but that doesn't mean I must agree with
>> it. And if I do agree, that doesn't mean I must act upon it.
>
> Why not?  Am I being unreasonable in asking you to justify your
> changes, so I can understand what you've done with one quick reading?

I did justify everything, I just didn't act the way you wanted. I
didn't immediately resend the series with a full description of the
changes, because the changes, as I described before, are trivial. I
simply dropped the change you had a problem with, and moved on. It's
perfectly reasonable.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 4:32 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:

> Junio C Hamano wrote:
>> I do
>> not agree with Ram at all when he says that developers are more
>> important than users, and I agree with you that the project exists
>> for users, and not for developers.
>
> On this.
>
> If Peff were to suddenly stop working on git one day (because he was
> frustrated with the community/ development practices), we'd all lose a
> lot more than if one random end-user switches to using hg for his tiny
> personal projects.

Yeah but that's not happening is it? This is yet another hypothetical.
Last I checked Peff never threatened to leave the project. Did I miss
a memo?

The last time somebody announced he was going to leave the project we
did what was reasonable; investigate the reasons. And that's what we
would do if Peff threatened to leave.

But fine, lets assume there's a hypothetical Peff, with hypothetical
reasons to leave the project...

> I'm _not_ claiming that there's a split between
> users and users that are developers (we have one mailing list for
> everyone, and I like that).  What I'm claiming is that we cannot (and
> should not) pay equal attention to every user of git.  Some users are
> more important than others.  Again, that does _not_ mean that we push
> a change that benefits one important user but breaks everyone else's
> setup.

The importance of users changes all the time. The 15 year old kid in
Sao Paulo might not be important today, but he might be the single
most important contributor ten years from now. Hell, he might even
replace Junio as the maintainer.

Who are you to decide which users are important, and which are not?

> Ofcourse the project exists for its users; we're not doing research.
> However, we don't all have to write tutorials to keep in touch with
> end-users who are completely detached from the development process
> (our time is better spent elsewhere),

Maybe we should, and maybe we would see then some areas of
improvement. In fact, I have done so, and I do see lots of areas of
improvement in git's UI.

I agree that there are more important areas, or rather, more fun to
work with, but the fact that most git developers don't pay too much
attention to the pain of newcomers shows, and it's a very common
criticism of git; it's difficult to learn, it doesn't have a
consistent UI, many commands don't make sense. And I happen to agree
with that claim, but it's not an easy problem to solve, specially when
you care about *all* users, both old and new, which we do.

We should keep in mind the problems in git's UI for newcomers. There's
no reason no to.

> or even have an
> end-user-friendly bug tracker (where the SNR is very low).  We don't
> have to consciously reach out to people we're not connected to
> directly: if we're all sufficiently connected to the real world, the
> itches/ bugs worth working on will always find their way to us.  We
> live in a connected world.

Nobody is claiming we need a bug tracker, there's no point in arguing
about that. The rate at which we fix bugs or our tracking of them is
not a problem.

> Yes, I know.  You're going to respond to this email arguing about why
> you're Right and why I (and everyone else) is Wrong, either by quoting
> what Linus (TM) said and twisting it to mean what you want, belaboring
> over what you've already said, or something similar.

Where did I twist anything? You can see Linus talk himself:
http://www.youtube.com/watch?v=kzKzUBLEzwk

Point me exactly where does he say some users are more important than
others, in fact, he is saying the opposite, the amount of people that
needed the Linux version compatibility flag was really really small,
yet they did it, why? Because *all* users matter. Not that it matters
what Linus says, what matters is that it's right; the moment you start
balkanizing your user base, the moment you start giving some them
reason to fork the project. When was the last time Linux was forked?
GNOME did exactly that, they said; you, users over there, we don't
care about you anymore, what did they do? Fork. They lost so many
users they had to revert their decision.

Should we willingly and knowingly neglect some git user-base? No, why
would you want them to fork? In a way, git's UI has been so bad, that
some kind-of-forks have happened, that tells us something; the UI
needs some love, fortunately none of those forks worked, which tells
us something too; it's not too atrocious.

That's not to say we shouldn't fix the UI, we should, in a way that
everyone's happy, which is hard, but we will do it, eventually.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> The importance of users changes all the time. The 15 year old kid in
> Sao Paulo might not be important today, but he might be the single
> most important contributor ten years from now. Hell, he might even
> replace Junio as the maintainer.

Yes, they do.  Did I say that they don't change?

> Where did I twist anything? You can see Linus talk himself:
> http://www.youtube.com/watch?v=kzKzUBLEzwk

Yes, I watched the talk when you posted the link last time.  And yes,
I learnt something.

> Should we willingly and knowingly neglect some git user-base? No, why
> would you want them to fork? In a way, git's UI has been so bad, that
> some kind-of-forks have happened, that tells us something; the UI
> needs some love, fortunately none of those forks worked, which tells
> us something too; it's not too atrocious.

No, we should never neglect.  I believe in including everyone.  In
fact I take it to an extreme: on many instances, I have pointed out
what I want specifically, and asked for a configuration option if it's
not necessarily a sane default.  Git is a toolkit, and should be
loaded with features that even a few users want.

> That's not to say we shouldn't fix the UI, we should, in a way that
> everyone's happy, which is hard, but we will do it, eventually.

On this, I think the way forward is complete-implicit'ness via
configuration variables.  I recently wrote remote.pushdefault to
simply 'git push', and proposed 'git push +ref1 ref2 ref3' to
automatically push to the correct pushdefaults (but that proposal was
rejected).

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Fri, Apr 26, 2013 at 3:17 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> The importance of users changes all the time. The 15 year old kid in
>> Sao Paulo might not be important today, but he might be the single
>> most important contributor ten years from now. Hell, he might even
>> replace Junio as the maintainer.
>
> Yes, they do.  Did I say that they don't change?

But you implied we shouldn't care about Thiago (our hypothetical
future overlord), because he is among the users we should't care for
(right now).

>> Should we willingly and knowingly neglect some git user-base? No, why
>> would you want them to fork? In a way, git's UI has been so bad, that
>> some kind-of-forks have happened, that tells us something; the UI
>> needs some love, fortunately none of those forks worked, which tells
>> us something too; it's not too atrocious.
>
> No, we should never neglect.  I believe in including everyone.  In
> fact I take it to an extreme: on many instances, I have pointed out
> what I want specifically, and asked for a configuration option if it's
> not necessarily a sane default.  Git is a toolkit, and should be
> loaded with features that even a few users want.
>
>> That's not to say we shouldn't fix the UI, we should, in a way that
>> everyone's happy, which is hard, but we will do it, eventually.
>
> On this, I think the way forward is complete-implicit'ness via
> configuration variables.  I recently wrote remote.pushdefault to
> simply 'git push', and proposed 'git push +ref1 ref2 ref3' to
> automatically push to the correct pushdefaults (but that proposal was
> rejected).

Indeed, I learned about that, and I tried to use it, but I think
there's a lot that is missing, and I don't know myself what would be
ideal. I'm starting to think that a branch should have two upstreams;
one that is used for rebasing, and another that is used for pushing.
But I'm not sure.

Eventually, I would like to do 'git push' and I would push different
branches to different repositories in different destination branches
in a way that requires multiple commands right now 'git push github
fc/remote-old/hg:fc/remote/hg', 'git push --prune backup
refs/heads/*:refs/heads/* refs/tags/*:refs/tags/*'. And to figure
things out I'm also helping; I added the --prune option to push, and I
added color to visualize upstream branches in 'git branch'.

But I don't think any of those are as important as having a proper
'git stage' command, and getting rid of --cached and --index, which
will be a huge effort, but would pay even bigger dividends. Step by
step.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Stefano Lattarini]

On 04/25/2013 08:19 PM, Ramkumar Ramachandra wrote:
> Felipe Contreras wrote:
>> diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
>> index aa7bc97..82bf7c7 100755
>> --- a/contrib/remote-helpers/git-remote-bzr
>> +++ b/contrib/remote-helpers/git-remote-bzr
>> @@ -94,7 +94,7 @@ class Marks:
>>          return self.last_mark
>>
>>      def is_marked(self, rev):
>> -        return self.marks.has_key(rev)
>> +        return rev in self.marks
> 
> Why?  Is the new form faster than the older one?
>
I think the has_key() method is "deprecated" in modern python,
and the 'key in dict' usage is more idiomatic.

>> @@ -224,7 +224,7 @@ def export_files(tree, files):
>>              else:
>>                  mode = '100644'
>>
>> -            # is the blog already exported?
>> +            # is the blob already exported?
> 
> What is this?  Whitespace?
>
Typofix: s/blog/blob/

>> @@ -521,7 +521,7 @@ def c_style_unescape(string):
>>      return string
>>
>>  def parse_commit(parser):
>> -    global marks, blob_marks, bmarks, parsed_refs
>> +    global marks, blob_marks, parsed_refs
> 
> How is this trivial?  You just removed one argument.
>
Maybe bmarks was no longer used there as a global variable
(left-over from previous patches?), so there is no longer any
need to declare it global.

>> @@ -555,7 +555,7 @@ def parse_commit(parser):
>>              mark = int(mark_ref[1:])
>>              f = { 'mode' : m, 'data' : blob_marks[mark] }
>>          elif parser.check('D'):
>> -            t, path = line.split(' ')
>> +            t, path = line.split(' ', 1)
> 
> How on earth is this trivial?  It changes the entire meaning!
>
Indeed, that should best go in a separate path with a proper
explanation in the commit message.

>> @@ -643,6 +643,7 @@ def do_export(parser):
>>                  wt = repo.bzrdir.open_workingtree()
>>                  wt.update()
>>          print "ok %s" % ref
>> +
> 
> Whitespace?
>
Isn't that obvious?

> I'm outraged by this.  What kind of changes are you pushing to
> remote-hg?  A "trivial cleanups" bundling miscellaneous changes, with
> no commit message?  Why don't you just squash everything into one
> "miscellaneous changes" patch?
>
I have no opinion on this, so I won't comment.

Regard,
  Stefano

Re: [PATCH 1/9] remote-bzr: trivial cleanups

[Felipe Contreras]

On Thu, Apr 25, 2013 at 2:29 PM, Stefano Lattarini
<stefano.lattarini@gmail.com> wrote:
> On 04/25/2013 08:19 PM, Ramkumar Ramachandra wrote:

>>> @@ -521,7 +521,7 @@ def c_style_unescape(string):
>>>      return string
>>>
>>>  def parse_commit(parser):
>>> -    global marks, blob_marks, bmarks, parsed_refs
>>> +    global marks, blob_marks, parsed_refs
>>
>> How is this trivial?  You just removed one argument.
>>
> Maybe bmarks was no longer used there as a global variable
> (left-over from previous patches?), so there is no longer any
> need to declare it global.

Even more, it never was used, it was a mistake carried when copying
this method from remote-hg; we don't have bookmarks in bazaar. And
bmarks wasn't even used in this method in remote-hg either =/

But it would be obvious that it was not used once one ran the tests
and they passed, which they do.

Cheers.

-- 
Felipe Contreras

[PATCH 2/9] remote-hg: remove extra check

[Felipe Contreras]

Not needed since we use xrange ourselves.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 5481331..0b7c81f 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -332,10 +332,6 @@ def export_ref(repo, name, kind, head):
     ename = '%s/%s' % (kind, name)
     tip = marks.get_tip(ename)
 
-    # mercurial takes too much time checking this
-    if tip and tip == head.rev():
-        # nothing to do
-        return
     revs = xrange(tip, head.rev() + 1)
     count = 0
 
-- 
1.8.2.1

Re: [PATCH 2/9] remote-hg: remove extra check

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
> index 5481331..0b7c81f 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -332,10 +332,6 @@ def export_ref(repo, name, kind, head):
>      ename = '%s/%s' % (kind, name)
>      tip = marks.get_tip(ename)
>
> -    # mercurial takes too much time checking this
> -    if tip and tip == head.rev():
> -        # nothing to do
> -        return
>      revs = xrange(tip, head.rev() + 1)

I'm surprised these four lines were even there in a previous revision.
 Again, you changed the meaning: if xrange() returns an empty range,
you must return, by extension.

Re: [PATCH 2/9] remote-hg: remove extra check

[Felipe Contreras]

On Thu, Apr 25, 2013 at 1:23 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
>> index 5481331..0b7c81f 100755
>> --- a/contrib/remote-helpers/git-remote-hg
>> +++ b/contrib/remote-helpers/git-remote-hg
>> @@ -332,10 +332,6 @@ def export_ref(repo, name, kind, head):
>>      ename = '%s/%s' % (kind, name)
>>      tip = marks.get_tip(ename)
>>
>> -    # mercurial takes too much time checking this
>> -    if tip and tip == head.rev():
>> -        # nothing to do
>> -        return
>>      revs = xrange(tip, head.rev() + 1)
>
> I'm surprised these four lines were even there in a previous revision.
>  Again, you changed the meaning: if xrange() returns an empty range,
> you must return, by extension.

I'm not going to go back in history, but we were not always using
xrange, but the mercurial API helper, which was dead slow, and in the
end did basically an xrange().

-- 
Felipe Contreras

[PATCH 3/9] remote-bzr: fix bad state issue

[Felipe Contreras]

Carried from remote-hg.

The problem reportedly happened after doing a push that fails, the abort
causes the state of remote-hg to go bad, this happens because
remote-hg's marks are not stored, but 'git fast-export' marks are.

Ensure that the marks are _always_ stored.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 82bf7c7..84734c7 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -32,6 +32,7 @@ import os
 import json
 import re
 import StringIO
+import atexit
 
 NAME_RE = re.compile('^([^<>]+)')
 AUTHOR_RE = re.compile('^([^<>]+?)? ?<([^<>]*)>$')
@@ -728,6 +729,7 @@ def main(args):
     blob_marks = {}
     parsed_refs = {}
     files_cache = {}
+    marks = None
 
     gitdir = os.environ['GIT_DIR']
     dirname = os.path.join(gitdir, 'bzr', alias)
@@ -754,6 +756,10 @@ def main(args):
             die('unhandled command: %s' % line)
         sys.stdout.flush()
 
+def bye():
+    if not marks:
+        return
     marks.store()
 
+atexit.register(bye)
 sys.exit(main(sys.argv))
-- 
1.8.2.1

[PATCH 4/9] remote-bzr: add support to push URLs

[Felipe Contreras]

Just like in remote-hg.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 84734c7..d6319d6 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -32,7 +32,7 @@ import os
 import json
 import re
 import StringIO
-import atexit
+import atexit, shutil, hashlib
 
 NAME_RE = re.compile('^([^<>]+)')
 AUTHOR_RE = re.compile('^([^<>]+?)? ?<([^<>]*)>$')
@@ -719,11 +719,11 @@ def main(args):
     global blob_marks
     global parsed_refs
     global files_cache
+    global is_tmp
 
     alias = args[1]
     url = args[2]
 
-    prefix = 'refs/bzr/%s' % alias
     tags = {}
     filenodes = {}
     blob_marks = {}
@@ -731,6 +731,13 @@ def main(args):
     files_cache = {}
     marks = None
 
+    if alias[5:] == url:
+        is_tmp = True
+        alias = hashlib.sha1(alias).hexdigest()
+    else:
+        is_tmp = False
+
+    prefix = 'refs/bzr/%s' % alias
     gitdir = os.environ['GIT_DIR']
     dirname = os.path.join(gitdir, 'bzr', alias)
 
@@ -759,7 +766,10 @@ def main(args):
 def bye():
     if not marks:
         return
-    marks.store()
+    if not is_tmp:
+        marks.store()
+    else:
+        shutil.rmtree(dirname)
 
 atexit.register(bye)
 sys.exit(main(sys.argv))
-- 
1.8.2.1

[PATCH 5/9] remote-hg: use hashlib instead of hg sha1 util

[Felipe Contreras]

To be in sync with remote-bzr.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 0b7c81f..99abda4 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -22,6 +22,7 @@ import shutil
 import subprocess
 import urllib
 import atexit
+import hashlib
 
 #
 # If you want to switch to hg-git compatibility mode:
@@ -830,7 +831,7 @@ def main(args):
 
     if alias[4:] == url:
         is_tmp = True
-        alias = util.sha1(alias).hexdigest()
+        alias = hashlib.sha1(alias).hexdigest()
     else:
         is_tmp = False
 
-- 
1.8.2.1

Re: [PATCH 5/9] remote-hg: use hashlib instead of hg sha1 util

[Ramkumar Ramachandra]

Felipe Contreras wrote:
> To be in sync with remote-bzr.

Huh?  Why do you have to be in sync with remote-bzr?  Are you sharing
code between remote-hg and remote-bzr?

> @@ -830,7 +831,7 @@ def main(args):
>
>      if alias[4:] == url:
>          is_tmp = True
> -        alias = util.sha1(alias).hexdigest()
> +        alias = hashlib.sha1(alias).hexdigest()

Did you eve bother justifying this change with a line in the commit
message?  How is the new form different from the old form?

Re: [PATCH 5/9] remote-hg: use hashlib instead of hg sha1 util

[Felipe Contreras]

On Thu, Apr 25, 2013 at 1:25 PM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Felipe Contreras wrote:
>> To be in sync with remote-bzr.
>
> Huh?  Why do you have to be in sync with remote-bzr?  Are you sharing
> code between remote-hg and remote-bzr?

We don't have to.

>> @@ -830,7 +831,7 @@ def main(args):
>>
>>      if alias[4:] == url:
>>          is_tmp = True
>> -        alias = util.sha1(alias).hexdigest()
>> +        alias = hashlib.sha1(alias).hexdigest()
>
> Did you eve bother justifying this change with a line in the commit
> message?  How is the new form different from the old form?

Why would it be any difference? It's a hex version of the SHA-1
digest. It would be the same in every language and every tool.

And a bit of context: historically the reason I started remote-bzr was
to show that we didn't need the *huge* infrastructure that is sitting
git_remote_helpers, which is nothing compared to what was prepared to
be merged for msysgit's remote-hg. I wrote it as a proof-of-concept to
show we didn't need a framework, and if we do, it would only be clear
after having _two_ remote helpers, which we now do. It might make
sense to refactor the common parts into a framework later on, so
having them in sync as much as it's reasonably possible makes sense.

But if even if it wasn't, there's nothing wrong with this patch. Also,
who knows, maybe old versions of mercurial don't have util.sha1(), or
maybe newer ones will move it, who knows.

Cheers.

-- 
Felipe Contreras

[PATCH 6/9] remote-bzr: store converted URL

[Felipe Contreras]

Mercurial might convert the URL to something more appropriate, like an
absolute path. Lets store that instead of the original URL, which won't
work from a different working directory if it's relative.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index d6319d6..3d3b1c1 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -32,7 +32,7 @@ import os
 import json
 import re
 import StringIO
-import atexit, shutil, hashlib
+import atexit, shutil, hashlib, urlparse, subprocess
 
 NAME_RE = re.compile('^([^<>]+)')
 AUTHOR_RE = re.compile('^([^<>]+?)? ?<([^<>]*)>$')
@@ -713,6 +713,14 @@ def get_repo(url, alias):
 
     return branch
 
+def fix_path(alias, orig_url):
+    url = urlparse.urlparse(orig_url, 'file')
+    if url.scheme != 'file' or os.path.isabs(url.path):
+        return
+    abs_url = urlparse.urljoin("%s/" % os.getcwd(), orig_url)
+    cmd = ['git', 'config', 'remote.%s.url' % alias, "bzr::%s" % abs_url]
+    subprocess.call(cmd)
+
 def main(args):
     global marks, prefix, dirname
     global tags, filenodes
@@ -741,6 +749,9 @@ def main(args):
     gitdir = os.environ['GIT_DIR']
     dirname = os.path.join(gitdir, 'bzr', alias)
 
+    if not is_tmp:
+        fix_path(alias, url)
+
     if not os.path.exists(dirname):
         os.makedirs(dirname)
 
-- 
1.8.2.1

[PATCH 7/9] remote-hg: use python urlparse

[Felipe Contreras]

It's simpler, and we don't need to depend on certain Mercurial versions.

Also, now we don't update the URL if 'file://' is not present.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-hg | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 99abda4..67c3074 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -12,7 +12,7 @@
 # For remote repositories a local clone is stored in
 # "$GIT_DIR/hg/origin/clone/.hg/".
 
-from mercurial import hg, ui, bookmarks, context, util, encoding, node, error
+from mercurial import hg, ui, bookmarks, context, encoding, node, error
 
 import re
 import sys
@@ -22,7 +22,7 @@ import shutil
 import subprocess
 import urllib
 import atexit
-import hashlib
+import urlparse, hashlib
 
 #
 # If you want to switch to hg-git compatibility mode:
@@ -788,11 +788,11 @@ def do_export(parser):
     print
 
 def fix_path(alias, repo, orig_url):
-    repo_url = util.url(repo.url())
-    url = util.url(orig_url)
-    if str(url) == str(repo_url):
+    url = urlparse.urlparse(orig_url, 'file')
+    if url.scheme != 'file' or os.path.isabs(url.path):
         return
-    cmd = ['git', 'config', 'remote.%s.url' % alias, "hg::%s" % repo_url]
+    abs_url = urlparse.urljoin("%s/" % os.getcwd(), orig_url)
+    cmd = ['git', 'config', 'remote.%s.url' % alias, "hg::%s" % abs_url]
     subprocess.call(cmd)
 
 def main(args):
-- 
1.8.2.1

[PATCH 8/9] remote-bzr: tell bazaar to be quiet

[Felipe Contreras]

Otherwise we get notification, progress bars, and what not.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 3d3b1c1..19668a9 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -26,6 +26,7 @@ bzrlib.plugin.load_plugins()
 import bzrlib.generate_ids
 import bzrlib.transport
 import bzrlib.errors
+import bzrlib.ui
 
 import sys
 import os
@@ -755,6 +756,8 @@ def main(args):
     if not os.path.exists(dirname):
         os.makedirs(dirname)
 
+    bzrlib.ui.ui_factory.be_quiet(True)
+
     repo = get_repo(url, alias)
 
     marks_path = os.path.join(dirname, 'marks-int')
-- 
1.8.2.1

[PATCH 9/9] remote-bzr: strip extra newline

[Felipe Contreras]

It's added by fast-export, the user didn't type it.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/remote-helpers/git-remote-bzr | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 19668a9..8c316fe 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -549,6 +549,10 @@ def parse_commit(parser):
         parents.append(parser.get_mark())
         parser.next()
 
+    # fast-export adds an extra newline
+    if data[-1] == '\n':
+        data = data[:-1]
+
     files = {}
 
     for line in parser:
-- 
1.8.2.1