Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jun 13, 2013, 18:16 UTC
Message-ID: <CAMP44s3atPW-SE1yQzep-F6M13g1fPP_q2RqHKofPL0B8=JfYQ@mail.gmail.com>
In-Reply-To: <7vsj0lvs8f.fsf@alter.siamese.dyndns.org>

On Thu, Jun 13, 2013 at 12:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Wed, Jun 12, 2013 at 3:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> The proposed patch was rejected on the basis that it was organized
>>> the code in a wrong way.  And your patch shows how it should be
>>> done.
>>
>> In your opinion.
>>
>> The fact that nobody outside of 'git' will ever use
>> init_copy_notes_for_rewrite() still remains. Therefore this
>> "organization" is wrong.
>
> That is a fact?

No, it's not.

> It is your opinion on what might happen in the future.

That's right, an informed opinion.

> And you ignored external projects that may want to link with
> libgit.a,

Like which project?

Moreover:

% find /opt/git -name '*.a'

Returns nothing. The cannot link to libgit.a, and besides, we don't provide a public API at all.

> and closed the door for future improvements.  Johan's
> implementation has the same effect of allowing sequencer.c to call
> these functions without doing so.

That would be closing the door to ghosts.

Do you want to bet? Five years from now nobody will be using init_copy_notes_for_rewrite().

You loose, we move it to builtin/lib.a, you win, we don't.

Show 5 quoted lines

> Anyway, I have a more important thing to say.
>
> You sometimes identify the right problem to tackle, but often the
> discussions on your patches go in a wrong direction that does not
> help solving the original problem at all.

So what? I'm a human, am I not allowed to make mistakes?

You make mistakes too.

Show 6 quoted lines

> The two examples I can immediately recall offhand are:
>
>  (1) a possible "blame" enhancement, where gitk, that currently runs
>      two passes of it to identify where each line ultimately came
>      from and to identify where each line was moved to the current
>      place, could ask it to learn both with a single run.

Yes, *I ACKNOWLEDGED* the direction was not the right one, and I didn't have the time nor the patience to go into such a tedious direction.

>From my recommended guideline:

* Accept comments on your reviews gracefully. If the original patch
submitter doesn't agree with your review, don't take offense. Don't
assume the submitter has to automatically modify the patches according
to your comments, or even necessarily seek a compromise. The submitter
is entitled to his opinion, and so are you. Also, remember that each
person has their own priorities in life, and it might take time before
the submitter has time to implement the changes, if ever. The changes
you request might be beyond the time the submitter is willing to
spend, and it's OK for him to decide to drop the patches as a result.
You can help by picking the patches yourself in those situations.

>  (2) refactoring builtin/notes.c to make it possible for sequencer
>      machinery can also call useful helper functions buried in it.

You are wrong. My patches did solve the original problem, I know because I was the one that found the original problem.

> The solution to these problems is
> for contributors and reviewers to _collaborate_ to come up with a
> better end result, which is often different from both the original
> patch and the suggestions in the initial review.

Collaboration requires both sides to work on the problem. Not one side pointing fingers and the other side doing all the work.

> When it is your patch, however, we repeatedly saw that the review
> process got derailed in the middle.

When working collaboratively it's fine to disagree, and it's fine to have two sides come up with two different patches.

If you disagree with the other side, send a patch that does it properly.

If the other side doesn't do *exactly* what you want, that's not the review process being derailed.

> If there is no will to collaborate on the contributor's end,
> however, and the primary thing the contributor wants to do is to
> endlessly argue, the efforts by reviewers are all wasted. We do not
> get anywhere.

In order to have and endless argument, *both sides* need to be engaged in the argument. If you decide that a disagreement has been reached, the argument ends in a disagreement.

> That is how I perceive what happens to many of your patches.  I am
> sure you will say "that is your opinion", but I do not think I am
> alone.

The opinion of a billion people is still an opinion.

> But the thing is, that majority is what writes the majority of the
> code and does the majority of the reviews, so as maintainer I *do*
> have to give their opinion a lot of weight, not to mention my own
> opinion about how to help keep the community the most productive.

Indeed, but that doesn't make it a fact. It remains an opinion.

> And I have to conclude that the cost of having to deal with you
> outweighs the benefit the project gets out of having you around.
> Therefore I have ask you to leave and not bother us anymore.

We shall see.

[1] http://article.gmane.org/gmane.comp.version-control.git/225325

-- 
Felipe Contreras