Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]
Felipe Contreras felipe.contre...@gmail.com writes: On Wed, Jun 12, 2013 at 3:02 PM, Junio C Hamano gits...@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? It is your opinion on what might happen in the future. And you ignored external projects that may want to link with libgit.a, 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. 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. 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. (2) refactoring builtin/notes.c to make it possible for sequencer machinery can also call useful helper functions buried in it. but I am sure other reviewers can recall other instances in the recent past. Your patches were wrong in both cases, but that is not an issue. If you are not familiar with the area you are trying to improve, it is understandable that initial attempts may try to solve the right problem in a wrong way. That is perfectly normal. That is what the patch review process is there to help. Reviewers who are more familiar with the area (either the code flow and data structure used in blame, or how the object files are laid out in the source tree and the build procedure is designed to link them to which binary) can point the contributor in a direction that would take us to a better result in the end. During the discussion, it may turn out that reviewers have overlooked issues that also need to be addressed, or there may be further adjustments needed that are initially overlooked by everyone. 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. When it is your patch, however, we repeatedly saw that the review process got derailed in the middle. The reviewers tried to reach a good end result in the same way as they interact with other contributors, i.e. by showing a way they think is better, trying to make the contributor realize why it is better by rephrasing and coming up with other examples. This iteration takes a lot of resources, but the reviewers are hoping that we will see a good result at the end of the review and everybody wins. They are trying to collaborate. 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. 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. And I am also sure you will then say majority is not always right. 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. 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. Goodbye. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]
On Thu, Jun 13, 2013 at 12:24 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Wed, Jun 12, 2013 at 3:02 PM, Junio C Hamano gits...@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. 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. 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 -- To unsubscribe from this list: send the line unsubscribe
Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]
On Thu, Jun 13, 2013 at 1:16 PM, Felipe Contreras felipe.contre...@gmail.com wrote: On Thu, Jun 13, 2013 at 12:24 PM, Junio C Hamano gits...@pobox.com wrote: 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 just to make it clear, I didn't deny you are the only one with commit access, and therefore you make all the shots. You made a decision, fine, I never said you can't do that. What I said is that you should not use words to imply that your *opinion* is a fact. The fact that you make a decision doesn't make your opinion a fact, and the fact that many people share your opinion doesn't make it a fact either. So, instead of saying: Just one side being right, and the other side continuing to repeat nonsense without listening. You should say: Simply a matter of disagreement where the code belongs. -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]
Johan Herland jo...@herland.net writes:
There is only one right solution. If a useful function is buried in
builtin/*.o as a historical accident (i.e. it started its life as a
helper for that particular command, and nobody else used it from
outside so far) and that makes it impossible to use the function
from outside builtin/*.o, refactor the function and its callers and
move it to libgit.a.
Here goes...
...Johan
With these three patches, if you apply the following skeleton patch
(lifted from $gmane/226851 and adjusted minimally to the change
these patches introduce), we can see that the link breakage Felipe
observed in the message:
Felipe Contreras felipe.contre...@gmail.com writes in $gmane/226851:
What happens?
libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'
is gone.
It is not the first time, nor the last that top-level code needs
builtin code, and the solution is easy; organize the code.
And as I already said, the above is correct. The problem and the
general approach to solve it correctly were identified in the
message.
But what followed was a nonsense, which ended up wastign everybody's
time:
... Alas, this
simple solution reject on the basis that we shouldn't organize the
code, because the code is not meant to be organized.
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.
Thanks for doing it right.
-- skeleton patch --
sequencer.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index ab6f8a7..4281466 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -14,6 +14,7 @@
#include merge-recursive.h
#include refs.h
#include argv-array.h
+#include notes-utils.h
#define GIT_REFLOG_ACTION GIT_REFLOG_ACTION
@@ -979,6 +980,17 @@ static void save_opts(struct replay_opts *opts)
}
}
+static void copy_notes(const char *name, const char *msg)
+{
+ struct notes_rewrite_cfg *cfg;
+
+ cfg = init_copy_notes_for_rewrite(name);
+ if (!cfg)
+ return;
+
+ finish_copy_notes_for_rewrite(cfg, msg);
+}
+
static int pick_commits(struct commit_list *todo_list, struct replay_opts
*opts)
{
struct commit_list *cur;
@@ -997,6 +1009,8 @@ static int pick_commits(struct commit_list *todo_list,
struct replay_opts *opts)
return res;
}
+ copy_notes(cherry-pick, notes copied by cherry-pick);
+
/*
* Sequence of picks finished successfully; cleanup by
* removing the .git/sequencer directory
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] Refactor useful notes functions into notes-utils.[ch]
There is only one right solution. If a useful function is buried in builtin/*.o as a historical accident (i.e. it started its life as a helper for that particular command, and nobody else used it from outside so far) and that makes it impossible to use the function from outside builtin/*.o, refactor the function and its callers and move it to libgit.a. Here goes... ...Johan Johan Herland (3): finish_copy_notes_for_rewrite(): Let caller provide commit message Move copy_note_for_rewrite + friends from builtin/notes.c to notes-utils.c Move create_notes_commit() from notes-merge.c into notes-utils.c Makefile | 2 + builtin.h| 16 -- builtin/commit.c | 3 +- builtin/notes.c | 136 ++- notes-merge.c| 27 +- notes-merge.h| 14 - notes-utils.c| 157 +++ notes-utils.h| 37 + 8 files changed, 203 insertions(+), 189 deletions(-) create mode 100644 notes-utils.c create mode 100644 notes-utils.h -- 1.8.1.3.704.g33f7d4f -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
