Re: [PATCH] Documentation/CommunityGuidelines
On Mon, Jun 10, 2013 at 11:41 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 06/10/2013 03:28 PM, Ramkumar Ramachandra wrote: I've tried to write down a bare minimum, without restating the obvious. Thank you for drafting a proposed CommunityGuidelines document; I think such a document would be helpful. But I don't like the overall flavor of your proposal; frankly, it sounds to me more like Documentation/GuidelinesForCommunityToBendOverBackwardsToLiveWithFCsProvocations and I don't think that is healthy. The LKML would disagree with you, as this draft is rather similar to what they do. Have you ever heard the phrase don't feed the troll? Well, it's every similar. This also happens in any civilized modern society. Maybe you don't agree with the Tea Party, or some other political group, but you deport them? Do you squash their protests? No. You let them say what they need to say, and you ignore them. It's best for you, and it's best for the community. Ignore them and move on. 0. You do not take offense, no matter what. If someone attacks you irrationally, you do not respond. This is a public mailing list, and we are all rational people: the attacker has already humiliated herself in public, and everyone can see that. This is secondary to the more important rule, do not attack other people on the mailing list. Not taking offense is at best a(n important) fallback position for those regrettable occasions when somebody else has any other already violated the primary guideline. Yes but you can't control what other people do, only what you do. Presumably you think you are not going to violate any of the other rules, so it's all the more important that you do follow this one, because that's the one *you* are possibly going to need to remember. But by you I really me we, because we all think we are not going to violate the other rules. We all think we don't commit logical fallacies, we all think our comments are right, productive, rational, and sensible. Of course that's not the case, what you think is a perfectly reasonable comment, somebody else might consider offensive. In fact, somebody is bound to find something offensive, so when that someone happens to be you, take a deep breath, and don't. 1. You do not take sides or vote. Do not post emails under the pretext of agreement: repeating what has already been said does not strengthen the argument. Post only if you have something unique to add to the discussion. 2. You stop pointing fingers. Every heated discussion requires more than one participant, and a flamewar requires many participants. If you participate, you have implicitly agreed to share the blame for whatever happens on the thread. People can judge for themselves who is to blame. Here your wording every heated discussion requires more than one participant seems to put more of the blame for heated discussions on participants 2..N and give a pass to participant number one. Which might actually be the case. If a drunk punches you in the face, and you fight back. Who do you think the police is going to find guilty of brawling? 3. Thou shalt not commit logical fallacies. The ones that are most common on this list: strawman, ad hominem, burden of proof, false cause, the texas sharpshooter, and appeal to authority. I think putting a rule like this in CommunityGuidelines puts too much weight on it. In my recollection, pointing out other people's supposed logical fallacies is far more often used on this list as a nitpicking diversionary tactic that usually leads a conversation *further* away from the real issues. I think it would be a mistake to encourage such formal and stylized argument on the ML. If you are not going to argue on the basis of logic and reason, what are you going to argue on the basis of? Being logical and reasonable is not finicky, it's a necessity. At least if you want to stay close to the real world. 4. Lead by example. If you do not like how someone presents themselves on the list, you counter it by presenting yourself nicely on the list. Others will follow your example, making that person's behavior the minority. It is far more powerful than explicitly stating what is acceptable behavior and what is not. Leading by example is a great approach, and has the effect that you describe on the majority of people. But I also think it would be helpful for the community to agree on a few very minimum standards of behavior that we insist on, and to call people out (preferably in a private email) if they fall short of these standards. 5. We are a community of programmers, and we are here to collaborate on code. The argument that leads to higher efficiency and better code has an automatic advantage over the argument that doesn't. If someone breaks one of these rules, there's a very simple way to communicate this to them: you don't respond to their email. Optionally, respond to their email off-list calmly
Re: [PATCH v2 2/4] commit-queue: LIFO or priority queue of commits
On Mon, Jun 10, 2013 at 04:23:31PM -0700, Junio C Hamano wrote: OK, I pushed out a result of some renaming and rebasing. Notable changes are: - The data and API is called prio-queue and they live in prio-queue.[ch]; - The test script is also named test-prio-queue.c, to leave the door open for other kinds of queue; Sounds reasonable, though you may want to update the commit message of jc/topo-author-date-sort~2. - For now, record_author_date() does the obvious read-sha1-file and free; and I think that is a good place to leave it in this series. It does not hurt performance in any existing cases, and any parsing refactoring can come later if somebody wants to work on it. - The comparison callback's function signature had three void *, so they are named in the header file now. Also two thing pointers are marked as const void *. Yeah, I noticed both when porting my tests, but didn't want to add too many distracting details. Thanks for fixing. Overall, it looks good for me except for the commit message tweaks I mentioned above. -Peff -- 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: v3 [PATCH 2/2] status:introduce status.branch to enable --branch by default
y...@ensimag.imag.fr wrote: [...] Good change. diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 9a07f15..958617a 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh I expected one test. Two, at most. This is a bit overzealous. I don't mind leaving it as it is, but as a note for the future: this kind of overkill is not necessarily a good thing; it makes it hard to add/remove tests, multiple tests fail when one tiny feature doesn't work as expected, and the testsuite takes longer to run. -- 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: v3 [PATCH 2/2] status:introduce status.branch to enable --branch by default
Ramkumar Ramachandra artag...@gmail.com writes: y...@ensimag.imag.fr wrote: [...] Good change. diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 9a07f15..958617a 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh I expected one test. Two, at most. This is a bit overzealous. I actually had this discussion with Jorge (in real life). I first thought this was too much, and I think we already removed one rendundant test. But actually, this should be two at least, since you really want to test whether --[no-]branch takes precedence over status.branch or not, in addition to testing status.branch alone. The last two tests are less important, but they also test something (in short: what happens when branch.status is set to something other than true). So I think it makes sense to keep them. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: [IGNORE] Implement 'git rebase' in ruby
On Mon, Jun 10, 2013 at 11:47 PM, Felipe Contreras felipe.contre...@gmail.com wrote: This is not my first patch that gets rejected, but it's the first one that gets rejected by Junio without even looking at it. What is anybody supposed to think about that? My very humble opinion: submit again the series without including in the cover letter This patch will not be applied and then handle the feedbacks. In short, everybody should simply approach again your work with a fresh mind. regards, -- Paolo -- 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] Documentation/CommunityGuidelines
Junio C Hamano wrote: The intent behind the document might be a noble one, but I am afraid that the text is too broad and vague and does not address the real issue to be of practical use. Drafting something like this is shit work, which explains why nobody has attempted it yet. I have no intent of collecting feedback and doing iterations: it's going to be an extraordinarily hard and boring task; _much_ worse than any technical documentation. Let me be clear that I have no hopes of landing this patch: I just wanted to create a calm and rational atmosphere for people to discuss the problem, in the hopes of minimizing the chances of large frequent fires. If you think we should put _something_ in our tree, I suggest dumping a few raw emails from this thread into contrib/CommunityGuidelines/ (or something). Taking one bullet point from the top for example: 0. You do not take offense, no matter what. If someone attacks you irrationally, you do not respond. This is a public mailing list, and we are all rational people: the attacker has already humiliated herself in public, and everyone can see that. What does saying we are all rational people help when the attacker poses a risk to destroy the community? What does we are all rational people even mean in this sentence? I intended it as a way to reassure everyone that we will make unbiased, rational judgements to the extent possible. It does not address the real cause of flamewars---why do rational people feel the need to respond when an irrational comment is made, e.g. when a reasonable review comments were responded not with either Yeah, you are right, thanks. or Not really, because you missed this case, I think... but with nitpicks with immaterial details or repetition without justification that takes account that the reviewer is in disagreement and there must be some reason behind it, i.e. a poisonous behaviour? There is no great truth about some hidden real cause to be found. For instance, in the one we just had, I would argue that it started with your non-patch administriva email with a huge number of people marked in the initial CC. Disaster waiting to happen, if you ask me. I'm not blaming you, but the lesson to be learnt is: avoid non-patch emails, and CC conservatively; if you want to discuss some changes, send a patch. That would explain why this very email is disguised as a [PATCH], with exactly one person in the initial CC. In short, the reason is a complex mix of various people's interactions under the current circumstances. Fires happen, and that is a fact; we can only look for common patterns and attempt to avoid fires by documenting these patterns as violations. Which is exactly what I have done (or attempted to do). I suspect it mostly has to do with the desire to make sure that bystanders do not get an impression that the one who speaks last gives the conclusion to the discussion, so stating The attacker being the one who speaks last in the discussion does not mean the conclusion is his. explicitly might be one way to make it more practically useful by alleviating the urge to respond, instead of saying no matter what. That is one pattern, but by no means the only one or even the most important one. I thought 0 was a nice generalization. -- 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 2/2] Move sequencer to builtin
On 2013-06-09 13:01:30 -0500, Felipe Contreras wrote: You don't agree that 1) a collegial work environment is overrated, 2) that the Linux kernel doesn't put an emphasis on being collegial, or 3) that it's the most successful software project in history? Point 1. Good, so we agree that a project doesn't need a collegial work environment to be extremely and amazingly successful. In fact, any rational person would keep an open mind to the fact that perhaps it actually _helps_ to not have such environment, based on the evidence. Just from skimming both lists, most of the time I find lkml to be nicer (and more collegial) to read because it has a better atmosphere than git@ had in the last year or two. And yes, a good atmosphere plays an important role. One of the reasons is that it makes it easier to discern arguments based on personality disputes - which certainly exist on lk - from actual technical disagreements that need to be resolved. Greetings, Andres Freund -- 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 2/2] Move sequencer to builtin
On Tue, Jun 11, 2013 at 4:18 AM, Andres Freund and...@anarazel.de wrote: On 2013-06-09 13:01:30 -0500, Felipe Contreras wrote: You don't agree that 1) a collegial work environment is overrated, 2) that the Linux kernel doesn't put an emphasis on being collegial, or 3) that it's the most successful software project in history? Point 1. Good, so we agree that a project doesn't need a collegial work environment to be extremely and amazingly successful. In fact, any rational person would keep an open mind to the fact that perhaps it actually _helps_ to not have such environment, based on the evidence. Just from skimming both lists, most of the time I find lkml to be nicer (and more collegial) to read because it has a better atmosphere than git@ had in the last year or two. A better atmosphere, yes, because they know how to avoid flamewars, and concentrate on technical issues, not because they have a collegial work environment. Unless you think this reply[1] is collegial. Even though I haven't been following Linux mailing lists that closely lately, I still manage to see a lot of these kinds of replies. And yes, a good atmosphere plays an important role. One of the reasons is that it makes it easier to discern arguments based on personality disputes - which certainly exist on lk - from actual technical disagreements that need to be resolved. That's right, but that's not because everyone is collegial in LKML, which they most certainly are not. Linus being one of many examples. [1] http://article.gmane.org/gmane.linux.usb.general/85952 -- 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] Documentation/CommunityGuidelines
Michael Haggerty wrote: Thank you for drafting a proposed CommunityGuidelines document; I think such a document would be helpful. But I don't like the overall flavor of your proposal; frankly, it sounds to me more like Documentation/GuidelinesForCommunityToBendOverBackwardsToLiveWithFCsProvocations It has nothing to do with Felipe. I've merely documented repeating patterns in fire threads as violations, in an attempt to avoid fires. I have not worked forward from axioms to derive transcendentally desirable behavior, but rather backwards from a disaster to derive patterns that have been shown to lead to large fires. Why? Because it's easier to derive unambiguous statements using my approach; as I will show shortly, there are various problems with your arguments. What gives you the impression that I documented everyone else's violations, but not Felipe's? ;) 0. You do not take offense, no matter what. If someone attacks you irrationally, you do not respond. This is a public mailing list, and we are all rational people: the attacker has already humiliated herself in public, and everyone can see that. This is secondary to the more important rule, do not attack other people on the mailing list. Not taking offense is at best a(n important) fallback position for those regrettable occasions when somebody else has already violated the primary guideline. The problem with your guideline is that you now need to define some sort of objective basis to determine whether or not someone attacks. What is this transcendental notion of attack? I say something, and you take offense, while someone else does not. Have I or have I not attacked you? One possible solution to this dilemma is to use majority opinion as a basis. This is a very dangerous road to go down, as fringe behaviors will keep getting eliminated until we're left with a bunch of yes-men on the list. In other words, an extremely suffocating atmosphere. My guideline does not suffer from this problem. It only requires you to believe that you were personally offended, and act accordingly. Whether or not you were justified in being offended is nobody's business. 2. You stop pointing fingers. Every heated discussion requires more than one participant, and a flamewar requires many participants. If you participate, you have implicitly agreed to share the blame for whatever happens on the thread. People can judge for themselves who is to blame. Here your wording every heated discussion requires more than one participant seems to put more of the blame for heated discussions on participants 2..N and give a pass to participant number one. I'm not going to comment on the issue of wording, since I've already made it clear that this patch is not for inclusion. It is unclear who Participant #1 is, but I'm not giving anyone a pass; everyone must share the blame. 3. Thou shalt not commit logical fallacies. The ones that are most common on this list: strawman, ad hominem, burden of proof, false cause, the texas sharpshooter, and appeal to authority. I think putting a rule like this in CommunityGuidelines puts too much weight on it. In my recollection, pointing out other people's supposed logical fallacies is far more often used on this list as a nitpicking diversionary tactic that usually leads a conversation *further* away from the real issues. I think it would be a mistake to encourage such formal and stylized argument on the ML. The guidelines serve as a means of educating people on the list about how they can avoid fuelling fires, not for literally quoting and beating up violators. As I have already stated in the final paragraph, there needs to be no consensus on whether or not a rule has been violated: everyone can judge that for themselves. 4. Lead by example. If you do not like how someone presents themselves on the list, you counter it by presenting yourself nicely on the list. Others will follow your example, making that person's behavior the minority. It is far more powerful than explicitly stating what is acceptable behavior and what is not. Leading by example is a great approach, and has the effect that you describe on the majority of people. But I also think it would be helpful for the community to agree on a few very minimum standards of behavior that we insist on, and to call people out (preferably in a private email) if they fall short of these standards. Let's see what your guidelines look like. 5. We are a community of programmers, and we are here to collaborate on code. The argument that leads to higher efficiency and better code has an automatic advantage over the argument that doesn't. If someone breaks one of these rules, there's a very simple way to communicate this to them: you don't respond to their email. Optionally, respond to their email off-list calmly explaining what went wrong. I would prefer a community standards document that looks more like this: [...]
Re: [PATCH] Documentation/CommunityGuidelines
Felipe Contreras wrote: I think there's an even more important number 0: Always assume good faith. When discussing through digital mediums, it's very easy to misconstrue the tone and intentions of other parties, so it's better to err on the side of caution, and if one is mistaken, assuming good faith doesn't cause harm, while the contrary does irreparable damage. This does not mean that one should continue to assume good faith when there's evidence to the contrary. Agreed. Always assume good faith is a good rule of thumb. 0. You do not take offense, no matter what. If someone attacks you irrationally, you do not respond. This is a public mailing list, and we are all rational people: the attacker has already humiliated herself in public, and everyone can see that. An even better and less absolutist version would be: I went for the absolutist version because I felt that this point needs to be driven in harder. This is the biggest problem, in my opinion. But yeah, your version is more technically correct. 3. Thou shalt not commit logical fallacies. The ones that are most common on this list: strawman, ad hominem, burden of proof, false cause, the texas sharpshooter, and appeal to authority. It might be better to turn this negative rule into a positive one: Discuss on the basis of logic and evidence. Then you can describe the common logical fallacies, and I would add If you make a claim, be prepared it to defend it with evidence, or add an appropriate qualifier; probably, most likely, I think, etc. Good addition. If someone breaks one of these rules, there's a very simple way to communicate this to them: you don't respond to their email. Optionally, respond to their email off-list calmly explaining what went wrong. I think you should reply, but not to her, to the mailing list, asking for others to don't reply. Then mute the thread. I already explained that about in the comment about flamewars. I don't think neglect is the solution to anything. We don't want contributors to feel neglected; we want to make them understand that their behavior was undesirable because of reasons X, Y, and Z. In a raging fire, they might not be able to see these reasons clearly. There's a corollary to that that works rather well in the LKML; you are permitted one flamewar per year. I'm not going to explain why this is a good thing, because unfortunately there's an irrational negative bias against me already, but there's a reason why this is a good rule. Even if you don't agree it's only one flamewar per year per person, it's not that much. I suppose it's a way for people to vent built-up emotion. Flamewars will happen, no matter what we do; we cannot control the actions of others. If too many people want to start a fire, we can do nothing: I don't propose an iron hand of suffocation. My objective is more realistic: it is to make people realize the undesirable effects and minimize fires. -- 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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 5:45 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Whether or not you were justified in being offended is nobody's business. In a parallel with law, there is no concept of justly offended, precisely because there is no way to determine what that even means. People get offended by all sorts of things. What's wrong with being offended? http://www.dailymotion.com/video/xl2w7q_easily-offended-then-watch-this_fun -- 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
[PATCH] pre-push.sample: Make the script executable
githooks(5) says that [...]the .sample files are executable by default which was not true. Signed-off-by: Wieland Hoffmann themi...@gmail.com --- templates/hooks--pre-push.sample | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 = 100755 templates/hooks--pre-push.sample diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample old mode 100644 new mode 100755 -- 1.8.3 -- 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] Documentation/CommunityGuidelines
Ramkumar Ramachandra artag...@gmail.com writes:
Michael Haggerty wrote:
Thank you for drafting a proposed CommunityGuidelines document; I think
such a document would be helpful. But I don't like the overall flavor
of your proposal; frankly, it sounds to me more like
Documentation/GuidelinesForCommunityToBendOverBackwardsToLiveWithFCsProvocations
It has nothing to do with Felipe. I've merely documented repeating
patterns in fire threads as violations, in an attempt to avoid fires.
I have not worked forward from axioms to derive transcendentally
desirable behavior, but rather backwards from a disaster to derive
patterns that have been shown to lead to large fires. Why? Because
it's easier to derive unambiguous statements using my approach; as I
will show shortly, there are various problems with your arguments.
What gives you the impression that I documented everyone else's
violations, but not Felipe's? ;)
It has become clear, also in discussion on IRC, that your preferred
approach is to fight the fires, attempting to extinguish flames as they
happen.
My approach -- and in my perception also that preferred by most of the
regulars who have spoken in this whole mess -- is that since there is a
fire hazard, it would be more effective firefighting to just remove the
hazard, thus preventing future fires.
I infer that in your view, there is an inalienable right for the fire
hazard to remain part of the community that you are not willing to give
up. I for one no longer have such qualms in this instance.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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
New feature discussion: git rebase --status
(Got the idea from: https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#git_rebase_--status) When in the middle of a rebase, users can be easily confused about what to do, or where they are in the rebase process. All the information is available in .git/rebase-merge/, but I believe it would be helpful to have a command (for example 'git rebase --status') which would explicitely indicate the state of the process. For instance, the output could look like: $ git rebase --status Rebasing my_last_commit onto base_commit Already applied 2 patches: b170635... my_commit_message b170635... my_commit_message Currently applying b170635... my_commit_message 2 patches left to apply: b170635... my_commit_message b170635... my_commit_message Another nice thing could be to improve the output of 'git status' by saying the number of patches left to apply. As an example, it could say: You are currently rebasing (patch 3/5). What do you think? Does the name rebase --status seem appropriate? Should the output be providing more/less information? Thanks =] -- Mathieu Liénard--Mayor, 2nd year at Grenoble INP - ENSIMAG (+33)6 80 56 30 02 -- 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: New feature discussion: git rebase --status
Mathieu Liénard--Mayor mathieu.lienard--ma...@ensimag.fr writes:
(Got the idea from:
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#git_rebase_--status)
When in the middle of a rebase, users can be easily confused about
what to do, or where they are in the rebase process.
All the information is available in .git/rebase-merge/, but I believe
it would be helpful to have a command (for example 'git rebase
--status') which would explicitely indicate the state of the process.
For instance, the output could look like:
$ git rebase --status
Rebasing my_last_commit onto base_commit
Already applied 2 patches:
b170635... my_commit_message
b170635... my_commit_message
Currently applying b170635... my_commit_message
2 patches left to apply:
b170635... my_commit_message
b170635... my_commit_message
Another nice thing could be to improve the output of 'git status' by
saying the number of patches left to apply.
As an example, it could say:
You are currently rebasing (patch 3/5).
What do you think?
Does the name rebase --status seem appropriate?
Should the output be providing more/less information?
I think a worthy goal would be to arrange things such that the here's
what you do next messages are shared between --status and the code that
stops. I.e., the same code should generate
When you have resolved this problem, run git rebase --continue.
If you prefer to skip this patch, run git rebase --skip instead.
To check out the original branch and stop rebasing, run git rebase --abort.
in both cases. Naturally --status should also explain how it got into
this state, as you outlined above.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 v3 00/28] Follow perlcritic's recommandations
So, do I send a last version of the patch? What is left is quick fix: - removing whitespace in [18/28] - typo in [09/28] - better line split in [22/28] I already fixed first two problems, so it would be done rapidly. -- Célestin Matte -- 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: New feature discussion: git rebase --status
John Keeping j...@keeping.me.uk writes: The one piece of information that I often want is the SHA1 of the commit that is currently being applied. Currently I have to look through my scrollback for the stopping message or poke around in .git/. Having that in the output of git status would be really nice, ... and should be rather easy as it is the content of .git/rebase-merge/stopped-sha Perhaps git status could say stg like (applying 1d3fb08, 2/5) output format you've posted is a big improvement over what we have at the moment for this case. My idea when I wrote the item on the wiki was to keep the a very short summary in git status, and to put all the information one could whish in a separate command. I'd describe it as a complement more than an improvement ;-). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 v4 1/2] status: introduce status.short to enable --short by default
From: Jorge Juan Garcia Garcia jorge-juan.garcia-gar...@ensimag.imag.fr
Some people always run 'git status -s'.
The configuration variable status.short allows to set it by default.
Signed-off-by: Jorge Juan Garcia Garcia
jorge-juan.garcia-gar...@ensimag.imag.fr
Signed-off-by: Mathieu Lienard--Mayor mathieu.lienard--ma...@ensimag.imag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
Changes since v3:
- Changes the order of the test between expected and actual.
- Format by default when status.short is not defined adequately.
- The number of test is the same (all posibilities that I wanted tested).
---
Documentation/config.txt |4
builtin/commit.c |7 +++
t/t7508-status.sh| 35 +++
3 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..1983bf7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2066,6 +2066,10 @@ status.relativePaths::
relative to the repository root (this was the default for Git
prior to v1.5.4).
+status.short::
+ Set to true to enable --short by default in linkgit:git-status[1].
+ The option --no-short takes precedence over this variable.
+
status.showUntrackedFiles::
By default, linkgit:git-status[1] and linkgit:git-commit[1] show
files which are not currently tracked by Git. Directories which
diff --git a/builtin/commit.c b/builtin/commit.c
index 1621dfc..075a91c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1112,6 +1112,13 @@ static int git_status_config(const char *k, const char
*v, void *cb)
s-submodule_summary = -1;
return 0;
}
+ if (!strcmp(k, status.short)) {
+ if (git_config_bool(k, v))
+ status_format = STATUS_FORMAT_SHORT;
+ else
+ status_format = STATUS_FORMAT_NONE;
+ return 0;
+ }
if (!strcmp(k, status.color) || !strcmp(k, color.status)) {
s-use_color = git_config_colorbool(k, v);
return 0;
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index e2ffdac..3c0818b 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1335,4 +1335,39 @@ test_expect_failure '.git/config ignore=all suppresses
submodule summary' '
git config -f .gitmodules --remove-section submodule.subname
'
+test_expect_success 'Setup of test environment' '
+ git config status.showUntrackedFiles no
+'
+
+test_expect_success 'status.short=true same as -s' '
+ git -c status.short=true status actual
+ git status -s expected_short
+ test_cmp expected_short actual
+'
+
+test_expect_success 'status.short=true different from --no-short' '
+ git status --no-short expected_noshort
+ test_must_fail test_cmp expected_noshort actual
+'
+
+test_expect_success 'status.short=true weaker than --no-short' '
+ git -c status.short=true status --no-short actual
+ test_cmp expected_noshort actual
+'
+
+test_expect_success 'status.short=false same as --no-short' '
+ git -c status.short=false status actual
+ git status -s expected_short
+ test_cmp expected_noshort actual
+'
+
+test_expect_success 'status.short=false weaker than -s' '
+ git -c status.short=false status -s actual
+ test_cmp expected_short actual
+'
+
+test_expect_success 'Restore default test environment' '
+ git config --unset status.showUntrackedFiles
+'
+
test_done
--
1.7.8
--
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] Documentation/CommunityGuidelines
Thomas Rast wrote: It has become clear, also in discussion on IRC, that your preferred approach is to fight the fires, attempting to extinguish flames as they happen. Incorrect. I am interested in minimizing occurrences, which is why I started this thread: to calmly and rationally discuss how to achieve that. I have listed many concrete proposals, and justified them with reason. My approach -- and in my perception also that preferred by most of the regulars who have spoken in this whole mess -- is that since there is a fire hazard, it would be more effective firefighting to just remove the hazard, thus preventing future fires. Presumably, Felipe is the fire hazard that we are talking about, and nobody else is to blame. He must be removed to prevent future fires. This is the perception of the regulars, correct? Then why haven't you removed him yet? What are you waiting for? You don't need my approval. Is it because you have realized deep down that you have absolutely no rational argument, and are arguing with an ill-formed majority opinion? I have words, you have words. Why are you incapable of using your words to counter my arguments rationally?Are you so blind that you cannot see the consequences of acting without reason? Tomorrow the majority opinion will dictate that I am a fire hazard and must be removed. Soon, anybody who disagrees with the majority opinion will be removed, and the community will be reduced to a handful of circlejerking yes-men. The git project will die a sad death. And the blood will be on your hands. I infer that in your view, there is an inalienable right for the fire hazard to remain part of the community that you are not willing to give up. I for one no longer have such qualms in this instance. Incorrect. There is no transcendental inalienable right that dictates that fire hazards must remain part of the community. I never made such an irrational argument. I already gave you the example of the survivors on the boat with limited food/water on IRC: it is you who stupidly refused to throw anyone overboard, killing all the survivors; I am the one who said that I would get them to draw sticks to fairly choose who to throw overboard, maximizing the chances of survival of the others. I am making a pragmatic argument, based on what is best for the community; not some stuck-up idealistic bullshit. Further, I tried to help you think through the justice problem, by recommending an accessible course. You have either not gone through it, or have gone through it and learnt nothing. What should I give up? My rationality? Man up, and stop hiding being the veils of majority opinion. _Your_ opinion is that Felipe must be removed from the list without reason. Don't talk for the others. I'm sick of you supporting another person's opinion. Stand up and speak for yourself; leave Haggerty out of it. You have embarrassed yourself and the entire git community today. -- 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 v4 2/2] status:introduce status.branch to enable --branch by default
jorge-juan.garcia-gar...@ensimag.imag.fr writes: +test_expect_success 'status.branch=true different from --no-branch' ' + git -c status.branch=true status -s actual + git status -s --no-branch expected_nobranch Two nitpicks: There are two spaces before , there should be one. If the first git command fails, then you fail to create expected_nobranch and the other tests fail too. Creating expected_nobranch before actual would solve this. You have the same issue in PATCH 1/2. With or without these fixed, Reviewed-by: Matthieu Moy matthieu@grenoble-inp.fr (both patches) -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] git-remote-mediawiki: display message when launched directly
Célestin Matte celestin.ma...@ensimag.fr writes: Users may be confused when they run the perl script directly. A good way to detect this is to check the number of parameters used to call the script, which is never different from 2 in a normal use. Display a proper error message to avoid any confusion. Sounds good, thanks. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] git-remote-mediawiki: display message when launched directly
Users may be confused when they run the perl script directly.
A good way to detect this is to check the number of parameters used to call the
script, which is never different from 2 in a normal use.
Display a proper error message to avoid any confusion.
Signed-off-by: Célestin Matte celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
contrib/mw-to-git/git-remote-mediawiki.perl | 15 +++
1 file changed, 15 insertions(+)
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
b/contrib/mw-to-git/git-remote-mediawiki.perl
index 9c14c1f..9b71972 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -41,6 +41,10 @@ use constant NULL_SHA1 =
;
# Used on Git's side to reflect empty edit messages on the wiki
use constant EMPTY_MESSAGE = '*Empty MediaWiki Message*';
+if (@ARGV != 2) {
+ exit_error_usage();
+}
+
my $remotename = $ARGV[0];
my $url = $ARGV[1];
@@ -156,6 +160,17 @@ while (STDIN) {
## Functions ##
+## error handling
+sub exit_error_usage {
+ die ERROR: git-remote-mediawiki module was not called with a correct
number of\n .
+ parameters\n .
+ You may obtain this error because you attempted to run the
git-remote-mediawiki\n .
+module directly.\n .
+ This module can be used the following way:\n .
+ \tgit clone mediawiki://address of a mediawiki\n .
+ Then, use git commit, push and pull as with every normal git
repository.\n;
+}
+
## credential API management (generic functions)
sub credential_read {
--
1.7.9.5
--
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 1/4] resolve_ref_unsafe(): extract function handle_missing_loose_ref()
The nesting was getting a bit out of hand, and it's about to get
worse.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 55 ++-
1 file changed, 34 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index d17931a..1acce17 100644
--- a/refs.c
+++ b/refs.c
@@ -1193,6 +1193,37 @@ static struct ref_entry *get_packed_ref(const char
*refname)
return find_ref(get_packed_refs(ref_cache), refname);
}
+/*
+ * A loose ref file doesn't exist; check for a packed ref. The
+ * options are forwarded from resolve_safe_unsafe().
+ */
+static const char *handle_missing_loose_ref(const char *refname,
+ unsigned char *sha1,
+ int reading,
+ int *flag)
+{
+ struct ref_entry *entry;
+
+ /*
+* The loose reference file does not exist; check for a packed
+* reference.
+*/
+ entry = get_packed_ref(refname);
+ if (entry) {
+ hashcpy(sha1, entry-u.value.sha1);
+ if (flag)
+ *flag |= REF_ISPACKED;
+ return refname;
+ }
+ /* The reference is not a packed reference, either. */
+ if (reading) {
+ return NULL;
+ } else {
+ hashclr(sha1);
+ return refname;
+ }
+}
+
const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int
reading, int *flag)
{
int depth = MAXDEPTH;
@@ -1218,28 +1249,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
git_snpath(path, sizeof(path), %s, refname);
if (lstat(path, st) 0) {
- struct ref_entry *entry;
-
- if (errno != ENOENT)
+ if (errno == ENOENT)
+ return handle_missing_loose_ref(refname, sha1,
reading, flag);
+ else
return NULL;
- /*
-* The loose reference file does not exist;
-* check for a packed reference.
-*/
- entry = get_packed_ref(refname);
- if (entry) {
- hashcpy(sha1, entry-u.value.sha1);
- if (flag)
- *flag |= REF_ISPACKED;
- return refname;
- }
- /* The reference is not a packed reference, either. */
- if (reading) {
- return NULL;
- } else {
- hashclr(sha1);
- return refname;
- }
}
/* Follow normalized - ie refs/.. symlinks by hand */
--
1.8.3
--
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] Documentation/CommunityGuidelines
On 06/11/2013 03:40 PM, Ramkumar Ramachandra wrote: Is it because you have realized deep down that you have absolutely no rational argument...Why are you incapable of using your words to counter my arguments rationally?Are you so blind that you cannot see the consequences of acting without reason? Ram, you are insulting Thomas the human being rather than addressing his points. Please stop. Tomorrow the majority opinion will dictate that I am a fire hazard and must be removed. Soon, anybody who disagrees with the majority opinion will be removed, and the community will be reduced to a handful of circlejerking yes-men. The git project will die a sad death. And the blood will be on your hands. It is not disagreement that is causing problems; it is the inflammatory tone of the discussion. Civil and constructive disagreement is completely welcome here. But hurtful and offensive discussion is not, even if it is in support of the party line (haha as if there were such a thing). And yes, I know that the word offensive is subjective, but for the sake of this discussion let's take it to mean offensive to the vast majority of a community. Not controversial, not contrarian, not even stupid; I don't think anybody is proposing to prohibit dissent or stupidity. But there is no reason for discussion that is gratuitously aggressive, insulting, or derogatory; such discussion is what I mean by offensive. [...] I already gave you the example of the survivors on the boat with limited food/water on IRC: it is you who stupidly refused to throw anyone overboard, killing all the survivors; I am the one who said that I would get them to draw sticks to fairly choose who to throw overboard, maximizing the chances of survival of the others. I am making a pragmatic argument, based on what is best for the community; not some stuck-up idealistic bullshit. Further, I tried to help you think through the justice problem, by recommending an accessible course. You have either not gone through it, or have gone through it and learnt nothing. Your idea that you can assign Thomas homework in ethics and call him stupid for coming to a different conclusion than you is presumptuous in the extreme. [...] You have embarrassed yourself and the entire git community today. This is also presumptuous, not to mention extremely ironic. In my opinion Thomas's email was calm and reasonable while yours is beyond the pale. Ram, don't just take my opinion on this matter. At the risk of being presumptuous myself, I suggest that you show a copy of your email to somebody whom you know and respect in the real world, somebody who is not immersed in the Git community meltdown. For example, somebody like your mother or father, or a teacher whom you respect, or a member of clergy if you are so inclined. Ask that person's opinion about your email. It is so easy to lose perspective in the Internet. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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 v4 2/2] rm: introduce advice.rmHints to shorten messages
Introduce advice.rmHints to choose whether to display advice or not
when git rm fails. Defaults to true, in order to preserve current behavior.
As an example, the message:
error: 'foo.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)
would look like, with advice.rmHints=false:
error: 'foo.txt' has changes staged in the index
Signed-off-by: Mathieu Lienard--Mayor mathieu.lienard--ma...@ensimag.imag.fr
Signed-off-by: Jorge Juan Garcia Garcia
jorge-juan.garcia-gar...@ensimag.imag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
Changes since v3:
- removal of spaces after redirection
- use of -\EOF to indent tests better
Documentation/config.txt |3 +++
advice.c |2 ++
advice.h |1 +
builtin/rm.c |3 ++-
t/t3600-rm.sh| 29 +
5 files changed, 37 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..eb04479 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -199,6 +199,9 @@ advice.*::
amWorkDir::
Advice that shows the location of the patch file when
linkgit:git-am[1] fails to apply it.
+ rmHints::
+ In case of failure in the output of linkgit:git-rm[1],
+ show directions on how to proceed from the current state.
--
core.fileMode::
diff --git a/advice.c b/advice.c
index a8deee6..a4c169c 100644
--- a/advice.c
+++ b/advice.c
@@ -14,6 +14,7 @@ int advice_resolve_conflict = 1;
int advice_implicit_identity = 1;
int advice_detached_head = 1;
int advice_set_upstream_failure = 1;
+int advice_rm_hints = 1;
static struct {
const char *name;
@@ -33,6 +34,7 @@ static struct {
{ implicitidentity, advice_implicit_identity },
{ detachedhead, advice_detached_head },
{ setupstreamfailure, advice_set_upstream_failure },
+ { rmhints, advice_rm_hints },
/* make this an alias for backward compatibility */
{ pushnonfastforward, advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 94caa32..36104c4 100644
--- a/advice.h
+++ b/advice.h
@@ -17,6 +17,7 @@ extern int advice_resolve_conflict;
extern int advice_implicit_identity;
extern int advice_detached_head;
extern int advice_set_upstream_failure;
+extern int advice_rm_hints;
int git_default_advice_config(const char *var, const char *value);
void advise(const char *advice, ...);
diff --git a/builtin/rm.c b/builtin/rm.c
index e284db3..70df77c 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -49,7 +49,8 @@ static void print_error_files(struct string_list *files_list,
strbuf_addf(err_msg,
\n%s,
files_list-items[i].string);
- strbuf_addstr(err_msg, hints_msg);
+ if (advice_rm_hints)
+ strbuf_addstr(err_msg, hints_msg);
*errs = error(%s, err_msg.buf);
strbuf_release(err_msg);
}
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 902993b..5c87b55 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -707,6 +707,18 @@ test_expect_success 'rm files with different staged
content' '
test_i18ncmp expect actual
'
+test_expect_success 'rm files with different staged content without hints' '
+ cat expect -\EOF
+ error: the following files have staged content different from both the
+ file and the HEAD:
+ bar.txt
+ foo.txt
+ EOF
+ echo content2 foo.txt
+ echo content2 bar.txt
+ test_must_fail git -c advice.rmhints=false rm foo.txt bar.txt 2actual
+ test_i18ncmp expect actual
+'
test_expect_success 'rm file with local modification' '
cat expect -\EOF
@@ -720,6 +732,15 @@ test_expect_success 'rm file with local modification' '
test_i18ncmp expect actual
'
+test_expect_success 'rm file with local modification without hints' '
+ cat expect -\EOF
+ error: the following file has local modifications:
+ bar.txt
+ EOF
+ echo content4 bar.txt
+ test_must_fail git -c advice.rmhints=false rm bar.txt 2actual
+ test_i18ncmp expect actual
+'
test_expect_success 'rm file with changes in the index' '
cat expect -\EOF
@@ -734,6 +755,14 @@ test_expect_success 'rm file with changes in the index' '
test_i18ncmp expect actual
'
+test_expect_success 'rm file with changes in the index without hints' '
+ cat expect -\EOF
+ error: the following file has changes staged in the index:
+ foo.txt
+ EOF
+ test_must_fail git -c advice.rmhints=false rm foo.txt 2actual
+ test_i18ncmp expect actual
+'
test_expect_success 'rm files with two different errors' '
cat expect -\EOF
--
1.7.8
--
To
[PATCH v4 1/2] rm: better error message on failure for multiple files
When 'git rm' fails, it now displays a single message
with the list of files involved, instead of displaying
a list of messages with one file each.
As an example, the old message:
error: 'foo.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)
error: 'bar.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)
would now be displayed as:
error: the following files have changes staged in the index:
foo.txt
bar.txt
(use --cached to keep the file, or -f to force removal)
Signed-off-by: Mathieu Lienard--Mayor mathieu.lienard--ma...@ensimag.imag.fr
Signed-off-by: Jorge Juan Garcia Garcia
jorge-juan.garcia-gar...@ensimag.imag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
Changes since v3:
- rename function print_error_files()
- use of strbuf_release to avoid leaking
- change of a forgotten message, now using print_error_files() aswell
- removal of useless braces
- use of Q_() to deal with plurals correctly
- removal of spaces after redirection
- use of -\EOF to indent tests better
builtin/rm.c | 95 +---
t/t3600-rm.sh | 67
2 files changed, 143 insertions(+), 19 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..e284db3 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -36,11 +36,32 @@ static int get_ours_cache_pos(const char *path, int pos)
return -1;
}
+static void print_error_files(struct string_list *files_list,
+ const char *main_msg,
+ const char *hints_msg,
+ int *errs)
+{
+ if (files_list-nr) {
+ struct strbuf err_msg = STRBUF_INIT;
+ int i;
+ strbuf_addstr(err_msg, main_msg);
+ for (i = 0; i files_list-nr; i++)
+ strbuf_addf(err_msg,
+ \n%s,
+ files_list-items[i].string);
+ strbuf_addstr(err_msg, hints_msg);
+ *errs = error(%s, err_msg.buf);
+ strbuf_release(err_msg);
+ }
+}
+
static int check_submodules_use_gitfiles(void)
{
int i;
int errs = 0;
+ struct string_list files = STRING_LIST_INIT_NODUP;
+
for (i = 0; i list.nr; i++) {
const char *name = list.entry[i].name;
int pos;
@@ -61,11 +82,17 @@ static int check_submodules_use_gitfiles(void)
continue;
if (!submodule_uses_gitfile(name))
- errs = error(_(submodule '%s' (or one of its nested
-submodules) uses a .git directory\n
-(use 'rm -rf' if you really want to
remove
-it including all of its history)), name);
+ string_list_append(files, name);
}
+ print_error_files(files,
+ Q_(the following submodule (or one of its nested
+submodules)\n uses a .git directory:,
+the following submodules (or one of its nested
+submodules)\n use a .git directory:,
+files.nr),
+ _(\n(use 'rm -rf' if you really want to remove
+ it including all of its history)),
+ errs);
return errs;
}
@@ -82,6 +109,11 @@ static int check_local_mod(unsigned char *head, int
index_only)
int i, no_head;
int errs = 0;
+ struct string_list files_staged = STRING_LIST_INIT_NODUP;
+ struct string_list files_cached = STRING_LIST_INIT_NODUP;
+ struct string_list files_submodule = STRING_LIST_INIT_NODUP;
+ struct string_list files_local = STRING_LIST_INIT_NODUP;
+
no_head = is_null_sha1(head);
for (i = 0; i list.nr; i++) {
struct stat st;
@@ -171,29 +203,54 @@ static int check_local_mod(unsigned char *head, int
index_only)
*/
if (local_changes staged_changes) {
if (!index_only || !(ce-ce_flags CE_INTENT_TO_ADD))
- errs = error(_('%s' has staged content
different
-from both the file and the HEAD\n
-(use -f to force removal)),
name);
+ string_list_append(files_staged, name);
}
else if (!index_only) {
if (staged_changes)
- errs = error(_('%s' has changes staged in the
index\n
-(use --cached to keep the file,
-
Re: [PATCH v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions
Célestin Matte celestin.ma...@ensimag.fr writes:
Subroutines' parameters should be affected to variable before doing anything
else
Besides, existing instruction affected a variable inside a if, which break
Git's coding style
I think s/affect/assign/g is what you meant.
By the way, I often see two styles of the let's take arguments into
parameters before doing anything else at the beginning of subs:
my ($namespace) = @_;
my $namespace = shift;
My impression has been that both are equally common, but the latter
is done more often when picking out small and fixed number of
mandatory parameters upfront (and later, optional parameters are
used by directly reading what remains in @_). Does Perlcritique say
anything about this issue?
Signed-off-by: Célestin Matte celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
contrib/mw-to-git/git-remote-mediawiki.perl |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
b/contrib/mw-to-git/git-remote-mediawiki.perl
index 431e063..2db6467 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -1333,7 +1333,8 @@ sub get_mw_namespace_id {
}
sub get_mw_namespace_id_for_page {
- if (my ($namespace) = $_[0] =~ /^([^:]*):/) {
+ my $namespace = shift;
+ if ($namespace =~ /^([^:]*):/) {
return get_mw_namespace_id($namespace);
} else {
return;
--
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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 7:33 AM, Thomas Rast tr...@inf.ethz.ch wrote: My approach -- and in my perception also that preferred by most of the regulars who have spoken in this whole mess -- is that since there is a fire hazard, it would be more effective firefighting to just remove the hazard, thus preventing future fires. You would make an excellent evil dictator. A benevolent dictator like Linus Torvalds knows better, in the LKML fire hazards are not removed, they are ignored before any flames go up. This achieves the best of both worlds; if the person is truly vicious, nothing happens, but if there's something to it, a person that doesn't offend so easily might have a fruitful discussion, while the rest ignore the thread. In a flamewar everyone is guilty. Apparently you never learned that but he started it! is not a defense worthy of an adult, hell, even most children know that. -- 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] Documentation/CommunityGuidelines
Hi, Before going to your arguments, can you stop conveniently *ignoring* my argument and answer this questions? When two children fight, who has the blame? The one that threw the first punch? Or the one that returned it? On Tue, Jun 11, 2013 at 9:40 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 06/11/2013 03:40 PM, Ramkumar Ramachandra wrote: Is it because you have realized deep down that you have absolutely no rational argument...Why are you incapable of using your words to counter my arguments rationally?Are you so blind that you cannot see the consequences of acting without reason? Ram, you are insulting Thomas the human being rather than addressing his points. Please stop. How is Ram insulting Thomas? By implying that Thomas is a human being? That he is imperfect and is making a mistake? My gosh! How offensive! Tomorrow the majority opinion will dictate that I am a fire hazard and must be removed. Soon, anybody who disagrees with the majority opinion will be removed, and the community will be reduced to a handful of circlejerking yes-men. The git project will die a sad death. And the blood will be on your hands. It is not disagreement that is causing problems; it is the inflammatory tone of the discussion. Civil and constructive disagreement is completely welcome here. But hurtful and offensive discussion is not, even if it is in support of the party line (haha as if there were such a thing). The difference between the two is totally and completely subjective, and only a despot would be conformable parting judgment about which is which. And yes, I know that the word offensive is subjective, but for the sake of this discussion let's take it to mean offensive to the vast majority of a community. Rule of the mob. How wise. [...] I already gave you the example of the survivors on the boat with limited food/water on IRC: it is you who stupidly refused to throw anyone overboard, killing all the survivors; I am the one who said that I would get them to draw sticks to fairly choose who to throw overboard, maximizing the chances of survival of the others. I am making a pragmatic argument, based on what is best for the community; not some stuck-up idealistic bullshit. Further, I tried to help you think through the justice problem, by recommending an accessible course. You have either not gone through it, or have gone through it and learnt nothing. Your idea that you can assign Thomas homework in ethics and call him stupid for coming to a different conclusion than you is presumptuous in the extreme. He didn't call him stupid, he said he was acting stupidly, big difference. [...] You have embarrassed yourself and the entire git community today. This is also presumptuous, not to mention extremely ironic. In my opinion Thomas's email was calm and reasonable while yours is beyond the pale. Of course it would be. In a witch hunt nobody sees what's wrong with burning the witch... until you become it. It's fine for Thomas Rast to call me a fire hazard, which ironically is itself an inflammatory comment. But it's not OK for Ramkumar to say that Thomas is acting stupidly *in this particular instance*. Double standards much? Ram, don't just take my opinion on this matter. At the risk of being presumptuous myself, I suggest that you show a copy of your email to somebody whom you know and respect in the real world, somebody who is not immersed in the Git community meltdown. For example, somebody like your mother or father, or a teacher whom you respect, or a member of clergy if you are so inclined. Ask that person's opinion about your email. I can offer my own perspective; I think Ramkumar's tone is not particularly useful, but to concentrate on *how* he is saying things, instead of *what* he is saying is an even bigger mistake, specially because it's *you* the one that is making it. You should concentrate on what *you* do, not what others do. Otherwise you will be forever frustrated. You have chosen to ignore *all* of Ramkumar's arguments, and all your arguments can be summarized as I don't like your tone, and by doing that, you have lost even more touch of the discussion than what you accused Ramkumar of doing. You have even violated two your own guidelines: * Conduct disagreements on a technical, not a personal, level. * It is not OK to use these guidelines as a stick with which to beat supposed violators. You have also violated some of Ramkumar: 0. You do not take offense, no matter what. In this particular case, you are taking offense by proxy, which might be even worst. 1. You do not take sides or vote. 2. You stop pointing fingers. I would also suggest another guideline based on Paul Graham's guide How to Disagree[1]. * Do not respond to tone. Concentrate on *what* is being said, not *how* it is being said. If the worst thing you can say about something is to criticize its tone, you're not saying much. A bad tone is highly
Re: [PATCH] Documentation/CommunityGuidelines
Felipe Contreras felipe.contre...@gmail.com writes:
On Tue, Jun 11, 2013 at 7:33 AM, Thomas Rast tr...@inf.ethz.ch wrote:
My approach -- and in my perception also that preferred by most of the
regulars who have spoken in this whole mess -- is that since there is a
fire hazard, it would be more effective firefighting to just remove the
hazard, thus preventing future fires.
You would make an excellent evil dictator.
A benevolent dictator like Linus Torvalds knows better, in the LKML
fire hazards are not removed, they are ignored before any flames go
up. This achieves the best of both worlds; if the person is truly
vicious, nothing happens, but if there's something to it, a person
that doesn't offend so easily might have a fruitful discussion, while
the rest ignore the thread.
In a flamewar everyone is guilty. Apparently you never learned that
but he started it! is not a defense worthy of an adult, hell, even
most children know that.
[Yes, I should let this thread die, but you are offering me too good a
chance to pass up.]
It's funny that you would mention Linus, considering there's at least
one instance on record where he broke almost every rule that Ram
attempted to set out in the thread starter, calling you among other
things a fucking moron and telling you to go away.
https://lkml.org/lkml/2012/4/12/434
In case our readers wonder why the flame war suddenly died out at a
depth of about 19 replies, fear not, for the story continued:
https://plus.google.com/108736516888538655285/posts/7QSVy8taWgC
And before you try to shoot that down, please make sure your argument
also applies to the recurring situation of your repeatedly ignoring
SubmittingPatches as far as commit messages go against explicit requests
to stop that.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 v3 21/28] git-remote-mediawiki: Put long code into a subroutine
Célestin Matte celestin.ma...@ensimag.fr writes:
Signed-off-by: Célestin Matte celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
contrib/mw-to-git/git-remote-mediawiki.perl | 50
+--
1 file changed, 31 insertions(+), 19 deletions(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
b/contrib/mw-to-git/git-remote-mediawiki.perl
index c404e7b..a66cef4 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -126,28 +126,13 @@ $wiki_name =~ s{[^/]*://}{};
$wiki_name =~ s/^.*@//;
# Commands parser
-my @cmd;
+my @cmds;
I am guessing that the new sub, parse_command, uses a local @cmd and
this is an attempt to avoid using the same name, but this renaming
of the variable is not explained.
I also wonder if you need this global @cmd/@cmds. Instead of
passing cmdref, wouldn't it be simpler to have the helper split the
line, i.e. something like...
sub parse_command {
my ($line) = @_;
my @cmd = split(/ /, $line);
unless (defined @cmd) {
return 0;
}
... check capabilities, list, etc
return 1;
}
while (STDIN) {
chomp;
if (!parse_command($_)) {
unknown command, aborting
last;
}
}
--
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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 10:41 AM, Thomas Rast tr...@inf.ethz.ch wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Tue, Jun 11, 2013 at 7:33 AM, Thomas Rast tr...@inf.ethz.ch wrote: My approach -- and in my perception also that preferred by most of the regulars who have spoken in this whole mess -- is that since there is a fire hazard, it would be more effective firefighting to just remove the hazard, thus preventing future fires. You would make an excellent evil dictator. A benevolent dictator like Linus Torvalds knows better, in the LKML fire hazards are not removed, they are ignored before any flames go up. This achieves the best of both worlds; if the person is truly vicious, nothing happens, but if there's something to it, a person that doesn't offend so easily might have a fruitful discussion, while the rest ignore the thread. In a flamewar everyone is guilty. Apparently you never learned that but he started it! is not a defense worthy of an adult, hell, even most children know that. [Yes, I should let this thread die, but you are offering me too good a chance to pass up.] It's funny that you would mention Linus, considering there's at least one instance on record where he broke almost every rule that Ram attempted to set out in the thread starter, calling you among other things a fucking moron and telling you to go away. https://lkml.org/lkml/2012/4/12/434 Yet, this fire hazzard was not removed, nor was there any threat of doing so at any point in the discussion. In case our readers wonder why the flame war suddenly died out at a depth of about 19 replies, fear not, for the story continued: https://plus.google.com/108736516888538655285/posts/7QSVy8taWgC You sure like your ad hominem arguments. Perhaps it would be wise to get the timeline right. The Google+ discussion you are pointing to happened *before* the thread ended, even Linus replied after that: http://article.gmane.org/gmane.linux.drivers.ath9k.devel/8675 And before you try to shoot that down, please make sure your argument also applies to the recurring situation of your repeatedly ignoring SubmittingPatches as far as commit messages go against explicit requests to stop that. There's nothing to shoot down, you are not making any argument. You are doing nothing but throwing personal attacks. If what you are suggesting is that we should do what they did in the LKML; they did *exactly* what I'm suggesting you should do; *ignore* the people that you think are vicious. Is that what you are suggesting? -- 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] Documentation/CommunityGuidelines
Michael Haggerty mhag...@alum.mit.edu writes:
* Accept reviewers' comments gratefully and take them very seriously.
Show that you appreciate the help by giving the reviewer the benefit of
the doubt. If, after careful consideration, you find that you cannot
agree with a reviewer's suggestion, explain your reasoning carefully
without taking or giving offense, and seek compromise.
I'm not sure yet how to phrase it, but I have come to the conclusion
that the dual nature of reviews is part of the problem.
On the one hand, reviews are code criticism: they are extra work spent
by the reviewer to try and help you improve your work.
On the other hand, they are also quality checks. Reviews serve to spot
bugs, misdesigns, noncompliance with project standards, etc. before they
ever go into the code base.
The problems start when these start having a contradictory impact on the
correct course of action in a discussion, or in the longer term in
dealing with a person. For example, I have attempted to deal with
Felipe at one point by refusing to review, i.e., ignore the email.
However, this must be weighed against the requirements of the second
kind of review. So while it is exceedingly easy for everyone to say
just ignore the flamebait, the flamewars keep recurring because this
gatekeeper type of review continues to be necessary, and continues to
elicit nonconstructive responses.
The easy solution is to simply stop taking patches from Felipe, but
opens pandora's box w.r.t. the just application of such a measure, as
Ram has noted repeatedly.
Yet that is the only measure that I currently know that both keeps up
code review standards and has any hope of improving the current climate.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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 v4 31/45] rebase: trivial cleanup
Fredrik Gustafsson iv...@iveqy.com writes: Here we have two sh-scripts (git rebase and git am) interacting with each other. Both uses GIT_QUIET, Correct. so if GIT_QUIET already is set by the caller (git rebase) the callee doesn't have to set it to. Incorrect. git rebase invokes git am, not dot-sources it, so it does not propagate. Both do dot-source git-sh-setup, which clears GIT_QUIET pretty early, so even GIT_QUIET exported into the environment by mistake will not affect them. However GIT_QUIET is undocumented for git-am (in Git Manual). If we translate git am to C/ruby/python/perl/etc. will we catch this? I think it is irrelevant. git-am uses many shell variables, e.g. HAS_HEAD, interactive, abort_safety, etc. but they are implementation details of the program and there is no point documenting it, and somebody rewriting it in C does not have to stick to the same name. What may deserve to be documented in Documentation/technical/ somewhere is the way how some shell variables (OPTIONS_SPEC, OPTIONS_KEEPDASHDASH, LONG_USAGE, USAGE, GIT_REFLOG_ACTION, etc.) are used in git-sh-setup, which is a collection of helper shell functions for use in scripted Porcelains that dot-source it. It allows the scripted Porcelains to say things like: die message say message and the latter pays attention to GIT_QUIET. This raises a few more generall questions: do we already pass information between processes(!) with enviroment variables? And is this documented the way it should be? There are a few used as implementation details, I think. GIT_QUIET is not among them. -- 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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 11:10 AM, Thomas Rast tr...@inf.ethz.ch wrote: Michael Haggerty mhag...@alum.mit.edu writes: * Accept reviewers' comments gratefully and take them very seriously. Show that you appreciate the help by giving the reviewer the benefit of the doubt. If, after careful consideration, you find that you cannot agree with a reviewer's suggestion, explain your reasoning carefully without taking or giving offense, and seek compromise. I'm not sure yet how to phrase it, but I have come to the conclusion that the dual nature of reviews is part of the problem. On the one hand, reviews are code criticism: they are extra work spent by the reviewer to try and help you improve your work. On the other hand, they are also quality checks. Reviews serve to spot bugs, misdesigns, noncompliance with project standards, etc. before they ever go into the code base. The problems start when these start having a contradictory impact on the correct course of action in a discussion, or in the longer term in dealing with a person. For example, I have attempted to deal with Felipe at one point by refusing to review, i.e., ignore the email. However, this must be weighed against the requirements of the second kind of review. So while it is exceedingly easy for everyone to say just ignore the flamebait, the flamewars keep recurring because this gatekeeper type of review continues to be necessary, and continues to elicit nonconstructive responses. Why do you assume the review is for the patch submitter? You can reply so your review is stored on the record, and the maintainer, Junio, can see it. Then you can ignore the fallout. I think this type of review is hurtful, because the fact that you said some words doesn't mean you are right, and you might be blocking a perfectly good patch by not following up the counter arguments. Presumably you are not worried about that, and you would be content with simply blocking all my patches. Whatever floats your boat. -- 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
Exact format of tree objets
Is there any official documentation of tree objets format? Are tree objects encoded specially in some way? How can I parse the inflated contents of a tree object? We're suspecting that there is some kind of special format or encoding, because the command git cat-file -p sha show me the expected output, something like: 100644 blob 2beae51a0e14b3167fd7e81119972caef95779f4.gitignore 100644 blob 7c817960e954f0278a6eee8d58611f61445167e8LICENSE.txt 100644 blob 30e849cba985d74bfd29696f6dee5a40abaacb03README ... While git cat-file tree sha generate an strange output, which indicate some kink of encoding problem. Something like: 100644 .gitignore+��▒,��Wy�100644 LICENSE.txt|�y`�T�'�n��XaaDQg�100644 README0�I˩��K�) Thanks, -- Chico Sokol -- 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
Tracking vendor release with Git
-- 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
Tracking vendor release with Git
Hi, I'm trying to setup a workflow to track vendor releases (upstream). Each new release are provided as an archive of source code, data, documentation, etc. For each vendor releases, fixes need to be applied before making them available to users (downstream). Seems to be a rather common use case, applied by most Linux distribution for decades. In my case, on top of each releases, a common set of patches will be applied, the biggest, the most intrusive one, being converting CRLF to LF using dos2unix, the others being small portability fixes. In this case, fixes are not going to be applied by upstream. I'm trying to design (copy ;) a workflow with following properties, in order of importance: 1- I wish to keep a branch with each new vendor release as a commit. This branch's history is only about vendor releases, so it's easy to read the changelog of the vendor releases with command such as git log vendor-release-branch 2- I'd like to ease the process of applying our patches on top of each new vendor release, eg. reduces the likeliness of conflicts. 3- I wish to keep a branch with each new fixed vendor release as a commit. Just like the upstream vendor-release-branch, only one commit per release, so it's easy to read the changelog of the vendor releases with command such as git log patched-release-branch (4- I wish nobody is going to think about ponies ...) The workflow, as I tried to implement it is, can be described like this: For each release, the new sources are imported in the upstream branch, then the previous branch holding fixes is rebased against the updated upstream branch, and finally, the updated fixes branch is merged in the downstream branch. Please find a diagram trying to explain the following workflow: Input Output . *(1) . ./ \. . / \ . . / \ . . / \ . ./ \. / \ Upstream release 1 | | \ v | -* | |\ | | \ | | || | v| Upstream release 2 | * fix 1 | \ | \ | \ |\ v \| -* Downstream release 1 \ v | --* | |\ | | \ | | || | v| | * fix 1' | | || Upstream release 3 | v| \ | * fix 2' | \ | \ | \|\ v \ | -* Downstream release 2 \ | | \ v | * | |\ | . \ | | || . v| . * fix 1 | | || . v| . * fix 2 | . || | v| * fix 3 | \ | \ v -* Downstream release 3 | . | . . (1) empty root commit I hope someone would come with an easier workflow, more sustainable. At least, please help me find flaws in this workflow, tell me where I can found the documentation of others workflows achieving the same results. BTW while testing the workflow, I tried git merge -s theirs and found it doesn't exist. I thought it would be available for such a common use case. For each merge to the of the fixes branch to the downstream branch, something like a whole new content is dropped to the branch. The previous content is still needed but its only for history. So I prefer to overwrite the content of the downstream branch, instead of fixing each conflicts manually when using git merge with default strategy and option. In some article[1], I've found an explanation of the lack of git merge -s theirs, quoting
Re: New feature discussion: git rebase --status
On 11 June 2013 06:19, Matthieu Moy matthieu@grenoble-inp.fr wrote: John Keeping j...@keeping.me.uk writes: The one piece of information that I often want is the SHA1 of the commit that is currently being applied. Currently I have to look through my scrollback for the stopping message or poke around in .git/. Having that in the output of git status would be really nice, ... and should be rather easy as it is the content of .git/rebase-merge/stopped-sha Perhaps git status could say stg like (applying 1d3fb08, 2/5) output format you've posted is a big improvement over what we have at the moment for this case. My idea when I wrote the item on the wiki was to keep the a very short summary in git status, and to put all the information one could whish in a separate command. I'd describe it as a complement more than an improvement ;-). Having git status display (even more) context sensitive information during git rebase or git merge would be very welcome. Please, if at all possible, don't make that a separate command. -- 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 v4 31/45] rebase: trivial cleanup
On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote: It's not removed. It's simply moved. Sorry about that, I wasn't paying enough attention. But why are you moving it? All other arguments to git am is set in git-rebase.sh, why just set -q just before the invokation in git-rebase--am.sh? -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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: New feature discussion: git rebase --status
On Tue, Jun 11, 2013 at 10:18 AM, Hilco Wijbenga hilco.wijbe...@gmail.com wrote: Having git status display (even more) context sensitive information during git rebase or git merge would be very welcome. Please, if at all possible, don't make that a separate command. I agree. The rebase state etc is something that would be much better in git status output, and would avoid having people learn about another new flag to random commands. Linus -- 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 v4 31/45] rebase: trivial cleanup
On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote: It's not removed. It's simply moved. Sorry about that, I wasn't paying enough attention. But why are you moving it? All other arguments to git am is set in git-rebase.sh, why just set -q just before the invokation in git-rebase--am.sh? Because the next patch checks if there's any arguments meant for 'git am' to switch to am rebase mode. We shouldn't switch to that mode if the only argument to 'git am' is going to be -q. -- 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: Tracking vendor release with Git
I'm trying to setup a workflow to track vendor releases (upstream). Each new release are provided as an archive of source code, data, documentation, etc. I've been doing more or less this. A few comments: I suggest that you not view CRLF-LF as a patch. I would do EOL hygiene as a preprocessing script, with a checked-in script, after unpacking the tarball or whatever, and before 'import'. Otherwise it's just going to be too messy. I use vendor.foo as the branch name. If your repo is only for this program, you can ignore this, but otherwise you way want to use subtree merge so that vendor.foo: maps to master:foo (putting foo in a subtree in master). This lets you have multiple upstreams in one repo, which is useful for system building more than maintaining. I would avoid rebase. You are essentially merging someone else's branch (that they aren't putting in git, but you are with the vendor.foo) into your master. With regular merge, you can still diff, but the natural history will be right. With rebase each local version as you call it will have different commits that will not have clear ancestry. pgpKcjZY3cZsB.pgp Description: PGP signature
Re: [PATCH] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: - There may be pieces of usefully reusable code buried in builtin/*.o; - By definition, any code (piece of data or function definition) in builtin/*.o cannot be used in standalone binaries, because all of builtin/*.o expect to link with git.o and expect their cmd_foo() getting called from main in it; - By moving the useful reusable pieces ont of builtin/*.o and adding them to libgit.a, these pieces become usable from standalone binaries as well. What if these reusable pieces should not be used by standalone binaries? I am not sure what you mean. A piece is either reusable or not. When would one piece _be_ reusable and should *not* be used in one context but not in another? There are distinctions between being useful and usable, but I think the zeroth order of approximation when thinking about this is to think builtin/*.o as set of subroutines called by git.c::main(). These set of subroutines may call out to more generic helper functions that are usable from anywhere both within builtins and also from standalone. They may also call to their own helper functions that were originally designed to support only their use by the original caller from somewhere in builtin/*.o (most commonly in the same file, marked as static). The general direction, if we want to have an improve libgit.a, should be to see if the functions and their data that are private to builtin/*.o can be used from standalone, either as they are or with more generalization, and turn them from helpers specific to one cmd_foo() into more generally useful library-ish functions. There may be pieces in the callchain from that entry point to cmd_foo() that are implementation details of git.c::main(); for example the loop that does command dispatching to check with builtins, external commands that begin with git-, and aliases, is one of them, and would not be usable (nor it is useful) outside the context of git aggregate binary. But there are things that ought to be usable that are currently in builtin/*.o, which prevents them from being used by standalone binaries. If a remote helper binary that is standalone wants to call create_note(), it is not sufficient to make it non-static in builtin/notes.o, for example. But if it is moved outside builtin/notes.o, it becomes usable. I think the git notes, being one of the most recent additions, haven't gone through enough round of refactoring to come to the best separation between library-ish part (i.e. could be in notes.o, even though it is mostly about the underlying data structure manipulation and contains no higher-level operations like actually creating and copying, which might want to be in a separate source notes-lib.o) and its CLI implementation builtin/notes.o. But this doesn't answer the question; what about code that is shared between builtins, but cannot be used by standalone programs? Again, I do not know what you mean by cannot here. My tentative answer to that question is the eventual goal should be not to have any code in that class, and that is a reasonable goal we can achieve once we refactor what ought to be reusable out of builtin/*.o. What are the examples you have in mind, code that we want to forbid standalone from using? -- 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 v4 31/45] rebase: trivial cleanup
On Tue, Jun 11, 2013 at 12:26:42PM -0500, Felipe Contreras wrote: On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote: It's not removed. It's simply moved. Sorry about that, I wasn't paying enough attention. But why are you moving it? All other arguments to git am is set in git-rebase.sh, why just set -q just before the invokation in git-rebase--am.sh? Because the next patch checks if there's any arguments meant for 'git am' to switch to am rebase mode. We shouldn't switch to that mode if the only argument to 'git am' is going to be -q. Okay, that make sense. How about rephrase the commit message and add this explanation. It's really not a cleanup but a preparation for the next patch. If I was a maintainer and only got this patch I would reject it. Every patch in a patch serie should be justified to be applied as a single patch, yes? -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 12:33 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: - There may be pieces of usefully reusable code buried in builtin/*.o; - By definition, any code (piece of data or function definition) in builtin/*.o cannot be used in standalone binaries, because all of builtin/*.o expect to link with git.o and expect their cmd_foo() getting called from main in it; - By moving the useful reusable pieces ont of builtin/*.o and adding them to libgit.a, these pieces become usable from standalone binaries as well. What if these reusable pieces should not be used by standalone binaries? I am not sure what you mean. A piece is either reusable or not. It can be reusable for A, but not for B. A being the 'git' binary, B being other standalone binaries. But this doesn't answer the question; what about code that is shared between builtins, but cannot be used by standalone programs? Again, I do not know what you mean by cannot here. My tentative answer to that question is the eventual goal should be not to have any code in that class, and that is a reasonable goal we can achieve once we refactor what ought to be reusable out of builtin/*.o. What are the examples you have in mind, code that we want to forbid standalone from using? init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would need that. If you disagree, show me an example. -- 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 v4 31/45] rebase: trivial cleanup
On Tue, Jun 11, 2013 at 12:41 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Tue, Jun 11, 2013 at 12:26:42PM -0500, Felipe Contreras wrote: On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson iv...@iveqy.com wrote: On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote: It's not removed. It's simply moved. Sorry about that, I wasn't paying enough attention. But why are you moving it? All other arguments to git am is set in git-rebase.sh, why just set -q just before the invokation in git-rebase--am.sh? Because the next patch checks if there's any arguments meant for 'git am' to switch to am rebase mode. We shouldn't switch to that mode if the only argument to 'git am' is going to be -q. Okay, that make sense. How about rephrase the commit message and add this explanation. It's really not a cleanup but a preparation for the next patch. If I was a maintainer and only got this patch I would reject it. Every patch in a patch serie should be justified to be applied as a single patch, yes? Yes, but it doesn't matter, because the maintainer has made it clear he is not going to even read this patch series. -- 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] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: What are the examples you have in mind, code that we want to forbid standalone from using? init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would need that. If you disagree, show me an example. Nothing would need that, if you are talking about the current codebase, I would agree that nothing would link to it. But that is not a good justification for closing door to others that come later who may want to have a standalone that would want to use it. Think about rewriting filter-branch.sh in C but not as a built-in, for example. -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 12:58 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: What are the examples you have in mind, code that we want to forbid standalone from using? init_copy_notes_for_rewrite(). Nothing outside the 'git' binary would need that. If you disagree, show me an example. Nothing would need that, if you are talking about the current codebase, I would agree that nothing would link to it. But that is not a good justification for closing door to others that come later who may want to have a standalone that would want to use it. Think about rewriting filter-branch.sh in C but not as a built-in, for example. Why would anybody rewrite filter-branch, and not make it a builtin? It should be a builtin. That's the whole point of builtins. Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? If you are seriously considering the highly unlikely hypothetical standalone git-filter-branch scenario, you should consider the even more likely scenario where somebody needs to access code from ./builtin/*.o; that scenario is not even hypothetical, we know it's happened multiple times, and we know it's going to happen again. -- 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 v4 1/2] rm: better error message on failure for multiple files
Mathieu Lienard--Mayor mathieu.lienard--ma...@ensimag.imag.fr
writes:
+static void print_error_files(struct string_list *files_list,
+ const char *main_msg,
+ const char *hints_msg,
+ int *errs)
+{
+ if (files_list-nr) {
+ struct strbuf err_msg = STRBUF_INIT;
+ int i;
+ strbuf_addstr(err_msg, main_msg);
+ for (i = 0; i files_list-nr; i++)
+ strbuf_addf(err_msg,
+ \n%s,
+ files_list-items[i].string);
+ strbuf_addstr(err_msg, hints_msg);
+ *errs = error(%s, err_msg.buf);
+ strbuf_release(err_msg);
+ }
+}
+
static int check_submodules_use_gitfiles(void)
{
int i;
int errs = 0;
+ struct string_list files = STRING_LIST_INIT_NODUP;
+
for (i = 0; i list.nr; i++) {
The blank after the initialization lines before the first statement
is very much welcom, but please drop the blank line before this new
initialization, i.e.
int i;
int errs = 0;
struct string_list files = STRING_LIST_INIT_NODUP;
for (i = 0; i list.nr; i++) {
...
@@ -61,11 +82,17 @@ static int check_submodules_use_gitfiles(void)
continue;
if (!submodule_uses_gitfile(name))
+ string_list_append(files, name);
}
+ print_error_files(files,
+ Q_(the following submodule (or one of its nested
+ submodules)\n uses a .git directory:,
+ the following submodules (or one of its nested
+ submodules)\n use a .git directory:,
+ files.nr),
+ _(\n(use 'rm -rf' if you really want to remove
+ it including all of its history)),
+ errs);
return errs;
No string_list_clear() on files?
}
@@ -82,6 +109,11 @@ static int check_local_mod(unsigned char *head, int
index_only)
int i, no_head;
int errs = 0;
+ struct string_list files_staged = STRING_LIST_INIT_NODUP;
+ struct string_list files_cached = STRING_LIST_INIT_NODUP;
+ struct string_list files_submodule = STRING_LIST_INIT_NODUP;
+ struct string_list files_local = STRING_LIST_INIT_NODUP;
+
...
+ print_error_files(files_local,
+ Q_(the following file has local modifications:,
+ the following files have local modifications:,
+ files_local.nr),
+ _(\n(use --cached to keep the file,
+ or -f to force removal)),
+ errs);
+
No string_list_clear() on files_*?
--
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 v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions
Matthieu Moy matthieu@grenoble-inp.fr writes: my ($namespace) = @_; my $namespace = shift; My impression has been that both are equally common, The second is the most common in git-remote-mediawiki (but I don't have any preference nor know what is recommended elsewhere). I wasn't implying I prefer the former. I was just being curious, and if the latter is more prevalent in the code _and_ Perlcritique does not have any issue with it, it is perfectly fine. Thanks. -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 11:06 AM, Felipe Contreras felipe.contre...@gmail.com wrote: Moreover, if you are going to argue that we shouldn't be closing the door [...] Felipe, you saying if you are going to argue ... to anybody else is kind of ironic. Why is it every thread I see you in, you're being a dick and arguing for some theoretical thing that nobody else cares about? This whole thread has been one long argument about totally pointless things that wouldn't improve anything one way or the other. It's bikeshedding of the worst kind. Just let it go. Linus -- 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] Documentation/CommunityGuidelines
This is an exercise. I can easily be more tactful (as evidenced by other threads), but I'm choosing not to be. I want you to focus on the argument, and not the tone. Michael Haggerty wrote: Ram, you are insulting Thomas the human being rather than addressing his points. Please stop. He doesn't have a point! He makes the assumption that the perception of the regulars is that a fire hazard must be removed from the community. There are absolutely no rational arguments in his email, he violated virtually every rule that we were working towards, and he made an inflammatory comment by calling Felipe a fire hazard. Yes I was particularly harsh, because Rast was particularly irrational. I did not insult him as a human being; I criticized his email which was completely devoid of reason. In case you're wondering, this is what an ad hominem looks like: You are studying a subject that requires extensive application of logic: combinatorial structures and algorithms at ETH Zurich. You live in a well-to-do progressive society. I live in this poor country called India, am much younger than you, and have studied nothing. Yet, you make the irrational argument, while I make the rational argument. As you can clearly see, I focused on his argument; not on him. It is not disagreement that is causing problems; it is the inflammatory tone of the discussion. Civil and constructive disagreement is completely welcome here. But hurtful and offensive discussion is not, even if it is in support of the party line (haha as if there were such a thing). Incorrect. The problem is that Rast is made an irrational argument, and that you are supporting him now. If you were fair you would have criticized Rast's inflammatory comment about classifying Felipe as a fire hazard, without justification. But you didn't. _You_ are making my tone the subject of discussion now, and claim that I have been hurtful and offensive. My email was very much constructive disagreement, in that I have laid out why one should not perform actions without reason; I even assigned him homework, because I _want_ him to understand justice and argue rationally. How could I have been more constructive? I do not support Felipe, or defend him. I do not share his exact opinions, and often criticize him. I am fair in that I praise rational arguments, and criticize irrational arguments. I don't want to speak for him, but I believe that he gives me the same treatment, and I thank him for that. I do not appreciate this ganging-up one bit. I'm one person arguing against an opaque majority opinion veil. For the last time, stop taking sides, and make a goddamn rational argument! And yes, I know that the word offensive is subjective, but for the sake of this discussion let's take it to mean offensive to the vast majority of a community. Not controversial, not contrarian, not even stupid; I don't think anybody is proposing to prohibit dissent or stupidity. But there is no reason for discussion that is gratuitously aggressive, insulting, or derogatory; such discussion is what I mean by offensive. You have made the same argument that I criticized over and over again: majority opinion. If you agree that tone is subjective, why are you trying to objectively criticize it by using majority opinion as the basis? You might not like a piece of artwork personally, and the majority of the git list might agree with you, but that does not mean you can authoritatively claim that the piece of art is junk. You have every right to dislike it personally, but that is an entirely different matter. [...] I already gave you the example of the survivors on the boat with limited food/water on IRC: it is you who stupidly refused to throw anyone overboard, killing all the survivors; I am the one who said that I would get them to draw sticks to fairly choose who to throw overboard, maximizing the chances of survival of the others. I am making a pragmatic argument, based on what is best for the community; not some stuck-up idealistic bullshit. Further, I tried to help you think through the justice problem, by recommending an accessible course. You have either not gone through it, or have gone through it and learnt nothing. Your idea that you can assign Thomas homework in ethics and call him stupid for coming to a different conclusion than you is presumptuous in the extreme. Incorrect. I used stupid to describe his solution to the survivors-in-the-boat problem. I gave him homework (and this is Harvard Justice, by the way), in an attempt to get him to think clearly and come up with less stupid solutions to similar problems. If you are defending throwing modern justice theories out the window, and replacing it with a crude irrational argument, I have nothing more to say. [...] You have embarrassed yourself and the entire git community today. This is also presumptuous, not to mention extremely ironic. In my opinion Thomas's email was calm
Re: [PATCH] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? Huh? It does not make any sense. builtin/*.o files have cmd_foo() that are expected to be called from git.c::main(), but libgit.a files are linked with no constraints whose main() they are linking with. If you are seriously considering the highly unlikely hypothetical standalone git-filter-branch scenario, you should consider the even more likely scenario where somebody needs to access code from ./builtin/*.o; that scenario is not even hypothetical, we know it's happened multiple times, and we know it's going to happen again. That is exactly why I said that builtin/*.o should be refactored to pick does not have to be in builtin bits, which will result in a better division of labor. Reusable bits should live in the library, while a particular implementation of command remain in builtin/* that utilize the reusable bits. You still haven't justified why we have to _forbid_ any outside callers from calling copy_notes_for_rewrite(). -- 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] Documentation/CommunityGuidelines
On 06/11/2013 07:00 PM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: [...] * When reviewing other peoples' code, be tactful and constructive. Set high expectations, but do what you can to help the submitter achieve them. Don't demand changes based only on your personal preferences. Don't let the perfect be the enemy of the good. I think this is 30% aimed at me (as I think I do about that much of the reviews around here). I fully agree with most of them, but the last sentence is a bit too fuzzy to be a practically useful guideline. Somebody's bare minimum is somebody else's perfection. An unqualified perfect is the enemy of good is often incorrectly used to justify It works for me. and There already are other codepaths that do it in the same wrong way., both of which make things _worse_ for the long term project health. I agree that the last line is fuzzy. And I don't think that I've observed any cases where I thought that reviewers were being too strict, so in a way it's just trying to head off hypothetical future problems and to make sure that the balance between submitter and reviewer is not *entirely* one-sided. Given our (proper, I think) strong deference to reviewers, one could imagine a reviewer abusing his/her authority to obstruct reasonable changes by (for example) making demands that the submitter also fix tangentially-related things that are beyond the scope of the patch. In my own projects I have a rough policy of not worse than before, meaning that as long as a patch makes progress in at least one dimension, and doesn't make things worse in any other dimension, then it is acceptable. (Of course worse can include internal quality issues like copy-pasting code or even an increase in the amount of code disproportionate to its benefit.) A failure to make improvements in one area should not be a reason to block an improvement in another area, as long as nothing is made worse. But I can't right now think of a succinct way to express what I have in mind. * It is not OK to use these guidelines as a stick with which to beat supposed violators. However, if you genuinely feel that another community member is routinely behaving in ways that are detrimental to the community, it might help to calmly express your concerns to that person, preferably in a private email, and naming concrete and specific incidents rather than broad generalizations. I would think it is perfectly OK to say The way you are refusing to listen to constructive comments is not how things work around here by pointing at a set of guidelines. I agree. Why do you think is it not OK? The beating part? I think it would be counterproductive for people to start saying things like that is a violation of rule 3, section 2 *in everyday discussions*. This shouldn't be taken as a list of black-and-white laws, with allegations of small infractions used to shut down discussions. And on the other hand, if somebody shows a long history of acting contrary to the guidelines, and persists despite repeated requests to stop, I don't want the discussion to turn into a lawyerly analysis of the guidelines with point-by-point rebuttals and counter-rebuttals of whether this or that guideline was violated. The guidelines should just describe the expected tone of the community in a way that the vast majority of participants can agree on, and any kind of actions to enforce the guidelines should only be taken when an overwhelming majority of the community I think the CommunityGuidelines should have three main uses: 1. An artifact documenting the community consensus about what kinds of behaviors are encouraged and what kinds are considered unacceptable. It should only be accepted, and it only has value, if there is a strong consensus in favor of it. 2. A resource to help new community members get up to speed on our practices and expectations. 3. As a point of reference in the direst meltdowns, such as IMO we are having right now. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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: Tracking vendor release with Git
Am 11.06.2013 19:06, schrieb Yann Droneaud: Hi, I'm trying to setup a workflow to track vendor releases (upstream). Each new release are provided as an archive of source code, data, documentation, etc. For each vendor releases, fixes need to be applied before making them available to users (downstream). Seems to be a rather common use case, applied by most Linux distribution for decades. In my case, on top of each releases, a common set of patches will be applied, the biggest, the most intrusive one, being converting CRLF to LF using dos2unix, the others being small portability fixes. In this case, fixes are not going to be applied by upstream. I'm trying to design (copy ;) a workflow with following properties, in order of importance: 1- I wish to keep a branch with each new vendor release as a commit. This branch's history is only about vendor releases, so it's easy to read the changelog of the vendor releases with command such as git log vendor-release-branch 2- I'd like to ease the process of applying our patches on top of each new vendor release, eg. reduces the likeliness of conflicts. 3- I wish to keep a branch with each new fixed vendor release as a commit. Just like the upstream vendor-release-branch, only one commit per release, so it's easy to read the changelog of the vendor releases with command such as git log patched-release-branch I suggest you aim for the following history (time flows from left to right): U---V-W -- upstream branch \ \ \ C---D-E-- CRLF conversion branch \ \ \ K---L--M--N--O -- downstream branch U, V, W are the upstream releases. C is the initial CRLF-LF conversion. D merges the second upstream release into the CRLF branch, E the third upstream release. These merges very likely create tons of conflicts. But that does not matter, because you know that the only change in our side is CRLF conversion. The commits on this branch can easily be automated. That's the primary motivation for this scheme. K is your first small bugfix and also your first downstream release. After merging L, the second, CRLF-converted, upstream release, you make your second small change, M, which is also your second downstream release. Rinse and repeat with N and O for the third release. -- Hannes -- 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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 10:00:56AM -0700, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: * When reviewing other peoples' code, be tactful and constructive. Set high expectations, but do what you can to help the submitter achieve them. Don't demand changes based only on your personal preferences. Don't let the perfect be the enemy of the good. I think this is 30% aimed at me (as I think I do about that much of the reviews around here). I fully agree with most of them, but the last sentence is a bit too fuzzy to be a practically useful guideline. Somebody's bare minimum is somebody else's perfection. An unqualified perfect is the enemy of good is often incorrectly used to justify It works for me. and There already are other codepaths that do it in the same wrong way., both of which make things _worse_ for the long term project health. One thing that I think is missing from these proposals so far is some clear indication that a review should not be confrontational. Consider the following two review comments (taken from a recent example that happened to stick in my mind, but I don't want to single out any one individual here): Ugh, why this roundabout-passive-past tone? Use imperative tone like this: ... vs. We normally use the imperative in commit messages, perhaps like this? ... Both say the same thing but the first immediately puts the submitter on the defensive. If I see something like that on one of my patches I have to consciously resist the urge to reply immediately and instead review what I'm about to send once I've calmed down. I realise that we shouldn't take offence to review comments, but we are all human and it is sometimes hard not to take things personally. In the examples above, the first makes it feel like the submitter is fighting to get a patch included, but the second feels like we're collaborating to get to the best result for the project. As my mother would say, politeness costs nothing ;-) -- 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] pre-push.sample: Make the script executable
I was reasonably sure that I've seen this patch before, and checked to see everybody else in that directory has executable bit, but it seems that I forgot to apply it. Thanks. Applied. -- 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: Exact format of tree objets
On Tue, Jun 11, 2013 at 01:25:14PM -0300, Chico Sokol wrote: Is there any official documentation of tree objets format? Are tree objects encoded specially in some way? How can I parse the inflated contents of a tree object? Tree object consists of entries, each concatenation of: - Octal mode (using ASCII digits 0-7). - Single SPACE (0x20) - Filename - Single NUL (0x00) - 20-byte binary SHA-1 of referenced object. At least following octal modes are known: 4: Directory (tree). 100644: Regular file (blob). 100755: Executable file (blob). 12: Symbolic link (blob). 16: Submodule (commit). The entries are always sorted in (bytewise) lexicographical order, except directories sort like there was impiled '/' at the end. So e.g.: ! 0 9 a a- a- (directory) a (directory) a0 ab b z. The idea of sorting directories specially is that if one recurses upon hitting a directory and uses '/' as path separator, then the full filenames are in bytewise lexicographical order. -Ilari -- 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: Exact format of tree objets
Chico Sokol chico.so...@gmail.com writes: Is there any official documentation of tree objets format? Are tree objects encoded specially in some way? How can I parse the inflated contents of a tree object? We're suspecting that there is some kind of special format or encoding, because the command git cat-file -p sha show me ... While git cat-file tree sha generate ... cat-file -p is meant to be human-readable form. The latter gives the exact byte contents read_sha1_file() sees, which is a binary format. Essentially, it is a sequence of: - mode of the entry encoded in octal, without any leading '0' pad; - pathname component of the entry, terminated with NUL; - 20-byte SHA-1 object name. sorted in a particular order. -- 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
rebase --autosquash does not handle fixup! of fixup!
git rebase -i --autosquash does not handle a fixup! of a fixup!, such as the history: aaa fix nasty bug ... bbb fixup! fix nasty bug ... ccc fixup! fixup! fix nasty bug --autosquash produces: pick aaa fix nasty bug fixup bbb fixup! fix nasty bug ... pick ccc fixup! fixup! fix nasty bug This defeats the workflow I was hoping to use: git commit -m 'fix nasty bug' ... git commit --fixup :/nasty ... git commit --fixup :/nasty The second :/nasty resolves to the previous fixup, not the initial commit. I could have made the regular expression more precise, but this would be a hassle. Would a change to support fixup! fixup! be considered? Andrew (Please Cc: me on replies.) -- 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] Documentation/CommunityGuidelines
On 06/11/2013 08:16 PM, Ramkumar Ramachandra wrote: This is an exercise. I can easily be more tactful (as evidenced by other threads), but I'm choosing not to be. I want you to focus on the argument, and not the tone. I stopped reading your email here. I've read enough tactless emails over the last few days, but to be asked to read an email that was *intentionally* written tactlessly is too detrimental to my quality of life. In German there is an expression Der Ton macht die Musik: the tone makes the music, by which they mean that the *way* something is said is at least as important as what is said. The tone *is* an integral part of the message and (1) the writer will be much more effective by making the tone agree with the literal words of the message and (2) for this particular reader, the effort of accommodating writers who are unwilling to do so has become too exhausting. I naively thought that I might be able to help calm the situation, but I have concluded that nothing I can say or do will have that effect. Therefore I bow out of this part of the conversation and hope either that it will fizzle out or that perhaps a deus ex machina will appear. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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: Tracking vendor release with Git
From: Yann Droneaud ydrone...@opteya.com Sent: Tuesday, June 11, 2013 6:06 PM Hi, I'm trying to setup a workflow to track vendor releases (upstream). Each new release are provided as an archive of source code, data, documentation, etc. For each vendor releases, fixes need to be applied before making them available to users (downstream). Seems to be a rather common use case, ... Have you looked at the msysgit process that has to cope with upstream git ;-) e.g. https://code.google.com/p/msysgit/source/browse/share/msysGit/merging-rebase.sh?name=python https://github.com/msysgit/msysgit/blob/master/share/msysGit/merging-rebase.sh and https://github.com/msysgit/msysgit/blob/master/share/msysGit/rebasing-merge.sh whereby the guys re-apply all the patches that haven't been accepted upstream, along with local fixups to get each new release working. Philip -- 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] Documentation/CommunityGuidelines
John Keeping wrote: Ugh, why this roundabout-passive-past tone? Use imperative tone like this: ... vs. We normally use the imperative in commit messages, perhaps like this? ... As my mother would say, politeness costs nothing ;-) The review is being honest about her feelings in the first one, and being artificially diplomatic in the second one. Both of them are constructive and friendly, in that they provide an example for the submitter to follow. Either way, I'm not interested in problems that have no solutions. The only solution I see here is to suffocate every contributor until they are tactful enough for the majority's liking, and remove the ones that don't conform. If you do have an alternate solution, please share it with us. -- 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] Documentation/CommunityGuidelines
On 06/11/2013 08:29 PM, John Keeping wrote: On Tue, Jun 11, 2013 at 10:00:56AM -0700, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: * When reviewing other peoples' code, be tactful and constructive. Set high expectations, but do what you can to help the submitter achieve them. Don't demand changes based only on your personal preferences. Don't let the perfect be the enemy of the good. I think this is 30% aimed at me (as I think I do about that much of the reviews around here). I fully agree with most of them, but the last sentence is a bit too fuzzy to be a practically useful guideline. Somebody's bare minimum is somebody else's perfection. An unqualified perfect is the enemy of good is often incorrectly used to justify It works for me. and There already are other codepaths that do it in the same wrong way., both of which make things _worse_ for the long term project health. One thing that I think is missing from these proposals so far is some clear indication that a review should not be confrontational. Consider the following two review comments (taken from a recent example that happened to stick in my mind, but I don't want to single out any one individual here): Ugh, why this roundabout-passive-past tone? Use imperative tone like this: ... vs. We normally use the imperative in commit messages, perhaps like this? ... Both say the same thing but the first immediately puts the submitter on the defensive. If I see something like that on one of my patches I have to consciously resist the urge to reply immediately and instead review what I'm about to send once I've calmed down. That's a very good point (and a good illustration, too). How do you like the new second and third sentences below? * When reviewing other peoples' code, be tactful and constructive. Remember that submitting patches for public critique can be very intimidating and when mistakes are found it can be embarrassing. Do what you can to make it a positive and pleasant experience for the submitter. Set high expectations, but do what you can to help the submitter achieve them. Don't demand changes based only on your personal preferences. Don't let the perfect be the enemy of the good. (As Junio pointed out, the last sentence is not so great and a better replacement would be welcome.) As my mother would say, politeness costs nothing ;-) Does your mother program C? We could use her around here :-) Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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] Documentation/CommunityGuidelines
Michael Haggerty wrote: I stopped reading your email here. I've read enough tactless emails over the last few days, but to be asked to read an email that was *intentionally* written tactlessly is too detrimental to my quality of life. I'm sorry, but the problem has no solution then. The problem we are dealing with is irrational and/or out-of-tone emails. Unless you possess some mind-control mechanism that will get all contributors to write emails that conform to your standards, there is no solution. If you feel strongly that everyone must conform to your standards, you must remove the members that do not conform to that standard. I have no desire to be suffocated to conform to your standard, so I'm ready to leave. -- 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: rebase --autosquash does not handle fixup! of fixup!
Andrew Pimlott and...@pimlott.net writes:
git rebase -i --autosquash does not handle a fixup! of a fixup!, such as
the history:
aaa fix nasty bug
...
bbb fixup! fix nasty bug
...
ccc fixup! fixup! fix nasty bug
--autosquash produces:
pick aaa fix nasty bug
fixup bbb fixup! fix nasty bug
...
pick ccc fixup! fixup! fix nasty bug
This defeats the workflow I was hoping to use:
git commit -m 'fix nasty bug'
...
git commit --fixup :/nasty
...
git commit --fixup :/nasty
The second :/nasty resolves to the previous fixup, not the initial
commit. I could have made the regular expression more precise, but this
would be a hassle.
Would a change to support fixup! fixup! be considered?
Sure, why not. You could start with something like the patch below
(untested). If that happens to work, just add a test and a good commit
message.
diff --git i/git-rebase--interactive.sh w/git-rebase--interactive.sh
index f953d8d..798ae81 100644
--- i/git-rebase--interactive.sh
+++ w/git-rebase--interactive.sh
@@ -689,7 +689,17 @@ rearrange_squash () {
case $message in
squash! *|fixup! *)
action=${message%%!*}
- rest=${message#*! }
+ rest=$message
+ while : ; do
+ case $rest in
+ squash! *|fixup! *)
+ rest=${rest#*! }
+ ;;
+ *)
+ break
+ ;;
+ esac
+ done
echo $sha1 $action $rest
# if it's a single word, try to resolve to a full sha1
and
# emit a second copy. This allows us to match on both
message
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? Huh? It does not make any sense. builtin/*.o files have cmd_foo() that are expected to be called from git.c::main(), but libgit.a files are linked with no constraints whose main() they are linking with. If you are seriously considering the highly unlikely hypothetical standalone git-filter-branch scenario, you should consider the even more likely scenario where somebody needs to access code from ./builtin/*.o; that scenario is not even hypothetical, we know it's happened multiple times, and we know it's going to happen again. That is exactly why I said that builtin/*.o should be refactored to pick does not have to be in builtin bits, which will result in a better division of labor. Reusable bits should live in the library, while a particular implementation of command remain in builtin/* that utilize the reusable bits. You still haven't justified why we have to _forbid_ any outside callers from calling copy_notes_for_rewrite(). Because only builtins _should_ use it. I asked you for an example, and you said a hypothetical standalone 'git-filter-branch' might use it, but you have not explained why it should be standalone, when it's clear it should be a builtin. If we assume your argument is valid for the hypothetical 'git-filter-branch', if that's the case, then it would be even more reasonable to assume that there will be other standalone binaries that would want to use all sort of functions from ./builtin/*.o. Let's put an example: reset_index(). Some standalone program wants to use that function. What do you we do? The shortest route is to make it non-static, and add it to builtin.h. But that would not be enough, we need the infrastructure prepared for that; link libgit.a with ./builtin/*.o. I don't think that's the way to go, but your line of argumentation leads directly there; if we are worrying about anything that any potential standalone program could want, then we should worry about reset_index() not being easily accessible to them. IMO we should be clear and say no; standalone programs should not access copy_notes_for_rewrite(), that's for builtins. If we move all the code that potential standalone programs could want to libgit.a, it wouldn't be a library at all, and it would basically contain everything. -- 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] Documentation/CommunityGuidelines
Ramkumar Ramachandra artag...@gmail.com writes: Michael Haggerty wrote: I stopped reading your email here. I've read enough tactless emails over the last few days, but to be asked to read an email that was *intentionally* written tactlessly is too detrimental to my quality of life. I'm sorry, but the problem has no solution then. The problem we are dealing with is irrational and/or out-of-tone emails. Unless you possess some mind-control mechanism that will get all contributors to write emails that conform to your standards, there is no solution. Actually there is. Just ignore the troll. In the past few days, I've learned to mostly skim mails from you and Felipe on this topic (and perhaps some other topics) just enough to see if there is anything worth reading and/or responding to in them, and have ignored most of them. That gave me some time back to do the real work. If you argue that we should not punish people but bad behaviour, that is fine. The From: field, combined with the Subject: field, is often a pretty good indication to tell if a message is worth reading and/or responding to, so ignoring such messages and ignoring troll senders practically amount to the same thing. -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 1:14 PM, Linus Torvalds torva...@linux-foundation.org wrote: On Tue, Jun 11, 2013 at 11:06 AM, Felipe Contreras felipe.contre...@gmail.com wrote: Moreover, if you are going to argue that we shouldn't be closing the door [...] Felipe, you saying if you are going to argue ... to anybody else is kind of ironic. Supposing the other side's argument is correct is a standard discussing technique. Why is it every thread I see you in, you're being a dick and arguing for some theoretical thing that nobody else cares about? I don't know. I've sent 800 patches in the last three months (patches, not email comments), and you pick this one to reply to. Maybe because you enjoy insulting people? This whole thread has been one long argument about totally pointless things that wouldn't improve anything one way or the other. It's bikeshedding of the worst kind. Just let it go. Why don't you ask Junio to let it go? If it's irrelevant, than it doesn't matter if this patch is applied or not. You say it's bike-shedding, that implies that Junio likes red, and I like blue, and both are equally useless. So let's go for blue then. Presumably Junio doesn't agree with you, he does truly think it should be red, in fact, he doesn't think it's just a color, it's something important, and I agree, and apparently other people in the mailing list also agree, as most of them have voiced their opinion that red is the color. Now, do you have something of value to say which of the two options should be, or are you just going to engage in double standards and personal attacks? If you truly think this is bikeshedding, at least be fair and complain about that to the people that argue for red, not just the ones that argue for blue. -- 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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 08:52:05PM +0200, Michael Haggerty wrote: That's a very good point (and a good illustration, too). How do you like the new second and third sentences below? * When reviewing other peoples' code, be tactful and constructive. Remember that submitting patches for public critique can be very intimidating and when mistakes are found it can be embarrassing. Do what you can to make it a positive and pleasant experience for the submitter. Set high expectations, but do what you can to help the submitter achieve them. Don't demand changes based only on your personal preferences. Don't let the perfect be the enemy of the good. I'm not sure. I like the intent, but I'm not sure that it's clear enough that we're talking about the tone of comments rather than the type of feedback to provide. How about something like this? * Having your code reviewed should feel like a collaboration aiming for the best result for the project, not like a fight to get your patch accepted. Try to bear this in mind when reviewing other peoples' code and consider how you would feel reading the same comments if the review was the other way round. We are only human and the tone of a review can influence how the following discussion progresses. * If you do feel that a review is aggressive, don't reply immediately. Contributors are spread around the world in different timezones and it is often better to wait a few hours for others to comment before rushing to defend your patch. -- 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] build: get rid of the notion of a git library
Felipe Contreras felipe.contre...@gmail.com writes: On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? Huh? It does not make any sense. builtin/*.o files have cmd_foo() that are expected to be called from git.c::main(), but libgit.a files are linked with no constraints whose main() they are linking with. ... That is exactly why I said that builtin/*.o should be refactored to pick does not have to be in builtin bits, which will result in a better division of labor. Reusable bits should live in the library, while a particular implementation of command remain in builtin/* that utilize the reusable bits. You still haven't justified why we have to _forbid_ any outside callers from calling copy_notes_for_rewrite(). Because only builtins _should_ use it. And there is no justification behind that _should_ claim; you are not making any technical argument to explain it. I asked you for an example, and you said a hypothetical standalone 'git-filter-branch' might use it, Of course it has to be hypothetical; I already said with the current code no standalone does use it---it is not arranged to be doable so there is no user. If you want to have examples of future possible callers, they have to be hypothetical---the future by definition hasn't happened. But that does not mean hypothetical is impractical nor useless. There are out-of-tree programs like cgit that will not be built-in but already link with libgit.a. Moving things that can be used by outside people out of builtin/*.o to libgit.a would allow uses that you and I cannot imagine offhand. I do not see a reason for us to forbid a filter-branch replacement out of tree as a standalone. I do not see a point in continuing to discuss this (or any design level issues) with you. You seem to go into a wrong direction to break the design of the overall system, not in a direction to improve anything. I do not know, and at this point I do not care, if you are doing so deliberately to sabotage Git. Just stop. -- 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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 1:29 PM, John Keeping j...@keeping.me.uk wrote: I realise that we shouldn't take offence to review comments, but we are all human and it is sometimes hard not to take things personally. In the examples above, the first makes it feel like the submitter is fighting to get a patch included, but the second feels like we're collaborating to get to the best result for the project. As my mother would say, politeness costs nothing ;-) That's right, I agree with everything you said, but what would your mother say about the people are not polite towards you? I doubt it would be fuck them then. You should be polite, you should not demand politeness. Being polite towards the people that are polite to you barely has any merit, swallowing your pride, taking a deep breath, trying to understand that the other side might be just having a bad day, or any number of reasons for the impoliteness... that's what takes effort. Escalating violence is easy, blaming the other side for starting is also easy (as any toddler would tell you), being the side that puts the other cheek is what's hard. -- 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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 2:06 PM, Junio C Hamano gits...@pobox.com wrote: Ramkumar Ramachandra artag...@gmail.com writes: I'm sorry, but the problem has no solution then. The problem we are dealing with is irrational and/or out-of-tone emails. Unless you possess some mind-control mechanism that will get all contributors to write emails that conform to your standards, there is no solution. Actually there is. Just ignore the troll. Congratulations Junio. You have followed our drafted guidelines by choosing to lead by example how not to not propagate the violence, turn around, and take the high road. After kicking your opponent in the groin. -- 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] Documentation/CommunityGuidelines
From: Michael Haggerty mhag...@alum.mit.edu Sent: Tuesday, June 11, 2013 7:52 PM [...] That's a very good point (and a good illustration, too). How do you like the new second and third sentences below? * When reviewing other peoples' code, be tactful and constructive. Remember that submitting patches for public critique can be very intimidating I found this to be true. The tone on the list could at times feel un-helpful (to the new person). It is almost as if it is an initiation - those on the list know the protocols, and new folk either arrive like a bull in a china shop, or more likely, timidly push the patch under the door and run away (and variations in between) - some never push out their (drafted) patch. and when mistakes are found it can be embarrassing. Sometimes it isn't 'mistakes', rather it is simply a lack of sufficient explanation to communicate intent, which may not have been understood by the reviewer/responder. In such cases it can be a frustration to know what was meant in the response, especially if the response is terse. [i.e. I think it would be reasonable to squeeze part of this in here somewhere to guide new contributors about this step] There is separately a need to note the role of the maintainer, who has a more difficult role as gatekeeper who's higher standards in applying the precautionary principle http://en.wikipedia.org/wiki/Precautionary_principle can feel like unhelpfulness, or worse if misunderstood. Do what you can to make it a positive and pleasant experience for the submitter. Set high expectations, but do what you can to help the submitter achieve them. Don't demand changes based only on your personal preferences. Don't let the perfect be the enemy of the good. (As Junio pointed out, the last sentence is not so great and a better replacement would be welcome.) As my mother would say, politeness costs nothing ;-) Does your mother program C? We could use her around here :-) I think she programmed in Smalltalk and CleanYourRoom. (sorry not my question ;-) Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ regards Philip -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 2:24 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Tue, Jun 11, 2013 at 1:17 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: Moreover, if you are going to argue that we shouldn't be closing the door, then why not link ./builtin/*.o to libgit.a? Huh? It does not make any sense. builtin/*.o files have cmd_foo() that are expected to be called from git.c::main(), but libgit.a files are linked with no constraints whose main() they are linking with. ... That is exactly why I said that builtin/*.o should be refactored to pick does not have to be in builtin bits, which will result in a better division of labor. Reusable bits should live in the library, while a particular implementation of command remain in builtin/* that utilize the reusable bits. You still haven't justified why we have to _forbid_ any outside callers from calling copy_notes_for_rewrite(). Because only builtins _should_ use it. And there is no justification behind that _should_ claim; you are not making any technical argument to explain it. The first argument of init_copy_notes_for_rewrite() is the name of the builtin command. There hardly could be any more justification. I asked you for an example, and you said a hypothetical standalone 'git-filter-branch' might use it, Of course it has to be hypothetical; I already said with the current code no standalone does use it---it is not arranged to be doable so there is no user. If you want to have examples of future possible callers, they have to be hypothetical---the future by definition hasn't happened. But that does not mean hypothetical is impractical nor useless. So? It's still hypothetical, which is what I said. What are you complaining about? About the fact that I made a correct assessment? There are out-of-tree programs like cgit that will not be built-in but already link with libgit.a. Moving things that can be used by outside people out of builtin/*.o to libgit.a would allow uses that you and I cannot imagine offhand. I do not see a reason for us to forbid a filter-branch replacement out of tree as a standalone. Yeah, I already ran that argument, and you conveniently chose to escape the next logical conclusion that I already put forward: --- a/Makefile +++ b/Makefile @@ -990,6 +990,8 @@ BUILTIN_OBJS += builtin/verify-pack.o BUILTIN_OBJS += builtin/verify-tag.o BUILTIN_OBJS += builtin/write-tree.o +LIB_OBJS += $(BUILTIN_OBJS) + GITLIBS = $(LIB_FILE) $(XDIFF_LIB) EXTLIBS = I don't think that's the right direction. I do not see a point in continuing to discuss this (or any design level issues) with you. You seem to go into a wrong direction to break the design of the overall system, not in a direction to improve anything. I do not know, and at this point I do not care, if you are doing so deliberately to sabotage Git. Just stop. That's your opinion, and it's not shared by others (outside of the Git project). If you were right and Git was moving in the right direction, there would be no need for libgit2. -- 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] Documentation/CommunityGuidelines
On Wed, Jun 12, 2013 at 12:16:28AM +0530, Ramkumar Ramachandra wrote: John Keeping wrote: Ugh, why this roundabout-passive-past tone? Use imperative tone like this: ... vs. We normally use the imperative in commit messages, perhaps like this? ... As my mother would say, politeness costs nothing ;-) The review is being honest about her feelings in the first one, and being artificially diplomatic in the second one. I don't think it is artificially diplomatic, it's an attempt to convey a helpful tone in an email. As has been said elsewhere, it is easy to read an email in the wrong tone (there is an oft-cited statistic about the percentage of communication that is non-verbal, and which cannot be inferred from written text). For this reason I think it is important for reviewers to make an effort to minimise the risk that what they write can be interpreted as being aggressive. Both of them are constructive and friendly, in that they provide an example for the submitter to follow. Both provide the same advice, yes. But I disagree that they are both friendly. The top example reads (to me at least) as an attack on the submitter for not knowing better. It may sometimes be necessary to resort to strong wording if someone appears to be wilfully ignoring sensible advice but we should not expect every submitter to know the expectations of the project; the first message to someone should gently guide them in the right direction. Either way, I'm not interested in problems that have no solutions. The only solution I see here is to suffocate every contributor until they are tactful enough for the majority's liking, and remove the ones that don't conform. If you do have an alternate solution, please share it with us. I don't have a solution, only a hope that regular contributors will learn from others how they can phrase review comments less aggressively. I expect different people will read the same statement differently; people are from different cultures and what is considered acceptable in one culture can be considered rude in another. We should aim to cultivate our own culture where we try to minimise the risk that what we write will be misinterpreted by someone with a different cultural background. -- 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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 7:40 AM, Michael Haggerty mhag...@alum.mit.edu wrote: At the risk of being presumptuous myself, I suggest that you show a copy of your email to somebody whom you know and respect in the real world, somebody who is not immersed in the Git community meltdown. For example, somebody like your mother or father, or a teacher whom you respect, or a member of clergy if you are so inclined. Ask that person's opinion about your email. It is so easy to lose perspective in the Internet. Such excellent advice. Even if the advice is not taken literally, it is probably enough to just imagine how that person whom you respect would respond to the words in your emails. I am sure I do not do this enough in my own communications. I just wanted to draw attention to this wonderful suggestion again. Sometimes it is necessary to take a step back when discussions get heated, to regain perspective. -Brandon -- 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] build: get rid of the notion of a git library
Linus Torvalds torva...@linux-foundation.org writes: This whole thread has been one long argument about totally pointless things that wouldn't improve anything one way or the other. It's bikeshedding of the worst kind. Just let it go. The proposal to move sequencer.c to builtins/sequencer.c and then adding a filter in Makefile to exclude so that git-sequencer is not built is it wouldn't improve anything one way or the other. It is to throw in something into a set to which it does not belong, and then working around that mistake with another kludge. The problem that triggered the wrong solution actually is real, however. A function that sequencer.c (in libgit.a so that it could be used by standalone) may want to use in the future currently lives in builtin/notes.c. If you add a call to that function to sequencer.c without doing anything else, standalones like git-upload-pack will stop linking correctly. The git-upload-pack wants the revision traversal machinery in revision.o, which in turn wants to be able to see log-tree.o, which in turn wants to link with sequencer.o to see one global variable (there may be other dependencies). All of these objects are currently in libgit.a so that both builtins and standalones can use them. Moving sequencer.c to builtin/ is not even a solution. Linking git-upload-pack will still pull in builtin/notes.o along with cmd_notes(), which is not called from main(); as you remember, cmd_foo() in all builtin/*.o are designed to be called from git.c::main(). 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. So I do not think this is not even a bikeshedding. Just one side being right, and the other side continuing to repeat nonsense without listening. -- 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] build: get rid of the notion of a git library
On Tue, Jun 11, 2013 at 2:59 PM, Junio C Hamano gits...@pobox.com wrote: Linus Torvalds torva...@linux-foundation.org writes: This whole thread has been one long argument about totally pointless things that wouldn't improve anything one way or the other. It's bikeshedding of the worst kind. Just let it go. The proposal to move sequencer.c to builtins/sequencer.c and then adding a filter in Makefile to exclude so that git-sequencer is not built is it wouldn't improve anything one way or the other. It is to throw in something into a set to which it does not belong, In your opinion. and then working around that mistake with another kludge. In your opinion. You continually use absolutist rhetoric to try to convince yourself and others that what you say is absolute 100% fact. But it's not, it's your opinion. So I do not think this is not even a bikeshedding. Just one side being right, and the other side continuing to repeat nonsense without listening. George W. Bush said history would prove him right, but saying so doesn't make it so. At least he had the decency to acknowledge that other people had different valid opinions. builtin/lib.a makes perfect sense, and it's the first logical step in moving libgit.a towards libgit2. -- 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 v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions
Le 11/06/2013 20:09, Junio C Hamano a écrit : Matthieu Moy matthieu@grenoble-inp.fr writes: my ($namespace) = @_; my $namespace = shift; My impression has been that both are equally common, The second is the most common in git-remote-mediawiki (but I don't have any preference nor know what is recommended elsewhere). I wasn't implying I prefer the former. I was just being curious, and if the latter is more prevalent in the code _and_ Perlcritique does not have any issue with it, it is perfectly fine. Perlcritic doesn't have an issue with any of both cases. I think the second method is clearer when there is only one argument, because it makes it clear that there is only one. -- Célestin Matte -- 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] instaweb: make the perl path configurable
It is convenient for the user to be able to customize the path to perl if they
do not want to use the system perl. This may be the case, for example, if the
user wants to use the plackup httpd but its extra dependencies are not
installed in the system perl; they can set the perl path to a perl that they
install and have control over in their own home directory.
Signed-off-by: Charles McGarvey chazmcgar...@brokenzipper.com
---
Documentation/config.txt | 4
git-instaweb.sh | 4 +++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..e103594 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1549,6 +1549,10 @@ instaweb.modulepath::
instead of /usr/lib/apache2/modules. Only used if httpd
is Apache.
+instaweb.perlpath::
+ The path to the perl executable used by linkgit:git-instaweb[1] to
+ run gitweb and/or verify that the HTTP daemon is running.
+
instaweb.port::
The port number to bind the gitweb httpd to. See
linkgit:git-instaweb[1].
diff --git a/git-instaweb.sh b/git-instaweb.sh
index 01a1b05..8cfbdf2 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -3,7 +3,6 @@
# Copyright (c) 2006 Eric Wong
#
-PERL='@@PERL@@'
OPTIONS_KEEPDASHDASH=
OPTIONS_SPEC=\
git instaweb [options] (--start | --stop | --restart)
@@ -26,9 +25,12 @@ local=$(git config --bool --get instaweb.local)
httpd=$(git config --get instaweb.httpd)
root=$(git config --get instaweb.gitwebdir)
port=$(git config --get instaweb.port)
+perl_path=$(git config --get instaweb.perlpath)
module_path=$(git config --get instaweb.modulepath)
action=browse
+PERL=${perl_path:-@@PERL@@}
+
conf=$GIT_DIR/gitweb/httpd.conf
# Defaults:
--
1.8.1.5
--
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] Documentation/CommunityGuidelines
On Tue, Jun 11, 2013 at 10:00:56AM -0700, Junio C Hamano wrote: * Accept reviewers' comments gratefully and take them very seriously. Show that you appreciate the help by giving the reviewer the benefit of the doubt. If, after careful consideration, you find that you cannot agree with a reviewer's suggestion, explain your reasoning carefully without taking or giving offense, and seek compromise. In short, the only acceptable response to review comments are You are right. Here is a reroll, or I think your suggestion will miss these cases which I wanted to cover and that is why I did it this way. There may be other small variants of the above two, but I think I can agree with the general principle. In cases, there are two or more equally valid approaches to solving a problem. I do not think I had to accept (or reject) many it can be done better in different ways and this perhaps is not the best one (or it could be argued that it is correct) borderline topics in the recent past, but I suspect that a disagreement is healthy has to be accompanied by how disagreements that do not resolve themselves are resolved (I think I've heard somewhere that some communities break ties in favor of reviewers, for example). I more or less agree with what both of you have said above. The ties goes to reviewers thing I would be very wary of, at least as a hard rule. We do not (and do not want to) put any restrictions on who is allowed to do review. That sometimes results in unhelpful or even wrong reviews by new people, but those reviews are a stepping stone to being a more experienced and capable reviewer. Most of the time such reviews are resolved by other community members joining the discussion and coming to some agreement, but not always. And that is not even getting into the cases where long-time experienced reviewers are simply wrong or misguided, or the issue is legitimately a difficult tradeoff to consider, and the discussion ends in a stalemate. And I think that is where the benevolent dictator role comes in. They weigh not just the points made in the discussion (or a summary of it), but also use their judgement on who is making comments (how many people, the utility of their past comments) and other factors (other things happening in the project, being conservative because of recent mistakes made, etc). They may break such a tie by applying or rejecting, even by putting off a decision to revisit later (which is a de facto reject, of course). So there are no hard rules, and this is not a democracy[1]. For the most part the community runs itself in an open and collective fashion, and the dictator's job is easy; but ultimately, he or she is in charge of what gets applied and what doesn't. Rules like break ties in favor of reviewers are just a guideline for the dictator to use in making decisions. I do not think any of that is news to you, but I think the point needs to be made, as it applies to any concrete rules. -Peff [1] Note that I think a benevolent dictator is a _terrible_ way to run a real government, but it works in an open source project. I think the difference is that dictatorship is open to abuse of power. In the real world, there is a lot of power to abuse, and it is hard for people to opt out of it. In the open source world, there is not that much power, and if there is a bad dictator everyone can go somewhere else (another project, or even a fork). So while a dictator _can_ play favorites, or start deciding which patches to take based on what they had for breakfast, there is a real incentive to remain fair and reasonable. -- 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] Documentation/CommunityGuidelines
Jeff King p...@peff.net writes: So there are no hard rules, and this is not a democracy[1]. For the most part the community runs itself in an open and collective fashion, and the dictator's job is easy; but ultimately, he or she is in charge of what gets applied and what doesn't. Rules like break ties in favor of reviewers are just a guideline for the dictator to use in making decisions. I do not think any of that is news to you, but I think the point needs to be made, as it applies to any concrete rules. My original draft had I am hoping we do not have to come to that after (I heard some communities break ties this way), but I removed it by mistake. And I think you are right. I also am hoping that I am being fair to dictate ;-) -Peff [1] Note that I think a benevolent dictator is a _terrible_ way to run a real government, but it works in an open source project. I think the difference is that dictatorship is open to abuse of power. In the real world, there is a lot of power to abuse, and it is hard for people to opt out of it. In the open source world, there is not that much power, and if there is a bad dictator everyone can go somewhere else (another project, or even a fork). So while a dictator _can_ play favorites, or start deciding which patches to take based on what they had for breakfast, there is a real incentive to remain fair and reasonable. -- 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/4] Fix a race condition when reading loose refs
I've read the series while on a bus and found all of them sensible. I do share the worry of retry storm you expressed in the last one, and I agree that giving up after N times is a reasonable way out, when it becomes necessary. Thanks. -- 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/RFC] git-remote-mediawiki: new tool to preview local changes without pushing
The V2 is on the launchpad but I am still struggling with the code
factoring between git-mw.perl and git-remote-mediawiki.perl :/ .
On 9 June 2013 08:08, Jeff King p...@peff.net wrote:
You could make a Git::MediaWiki.pm module, but installing that would
significantly complicate the build procedure, and potentially be
annoying for users. One trick I have done in the past is to concatenate
bits of perl script together in the Makefile, like this:
foo: common.pl foo.pl
{ \
echo '$(PERL_PATH_SQ)' \
for i in $^; do \
echo #line 1 $src \
cat $src \
done \
} $@+
mv $@+ $@
That would conflict a bit with the way we chain to git's Makefile,
though. I suspect you could do something complicated like build foo.pl
from common.pl and foo-main.pl, then chain to git's Makefile to
build foo from foo.pl.
I've implemented this one for now but after a real-life meeting with
Matthieu Moy we discussed the possibility to build a GitMediawiki.pm
module. It seems more clean than the concatenation of perl scripts.
Plus, it would force people to limit side effects inside the functions
used in this package/utils file (I have in mind the mw_connect_maybe
function here and a couple of others which directly *hope* for global
vars to be set to a nice value before being called).
What I find bad in the concatenating-thingy is the mandatory rename of
git-mw.perl into something like git-mw.unmerged.perl and
git-remote-mediawiki.perl into git-remote-mediawiki.unmerged.perl.
Otherwise, like you said, it would be hard to chain to git's Makefile
after the merge.
For now, I have really no idea which one is the best. If I may ask,
what did you have in mind while saying:
You could make a Git::MediaWiki.pm module, but installing that would
significantly complicate the build procedure, and potentially be
annoying for users.
?
Since my previous commit (ea07ec1 in next - use Git.pm functions for
credentials), git-remote-mediawiki.perl already depends on the proper
installation of the Git.pm package. In what ways the need for the
installation of yet another package (GitMediawiki.pm) would annoy a
user ?
Thank you for your time,
Benoit
--
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/RFC] git-remote-mediawiki: new tool to preview local changes without pushing
On Tue, Jun 11, 2013 at 11:31:31PM +0200, Benoît Person wrote: I've implemented this one for now but after a real-life meeting with Matthieu Moy we discussed the possibility to build a GitMediawiki.pm module. It seems more clean than the concatenation of perl scripts. Plus, it would force people to limit side effects inside the functions used in this package/utils file (I have in mind the mw_connect_maybe function here and a couple of others which directly *hope* for global vars to be set to a nice value before being called). What I find bad in the concatenating-thingy is the mandatory rename of git-mw.perl into something like git-mw.unmerged.perl and git-remote-mediawiki.perl into git-remote-mediawiki.unmerged.perl. Otherwise, like you said, it would be hard to chain to git's Makefile after the merge. Yeah, I agree it's very hacky. For now, I have really no idea which one is the best. If I may ask, what did you have in mind while saying: You could make a Git::MediaWiki.pm module, but installing that would significantly complicate the build procedure, and potentially be annoying for users. Since my previous commit (ea07ec1 in next - use Git.pm functions for credentials), git-remote-mediawiki.perl already depends on the proper installation of the Git.pm package. In what ways the need for the installation of yet another package (GitMediawiki.pm) would annoy a user ? I was thinking that you would be self-contained inside the contrib/mw-to-git directory, and therefore you would have to teach your code how to install the Git module, and you could not longer just cp git-remote-mediawiki into the right place to install it. But I think we have already crossed that bridge somewhat with Git.pm. And if you add your module as perl/Git/MediaWiki.pm and use the existing perl build system, then it is not any extra effort from the build system. That is probably the most sane way to go. -Peff -- 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 v4 1/2] status: introduce status.short to enable --short by default
jorge-juan.garcia-gar...@ensimag.imag.fr writes: diff --git a/t/t7508-status.sh b/t/t7508-status.sh index e2ffdac..3c0818b 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -1335,4 +1335,39 @@ test_expect_failure '.git/config ignore=all suppresses submodule summary' ' git config -f .gitmodules --remove-section submodule.subname ' +test_expect_success 'Setup of test environment' ' + git config status.showUntrackedFiles no +' + +test_expect_success 'status.short=true same as -s' ' + git -c status.short=true status actual + git status -s expected_short + test_cmp expected_short actual +' + +test_expect_success 'status.short=true different from --no-short' ' + git status --no-short expected_noshort + test_must_fail test_cmp expected_noshort actual +' I am not sure if must be different, and I do not care what kind of difference we get is a good test. +test_expect_success 'status.short=true weaker than --no-short' ' + git -c status.short=true status --no-short actual + test_cmp expected_noshort actual +' + +test_expect_success 'status.short=false same as --no-short' ' + git -c status.short=false status actual + git status -s expected_short + test_cmp expected_noshort actual +' I think the second line in this test is unwanted. + +test_expect_success 'status.short=false weaker than -s' ' + git -c status.short=false status -s actual + test_cmp expected_short actual +' + +test_expect_success 'Restore default test environment' ' + git config --unset status.showUntrackedFiles +' + test_done I'll queue this patch after tweaking the test part like this. Thanks. diff --git a/t/t7508-status.sh b/t/t7508-status.sh index e2ffdac..33cadd0 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -1335,4 +1335,34 @@ test_expect_failure '.git/config ignore=all suppresses submodule summary' ' git config -f .gitmodules --remove-section submodule.subname ' +test_expect_success 'setup of test environment' ' + git config status.showUntrackedFiles no + git status -s expected_short + git status --no-short expected_noshort +' + +test_expect_success 'status.short=true same as -s' ' + git -c status.short=true status actual + test_cmp expected_short actual +' + +test_expect_success 'status.short=true weaker than --no-short' ' + git -c status.short=true status --no-short actual + test_cmp expected_noshort actual +' + +test_expect_success 'status.short=false same as --no-short' ' + git -c status.short=false status actual + test_cmp expected_noshort actual +' + +test_expect_success 'status.short=false weaker than -s' ' + git -c status.short=false status -s actual + test_cmp expected_short actual +' + +test_expect_success 'Restore default test environment' ' + git config --unset status.showUntrackedFiles +' + test_done -- 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 08/12] Extract a struct stat_data from cache_entry
Add public functions fill_stat_data() and match_stat_data() to work
with it. This infrastructure will later be used to check the validity
of other types of file.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
I'm not too familiar with this part of the code, so please make sure
that I've put the dividing line at the right place.
builtin/ls-files.c | 12 +++--
cache.h| 33 +---
read-cache.c | 151 +
3 files changed, 116 insertions(+), 80 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 2202072..6a0730f 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -165,11 +165,13 @@ static void show_ce_entry(const char *tag, struct
cache_entry *ce)
}
write_name(ce-name, ce_namelen(ce));
if (debug_mode) {
- printf( ctime: %d:%d\n, ce-ce_ctime.sec, ce-ce_ctime.nsec);
- printf( mtime: %d:%d\n, ce-ce_mtime.sec, ce-ce_mtime.nsec);
- printf( dev: %d\tino: %d\n, ce-ce_dev, ce-ce_ino);
- printf( uid: %d\tgid: %d\n, ce-ce_uid, ce-ce_gid);
- printf( size: %d\tflags: %x\n, ce-ce_size, ce-ce_flags);
+ struct stat_data *sd = ce-ce_stat_data;
+
+ printf( ctime: %d:%d\n, sd-sd_ctime.sec, sd-sd_ctime.nsec);
+ printf( mtime: %d:%d\n, sd-sd_mtime.sec, sd-sd_mtime.nsec);
+ printf( dev: %d\tino: %d\n, sd-sd_dev, sd-sd_ino);
+ printf( uid: %d\tgid: %d\n, sd-sd_uid, sd-sd_gid);
+ printf( size: %d\tflags: %x\n, sd-sd_size, ce-ce_flags);
}
}
diff --git a/cache.h b/cache.h
index df532f8..207f849 100644
--- a/cache.h
+++ b/cache.h
@@ -119,15 +119,19 @@ struct cache_time {
unsigned int nsec;
};
+struct stat_data {
+ struct cache_time sd_ctime;
+ struct cache_time sd_mtime;
+ unsigned int sd_dev;
+ unsigned int sd_ino;
+ unsigned int sd_uid;
+ unsigned int sd_gid;
+ unsigned int sd_size;
+};
+
struct cache_entry {
- struct cache_time ce_ctime;
- struct cache_time ce_mtime;
- unsigned int ce_dev;
- unsigned int ce_ino;
+ struct stat_data ce_stat_data;
unsigned int ce_mode;
- unsigned int ce_uid;
- unsigned int ce_gid;
- unsigned int ce_size;
unsigned int ce_flags;
unsigned int ce_namelen;
unsigned char sha1[20];
@@ -509,6 +513,21 @@ extern int limit_pathspec_to_literal(void);
#define HASH_FORMAT_CHECK 2
extern int index_fd(unsigned char *sha1, int fd, struct stat *st, enum
object_type type, const char *path, unsigned flags);
extern int index_path(unsigned char *sha1, const char *path, struct stat *st,
unsigned flags);
+
+/*
+ * Record to sd the data from st that we use to check whether a file
+ * might have changed.
+ */
+extern void fill_stat_data(struct stat_data *sd, struct stat *st);
+
+/*
+ * Return 0 if st is consistent with a file not having been changed
+ * since sd was filled. If there are differences, return a
+ * combination of MTIME_CHANGED, CTIME_CHANGED, OWNER_CHANGED,
+ * INODE_CHANGED, and DATA_CHANGED.
+ */
+extern int match_stat_data(struct stat_data *sd, struct stat *st);
+
extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
#define REFRESH_REALLY 0x0001 /* ignore_valid */
diff --git a/read-cache.c b/read-cache.c
index 04ed561..4c4328e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -67,6 +67,61 @@ void rename_index_entry_at(struct index_state *istate, int
nr, const char *new_n
add_index_entry(istate, new,
ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
}
+void fill_stat_data(struct stat_data *sd, struct stat *st)
+{
+ sd-sd_ctime.sec = (unsigned int)st-st_ctime;
+ sd-sd_mtime.sec = (unsigned int)st-st_mtime;
+ sd-sd_ctime.nsec = ST_CTIME_NSEC(*st);
+ sd-sd_mtime.nsec = ST_MTIME_NSEC(*st);
+ sd-sd_dev = st-st_dev;
+ sd-sd_ino = st-st_ino;
+ sd-sd_uid = st-st_uid;
+ sd-sd_gid = st-st_gid;
+ sd-sd_size = st-st_size;
+}
+
+int match_stat_data(struct stat_data *sd, struct stat *st)
+{
+ int changed = 0;
+
+ if (sd-sd_mtime.sec != (unsigned int)st-st_mtime)
+ changed |= MTIME_CHANGED;
+ if (trust_ctime check_stat
+ sd-sd_ctime.sec != (unsigned int)st-st_ctime)
+ changed |= CTIME_CHANGED;
+
+#ifdef USE_NSEC
+ if (check_stat sd-sd_mtime.nsec != ST_MTIME_NSEC(*st))
+ changed |= MTIME_CHANGED;
+ if (trust_ctime check_stat
+ sd-sd_ctime.nsec != ST_CTIME_NSEC(*st))
+ changed |= CTIME_CHANGED;
+#endif
+
+ if (check_stat) {
+ if (sd-sd_uid != (unsigned int) st-st_uid ||
+ sd-sd_gid != (unsigned int) st-st_gid)
+ changed |= OWNER_CHANGED;
+ if (sd-sd_ino != (unsigned int) st-st_ino)
+
[PATCH 10/12] get_packed_ref_cache: reload packed-refs file when it changes
From: Jeff King p...@peff.net
Once we read the packed-refs file into memory, we cache it
to save work on future ref lookups. However, our cache may
be out of date with respect to what is on disk if another
process is simultaneously packing the refs. Normally it
is acceptable for us to be a little out of date, since there
is no guarantee whether we read the file before or after the
simultaneous update. However, there is an important special
case: our packed-refs file must be up to date with respect
to any loose refs we read. Otherwise, we risk the following
race condition:
0. There exists a loose ref refs/heads/master.
1. Process A starts and looks up the ref master. It
first checks $GIT_DIR/master, which does not exist. It
then loads (and caches) the packed-refs file to see if
master exists in it, which it does not.
2. Meanwhile, process B runs pack-refs --all --prune. It
creates a new packed-refs file which contains
refs/heads/master, and removes the loose copy at
$GIT_DIR/refs/heads/master.
3. Process A continues its lookup, and eventually tries
$GIT_DIR/refs/heads/master. It sees that the loose ref
is missing, and falls back to the packed-refs file. But
it examines its cached version, which does not have
refs/heads/master. After trying a few other prefixes,
it reports master as a non-existent ref.
There are many variants (e.g., step 1 may involve process A
looking up another ref entirely, so even a fully qualified
refname can fail). One of the most interesting ones is if
refs/heads/master is already packed. In that case process
A will not see it as missing, but rather will report
whatever value happened to be in the packed-refs file before
process B repacked (which might be an arbitrarily old
value).
We can fix this by making sure we reload the packed-refs
file from disk after looking at any loose refs. That's
unacceptably slow, so we can check its stat()-validity as a
proxy, and read it only when it appears to have changed.
Reading the packed-refs file after performing any loose-ref
system calls is sufficient because we know the ordering of
the pack-refs process: it always makes sure the newly
written packed-refs file is installed into place before
pruning any loose refs. As long as those operations by B
appear in their executed order to process A, by the time A
sees the missing loose ref, the new packed-refs file must be
in place.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
This is Peff's work, rebased and with some smallish changes to fit it
in with the packed_ref_cache data structure.
refs.c | 21 -
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index 92c8e97..64f72ab 100644
--- a/refs.c
+++ b/refs.c
@@ -824,6 +824,9 @@ struct packed_ref_cache {
/* If locked, the file descriptor of the lock file. */
int fd;
+
+ /* The metadata from when this packed-refs cache was read */
+ struct stat_validity validity;
};
/*
@@ -858,6 +861,7 @@ static int release_packed_ref_cache(struct packed_ref_cache
*packed_refs)
{
if (!--packed_refs-referrers) {
free_ref_entry(packed_refs-root);
+ stat_validity_clear(packed_refs-validity);
free(packed_refs);
return 1;
} else {
@@ -1049,20 +1053,27 @@ static void read_packed_refs(FILE *f, struct ref_dir
*dir)
*/
static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
{
+ const char *packed_refs_file;
+
+ if (*refs-name)
+ packed_refs_file = git_path_submodule(refs-name,
packed-refs);
+ else
+ packed_refs_file = git_path(packed-refs);
+
+ if (refs-packed
+ !stat_validity_check(refs-packed-validity, packed_refs_file))
+ clear_packed_ref_cache(refs);
+
if (!refs-packed) {
- const char *packed_refs_file;
FILE *f;
refs-packed = xcalloc(1, sizeof(*refs-packed));
acquire_packed_ref_cache(refs-packed);
refs-packed-root = create_dir_entry(refs, , 0, 0);
refs-packed-fd = -1;
- if (*refs-name)
- packed_refs_file = git_path_submodule(refs-name,
packed-refs);
- else
- packed_refs_file = git_path(packed-refs);
f = fopen(packed_refs_file, r);
if (f) {
+ stat_validity_update(refs-packed-validity,
fileno(f));
read_packed_refs(f, get_ref_dir(refs-packed-root));
fclose(f);
}
--
1.8.3
--
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 02/12] pack_refs(): split creation of packed refs and entry writing
Split pack_refs() into multiple passes:
* Iterate over loose refs. For each one that can be turned into a
packed ref, create a corresponding entry in the packed refs cache.
* Write the packed refs to the packed-refs file.
This change isolates the mutation of the packed-refs file to a single
place.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 48 ++--
1 file changed, 34 insertions(+), 14 deletions(-)
diff --git a/refs.c b/refs.c
index dccfe14..bb3640f 100644
--- a/refs.c
+++ b/refs.c
@@ -2019,35 +2019,50 @@ struct ref_to_prune {
struct pack_refs_cb_data {
unsigned int flags;
+ struct ref_dir *packed_refs;
struct ref_to_prune *ref_to_prune;
- int fd;
};
-static int pack_one_ref(struct ref_entry *entry, void *cb_data)
+/*
+ * An each_ref_entry_fn that is run over loose references only. If
+ * the loose reference can be packed, add an entry in the packed ref
+ * cache. If the reference should be pruned, also add it to
+ * ref_to_prune in the pack_refs_cb_data.
+ */
+static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
{
struct pack_refs_cb_data *cb = cb_data;
enum peel_status peel_status;
+ struct ref_entry *packed_entry;
int is_tag_ref = !prefixcmp(entry-name, refs/tags/);
- /* ALWAYS pack refs that were already packed or are tags */
- if (!(cb-flags PACK_REFS_ALL) !is_tag_ref
- !(entry-flag REF_ISPACKED))
+ /* ALWAYS pack tags */
+ if (!(cb-flags PACK_REFS_ALL) !is_tag_ref)
return 0;
/* Do not pack symbolic or broken refs: */
if ((entry-flag REF_ISSYMREF) || !ref_resolves_to_object(entry))
return 0;
+ /* Add a packed ref cache entry equivalent to the loose entry. */
peel_status = peel_entry(entry, 1);
if (peel_status != PEEL_PEELED peel_status != PEEL_NON_TAG)
die(internal error peeling reference %s (%s),
entry-name, sha1_to_hex(entry-u.value.sha1));
- write_packed_entry(cb-fd, entry-name, entry-u.value.sha1,
- peel_status == PEEL_PEELED ?
- entry-u.value.peeled : NULL);
+ packed_entry = find_ref(cb-packed_refs, entry-name);
+ if (packed_entry) {
+ /* Overwrite existing packed entry with info from loose entry */
+ packed_entry-flag = REF_ISPACKED | REF_KNOWS_PEELED;
+ hashcpy(packed_entry-u.value.sha1, entry-u.value.sha1);
+ } else {
+ packed_entry = create_ref_entry(entry-name,
entry-u.value.sha1,
+ REF_ISPACKED |
REF_KNOWS_PEELED, 0);
+ add_ref(cb-packed_refs, packed_entry);
+ }
+ hashcpy(packed_entry-u.value.peeled, entry-u.value.peeled);
- /* If the ref was already packed, there is no need to prune it. */
- if ((cb-flags PACK_REFS_PRUNE) !(entry-flag REF_ISPACKED)) {
+ /* Schedule the loose reference for pruning if requested. */
+ if ((cb-flags PACK_REFS_PRUNE)) {
int namelen = strlen(entry-name) + 1;
struct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);
hashcpy(n-sha1, entry-u.value.sha1);
@@ -2114,16 +2129,21 @@ static struct lock_file packlock;
int pack_refs(unsigned int flags)
{
struct pack_refs_cb_data cbdata;
+ int fd;
memset(cbdata, 0, sizeof(cbdata));
cbdata.flags = flags;
- cbdata.fd = hold_lock_file_for_update(packlock,
git_path(packed-refs),
- LOCK_DIE_ON_ERROR);
+ fd = hold_lock_file_for_update(packlock, git_path(packed-refs),
+ LOCK_DIE_ON_ERROR);
+ cbdata.packed_refs = get_packed_refs(ref_cache);
- write_or_die(cbdata.fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
+ do_for_each_entry_in_dir(get_loose_refs(ref_cache), 0,
+pack_if_possible_fn, cbdata);
+
+ write_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
+ do_for_each_entry_in_dir(cbdata.packed_refs, 0, write_packed_entry_fn,
fd);
- do_for_each_entry(ref_cache, , pack_one_ref, cbdata);
if (commit_lock_file(packlock) 0)
die_errno(unable to overwrite old ref-pack file);
prune_refs(cbdata.ref_to_prune);
--
1.8.3
--
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 11/12] for_each_ref: load all loose refs before packed refs
From: Jeff King p...@peff.net
If we are iterating through the refs using for_each_ref (or
any of its sister functions), we can get into a race
condition with a simultaneous pack-refs --prune that looks
like this:
0. We have a large number of loose refs, and a few packed
refs. refs/heads/z/foo is loose, with no matching entry
in the packed-refs file.
1. Process A starts iterating through the refs. It loads
the packed-refs file from disk, then starts lazily
traversing through the loose ref directories.
2. Process B, running pack-refs --prune, writes out the
new packed-refs file. It then deletes the newly packed
refs, including refs/heads/z/foo.
3. Meanwhile, process A has finally gotten to
refs/heads/z (it traverses alphabetically). It
descends, but finds nothing there. It checks its
cached view of the packed-refs file, but it does not
mention anything in refs/heads/z/ at all (it predates
the new file written by B in step 2).
The traversal completes successfully without mentioning
refs/heads/z/foo at all (the name, of course, isn't
important; but the more refs you have and the farther down
the alphabetical list a ref is, the more likely it is to hit
the race). If refs/heads/z/foo did exist in the packed refs
file at state 0, we would see an entry for it, but it would
show whatever sha1 the ref had the last time it was packed
(which could be an arbitrarily long time ago).
This can be especially dangerous when process A is git
prune, as it means our set of reachable tips will be
incomplete, and we may erroneously prune objects reachable
from that tip (the same thing can happen if repack -ad is
used, as it simply drops unreachable objects that are
packed).
This patch solves it by loading all of the loose refs for
our traversal into our in-memory cache, and then refreshing
the packed-refs cache. Because a pack-refs writer will
always put the new packed-refs file into place before
starting the prune, we know that any loose refs we fail to
see will either truly be missing, or will have already been
put in the packed-refs file by the time we refresh.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
Ditto.
refs.c | 39 +++
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 64f72ab..aa4641b 100644
--- a/refs.c
+++ b/refs.c
@@ -746,6 +746,21 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,
}
/*
+ * Load all of the refs from the dir into our in-memory cache. The hard work
+ * of loading loose refs is done by get_ref_dir(), so we just need to recurse
+ * through all of the sub-directories. We do not even need to care about
+ * sorting, as traversal order does not matter to us.
+ */
+static void prime_ref_dir(struct ref_dir *dir)
+{
+ int i;
+ for (i = 0; i dir-nr; i++) {
+ struct ref_entry *entry = dir-entries[i];
+ if (entry-flag REF_DIR)
+ prime_ref_dir(get_ref_dir(entry));
+ }
+}
+/*
* Return true iff refname1 and refname2 conflict with each other.
* Two reference names conflict if one of them exactly matches the
* leading components of the other; e.g., foo/bar conflicts with
@@ -1600,15 +1615,31 @@ void warn_dangling_symref(FILE *fp, const char
*msg_fmt, const char *refname)
static int do_for_each_entry(struct ref_cache *refs, const char *base,
each_ref_entry_fn fn, void *cb_data)
{
- struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
- struct ref_dir *packed_dir = get_packed_ref_dir(packed_ref_cache);
- struct ref_dir *loose_dir = get_loose_refs(refs);
+ struct packed_ref_cache *packed_ref_cache;
+ struct ref_dir *loose_dir;
+ struct ref_dir *packed_dir;
int retval = 0;
+ /*
+* We must make sure that all loose refs are read before accessing the
+* packed-refs file; this avoids a race condition in which loose refs
+* are migrated to the packed-refs file by a simultaneous process, but
+* our in-memory view is from before the migration.
get_packed_ref_cache()
+* takes care of making sure our view is up to date with what is on
+* disk.
+*/
+ loose_dir = get_loose_refs(refs);
+ if (base *base) {
+ loose_dir = find_containing_dir(loose_dir, base, 0);
+ }
+ if (loose_dir)
+ prime_ref_dir(loose_dir);
+
+ packed_ref_cache = get_packed_ref_cache(refs);
acquire_packed_ref_cache(packed_ref_cache);
+ packed_dir = get_packed_ref_dir(packed_ref_cache);
if (base *base) {
packed_dir = find_containing_dir(packed_dir, base, 0);
- loose_dir = find_containing_dir(loose_dir, base, 0);
}
if (packed_dir loose_dir) {
--
1.8.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a
[PATCH 07/12] packed_ref_cache: increment refcount when locked
Increment the packed_ref_cache reference count while it is locked to
prevent its being freed.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index d8e8ce2..92c8e97 100644
--- a/refs.c
+++ b/refs.c
@@ -816,7 +816,9 @@ struct packed_ref_cache {
/*
* Iff the packed-refs file associated with this instance is
* currently locked for writing, this points at the associated
-* lock (which is owned by somebody else).
+* lock (which is owned by somebody else). The referrer count
+* is also incremented when the file is locked and decremented
+* when it is unlocked.
*/
struct lock_file *lock;
@@ -2099,6 +2101,8 @@ int lock_packed_refs(struct lock_file *lock, int flags)
packed_ref_cache = get_packed_ref_cache(ref_cache);
packed_ref_cache-lock = lock;
packed_ref_cache-fd = fd;
+ /* Increment the reference count to prevent it from being freed: */
+ acquire_packed_ref_cache(packed_ref_cache);
return 0;
}
@@ -2119,6 +2123,7 @@ int commit_packed_refs(void)
error = -1;
packed_ref_cache-lock = NULL;
packed_ref_cache-fd = -1;
+ release_packed_ref_cache(packed_ref_cache);
return error;
}
@@ -2132,6 +2137,7 @@ void rollback_packed_refs(void)
rollback_lock_file(packed_ref_cache-lock);
packed_ref_cache-lock = NULL;
packed_ref_cache-fd = -1;
+ release_packed_ref_cache(packed_ref_cache);
clear_packed_ref_cache(ref_cache);
}
--
1.8.3
--
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 04/12] refs: implement simple transactions for the packed-refs file
Handle simple transactions for the packed-refs file at the
packed_ref_cache level via new functions lock_packed_refs(),
commit_packed_refs(), and rollback_packed_refs().
Only allow the packed ref cache to be modified (via add_packed_ref())
while the packed refs file is locked.
Change clone to add the new references within a transaction.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
The API docs are not clear about whether it is kosher to read
lock_file::fd directly. It is only done in one file outside of
lockfile.c. So this patch stores the fd of the lockfile separately in
struct packed_ref_cache, even though the same struct also has a
pointer to the struct lock_file.
So please let me know if it is OK to read lock_file::fd directly. If
so, then I will drop the fd member of struct packed_ref_cache, as well
as the local variable fd in lock_packed_refs().
builtin/clone.c | 7 -
refs.c | 91 +++--
refs.h | 27 +++--
3 files changed, 106 insertions(+), 19 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 66bff57..b0c000a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -489,17 +489,22 @@ static struct ref *wanted_peer_refs(const struct ref
*refs,
return local_refs;
}
+static struct lock_file packed_refs_lock;
+
static void write_remote_refs(const struct ref *local_refs)
{
const struct ref *r;
+ lock_packed_refs(packed_refs_lock, LOCK_DIE_ON_ERROR);
+
for (r = local_refs; r; r = r-next) {
if (!r-peer_ref)
continue;
add_packed_ref(r-peer_ref-name, r-old_sha1);
}
- pack_refs(PACK_REFS_ALL);
+ if (commit_packed_refs())
+ die_errno(unable to overwrite old ref-pack file);
}
static void write_followtags(const struct ref *refs, const char *msg)
diff --git a/refs.c b/refs.c
index 4cee36b..114bd5b 100644
--- a/refs.c
+++ b/refs.c
@@ -804,6 +804,16 @@ static int is_refname_available(const char *refname, const
char *oldrefname,
struct packed_ref_cache {
struct ref_entry *root;
+
+ /*
+* Iff the packed-refs file associated with this instance is
+* currently locked for writing, this points at the associated
+* lock (which is owned by somebody else).
+*/
+ struct lock_file *lock;
+
+ /* If locked, the file descriptor of the lock file. */
+ int fd;
};
/*
@@ -825,6 +835,8 @@ static struct ref_cache {
static void clear_packed_ref_cache(struct ref_cache *refs)
{
if (refs-packed) {
+ if (refs-packed-lock)
+ die(internal error: packed-ref cache cleared while
locked);
free_ref_entry(refs-packed-root);
free(refs-packed);
refs-packed = NULL;
@@ -1009,6 +1021,7 @@ static struct packed_ref_cache
*get_packed_ref_cache(struct ref_cache *refs)
refs-packed = xcalloc(1, sizeof(*refs-packed));
refs-packed-root = create_dir_entry(refs, , 0, 0);
+ refs-packed-fd = -1;
if (*refs-name)
packed_refs_file = git_path_submodule(refs-name,
packed-refs);
else
@@ -1034,7 +1047,12 @@ static struct ref_dir *get_packed_refs(struct ref_cache
*refs)
void add_packed_ref(const char *refname, const unsigned char *sha1)
{
- add_ref(get_packed_refs(ref_cache),
+ struct packed_ref_cache *packed_ref_cache =
+ get_packed_ref_cache(ref_cache);
+
+ if (!packed_ref_cache-lock)
+ die(internal error: packed refs not locked);
+ add_ref(get_packed_ref_dir(packed_ref_cache),
create_ref_entry(refname, sha1, REF_ISPACKED, 1));
}
@@ -2031,6 +2049,56 @@ static int write_packed_entry_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
+int lock_packed_refs(struct lock_file *lock, int flags)
+{
+ int fd;
+ struct packed_ref_cache *packed_ref_cache;
+
+ /* Discard the old cache because it might be invalid: */
+ clear_packed_ref_cache(ref_cache);
+ fd = hold_lock_file_for_update(lock, git_path(packed-refs), flags);
+ if (fd 0)
+ return -1;
+ /* Read the current packed-refs while holding the lock: */
+ packed_ref_cache = get_packed_ref_cache(ref_cache);
+ packed_ref_cache-lock = lock;
+ packed_ref_cache-fd = fd;
+ return 0;
+}
+
+int commit_packed_refs(void)
+{
+ struct packed_ref_cache *packed_ref_cache =
+ get_packed_ref_cache(ref_cache);
+ int fd = packed_ref_cache-fd;
+ int error = 0;
+
+ if (!packed_ref_cache-lock)
+ die(internal error: packed-refs not locked);
+ write_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));
+
+ do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
+
[PATCH 09/12] add a stat_validity struct
It can sometimes be useful to know whether a path in the
filesystem has been updated without going to the work of
opening and re-reading its content. We trust the stat()
information on disk already to handle index updates, and we
can use the same trick here.
This patch introduces a stat_validity struct which
encapsulates the concept of checking the stat-freshness of a
file. It is implemented on top of struct stat_data to
reuse the logic about which stat entries to trust for a
particular platform, but hides the complexity behind two
simple functions: check and update.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
This is *very* similar to a patch by Jeff King p...@peff.net [1],
except that it is based on the struct stat_data that I extracted from
cache_entry rather than using cache_entries directly. I would have
left Peff the author except that I don't want to risk putting him on
the hook for any mistakes that I might have made. But if it is
appropriate, don't hesitate to make him author again.
[1] http://article.gmane.org/gmane.comp.version-control.git/223526
cache.h | 27 +++
read-cache.c | 30 ++
2 files changed, 57 insertions(+)
diff --git a/cache.h b/cache.h
index 207f849..15f5110 100644
--- a/cache.h
+++ b/cache.h
@@ -1358,4 +1358,31 @@ int checkout_fast_forward(const unsigned char *from,
int sane_execvp(const char *file, char *const argv[]);
+/*
+ * A struct to encapsulate the concept of whether a file has changed
+ * since we last checked it. This uses criteria similar to those used
+ * for the index.
+ */
+struct stat_validity {
+ struct stat_data *sd;
+};
+
+void stat_validity_clear(struct stat_validity *sv);
+
+/*
+ * Returns 1 if the path is a regular file (or a symlink to a regular
+ * file) and matches the saved stat_validity, 0 otherwise. A missing
+ * or inaccessible file is considered a match if the struct was just
+ * initialized, or if the previous update found an inaccessible file.
+ */
+int stat_validity_check(struct stat_validity *sv, const char *path);
+
+/*
+ * Update the stat_validity from a file opened at descriptor fd. If
+ * the file is missing, inaccessible, or not a regular file, then
+ * future calls to stat_validity_check will match iff one of those
+ * conditions continues to be true.
+ */
+void stat_validity_update(struct stat_validity *sv, int fd);
+
#endif /* CACHE_H */
diff --git a/read-cache.c b/read-cache.c
index 4c4328e..73e85a4 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1948,3 +1948,33 @@ void *read_blob_data_from_index(struct index_state
*istate, const char *path, un
*size = sz;
return data;
}
+
+void stat_validity_clear(struct stat_validity *sv)
+{
+ free(sv-sd);
+ sv-sd = NULL;
+}
+
+int stat_validity_check(struct stat_validity *sv, const char *path)
+{
+ struct stat st;
+
+ if (stat(path, st) 0)
+ return sv-sd == NULL;
+ if (!sv-sd)
+ return 0;
+ return S_ISREG(st.st_mode) !match_stat_data(sv-sd, st);
+}
+
+void stat_validity_update(struct stat_validity *sv, int fd)
+{
+ struct stat st;
+
+ if (fstat(fd, st) 0 || !S_ISREG(st.st_mode))
+ stat_validity_clear(sv);
+ else {
+ if (!sv-sd)
+ sv-sd = xcalloc(1, sizeof(struct stat_data));
+ fill_stat_data(sv-sd, st);
+ }
+}
--
1.8.3
--
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 06/12] do_for_each_entry(): increment the packed refs cache refcount
This function calls a user-supplied callback function which could do
something that causes the packed refs cache to be invalidated. So
acquire a reference count on the data structure to prevent our copy
from being freed while we are iterating over it.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index b0d77a7..d8e8ce2 100644
--- a/refs.c
+++ b/refs.c
@@ -1587,10 +1587,12 @@ void warn_dangling_symref(FILE *fp, const char
*msg_fmt, const char *refname)
static int do_for_each_entry(struct ref_cache *refs, const char *base,
each_ref_entry_fn fn, void *cb_data)
{
- struct ref_dir *packed_dir = get_packed_refs(refs);
+ struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);
+ struct ref_dir *packed_dir = get_packed_ref_dir(packed_ref_cache);
struct ref_dir *loose_dir = get_loose_refs(refs);
int retval = 0;
+ acquire_packed_ref_cache(packed_ref_cache);
if (base *base) {
packed_dir = find_containing_dir(packed_dir, base, 0);
loose_dir = find_containing_dir(loose_dir, base, 0);
@@ -1611,6 +1613,7 @@ static int do_for_each_entry(struct ref_cache *refs,
const char *base,
loose_dir, 0, fn, cb_data);
}
+ release_packed_ref_cache(packed_ref_cache);
return retval;
}
--
1.8.3
--
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 v3 07/28] git-remote-mediawiki: Rewrite unclear line of instructions
Célestin Matte celestin.ma...@ensimag.fr writes: Le 11/06/2013 20:09, Junio C Hamano a écrit : Matthieu Moy matthieu@grenoble-inp.fr writes: my ($namespace) = @_; my $namespace = shift; My impression has been that both are equally common, The second is the most common in git-remote-mediawiki (but I don't have any preference nor know what is recommended elsewhere). I wasn't implying I prefer the former. I was just being curious, and if the latter is more prevalent in the code _and_ Perlcritique does not have any issue with it, it is perfectly fine. Perlcritic doesn't have an issue with any of both cases. OK. As this topic is about matching the opinion of Perlcritique, I think it is fine either way (which was the primary thing that I wanted to find out). I think the second method is clearer when there is only one argument, because it makes it clear that there is only one. Hmm, from the maintenance point of view, the second one invites the next person to extend this function like this: my $namespace = shift; + my $newargument = shift; + my $anotherargument = shift; If your original were in the first style, instead you would likely to get this: - my ($namespace) = @_; + my ($namespace, $newargument, $anotherargument) = @_; When there is only one argument, it is clear that there is only one argument in either style. It is not a strong factor to pick one style over the other. Once you start taking more than one argument, however, I think it makes it clear what arguments the function takes would actually favor the style to split @_ into a list of local variables. But as I said earlier, this patch is about following Perlcritique's advice, and because it does not have opinion on these styles, it is outside the scope of this patch. Thanks for checking. -- 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 v4 00/31] git-remote-mediawiki: Follow perlcritic's recommandations
New (and hopefully last version) of my series of patches to follow perlcritic's recommandations Changes with v3: - Remove whitespace in [18/28] - Typo in [09/28] - Better line split in [22/28] - A part of the file @@ -610,9 +610,9 @@ had escaped patches [22/31] and [23/31] for some reason. This is fixed. - patch [29/31] and [30/31] are new: they add a .perlcriticrc file to ignore some rules and add a rule in the Makefile for perlcritic - patch [31/31] is also a new one, which intends to make some error messages more precise. It comes from an advice from es in the reviewing of v1, that I had forgotten to add in earlier versions. It is not related to perlcritic, but I hope it can be included into this series of patches anyway. Changes with v2: - Remove patch [02/22] about using the Readonly module - Split commit [07/22] into 5 different ones - Split commit [14/22] into 2 different ones - Patch [17/22] was *not* split: tell me if it is necessary - Remove wrong change in patch [22/22] Changes with v1: - split first commit into 6 different commits - remove commit [17/18] about moving open() call - took every other comment into account Célestin Matte (31): git-remote-mediawiki: Make a regexp clearer git-remote-mediawiki: Move use warnings; before any instruction git-remote-mediawiki: Replace :utf8 by :encoding(UTF-8) git-remote-mediawiki: Always end a subroutine with a return git-remote-mediawiki: Move a variable declaration at the top of the code git-remote-mediawiki: Change syntax of map calls git-remote-mediawiki: Rewrite unclear line of instructions git-remote-mediawiki: Remove useless regexp modifier (m) git-remote-mediawiki: Change the behaviour of a split git-remote-mediawiki: Change separator of some regexps git-remote-mediawiki: Change style in a regexp git-remote-mediawiki: Change style in a regexp git-remote-mediawiki: Add newline in the end of die() error messages git-remote-mediawiki: Change the name of a variable git-remote-mediawiki: Turn double-negated expressions into simple expressions git-remote-mediawiki: Remove unused variable $entry git-remote-mediawiki: Rename a variable ($last) which has the name of a keyword git-remote-mediawiki: Assign a variable as undef and make proper indentation git-remote-mediawiki: Check return value of open git-remote-mediawiki: remove import of unused open2 git-remote-mediawiki: Put long code into a subroutine git-remote-mediawiki: Modify strings for a better coding-style git-remote-mediawiki: Brace file handles for print for more clarity git-remote-mediawiki: Replace unless statements with negated if statements git-remote-mediawiki: Don't use quotes for empty strings git-remote-mediawiki: Put non-trivial numeric values in constants. git-remote-mediawiki: Fix a typo (mediwiki instead of mediawiki) git-remote-mediawiki: Clearly rewrite double dereference git-remote-mediawiki: Add a .perlcriticrc file git-remote-mediawiki: add a perlcritic rule in Makefile git-remote-mediawiki: Make error message more precise contrib/mw-to-git/.perlcriticrc | 28 ++ contrib/mw-to-git/Makefile |5 +- contrib/mw-to-git/git-remote-mediawiki.perl | 543 +++ 3 files changed, 327 insertions(+), 249 deletions(-) create mode 100644 contrib/mw-to-git/.perlcriticrc -- 1.7.9.5 -- 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 v4 24/31] git-remote-mediawiki: Replace unless statements with negated if statements
Signed-off-by: Célestin Matte celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
contrib/mw-to-git/git-remote-mediawiki.perl | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl
b/contrib/mw-to-git/git-remote-mediawiki.perl
index 5174080..4764be5 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -86,11 +86,11 @@ $shallow_import = ($shallow_import eq 'true');
# - by_rev: perform one query per new revision on the remote wiki
# - by_page: query each tracked page for new revision
my $fetch_strategy = run_git(config --get
remote.${remotename}.fetchStrategy);
-unless ($fetch_strategy) {
+if (!$fetch_strategy) {
$fetch_strategy = run_git('config --get mediawiki.fetchStrategy');
}
chomp($fetch_strategy);
-unless ($fetch_strategy) {
+if (!$fetch_strategy) {
$fetch_strategy = 'by_page';
}
@@ -112,7 +112,7 @@ my %basetimestamps;
# deterministic, this means everybody gets the same sha1 for each
# MediaWiki revision.
my $dumb_push = run_git(config --get --bool remote.${remotename}.dumbPush);
-unless ($dumb_push) {
+if (!$dumb_push) {
$dumb_push = run_git('config --get --bool mediawiki.dumbPush');
}
chomp($dumb_push);
@@ -671,7 +671,7 @@ sub fetch_mw_revisions_for_page {
push(@page_revs, $page_rev_ids);
$revnum++;
}
- last unless $result-{'query-continue'};
+ last if (!$result-{'query-continue'});
$query-{rvstartid} =
$result-{'query-continue'}-{revisions}-{rvstartid};
}
if ($shallow_import @page_revs) {
@@ -1243,7 +1243,7 @@ sub mw_push_revision {
die(Unknown error from mw_push_file()\n);
}
}
- unless ($dumb_push) {
+ if (!$dumb_push) {
run_git(qq(notes --ref=${remotename}/mediawiki add -f
-m mediawiki_revision: ${mw_revision} ${sha1_commit}));
run_git(qq(update-ref -m Git-MediaWiki push
refs/mediawiki/${remotename}/master ${sha1_commit} ${sha1_child}));
}
@@ -1324,7 +1324,7 @@ sub get_mw_namespace_id {
my $ns = $namespace_id{$name};
my $id;
- unless (defined $ns) {
+ if (!defined $ns) {
print {*STDERR} No such namespace ${name} on MediaWiki.\n;
$ns = {is_namespace = 0};
$namespace_id{$name} = $ns;
--
1.7.9.5
--
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 v4 29/31] git-remote-mediawiki: Add a .perlcriticrc file
Such a file allows to configure perlcritic. Here, it is used to prevent to remove many unwanted rules and configure one to remove unwanted warnings. Signed-off-by: Célestin Matte celestin.ma...@ensimag.fr Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr --- contrib/mw-to-git/.perlcriticrc | 28 1 file changed, 28 insertions(+) create mode 100644 contrib/mw-to-git/.perlcriticrc diff --git a/contrib/mw-to-git/.perlcriticrc b/contrib/mw-to-git/.perlcriticrc new file mode 100644 index 000..a1f8451 --- /dev/null +++ b/contrib/mw-to-git/.perlcriticrc @@ -0,0 +1,28 @@ +# These 3 rules demand to add the s, m and x flag to *every* regexp. This is +# overkill and would be harmful for readability. +[-RegularExpressions::RequireExtendedFormatting] +[-RegularExpressions::RequireDotMatchAnything] +[-RegularExpressions::RequireLineBoundaryMatching] + +# This rules says that builtin functions should not be called with parenthesis +# e.g.: (taken from CPAN's documentation) +# open($handle, '', $filename); #not ok +# open $handle, '', $filename; #ok +# Applying such a rule would mean modifying a huge number of lines for a +# question of style. +[-CodeLayout::ProhibitParensWithBuiltins] + +# This rule states that each system call should have its return value checked +# The problem is that it includes the print call. Checking every print call's +# return value would be harmful to the code readabilty. +# This configuration keeps all default function but print. +[InputOutput::RequireCheckedSyscalls] +functions = open say close + +# This rules demands to add a dependancy for the Readonly module. This is not +# wished. +[-ValuesAndExpressions::ProhibitConstantPragma] + +# This rule is not really useful (rather a question of style) and produces many +# warnings among the code. +[-ValuesAndExpressions::ProhibitNoisyQuotes] -- 1.7.9.5 -- 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
