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: [PATCH] Documentation/CommunityGuidelines
On Mon, Jun 10, 2013 at 11:41 PM, Michael Haggerty 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'
Re: [PATCH] Documentation/CommunityGuidelines
On Mon, Jun 10, 2013 at 8:28 AM, Ramkumar Ramachandra wrote: > I've tried to write down a bare minimum, without restating the obvious. 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. > 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. I don't like the wording of this. "Attacker", "humiliation", "everyone"; it's very absolutist rhetoric. Yes, you see the other person as an attacker, and yes you think she is humiliating herself in front of everyone, but thinking so doesn't make it so. An even better and less absolutist version would be: Do not participate in flamewars. It is very tempting to prove somebody else wrong, but if you think a discussion is turning into a flamewar, just say so, and avoid it. Do not throw lumber to the flames. You might feel you should correct the erroneous claims being made in public, but by replying you are making things worst. Leave the erroneous (in your opinion) claims alone, the damage has been done, all that is left is what *you* can do, and the best you can do is ignore them. > 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." > 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. 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. -- 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 Mon, Jun 10, 2013 at 12:42 PM, Junio C Hamano wrote: > "Robin H. Johnson" writes: > >> On Mon, Jun 10, 2013 at 04:04:29PM +0200, Matthieu Moy wrote: >>> Célestin Matte writes: >>> >>> > Le 10/06/2013 15:28, Ramkumar Ramachandra a écrit : >>> >> 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. >>> > >>> > "Herself"? >>> > Typo I guess :) >>> >>> Not necessarily. It's quite common in english to use "she" when the >>> gender is not known. >> Could you please use "themself" instead? > > I think "himself or herself" is the politically correct form ;-) > > But more seriously. > > 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. > > 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? Science shows that humans are in fact, not rational people. It's simply one of our countless limitations. We acknowledge we have physical limitations, but we forget our mental limitations. We should aim to be rational people, yes, but we are not. > 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? First of all, you should not refer to it as "poisonous behavior". Maybe you think it's poisonous, maybe everyone else in the mailing list agrees it's poisonous, but talk doesn't make things real, otherwise there were a lot of real witches in the past. You should refer to it as 'what could be considered poisonous behavior'. That is accurate. Calling it "poisonous behavior" at best can be considered a logical fallacy, and at worst could even be described as a poisonous comment itself. > 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". > > I dunno. I think we all know at some level why flame war arise, as XKCD makes it comically succinct[1]. If somebody wants to argue for the sake of arguing, they should go to some forum, or reddit, or something else other than the mailing list. In the mailing list you should avoid flamewars. If you have identified a flamewar, don't poke it, and ask for others to do the same; don't throw lumber unto the flames. I know you worry that somebody is wrong on the Internet, and you worry that somebody else might read that, and think the person that is wrong is actually right. But you cannot fix that. Move on. If the reader is smart, they'll understand the signal "Don't throw lumber unto the flames." followed by silence from other members of the community. Trying to "correct" somebody often sends the wrong signal; you validate the other person's point of view as something worth arguing about. If you truly think a flamewar is taking place, resist your urge to participate in it, and mute it. Maybe it's not really flamewar, and something important is being discussed, but you should leave the people that do not think a flamewar is taking place to argue with each other, and stay out of that. If you think it's a flamewar, and you comment in it, you are making it worst, and perhaps turning it into a real flamewar if it wasn't. In general; do not participate in a flamewar. Period. [1] http://xkcd.com/386/ -- 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 45/45] tests: update topology tests
On Mon, Jun 10, 2013 at 11:37 PM, Martin von Zweigbergk
wrote:
> On Sun, Jun 9, 2013 at 9:40 AM, Felipe Contreras
> wrote:
>> Signed-off-by: Felipe Contreras
>> ---
>> t/t3425-rebase-topology-merges.sh | 15 ++-
>> 1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/t/t3425-rebase-topology-merges.sh
>> b/t/t3425-rebase-topology-merges.sh
>> index 5400a05..96cc479 100755
>> --- a/t/t3425-rebase-topology-merges.sh
>> +++ b/t/t3425-rebase-topology-merges.sh
>> @@ -70,9 +70,8 @@ test_run_rebase () {
>> test_linear_range "\'"$expected"\'" d..
>> "
>> }
>> -#TODO: make order consistent across all flavors of rebase
>> -test_run_rebase success 'e n o' ''
>> -test_run_rebase success 'e n o' -m
>> +test_run_rebase success 'n o e' ''
>> +test_run_rebase success 'n o e' -m
>> test_run_rebase success 'n o e' -i
>
> If you do end up re-sending the series on top of my series, I'd prefer
> to see the end result having the first argument inlined, so these few
> lines become simply:
Somebody else would need to do that, there's no point in me sending
these patches, just to increase the number of my patches that get
completely ignored by Junio.
--
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 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. > 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. > 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. > 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. > 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 explaining what > went wrong. I would prefer a community standards document that looks more like this: * Treat other community members with courteousness and respect. * Conduct disagreements on a technical, not a personal, level. It is unacceptable to attack another community member personally, even by insinuation. * Keep in mind that email is a medium prone to misunderstandings, and that many mailing list participants do not speak English as their first language. Interpret other people's emails charitably. If you are not sure that you understand, ask for clarification. Assume good intentions on the part of others, and do not attribute technical disagreements to ulterior motives. Choose your words carefully to help other people avoid misinterpreting them, and avoid hyperbole. * Strive to keep the mailing list a forum for effective collaboration. Only post if you have something worthwhile to add to the discussion. Be concise and do not repeat what has already been said. Code reviews, contributions of patches, and concrete data such as bug reports are far preferable to philosophizing, vague suggestions, and whining. Avoid bikeshedding and do not participate in flame wars. Avoid revisiting settled debates unless the facts have changed. * 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. * When reviewing other peoples' code, be tactful and constructive. Set
Re: [PATCH v4 45/45] tests: update topology tests
On Sun, Jun 9, 2013 at 9:40 AM, Felipe Contreras
wrote:
> Signed-off-by: Felipe Contreras
> ---
> t/t3425-rebase-topology-merges.sh | 15 ++-
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/t/t3425-rebase-topology-merges.sh
> b/t/t3425-rebase-topology-merges.sh
> index 5400a05..96cc479 100755
> --- a/t/t3425-rebase-topology-merges.sh
> +++ b/t/t3425-rebase-topology-merges.sh
> @@ -70,9 +70,8 @@ test_run_rebase () {
> test_linear_range "\'"$expected"\'" d..
> "
> }
> -#TODO: make order consistent across all flavors of rebase
> -test_run_rebase success 'e n o' ''
> -test_run_rebase success 'e n o' -m
> +test_run_rebase success 'n o e' ''
> +test_run_rebase success 'n o e' -m
> test_run_rebase success 'n o e' -i
If you do end up re-sending the series on top of my series, I'd prefer
to see the end result having the first argument inlined, so these few
lines become simply:
test_run_rebase success ''
test_run_rebase success -m
test_run_rebase success -i
--
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 Mon, Jun 10, 2013 at 8:53 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> And I do not see the reason why builtin/*.o should not depend on >> each other. It is not messed up at all. They are meant to be >> linked into a single binary---that is what being "built-in" is. >> >> A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o >> by moving parts that do not have to be in the single "git" binary >> but are also usable in standalone binaries out of them. > > Actually, as long as these pieces are currently used by builtins, > moving them (e.g. init_copy_notes_for_rewrite()) out of builtin/*.o > will not make these parts not to be in the single "git" binary at > all, so the above is grossly misstated. > > - 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? > And that is the reason why slimming builtin/*.o is the way it > *SHOULD* be. > > Another thing to think about is looking at pieces of data and > functions defined in each *.o files and moving things around within > them. For example, looking at the dependency chain I quoted earlier > for sequencer.o to build upload-pack, which is about responding to > "git fetch" on the sending side: > > upload-pack.c wants handle_revision_opt etc. > revision.c provides handle_revision_opt > wants name_decoration etc. > log-tree.c provides name_decoration > wants append_signoff > sequencer.c provides append_signoff > > It is already crazy. There is no reason for the pack sender to be > linking with the sequencer interpreter machinery. If the function > definition (and possibly other ones) are split into separate source > files (still in libgit.a), git-upload-pack binary does not have to > pull in the whole sequencer.c at all. Agreed, which is precisely why my patches move that code out of sequencer.c. Maybe log-tree.c is not the right destination, but it is a step in the right direction. > Coming back to the categorization Peff earlier made in the thread, I > think I am OK with adding new two subdirectories to the root level, > i.e. > > builtin/- the ones that define cmd_foo() As is the case right now. > commands/ - the ones that has main() for standalone commands Good. > libsrc/ - the ones that go in libgit.a lib/ is probably descriptive enough. But this doesn't answer the question; what about code that is shared between builtins, but cannot be used by standalone programs? I'd wager it belongs to builtin/ and should be linked to builtin/lib.a. Maybe you would like to have a separate builtin/lib/ directory, but I think that's overkill. > We may also want to add another subdirectory to hold scripted > Porcelains, but the primary topic of this thread is what to do about > the C library, so it is orthogonal in that sense, but if we were to > go in the "group things in subdirectories to slim the root level" > direction, it may be worth considering doing so at the same time. Agreed. Plus there's completions, shell prompt, and other script-like tools that shouldn't really belong in contrib/, and probably installed by default. -- 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
On Mon, Jun 10, 2013 at 7:04 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >>> *1* ... which is a very reasonable thing to do. But moving >>> sequencer.o to builtin/sequencer.o is *not* the way to do this. >> >> By now we all know what is the *CURRENT* way to do this; in other >> words, the status quo, which is BTW all messed up, because builtin/*.o >> objects depend on each other already. > > builtin/*.o are allowed to depend on each other. They are by > definition builtins, meant to be linked into a single binary. That's not what John Keeping said[1]. I'm going to assume he was wrong, but I don't think that's relevant for my point. Either way, the meaning of builtin/ should probably be explained somewhere. >> We are discussing the way it *SHOULD* be. Why aren't you leaning on that? > > And I do not see the reason why builtin/*.o should not depend on > each other. It is not messed up at all. They are meant to be > linked into a single binary---that is what being "built-in" is. > > A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o > by moving parts that do not have to be in the single "git" binary > but are also usable in standalone binaries out of them. > > And that is what I just suggested. But init_copy_notes_for_rewrite() can *not* be used by anything other than git builtins. Standalone binaries will never use such a function, therefore it doesn't belong in libgit.a. Another example is alias_lookup(). They belong in builtin/lib.a. [1] http://article.gmane.org/gmane.comp.version-control.git/227017 -- 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 v5 00/36] Massive improvents to rebase and cherry-pick
On Mon, Jun 10, 2013 at 8:09 PM, Phil Hord wrote: > On Mon, Jun 10, 2013 at 7:43 PM, Felipe Contreras > wrote: >> On Mon, Jun 10, 2013 at 5:55 PM, Phil Hord wrote: >>> On Sun, Jun 9, 2013 at 3:37 PM, Felipe Contreras >>> wrote: On Sun, Jun 9, 2013 at 2:24 PM, Felipe Contreras wrote: > Same as before, but: Also, remove the patches from Martin von Zweigbergk, because apparently some people have trouble understanding that they were not part of this series. >>> >>> Please try not to sound disgruntled. This attitude is toxic. You have >>> turned this change into a complaint: that "some people have trouble >>> understanding" which shows a genuine lack of understanding and >>> compassion on your part. Instead you can phrase your change notes >>> more helpfully if you make changes only when you yourself actually >>> believe the change should be made. If you cannot do this, perhaps you >>> can pretend. >> >> That would be dishonest. Moreover, there wasn't a good reason to >> remove these patches, I made it clear I added those patches only to >> make sure the real patches of this series worked correctly. Also, I >> clarified that to Thomas Rast[1], only to receive a totally >> unconstructive comment[2]. >> >> Why don't you ask Thomas Rast to be more constructive[2]? >> >> Then Johan Herland uses that as an example of a constructive >> comment[3]. Why don't you correct Johan Herland? > > I do not see what their comments have to do with your attitude. My attitude is fine. I sent a lot of patches, and I made clear that some of them were meant only to test the rest. And I clarified that twice. There's nothing wrong with that. > Aren't your own man with cogent self-will and personal responsibility? > Why should I also have to consider these other emails which I have > not bothered to read yet? Don't be that girlfriend that brings the times you haven't picked up the towel properly when talking about something completely and totally different. When talking about the attitude in *this* patch series, limit yourself to *this* patch series. >> No, you pick the easy target: me. > > You seem to have mistaken me for someone else. Moreover, you seem to > have mistaken you for someone else. You are the least easy target I > know of on this list. > Everyone else seems open to community standards. And yet you try to correct me, who did nothing wrong. And ignore the transgressions of the other people, whom I already demonstrated actually *did* do something wrong. How convenient of you to not mention my arguments *at all*. >> I already dd more than my fair share by carrying these 36 patches >> through several iterations, yet you ask *more* of me. Why don't you >> ask more of the people that just hit reply on their MUA? >> >> Thomas' task was easy; he simply had to say "Oh, these aren't meant to >> be applied, got it." >> >> [1] http://article.gmane.org/gmane.comp.version-control.git/227039 >> [2] http://article.gmane.org/gmane.comp.version-control.git/227040 >> [3] http://article.gmane.org/gmane.comp.version-control.git/227102 > > I did not comment on their posts because they did not catch my eye. > Rebase and cherry-pick improvements are interesting to me, so I read > your post. I will try not to make this mistake again. Yes, because my patches are so obviously wrong. If you were a truly productive member of this community, you would ignore all the bullshit, take the patches, fix whatever is technically wrong with them (nothing), and resend them. But no, that would be way too productive. -- 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
Junio C Hamano writes: > And I do not see the reason why builtin/*.o should not depend on > each other. It is not messed up at all. They are meant to be > linked into a single binary---that is what being "built-in" is. > > A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o > by moving parts that do not have to be in the single "git" binary > but are also usable in standalone binaries out of them. Actually, as long as these pieces are currently used by builtins, moving them (e.g. init_copy_notes_for_rewrite()) out of builtin/*.o will not make these parts not to be in the single "git" binary at all, so the above is grossly misstated. - 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. And that is the reason why slimming builtin/*.o is the way it *SHOULD* be. Another thing to think about is looking at pieces of data and functions defined in each *.o files and moving things around within them. For example, looking at the dependency chain I quoted earlier for sequencer.o to build upload-pack, which is about responding to "git fetch" on the sending side: upload-pack.c wants handle_revision_opt etc. revision.c provides handle_revision_opt wants name_decoration etc. log-tree.c provides name_decoration wants append_signoff sequencer.c provides append_signoff It is already crazy. There is no reason for the pack sender to be linking with the sequencer interpreter machinery. If the function definition (and possibly other ones) are split into separate source files (still in libgit.a), git-upload-pack binary does not have to pull in the whole sequencer.c at all. Coming back to the categorization Peff earlier made in the thread, I think I am OK with adding new two subdirectories to the root level, i.e. builtin/- the ones that define cmd_foo() commands/ - the ones that has main() for standalone commands libsrc/ - the ones that go in libgit.a We may also want to add another subdirectory to hold scripted Porcelains, but the primary topic of this thread is what to do about the C library, so it is orthogonal in that sense, but if we were to go in the "group things in subdirectories to slim the root level" direction, it may be worth considering doing so at the same time. -- 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
http://info.yahoo.com/legal/ru/yahoo/mail/atos.html
Отправлено с iPhone пл
Re: [PATCH v5 00/36] Massive improvents to rebase and cherry-pick
On Mon, Jun 10, 2013 at 7:43 PM, Felipe Contreras wrote: > On Mon, Jun 10, 2013 at 5:55 PM, Phil Hord wrote: >> On Sun, Jun 9, 2013 at 3:37 PM, Felipe Contreras >> wrote: >>> >>> On Sun, Jun 9, 2013 at 2:24 PM, Felipe Contreras >>> wrote: >>> > Same as before, but: >>> >>> Also, remove the patches from Martin von Zweigbergk, because >>> apparently some people have trouble understanding that they were not >>> part of this series. >> >> Please try not to sound disgruntled. This attitude is toxic. You have >> turned this change into a complaint: that "some people have trouble >> understanding" which shows a genuine lack of understanding and >> compassion on your part. Instead you can phrase your change notes >> more helpfully if you make changes only when you yourself actually >> believe the change should be made. If you cannot do this, perhaps you >> can pretend. > > That would be dishonest. Moreover, there wasn't a good reason to > remove these patches, I made it clear I added those patches only to > make sure the real patches of this series worked correctly. Also, I > clarified that to Thomas Rast[1], only to receive a totally > unconstructive comment[2]. > > Why don't you ask Thomas Rast to be more constructive[2]? > > Then Johan Herland uses that as an example of a constructive > comment[3]. Why don't you correct Johan Herland? I do not see what their comments have to do with your attitude. Aren't your own man with cogent self-will and personal responsibility? Why should I also have to consider these other emails which I have not bothered to read yet? > No, you pick the easy target: me. You seem to have mistaken me for someone else. Moreover, you seem to have mistaken you for someone else. You are the least easy target I know of on this list. Everyone else seems open to community standards. > I already dd more than my fair share by carrying these 36 patches > through several iterations, yet you ask *more* of me. Why don't you > ask more of the people that just hit reply on their MUA? > > Thomas' task was easy; he simply had to say "Oh, these aren't meant to > be applied, got it." > > [1] http://article.gmane.org/gmane.comp.version-control.git/227039 > [2] http://article.gmane.org/gmane.comp.version-control.git/227040 > [3] http://article.gmane.org/gmane.comp.version-control.git/227102 I did not comment on their posts because they did not catch my eye. Rebase and cherry-pick improvements are interesting to me, so I read your post. I will try not to make this mistake again. Phil -- 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
Пл
Отправлено с iPhone
Re: [PATCH] build: get rid of the notion of a git library
Felipe Contreras writes: >> *1* ... which is a very reasonable thing to do. But moving >> sequencer.o to builtin/sequencer.o is *not* the way to do this. > > By now we all know what is the *CURRENT* way to do this; in other > words, the status quo, which is BTW all messed up, because builtin/*.o > objects depend on each other already. builtin/*.o are allowed to depend on each other. They are by definition builtins, meant to be linked into a single binary. > We are discussing the way it *SHOULD* be. Why aren't you leaning on that? And I do not see the reason why builtin/*.o should not depend on each other. It is not messed up at all. They are meant to be linked into a single binary---that is what being "built-in" is. A good way forward, the way it *SHOULD* be, is to slim the builtin/*.o by moving parts that do not have to be in the single "git" binary but are also usable in standalone binaries out of them. And that is what I just suggested. -- 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 Mon, Jun 10, 2013 at 6:41 PM, Junio C Hamano wrote: > For the particular case of trying to make sequencer.o, which does > not currently have dependencies on builtin/*.o, depend on something > that is in builtin/notes.o, the link phase of standalone that wants > anything from revision.o (which is pretty much everything ;-) goes > like this: > > upload-pack.c wants handle_revision_opt etc. > revision.c provides handle_revision_opt > wants name_decoration etc. > log-tree.c provides name_decoration > wants append_signoff > sequencer.c provides append_signoff > > So sequencer.o _is_ meant to be usable from standalone and belongs > to libgit.a Not after my patch. It moves append_signoff *out* of sequencer, which in fact has nothing to do with the sequencer in the first place. > If sequencer.o wants to call init_copy_notes_for_rewrite() and its > friends [*1*] that are currently in builtin/notes.o, first the > called function(s) should be moved outside builtin/notes.o to > notes.o or somewhere more library-ish place to be included in > libgit.a, which is meant to be usable from standalone. > > > [Footnote] > > *1* ... which is a very reasonable thing to do. But moving > sequencer.o to builtin/sequencer.o is *not* the way to do this. By now we all know what is the *CURRENT* way to do this; in other words, the status quo, which is BTW all messed up, because builtin/*.o objects depend on each other already. We are discussing the way it *SHOULD* be. Why aren't you leaning on 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 v5 00/36] Massive improvents to rebase and cherry-pick
On Mon, Jun 10, 2013 at 5:55 PM, Phil Hord wrote: > On Sun, Jun 9, 2013 at 3:37 PM, Felipe Contreras > wrote: >> >> On Sun, Jun 9, 2013 at 2:24 PM, Felipe Contreras >> wrote: >> > Same as before, but: >> >> Also, remove the patches from Martin von Zweigbergk, because >> apparently some people have trouble understanding that they were not >> part of this series. > > Please try not to sound disgruntled. This attitude is toxic. You have > turned this change into a complaint: that "some people have trouble > understanding" which shows a genuine lack of understanding and > compassion on your part. Instead you can phrase your change notes > more helpfully if you make changes only when you yourself actually > believe the change should be made. If you cannot do this, perhaps you > can pretend. That would be dishonest. Moreover, there wasn't a good reason to remove these patches, I made it clear I added those patches only to make sure the real patches of this series worked correctly. Also, I clarified that to Thomas Rast[1], only to receive a totally unconstructive comment[2]. Why don't you ask Thomas Rast to be more constructive[2]? Then Johan Herland uses that as an example of a constructive comment[3]. Why don't you correct Johan Herland? No, you pick the easy target: me. I already dd more than my fair share by carrying these 36 patches through several iterations, yet you ask *more* of me. Why don't you ask more of the people that just hit reply on their MUA? Thomas' task was easy; he simply had to say "Oh, these aren't meant to be applied, got it." [1] http://article.gmane.org/gmane.comp.version-control.git/227039 [2] http://article.gmane.org/gmane.comp.version-control.git/227040 [3] http://article.gmane.org/gmane.comp.version-control.git/227102 -- 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
Junio C Hamano writes: > Jeff King writes: > >> My general impression of the goal of our current code organization is: >> >> 1. builtin/*.c should each contain a single builtin command and its >> supporting static functions. Each file gets linked into git.o to >> make the "main" git executable. > > Correct; that is what we aimed for when we made builtin-*.c (later > moved to builtin/*.c). Some builtin/*.c files can contain more than > one cmd_foo implementations, so "single" is not a solid rule, and it > does not have to be, because all of them are expected to be linked > into the main binary together with git.c to be called from main(). > > And as you hinted, if some global data or functions in it turns out > to be useful for standalone binaries, their definitions must migrate > out of buitlin/*.c to ./*.c files, because standalone binaries with > their own main() definition cannot be linked with builtin/*.o, the > latter of which requires to be linked with git.o with its own main(). > ... > The rationale behind libgit.a was so that make targets for the > standalone binaries (note: all of them were standalone in the > beginning) do not have to list *.o files that each of them needs to > be linked with. It was primary done as a convenient way to have the > linker figure out the dependency and link only what was needed. For the particular case of trying to make sequencer.o, which does not currently have dependencies on builtin/*.o, depend on something that is in builtin/notes.o, the link phase of standalone that wants anything from revision.o (which is pretty much everything ;-) goes like this: upload-pack.c wants handle_revision_opt etc. revision.c provides handle_revision_opt wants name_decoration etc. log-tree.c provides name_decoration wants append_signoff sequencer.c provides append_signoff So sequencer.o _is_ meant to be usable from standalone and belongs to libgit.a If sequencer.o wants to call init_copy_notes_for_rewrite() and its friends [*1*] that are currently in builtin/notes.o, first the called function(s) should be moved outside builtin/notes.o to notes.o or somewhere more library-ish place to be included in libgit.a, which is meant to be usable from standalone. [Footnote] *1* ... which is a very reasonable thing to do. But moving sequencer.o to builtin/sequencer.o is *not* the way to do this. -- 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
R
Отправлено с iPhonegmail-- 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 v2 2/4] commit-queue: LIFO or priority queue of commits
Jeff King writes: > On Mon, Jun 10, 2013 at 11:56:33AM -0700, Junio C Hamano wrote: > >> > or similar. I didn't change the name, either. It may be silly to call it >> > "commit_queue" still since it is now more general. I simply called mine >> > "queue" (I wanted "pqueue", but that conflicted with globals defined by >> > OpenSSL; yours is a more general queue anyway, so maybe that is a good >> > name). >> >> I agree that it makes sense not to call it either commit-queue or >> pqueue. While at it, the filenames should probably be moved as >> well, no? > > Yeah, definitely. I left all of that as an exercise for you, since the > name change will involve a lot of fallout in the other patches. 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; - For now, record_author_date() does the obvious read-sha1-file and free; and - 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 *". I may have flipped the comparison < vs <= as well. 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
Jeff King writes: > My general impression of the goal of our current code organization is: > > 1. builtin/*.c should each contain a single builtin command and its > supporting static functions. Each file gets linked into git.o to > make the "main" git executable. Correct; that is what we aimed for when we made builtin-*.c (later moved to builtin/*.c). Some builtin/*.c files can contain more than one cmd_foo implementations, so "single" is not a solid rule, and it does not have to be, because all of them are expected to be linked into the main binary together with git.c to be called from main(). And as you hinted, if some global data or functions in it turns out to be useful for standalone binaries, their definitions must migrate out of buitlin/*.c to ./*.c files, because standalone binaries with their own main() definition cannot be linked with builtin/*.o, the latter of which requires to be linked with git.o with its own main(). > 2. ./*.c is one of: > >a. Shared code usable by externals and builtins, which gets > linked into libgit.a > >b. An external command itself, with its own main(). It gets > linked against libgit.a. > > 3. Functions in libgit.a should be defined in a header file specific > to their module (e.g., refs.h). cache.h picks up the slack for > things that are general, or too small to get their own header file, > or otherwise don't group well. > > I said it was a "goal", because I know that we do not follow that in > many places, so it is certainly easy to find counter-examples (and nor > do I think it cannot be changed; I am just trying to describe the > current rationale). Under that organization, there is no place for "code > that does not go into libgit.a, but is not a builtin command in itself". > There was never a need in the past, because libgit.a was a bit of a > dumping ground for linkable functions, and nobody cared that it had > everything and the kitchen sink. The rationale behind libgit.a was so that make targets for the standalone binaries (note: all of them were standalone in the beginning) do not have to list *.o files that each of them needs to be linked with. It was primary done as a convenient way to have the linker figure out the dependency and link only what was needed. -- 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 6/6] template: Fix comment indentation in pre-rebase hook
On Tue, Jun 11, 2013 at 12:57 AM, Junio C Hamano wrote: > [PATCH 1/6] templates: Fewer subprocesses in pre-commit hook > > I agree with Peff that "less fork" is a bad justification for this > change, and also > > echo 'First line > second line > third lie' > > looks somewhat bad. The repeated echo also looks bad, imo. Also, 2/6 depends on this to save lines. Should I rewrite with EOF, keep as is, or drop? > [PATCH 2/6] templates: Reformat pre-commit hook's message > > I think it is a good thing to make the output short by widening. As I said, 2/6 depends on 1/6 to some extent. > [PATCH 4/6] Documentation: Update manpage for pre-commit hook > > I debated myself if it should say "The hook _by default_ prevents > addition of non-ASCII filenames", hinting that it can be > configured out if it is unwanted. > > Other than that, I think it is a good addition. Will update once I know the complete TODO. > [PATCH 5/6] templates: Fix ASCII art in pre-rebase hook > > Good, but see Peff's comments on the explanation. OK. > [PATCH 6/6] template: Fix comment indentation in pre-rebase hook > > After reading the original once again, it is fine as-is without > the change at all, I think. Alternatively, "if these match" and > "if this is empty" lines can be flushed to the left, which also is > readable. I think I will flush and capitalize, then. Richard -- 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 6/6] template: Fix comment indentation in pre-rebase hook
Richard Hartmann writes: > On Mon, Jun 10, 2013 at 9:52 PM, Junio C Hamano wrote: > > >> I think offsetting the actual commands to the right is correct, but >> "if these match" and "if this is empty" should be flushed to left as >> this patch shows. > > I actually considered this and decided against it as it seemed to be > deliberate. Should I re-roll and re-send? > > I will gladly re-send the whole, or part of the, series once I know > which patches are OK and which need more work. [PATCH 1/6] templates: Fewer subprocesses in pre-commit hook I agree with Peff that "less fork" is a bad justification for this change, and also echo 'First line second line third lie' looks somewhat bad. [PATCH 2/6] templates: Reformat pre-commit hook's message I think it is a good thing to make the output short by widening. [PATCH 3/6] templates: Fix spelling in pre-commit hook Good. [PATCH 4/6] Documentation: Update manpage for pre-commit hook I debated myself if it should say "The hook _by default_ prevents addition of non-ASCII filenames", hinting that it can be configured out if it is unwanted. Other than that, I think it is a good addition. [PATCH 5/6] templates: Fix ASCII art in pre-rebase hook Good, but see Peff's comments on the explanation. [PATCH 6/6] template: Fix comment indentation in pre-rebase hook After reading the original once again, it is fine as-is without the change at all, I think. Alternatively, "if these match" and "if this is empty" lines can be flushed to the left, which also is readable. 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 v5 00/36] Massive improvents to rebase and cherry-pick
On Sun, Jun 9, 2013 at 3:37 PM, Felipe Contreras wrote: > > On Sun, Jun 9, 2013 at 2:24 PM, Felipe Contreras > wrote: > > Same as before, but: > > Also, remove the patches from Martin von Zweigbergk, because > apparently some people have trouble understanding that they were not > part of this series. Please try not to sound disgruntled. This attitude is toxic. You have turned this change into a complaint: that "some people have trouble understanding" which shows a genuine lack of understanding and compassion on your part. Instead you can phrase your change notes more helpfully if you make changes only when you yourself actually believe the change should be made. If you cannot do this, perhaps you can pretend. Also, remove the patches from Martin von Zweigbergk, which are not a part of this series. Or even this: Also, remove the patches from Martin von Zweigbergk to avoid confusing reviewers. Thanks, Phil -- 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 Mon, Jun 10, 2013 at 5:06 PM, Jeff King wrote: > On Mon, Jun 10, 2013 at 04:52:57PM -0500, Felipe Contreras wrote: > >> On Mon, Jun 10, 2013 at 4:45 PM, Jeff King wrote: >> >> > That is what libgit.a _is_ now. I do not mean to imply any additional >> > judgement on what it could be. But if the goal is to make libgit.a >> > "functions that programs outside git.git would want, and nothing else", >> > we may want to additionally split out a "libgit-internal.a" consisting >> > of code that is used by multiple externals in git, but which would not >> > be appropriate for clients to use. >> >> That might make sense, but that still doesn't clarify what belongs in >> ./*.o, and what belongs in ./builtin/*.o. And right now that creates a >> mess where you have code shared between ./builtin/*.o that is defined >> in cache.h (overlay_tree_on_cache), and some in builtin.h >> (init_copy_notes_for_rewrite). And it's not clear what should be done >> when code in ./*.o needs to access functionality in ./builtin/*.o, >> specially if that code is only useful for git builtins, and nothing >> else. > > My general impression of the goal of our current code organization is: > > 1. builtin/*.c should each contain a single builtin command and its > supporting static functions. Each file gets linked into git.o to > make the "main" git executable. We already know this is not the case. Maybe this should be fixed by moving all the shared code between builtins to libgit.a, but maybe we already know at some level this is not wise, and that's why we haven't done so. > If we want to start caring, then we probably need to create a separate > "kitchen sink"-like library, with the rule that things in libgit.a > cannot depend on it. In other words, a support library for Git's > commands, for the parts that are not appropriate to expose as part of a > library API. Yes, that's clearly what we should be doing, which is precisely what my patch that creates builtin/lib.a does. So we have two options: a) Do what we clearly should do; create builtin/lib.a, and move code there that is specific to builtin commands. b) Do what we think we have been doing; and move _all_ shared code to libgit.a (which shouldn't be called libgit, because it's not really a library), and cleanup builtin/*.c so they don't share anything among themselves directly. I vote for a), not only because we already know that's what we _should_ do, but because we are basically already there. Leaving things as they are is not really an option; that's a mess. -- 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
On Mon, Jun 10, 2013 at 04:52:57PM -0500, Felipe Contreras wrote: > On Mon, Jun 10, 2013 at 4:45 PM, Jeff King wrote: > > > That is what libgit.a _is_ now. I do not mean to imply any additional > > judgement on what it could be. But if the goal is to make libgit.a > > "functions that programs outside git.git would want, and nothing else", > > we may want to additionally split out a "libgit-internal.a" consisting > > of code that is used by multiple externals in git, but which would not > > be appropriate for clients to use. > > That might make sense, but that still doesn't clarify what belongs in > ./*.o, and what belongs in ./builtin/*.o. And right now that creates a > mess where you have code shared between ./builtin/*.o that is defined > in cache.h (overlay_tree_on_cache), and some in builtin.h > (init_copy_notes_for_rewrite). And it's not clear what should be done > when code in ./*.o needs to access functionality in ./builtin/*.o, > specially if that code is only useful for git builtins, and nothing > else. My general impression of the goal of our current code organization is: 1. builtin/*.c should each contain a single builtin command and its supporting static functions. Each file gets linked into git.o to make the "main" git executable. 2. ./*.c is one of: a. Shared code usable by externals and builtins, which gets linked into libgit.a b. An external command itself, with its own main(). It gets linked against libgit.a. 3. Functions in libgit.a should be defined in a header file specific to their module (e.g., refs.h). cache.h picks up the slack for things that are general, or too small to get their own header file, or otherwise don't group well. I said it was a "goal", because I know that we do not follow that in many places, so it is certainly easy to find counter-examples (and nor do I think it cannot be changed; I am just trying to describe the current rationale). Under that organization, there is no place for "code that does not go into libgit.a, but is not a builtin command in itself". There was never a need in the past, because libgit.a was a bit of a dumping ground for linkable functions, and nobody cared that it had everything and the kitchen sink. If we want to start caring, then we probably need to create a separate "kitchen sink"-like library, with the rule that things in libgit.a cannot depend on it. In other words, a support library for Git's commands, for the parts that are not appropriate to expose as part of a library API. -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: Different diff strategies in add --interactive
On Mon, Jun 10, 2013 at 10:46:38PM +0100, John Keeping wrote:
> > Overall, I think respecting diff.algorithm in add--interactive is a very
> > sane thing to do. I would even be tempted to say we should allow a few
> > other select diff options (e.g., fewer or more context lines). If you
> > allowed diff options like this:
> >
> > git add --patch="--patience -U5"
> >
> > that is very flexible, but I would not want to think about what the code
> > does when you pass --patch="--raw" or equal nonsense.
>
> An alternative would be to permit them to be set from within the
> interactive UI. I'd find it quite useful to experiment with various
> diff options when I encounter a hunk that isn't as easy to pick as I'd
> like. I expect it would be very hard to do that on a per-hunk basis,
> although per-file doesn't seem like it would be too hard.
That's an interesting idea, for a subset of options (e.g., "increase
context for this hunk"). I suspect implementing it would be painful,
though, as you would have to re-run diff, and you have no guarantee of
getting the same set of hunks (e.g., the hunk might end up coalesced
with another).
> I don't intend to investigate that though - respecting diff.algorithm is
> good enough for my usage.
I don't blame you. :)
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index d2c4ce6..0b0fac2 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -44,6 +44,8 @@ my ($diff_new_color) =
>
> my $normal_color = $repo->get_color("", "reset");
>
> +my $diff_algorithm = ($repo->config('diff.algorithm') or 'default');
> +
> my $use_readkey = 0;
> my $use_termcap = 0;
> my %term_escapes;
> @@ -731,6 +733,9 @@ sub run_git_apply {
> sub parse_diff {
> my ($path) = @_;
> my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
> + if ($diff_algorithm ne "default") {
> + push @diff_cmd, "--diff-algorithm=${diff_algorithm}";
> + }
> if (defined $patch_mode_revision) {
> push @diff_cmd, $patch_mode_revision;
Yeah, that looks like the sane way to do it to me. As a perl style
thing, I think the usual way of spelling 'default' is 'undef'. I.e.:
my $diff_algorithm = $repo->config('diff.algorithm');
...
if (defined $diff_algorithm) {
push @diff_cmd, "--diff-algorithm=$diff_algorithm";
}
-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] build: get rid of the notion of a git library
On Mon, Jun 10, 2013 at 4:45 PM, Jeff King wrote: > That is what libgit.a _is_ now. I do not mean to imply any additional > judgement on what it could be. But if the goal is to make libgit.a > "functions that programs outside git.git would want, and nothing else", > we may want to additionally split out a "libgit-internal.a" consisting > of code that is used by multiple externals in git, but which would not > be appropriate for clients to use. That might make sense, but that still doesn't clarify what belongs in ./*.o, and what belongs in ./builtin/*.o. And right now that creates a mess where you have code shared between ./builtin/*.o that is defined in cache.h (overlay_tree_on_cache), and some in builtin.h (init_copy_notes_for_rewrite). And it's not clear what should be done when code in ./*.o needs to access functionality in ./builtin/*.o, specially if that code is only useful for git builtins, and nothing else. -- 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 6/6] template: Fix comment indentation in pre-rebase hook
On Mon, Jun 10, 2013 at 9:52 PM, Junio C Hamano wrote: > I think offsetting the actual commands to the right is correct, but > "if these match" and "if this is empty" should be flushed to left as > this patch shows. I actually considered this and decided against it as it seemed to be deliberate. Should I re-roll and re-send? I will gladly re-send the whole, or part of the, series once I know which patches are OK and which need more work. Richard -- 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 3:07 PM, Ramkumar Ramachandra wrote: > Felipe Contreras wrote: >> I think that's the only way forward, since the Git project doesn't >> wish to be improved. >> >> Perhaps it's time for me to create a pseudonym, and when you have to >> do that to land clearly good patches, you know something is *REALLY* >> wrong with the project. > > I ask only for your patience, Felipe. How much time? A day? A week? A month? Not even a year would make my patches to 'git cherry-pick' and 'git rebase' be magically applied. 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? -- 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: Different diff strategies in add --interactive
On Mon, Jun 10, 2013 at 05:11:41PM -0400, Jeff King wrote:
> On Mon, Jun 10, 2013 at 12:28:55PM -0700, Junio C Hamano wrote:
> > John Keeping writes:
> >
> > > I think the first thing to do is read the "diff.algorithm" setting in
> > > git-add--interactive and pass its value to the underlying diff-index and
> > > diff-files commands, but should we also have a command line parameter to
> > > git-add to specify the diff algorithm in interactive mode? And if so,
> > > can we simply add "--diff-algorithm" to git-add, or is that too
> > > confusing?
> >
> > Making "git add--interactive" read from diff.algorithm is probably a
> > good idea, because the command itself definitely is a Porcelain. We
> > would probably need a way to defeat the configured default for
> > completeness, either:
> >
> > git add -p --diff-algorithm=default
> > git -c diff.algorithm=default add -p
> >
> > but I suspect that a new option to "git add" that only takes effect
> > together with "-p" is probably an overkill, only in order to support
> > the former and not having to say the latter, but I can be persuaded
> > either way.
>
> Worse than that, you would need to add such an option to "checkout -p",
> "reset -p", "stash -p", etc. I think the latter form you suggest is
> probably acceptable in this case.
That's what I'm planning to do at the moment, if anyone wants to extend
it further in the future then that can be built on top.
> Overall, I think respecting diff.algorithm in add--interactive is a very
> sane thing to do. I would even be tempted to say we should allow a few
> other select diff options (e.g., fewer or more context lines). If you
> allowed diff options like this:
>
> git add --patch="--patience -U5"
>
> that is very flexible, but I would not want to think about what the code
> does when you pass --patch="--raw" or equal nonsense.
An alternative would be to permit them to be set from within the
interactive UI. I'd find it quite useful to experiment with various
diff options when I encounter a hunk that isn't as easy to pick as I'd
like. I expect it would be very hard to do that on a per-hunk basis,
although per-file doesn't seem like it would be too hard.
I don't intend to investigate that though - respecting diff.algorithm is
good enough for my usage.
> But I cannot off the top of my head think of other options besides -U
> that would be helpful. I have never particularly wanted it for "add -p",
> either, though I sometimes generate patches to the list with a greater
> number of context lines when I think it makes the changes to a short
> function more readable.
--function-context might also be useful, but that's in the same category
as -U.
The patch I'm using is below. I'm not sure how we can test this though;
it seems fragile to invent a patch that appears different with different
diff algorithms. Any suggestions?
-- >8 --
git-add--interactive.perl | 5 +
1 file changed, 5 insertions(+)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index d2c4ce6..0b0fac2 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -44,6 +44,8 @@ my ($diff_new_color) =
my $normal_color = $repo->get_color("", "reset");
+my $diff_algorithm = ($repo->config('diff.algorithm') or 'default');
+
my $use_readkey = 0;
my $use_termcap = 0;
my %term_escapes;
@@ -731,6 +733,9 @@ sub run_git_apply {
sub parse_diff {
my ($path) = @_;
my @diff_cmd = split(" ", $patch_mode_flavour{DIFF});
+ if ($diff_algorithm ne "default") {
+ push @diff_cmd, "--diff-algorithm=${diff_algorithm}";
+ }
if (defined $patch_mode_revision) {
push @diff_cmd, $patch_mode_revision;
}
--
1.8.3.779.g691e267
--
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 Sun, Jun 09, 2013 at 07:30:31PM +0200, Vincent van Ravesteijn wrote: > I think that libgit.a should contain all code to be able to carry out > all functions of git. The stuff in builtin/ is just a command-line > user interface. Whether or not sequencer should be in builtin depends > on whether the sequencer is only part of this command-line user > interface. One code organization issue I have not seen mentioned is that there is more CLI than what is in builtin, and libgit.a does more than simply provide code for the sources in builtin/. There are also external commands shipped in git.git that are not linked against git.c or the other builtins. Once upon a time, all commands were that way, and that was the origin of libgit.a: the set of common code used by all of the C commands in git.git. Over time, those commands became builtins (mostly to keep the size of the libexec dir down). These days there are only a handful of external commands left, mostly ones that have startup time overhead from the dynamic loader (e.g., remote-curl, http-push, imap-send). That is what libgit.a _is_ now. I do not mean to imply any additional judgement on what it could be. But if the goal is to make libgit.a "functions that programs outside git.git would want, and nothing else", we may want to additionally split out a "libgit-internal.a" consisting of code that is used by multiple externals in git, but which would not be appropriate for clients to use. For example, I think most of "http.c" is in that boat, as it is full of wrappers for curl code that are of enough quality to reuse within git, but a little too half-baked to be part of a stable API. We can always link directly against http.o, too, of course. The point of putting the files into a static library is that it makes the link faster, and if there are only a handful of such links, it may not be worth the effort. -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] diff: add --ignore-blank-lines option
Antoine Pelisse writes: > On Sun, Jun 9, 2013 at 10:07 PM, Junio C Hamano wrote: >> When any ignore blank option is used, there will be lines that >> actually has changes (hence should be shown with +/-) but we >> deliberately ignore their changes (hence, if they ever appear in the >> hunk, they do so as context lines prefixed with SP not +/-). When >> we do so, we show the lines from the postimage in the context. > > Don't we actually use preimage (see below) ? I think using pre-image > allows the patch to be applicable to another tree (but ignoring the > space changes). But the result of such patch application is not usually what you want to use. If we use postimage (which by the way was a deliberate design decision we made earlier), at least the review of the patch is easier because you would see the end result more clearly. > If we actually hide new blank lines that are in the context, it means > that we won't be able to apply a patch with 2 new blank lines in the 3 > line context. Yes, but I do not think the point of --ignore-blank-lines is to produce a patch that can be applied in the first place. It is to allow easier eyeballing. > Anyway, I'm starting to think that "show blank lines changes near > other changes" makes sense more and more sense. Probably. -- 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 Mon, Jun 10, 2013 at 12:34 PM, Matthieu Moy wrote: > Felipe Contreras writes: > >> It is not bad behavior. It is bad behavior *in your opinion*, > > And in essentially everyone else on this list, it seems. So? An opinion shared by a billion people is still an opinion, not a fact. To think otherwise is to fall in the argumentum ad populum fallacy. >> an opinion that wouldn't be shared by other projects, like the Linux >> kernel. > > Googling your name and LKML gives me this in the first page (addressed > to you): > > https://lkml.org/lkml/2012/4/12/434 > "I'm stupider for just reading your email. Go away." > > https://lkml.org/lkml/2012/4/15/112 > "I'll make one more try at explaining to you, but then I'll just set my > mail reader to ignore you, because judging by past performance (not > just in this thread) you will just continue to argue." > > I don't follow the lkml so maybe I've just been unlucky and Google > didn't show me an accurate sample, but arguing that your behavior is > welcome on the LKML seems weird. Now you are committing two fallacies at the same time; argument from authority and hasty generalization. Yes, Linus Torvalds lost his temper with me, he has done so with so many people that's hardly surprising. I still think he is wrong, but to prove it I need information that is not readily available, and it's not that important anyway. That doesn't mean that Linus' opinion is shared by the list (or any other Linux mailing list); if you think so you are committing the hasty generalization fallacy. And if you think Linus' opinion means something is a fact you commit the argument from authority fallacy. None of this mean that my patches are not welcome in LKML, or any other Linux mailing list. I repeat what Linus said: Talk is cheap, show me the code. -- 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: Bad attitudes and problems in the Git community (was: Re: [PATCH 2/2] Move sequencer to builtin)
On Mon, Jun 10, 2013 at 1:11 PM, Martin von Zweigbergk wrote: > On Mon, Jun 10, 2013 at 9:58 AM, Felipe Contreras > wrote: >> On Mon, Jun 10, 2013 at 4:05 AM, Stefano Lattarini >> wrote: >> You need two sides to have an argument. >> >>> I disagree. Unless you mean than, whenever a part behaves in a >>> hostile and aggressive way, the other part should just silently >>> knuckle under. >> >> You are wrong. If a bum in the street starts talking about you about >> why you are going to hell, and you reply to him and argue. Who has the >> fault of starting an argument? > > I'm not sure I follow the analogy. Are you the bum or the passer-by? It doesn't matter. Both sides are at fault of an argument. -- 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 5/6] templates: Fix ASCII art in pre-rebase hook
On Mon, Jun 10, 2013 at 08:36:04PM +0200, Richard Hartmann wrote: > The example assumes 8-char wide tabs and breaks for people with > 4-char wide tabs. If you end up re-rolling, it might be worth saying "Let's just convert all of the tabs to spaces" in the commit message. I was curious what your solution was (all spaces, or consistent start-tab indentation followed by spaces), and it was somewhat hard to see in the patch since the changes were pure whitespace. :) -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 1/6] templates: Fewer subprocesses in pre-commit hook
On Mon, Jun 10, 2013 at 08:36:00PM +0200, Richard Hartmann wrote: > Spawning a new subprocess for every line printed is inefficient. > Thus spawn only one instance of `echo`. Most modern shells have "echo" as a built-in these days, and do not fork at all to run it. E.g., try "strace sh -c 'echo foo'" with your shell of choice; neither dash nor bash will fork at all. IMHO the indentation issues make the end result of your patch less readable (and here-doc with cat is more readable, but actually _increases_ the number of processes, since cat is not usually a built-in). So I'd be in favor of keeping it as-is. -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: Different diff strategies in add --interactive
On Mon, Jun 10, 2013 at 12:28:55PM -0700, Junio C Hamano wrote: > John Keeping writes: > > > I think the first thing to do is read the "diff.algorithm" setting in > > git-add--interactive and pass its value to the underlying diff-index and > > diff-files commands, but should we also have a command line parameter to > > git-add to specify the diff algorithm in interactive mode? And if so, > > can we simply add "--diff-algorithm" to git-add, or is that too > > confusing? > > Making "git add--interactive" read from diff.algorithm is probably a > good idea, because the command itself definitely is a Porcelain. We > would probably need a way to defeat the configured default for > completeness, either: > > git add -p --diff-algorithm=default > git -c diff.algorithm=default add -p > > but I suspect that a new option to "git add" that only takes effect > together with "-p" is probably an overkill, only in order to support > the former and not having to say the latter, but I can be persuaded > either way. Worse than that, you would need to add such an option to "checkout -p", "reset -p", "stash -p", etc. I think the latter form you suggest is probably acceptable in this case. Overall, I think respecting diff.algorithm in add--interactive is a very sane thing to do. I would even be tempted to say we should allow a few other select diff options (e.g., fewer or more context lines). If you allowed diff options like this: git add --patch="--patience -U5" that is very flexible, but I would not want to think about what the code does when you pass --patch="--raw" or equal nonsense. But I cannot off the top of my head think of other options besides -U that would be helpful. I have never particularly wanted it for "add -p", either, though I sometimes generate patches to the list with a greater number of context lines when I think it makes the changes to a short function more readable. -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] Documentation/CommunityGuidelines
On 06/10/2013 04:56 PM, Ramkumar Ramachandra wrote: A Large Angry SCM wrote: It is absolutely imperative to keep all our contributors productive, and maximize output. Why? A useful "product" with a maintainable code base are what seems to be more important to a successful open source effort. Doesn't a successful open source effort (with a good review process, which we already have) imply a maintainable product with lots of users? What am I missing, and what change do you propose? It's not about keeping all of the contributers productive or maximizing output. It's about the result being useful. -- 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
[ANNOUNCE] Git v1.8.3.1
The latest maintenance release Git v1.8.3.1 is now available at the usual places. This is primarily to push out fixes to two regressions that seem to affect many people, namely, the ".gitignore !directory" bug and the "daemon cannot read from $HOME owned by root" bug. The release tarballs are found at: http://code.google.com/p/git-core/downloads/list and their SHA-1 checksums are: 32562a231fe4422bc033bf872fffa61f41ee2669 git-1.8.3.1.tar.gz 94d48f6f8684aec851124e7d0b835b338a9187ad git-htmldocs-1.8.3.1.tar.gz 0cd759579d4bd75f1cf1ba073b1ab96c49390426 git-manpages-1.8.3.1.tar.gz The following public repositories all have a copy of the v1.8.3.1 tag and the maint branch that the tag points at: url = https://kernel.googlesource.com/pub/scm/git/git url = git://repo.or.cz/alt-git.git url = https://code.google.com/p/git-core/ url = git://git.sourceforge.jp/gitroot/git-core/git.git url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core url = https://github.com/gitster/git Also, http://www.kernel.org/pub/software/scm/git/ has copies of the release tarballs. Git v1.8.3.1 Release Notes Fixes since v1.8.3 -- * When $HOME is misconfigured to point at an unreadable directory, we used to complain and die. The check has been loosened. * Handling of negative exclude pattern for directories "!dir" was broken in the update to v1.8.3. Also contains a handful of trivial code clean-ups, documentation updates, updates to the test suite, etc. -- 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] diff: add --ignore-blank-lines option
On Sun, Jun 9, 2013 at 10:07 PM, Junio C Hamano wrote: > When any ignore blank option is used, there will be lines that > actually has changes (hence should be shown with +/-) but we > deliberately ignore their changes (hence, if they ever appear in the > hunk, they do so as context lines prefixed with SP not +/-). When > we do so, we show the lines from the postimage in the context. Don't we actually use preimage (see below) ? I think using pre-image allows the patch to be applicable to another tree (but ignoring the space changes). If we actually hide new blank lines that are in the context, it means that we won't be able to apply a patch with 2 new blank lines in the 3 line context. Anyway, I'm starting to think that "show blank lines changes near other changes" makes sense more and more sense. By the way I have a patch I *think* is working, but I will check it another thousand times before sending. Cheers, Antoine $ git diff diff --git a/x b/x index e562137..226e35a 100644 --- a/x +++ b/x @@ -4,8 +4,9 @@ change 3 4 5 -6 -7 -8 -9 + 6 +7 +change + 8 +9 10 $ git diff -w diff --git a/x b/x index e562137..226e35a 100644 --- a/x +++ b/x @@ -6,6 +6,7 @@ change 5 6 7 +change 8 9 10 -- 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
A Large Angry SCM wrote: >> It is absolutely imperative to keep all our contributors productive, >> and maximize output. > > > Why? > > A useful "product" with a maintainable code base are what seems to be more > important to a successful open source effort. Doesn't a successful open source effort (with a good review process, which we already have) imply a maintainable product with lots of users? What am I missing, and what change do you propose? -- 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 1/2] rm: better error message on failure for multiple files
Matthieu Moy writes:
> Junio C Hamano writes:
>
>>> +{
>>> + 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",
>>
>> Is there an implication of having always 4 spaces here to l10n/i18n
>> here? I am wondering if it should be _("\n%s").
>
> I'd say this is just formatting and should be the same in every
> languages, but I'm far from an expert in the domain.
After looking at the patch again I do not think 4-SP matters. I was
primarily worried if this was to align with some column of the first
line of output, e.g.
error: lorem ipsum dolor sit amet, consectetur adipisicing
elit, sed do eiusmod tempor incididunt ut labore et
dolore magna aliqua.
but that is not what this 4-SP indent is about, so it is OK.
>> test_expect_success 'rm files with different staged content' '
>> cat >expect <<\-EOF &&
>
> (that should be -\EOF, not \-EOF I think)
Sorry, my bad. You are of course right.
>> (2) by using a dash '-' before the end-of-here-text marker, you can
>> align the body of here text with a leading tab (HT).
>
> This works because the list of files is aligned with spaces, but is
> seems a bit fragile to me to use this -EOF on a text which uses
> indentation. Anyway, I'm fine with both.
True.
--
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 1/2] config: refactor management of color.ui's default value
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] mingw: make mingw_signal return the correct handler
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] completion: remove ancient "ensure colon in COMP_WORDBREAKS" workaround
SZEDER Gábor writes: > Anyway. Although the completion script in Fedora's gvfs package might > be fixed, there are other completion scripts making the same mistake, > so I'm afraid we should keep the workaround and drop this patch. > Moreover, we should even consider extending our workaround by adding > back '=' to COMP_WORDBREAKS, too. Oh, well. Thanks. Perhaps the summary of your problem analysis deserves to be added as a in-code comment to make sure others won't later try to remove it only by checking how old Fedora 9 is? -- 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/10/2013 03:45 PM, Ramkumar Ramachandra wrote: [...] It is absolutely imperative to keep all our contributors productive, and maximize output. Why? A useful "product" with a maintainable code base are what seems to be more important to a successful open source effort. A Large Angry SCM -- 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/3] test: improve rebase -q test
On Sun, Jun 09, 2013 at 11:30:01AM -0700, Junio C Hamano wrote: > -- >8 -- > Subject: [PATCH] test: test_output_must_be_empty helper > > There are quite a lot places where an output file is expected to be > empty, and we fail the test when it is not. The output from running > the test script with -i -v can be helped if we showed the unexpected > contents at that point. > > We could of course do > > >expected.empty && test_cmp expected.empty actual > > but this is commmon enough to be done with a dedicated helper. Thanks, I think this improves the readability of the test suite (and its output when there are failures). You can also do: test_cmp /dev/null actual for the same effect, but I guess the diff is not all that interesting (by definition, it would consist only of added lines, and you are already showing them, so it would not be an improvement). -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 1/6] templates: Fewer subprocesses in pre-commit hook
Hi Junio, if you want a new patch, just say the word. Richard -- 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/3] test: improve rebase -q test
On Mon, Jun 10, 2013 at 09:07:06PM +0200, Johannes Sixt wrote: > Am 10.06.2013 19:27, schrieb SZEDER Gábor: > > My main motivation is that, like in your example, in the bash prompt > > tests I only have to check a single line of output, but because of > > debuggability I always did: > > > > echo "(master)" >expected > > __git_ps1 >actual > > test_cmp expected actual > > Chained by &&, I presume. Sure. > > With such a helper function this could be reduced to a single line: > > > > test_string_equal "(master)" "$(__git_ps1)" > > > > without loss of functionality > > Not quite: A non-zero exit code of the $(__git_ps1) is lost. (It > probably doesn't matter here, but it certainly does if the command is > $(git rev-parse foo) or similar.) Ouch, indeed. Yeah, the exit code doesn't matter for the prompt tests (I mean for __git_ps1() tests, but maybe it does matter for some __gitdir() tests), but I suppose it does matter everywhere else where the same construct is used. We could still do actual="$(git foo)" && test_string_equal "good" "$actual" to preserve and check the exit code, and this is still one line shorter, but overall not that appealing anymore. However. The git command's exit code is lost the same way in 'test good = $(git foo)' constructs, too, and plenty of such constructs are all over the test suite. Shouldn't we avoid using such constucts then? Gábor -- 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
Felipe Contreras wrote: > I think that's the only way forward, since the Git project doesn't > wish to be improved. > > Perhaps it's time for me to create a pseudonym, and when you have to > do that to land clearly good patches, you know something is *REALLY* > wrong with the project. I ask only for your patience, Felipe. Let's give it a few days to calm down, and discuss things rationally with people. The project does have some mental blocks, but it is not an insurmountable problem. -- 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 6/6] template: Fix comment indentation in pre-rebase hook
Richard Hartmann writes: > The other hooks use two whitespace for indentation instead of tabs > to signify code in the example/echo output. > Follow the same layout in templates/hooks--pre-rebase.sample > > Signed-off-by: Richard Hartmann > --- > templates/hooks--pre-rebase.sample | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/templates/hooks--pre-rebase.sample > b/templates/hooks--pre-rebase.sample > index b74cd1d..43426e0 100755 > --- a/templates/hooks--pre-rebase.sample > +++ b/templates/hooks--pre-rebase.sample > @@ -157,13 +157,13 @@ B to be deleted. > > To compute (1): > > - git rev-list ^master ^topic next > - git rev-list ^masternext > + git rev-list ^master ^topic next > + git rev-list ^masternext > > - if these match, topic has not merged in next at all. > + if these match, topic has not merged in next at all. > > To compute (2): > > - git rev-list master..topic > + git rev-list master..topic > > - if this is empty, it is fully merged to "master". > + if this is empty, it is fully merged to "master". I think offsetting the actual commands to the right is correct, but "if these match" and "if this is empty" should be flushed to left as this patch shows. -- 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 5/6] templates: Fix ASCII art in pre-rebase hook
Richard Hartmann writes: > The example assumes 8-char wide tabs and breaks for people with > 4-char wide tabs. Even though as far as this project is concerned, a tab stop is every 8 columns, this is for consumption by end-users who use Git, not for people who want to improve the code in Git (which this file is part of), so this "untabify" may make sense. Thanks. > Signed-off-by: Richard Hartmann > --- > templates/hooks--pre-rebase.sample | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/templates/hooks--pre-rebase.sample > b/templates/hooks--pre-rebase.sample > index 053f111..b74cd1d 100755 > --- a/templates/hooks--pre-rebase.sample > +++ b/templates/hooks--pre-rebase.sample > @@ -132,14 +132,14 @@ With this workflow, you would want to know: > > Let's look at this example: > > -o---o---o---o---o---o---o---o---o---o "next" > - / / / / > - / a---a---b A / / > - / / / / > -/ / c---c---c---c B / > - / / / \ / > - / / / b---b C \ / > - / / / / \ / > + o---o---o---o---o---o---o---o---o---o "next" > + / / / / > + / a---a---b A / / > +/ / / / > + / / c---c---c---c B / > + / / / \ / > + / / / b---b C \ / > +/ / / / \ / > ---o---o---o---o---o---o---o---o---o---o---o "master" -- 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/6] templates: Reformat pre-commit hook's message
Richard Hartmann writes: > Now that the there's only one echo being spawned, the message can span > the full 80 chars. > > Signed-off-by: Richard Hartmann > --- > templates/hooks--pre-commit.sample |6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/templates/hooks--pre-commit.sample > b/templates/hooks--pre-commit.sample > index 126ae13..7676c6e 100755 > --- a/templates/hooks--pre-commit.sample > +++ b/templates/hooks--pre-commit.sample > @@ -33,13 +33,11 @@ if [ "$allownonascii" != "true" ] && > then > echo 'Error: Attempt to add a non-ascii file name. > > -This can cause problems if you want to work > -with people on other platforms. > +This can cause problems if you want to work with people on other platforms. > > To be portable it is advisable to rename the file. > > -If you know what you are doing you can disable this > -check using: > +If you know what you are doing you can disable this check using: > >git config hooks.allownonascii true > ' OK. Occupying 75-col feels like it is pushing a bit, but the result does look more readable. -- 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
Jonathan Nieder wrote: > I don't think most bystanders would misunderstand if I let a certain > person alone instead of responding and saying "You are being > unproductive. Please stop." But that certain person seems to > misunderstand, whether I say that or not. So when I lose patience I > say so, knowing that it will spark a discussion with others, knowing > that that discussion needs to happen and that if the problem is not > addressed I will continue to lose motivation for regular work on-list. > > Is that an instance of taking offense and letting emotion overtake > reason? Is that against the rules? The problem needs to be addressed, Jonathan. Which is precisely why I wrote this patch: to calmly and rationally discuss the issue, and dampen the chances of repetition. You do not do it by losing your patience, becoming emotional, and fueling a large ongoing fire. Prolonging fires do not help prevent them from recurring, as evidenced by previous fires; this is because there is no takeaway from a fire. All that's left are a few shreds and ashes. From this very fire, we gained NOTHING, and lost Duy. It is absolutely imperative to keep all our contributors productive, and maximize output. If there is something troubling you, this is the right thread to speak on. -- 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 1/6] templates: Fewer subprocesses in pre-commit hook
Richard Hartmann writes: > Spawning a new subprocess for every line printed is inefficient. > Thus spawn only one instance of `echo`. > > Signed-off-by: Richard Hartmann > --- > templates/hooks--pre-commit.sample | 24 > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/templates/hooks--pre-commit.sample > b/templates/hooks--pre-commit.sample > index 18c4829..126ae13 100755 > --- a/templates/hooks--pre-commit.sample > +++ b/templates/hooks--pre-commit.sample > @@ -31,18 +31,18 @@ if [ "$allownonascii" != "true" ] && > test $(git diff --cached --name-only --diff-filter=A -z $against | > LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0 > then > - echo "Error: Attempt to add a non-ascii file name." > - echo > - echo "This can cause problems if you want to work" > - echo "with people on other platforms." > - echo > - echo "To be portable it is advisable to rename the file ..." > - echo > - echo "If you know what you are doing you can disable this" > - echo "check using:" > - echo > - echo " git config hooks.allownonascii true" > - echo > + echo 'Error: Attempt to add a non-ascii file name. > + > +This can cause problems if you want to work > +with people on other platforms. > + > +To be portable it is advisable to rename the file. > + > +If you know what you are doing you can disable this > +check using: > + > + git config hooks.allownonascii true > +' > exit 1 > fi Thanks. Writing it as a single here-text cat <<-EOF Error: Attempt to... the message body that is multi-line EOF might make it easier for people who want to activate and customize the message, but honestly this is a borderline "Meh" at least to me. Will take a look at other patches first before further commenting on this. -- 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: Different diff strategies in add --interactive
John Keeping writes: > I think the first thing to do is read the "diff.algorithm" setting in > git-add--interactive and pass its value to the underlying diff-index and > diff-files commands, but should we also have a command line parameter to > git-add to specify the diff algorithm in interactive mode? And if so, > can we simply add "--diff-algorithm" to git-add, or is that too > confusing? Making "git add--interactive" read from diff.algorithm is probably a good idea, because the command itself definitely is a Porcelain. We would probably need a way to defeat the configured default for completeness, either: git add -p --diff-algorithm=default git -c diff.algorithm=default add -p but I suspect that a new option to "git add" that only takes effect together with "-p" is probably an overkill, only in order to support the former and not having to say the latter, but I can be persuaded either way. As long as "git add --diff-algorithm=foo" without "-i" or "-p" option lets the user know it has no effect (error out, give warning and continue, etc. whose details I do not deeply care), that is. -- 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/3] test: improve rebase -q test
SZEDER Gábor writes: > With such a helper function this could be reduced to a single line: > > test_string_equal "(master)" "$(__git_ps1)" > > without loss of functionality or debuggability, because in case of a > failure it would output something like this (bikesheddable, of > course): > > Error: > expected: "(master)" > got: "((deadbeef...))" Yeah, that looks sensible. -- 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/4] push: make upstream, simple work with pushdefault
Ramkumar Ramachandra writes:
>> I am not saying that you have to pick one to use for push.default
>> among the remaining ones (i.e. matching, current, what else?). It
>> is very plausible that the triangular workflow wants a different
>> logic to pick what branches are to be updated and how. Perhaps we
>> would want something that is capable of mapping your local branch
>> name to a branch name suitable in your publishing repository, and I
>> am not opposed to have such a mode.
>
> Okay, we'll have to do some sort of split and mark push.default =
> upstream/ simple suitable-only-for-centralized-workflows, or something
> to that effect (deprecation?) :|
Among the current ones, I think "upstream" is the only one that has
the "branch I fetch and integrate with is the one I want to update
with my result" connotation. The "current" and "matching" modes
determine what gets pushed solely between the local repository you
are pushing from and the remote repository you are pushing to,
without getting "what do I fetch and integrate with" in the
equation.
As an extension to "upstream", the current implementation of
"simple" of course has the same issue, but because the name "simple"
does not inherently have such "branch I fetch and integrate with is
the one I want to update with my result" connotation, we can clean
up its semantics to match the new reality after triangular workflow.
If you recall the earlier discussion on "@{publish} which is
different from @{upstream}", one idea to allow mapping on the push
end was to introduce "push.default = single" that would act as
"upstream" when in "branch I fetch and integrate with is the same
branch at the same repository the one I want to update with my
result" workflow, and in a triangular workflow maps the branch being
pushed using remote.$name.push refspecs (if exists).
I think extending it further to act as 'current' if no push refspecs
are set for the remote you push to in a triangular workflow might be
a way to go if we want the mapping flexibility without having to set
branch.$name.push to each and every branch.
--
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/3] test: improve rebase -q test
Am 10.06.2013 19:27, schrieb SZEDER Gábor: > My main motivation is that, like in your example, in the bash prompt > tests I only have to check a single line of output, but because of > debuggability I always did: > > echo "(master)" >expected > __git_ps1 >actual > test_cmp expected actual Chained by &&, I presume. > With such a helper function this could be reduced to a single line: > > test_string_equal "(master)" "$(__git_ps1)" > > without loss of functionality Not quite: A non-zero exit code of the $(__git_ps1) is lost. (It probably doesn't matter here, but it certainly does if the command is $(git rev-parse foo) or similar.) -- 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
Junio C Hamano wrote: > 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. [...] > 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". > > I dunno. Actually my motivation is worse than that in at least one of the cases I am assuming Ram is referring to. I don't think most bystanders would misunderstand if I let a certain person alone instead of responding and saying "You are being unproductive. Please stop." But that certain person seems to misunderstand, whether I say that or not. So when I lose patience I say so, knowing that it will spark a discussion with others, knowing that that discussion needs to happen and that if the problem is not addressed I will continue to lose motivation for regular work on-list. Is that an instance of taking offense and letting emotion overtake reason? Is that against the rules? Jonathan -- 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 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Matthieu Moy writes: > Please, don't top-post. Quote the part of the message you're replying > to, and reply below. > > Benoît Person writes: > >> Well, I think next step would be to replace all those calls with >> Git.pm `command`, `command_oneline` and `config``which take an array >> of arguments after the "command". In the preview tool we use those but >> I don't know if we will find the time to clean that up too in >> git-remote-mediawiki :) . > > Agreed. run_git was written to avoid having to depend on Git.pm, but now > that we do, we should do it the Git.pm way (although this is not a > high priority for now). > >> Don't know though if it's better to mix that with this serie which is >> mainly based on what perlcritic returns. > > If you go this way, I'd rather have it on top (i.e. a separate patch > series). Or not worry too much about it in the 3-week long school project. Finish one that you started and then build on top. -- 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 v2 2/4] commit-queue: LIFO or priority queue of commits
On Mon, Jun 10, 2013 at 11:56:33AM -0700, Junio C Hamano wrote: > > or similar. I didn't change the name, either. It may be silly to call it > > "commit_queue" still since it is now more general. I simply called mine > > "queue" (I wanted "pqueue", but that conflicted with globals defined by > > OpenSSL; yours is a more general queue anyway, so maybe that is a good > > name). > > I agree that it makes sense not to call it either commit-queue or > pqueue. While at it, the filenames should probably be moved as > well, no? Yeah, definitely. I left all of that as an exercise for you, since the name change will involve a lot of fallout in the other patches. -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 v2 2/4] commit-queue: LIFO or priority queue of commits
Jeff King writes: > On Mon, Jun 10, 2013 at 12:21:00AM -0700, Junio C Hamano wrote: > >> > It may be worth looking again for other places to use this over >> > commit_list, but even the caller you are introducing here justifies its >> > presence. >> >> The next candidate is paint-down-to-common, probably. > > Yeah, I don't think I looked at that at all last time (mostly because it > only large as the graph gets wide, which is typically acceptable for > us). But it should be easy to do. > >> > Also, I wrote some basic tests to cover the priority queue as a unit. I >> > can rebase them on your commit if you are interested. >> >> It would be great. > > Squashable patch is below. > >> > Is it worth making this "struct commit *" a void pointer, and handling >> > arbitrary items in our priority queue? The compare function should be >> > the only thing that dereferences them. >> > >> > I do not have any non-commit priority queue use in mind, but I do not >> > think it adds any complexity in this case. >> >> I didn't either (and still I don't think of one), but I agree that >> the implementation can be reused for pq of any type, as long as it >> is a pointer to struct. > > I converted this to a void pointer in my patch below, simply because it > makes it easier to write a test-queue that operates on ints. Due to > implicit casting, it should work for the most part without changing the > calling code unless you have a caller that does something like: > > commit_queue_get(&q)->date > > or similar. I didn't change the name, either. It may be silly to call it > "commit_queue" still since it is now more general. I simply called mine > "queue" (I wanted "pqueue", but that conflicted with globals defined by > OpenSSL; yours is a more general queue anyway, so maybe that is a good > name). I agree that it makes sense not to call it either commit-queue or pqueue. While at it, the filenames should probably be moved as well, no? > Here's the patch with the tests, meant to be squashed into your 2/4. As > I mentioned above, you may want to further tweak the name, which would > require fixing up the rebase patches on top. > > If you don't want to do the "s/struct commit/void/" change now, we can > probably just have test-queue stuff the ints into commit pointers. > > The tests themselves are not extremely extensive, but at least let you > check that you implemented the heap correctly. :) 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 v2 4/4] log: --author-date-order
On Mon, Jun 10, 2013 at 12:39:24AM -0700, Junio C Hamano wrote:
> > I'm not excited about introducing yet another place that parses commit
> > objects (mostly not for correctness, but because we have had
> > inconsistency in how malformed objects are treated). It is at least
> > using split_ident_line which covers the hard bits. I wonder how much
> > slower it would be to simply call format_commit_message to do the
> > parsing.
>
> The thought certainly crossed my mind, not exactly in that form but
> more about splitting the machinery used in pretty.c into a more
> reusable form.
>
> The result of my attempt however did not become all that reusable
> (admittedly I didn't spend too much brain cycles on it), so I punted
> ;-).
Yes, I feel like it has been tried before. The problem is that a clean
interface would let you get individual pieces of information with a
single call. But an efficient interface will utilize the same parsing
pass to get multiple items out, and stop parsing when we have gotten all
required items (but leave the parser in a consistent state so that we
can pick it up later).
The format_commit_one parser does that, but the "format_commit_context"
it holds is a bit bulky. I think it might be possible to pull out the
parsing bits into a separate struct, and you could call it something
like:
struct commit_parser parser;
unsigned long authordate;
const char *authorname;
int authorlen;
commit_parser_init(&parser, commit);
authordate = commit_parse_authordate(&parser);
authorname = commit_parse_authorname(&parser, &authorlen);
where the second parse call is basically "free", because we've already
done (and cached) the hard work in the first call.
So they might look like:
static void parse_author_ident(struct commit_parser *parser)
{
if (!parser->author.name_begin) {
if (!parser->authorline.start)
parse_commit_header(parser);
split_ident_line(&parser->author,
parser->authorline.start,
parser->authorline.len);
}
}
unsigned long commit_parse_authordate(struct commit_parser *parser)
{
parse_author_ident(parser);
/* XXX should check for malformedness here */
return strtoul(ident.date_begin, NULL, 10);
}
const char *commit_parse_authorname(struct commit_parser *parser,
unsigned long *len)
{
parse_author_ident(parser);
*len = parser.author.name_end - parser.author.name_begin;
return parser.author.name_begin;
}
and so forth. It would be easy (and have the same efficiency) for
format_commit_message to build on that, and it calling it from regular
code is not too bad.
> But you are right. The commit->buffer may no longer be there, and
> the --author-date-order option needs to read the object again
> in this codepath. That would be in line with what --pretty/format
> would do, I guess.
>
> Or we could extend parse_commit() API to take an optional commit
> info slab to store not just author date but other non-essential
> stuff like people's names, and we arrange that extended API to be
> triggered when we know --author-date-order is in effect?
I like the latter option. It takes a non-trivial amount of time to load
the commits from disk, and now we are potentially doing it 2 or 3 times
for a run (once to parse, once to get the author info for topo-sort, and
possibly later to show it if --pretty is given; though I did not check
and maybe we turn off save_commit_buffer with --pretty). It would be
nice to have an extended parse_object that handled that. I'm not sure of
the interface. Maybe variadic with pairs of type/slab, like:
parse_commit_extended(commit,
PARSE_COMMIT_AUTHORDATE, &authordate_slab,
PARSE_COMMIT_DONE);
?
-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: Bad attitudes and problems in the Git community (was: Re: [PATCH 2/2] Move sequencer to builtin)
Yes, sorry. I find this whole story quite amusing (albeit distracting and unnecessary), but sorry for adding to the spam. I'll be quiet now. On Mon, Jun 10, 2013 at 11:33 AM, Martin Langhoff wrote: > On Mon, Jun 10, 2013 at 2:11 PM, Martin von Zweigbergk > wrote: >> On Mon, Jun 10, 2013 at 9:58 AM, Felipe Contreras >> wrote: >>> On Mon, Jun 10, 2013 at 4:05 AM, Stefano Lattarini >>> wrote: >>> > You need two sides to have an argument. >>> I disagree. Unless you mean than, whenever a part behaves in a hostile and aggressive way, the other part should just silently knuckle under. >>> >>> You are wrong. If a bum in the street starts talking about you about >>> why you are going to hell, and you reply to him and argue. Who has the >>> fault of starting an argument? >> >> I'm not sure I follow the analogy. Are you the bum or the passer-by? > > http://xkcd.com/386/ > > Someone is wrong on the Internet! > > Let it be. > > > m > -- > martin.langh...@gmail.com > - ask interesting questions > - don't get distracted with shiny stuff - working code first > ~ http://docs.moodle.org/en/User:Martin_Langhoff -- 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 3/3] rebase -i: write better reflog messages for start
Junio C Hamano wrote: > I am curious what breaks, though. t/status-help. Looks seriously unrelated, and I'm breaking my head over it. Any clues? --- expected2013-06-10 17:16:42.276356867 + +++ actual 2013-06-10 17:16:42.279690201 + @@ -1,4 +1,4 @@ -# HEAD detached at 000106f +# HEAD detached from 88a81b6 # You are currently rebasing branch 'rebase_conflicts' on '000106f'. # (fix conflicts and then run "git rebase --continue") # (use "git rebase --skip" to skip this patch) not ok 5 - status when rebase in progress before resolving conflicts -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/4] Janitorial work on hook templates
Dear all, attached, you will find a series of small, obvious, and hopefully uncontroversial patches for the hook templates and the manpage. They are all tiny, but I still decided to submit distinct patches to make modification/discussion easier. Richard Hartmann (6): templates: Fewer subprocesses in pre-commit hook templates: Reformat pre-commit hook's message templates: Fix spelling in pre-commit hook Documentation: Update manpage for pre-commit hook templates: Fix ASCII art in pre-rebase hook template: Fix comment indentation in pre-rebase hook Documentation/githooks.txt |3 ++- templates/hooks--pre-commit.sample | 26 -- templates/hooks--pre-rebase.sample | 26 +- 3 files changed, 27 insertions(+), 28 deletions(-) -- 1.7.10.4 -- 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 5/6] templates: Fix ASCII art in pre-rebase hook
The example assumes 8-char wide tabs and breaks for people with 4-char wide tabs. Signed-off-by: Richard Hartmann --- templates/hooks--pre-rebase.sample | 16 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/templates/hooks--pre-rebase.sample b/templates/hooks--pre-rebase.sample index 053f111..b74cd1d 100755 --- a/templates/hooks--pre-rebase.sample +++ b/templates/hooks--pre-rebase.sample @@ -132,14 +132,14 @@ With this workflow, you would want to know: Let's look at this example: - o---o---o---o---o---o---o---o---o---o "next" - / / / / -/ a---a---b A / / - / / / / - / / c---c---c---c B / - / / / \ / -/ / / b---b C \ / - / / / / \ / + o---o---o---o---o---o---o---o---o---o "next" + / / / / + / a---a---b A / / +/ / / / + / / c---c---c---c B / + / / / \ / + / / / b---b C \ / +/ / / / \ / ---o---o---o---o---o---o---o---o---o---o---o "master" -- 1.7.10.4 -- 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 3/6] templates: Fix spelling in pre-commit hook
Signed-off-by: Richard Hartmann --- templates/hooks--pre-commit.sample |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample index 7676c6e..a982d99 100755 --- a/templates/hooks--pre-commit.sample +++ b/templates/hooks--pre-commit.sample @@ -15,13 +15,13 @@ else against=4b825dc642cb6eb9a060e54bf8d69288fbee4904 fi -# If you want to allow non-ascii filenames set this variable to true. +# If you want to allow non-ASCII filenames set this variable to true. allownonascii=$(git config hooks.allownonascii) # Redirect output to stderr. exec 1>&2 -# Cross platform projects tend to avoid non-ascii filenames; prevent +# Cross platform projects tend to avoid non-ASCII filenames; prevent # them from being added to the repository. We exploit the fact that the # printable range starts at the space character and ends with tilde. if [ "$allownonascii" != "true" ] && @@ -31,7 +31,7 @@ if [ "$allownonascii" != "true" ] && test $(git diff --cached --name-only --diff-filter=A -z $against | LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0 then - echo 'Error: Attempt to add a non-ascii file name. + echo 'Error: Attempt to add a non-ASCII file name. This can cause problems if you want to work with people on other platforms. -- 1.7.10.4 -- 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/6] templates: Fewer subprocesses in pre-commit hook
Spawning a new subprocess for every line printed is inefficient. Thus spawn only one instance of `echo`. Signed-off-by: Richard Hartmann --- templates/hooks--pre-commit.sample | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample index 18c4829..126ae13 100755 --- a/templates/hooks--pre-commit.sample +++ b/templates/hooks--pre-commit.sample @@ -31,18 +31,18 @@ if [ "$allownonascii" != "true" ] && test $(git diff --cached --name-only --diff-filter=A -z $against | LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0 then - echo "Error: Attempt to add a non-ascii file name." - echo - echo "This can cause problems if you want to work" - echo "with people on other platforms." - echo - echo "To be portable it is advisable to rename the file ..." - echo - echo "If you know what you are doing you can disable this" - echo "check using:" - echo - echo " git config hooks.allownonascii true" - echo + echo 'Error: Attempt to add a non-ascii file name. + +This can cause problems if you want to work +with people on other platforms. + +To be portable it is advisable to rename the file. + +If you know what you are doing you can disable this +check using: + + git config hooks.allownonascii true +' exit 1 fi -- 1.7.10.4 -- 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 2/6] templates: Reformat pre-commit hook's message
Now that the there's only one echo being spawned, the message can span the full 80 chars. Signed-off-by: Richard Hartmann --- templates/hooks--pre-commit.sample |6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample index 126ae13..7676c6e 100755 --- a/templates/hooks--pre-commit.sample +++ b/templates/hooks--pre-commit.sample @@ -33,13 +33,11 @@ if [ "$allownonascii" != "true" ] && then echo 'Error: Attempt to add a non-ascii file name. -This can cause problems if you want to work -with people on other platforms. +This can cause problems if you want to work with people on other platforms. To be portable it is advisable to rename the file. -If you know what you are doing you can disable this -check using: +If you know what you are doing you can disable this check using: git config hooks.allownonascii true ' -- 1.7.10.4 -- 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 6/6] template: Fix comment indentation in pre-rebase hook
The other hooks use two whitespace for indentation instead of tabs to signify code in the example/echo output. Follow the same layout in templates/hooks--pre-rebase.sample Signed-off-by: Richard Hartmann --- templates/hooks--pre-rebase.sample | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/templates/hooks--pre-rebase.sample b/templates/hooks--pre-rebase.sample index b74cd1d..43426e0 100755 --- a/templates/hooks--pre-rebase.sample +++ b/templates/hooks--pre-rebase.sample @@ -157,13 +157,13 @@ B to be deleted. To compute (1): - git rev-list ^master ^topic next - git rev-list ^masternext + git rev-list ^master ^topic next + git rev-list ^masternext - if these match, topic has not merged in next at all. + if these match, topic has not merged in next at all. To compute (2): - git rev-list master..topic + git rev-list master..topic - if this is empty, it is fully merged to "master". + if this is empty, it is fully merged to "master". -- 1.7.10.4 -- 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 4/6] Documentation: Update manpage for pre-commit hook
Signed-off-by: Richard Hartmann --- Documentation/githooks.txt |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index b9003fe..1276730 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -80,7 +80,8 @@ causes the 'git commit' to abort. The default 'pre-commit' hook, when enabled, catches introduction of lines with trailing whitespaces and aborts the commit when -such a line is found. +such a line is found. It will also prevent addition of non-ASCII +file names. All the 'git commit' hooks are invoked with the environment variable `GIT_EDITOR=:` if the command will not bring up an editor -- 1.7.10.4 -- 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: Bad attitudes and problems in the Git community (was: Re: [PATCH 2/2] Move sequencer to builtin)
On Mon, Jun 10, 2013 at 2:11 PM, Martin von Zweigbergk wrote: > On Mon, Jun 10, 2013 at 9:58 AM, Felipe Contreras > wrote: >> On Mon, Jun 10, 2013 at 4:05 AM, Stefano Lattarini >> wrote: >> You need two sides to have an argument. >> >>> I disagree. Unless you mean than, whenever a part behaves in a >>> hostile and aggressive way, the other part should just silently >>> knuckle under. >> >> You are wrong. If a bum in the street starts talking about you about >> why you are going to hell, and you reply to him and argue. Who has the >> fault of starting an argument? > > I'm not sure I follow the analogy. Are you the bum or the passer-by? http://xkcd.com/386/ Someone is wrong on the Internet! Let it be. m -- martin.langh...@gmail.com - ask interesting questions - don't get distracted with shiny stuff - working code first ~ http://docs.moodle.org/en/User:Martin_Langhoff -- 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 3/3] rebase -i: write better reflog messages for start
Ramkumar Ramachandra writes: > Invoking 'git rebase -i' writes the following line to the reflog at the > start of the operation: > > rebase -i (start) > > This is not very useful. Make it more informative like: > > rebase -i (start): checkout master Makes sense to me, at least within the scope of the patch context. I am curious what breaks, though. > Signed-off-by: Ramkumar Ramachandra > --- > git-rebase--interactive.sh | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > index 5822b2c..a05a6e4 100644 > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh > @@ -837,6 +837,7 @@ comment_for_reflog start > > if test ! -z "$switch_to" > then > + GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" > output git checkout "$switch_to" -- || > die "Could not checkout $switch_to" > fi > @@ -980,6 +981,7 @@ has_action "$todo" || > > test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks > > +GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" > output git checkout $onto || die_abort "could not detach HEAD" > git update-ref ORIG_HEAD $orig_head > do_rest -- 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/3] checkout: respect GIT_REFLOG_ACTION
Ramkumar Ramachandra writes:
> GIT_REFLOG_ACTION is an environment variable specifying the reflog
> message to write after an action is completed. Other commands including
> merge, reset, and commit respect it.
>
> This incidentally fixes a bug in t/checkout-last. You can now expect
>
> $ git checkout -
>
> to work fine after an interactive rebase.
>
> Signed-off-by: Ramkumar Ramachandra
> ---
> builtin/checkout.c | 11 ---
> t/t2012-checkout-last.sh | 2 +-
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index f5b50e5..1e2af85 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -587,7 +587,7 @@ static void update_refs_for_switch(const struct
> checkout_opts *opts,
> struct branch_info *new)
> {
> struct strbuf msg = STRBUF_INIT;
> - const char *old_desc;
> + const char *old_desc, *reflog_msg;
> if (opts->new_branch) {
> if (opts->new_orphan_branch) {
> if (opts->new_branch_log && !log_all_ref_updates) {
> @@ -620,8 +620,13 @@ static void update_refs_for_switch(const struct
> checkout_opts *opts,
> old_desc = old->name;
> if (!old_desc && old->commit)
> old_desc = sha1_to_hex(old->commit->object.sha1);
> - strbuf_addf(&msg, "checkout: moving from %s to %s",
> - old_desc ? old_desc : "(invalid)", new->name);
> +
> + reflog_msg = getenv("GIT_REFLOG_ACTION");
> + if (!reflog_msg)
> + strbuf_addf(&msg, "checkout: moving from %s to %s",
> + old_desc ? old_desc : "(invalid)", new->name);
> + else
> + strbuf_insert(&msg, 0, reflog_msg, strlen(reflog_msg));
Looks very sensible; we may need to audit programs that set and
export REFLOG_ACTION to make sure they do not do so incorrectly,
which wouldn't have mattered if they called "checkout" but now it
would with this fix, though.
> if (!strcmp(new->name, "HEAD") && !new->path && !opts->force_detach) {
> /* Nothing to do. */
> diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
> index 5729487..ab80da7 100755
> --- a/t/t2012-checkout-last.sh
> +++ b/t/t2012-checkout-last.sh
> @@ -116,7 +116,7 @@ test_expect_success 'master...' '
> test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify
> master^)"
> '
>
> -test_expect_failure '"checkout -" works after an interactive rebase' '
> +test_expect_success '"checkout -" works after an interactive rebase' '
> git checkout master &&
> git checkout other &&
> git rebase -i master &&
--
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 1/3] t/checkout-last: checkout - doesn't work after rebase -i
Ramkumar Ramachandra writes: > The following command > > $ git checkout - > > does not work as expected after a 'git rebase -i'. > > Add a failing test documenting this bug. > > Signed-off-by: Ramkumar Ramachandra > --- > t/t2012-checkout-last.sh | 8 > 1 file changed, 8 insertions(+) > > diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh > index b44de9d..5729487 100755 > --- a/t/t2012-checkout-last.sh > +++ b/t/t2012-checkout-last.sh > @@ -116,4 +116,12 @@ test_expect_success 'master...' ' > test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify > master^)" > ' > > +test_expect_failure '"checkout -" works after an interactive rebase' ' > + git checkout master && > + git checkout other && > + git rebase -i master && > + git checkout - && > + test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master" > +' Hmph, you were on "other" and then ran "rebase -i" to rebase it. When you are done, you are on "other". You want to go back to the one before you checked out the branch you started your "rebase -i" on, which is "master". OK, the expectation makes sense to me. These four are all valid ways to spell the "rebase -i master" step. git rebase -i master git rebase -i --onto master master git rebase -i master other git rebase -i --onto master master other and I think it is sensible to expect (1) they all behave the same way; or (2) the first two behave the same, but the latter two explicitly checks out 'other' by giving the last argument. If you are not on 'other' when you run the "rebase -i", "checkout -" should come back to the branch before 'other', i.e. the branch you started your "rebase -i" on. In other words, (2) would mean: git checkout master && git checkout -b naster && git rebase -i master other && # ends up on other test_string_equal "$(git symbolic-ref HEAD)" refs/heads/other && git checkout - && # we were on naster before we rebased 'other' test_string_equal "$(git symbolic-ref HEAD)" refs/heads/naster It is a bit unclear what the following should expect. git checkout master && git checkout other && git rebase -i master other && # ends up on other test_string_equal "$(git symbolic-ref HEAD)" refs/heads/other && git checkout - && # we are on ??? before we rebased other test_string_equal "$(git symbolic-ref HEAD)" refs/heads/??? One could argue that the "first checkout other and then rebase" done by rebase becomes a no-op and the user should be aware of that because rebase is started while on that other branch, in which case we should come back to 'master'. From consistency point of view, one could instead argue that we were on 'other' before we rebased it, in which case it may not be unreasonable for "checkout -" to come back to 'other'. I tend to prefer the former (i.e. go back to 'master') over the latter but not by a large margin. -- 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 1/2] status: introduce status.short to enable --short by default
Junio C Hamano writes: > test_expect_success '-c status.short=true == status -s' ' > test_config status.showUntrackedFile no && That's an option, but having status.showUntrackedFile set in a separate setup test makes the actual tests shorter. The setup test has no reason to fail, so I find it OK. -- 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 v2 3/4] sort-in-topological-order: use commit-queue
On Mon, Jun 10, 2013 at 12:27:31AM -0700, Junio C Hamano wrote: > > Around the same time, though, René wrote the linked-list merge sort that > > powers commit_list_sort_by_date. And topo-sort learned to do O(1) > > insertions into the unsorted list, and then one O(n log n) sort. > > Yes, but that only affects the "sort the work queue in date order" > before entering the main loop, and maintenance of work queue as we > dig along still is "find the place to put this in the date-order > sorted linked list", no? Ah, you're right. I was thinking that we saw all of the commits up front and then sorted. And we do, but we still keep a separate list in the work queue. So I think it may just be the case that "N" does not get very big here (the width of the graph), so log(N) versus (N) does not make a big difference. > I've been disturbed every time I saw the commit_list insertion > function that does a small allocation which will be freed fairly > often and have been wondering if we can rewrite it with custom slab > allocator, but not using linked list where we do not have to feels > like a better solution to that issue, and use of pqueue may be a > right direction to go in. Agreed. The only thing I'd worry about is that somebody cares about the order stability of same-time commits in the output. But I cannot think of a case where it is important (especially because the timestamps are subject to minor skew anyway, so it is not like you could even count on particular commits having equivalent timestamps). -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 v3 2/2] status:introduce status.branch to enable --branch by default
Jorge Juan Garcia Garcia
writes:
> Some people often run 'git status -b'.
> The config variable status.branch allows to set it by default.
>
> Signed-off-by: Jorge Juan Garcia Garcia
>
> Signed-off-by: Mathieu Lienard--Mayor
> Signed-off-by: Matthieu Moy
> ---
>
> Changes since v2:
> -removal of double quotes in test
>
> Documentation/config.txt |4
> builtin/commit.c |4
> t/t7508-status.sh| 27 +++
> 3 files changed, 35 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1983bf7..ecdcd6d 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2070,6 +2070,10 @@ 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.branch::
> + Set to true to enable --branch by default in linkgit:git-status[1].
> + The option --no-branch 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 287f1cb..f2b5d44 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1117,6 +1117,10 @@ static int git_status_config(const char *k, const char
> *v, void *cb)
> status_format = STATUS_FORMAT_SHORT;
> return 0;
> }
> + if (!strcmp(k, "status.branch")) {
> + s->show_branch = git_config_bool(k, v);
This one, unlike 1/2, acts correctly when status.branch is set to
no. Good.
The same comments as 1/2 apply to the test script additions in this
patch.
> + 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 d99ca9f..5e6df95 100755
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1366,6 +1366,33 @@ test_expect_success '"status.short=false" weaker than
> "-s"' '
> test_cmp actual expected_short
> '
>
> +test_expect_success '"status.branch=true" same as "-b"' '
> + git -c status.branch=true status -s >actual &&
> + git status -sb >expected_branch &&
> + test_cmp actual expected_branch
> +'
> +
> +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 &&
> + test_must_fail test_cmp actual expected_nobranch
> +'
> +
> +test_expect_success '"status.branch=true" weaker than "--no-branch"' '
> + git -c status.branch=true status -s --no-branch >actual &&
> + test_cmp actual expected_nobranch
> +'
> +
> +test_expect_success '"status.branch=false" same as "--no-branch"' '
> + git -c status.branch=false status -s >actual &&
> + test_cmp actual expected_nobranch
> +'
> +
> +test_expect_success '"status.branch=false" weaker than "-b"' '
> + git -c status.branch=false status -sb >actual &&
> + test_cmp actual expected_branch
> +'
> +
> test_expect_success 'Restore default test environment' '
> git config --unset status.showUntrackedFiles
> '
--
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 1/2] status: introduce status.short to enable --short by default
Jorge Juan Garcia Garcia
writes:
> 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
>
> Signed-off-by: Mathieu Lienard--Mayor
> Signed-off-by: Matthieu Moy
> ---
>
> Changes since v2:
> -removal of double quotes in test
> -use of git config --unset to clean up test environment
>
> Documentation/config.txt |4
> builtin/commit.c |5 +
> t/t7508-status.sh| 35 +++
> 3 files changed, 44 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..287f1cb 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1112,6 +1112,11 @@ 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;
And if the user has
[status]
short = no
in $GIT_DIR/config for this particular project, perhaps in order to
override a blanket setting
[status]
short
that is in $HOME/.gitconfig, what should happen?
Perhaps you need
if (git_config_bool(...))
status_format = STATUS_FORMAT_SHORT;
else
status_format = STATUS_FORMAT_NONE; /* default */
or something here?
> + 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..d99ca9f 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 actual expected_short
> +'
> +
> +test_expect_success '"status.short=true" different from "--no-short"' '
> + git status --no-short >expected_noshort &&
> + test_must_fail test_cmp actual expected_noshort
> +'
> +
> +test_expect_success '"status.short=true" weaker than "--no-short"' '
> + git -c status.short=true status --no-short >actual &&
> + test_cmp actual expected_noshort
> +'
> +
> +test_expect_success '"status.short=false" same as "--no-short"' '
> + git -c status.short=false status >actual &&
> + git status -s >expected_short &&
> + test_cmp actual expected_noshort
> +'
> +
> +test_expect_success '"status.short=false" weaker than "-s"' '
> + git -c status.short=false status -s >actual &&
> + test_cmp actual expected_short
> +'
> +
> +test_expect_success 'Restore default test environment' '
> + git config --unset status.showUntrackedFiles
> +'
A few observations.
* It is very good that you check not just positive cases that show
off how well this new feature works (i.e. status.short set
without command line override gives a short output) but also
negative cases that make sure the new feature does not kick in
when it should not. You test all four combinations, which is
good.
* If any of the first three fails, you may not have the correct
string in expected_short or expected_noshort when running later
tests that depend on them.
* Similarly, if the first one to set showUntrackedFiles fails, the
last one to --unset would also fail.
Perhaps limiting the number of tests that must pass (otherwise the
remainder becomes useless) by doing something like this is a better
alternative:
test_expect_success 'setup for status.short' '
git status --short >expected_short &&
git status --no-short >expected_noshort
'
test_expect_success '-c status.short=true == status -s' '
test_config status.showUntrackedFile no &&
test_config status.
Re: [PATCH v2 2/4] commit-queue: LIFO or priority queue of commits
On Mon, Jun 10, 2013 at 12:21:00AM -0700, Junio C Hamano wrote:
> > It may be worth looking again for other places to use this over
> > commit_list, but even the caller you are introducing here justifies its
> > presence.
>
> The next candidate is paint-down-to-common, probably.
Yeah, I don't think I looked at that at all last time (mostly because it
only large as the graph gets wide, which is typically acceptable for
us). But it should be easy to do.
> > Also, I wrote some basic tests to cover the priority queue as a unit. I
> > can rebase them on your commit if you are interested.
>
> It would be great.
Squashable patch is below.
> > Is it worth making this "struct commit *" a void pointer, and handling
> > arbitrary items in our priority queue? The compare function should be
> > the only thing that dereferences them.
> >
> > I do not have any non-commit priority queue use in mind, but I do not
> > think it adds any complexity in this case.
>
> I didn't either (and still I don't think of one), but I agree that
> the implementation can be reused for pq of any type, as long as it
> is a pointer to struct.
I converted this to a void pointer in my patch below, simply because it
makes it easier to write a test-queue that operates on ints. Due to
implicit casting, it should work for the most part without changing the
calling code unless you have a caller that does something like:
commit_queue_get(&q)->date
or similar. I didn't change the name, either. It may be silly to call it
"commit_queue" still since it is now more general. I simply called mine
"queue" (I wanted "pqueue", but that conflicted with globals defined by
OpenSSL; yours is a more general queue anyway, so maybe that is a good
name).
> >> + /* Bubble up the new one */
> >> + for (ix = queue->nr - 1; ix; ix = parent) {
> >> + parent = (ix - 1) / 2;
> >> + if (compare(queue->array[parent], queue->array[ix],
> >> + queue->cb_data) < 0)
> >> + break;
> >
> > In my implementation, I stopped on "compare() <= 0". It is late and my
> > mind is fuzzy, but I recall that heaps are never stable with respect to
> > insertion order, so I don't think it would matter.
>
> It would matter in the sense that we cannot replace linked-list, if
> the caller wants stability. It is more like "we cannot do anything
> about it" than "it would not matter".
Right. I meant "I do not think it matters if you do <= or < here, as we
are not stable anyway". Doing "<= 0" stops the heapify operation sooner,
though I doubt it matters in practice (it is not an algorithmic
complexity change, but just that you can sometimes quit early).
I think it is the same situation in your "push down", too, where you can
quit when the parent is equal to the largest child.
> We can make each queue element a pair of insertion counter>, and tiebreak using the insertion order, if the
> callers want the same stability as linked-list implementation, but
> I tend to think it really matters.
Yes, I think that is the usual solution.
Here's the patch with the tests, meant to be squashed into your 2/4. As
I mentioned above, you may want to further tweak the name, which would
require fixing up the rebase patches on top.
If you don't want to do the "s/struct commit/void/" change now, we can
probably just have test-queue stuff the ints into commit pointers.
The tests themselves are not extremely extensive, but at least let you
check that you implemented the heap correctly. :)
---
.gitignore | 1 +
Makefile | 1 +
commit-queue.c | 6 ++---
commit-queue.h | 8 +++---
t/t0009-queue.sh | 50 +++
test-queue.c | 39 +++
6 files changed, 98 insertions(+), 7 deletions(-)
diff --git a/.gitignore b/.gitignore
index 6669bf0..8670e6d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -193,6 +193,7 @@
/test-regex
/test-revision-walking
/test-run-command
+/test-queue
/test-sha1
/test-sigchain
/test-string-list
diff --git a/Makefile b/Makefile
index 3cf55e9..c957637 100644
--- a/Makefile
+++ b/Makefile
@@ -552,6 +552,7 @@ TEST_PROGRAMS_NEED_X += test-path-utils
TEST_PROGRAMS_NEED_X += test-mktemp
TEST_PROGRAMS_NEED_X += test-parse-options
TEST_PROGRAMS_NEED_X += test-path-utils
+TEST_PROGRAMS_NEED_X += test-queue
TEST_PROGRAMS_NEED_X += test-regex
TEST_PROGRAMS_NEED_X += test-revision-walking
TEST_PROGRAMS_NEED_X += test-run-command
diff --git a/commit-queue.c b/commit-queue.c
index 77d4b02..04acf23 100644
--- a/commit-queue.c
+++ b/commit-queue.c
@@ -10,7 +10,7 @@ void clear_commit_queue(struct commit_queue *queue)
queue->array = NULL;
}
-void commit_queue_put(struct commit_queue *queue, struct commit *commit)
+void commit_queue_put(struct commit_queue *queue, void *commit)
{
commit_compare_fn compare = queue->compare;
int ix, parent;
@@ -34,9 +34,9 @@ struct commit *commit_queue_get(struct commit_queue *queue)
}
}
-st
Re: Bad attitudes and problems in the Git community (was: Re: [PATCH 2/2] Move sequencer to builtin)
On Mon, Jun 10, 2013 at 9:58 AM, Felipe Contreras wrote: > On Mon, Jun 10, 2013 at 4:05 AM, Stefano Lattarini > wrote: > >>> You need two sides to have an argument. > >> I disagree. Unless you mean than, whenever a part behaves in a >> hostile and aggressive way, the other part should just silently >> knuckle under. > > You are wrong. If a bum in the street starts talking about you about > why you are going to hell, and you reply to him and argue. Who has the > fault of starting an argument? I'm not sure I follow the analogy. Are you the bum or the passer-by? Sorry, couldn't help it. :-) -- 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
Matthieu Moy wrote: > https://lkml.org/lkml/2012/4/12/434 > https://lkml.org/lkml/2012/4/15/112 We don't want things taken out of context now, do we? Follow up this thread [1], if you're interested in that discussion. I did clip out the quotes you chose on purpose, in the interest of presenting evidence in an unbiased manner. > I don't follow the lkml so maybe I've just been unlucky and Google > didn't show me an accurate sample, but arguing that your behavior is > welcome on the LKML seems weird. Are people criticizing his discussion style, tone, and demeanour, instead of focusing on the argument? [1]: http://thread.gmane.org/gmane.linux.kernel/1280458/focus=8675 -- 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 1/2] status: introduce status.short to enable --short by default
Matthieu Moy writes: > y...@ensimag.imag.fr writes: > >> To: y...@ensimag.imag.fr > > Common mistake, but you're not supposed to answer "y" when you're > prompted for an email ;-). Didn't we introduce safety against this in v1.7.12.1 and later? Is the new release taking more than 9 months to percolate, or are there still other codepaths that allow this to happen that we need to add further safety? -- 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
"Robin H. Johnson" writes: > On Mon, Jun 10, 2013 at 04:04:29PM +0200, Matthieu Moy wrote: >> Célestin Matte writes: >> >> > Le 10/06/2013 15:28, Ramkumar Ramachandra a écrit : >> >> 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. >> > >> > "Herself"? >> > Typo I guess :) >> >> Not necessarily. It's quite common in english to use "she" when the >> gender is not known. > Could you please use "themself" instead? I think "himself or herself" is the politically correct form ;-) But more seriously. 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. 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? 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? 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". I dunno. -- 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 1/2] rm: better error message on failure for multiple files
Junio C Hamano writes:
>> +{
>> +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",
>
> Is there an implication of having always 4 spaces here to l10n/i18n
> here? I am wondering if it should be _("\n%s").
I'd say this is just formatting and should be the same in every
languages, but I'm far from an expert in the domain. Maybe some
right-to-left languages would need this.
> test_expect_success 'rm files with different staged content' '
> cat >expect <<\-EOF &&
(that should be -\EOF, not \-EOF I think)
> (2) by using a dash '-' before the end-of-here-text marker, you can
> align the body of here text with a leading tab (HT).
This works because the list of files is aligned with spaces, but is
seems a bit fragile to me to use this -EOF on a text which uses
indentation. Anyway, I'm fine with both.
--
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 2/2] Move sequencer to builtin
Felipe Contreras writes: > It is not bad behavior. It is bad behavior *in your opinion*, And in essentially everyone else on this list, it seems. > an opinion that wouldn't be shared by other projects, like the Linux > kernel. Googling your name and LKML gives me this in the first page (addressed to you): https://lkml.org/lkml/2012/4/12/434 "I'm stupider for just reading your email. Go away." https://lkml.org/lkml/2012/4/15/112 "I'll make one more try at explaining to you, but then I'll just set my mail reader to ignore you, because judging by past performance (not just in this thread) you will just continue to argue." I don't follow the lkml so maybe I've just been unlucky and Google didn't show me an accurate sample, but arguing that your behavior is welcome on the LKML seems weird. -- 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 v3 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Please, don't top-post. Quote the part of the message you're replying to, and reply below. Benoît Person writes: > Well, I think next step would be to replace all those calls with > Git.pm `command`, `command_oneline` and `config``which take an array > of arguments after the "command". In the preview tool we use those but > I don't know if we will find the time to clean that up too in > git-remote-mediawiki :) . Agreed. run_git was written to avoid having to depend on Git.pm, but now that we do, we should do it the Git.pm way (although this is not a high priority for now). > Don't know though if it's better to mix that with this serie which is > mainly based on what perlcritic returns. If you go this way, I'd rather have it on top (i.e. a separate patch series). -- 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 2/3] test: improve rebase -q test
On Mon, Jun 10, 2013 at 08:56:58AM -0700, Junio C Hamano wrote:
> SZEDER Gábor writes:
>
> > On Sun, Jun 09, 2013 at 03:41:54PM -0500, Felipe Contreras wrote:
> >> There
> >> will not be a need for test_string_must_be_empty() just like there's
> >> no need for test_string_cmp().
> >
> > Actually, if there were a test_string_cmp(), that would be the test
> > helper function I used most often.
>
> Hmm, there indeed are quite a many "At this point, the variable's
> value must be this" in the test scripts. With things like this:
>
> t/t0002-gitfile.sh: test "$SHA" = "$(git rev-list HEAD)"
>
> we can go to the trash directory upon seeing a failure to run the
> command used on the RHS, but the value in $SHA is cumbersome to find
> out (either running it under sh -x or insert an extra echo before
> it), so such a helper function may be useful.
>
> Do you really need a general comparison ("does A sort before B") or
> just equality? If the latter, test_string_equal (or even
> string_equal) might be a better name for it.
Yeah, I need only equality. Or at least it would be nice to have.
My main motivation is that, like in your example, in the bash prompt
tests I only have to check a single line of output, but because of
debuggability I always did:
echo "(master)" >expected
__git_ps1 >actual
test_cmp expected actual
With such a helper function this could be reduced to a single line:
test_string_equal "(master)" "$(__git_ps1)"
without loss of functionality or debuggability, because in case of a
failure it would output something like this (bikesheddable, of
course):
Error:
expected: "(master)"
got: "((deadbeef...))"
And perhaps with a description as an optional third argument to help
identify the failed check if multiple such checks are done in a single
test, e.g. the test_rev_parse() helper in t/t1501-worktree.sh, 'setup:
helper for testing rev-parse', which could be shortened as:
test_string_equal "$1" "$(git rev-parse --is-bare-repository)" "bare"
test_string_equal "$2" "$(git rev-parse --is-inside-git-dir)" "gitdir"
test_string_equal "$3" "$(git rev-parse --is-inside-work-tree)" "worktree"
and if something goes wrong we'd get:
Error: worktree
expected: "true"
got: "false"
Perhaps I could find some time in the days ahead to give it a go.
Gábor
--
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 2/2] rm: introduce advice.rmHints to shorten messages
Mathieu Lienard--Mayor
writes:
> 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
> Signed-off-by: Jorge Juan Garcia Garcia
>
> Signed-off-by: Matthieu Moy
> ---
> Documentation/config.txt |3 +++
> advice.c |2 ++
> advice.h |1 +
> builtin/rm.c | 11 +++
> t/t3600-rm.sh| 29 +
> 5 files changed, 42 insertions(+), 4 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;
The handling of a new advice variable (i.e. definition, declaration
and reading from configuration) looks correct in this patch. Good
job.
> 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 07306eb..c991fe6 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -62,9 +62,11 @@ static int check_submodules_use_gitfiles(void)
>
> if (!submodule_uses_gitfile(name))
> errs = error(_("submodule '%s' (or one of its nested "
> +"submodules) uses a .git directory%s"),
> name,
> +advice_rm_hints
> +? "\n(use 'rm -rf' if you really want to
> remove "
> +"it including all of its history)"
> +: "");
The advice part is not subject to i18n?
> }
>
> return errs;
Interesting.
Is there a reason why this kind of errors are not collected together
into one "error message and then list of paths", like all the other
kinds of errors are done with print_eventual_error_files()?
> @@ -83,7 +85,8 @@ static void print_eventual_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);
> }
> }
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 10dd380..74f048c 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -707,6 +707,18 @@ EOF
> test_cmp 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 2>actual
> &&
> + test_cmp expect actual
> +'
Same comments as the ones for 1/2 applies to the tests in this 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 2/2] rm: introduce advice.rmHints to shorten messages
Le 2013-06-10 18:57, Junio C Hamano a écrit : Mathieu Liénard--Mayor writes: Please ignore this, manipulation error while in the git send-email command line. Here is what my mailbox looks like (the penultimate one with 252 lines is what I am responding to). R. [ 146: Mathieu Lienard--Mayor ] [PATCH 1/2] rm: better error messa R. [ 231: Mathieu Lienard--Mayor ] [PATCH 2/2] rm: introduce advice.r R. [ 157: Mathieu Lienard--Mayor ] [PATCH 2/2] rm: introduce advice.r R. [ 198: Mathieu Lienard--Mayor ] [PATCH v2 1/2] rm: better error me R. [ 157: Mathieu Lienard--Mayor ] [PATCH v2 2/2] rm: introduce advic . [ 153: Mathieu Lienard--Mayor ] [PATCH v3 2/2] rm: introduce advic . [ 214: Mathieu Lienard--Mayor ] [PATCH v3 1/2] rm: better error m . [ 214: Mathieu Lienard--Mayor ] [PATCH v3 1/2] rm: better error m . [ 153: Mathieu Lienard--Mayor ] [PATCH v3 2/2] rm: introduce advic R [ 33: Mathieu Liénard--Mayor ] Re: [PATCH 1/2] rm: better error m O [ 38: Mathieu Liénard--Mayor ] Re: [PATCH 2/2] rm: introduce advi R. [ 156: Mathieu Liénard--Mayor ] Re: [PATCH 1/2] rm: better error m R. [ 252: Mathieu Liénard--Mayor ] Re: [PATCH 2/2] rm: introduce advi . [ 84: Mathieu Liénard--Mayor ] Re: [PATCH v2 1/2] rm: better erro I am guessing that [v3 1/2] and [v3 2/2] are the final ones but it that is not the case please holler. Yes, [v3 1/2] and [v3 2/2] are the final ones. i'm sorry, i really don't know how i managed to create such a mess, i'm still not familiar with the send-email tool =/ -- 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: [IGNORE] Implement 'git rebase' in ruby
On Mon, Jun 10, 2013 at 2:48 AM, Ramkumar Ramachandra wrote: > Felipe Contreras wrote: >> git-rebase.rb | 2056 >> + >> 1 file changed, 2056 insertions(+) >> create mode 100755 git-rebase.rb > > I suggest putting this in contrib/ and cooking it. As usual, my > mantra is: let the patches decide what to do. I'll help review and > improve this soon. What would be the point? There's only so much this script could do. I was already relying on an improved 'git cherry-pick' that's never going to happen. I think it would be more interesting to fork Git, replace all the main builtin commands with Ruby scripts that access libgit.a, and copy the code to a libgit-ng library that has stuff that is worthy of being on a library until the scripts don't access libgit.a any more. All the time passing all tests in the test framework of course. Then there would be a truly useful and stable Git library, and we would have scripts that work on all platforms, including Windows, without any forks, and the commands would be more maintainable, and would gather the potential of so many Ruby developers, which if GitHub is any indication; a lot of them love Git. I think that's the only way forward, since the Git project doesn't wish to be improved. There's only one last patch series that I'll try to push to Git that I've been cooking for years. Sadly, I don't think it's going to land, despite it being clearly good, and a simple single patch doing the trick, and it would be immensely useful for our users, who have complained about that for years, and most Git developers have agreed it needs to be done. Yet, if I send it, it would not land. Perhaps it's time for me to create a pseudonym, and when you have to do that to land clearly good patches, you know something is *REALLY* wrong with the project. -- 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 22/28] git-remote-mediawiki: Modify strings for a better coding-style
Well, I think next step would be to replace all those calls with
Git.pm `command`, `command_oneline` and `config``which take an array
of arguments after the "command". In the preview tool we use those but
I don't know if we will find the time to clean that up too in
git-remote-mediawiki :) .
Don't know though if it's better to mix that with this serie which is
mainly based on what perlcritic returns.
On 10 June 2013 18:41, Junio C Hamano wrote:
> Matthieu Moy writes:
>
>> Célestin Matte writes:
>>
>>> @@ -1285,8 +1285,7 @@ sub get_mw_namespace_id {
>>> # Look at configuration file, if the record for that namespace
>>> is
>>> # already cached. Namespaces are stored in form:
>>> # "Name_of_namespace:Id_namespace", ex.: "File:6".
>>> -my @temp = split(/\n/, run_git("config --get-all remote."
>>> -. $remotename
>>> .".namespaceCache"));
>>> +my @temp = split(/\n/, run_git("config --get-all
>>> remote.${remotename}.namespaceCache"));
>>
>> I tend to prefer the former, as it avoids long lines (> 80 columns)
>
> But you split the name of a single variable across lines by doing so.
>
> You could split lines to make it a bit more readable.
>
> my @temp = split(/\n/,
> run_git("config --get-all
> remote.${remotename}.namespaceCache"));
>
> It still overflows the 80-column limit, but the "namespaceCache" is
> the only thing that overflows; not much harm is done.
>
> This is totally outside of the topic of "coding-style" series, but I
> would be more concerned about the use of ${remotename}, though. Has
> it (and in general the parameters to all calls to run_git()) been
> sanitized for shell metacharacters? If we had a variant of run_git
> that took an array of command line arguments given to git, you could
> do this
>
> my @temp = split(/\n/,
> run_git([qw(config --get-all),
> "remote.${remotename}.namespaceCache")]);
>
> which would be safer and easier to read.
>
>>> @@ -1339,8 +1338,7 @@ sub get_mw_namespace_id {
>>>
>>> # Store explicitely requested namespaces on disk
>>> if (!exists $cached_mw_namespace_id{$name}) {
>>> -run_git("config --add remote.". $remotename
>>> -.".namespaceCache \"". $name .":". $store_id ."\"");
>>> +run_git(qq(config --add remote.${remotename}.namespaceCache
>>> "${name}:${store_id}"));
>>
>> Same.
> --
> 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
--
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 1/2] rm: better error message on failure for multiple files
Mathieu Lienard--Mayor
writes:
> 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
> Signed-off-by: Jorge Juan Garcia Garcia
>
> Signed-off-by: Matthieu Moy
> ---
>
> Changes since v2:
> -couple typo in commit message and in code
> -rename and redefinition of the intermediate function
> -move the 4 "if(nr)" inside the function
>
> builtin/rm.c | 71 +---
> t/t3600-rm.sh | 67 +
> 2 files changed, 124 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 7b91d52..07306eb 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -70,6 +70,24 @@ static int check_submodules_use_gitfiles(void)
> return errs;
> }
>
> +static void print_eventual_error_files(struct string_list *files_list,
> +const char *main_msg,
> +const char *hints_msg,
> +int *errs)
Hrm, I do not see the point of "eventual" there, by the way. Are
there other kinds of error files?
> +{
> + 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",
Is there an implication of having always 4 spaces here to l10n/i18n
here? I am wondering if it should be _("\n%s").
> + files_list->items[i].string);
> + strbuf_addstr(&err_msg, hints_msg);
> + *errs = error("%s", err_msg.buf);
There needs a strbuf_release(&err_msg) somewhere before leaving this
scope to avoid leaking its buffer, no?
> + }
> +}
> +
> static int check_local_mod(unsigned char *head, int index_only)
> {
> /*
> @@ -82,6 +100,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 +194,49 @@ 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))
> + 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, "
> - "or -f to force removal)"), name);
> + string_list_append(&files_cached, name);
> if (local_changes) {
> if (S_ISGITLINK(ce->ce_mode) &&
> !submodule_uses_gitfile(name)) {
> + string_list_append(&files_submodule,
> +name);
> + } else {
> + string_list_append(&files_local, name);
> + }
The innermost if/else no longer needs braces. Also even though it
may push it slightly over 80-column, I think the files_submodule
side of string_list_append() is easier to read if it were on a
single line.
> + print_eventual_error_files(&files_staged,
> +_("the following files have staged "
> + "content different from both the"
> + "\nfile and the HEAD:"),
> +_("\n(use -f to force removal)"),
> +&errs);
Hmph. I wonder if we want to properly i18n plurals, depending on
the number of files, e.g.
print_error_files(&
Re: [PATCH 2/3] test: improve rebase -q test
On Mon, Jun 10, 2013 at 10:56 AM, Junio C Hamano wrote: > By the way, test_cmp() is a replacement for the "cmp" command and > that is why it does not have "file" in its name. That's an irrelevant implementation detail. But if you want to be driven the the implementation, call it test_zero(). -- 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: Bad attitudes and problems in the Git community (was: Re: [PATCH 2/2] Move sequencer to builtin)
On Mon, Jun 10, 2013 at 4:05 AM, Stefano Lattarini wrote: >> You need two sides to have an argument. > I disagree. Unless you mean than, whenever a part behaves in a > hostile and aggressive way, the other part should just silently > knuckle under. You are wrong. If a bum in the street starts talking about you about why you are going to hell, and you reply to him and argue. Who has the fault of starting an argument? Both. Maybe you have even more blame. -- 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 2/2] rm: introduce advice.rmHints to shorten messages
Mathieu Liénard--Mayor writes: > Please ignore this, manipulation error while in the git send-email > command line. Here is what my mailbox looks like (the penultimate one with 252 lines is what I am responding to). R. [ 146: Mathieu Lienard--Mayor ] [PATCH 1/2] rm: better error messa R. [ 231: Mathieu Lienard--Mayor ] [PATCH 2/2] rm: introduce advice.r R. [ 157: Mathieu Lienard--Mayor ] [PATCH 2/2] rm: introduce advice.r R. [ 198: Mathieu Lienard--Mayor ] [PATCH v2 1/2] rm: better error me R. [ 157: Mathieu Lienard--Mayor ] [PATCH v2 2/2] rm: introduce advic . [ 153: Mathieu Lienard--Mayor ] [PATCH v3 2/2] rm: introduce advic . [ 214: Mathieu Lienard--Mayor ] [PATCH v3 1/2] rm: better error m . [ 214: Mathieu Lienard--Mayor ] [PATCH v3 1/2] rm: better error m . [ 153: Mathieu Lienard--Mayor ] [PATCH v3 2/2] rm: introduce advic R [ 33: Mathieu Liénard--Mayor ] Re: [PATCH 1/2] rm: better error m O [ 38: Mathieu Liénard--Mayor ] Re: [PATCH 2/2] rm: introduce advi R. [ 156: Mathieu Liénard--Mayor ] Re: [PATCH 1/2] rm: better error m R. [ 252: Mathieu Liénard--Mayor ] Re: [PATCH 2/2] rm: introduce advi . [ 84: Mathieu Liénard--Mayor ] Re: [PATCH v2 1/2] rm: better erro I am guessing that [v3 1/2] and [v3 2/2] are the final ones but it that is not the case please holler. -- 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 Mon, Jun 10, 2013 at 11:53 AM, Felipe Contreras wrote: > On Mon, Jun 10, 2013 at 3:32 AM, Junio C Hamano wrote: >> E.g. >> convincing people that it is not worth their time interacting with >> you, especially when there are better things to do like tending to >> other topics, and you lose the chance to show that your patches are >> good when they indeed are (I don't even know if these patches in >> question are good, and I am not going to find out). > > You are hurting the Git project by doing that, and our users, > specially our Windows users. > > I thought you were a good maintainer. But apparently you would rather > listen to the people that only complain, rather than actual code, that > actually improves things. And this in fact has a name; *bias*. It is bad in any human endeavor, and in logic and argumentation, letting yourself be blinded by who is making the arguments, rather than the arguments themselves has a name; ad hominem. That is a mistake. -- 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
